On 2025-05-21 at 09:06:43 GMT, Tvrtko Ursulin wrote: > > On 20/05/2025 16:01, Joonas Lahtinen wrote: > > (+ Tvrtko, Rodrigo and Jani) > > > > Quoting Krzysztof Niemiec (2025-05-19 18:34:14) > > > Hi, > > > > > > 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. > > Agreed, no need to complicate with redundant access controls in the kernel. > > > 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. > > > > 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. > > Cgroup visibility for device memory exists in principle although i915 hasn't > been wired up to it. > > For system memory (integrated GPUs) there is some work in progress around > memcg accounting, although I am unsure if i915 would be automatically > covered or not. > > Also going a step back, from MangoHUDs point of view, I don't really see why > total GPU memory is very interesting? I would have thought it is more > interesting to know how much the monitored client is using, which is already > available via fdinfo. Total memory sounds like something which it could > leave to interpretation by the entity looking at the traces. (If the > monitored client is running alone then total_free =~ total - client, and if > it isn't running alone then it doesn't matter, no?) >
They use it to plot the VRAM usage so you have a rough idea of how much of the total the client is using. [1] [1] https://github.com/flightlessmango/MangoHud/blob/master/src/hud_elements.cpp#L1471-L1485 > Regards, > > Tvrtko > > > > 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 > > > _ >
