Re: [RFC 1/2] drm: Add fdinfo memory stats

[Rob Clark]

On Sat, Apr 8, 2023 at 5:20 AM Emil Velikov <emil.l.velikov@xxxxxxxxx> wrote:
>
> Hey Rob,
>
> On Thu, 6 Apr 2023 at 22:59, Rob Clark <robdclark@xxxxxxxxx> wrote:
>
> > +- drm-purgeable-memory: <uint> [KiB|MiB]
> > +
> > +The total size of buffers that are purgable.
>
> s/purgable/purgeable/
>
>
> > +static void print_size(struct drm_printer *p, const char *stat, size_t sz)
> > +{
> > +       const char *units[] = {"B", "KiB", "MiB", "GiB"};
>
> The documentation says:
>
> > Default unit shall be bytes with optional unit specifiers of 'KiB' or 'MiB'
> > indicating kibi- or mebi-bytes.
>
> So I would drop the B and/or update the documentation to mention B && GiB.
>
> > +       unsigned u;
> > +
> > +       for (u = 0; u < ARRAY_SIZE(units) - 1; u++) {
> > +               if (sz < SZ_1K)
> > +                       break;
> > +               sz /= SZ_1K;
>
> IIRC size_t can be 64bit, so we should probably use do_div() here.
>
> > +       }
> > +
> > +       drm_printf(p, "%s:\t%lu %s\n", stat, sz, units[u]);
> > +}
> > +
> > +/**
> > + * drm_print_memory_stats - Helper to print standard fdinfo memory stats
> > + * @file: the DRM file
> > + * @p: the printer to print output to
> > + * @status: callback to get driver tracked object status
> > + *
> > + * Helper to iterate over GEM objects with a handle allocated in the specified
> > + * file.  The optional status callback can return additional object state which
>
> s/return additional/return an additional/

"an" reads funny to me, as the state is plural (bitmask).. but agreed
on the other things

> > + * 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 destroy.
>
> s/destroy/destruction/
>
> > + */
> > +void drm_print_memory_stats(struct drm_file *file, struct drm_printer *p,
> > +                           enum drm_gem_object_status (*status)(struct drm_gem_object *))
> > +{
>
> > +               if (s & DRM_GEM_OBJECT_RESIDENT) {
> > +                       size.resident += obj->size;
> > +                       s &= ~DRM_GEM_OBJECT_PURGEABLE;
>
> Is MSM capable of marking the object as both purgeable and resident or
> is this to catch other drivers? Should we add a note to the
> documentation above - resident memory cannot be purgeable

It is just to simplify drivers so they don't have to repeat this
logic.  Ie. an object can be marked purgeable while it is still active
(so it will be eventually purgeable when it becomes idle).  Likewise
it doesn't make sense to count an object that has already been purged
(is no longer resident) as purgeable.

BR,
-R

> > +               }
> > +
> > +               if (s & DRM_GEM_OBJECT_ACTIVE) {
> > +                       size.active += obj->size;
> > +                       s &= ~DRM_GEM_OBJECT_PURGEABLE;
>
> Ditto.
>
> With the above nits, the patch is:
> Reviewed-by: Emil Velikov <emil.l.velikov@xxxxxxxxx>
>
> HTH
> Emil