Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230406215917.1475704-1-robdclark@gmail.com> <20230406215917.1475704-2-robdclark@gmail.com>
In-Reply-To: <20230406215917.1475704-2-robdclark@gmail.com>
From:   Emil Velikov <emil.l.velikov@gmail.com>
Date:   Sat, 8 Apr 2023 13:20:03 +0100
Message-ID: <CACvgo50nOw-82pc2mEbydWH3=RDXuOKwnBnjmOhV-UYcbjRKQA@mail.gmail.com>
Subject: Re: [RFC 1/2] drm: Add fdinfo memory stats
To:     Rob Clark <robdclark@gmail.com>
Cc:     dri-devel@lists.freedesktop.org,
        Rob Clark <robdclark@chromium.org>,
        Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
        Jonathan Corbet <corbet@lwn.net>,
        linux-arm-msm@vger.kernel.org,
        "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
        Christopher Healy <healych@amazon.com>,
        open list <linux-kernel@vger.kernel.org>,
        Boris Brezillon <boris.brezillon@collabora.com>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        freedreno@lists.freedesktop.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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