Re: [PATCH v2 6/6] drm/drm-file: Allow size unit selection in drm_show_memory_stats

[Rob Clark]

On Wed, Aug 30, 2023 at 8:51 AM Adrián Larumbe
<adrian.larumbe@xxxxxxxxxxxxx> wrote:
>
> >> The current implementation will try to pick the highest available
> >> unit. This is rather unflexible, and allowing drivers to display BO size
> >> statistics through fdinfo in units of their choice might be desirable.
> >>
> >> The new argument to drm_show_memory_stats is to be interpreted as the
> >> integer multiplier of a 10-power of 2, so 1 would give us size in Kib and 2
> >> in Mib. If we want drm-file functions to pick the highest unit, then 0
> >> should be passed.
> >>
> >> Signed-off-by: Adrián Larumbe <adrian.larumbe@xxxxxxxxxxxxx>
> >> ---
> >>  drivers/gpu/drm/drm_file.c              | 22 +++++++++++++---------
> >>  drivers/gpu/drm/msm/msm_drv.c           |  2 +-
> >>  drivers/gpu/drm/panfrost/panfrost_drv.c |  2 +-
> >>  include/drm/drm_file.h                  |  5 +++--
> >>  4 files changed, 18 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> >> index 762965e3d503..517e1fb8072a 100644
> >> --- a/drivers/gpu/drm/drm_file.c
> >> +++ b/drivers/gpu/drm/drm_file.c
> >> @@ -873,7 +873,7 @@ 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,
> >> -                      const char *region, u64 sz)
> >> +                      const char *region, u64 sz, unsigned int unit)
> >>  {
> >>         const char *units[] = {"", " KiB", " MiB"};
> >>         unsigned u;
> >> @@ -881,6 +881,8 @@ static void print_size(struct drm_printer *p, const char *stat,
> >>         for (u = 0; u < ARRAY_SIZE(units) - 1; u++) {
> >>                 if (sz < SZ_1K)
> >>                         break;
> >> +               if (unit > 0 && unit == u)
> >> +                       break;
> >>                 sz = div_u64(sz, SZ_1K);
> >>         }
> >>
> >> @@ -898,17 +900,18 @@ static void print_size(struct drm_printer *p, const char *stat,
> >>  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)
> >> +                           const char *region,
> >> +                           unsigned int unit)
> >
> >I'm not really adverse to changing what units we use.. or perhaps
> >changing the threshold to go to higher units to be 10000x or 100000x
> >of the previous unit.  But I'm less excited about having different
> >drivers using different units.
> >
> >BR,
> >-R
>
> Would it be alright if I left it set to the default unit, and allow changing it
> at runtime with a debugfs file?

I suppose we could, but it does seem a bit like overkill.  OTOH I
think it would make sense to increase the threshold, ie. switch to MiB
after 10MiB instead of 1MiB.. at that point the fractional component
is less significant..

BR,
-R

> >>  {
> >> -       print_size(p, "total", region, stats->private + stats->shared);
> >> -       print_size(p, "shared", region, stats->shared);
> >> -       print_size(p, "active", region, stats->active);
> >> +       print_size(p, "total", region, stats->private + stats->shared, unit);
> >> +       print_size(p, "shared", region, stats->shared, unit);
> >> +       print_size(p, "active", region, stats->active, unit);
> >>
> >>         if (supported_status & DRM_GEM_OBJECT_RESIDENT)
> >> -               print_size(p, "resident", region, stats->resident);
> >> +               print_size(p, "resident", region, stats->resident, unit);
> >>
> >>         if (supported_status & DRM_GEM_OBJECT_PURGEABLE)
> >> -               print_size(p, "purgeable", region, stats->purgeable);
> >> +               print_size(p, "purgeable", region, stats->purgeable, unit);
> >>  }
> >>  EXPORT_SYMBOL(drm_print_memory_stats);
> >>
> >> @@ -916,11 +919,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
> >> + * @unit: multipliyer of power of two exponent of desired unit
> >>   *
> >>   * 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, unsigned int unit)
> >>  {
> >>         struct drm_gem_object *obj;
> >>         struct drm_memory_stats status = {};
> >> @@ -967,7 +971,7 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
> >>         }
> >>         spin_unlock(&file->table_lock);
> >>
> >> -       drm_print_memory_stats(p, &status, supported_status, "memory");
> >> +       drm_print_memory_stats(p, &status, supported_status, "memory", unit);
> >>  }
> >>  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 2a0e3529598b..cd1198151744 100644
> >> --- a/drivers/gpu/drm/msm/msm_drv.c
> >> +++ b/drivers/gpu/drm/msm/msm_drv.c
> >> @@ -1067,7 +1067,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, 0);
> >>  }
> >>
> >>  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 93d5f5538c0b..79c08cee3e9d 100644
> >> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> >> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> >> @@ -563,7 +563,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, 1);
> >>  }
> >>
> >>  static const struct file_operations panfrost_drm_driver_fops = {
> >> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> >> index 010239392adf..21a3b022dd63 100644
> >> --- a/include/drm/drm_file.h
> >> +++ b/include/drm/drm_file.h
> >> @@ -466,9 +466,10 @@ enum drm_gem_object_status;
> >>  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);
> >> +                           const char *region,
> >> +                           unsigned int unit);
> >>
> >> -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, unsigned int unit);
> >>  void drm_show_fdinfo(struct seq_file *m, struct file *f);
> >>
> >>  struct file *mock_drm_getfile(struct drm_minor *minor, unsigned int flags);
> >> --
> >> 2.42.0
> >>