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

Emil Velikov
2023-04-08T12:20:19Z

Hey Rob,

On Thu, 6 Apr 2023 at 22:59, Rob Clark <robdclark@gmail.com> 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@gmail.com>

HTH
Emil