Hi Rob,
On Thu, 6 Apr 2023 14:59:16 -0700
Rob Clark <robdcl...@gmail.com> wrote:
> From: Rob Clark <robdcl...@chromium.org>
>
> Add a helper to dump memory stats to fdinfo. For the things the drm
> core isn't aware of, use a callback.
>
> Signed-off-by: Rob Clark <robdcl...@chromium.org>
> ---
> Documentation/gpu/drm-usage-stats.rst | 21 +++++++
> drivers/gpu/drm/drm_file.c | 79 +++++++++++++++++++++++++++
> include/drm/drm_file.h | 10 ++++
> 3 files changed, 110 insertions(+)
>
> diff --git a/Documentation/gpu/drm-usage-stats.rst
> b/Documentation/gpu/drm-usage-stats.rst
> index b46327356e80..56e3c95b6e0a 100644
> --- a/Documentation/gpu/drm-usage-stats.rst
> +++ b/Documentation/gpu/drm-usage-stats.rst
> @@ -105,6 +105,27 @@ object belong to this client, in the respective memory
> region.
> Default unit shall be bytes with optional unit specifiers of 'KiB' or 'MiB'
> indicating kibi- or mebi-bytes.
>
> +- drm-shared-memory: <uint> [KiB|MiB]
> +
> +The total size of buffers that are shared with another file (ie. have more
> +than a single handle).
> +
> +- drm-private-memory: <uint> [KiB|MiB]
> +
> +The total size of buffers that are not shared with another file.
> +
> +- drm-resident-memory: <uint> [KiB|MiB]
> +
> +The total size of buffers that are resident in system memory.
> +
> +- drm-purgeable-memory: <uint> [KiB|MiB]
> +
> +The total size of buffers that are purgable.
> +
> +- drm-active-memory: <uint> [KiB|MiB]
> +
> +The total size of buffers that are active on one or more rings.
> +
> - drm-cycles-<str> <uint>
>
> Engine identifier string must be the same as the one specified in the
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index a51ff8cee049..21911d6ff38d 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -42,6 +42,7 @@
> #include <drm/drm_client.h>
> #include <drm/drm_drv.h>
> #include <drm/drm_file.h>
> +#include <drm/drm_gem.h>
> #include <drm/drm_print.h>
>
> #include "drm_crtc_internal.h"
> @@ -868,6 +869,84 @@ 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, size_t sz)
> +{
> + const char *units[] = {"B", "KiB", "MiB", "GiB"};
> + unsigned u;
> +
> + for (u = 0; u < ARRAY_SIZE(units) - 1; u++) {
> + if (sz < SZ_1K)
> + break;
> + sz /= SZ_1K;
> + }
> +
> + 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
> + * 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.
> + */
> +void drm_print_memory_stats(struct drm_file *file, struct drm_printer *p,
> + enum drm_gem_object_status (*status)(struct
> drm_gem_object *))
Nit: status() returning an 'enum drm_gem_object_status' makes it look
like it can only return one of the DRM_GEM_OBJECT_* flag, when it can
actually be a combination of flags. Should we make it return an u32
instead? At the very least this should be clarified in the function doc.
> +{
> + struct drm_gem_object *obj;
> + struct {
> + size_t shared;
> + size_t private;
> + size_t resident;
> + size_t purgeable;
> + size_t active;
> + } size = {0};
> + int id;
> +
> + spin_lock(&file->table_lock);
> + idr_for_each_entry (&file->object_idr, obj, id) {
> + enum drm_gem_object_status s = 0;
> +
> + if (status)
> + s = status(obj);
> +
> + if (obj->handle_count > 1) {
> + size.shared += obj->size;
> + } else {
> + size.private += obj->size;
> + }
> +
> + if (s & DRM_GEM_OBJECT_RESIDENT) {
> + size.resident += obj->size;
> + s &= ~DRM_GEM_OBJECT_PURGEABLE;
> + }
> +
> + if (s & DRM_GEM_OBJECT_ACTIVE) {
> + size.active += obj->size;
> + s &= ~DRM_GEM_OBJECT_PURGEABLE;
> + }
> +
> + if (s & DRM_GEM_OBJECT_PURGEABLE)
> + size.purgeable += obj->size;
> + }
> + spin_unlock(&file->table_lock);
> +
> + print_size(p, "drm-shared-memory", size.shared);
> + print_size(p, "drm-private-memory", size.private);
> +
> + if (status) {
> + print_size(p, "drm-resident-memory", size.resident);
> + print_size(p, "drm-purgeable-memory", size.purgeable);
> + print_size(p, "drm-active-memory", size.active);
> + }
Should we assume that all users of this drm_print_memory_stats() helper
support tracking all these non-standard stats as soon as they provide a
status callback? If not, we should probably not print the
`drm-xxx-memory` line when the driver is not tracking this state (can
be done with a 'supported_status' mask passed to
drm_print_memory_stats()).
Other than these 2 minor things, I think it's a perfect match for
panfrost mem-tracking, and I certainly intend to use this helper in
panfrost.
Thanks,
Boris
