Date: Mon, 24 Jun 2024 10:47:28 -0700
From: Rob Clark <robdclark@...il.com>
To: Tvrtko Ursulin <tursulin@...ulin.net>
Cc: Adrián Larumbe <adrian.larumbe@...labora.com>, 
	Boris Brezillon <boris.brezillon@...labora.com>, Steven Price <steven.price@....com>, 
	Liviu Dudau <liviu.dudau@....com>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, 
	David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>, kernel@...labora.com, 
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 6/7] drm/drm_file: add display of driver's internal
 memory size

On Thu, Jun 6, 2024 at 1:35 AM Tvrtko Ursulin <tursulin@...ulin.net> wrote:
>
>
> On 06/06/2024 02:49, Adrián Larumbe wrote:
>
> > Some drivers must allocate a considerable amount of memory for bookkeeping
> > structures and GPU's MCU-kernel shared communication regions. These are
> > often created as a result of the invocation of the driver's ioctl()
> > interface functions, so it is sensible to consider them as being owned by
> > the render context associated with an open drm file.
> >
> > However, at the moment drm_show_memory_stats only traverses the UM-exposed
> > drm objects for which a handle exists. Private driver objects and memory
> > regions, though connected to a render context, are unaccounted for in their
> > fdinfo numbers.
> >
> > Add a new drm_memory_stats 'internal' memory category.
> >
> > Because deciding what constitutes an 'internal' object and where to find
> > these are driver-dependent, calculation of this size must be done through a
> > driver-provided function pointer, which becomes the third argument of
> > drm_show_memory_stats. Drivers which have no interest in exposing the size
> > of internal memory objects can keep passing NULL for unaltered behaviour.
> >
> > Signed-off-by: Adrián Larumbe <adrian.larumbe@...labora.com>
>
> Please Cc people who were previously involved in defining
> drm-usage-stats.rst. I added Rob, but I am not sure if I forgot someone
> from the top of my head.
>
> Internal as a category sounds potentially useful. One reservation I have
> though is itdoes not necessarily fit with the others but is something
> semantically different from them.
>
> In i915 I had the similar desire to account for internal objects and
> have approached it by similarly tracking them outside the DRM idr but
> counting them under the existing respective categories and memory
> regions. Ie. internal objects can also be purgeable or not, etc, and can
> be backed by either system memory or device local memory.
>
> Advantage is it is more accurate in those aspect and does not require
> adding a new category.
>
> Downside of this is that 'internal' is bunched with the explicit
> userspace objects so perhaps less accurate in this other aspect.
>
> Regards,
>
> Tvrtko
>
> > ---
> >   Documentation/gpu/drm-usage-stats.rst   | 4 ++++
> >   drivers/gpu/drm/drm_file.c              | 9 +++++++--
> >   drivers/gpu/drm/msm/msm_drv.c           | 2 +-
> >   drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
> >   include/drm/drm_file.h                  | 7 ++++++-
> >   5 files changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
> > index 6dc299343b48..0da5ebecd232 100644
> > --- a/Documentation/gpu/drm-usage-stats.rst
> > +++ b/Documentation/gpu/drm-usage-stats.rst
> > @@ -157,6 +157,10 @@ The total size of buffers that are purgeable.
> >
> >   The total size of buffers that are active on one or more engines.
> >
> > +- drm-internal-<region>: <uint> [KiB|MiB]
> > +
> > +The total size of GEM objects that aren't exposed to user space.
> > +
> >   Implementation Details
> >   ======================
> >
> > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > index 638ffa4444f5..d1c13eed8d34 100644
> > --- a/drivers/gpu/drm/drm_file.c
> > +++ b/drivers/gpu/drm/drm_file.c
> > @@ -874,9 +874,10 @@ void drm_print_memory_stats(struct drm_printer *p,
> >                           enum drm_gem_object_status supported_status,
> >                           const char *region)
> >   {
> > -     print_size(p, "total", region, stats->private + stats->shared);
> > +     print_size(p, "total", region, stats->private + stats->shared + stats->internal);
> >       print_size(p, "shared", region, stats->shared);
> >       print_size(p, "active", region, stats->active);
> > +     print_size(p, "internal", region, stats->internal);
> >
> >       if (supported_status & DRM_GEM_OBJECT_RESIDENT)
> >               print_size(p, "resident", region, stats->resident);
> > @@ -890,11 +891,12 @@ EXPORT_SYMBOL(drm_print_memory_stats);
> >    * drm_show_memory_stats - Helper to collect and show standard fdinfo memory stats
> >    * @p: the printer to print output to
> >    * @file: the DRM file
> > + * @func: driver-specific function pointer to count the size of internal objects
> >    *
> >    * Helper to iterate over GEM objects with a handle allocated in the specified
> >    * file.
> >    */
> > -void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
> > +void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file, internal_bos func)
> >   {
> >       struct drm_gem_object *obj;
> >       struct drm_memory_stats status = {};
> > @@ -940,6 +942,9 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
> >       }
> >       spin_unlock(&file->table_lock);
> >
> > +     if (func)
> > +             func(&status, file);

Seems like it would be simpler to just pass `u64 internal` to
drm_show_memory_stats() instead of a callback.

But I agree with Tvrtko's comment about being somewhat (I think?)
orthogonal to active/resident.  I guess somewhere you have a list of
internal BOs?

Perhaps another option is to pass an (optionally NULL) list of BOs to
drm_show_memory_stats() to iterate so that they can be counted as
active/resident/etc?  Or yet another alternative, pass an (optionally
NULL) `struct drm_memory_stats *` to initialize status.

BR,
-R

> > +
> >       drm_print_memory_stats(p, &status, supported_status, "memory");
> >   }
> >   EXPORT_SYMBOL(drm_show_memory_stats);
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index 9c33f4e3f822..f97d3cdc4f50 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -880,7 +880,7 @@ static void msm_show_fdinfo(struct drm_printer *p, struct drm_file *file)
> >
> >       msm_gpu_show_fdinfo(priv->gpu, file->driver_priv, p);
> >
> > -     drm_show_memory_stats(p, file);
> > +     drm_show_memory_stats(p, file, NULL);
> >   }
> >
> >   static const struct file_operations fops = {
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > index ef9f6c0716d5..53640ac44e42 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > @@ -570,7 +570,7 @@ static void panfrost_show_fdinfo(struct drm_printer *p, struct drm_file *file)
> >
> >       panfrost_gpu_show_fdinfo(pfdev, file->driver_priv, p);
> >
> > -     drm_show_memory_stats(p, file);
> > +     drm_show_memory_stats(p, file, NULL);
> >   }
> >
> >   static const struct file_operations panfrost_drm_driver_fops = {
> > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> > index ab230d3af138..d71a5ac50ea9 100644
> > --- a/include/drm/drm_file.h
> > +++ b/include/drm/drm_file.h
> > @@ -464,6 +464,7 @@ void drm_send_event_timestamp_locked(struct drm_device *dev,
> >    * @resident: Total size of GEM objects backing pages
> >    * @purgeable: Total size of GEM objects that can be purged (resident and not active)
> >    * @active: Total size of GEM objects active on one or more engines
> > + * @internal: Total size of GEM objects that aren't exposed to user space
> >    *
> >    * Used by drm_print_memory_stats()
> >    */
> > @@ -473,16 +474,20 @@ struct drm_memory_stats {
> >       u64 resident;
> >       u64 purgeable;
> >       u64 active;
> > +     u64 internal;
> >   };
> >
> >   enum drm_gem_object_status;
> >
> > +typedef void (*internal_bos)(struct drm_memory_stats *status,
> > +                          struct drm_file *file);
> > +
> >   void drm_print_memory_stats(struct drm_printer *p,
> >                           const struct drm_memory_stats *stats,
> >                           enum drm_gem_object_status supported_status,
> >                           const char *region);
> >
> > -void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file);
> > +void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file, internal_bos func);
> >   void drm_show_fdinfo(struct seq_file *m, struct file *f);
> >
> >   struct file *mock_drm_getfile(struct drm_minor *minor, unsigned int flags);

Powered by blists - more mailing lists