Re: [PATCH 3/3] drm/etnaviv: export client GPU usage statistics via fdinfo

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

Hi Lucas,

On 08/09/2022 19:10, Lucas Stach wrote:
> This exposes a accumulated GPU active time per client via the
> fdinfo infrastructure.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>   drivers/gpu/drm/etnaviv/etnaviv_drv.c | 38 ++++++++++++++++++++++++++-
>   1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index b69edb40ae2a..11b1f11fcb58 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -22,6 +22,7 @@
>   #include "etnaviv_gem.h"
>   #include "etnaviv_mmu.h"
>   #include "etnaviv_perfmon.h"
> +#include "common.xml.h"
>   
>   /*
>    * DRM operations:
> @@ -471,7 +472,42 @@ static const struct drm_ioctl_desc etnaviv_ioctls[] = {
>   	ETNA_IOCTL(PM_QUERY_SIG, pm_query_sig, DRM_RENDER_ALLOW),
>   };
>   
> -DEFINE_DRM_GEM_FOPS(fops);
> +static void etnaviv_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 etnaviv_drm_private *priv = dev->dev_private;
> +	struct etnaviv_file_private *ctx = file->driver_priv;
> +	struct drm_printer p = drm_seq_file_printer(m);

Any specific reason not to use seq_printf directly? (May be my ignorance.)

> +	int i;
> +
> +	drm_printf(&p, "drm-driver:\t%s\n", dev->driver->name);
> +	drm_printf(&p, "drm-client-id:\t%u\n", ctx->id);
> +
> +	for (i = 0; i < ETNA_MAX_PIPES; i++) {
> +                struct etnaviv_gpu *gpu = priv->gpu[i];
> +		char engine[8];
> +		int cur = 0;

Alignment renders odd in my client.

> +
> +		if (!gpu)
> +			continue;

I'd stick a comment in here to the effect of "For text output format 
description please see drm-usage-stats.rst!".

Just to leave a breadcrumb putting some restraint on adding vendor 
specific things which may be already covered by the common spec. To help 
with common tools in the future as much as possible.

> +
> +		if (gpu->identity.features & chipFeatures_PIPE_2D)
> +			cur = snprintf(engine, sizeof(engine), "2D");
> +		if (gpu->identity.features & chipFeatures_PIPE_3D)
> +			cur = snprintf(engine + cur, sizeof(engine) - cur,
> +				       "%s3D", cur ? "/" : "");
> +
> +		drm_printf(&p, "drm-engine-%s:\t%llu ns\n", engine,
> +			   ctx->sched_entity[i].elapsed_ns);

Two questions:

1)
So you have max four pipes, each can be either only 2d, 3d, or 2d/3d? 
Can you have multiple of the same like 2x 3D? If you do, have you 
considered exporting one drm-engine-% together with drm-engine-capacity- 
for it?

2)
Have you tried my, yet unmerged, vendor agnostic gputop tool with your 
changes?

https://patchwork.freedesktop.org/series/102175/

It would be interesting to know if it works.

Regards,

Tvrtko

> +	}
> +}
> +
> +static const struct file_operations fops = {
> +        .owner = THIS_MODULE,
> +        DRM_GEM_FOPS,
> +        .show_fdinfo = etnaviv_fop_show_fdinfo,
> +};
>   
>   static const struct drm_driver etnaviv_drm_driver = {
>   	.driver_features    = DRIVER_GEM | DRIVER_RENDER,