On Thu, Jun 6, 2024 at 1:35 AM Tvrtko Ursulin <tursulin@xxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>
> 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);
