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

[Emil Velikov]

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/

> + * 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

> +               }
> +
> +               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