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>
 <CACvgo50nOw-82pc2mEbydWH3=RDXuOKwnBnjmOhV-UYcbjRKQA@mail.gmail.com>
In-Reply-To: <CACvgo50nOw-82pc2mEbydWH3=RDXuOKwnBnjmOhV-UYcbjRKQA@mail.gmail.com>
From:   Rob Clark <robdclark@gmail.com>
Date:   Mon, 10 Apr 2023 12:01:18 -0700
Message-ID: <CAF6AEGuesqsWQEsEOwriaNC_1TkWoJ-d=mrrUgV9CoPXMftJVQ@mail.gmail.com>
Subject: Re: [RFC 1/2] drm: Add fdinfo memory stats
To:     Emil Velikov <emil.l.velikov@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"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sat, Apr 8, 2023 at 5:20=E2=80=AFAM Emil Velikov <emil.l.velikov@gmail.c=
om> wrote:
>
> 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[] =3D {"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 =3D 0; u < ARRAY_SIZE(units) - 1; u++) {
> > +               if (sz < SZ_1K)
> > +                       break;
> > +               sz /=3D 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 sta=
ts
> > + * @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 s=
pecified
> > + * file.  The optional status callback can return additional object st=
ate 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 +=3D obj->size;
> > +                       s &=3D ~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 +=3D obj->size;
> > +                       s &=3D ~DRM_GEM_OBJECT_PURGEABLE;
>
> Ditto.
>
> With the above nits, the patch is:
> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
>
> HTH
> Emil