Re: [Freedreno] [RFC 1/2] drm: Add fdinfo memory stats
Hey Rob,
On Thu, 6 Apr 2023 at 22:59, Rob Clark wrote:
> +- drm-purgeable-memory: [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
HTH
Emil
Re: [Freedreno] [RFC 2/2] drm/msm: Add memory stats to fdinfo
On Thu, 6 Apr 2023 at 22:59, Rob Clark wrote:
>
> From: Rob Clark
>
> Use the new helper to export stats about memory usage.
>
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/msm/msm_drv.c | 26 +-
> drivers/gpu/drm/msm/msm_gpu.c | 2 --
> 2 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 9b6f17b1261f..385776f6a531 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -1043,17 +1043,40 @@ static const struct drm_ioctl_desc msm_ioctls[] = {
> DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_QUERY, msm_ioctl_submitqueue_query,
> DRM_RENDER_ALLOW),
> };
>
> +enum drm_gem_object_status gem_status(struct drm_gem_object *obj)
> +{
> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
> + enum drm_gem_object_status status = 0;
> +
> + if (!dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true)))
> + status |= DRM_GEM_OBJECT_ACTIVE;
> +
> + if (msm_obj->pages)
> + status |= DRM_GEM_OBJECT_RESIDENT;
> +
> + if (msm_obj->madv == MSM_MADV_DONTNEED)
> + status |= DRM_GEM_OBJECT_PURGEABLE;
> +
> + return status;
> +}
> +
> static void msm_fop_show_fdinfo(struct seq_file *m, struct file *f)
> {
> struct drm_file *file = f->private_data;
> struct drm_device *dev = file->minor->dev;
> struct msm_drm_private *priv = dev->dev_private;
> + struct msm_file_private *ctx = file->driver_priv;
> struct drm_printer p = drm_seq_file_printer(m);
>
> if (!priv->gpu)
> return;
>
> - msm_gpu_show_fdinfo(priv->gpu, file->driver_priv, &p);
> + drm_printf(&p, "drm-driver:\t%s\n", dev->driver->name);
> + drm_printf(&p, "drm-client-id:\t%u\n", ctx->seqno);
> +
> + msm_gpu_show_fdinfo(priv->gpu, ctx, &p);
> +
> + drm_print_memory_stats(file, &p, gem_status);
> }
>
> static const struct file_operations fops = {
> @@ -1067,6 +1090,7 @@ static const struct drm_driver msm_driver = {
> DRIVER_RENDER |
> DRIVER_ATOMIC |
> DRIVER_MODESET |
> + DRIVER_SYNCOBJ_TIMELINE |
This line should probably be its own patch. AFAICT it was supported
since ab723b7a992a19b843f798b183f53f7472f598c8, although explicitly
kept disabled until there's userspace/turnip support.
With the above line removed, the patch is:
Reviewed-by: Emil Velikov
HTH
Emil
[Freedreno] [RFC PATCH] drm/msm/a5xx: really check for A510 in a5xx_gpu_init
The commit 010c8bbad2cb ("drm: msm: adreno: Disable preemption on Adreno
510") added special handling for a510 (this SKU doesn't seem to support
preemption, so the driver should clamp nr_rings to 1). However the
gpu->revn is not yet set (it is set later, in adreno_gpu_init()) and
thus the condition is always false. Check config->rev instead.
Fixes: 010c8bbad2cb ("drm: msm: adreno: Disable preemption on Adreno 510")
Reported-by: Adam Skladowski
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index 1e8d2982d603..a99310b68793 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -1743,6 +1743,7 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
{
struct msm_drm_private *priv = dev->dev_private;
struct platform_device *pdev = priv->gpu_pdev;
+ struct adreno_platform_config *config = pdev->dev.platform_data;
struct a5xx_gpu *a5xx_gpu = NULL;
struct adreno_gpu *adreno_gpu;
struct msm_gpu *gpu;
@@ -1769,7 +1770,7 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
nr_rings = 4;
- if (adreno_is_a510(adreno_gpu))
+ if (adreno_cmp_rev(ADRENO_REV(5, 1, 0, ANY_ID), config->rev))
nr_rings = 1;
ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, nr_rings);
--
2.30.2
