| Message ID | 20230412224311.23511-6-robdclark@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | drm: fdinfo memory stats | expand |
On Wed, 12 Apr 2023 at 23:43, Rob Clark <robdclark@gmail.com> 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 <emil.l.velikov@gmail.com> 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
On 21/04/2023 11:26, Emil Velikov wrote: > On Wed, 12 Apr 2023 at 23:43, Rob Clark <robdclark@gmail.com> 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 <emil.l.velikov@gmail.com> 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
On Fri, 21 Apr 2023 at 12:23, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > On 21/04/2023 11:26, Emil Velikov wrote: > > On Wed, 12 Apr 2023 at 23:43, Rob Clark <robdclark@gmail.com> 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 <emil.l.velikov@gmail.com> > > 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
On 21/04/2023 12:45, Emil Velikov wrote: > On Fri, 21 Apr 2023 at 12:23, Tvrtko Ursulin > <tvrtko.ursulin@linux.intel.com> wrote: > >> On 21/04/2023 11:26, Emil Velikov wrote: >>> On Wed, 12 Apr 2023 at 23:43, Rob Clark <robdclark@gmail.com> 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 <emil.l.velikov@gmail.com> >> >> 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
On Fri, Apr 21, 2023 at 4:59 AM Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > > > On 21/04/2023 12:45, Emil Velikov wrote: > > On Fri, 21 Apr 2023 at 12:23, Tvrtko Ursulin > > <tvrtko.ursulin@linux.intel.com> wrote: > > > >> On 21/04/2023 11:26, Emil Velikov wrote: > >>> On Wed, 12 Apr 2023 at 23:43, Rob Clark <robdclark@gmail.com> 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 <emil.l.velikov@gmail.com> > >> > >> 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
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: *