[v4,5/6] drm: Add fdinfo memory stats

Commit Message

Rob Clark April 12, 2023, 10:42 p.m. UTC

From: Rob Clark <[e-mail]>

Add support to dump GEM stats to fdinfo.

v2: Fix typos, change size units to match docs, use div_u64
v3: Do it in core
v4: more kerneldoc

Signed-off-by: Rob Clark <[e-mail]>
Reviewed-by: Emil Velikov <[e-mail]>
Reviewed-by: Daniel Vetter <[e-mail]>
---
 Documentation/gpu/drm-usage-stats.rst | 21 ++++++++
 drivers/gpu/drm/drm_file.c            | 76 +++++++++++++++++++++++++++
 include/drm/drm_file.h                |  1 +
 include/drm/drm_gem.h                 | 30 +++++++++++
 4 files changed, 128 insertions(+)

Comments

Emil Velikov April 21, 2023, 10:26 a.m. UTC | #1

On Wed, 12 Apr 2023 at 23:43, Rob Clark <[e-mail]> wrote:

> +/**
> + * enum drm_gem_object_status - bitmask of object state for fdinfo reporting
> + * @DRM_GEM_OBJECT_RESIDENT: object is resident in memory (ie. not unpinned)
> + * @DRM_GEM_OBJECT_PURGEABLE: object marked as purgeable by userspace
> + *
> + * Bitmask of status used for fdinfo memory stats, see &drm_gem_object_funcs.status
> + * and drm_show_fdinfo().  Note that an object can DRM_GEM_OBJECT_PURGEABLE if
> + * it still active or not resident, in which case drm_show_fdinfo() will not

nit: s/can/can be/;s/if it still/if it is still/

> + * account for it as purgeable.  So drivers do not need to check if the buffer
> + * is idle and resident to return this bit.  (Ie. userspace can mark a buffer
> + * as purgeable even while it is still busy on the GPU.. it does not _actually_
> + * become puregeable until it becomes idle.  The status gem object func does

nit: s/puregeable/purgeable/


I think we want a similar note in the drm-usage-stats.rst file.

With the above the whole series is:
Reviewed-by: Emil Velikov <[e-mail]>

Fwiw: Keeping the i915 patch as part of this series would be great.
Sure i915_drm_client->id becomes dead code, but it's a piece one can
live with for a release or two. Then again it's not my call to make.

HTH
Emil

Tvrtko Ursulin April 21, 2023, 11:23 a.m. UTC | #2

On 21/04/2023 11:26, Emil Velikov wrote:
> On Wed, 12 Apr 2023 at 23:43, Rob Clark <[e-mail]> wrote:
> 
>> +/**
>> + * enum drm_gem_object_status - bitmask of object state for fdinfo reporting
>> + * @DRM_GEM_OBJECT_RESIDENT: object is resident in memory (ie. not unpinned)
>> + * @DRM_GEM_OBJECT_PURGEABLE: object marked as purgeable by userspace
>> + *
>> + * Bitmask of status used for fdinfo memory stats, see &drm_gem_object_funcs.status
>> + * and drm_show_fdinfo().  Note that an object can DRM_GEM_OBJECT_PURGEABLE if
>> + * it still active or not resident, in which case drm_show_fdinfo() will not
> 
> nit: s/can/can be/;s/if it still/if it is still/
> 
>> + * account for it as purgeable.  So drivers do not need to check if the buffer
>> + * is idle and resident to return this bit.  (Ie. userspace can mark a buffer
>> + * as purgeable even while it is still busy on the GPU.. it does not _actually_
>> + * become puregeable until it becomes idle.  The status gem object func does
> 
> nit: s/puregeable/purgeable/
> 
> 
> I think we want a similar note in the drm-usage-stats.rst file.
> 
> With the above the whole series is:
> Reviewed-by: Emil Velikov <[e-mail]>

Have you maybe noticed my slightly alternative proposal? (*) I am not a 
fan of putting this flavour of accounting into the core with no way to 
opt out. I think it leaves no option but to add more keys in the future 
for any driver which will not be happy with the core accounting.

*) https://patchwork.freedesktop.org/series/116581/

> Fwiw: Keeping the i915 patch as part of this series would be great.
> Sure i915_drm_client->id becomes dead code, but it's a piece one can
> live with for a release or two. Then again it's not my call to make.

Rob can take the i915 patch from my RFC too.

Regards,

Tvrtko

Emil Velikov April 21, 2023, 11:45 a.m. UTC | #3

On Fri, 21 Apr 2023 at 12:23, Tvrtko Ursulin
<[e-mail]> wrote:

> On 21/04/2023 11:26, Emil Velikov wrote:
> > On Wed, 12 Apr 2023 at 23:43, Rob Clark <[e-mail]> wrote:
> >
> >> +/**
> >> + * enum drm_gem_object_status - bitmask of object state for fdinfo reporting
> >> + * @DRM_GEM_OBJECT_RESIDENT: object is resident in memory (ie. not unpinned)
> >> + * @DRM_GEM_OBJECT_PURGEABLE: object marked as purgeable by userspace
> >> + *
> >> + * Bitmask of status used for fdinfo memory stats, see &drm_gem_object_funcs.status
> >> + * and drm_show_fdinfo().  Note that an object can DRM_GEM_OBJECT_PURGEABLE if
> >> + * it still active or not resident, in which case drm_show_fdinfo() will not
> >
> > nit: s/can/can be/;s/if it still/if it is still/
> >
> >> + * account for it as purgeable.  So drivers do not need to check if the buffer
> >> + * is idle and resident to return this bit.  (Ie. userspace can mark a buffer
> >> + * as purgeable even while it is still busy on the GPU.. it does not _actually_
> >> + * become puregeable until it becomes idle.  The status gem object func does
> >
> > nit: s/puregeable/purgeable/
> >
> >
> > I think we want a similar note in the drm-usage-stats.rst file.
> >
> > With the above the whole series is:
> > Reviewed-by: Emil Velikov <[e-mail]>
>
> Have you maybe noticed my slightly alternative proposal? (*) I am not a
> fan of putting this flavour of accounting into the core with no way to
> opt out. I think it leaves no option but to add more keys in the future
> for any driver which will not be happy with the core accounting.
>
> *) https://patchwork.freedesktop.org/series/116581/
>

Indeed I saw it. Not a fan of it, I'm afraid.

> > Fwiw: Keeping the i915 patch as part of this series would be great.
> > Sure i915_drm_client->id becomes dead code, but it's a piece one can
> > live with for a release or two. Then again it's not my call to make.
>
> Rob can take the i915 patch from my RFC too.
>

Indeed.

-Emil

Tvrtko Ursulin April 21, 2023, 11:59 a.m. UTC | #4

On 21/04/2023 12:45, Emil Velikov wrote:
> On Fri, 21 Apr 2023 at 12:23, Tvrtko Ursulin
> <[e-mail]> wrote:
> 
>> On 21/04/2023 11:26, Emil Velikov wrote:
>>> On Wed, 12 Apr 2023 at 23:43, Rob Clark <[e-mail]> wrote:
>>>
>>>> +/**
>>>> + * enum drm_gem_object_status - bitmask of object state for fdinfo reporting
>>>> + * @DRM_GEM_OBJECT_RESIDENT: object is resident in memory (ie. not unpinned)
>>>> + * @DRM_GEM_OBJECT_PURGEABLE: object marked as purgeable by userspace
>>>> + *
>>>> + * Bitmask of status used for fdinfo memory stats, see &drm_gem_object_funcs.status
>>>> + * and drm_show_fdinfo().  Note that an object can DRM_GEM_OBJECT_PURGEABLE if
>>>> + * it still active or not resident, in which case drm_show_fdinfo() will not
>>>
>>> nit: s/can/can be/;s/if it still/if it is still/
>>>
>>>> + * account for it as purgeable.  So drivers do not need to check if the buffer
>>>> + * is idle and resident to return this bit.  (Ie. userspace can mark a buffer
>>>> + * as purgeable even while it is still busy on the GPU.. it does not _actually_
>>>> + * become puregeable until it becomes idle.  The status gem object func does
>>>
>>> nit: s/puregeable/purgeable/
>>>
>>>
>>> I think we want a similar note in the drm-usage-stats.rst file.
>>>
>>> With the above the whole series is:
>>> Reviewed-by: Emil Velikov <[e-mail]>
>>
>> Have you maybe noticed my slightly alternative proposal? (*) I am not a
>> fan of putting this flavour of accounting into the core with no way to
>> opt out. I think it leaves no option but to add more keys in the future
>> for any driver which will not be happy with the core accounting.
>>
>> *) https://patchwork.freedesktop.org/series/116581/
>>
> 
> Indeed I saw it. Not a fan of it, I'm afraid.

Hard to guess the reasons. :)

Anyway, at a minimum I suggest that if the no opt out version has to go 
in, it is clearly documented drm-*-memory-* is *not* about the full 
memory use of the client, but about memory belonging to user visible 
buffer objects *only*. Possibly going as far as naming the keys as 
drm-user-bo-memory-... That way there is a way to implement proper 
drm-*-memory- in the future.

Regards,

Tvrtko

>>> Fwiw: Keeping the i915 patch as part of this series would be great.
>>> Sure i915_drm_client->id becomes dead code, but it's a piece one can
>>> live with for a release or two. Then again it's not my call to make.
>>
>> Rob can take the i915 patch from my RFC too.
>>
> 
> Indeed.
> 
> -Emil

Rob Clark April 21, 2023, 2:50 p.m. UTC | #5

On Fri, Apr 21, 2023 at 4:59 AM Tvrtko Ursulin
<[e-mail]> wrote:
>
>
> On 21/04/2023 12:45, Emil Velikov wrote:
> > On Fri, 21 Apr 2023 at 12:23, Tvrtko Ursulin
> > <[e-mail]> wrote:
> >
> >> On 21/04/2023 11:26, Emil Velikov wrote:
> >>> On Wed, 12 Apr 2023 at 23:43, Rob Clark <[e-mail]> wrote:
> >>>
> >>>> +/**
> >>>> + * enum drm_gem_object_status - bitmask of object state for fdinfo reporting
> >>>> + * @DRM_GEM_OBJECT_RESIDENT: object is resident in memory (ie. not unpinned)
> >>>> + * @DRM_GEM_OBJECT_PURGEABLE: object marked as purgeable by userspace
> >>>> + *
> >>>> + * Bitmask of status used for fdinfo memory stats, see &drm_gem_object_funcs.status
> >>>> + * and drm_show_fdinfo().  Note that an object can DRM_GEM_OBJECT_PURGEABLE if
> >>>> + * it still active or not resident, in which case drm_show_fdinfo() will not
> >>>
> >>> nit: s/can/can be/;s/if it still/if it is still/
> >>>
> >>>> + * account for it as purgeable.  So drivers do not need to check if the buffer
> >>>> + * is idle and resident to return this bit.  (Ie. userspace can mark a buffer
> >>>> + * as purgeable even while it is still busy on the GPU.. it does not _actually_
> >>>> + * become puregeable until it becomes idle.  The status gem object func does
> >>>
> >>> nit: s/puregeable/purgeable/
> >>>
> >>>
> >>> I think we want a similar note in the drm-usage-stats.rst file.
> >>>
> >>> With the above the whole series is:
> >>> Reviewed-by: Emil Velikov <[e-mail]>
> >>
> >> Have you maybe noticed my slightly alternative proposal? (*) I am not a
> >> fan of putting this flavour of accounting into the core with no way to
> >> opt out. I think it leaves no option but to add more keys in the future
> >> for any driver which will not be happy with the core accounting.
> >>
> >> *) https://patchwork.freedesktop.org/series/116581/
> >>
> >
> > Indeed I saw it. Not a fan of it, I'm afraid.
>
> Hard to guess the reasons. :)
>
> Anyway, at a minimum I suggest that if the no opt out version has to go
> in, it is clearly documented drm-*-memory-* is *not* about the full
> memory use of the client, but about memory belonging to user visible
> buffer objects *only*. Possibly going as far as naming the keys as
> drm-user-bo-memory-... That way there is a way to implement proper
> drm-*-memory- in the future.

I'll go back to the helper approach, just been distracted by a few
other balls in the air.. should hopefully get to it in the next couple
days

BR,
-R

> Regards,
>
> Tvrtko
>
> >>> Fwiw: Keeping the i915 patch as part of this series would be great.
> >>> Sure i915_drm_client->id becomes dead code, but it's a piece one can
> >>> live with for a release or two. Then again it's not my call to make.
> >>
> >> Rob can take the i915 patch from my RFC too.
> >>
> >
> > Indeed.
> >
> > -Emil

Patch

diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
index 2ab32c40e93c..80003e27e28e 100644
--- a/Documentation/gpu/drm-usage-stats.rst
+++ b/Documentation/gpu/drm-usage-stats.rst
@@ -105,6 +105,27 @@  object belong to this client, in the respective memory region.
 Default unit shall be bytes with optional unit specifiers of 'KiB' or 'MiB'
 indicating kibi- or mebi-bytes.
 
+- drm-shared-memory: <uint> [KiB|MiB]
+
+The total size of buffers that are shared with another file (ie. have more
+than a single handle).
+
+- drm-private-memory: <uint> [KiB|MiB]
+
+The total size of buffers that are not shared with another file.
+
+- drm-resident-memory: <uint> [KiB|MiB]
+
+The total size of buffers that are resident in system memory.
+
+- drm-purgeable-memory: <uint> [KiB|MiB]
+
+The total size of buffers that are purgeable.
+
+- drm-active-memory: <uint> [KiB|MiB]
+
+The total size of buffers that are active on one or more rings.
+
 - drm-cycles-<str> <uint>
 
 Engine identifier string must be the same as the one specified in the
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 6d5bdd684ae2..04a7ed7eba8e 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -42,6 +42,7 @@ 
 #include <drm/drm_client.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_file.h>
+#include <drm/drm_gem.h>
 #include <drm/drm_print.h>
 
 #include "drm_crtc_internal.h"
@@ -871,6 +872,79 @@  void drm_send_event(struct drm_device *dev, struct drm_pending_event *e)
 }
 EXPORT_SYMBOL(drm_send_event);
 
+static void print_size(struct drm_printer *p, const char *stat, size_t sz)
+{
+	const char *units[] = {"", " KiB", " MiB"};
+	unsigned u;
+
+	for (u = 0; u < ARRAY_SIZE(units) - 1; u++) {
+		if (sz < SZ_1K)
+			break;
+		sz = div_u64(sz, SZ_1K);
+	}
+
+	drm_printf(p, "%s:\t%zu%s\n", stat, sz, units[u]);
+}
+
+static void print_memory_stats(struct drm_printer *p, struct drm_file *file)
+{
+	struct drm_gem_object *obj;
+	struct {
+		size_t shared;
+		size_t private;
+		size_t resident;
+		size_t purgeable;
+		size_t active;
+	} size = {0};
+	bool has_status = false;
+	int id;
+
+	spin_lock(&file->table_lock);
+	idr_for_each_entry (&file->object_idr, obj, id) {
+		enum drm_gem_object_status s = 0;
+
+		if (obj->funcs && obj->funcs->status) {
+			s = obj->funcs->status(obj);
+			has_status = true;
+		}
+
+		if (obj->handle_count > 1) {
+			size.shared += obj->size;
+		} else {
+			size.private += obj->size;
+		}
+
+		if (s & DRM_GEM_OBJECT_RESIDENT) {
+			size.resident += obj->size;
+		} else {
+			/* If already purged or not yet backed by pages, don't
+			 * count it as purgeable:
+			 */
+			s &= ~DRM_GEM_OBJECT_PURGEABLE;
+		}
+
+		if (!dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true))) {
+			size.active += obj->size;
+
+			/* If still active, don't count as purgeable: */
+			s &= ~DRM_GEM_OBJECT_PURGEABLE;
+		}
+
+		if (s & DRM_GEM_OBJECT_PURGEABLE)
+			size.purgeable += obj->size;
+	}
+	spin_unlock(&file->table_lock);
+
+	print_size(p, "drm-shared-memory", size.shared);
+	print_size(p, "drm-private-memory", size.private);
+	print_size(p, "drm-active-memory", size.active);
+
+	if (has_status) {
+		print_size(p, "drm-resident-memory", size.resident);
+		print_size(p, "drm-purgeable-memory", size.purgeable);
+	}
+}
+
 /**
  * drm_show_fdinfo - helper for drm file fops
  * @seq_file: output stream
@@ -900,6 +974,8 @@  void drm_show_fdinfo(struct seq_file *m, struct file *f)
 
 	if (dev->driver->show_fdinfo)
 		dev->driver->show_fdinfo(&p, file);
+
+	print_memory_stats(&p, file);
 }
 EXPORT_SYMBOL(drm_show_fdinfo);
 
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index 6de6d0e9c634..919284bb4f1d 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -41,6 +41,7 @@ 
 struct dma_fence;
 struct drm_file;
 struct drm_device;
+struct drm_printer;
 struct device;
 struct file;
 
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index 189fd618ca65..9ebd2820ad1f 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -42,6 +42,25 @@ 
 struct iosys_map;
 struct drm_gem_object;
 
+/**
+ * enum drm_gem_object_status - bitmask of object state for fdinfo reporting
+ * @DRM_GEM_OBJECT_RESIDENT: object is resident in memory (ie. not unpinned)
+ * @DRM_GEM_OBJECT_PURGEABLE: object marked as purgeable by userspace
+ *
+ * Bitmask of status used for fdinfo memory stats, see &drm_gem_object_funcs.status
+ * and drm_show_fdinfo().  Note that an object can DRM_GEM_OBJECT_PURGEABLE if
+ * it still active or not resident, in which case drm_show_fdinfo() will not
+ * account for it as purgeable.  So drivers do not need to check if the buffer
+ * is idle and resident to return this bit.  (Ie. userspace can mark a buffer
+ * as purgeable even while it is still busy on the GPU.. it does not _actually_
+ * become puregeable until it becomes idle.  The status gem object func does
+ * not need to consider this.)
+ */
+enum drm_gem_object_status {
+	DRM_GEM_OBJECT_RESIDENT  = BIT(0),
+	DRM_GEM_OBJECT_PURGEABLE = BIT(1),
+};
+
 /**
  * struct drm_gem_object_funcs - GEM object functions
  */
@@ -174,6 +193,17 @@  struct drm_gem_object_funcs {
 	 */
 	int (*evict)(struct drm_gem_object *obj);
 
+	/**
+	 * @status:
+	 *
+	 * The optional status callback can return additional object state
+	 * which determines which stats the object is counted against.  The
+	 * callback is called under table_lock.  Racing against object status
+	 * change is "harmless", and the callback can expect to not race
+	 * against object destruction.
+	 */
+	enum drm_gem_object_status (*status)(struct drm_gem_object *obj);
+
 	/**
 	 * @vm_ops:
 	 *