* [RFC] drm/amdgpu: Convert to common fdinfo format
@ 2022-05-10 8:23 Tvrtko Ursulin
2022-05-10 8:48 ` Christian König
0 siblings, 1 reply; 5+ messages in thread
From: Tvrtko Ursulin @ 2022-05-10 8:23 UTC (permalink / raw)
To: amd-gfx
Cc: Daniel Vetter, David M Nieto, Christian König, dri-devel,
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.)
* What are the semantics of AMD engine utilisation reported in percents?
* 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.
* Can amdgpu expose drm-client-id? Without it gputop will not work.
* drm-engine-capacity - does the concept translate etc.
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);
}
--
2.32.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC] drm/amdgpu: Convert to common fdinfo format
2022-05-10 8:23 [RFC] drm/amdgpu: Convert to common fdinfo format Tvrtko Ursulin
@ 2022-05-10 8:48 ` Christian König
2022-05-10 10:50 ` Tvrtko Ursulin
0 siblings, 1 reply; 5+ messages in thread
From: Christian König @ 2022-05-10 8:48 UTC (permalink / raw)
To: Tvrtko Ursulin, amd-gfx
Cc: dri-devel, Tvrtko Ursulin, Christian König, Daniel Vetter,
David M Nieto
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);
> }
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC] drm/amdgpu: Convert to common fdinfo format
2022-05-10 8:48 ` Christian König
@ 2022-05-10 10:50 ` Tvrtko Ursulin
2022-05-10 11:26 ` Christian König
0 siblings, 1 reply; 5+ messages in thread
From: Tvrtko Ursulin @ 2022-05-10 10:50 UTC (permalink / raw)
To: Christian König, amd-gfx
Cc: dri-devel, Christian König, Daniel Vetter, Tvrtko Ursulin
Hi,
On 10/05/2022 09:48, Christian König wrote:
> 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.
And how to motivate someone on your side to pick up the amdgpu work? :)
>> * 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.
If amdgpu could export accumulated time context spent on engines that
would indeed be perfect. It would make the gputop I sketched out most
probably just work, as it did for Rob on msm.
In which case, apart from the admgpu work, it would just be a matter of
me tidying that tool a bit and re-sending out for review.
>> * 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?
It should correspond 1:1 with drm_file, since the purpose is to enable
gputop distinguish between unique open file descriptors (aka clients).
In theory it could be just a hash value of a struct drm_file pointer but
that could confuse userspace if the struct gets reused within a single
userspace sampling period.
Because of that I track it in i915 separately since I wanted to have an
incrementing cyclic property to it - so that when a fd is closed and new
opened there is no practical chance they would have the same drm-client-id.
>> * drm-engine-capacity - does the concept translate etc.
>
> I don't think we are going to need that.
Okay, that one is optional for cases when there is more than one engine
of a type/class shown under a single entry in fdinfo. So when gputop
translates accumulated time into percentages it can do the right thing.
Code can already handle it not being present and assume one.
Regards,
Tvrtko
>
> 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);
>> }
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC] drm/amdgpu: Convert to common fdinfo format
2022-05-10 10:50 ` Tvrtko Ursulin
@ 2022-05-10 11:26 ` Christian König
2022-05-10 12:56 ` Tvrtko Ursulin
0 siblings, 1 reply; 5+ messages in thread
From: Christian König @ 2022-05-10 11:26 UTC (permalink / raw)
To: Tvrtko Ursulin, amd-gfx
Cc: dri-devel, Christian König, Daniel Vetter, Tvrtko Ursulin
Am 10.05.22 um 12:50 schrieb Tvrtko Ursulin:
>
> Hi,
>
> On 10/05/2022 09:48, Christian König wrote:
>> 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.
>
> And how to motivate someone on your side to pick up the amdgpu work? :)
Well if we could get more of my TTM/DRM patches reviewed I could have
same free time to do this :)
>>> * 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.
>
> If amdgpu could export accumulated time context spent on engines that
> would indeed be perfect. It would make the gputop I sketched out most
> probably just work, as it did for Rob on msm.
>
> In which case, apart from the admgpu work, it would just be a matter
> of me tidying that tool a bit and re-sending out for review.
Could you push this to some repository on fdo and send me a link? Going
to pick up this patch here and give it a try, shouldn't be more than a
day of work.
>
>>> * 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?
>
> It should correspond 1:1 with drm_file, since the purpose is to enable
> gputop distinguish between unique open file descriptors (aka clients).
Ah! We do have a 64bit counter for that already because of technical needs.
>
> In theory it could be just a hash value of a struct drm_file pointer
> but that could confuse userspace if the struct gets reused within a
> single userspace sampling period.
>
> Because of that I track it in i915 separately since I wanted to have
> an incrementing cyclic property to it - so that when a fd is closed
> and new opened there is no practical chance they would have the same
> drm-client-id.
>
>>> * drm-engine-capacity - does the concept translate etc.
>>
>> I don't think we are going to need that.
>
> Okay, that one is optional for cases when there is more than one
> engine of a type/class shown under a single entry in fdinfo. So when
> gputop translates accumulated time into percentages it can do the
> right thing. Code can already handle it not being present and assume one.
Yeah, we have that case for a couple of things. The GFX, SDMA and
multimedia engines all have different queues which needs to be accounted
together as far as I can see.
E.g. we have video decode and video encode as two separate rings, but
essentially they use the same engine.
Need to think about how to represent that.
Regards,
Christian.
>
> Regards,
>
> Tvrtko
>
>>
>> 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);
>>> }
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC] drm/amdgpu: Convert to common fdinfo format
2022-05-10 11:26 ` Christian König
@ 2022-05-10 12:56 ` Tvrtko Ursulin
0 siblings, 0 replies; 5+ messages in thread
From: Tvrtko Ursulin @ 2022-05-10 12:56 UTC (permalink / raw)
To: Christian König, amd-gfx
Cc: dri-devel, Christian König, Daniel Vetter, Tvrtko Ursulin
On 10/05/2022 12:26, Christian König wrote:
> Am 10.05.22 um 12:50 schrieb Tvrtko Ursulin:
>>
>> Hi,
>>
>> On 10/05/2022 09:48, Christian König wrote:
>>> 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.
>>
>> And how to motivate someone on your side to pick up the amdgpu work? :)
>
> Well if we could get more of my TTM/DRM patches reviewed I could have
> same free time to do this :)
Yeah, we have a bunch of folks dedicatedly working on TTM and scheduling
refactoring so I am hoping they will notice and get involved.
>>>> * 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.
>>
>> If amdgpu could export accumulated time context spent on engines that
>> would indeed be perfect. It would make the gputop I sketched out most
>> probably just work, as it did for Rob on msm.
>>
>> In which case, apart from the admgpu work, it would just be a matter
>> of me tidying that tool a bit and re-sending out for review.
>
> Could you push this to some repository on fdo and send me a link? Going
> to pick up this patch here and give it a try, shouldn't be more than a
> day of work.
Done, https://cgit.freedesktop.org/~tursulin/intel-gpu-tools/log/?h=gputop.
For extra reference the msm patch was this:
https://lore.kernel.org/lkml/20220225202614.225197-3-robdclark@gmail.com/
If you can expose the same fields gputtop should work.
>>>> * 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?
>>
>> It should correspond 1:1 with drm_file, since the purpose is to enable
>> gputop distinguish between unique open file descriptors (aka clients).
>
> Ah! We do have a 64bit counter for that already because of technical needs.
>
>>
>> In theory it could be just a hash value of a struct drm_file pointer
>> but that could confuse userspace if the struct gets reused within a
>> single userspace sampling period.
>>
>> Because of that I track it in i915 separately since I wanted to have
>> an incrementing cyclic property to it - so that when a fd is closed
>> and new opened there is no practical chance they would have the same
>> drm-client-id.
>>
>>>> * drm-engine-capacity - does the concept translate etc.
>>>
>>> I don't think we are going to need that.
>>
>> Okay, that one is optional for cases when there is more than one
>> engine of a type/class shown under a single entry in fdinfo. So when
>> gputop translates accumulated time into percentages it can do the
>> right thing. Code can already handle it not being present and assume one.
>
> Yeah, we have that case for a couple of things. The GFX, SDMA and
> multimedia engines all have different queues which needs to be accounted
> together as far as I can see.
>
> E.g. we have video decode and video encode as two separate rings, but
> essentially they use the same engine.
>
> Need to think about how to represent that.
I think you have some freedom there as to what to export - whether the
entity userspace submits to (is this ring in amdgpu?), or the entity
hardware actually executes on (your engine?).
We have somewhat similar setup in i915 and I decided to expose the
former. This makes it both (almost) match what our total metrics show
(engine performance counters exported via perf/pmu).
So in case of your video decode and encode rings which lead to the same
hw engine, that would mean exposing them as two entities, decode and encode.
But as said, my spec does prescribe that so it is up to implementations.
As long as it is useful for users as first port of enquiry for
performance problems I think it is fine.
Analogue would be hyper-threading from the CPU scheduling world and how
top(1) cannot distinguish why one core at 0% does not mean there is half
of the performance still one the table.
And then for a deeper down into performance more specialized GPU
profiling tools are required.
Regards,
Tvrtko
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-05-10 12:56 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-10 8:23 [RFC] drm/amdgpu: Convert to common fdinfo format Tvrtko Ursulin
2022-05-10 8:48 ` Christian König
2022-05-10 10:50 ` Tvrtko Ursulin
2022-05-10 11:26 ` Christian König
2022-05-10 12:56 ` Tvrtko Ursulin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).