Re: [RFC] drm/amdgpu: Convert to common fdinfo format

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Hi Tvrtko,

Am 10.05.22 um 10:23 schrieb Tvrtko Ursulin:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Convert fdinfo format to one documented in drm-usage-stats.rst.
>
> Opens/TODO:
>   * Does someone from AMD want to take over this patch?
>      (I have no access to amdgpu hardware so won't be able to test
>       any hypothetical gputop work.)

I can give that a try as soon as it is completed.

>   * What are the semantics of AMD engine utilisation reported in percents?

To be honest I haven't understood why we are using percents here either, 
that is not something the kernel should mess with.

>      * Can it align with what i915 does (same what msm will do) or need
>        to document the alternative in the specification document? Both
>        option are workable with instantaneous percent only needing support
>        to be added to vendor agnostic gputop.

I would prefer to just change to the ns format i915 and msm will be 
using, that makes much more sense from my experience.

As far as I know we haven't released any publicly available userspace 
using the existing AMD specific format. So that should still be possible.

>   * Can amdgpu expose drm-client-id? Without it gputop will not work.

How is that determined on i915 ? Does struct drm_file has that somewhere?

> * drm-engine-capacity - does the concept translate etc.

I don't think we are going to need that.

Regards,
Christian.

>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: David M Nieto <David.Nieto@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Acked-by: Christian König <christian.koenig@amd.com>
> ---
>   Documentation/gpu/amdgpu/usage-stats.rst   | 28 ++++++++++++++++++++++
>   Documentation/gpu/drm-usage-stats.rst      |  7 +++++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c | 18 ++++++++++----
>   3 files changed, 47 insertions(+), 6 deletions(-)
>   create mode 100644 Documentation/gpu/amdgpu/usage-stats.rst
>
> diff --git a/Documentation/gpu/amdgpu/usage-stats.rst b/Documentation/gpu/amdgpu/usage-stats.rst
> new file mode 100644
> index 000000000000..8be5009bd1a9
> --- /dev/null
> +++ b/Documentation/gpu/amdgpu/usage-stats.rst
> @@ -0,0 +1,28 @@
> +.. _amdgpu-usage-stats:
> +
> +============================================
> +AMDGPU DRM client usage stats implementation
> +============================================
> +
> +The amdgpu driver implements the DRM client usage stats specification as
> +documented in :ref:`drm-client-usage-stats`.
> +
> +Example of the output showing the implemented key value pairs and entirety of
> +the currenly possible format options:
> +
> +::
> +
> +      pos:    0
> +      flags:  0100002
> +      mnt_id: 21
> +      drm-driver: amdgpu
> +      drm-pdev:   0000:03:00.0
> +      drm-memory-vram: 0 KiB
> +      drm-memory-gtt: 0 KiB
> +      drm-memory-cpu: 0 KiB
> +      drm-engine-...: 0 %
> +                 ...
> +
> +Possible `drm-memory-` key names are: `vram`, `gtt`, `cpu`.
> +
> +Possible `drm-engine-` key names are: ``.
> diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
> index 6c9f166a8d6f..2d0ff6f2cc74 100644
> --- a/Documentation/gpu/drm-usage-stats.rst
> +++ b/Documentation/gpu/drm-usage-stats.rst
> @@ -69,7 +69,7 @@ scope of each device, in which case `drm-pdev` shall be present as well.
>   Userspace should make sure to not double account any usage statistics by using
>   the above described criteria in order to associate data to individual clients.
>   
> -- drm-engine-<str>: <uint> ns
> +- drm-engine-<str>: <uint> [ns|%]
>   
>   GPUs usually contain multiple execution engines. Each shall be given a stable
>   and unique name (str), with possible values documented in the driver specific
> @@ -84,6 +84,9 @@ larger value within a reasonable period. Upon observing a value lower than what
>   was previously read, userspace is expected to stay with that larger previous
>   value until a monotonic update is seen.
>   
> +Where time unit is given as a percentage...[AMD folks to fill the semantics
> +and interpretation of that]...
> +
>   - drm-engine-capacity-<str>: <uint>
>   
>   Engine identifier string must be the same as the one specified in the
> @@ -110,3 +113,5 @@ Driver specific implementations
>   ===============================
>   
>   :ref:`i915-usage-stats`
> +
> +:ref:`amdgpu-usage-stats`
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> index 5a6857c44bb6..8cbae61f1b3b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> @@ -32,6 +32,7 @@
>   
>   #include <drm/amdgpu_drm.h>
>   #include <drm/drm_debugfs.h>
> +#include <drm/drm_drv.h>
>   
>   #include "amdgpu.h"
>   #include "amdgpu_vm.h"
> @@ -83,11 +84,18 @@ void amdgpu_show_fdinfo(struct seq_file *m, struct file *f)
>   	amdgpu_bo_unreserve(root);
>   	amdgpu_bo_unref(&root);
>   
> -	seq_printf(m, "pdev:\t%04x:%02x:%02x.%d\npasid:\t%u\n", domain, bus,
> +	/*
> +	 * ******************************************************************
> +	 * For text output format description please see drm-usage-stats.rst!
> +	 * ******************************************************************
> +	 */
> +
> +	seq_printf(m, "drm-driver:\t%s\n", file->minor->dev->driver->name);
> +	seq_printf(m, "drm-pdev:\t%04x:%02x:%02x.%d\npasid:\t%u\n", domain, bus,
>   			dev, fn, fpriv->vm.pasid);
> -	seq_printf(m, "vram mem:\t%llu kB\n", vram_mem/1024UL);
> -	seq_printf(m, "gtt mem:\t%llu kB\n", gtt_mem/1024UL);
> -	seq_printf(m, "cpu mem:\t%llu kB\n", cpu_mem/1024UL);
> +	seq_printf(m, "drm-memory-vram:\t%llu KiB\n", vram_mem/1024UL);
> +	seq_printf(m, "drm-memory-gtt:\t%llu KiB\n", gtt_mem/1024UL);
> +	seq_printf(m, "drm-memory-cpu:\t%llu KiB\n", cpu_mem/1024UL);
>   	for (i = 0; i < AMDGPU_HW_IP_NUM; i++) {
>   		uint32_t count = amdgpu_ctx_num_entities[i];
>   		int idx = 0;
> @@ -103,7 +111,7 @@ void amdgpu_show_fdinfo(struct seq_file *m, struct file *f)
>   			perc = div64_u64(10000 * total, min);
>   			frac = perc % 100;
>   
> -			seq_printf(m, "%s%d:\t%d.%d%%\n",
> +			seq_printf(m, "drm-engine-%s%d:\t%d.%d %%\n",
>   					amdgpu_ip_name[i],
>   					idx, perc/100, frac);
>   		}