[RFC 0/2] Introduce a sysfs interface for lmem information

[RFC 0/2] Introduce a sysfs interface for lmem information

[Krzysztof Niemiec]

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. 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.

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
_

[RFC 1/2] drm/i915: Expose local memory information via sysfs

[Krzysztof Niemiec]

Introduce sysfs entries regarding basic local memory information
(unallocated memory and total memory, for both the entire region and the
CPU visible part). This simplifies how external tools might read this
information, which at the point of writing this patch is only accessible
via the i915_query() ioctl.

This change exposes that information to users without CAP_PERFMON.

This change was requested in [1] by a developer of one such external
tool, with the sysfs idea surfacing in a corresponding request for xe [2].

[1] https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14153
[2] https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/4861

Signed-off-by: Krzysztof Niemiec <krzysztof.niemiec@intel.com>
---
 drivers/gpu/drm/i915/i915_sysfs.c          |   6 ++
 drivers/gpu/drm/i915/intel_memory_region.c | 106 +++++++++++++++++++++
 drivers/gpu/drm/i915/intel_memory_region.h |   3 +
 3 files changed, 115 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index f936e8f1f129..048d6da2f6db 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -35,6 +35,8 @@
 #include "gt/intel_rps.h"
 #include "gt/sysfs_engines.h"
 
+#include "intel_memory_region.h"
+
 #include "i915_drv.h"
 #include "i915_sysfs.h"
 
@@ -182,6 +184,8 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv)
 
 	i915_gpu_error_sysfs_setup(dev_priv);
 
+	intel_memory_region_setup_sysfs(dev_priv);
+
 	intel_engines_add_sysfs(dev_priv);
 }
 
@@ -189,6 +193,8 @@ void i915_teardown_sysfs(struct drm_i915_private *dev_priv)
 {
 	struct device *kdev = dev_priv->drm.primary->kdev;
 
+	intel_memory_region_teardown_sysfs();
+
 	i915_gpu_error_sysfs_teardown(dev_priv);
 
 	device_remove_bin_file(kdev, &dpf_attrs_1);
diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c
index 59bd603e6deb..9558e300209b 100644
--- a/drivers/gpu/drm/i915/intel_memory_region.c
+++ b/drivers/gpu/drm/i915/intel_memory_region.c
@@ -3,14 +3,19 @@
  * Copyright © 2019 Intel Corporation
  */
 
+#include <linux/kobject.h>
 #include <linux/prandom.h>
+#include <linux/sysfs.h>
 
 #include <uapi/drm/i915_drm.h>
 
 #include "intel_memory_region.h"
 #include "i915_drv.h"
+#include "i915_sysfs.h"
 #include "i915_ttm_buddy_manager.h"
 
+static struct kobject *memory_info_dir;
+
 static const struct {
 	u16 class;
 	u16 instance;
@@ -423,6 +428,107 @@ void intel_memory_regions_driver_release(struct drm_i915_private *i915)
 	}
 }
 
+static ssize_t
+vram_total_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	struct device *dev = kobj_to_dev(kobj->parent);
+	struct intel_memory_region *mr;
+
+	mr = intel_memory_region_by_type(kdev_minor_to_i915(dev), INTEL_MEMORY_LOCAL);
+
+	return sysfs_emit(buf, "%llu\n", mr->total);
+}
+
+static const struct kobj_attribute vram_total_attr =
+__ATTR(vram_total, 0444, vram_total_show, NULL);
+
+static ssize_t
+vram_avail_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	struct device *dev = kobj_to_dev(kobj->parent);
+	struct intel_memory_region *mr;
+	u64 unallocated_size;
+	u64 dummy;
+
+	mr = intel_memory_region_by_type(kdev_minor_to_i915(dev), INTEL_MEMORY_LOCAL);
+	intel_memory_region_avail(mr, &unallocated_size, &dummy);
+
+	return sysfs_emit(buf, "%llu\n", unallocated_size);
+}
+
+static const struct kobj_attribute vram_avail_attr =
+__ATTR(vram_available, 0444, vram_avail_show, NULL);
+
+
+static ssize_t
+vram_total_visible_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	struct device *dev = kobj_to_dev(kobj->parent);
+	struct intel_memory_region *mr;
+
+	mr = intel_memory_region_by_type(kdev_minor_to_i915(dev), INTEL_MEMORY_LOCAL);
+
+	return sysfs_emit(buf, "%llu\n", resource_size(&mr->io));
+}
+
+static const struct kobj_attribute vram_total_visible_attr =
+__ATTR(vram_total_cpu_visible, 0444, vram_total_visible_show, NULL);
+
+static ssize_t
+vram_avail_visible_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	struct device *dev = kobj_to_dev(kobj->parent);
+	struct intel_memory_region *mr;
+	u64 unallocated_cpu_visible_size;
+	u64 dummy;
+
+	mr = intel_memory_region_by_type(kdev_minor_to_i915(dev), INTEL_MEMORY_LOCAL);
+	intel_memory_region_avail(mr, &dummy, &unallocated_cpu_visible_size);
+
+	return sysfs_emit(buf, "%llu\n", unallocated_cpu_visible_size);
+}
+
+static const struct kobj_attribute vram_avail_visible_attr =
+__ATTR(vram_available_cpu_visible, 0444, vram_avail_visible_show, NULL);
+
+int intel_memory_region_setup_sysfs(struct drm_i915_private *i915)
+{
+	static const struct attribute *const files[] = {
+		&vram_total_attr.attr,
+		&vram_avail_attr.attr,
+		&vram_total_visible_attr.attr,
+		&vram_avail_visible_attr.attr,
+		NULL
+	};
+	struct device *kdev = i915->drm.primary->kdev;
+	int err;
+
+	/* Skip this function completely if the system does not support lmem */
+	if(!intel_memory_region_by_type(i915, INTEL_MEMORY_LOCAL))
+		return 0;
+
+	memory_info_dir = kobject_create_and_add("memory_info", &kdev->kobj);
+	if (!memory_info_dir) {
+		drm_warn(&i915->drm, "Failed to create memory_info sysfs directory\n");
+		return -EAGAIN;
+	}
+
+	err = sysfs_create_files(memory_info_dir, files);
+	if (err) {
+		drm_warn(&i915->drm, "Failed to create memory info sysfs files: %d\n", err);
+		kobject_put(memory_info_dir);
+		return err;
+	}
+
+	return 0;
+}
+
+int intel_memory_region_teardown_sysfs(void)
+{
+	kobject_put(memory_info_dir);
+	return 0;
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftests/intel_memory_region.c"
 #include "selftests/mock_region.c"
diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h
index b3b75be9ced5..9838eca9344c 100644
--- a/drivers/gpu/drm/i915/intel_memory_region.h
+++ b/drivers/gpu/drm/i915/intel_memory_region.h
@@ -132,4 +132,7 @@ struct intel_memory_region *
 i915_gem_shmem_setup(struct drm_i915_private *i915,
 		     u16 type, u16 instance);
 
+int intel_memory_region_setup_sysfs(struct drm_i915_private *i915);
+int intel_memory_region_teardown_sysfs(void);
+
 #endif
-- 
2.45.2

[RFC 2/2] drm/i915: Add protections to sysfs local memory information

[Krzysztof Niemiec]

Introduce a CAP_PERFMON check when accessing sysfs entries related to
local memory information. Also introduce a intel_memory_info_paranoid
sysctl parameter, which allows the administrator to control whether the
check is enforced.

Exposing local memory information via sysfs, while convenient for users,
is stricly speaking an info leak. Ideally such information should be
guarded behind a capability check. In practice, this might be cumbersome
to deal with, especially on systems where root access is not a problem,
but for some reason applications don't want to be run as root (e.g.
gaming overlays)[1].

Adding a CAP_PERFMON check by default with a way to disable it should be
a nice compromise; this should prevent a full info leak, while a
conscious administrator might go for the convenience instead if a "leak"
is not really a problem on their system.

[1] https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14153

Signed-off-by: Krzysztof Niemiec <krzysztof.niemiec@intel.com>
---
 drivers/gpu/drm/i915/intel_memory_region.c | 38 +++++++++++++++++++---
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c
index 9558e300209b..4e39f76278fb 100644
--- a/drivers/gpu/drm/i915/intel_memory_region.c
+++ b/drivers/gpu/drm/i915/intel_memory_region.c
@@ -14,7 +14,10 @@
 #include "i915_sysfs.h"
 #include "i915_ttm_buddy_manager.h"
 
+static u32 intel_memory_info_paranoid = 1;
+
 static struct kobject *memory_info_dir;
+static struct ctl_table_header *memory_info_header;
 
 static const struct {
 	u16 class;
@@ -428,6 +431,18 @@ void intel_memory_regions_driver_release(struct drm_i915_private *i915)
 	}
 }
 
+static const struct ctl_table intel_memory_info_table[] = {
+	{
+		.procname	= "memory_info_paranoid",
+		.data		= &intel_memory_info_paranoid,
+		.maxlen		= sizeof(intel_memory_info_paranoid),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ONE,
+	},
+};
+
 static ssize_t
 vram_total_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
 {
@@ -436,7 +451,10 @@ vram_total_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
 
 	mr = intel_memory_region_by_type(kdev_minor_to_i915(dev), INTEL_MEMORY_LOCAL);
 
-	return sysfs_emit(buf, "%llu\n", mr->total);
+	if (perfmon_capable() || !intel_memory_info_paranoid)
+		return sysfs_emit(buf, "%llu\n", mr->total);
+
+	return sysfs_emit(buf, "0\n");
 }
 
 static const struct kobj_attribute vram_total_attr =
@@ -453,7 +471,10 @@ vram_avail_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
 	mr = intel_memory_region_by_type(kdev_minor_to_i915(dev), INTEL_MEMORY_LOCAL);
 	intel_memory_region_avail(mr, &unallocated_size, &dummy);
 
-	return sysfs_emit(buf, "%llu\n", unallocated_size);
+	if (perfmon_capable() || !intel_memory_info_paranoid)
+		return sysfs_emit(buf, "%llu\n", unallocated_size);
+
+	return sysfs_emit(buf, "0\n");
 }
 
 static const struct kobj_attribute vram_avail_attr =
@@ -468,7 +489,10 @@ vram_total_visible_show(struct kobject *kobj, struct kobj_attribute *attr, char
 
 	mr = intel_memory_region_by_type(kdev_minor_to_i915(dev), INTEL_MEMORY_LOCAL);
 
-	return sysfs_emit(buf, "%llu\n", resource_size(&mr->io));
+	if (perfmon_capable() || !intel_memory_info_paranoid)
+		return sysfs_emit(buf, "%llu\n", resource_size(&mr->io));
+
+	return sysfs_emit(buf, "0\n");
 }
 
 static const struct kobj_attribute vram_total_visible_attr =
@@ -485,7 +509,10 @@ vram_avail_visible_show(struct kobject *kobj, struct kobj_attribute *attr, char
 	mr = intel_memory_region_by_type(kdev_minor_to_i915(dev), INTEL_MEMORY_LOCAL);
 	intel_memory_region_avail(mr, &dummy, &unallocated_cpu_visible_size);
 
-	return sysfs_emit(buf, "%llu\n", unallocated_cpu_visible_size);
+	if (perfmon_capable() || !intel_memory_info_paranoid)
+		return sysfs_emit(buf, "%llu\n", unallocated_cpu_visible_size);
+
+	return sysfs_emit(buf, "0\n");
 }
 
 static const struct kobj_attribute vram_avail_visible_attr =
@@ -507,6 +534,8 @@ int intel_memory_region_setup_sysfs(struct drm_i915_private *i915)
 	if(!intel_memory_region_by_type(i915, INTEL_MEMORY_LOCAL))
 		return 0;
 
+	memory_info_header = register_sysctl("dev/i915", intel_memory_info_table);
+
 	memory_info_dir = kobject_create_and_add("memory_info", &kdev->kobj);
 	if (!memory_info_dir) {
 		drm_warn(&i915->drm, "Failed to create memory_info sysfs directory\n");
@@ -526,6 +555,7 @@ int intel_memory_region_setup_sysfs(struct drm_i915_private *i915)
 int intel_memory_region_teardown_sysfs(void)
 {
 	kobject_put(memory_info_dir);
+	unregister_sysctl_table(memory_info_header);
 	return 0;
 }
 
-- 
2.45.2

Re: [RFC 1/2] drm/i915: Expose local memory information via sysfs

[Jani Nikula]

On Mon, 19 May 2025, Krzysztof Niemiec <krzysztof.niemiec@intel.com> wrote:
> Introduce sysfs entries regarding basic local memory information
> (unallocated memory and total memory, for both the entire region and the
> CPU visible part). This simplifies how external tools might read this
> information, which at the point of writing this patch is only accessible
> via the i915_query() ioctl.
>
> This change exposes that information to users without CAP_PERFMON.
>
> This change was requested in [1] by a developer of one such external
> tool, with the sysfs idea surfacing in a corresponding request for xe [2].
>
> [1] https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14153
> [2] https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/4861
>
> Signed-off-by: Krzysztof Niemiec <krzysztof.niemiec@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_sysfs.c          |   6 ++
>  drivers/gpu/drm/i915/intel_memory_region.c | 106 +++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_memory_region.h |   3 +
>  3 files changed, 115 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index f936e8f1f129..048d6da2f6db 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -35,6 +35,8 @@
>  #include "gt/intel_rps.h"
>  #include "gt/sysfs_engines.h"
>  
> +#include "intel_memory_region.h"
> +
>  #include "i915_drv.h"
>  #include "i915_sysfs.h"
>  
> @@ -182,6 +184,8 @@ void i915_setup_sysfs(struct drm_i915_private *dev_priv)
>  
>  	i915_gpu_error_sysfs_setup(dev_priv);
>  
> +	intel_memory_region_setup_sysfs(dev_priv);
> +
>  	intel_engines_add_sysfs(dev_priv);
>  }
>  
> @@ -189,6 +193,8 @@ void i915_teardown_sysfs(struct drm_i915_private *dev_priv)
>  {
>  	struct device *kdev = dev_priv->drm.primary->kdev;
>  
> +	intel_memory_region_teardown_sysfs();
> +
>  	i915_gpu_error_sysfs_teardown(dev_priv);
>  
>  	device_remove_bin_file(kdev, &dpf_attrs_1);
> diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c
> index 59bd603e6deb..9558e300209b 100644
> --- a/drivers/gpu/drm/i915/intel_memory_region.c
> +++ b/drivers/gpu/drm/i915/intel_memory_region.c
> @@ -3,14 +3,19 @@
>   * Copyright © 2019 Intel Corporation
>   */
>  
> +#include <linux/kobject.h>
>  #include <linux/prandom.h>
> +#include <linux/sysfs.h>
>  
>  #include <uapi/drm/i915_drm.h>
>  
>  #include "intel_memory_region.h"
>  #include "i915_drv.h"
> +#include "i915_sysfs.h"
>  #include "i915_ttm_buddy_manager.h"
>  
> +static struct kobject *memory_info_dir;

This can't be per module.

BR,
Jani.


> +
>  static const struct {
>  	u16 class;
>  	u16 instance;
> @@ -423,6 +428,107 @@ void intel_memory_regions_driver_release(struct drm_i915_private *i915)
>  	}
>  }
>  
> +static ssize_t
> +vram_total_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> +	struct device *dev = kobj_to_dev(kobj->parent);
> +	struct intel_memory_region *mr;
> +
> +	mr = intel_memory_region_by_type(kdev_minor_to_i915(dev), INTEL_MEMORY_LOCAL);
> +
> +	return sysfs_emit(buf, "%llu\n", mr->total);
> +}
> +
> +static const struct kobj_attribute vram_total_attr =
> +__ATTR(vram_total, 0444, vram_total_show, NULL);
> +
> +static ssize_t
> +vram_avail_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> +	struct device *dev = kobj_to_dev(kobj->parent);
> +	struct intel_memory_region *mr;
> +	u64 unallocated_size;
> +	u64 dummy;
> +
> +	mr = intel_memory_region_by_type(kdev_minor_to_i915(dev), INTEL_MEMORY_LOCAL);
> +	intel_memory_region_avail(mr, &unallocated_size, &dummy);
> +
> +	return sysfs_emit(buf, "%llu\n", unallocated_size);
> +}
> +
> +static const struct kobj_attribute vram_avail_attr =
> +__ATTR(vram_available, 0444, vram_avail_show, NULL);
> +
> +
> +static ssize_t
> +vram_total_visible_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> +	struct device *dev = kobj_to_dev(kobj->parent);
> +	struct intel_memory_region *mr;
> +
> +	mr = intel_memory_region_by_type(kdev_minor_to_i915(dev), INTEL_MEMORY_LOCAL);
> +
> +	return sysfs_emit(buf, "%llu\n", resource_size(&mr->io));
> +}
> +
> +static const struct kobj_attribute vram_total_visible_attr =
> +__ATTR(vram_total_cpu_visible, 0444, vram_total_visible_show, NULL);
> +
> +static ssize_t
> +vram_avail_visible_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> +	struct device *dev = kobj_to_dev(kobj->parent);
> +	struct intel_memory_region *mr;
> +	u64 unallocated_cpu_visible_size;
> +	u64 dummy;
> +
> +	mr = intel_memory_region_by_type(kdev_minor_to_i915(dev), INTEL_MEMORY_LOCAL);
> +	intel_memory_region_avail(mr, &dummy, &unallocated_cpu_visible_size);
> +
> +	return sysfs_emit(buf, "%llu\n", unallocated_cpu_visible_size);
> +}
> +
> +static const struct kobj_attribute vram_avail_visible_attr =
> +__ATTR(vram_available_cpu_visible, 0444, vram_avail_visible_show, NULL);
> +
> +int intel_memory_region_setup_sysfs(struct drm_i915_private *i915)
> +{
> +	static const struct attribute *const files[] = {
> +		&vram_total_attr.attr,
> +		&vram_avail_attr.attr,
> +		&vram_total_visible_attr.attr,
> +		&vram_avail_visible_attr.attr,
> +		NULL
> +	};
> +	struct device *kdev = i915->drm.primary->kdev;
> +	int err;
> +
> +	/* Skip this function completely if the system does not support lmem */
> +	if(!intel_memory_region_by_type(i915, INTEL_MEMORY_LOCAL))
> +		return 0;
> +
> +	memory_info_dir = kobject_create_and_add("memory_info", &kdev->kobj);
> +	if (!memory_info_dir) {
> +		drm_warn(&i915->drm, "Failed to create memory_info sysfs directory\n");
> +		return -EAGAIN;
> +	}
> +
> +	err = sysfs_create_files(memory_info_dir, files);
> +	if (err) {
> +		drm_warn(&i915->drm, "Failed to create memory info sysfs files: %d\n", err);
> +		kobject_put(memory_info_dir);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +int intel_memory_region_teardown_sysfs(void)
> +{
> +	kobject_put(memory_info_dir);
> +	return 0;
> +}
> +
>  #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
>  #include "selftests/intel_memory_region.c"
>  #include "selftests/mock_region.c"
> diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h
> index b3b75be9ced5..9838eca9344c 100644
> --- a/drivers/gpu/drm/i915/intel_memory_region.h
> +++ b/drivers/gpu/drm/i915/intel_memory_region.h
> @@ -132,4 +132,7 @@ struct intel_memory_region *
>  i915_gem_shmem_setup(struct drm_i915_private *i915,
>  		     u16 type, u16 instance);
>  
> +int intel_memory_region_setup_sysfs(struct drm_i915_private *i915);
> +int intel_memory_region_teardown_sysfs(void);
> +
>  #endif

-- 
Jani Nikula, Intel

✗ Fi.CI.CHECKPATCH: warning for Introduce a sysfs interface for lmem information

[Patchwork]

== Series Details ==

Series: Introduce a sysfs interface for lmem information
URL   : https://patchwork.freedesktop.org/series/149193/
State : warning

== Summary ==

Error: dim checkpatch failed
91c2eb3567a7 drm/i915: Expose local memory information via sysfs
-:112: CHECK:LINE_SPACING: Please don't use multiple blank lines
#112: FILE: drivers/gpu/drm/i915/intel_memory_region.c:462:
+
+

-:157: ERROR:SPACING: space required before the open parenthesis '('
#157: FILE: drivers/gpu/drm/i915/intel_memory_region.c:507:
+	if(!intel_memory_region_by_type(i915, INTEL_MEMORY_LOCAL))

total: 1 errors, 0 warnings, 1 checks, 157 lines checked
19ba8dc0e159 drm/i915: Add protections to sysfs local memory information

✗ Fi.CI.SPARSE: warning for Introduce a sysfs interface for lmem information

[Patchwork]

== Series Details ==

Series: Introduce a sysfs interface for lmem information
URL   : https://patchwork.freedesktop.org/series/149193/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

✗ i915.CI.BAT: failure for Introduce a sysfs interface for lmem information

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 3642 bytes --]

== Series Details ==

Series: Introduce a sysfs interface for lmem information
URL   : https://patchwork.freedesktop.org/series/149193/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_16566 -> Patchwork_149193v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_149193v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_149193v1, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_149193v1/index.html

Participating hosts (45 -> 44)
------------------------------

  Missing    (1): fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_149193v1:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_exec_fence@basic-await@bcs0:
    - bat-twl-2:          [PASS][1] -> [FAIL][2] +1 other test fail
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16566/bat-twl-2/igt@gem_exec_fence@basic-await@bcs0.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_149193v1/bat-twl-2/igt@gem_exec_fence@basic-await@bcs0.html

  
Known issues
------------

  Here are the changes found in Patchwork_149193v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live:
    - bat-mtlp-8:         [PASS][3] -> [DMESG-FAIL][4] ([i915#12061]) +1 other test dmesg-fail
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16566/bat-mtlp-8/igt@i915_selftest@live.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_149193v1/bat-mtlp-8/igt@i915_selftest@live.html

  * igt@i915_selftest@live@workarounds:
    - bat-arlh-3:         [PASS][5] -> [DMESG-FAIL][6] ([i915#12061]) +1 other test dmesg-fail
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16566/bat-arlh-3/igt@i915_selftest@live@workarounds.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_149193v1/bat-arlh-3/igt@i915_selftest@live@workarounds.html

  
#### Warnings ####

  * igt@i915_selftest@live:
    - bat-atsm-1:         [DMESG-FAIL][7] ([i915#12061] / [i915#13929]) -> [DMESG-FAIL][8] ([i915#12061] / [i915#14204])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16566/bat-atsm-1/igt@i915_selftest@live.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_149193v1/bat-atsm-1/igt@i915_selftest@live.html

  * igt@i915_selftest@live@mman:
    - bat-atsm-1:         [DMESG-FAIL][9] ([i915#13929]) -> [DMESG-FAIL][10] ([i915#14204])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16566/bat-atsm-1/igt@i915_selftest@live@mman.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_149193v1/bat-atsm-1/igt@i915_selftest@live@mman.html

  
  [i915#12061]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12061
  [i915#13929]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13929
  [i915#14204]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/14204


Build changes
-------------

  * Linux: CI_DRM_16566 -> Patchwork_149193v1

  CI-20190529: 20190529
  CI_DRM_16566: 0225cf416edcc20d3559530295550235cd406e3c @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_8367: 8367
  Patchwork_149193v1: 0225cf416edcc20d3559530295550235cd406e3c @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_149193v1/index.html

[-- Attachment #2: Type: text/html, Size: 4739 bytes --]

Re: [RFC 1/2] drm/i915: Expose local memory information via sysfs

[kernel test robot]

Hi Krzysztof,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on drm-intel/for-linux-next-fixes drm-tip/drm-tip linus/master v6.15-rc7 next-20250516]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Krzysztof-Niemiec/drm-i915-Expose-local-memory-information-via-sysfs/20250519-233529
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
patch link:    https://lore.kernel.org/r/20250519153418.44543-2-krzysztof.niemiec%40intel.com
patch subject: [RFC 1/2] drm/i915: Expose local memory information via sysfs
config: i386-buildonly-randconfig-003-20250519 (https://download.01.org/0day-ci/archive/20250520/202505200118.s2XVIwkW-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250520/202505200118.s2XVIwkW-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505200118.s2XVIwkW-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/i915/intel_memory_region.c: In function 'vram_total_show':
>> drivers/gpu/drm/i915/intel_memory_region.c:439:36: warning: format '%llu' expects argument of type 'long long unsigned int', but argument 3 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
     439 |         return sysfs_emit(buf, "%llu\n", mr->total);
         |                                 ~~~^     ~~~~~~~~~
         |                                    |       |
         |                                    |       resource_size_t {aka unsigned int}
         |                                    long long unsigned int
         |                                 %u
   drivers/gpu/drm/i915/intel_memory_region.c: In function 'vram_total_visible_show':
   drivers/gpu/drm/i915/intel_memory_region.c:471:36: warning: format '%llu' expects argument of type 'long long unsigned int', but argument 3 has type 'resource_size_t' {aka 'unsigned int'} [-Wformat=]
     471 |         return sysfs_emit(buf, "%llu\n", resource_size(&mr->io));
         |                                 ~~~^     ~~~~~~~~~~~~~~~~~~~~~~
         |                                    |     |
         |                                    |     resource_size_t {aka unsigned int}
         |                                    long long unsigned int
         |                                 %u


vim +439 drivers/gpu/drm/i915/intel_memory_region.c

   430	
   431	static ssize_t
   432	vram_total_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
   433	{
   434		struct device *dev = kobj_to_dev(kobj->parent);
   435		struct intel_memory_region *mr;
   436	
   437		mr = intel_memory_region_by_type(kdev_minor_to_i915(dev), INTEL_MEMORY_LOCAL);
   438	
 > 439		return sysfs_emit(buf, "%llu\n", mr->total);
   440	}
   441	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [RFC 1/2] drm/i915: Expose local memory information via sysfs

[Krzysztof Karas]

Hi Krzysztof,

[...]

> +static ssize_t
> +vram_total_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> +	struct device *dev = kobj_to_dev(kobj->parent);
> +	struct intel_memory_region *mr;
> +
> +	mr = intel_memory_region_by_type(kdev_minor_to_i915(dev), INTEL_MEMORY_LOCAL);
intel_memory_region_by_type() may return a NULL, which would
cause NULLPTR dereference on emit here and in other places
where you call this function.

> +
> +	return sysfs_emit(buf, "%llu\n", mr->total);
> +}
> +
> +static const struct kobj_attribute vram_total_attr =
> +__ATTR(vram_total, 0444, vram_total_show, NULL);
> +
> +static ssize_t
> +vram_avail_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> +	struct device *dev = kobj_to_dev(kobj->parent);
> +	struct intel_memory_region *mr;
> +	u64 unallocated_size;
> +	u64 dummy;
> +
> +	mr = intel_memory_region_by_type(kdev_minor_to_i915(dev), INTEL_MEMORY_LOCAL);
> +	intel_memory_region_avail(mr, &unallocated_size, &dummy);
> +
> +	return sysfs_emit(buf, "%llu\n", unallocated_size);
> +}
> +
> +static const struct kobj_attribute vram_avail_attr =
> +__ATTR(vram_available, 0444, vram_avail_show, NULL);
> +
> +
> +static ssize_t
> +vram_total_visible_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> +	struct device *dev = kobj_to_dev(kobj->parent);
> +	struct intel_memory_region *mr;
> +
> +	mr = intel_memory_region_by_type(kdev_minor_to_i915(dev), INTEL_MEMORY_LOCAL);
> +
> +	return sysfs_emit(buf, "%llu\n", resource_size(&mr->io));
> +}
> +
> +static const struct kobj_attribute vram_total_visible_attr =
> +__ATTR(vram_total_cpu_visible, 0444, vram_total_visible_show, NULL);
> +
> +static ssize_t
> +vram_avail_visible_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> +	struct device *dev = kobj_to_dev(kobj->parent);
> +	struct intel_memory_region *mr;
> +	u64 unallocated_cpu_visible_size;
> +	u64 dummy;
> +
> +	mr = intel_memory_region_by_type(kdev_minor_to_i915(dev), INTEL_MEMORY_LOCAL);
> +	intel_memory_region_avail(mr, &dummy, &unallocated_cpu_visible_size);
> +
> +	return sysfs_emit(buf, "%llu\n", unallocated_cpu_visible_size);
> +}
> +
> +static const struct kobj_attribute vram_avail_visible_attr =
> +__ATTR(vram_available_cpu_visible, 0444, vram_avail_visible_show, NULL);
> +
> +int intel_memory_region_setup_sysfs(struct drm_i915_private *i915)
> +{
> +	static const struct attribute *const files[] = {
> +		&vram_total_attr.attr,
> +		&vram_avail_attr.attr,
> +		&vram_total_visible_attr.attr,
> +		&vram_avail_visible_attr.attr,
> +		NULL
> +	};
> +	struct device *kdev = i915->drm.primary->kdev;
> +	int err;
> +
> +	/* Skip this function completely if the system does not support lmem */
> +	if(!intel_memory_region_by_type(i915, INTEL_MEMORY_LOCAL))
> +		return 0;
> +
> +	memory_info_dir = kobject_create_and_add("memory_info", &kdev->kobj);
> +	if (!memory_info_dir) {
> +		drm_warn(&i915->drm, "Failed to create memory_info sysfs directory\n");
> +		return -EAGAIN;
Maybe ENOMEM would be better? EAGAIN suggests that we have some
confidence that retrying this call will yield a different result.

[...]

Best Regards,
Krzysztof

Re: [RFC 0/2] Introduce a sysfs interface for lmem information

[Krzysztof Karas]

Hi Krzysztof,

> 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. 
"So far the only way to do this was via i915_query ioctl. This
is slightly less handy than sysfs for external users."
->
"So far the only way to do this was via i915_query ioctl, which
is slightly less handy than sysfs for external users."
- otherwise it might seem you are relating to the "sysfs" approach.

Best Regards,
Krzysztof

Re: [RFC 2/2] drm/i915: Add protections to sysfs local memory information

[Krzysztof Karas]

Hi Krzysztof,

> Introduce a CAP_PERFMON check when accessing sysfs entries related to
> local memory information. Also introduce a intel_memory_info_paranoid
> sysctl parameter, which allows the administrator to control whether the
> check is enforced.
If we decide that this patch is neede, I think we should squash
it with the first one to introduce a mechanism that can already
be secured.

[...]
>  
> +static u32 intel_memory_info_paranoid = 1;
Maybe change that to "intel_memory_info_restrictive"?
"Paranoid" relates to extreme fearfulness/paranoia/anxiety,
which might seem a bit over the top :)

Best Regards,
Krzysztof

Re: [RFC 0/2] Introduce a sysfs interface for lmem information

[Joonas Lahtinen]

(+ 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.

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.

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
> _

Re: [RFC 0/2] Introduce a sysfs interface for lmem information

[Andi Shyti]

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.

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
> > _

Re: [RFC 0/2] Introduce a sysfs interface for lmem information

[Tvrtko Ursulin]

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?)

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
>> _

Re: [RFC 0/2] Introduce a sysfs interface for lmem information

[Krzysztof Niemiec]

Hi,

On 2025-05-20 at 18:01:12 GMT, 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.
> 

This is something I missed, my bad. That is of course the better way to
handle the access control.

> 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.
> 

This is in fact mostly a matter of convenience, as for the usecase under
consideration here (mangohud) it can be assumed that root can be
obtained anyway. It would just be MUCH more convenient for users of the
driver to just chmod the appropriate files and go, instead of setting up
whole other binary just to invoke the ioctl and parse it, and then have
it do IPC with the main binary that can't be run as root. Just way less
hoops to go through. We could just tell userspace to "deal with it", but
then I don't know why can't we just play nice. Is this kind of change
too intrusive for us to warrant the userspace gain?

And it's true that reporting this via sysfs is redundant, but then, for
example, engine information is also available via both sysfs and
i915_query. Is there a particular reason for it to be the case for
engine info that can't be applied to memory info? Access control
notwithstanding, as it can be handled by file permissions.

Thanks
Krzysztof

> 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
> > _

Re: [RFC 0/2] Introduce a sysfs interface for lmem information

[Krzysztof Niemiec]

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
> > > _

Re: [RFC 0/2] Introduce a sysfs interface for lmem information

[Krzysztof Niemiec]

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
> > > _
> 

Re: [RFC 0/2] Introduce a sysfs interface for lmem information

[Tvrtko Ursulin]

On 21/05/2025 18:10, Krzysztof Niemiec wrote:
> 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

I get what it does, I just don't see the huge value in plotting the 
total. Nevertheless it is available from the memory query, that part is 
not privileged.

If the query is too cumbersome, for some reason, I could be convinced 
for a total in sysfs but the rest looks too much.

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
>>>> _
>>