Hi, On 2025-05-20 at 19:46:48 GMT, Andi Shyti wrote: > Hi, > > On Tue, May 20, 2025 at 06:01:12PM +0300, Joonas Lahtinen wrote: > > Quoting Krzysztof Niemiec (2025-05-19 18:34:14) > > > This series introduces a way for applications to read local memory > > > information via files in the sysfs. So far the only way to do this was > > > via i915_query ioctl. This is slightly less handy than sysfs for > > > external users. Additionally, the ioctl has a capability check which > > > limits which users of a system might use it to get information. > > > > > > The goals of this series are: > > > > > > 1) Introduce a simpler interface to access lmem information, > > > 2) Lift the CAP_PERFMON check on that information, OR provide > > > the administrator with a way to optionally lift it. > > > > > > The first patch introduces the general mechanism without protections. > > > This will effectively lift a capability check on obtaining the memory > > > information. The second patch introduces that check back inside the > > > _show() functions, but also adds a sysctl parameter allowing to override > > > the checks, if an administrator so decides. > > > > > > I'm sending this as RFC because I have a feeling that there's no > > > consensus whether memory information exposed in the patch should be > > > protected or not. Showing it to any user is strictly speaking an info > > > leak, but the severity thereof might be considered not that high, so I'd > > > rather leave it up to discussion first. > > > > > > If we decide for lifting the check, the first patch is sufficient. > > > > Nack on that. > > > > CPU memory footprint and GPU memory footprint have a very different > > nature. This was discussed to quite a length, please refer to mailing > > list archives. > > > > > If we > > > decide against it, the second patch protects the information by default, > > > but with a way to expose it as a conscious decision of the admin. I find > > > it a decent compromise. > > > > No need for the added complexity if we were to add a sysfs. > > > > If a sysfs is added, it can be made root readable by default but system > > admin is free to chown or chmod the file for more relaxed access. Back > > in the original discussion time, this was omitted for lack of users. > > > > Even now, userspace/sysadmin could already essentially use setuid helper > > process that will only report the memory statistics. > > > > So I'm not really fully convinced this is needed at all. > > yeah! What is the real use case? Who is the userspace client? > > There are already ways to read out the GPU memory footprint so > that we need to know whether we need for another uAPI. >
I'm sorry if I wasn't clear enough in the cover letter; client is MangoHUD[1]. It's a popular overlay for benchmarking games. They reached out to us because they have no convenient way to read total memory information. [1] https://github.com/flightlessmango/MangoHud Thanks Krzysztof > Andi > > > And if it is to be added for the convenience of usersppace, it should > > probably then be considered to be a standard interface across DRM drivers > > ala fdinfo or cgroups. > > > > Regards, Joonas > > > > > > > > This change has been requested in these parallel issues for i915 and Xe: > > > > > > https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14153 > > > https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/4861 > > > > > > Thanks > > > Krzysztof > > > > > > Krzysztof Niemiec (2): > > > drm/i915: Expose local memory information via sysfs > > > drm/i915: Add protections to sysfs local memory information > > > > > > drivers/gpu/drm/i915/i915_sysfs.c | 6 + > > > drivers/gpu/drm/i915/intel_memory_region.c | 136 +++++++++++++++++++++ > > > drivers/gpu/drm/i915/intel_memory_region.h | 3 + > > > 3 files changed, 145 insertions(+) > > > > > > -- > > > 2.45.2 > > > _
