[PATCH 1/3] drm: Extend DEFINE_DRM_GEM_FOPS() for optional fops

[PATCH 1/3] drm: Extend DEFINE_DRM_GEM_FOPS() for optional fops

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Extend the helper macro so we don't have to throw it away if driver adds
support for optional fops, like show_fdinfo().

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 include/drm/drm_gem.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index 35e7f44c2a75..987e78b18244 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -327,7 +327,7 @@ struct drm_gem_object {
  * non-static version of this you're probably doing it wrong and will break the
  * THIS_MODULE reference by accident.
  */
-#define DEFINE_DRM_GEM_FOPS(name) \
+#define DEFINE_DRM_GEM_FOPS(name, ...) \
 	static const struct file_operations name = {\
 		.owner		= THIS_MODULE,\
 		.open		= drm_open,\
@@ -338,6 +338,7 @@ struct drm_gem_object {
 		.read		= drm_read,\
 		.llseek		= noop_llseek,\
 		.mmap		= drm_gem_mmap,\
+		##__VA_ARGS__\
 	}
 
 void drm_gem_object_release(struct drm_gem_object *obj);
-- 
2.35.1

[PATCH 2/3] drm/msm: Remove unused field in submit

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Noticed this was unused and never set.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/msm_gem.h | 1 -
 drivers/gpu/drm/msm/msm_gpu.h | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index af612add5264..58e11c282928 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -329,7 +329,6 @@ struct msm_gem_submit {
 	bool valid;         /* true if no cmdstream patching needed */
 	bool in_rb;         /* "sudo" mode, copy cmds into RB */
 	struct msm_ringbuffer *ring;
-	struct msm_file_private *ctx;
 	unsigned int nr_cmds;
 	unsigned int nr_bos;
 	u32 ident;	   /* A "identifier" for the submit for logging */
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index c99627fc99dd..696e2ed8a236 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -21,6 +21,7 @@
 struct msm_gem_submit;
 struct msm_gpu_perfcntr;
 struct msm_gpu_state;
+struct msm_file_private;
 
 struct msm_gpu_config {
 	const char *ioname;
-- 
2.35.1

[PATCH 3/3] drm/msm: Expose client engine utilization via fdinfo

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Similar to AMD commit
874442541133 ("drm/amdgpu: Add show_fdinfo() interface"), using the
infrastructure added in previous patches, we add basic client info
and GPU engine utilisation for msm.

Example output:

	# cat /proc/`pgrep glmark2`/fdinfo/6
	pos:	0
	flags:	02400002
	mnt_id:	21
	ino:	162
	drm-driver:	msm
	drm-client-id:	7
	drm-engine-gpu:	1734371319 ns
	drm-cycles-gpu:	1153645024

See also: https://patchwork.freedesktop.org/patch/468505/

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 17 ++++++++++++++++-
 drivers/gpu/drm/msm/msm_gpu.c | 20 ++++++++++++++++++--
 drivers/gpu/drm/msm/msm_gpu.h | 19 +++++++++++++++++++
 3 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 16f37f3d9061..fdf401e6f09e 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -911,7 +911,22 @@ static const struct drm_ioctl_desc msm_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_QUERY, msm_ioctl_submitqueue_query, DRM_RENDER_ALLOW),
 };
 
-DEFINE_DRM_GEM_FOPS(fops);
+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 drm_printer p = drm_seq_file_printer(m);
+
+	if (!priv->gpu)
+		return;
+
+	msm_gpu_show_fdinfo(priv->gpu, file->driver_priv, &p);
+}
+
+DEFINE_DRM_GEM_FOPS(fops,
+	.show_fdinfo = msm_fop_show_fdinfo,
+);
 
 static const struct drm_driver msm_driver = {
 	.driver_features    = DRIVER_GEM |
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 173ebd449f2f..6302f3fe564b 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -4,6 +4,8 @@
  * Author: Rob Clark <robdclark@gmail.com>
  */
 
+#include "drm/drm_drv.h"
+
 #include "msm_gpu.h"
 #include "msm_gem.h"
 #include "msm_mmu.h"
@@ -146,6 +148,15 @@ int msm_gpu_pm_suspend(struct msm_gpu *gpu)
 	return 0;
 }
 
+void msm_gpu_show_fdinfo(struct msm_gpu *gpu, struct msm_file_private *ctx,
+			 struct drm_printer *p)
+{
+	drm_printf(p, "drm-driver:\t%s\n", gpu->dev->driver->name);
+	drm_printf(p, "drm-client-id:\t%u\n", ctx->seqno);
+	drm_printf(p, "drm-engine-gpu:\t%llu ns\n", ctx->elapsed_ns);
+	drm_printf(p, "drm-cycles-gpu:\t%llu\n", ctx->cycles);
+}
+
 int msm_gpu_hw_init(struct msm_gpu *gpu)
 {
 	int ret;
@@ -643,7 +654,7 @@ static void retire_submit(struct msm_gpu *gpu, struct msm_ringbuffer *ring,
 {
 	int index = submit->seqno % MSM_GPU_SUBMIT_STATS_COUNT;
 	volatile struct msm_gpu_submit_stats *stats;
-	u64 elapsed, clock = 0;
+	u64 elapsed, clock = 0, cycles;
 	unsigned long flags;
 
 	stats = &ring->memptrs->stats[index];
@@ -651,12 +662,17 @@ static void retire_submit(struct msm_gpu *gpu, struct msm_ringbuffer *ring,
 	elapsed = (stats->alwayson_end - stats->alwayson_start) * 10000;
 	do_div(elapsed, 192);
 
+	cycles = stats->cpcycles_end - stats->cpcycles_start;
+
 	/* Calculate the clock frequency from the number of CP cycles */
 	if (elapsed) {
-		clock = (stats->cpcycles_end - stats->cpcycles_start) * 1000;
+		clock = cycles * 1000;
 		do_div(clock, elapsed);
 	}
 
+	submit->queue->ctx->elapsed_ns += elapsed;
+	submit->queue->ctx->cycles     += cycles;
+
 	trace_msm_gpu_submit_retired(submit, elapsed, clock,
 		stats->alwayson_start, stats->alwayson_end);
 
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index 696e2ed8a236..ad4fe05dee03 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -328,6 +328,22 @@ struct msm_file_private {
 	struct kref ref;
 	int seqno;
 
+	/**
+	 * elapsed:
+	 *
+	 * The total (cumulative) elapsed time GPU was busy with rendering
+	 * from this context in ns.
+	 */
+	uint64_t elapsed_ns;
+
+	/**
+	 * cycles:
+	 *
+	 * The total (cumulative) GPU cycles elapsed attributed to this
+	 * context.
+	 */
+	uint64_t cycles;
+
 	/**
 	 * entities:
 	 *
@@ -511,6 +527,9 @@ static inline void gpu_write64(struct msm_gpu *gpu, u32 lo, u32 hi, u64 val)
 int msm_gpu_pm_suspend(struct msm_gpu *gpu);
 int msm_gpu_pm_resume(struct msm_gpu *gpu);
 
+void msm_gpu_show_fdinfo(struct msm_gpu *gpu, struct msm_file_private *ctx,
+			 struct drm_printer *p);
+
 int msm_submitqueue_init(struct drm_device *drm, struct msm_file_private *ctx);
 struct msm_gpu_submitqueue *msm_submitqueue_get(struct msm_file_private *ctx,
 		u32 id);
-- 
2.35.1

Re: [PATCH 3/3] drm/msm: Expose client engine utilization via fdinfo

[Rob Clark]

On Fri, Feb 25, 2022 at 12:25 PM Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> Similar to AMD commit
> 874442541133 ("drm/amdgpu: Add show_fdinfo() interface"), using the
> infrastructure added in previous patches, we add basic client info
> and GPU engine utilisation for msm.
>
> Example output:
>
>         # cat /proc/`pgrep glmark2`/fdinfo/6
>         pos:    0
>         flags:  02400002
>         mnt_id: 21
>         ino:    162
>         drm-driver:     msm
>         drm-client-id:  7
>         drm-engine-gpu: 1734371319 ns
>         drm-cycles-gpu: 1153645024

Note that it might be useful to have a standardized way to report # of
cycles and max freq, so userspace tool can derive %utilization in
addition to just %busy

BR,
-R

>
> See also: https://patchwork.freedesktop.org/patch/468505/
>
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>  drivers/gpu/drm/msm/msm_drv.c | 17 ++++++++++++++++-
>  drivers/gpu/drm/msm/msm_gpu.c | 20 ++++++++++++++++++--
>  drivers/gpu/drm/msm/msm_gpu.h | 19 +++++++++++++++++++
>  3 files changed, 53 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 16f37f3d9061..fdf401e6f09e 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -911,7 +911,22 @@ static const struct drm_ioctl_desc msm_ioctls[] = {
>         DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_QUERY, msm_ioctl_submitqueue_query, DRM_RENDER_ALLOW),
>  };
>
> -DEFINE_DRM_GEM_FOPS(fops);
> +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 drm_printer p = drm_seq_file_printer(m);
> +
> +       if (!priv->gpu)
> +               return;
> +
> +       msm_gpu_show_fdinfo(priv->gpu, file->driver_priv, &p);
> +}
> +
> +DEFINE_DRM_GEM_FOPS(fops,
> +       .show_fdinfo = msm_fop_show_fdinfo,
> +);
>
>  static const struct drm_driver msm_driver = {
>         .driver_features    = DRIVER_GEM |
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 173ebd449f2f..6302f3fe564b 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -4,6 +4,8 @@
>   * Author: Rob Clark <robdclark@gmail.com>
>   */
>
> +#include "drm/drm_drv.h"
> +
>  #include "msm_gpu.h"
>  #include "msm_gem.h"
>  #include "msm_mmu.h"
> @@ -146,6 +148,15 @@ int msm_gpu_pm_suspend(struct msm_gpu *gpu)
>         return 0;
>  }
>
> +void msm_gpu_show_fdinfo(struct msm_gpu *gpu, struct msm_file_private *ctx,
> +                        struct drm_printer *p)
> +{
> +       drm_printf(p, "drm-driver:\t%s\n", gpu->dev->driver->name);
> +       drm_printf(p, "drm-client-id:\t%u\n", ctx->seqno);
> +       drm_printf(p, "drm-engine-gpu:\t%llu ns\n", ctx->elapsed_ns);
> +       drm_printf(p, "drm-cycles-gpu:\t%llu\n", ctx->cycles);
> +}
> +
>  int msm_gpu_hw_init(struct msm_gpu *gpu)
>  {
>         int ret;
> @@ -643,7 +654,7 @@ static void retire_submit(struct msm_gpu *gpu, struct msm_ringbuffer *ring,
>  {
>         int index = submit->seqno % MSM_GPU_SUBMIT_STATS_COUNT;
>         volatile struct msm_gpu_submit_stats *stats;
> -       u64 elapsed, clock = 0;
> +       u64 elapsed, clock = 0, cycles;
>         unsigned long flags;
>
>         stats = &ring->memptrs->stats[index];
> @@ -651,12 +662,17 @@ static void retire_submit(struct msm_gpu *gpu, struct msm_ringbuffer *ring,
>         elapsed = (stats->alwayson_end - stats->alwayson_start) * 10000;
>         do_div(elapsed, 192);
>
> +       cycles = stats->cpcycles_end - stats->cpcycles_start;
> +
>         /* Calculate the clock frequency from the number of CP cycles */
>         if (elapsed) {
> -               clock = (stats->cpcycles_end - stats->cpcycles_start) * 1000;
> +               clock = cycles * 1000;
>                 do_div(clock, elapsed);
>         }
>
> +       submit->queue->ctx->elapsed_ns += elapsed;
> +       submit->queue->ctx->cycles     += cycles;
> +
>         trace_msm_gpu_submit_retired(submit, elapsed, clock,
>                 stats->alwayson_start, stats->alwayson_end);
>
> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> index 696e2ed8a236..ad4fe05dee03 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.h
> +++ b/drivers/gpu/drm/msm/msm_gpu.h
> @@ -328,6 +328,22 @@ struct msm_file_private {
>         struct kref ref;
>         int seqno;
>
> +       /**
> +        * elapsed:
> +        *
> +        * The total (cumulative) elapsed time GPU was busy with rendering
> +        * from this context in ns.
> +        */
> +       uint64_t elapsed_ns;
> +
> +       /**
> +        * cycles:
> +        *
> +        * The total (cumulative) GPU cycles elapsed attributed to this
> +        * context.
> +        */
> +       uint64_t cycles;
> +
>         /**
>          * entities:
>          *
> @@ -511,6 +527,9 @@ static inline void gpu_write64(struct msm_gpu *gpu, u32 lo, u32 hi, u64 val)
>  int msm_gpu_pm_suspend(struct msm_gpu *gpu);
>  int msm_gpu_pm_resume(struct msm_gpu *gpu);
>
> +void msm_gpu_show_fdinfo(struct msm_gpu *gpu, struct msm_file_private *ctx,
> +                        struct drm_printer *p);
> +
>  int msm_submitqueue_init(struct drm_device *drm, struct msm_file_private *ctx);
>  struct msm_gpu_submitqueue *msm_submitqueue_get(struct msm_file_private *ctx,
>                 u32 id);
> --
> 2.35.1
>

Re: [PATCH 3/3] drm/msm: Expose client engine utilization via fdinfo

[Tvrtko Ursulin]

On 25/02/2022 22:14, Rob Clark wrote:
> On Fri, Feb 25, 2022 at 12:25 PM Rob Clark <robdclark@gmail.com> wrote:
>>
>> From: Rob Clark <robdclark@chromium.org>
>>
>> Similar to AMD commit
>> 874442541133 ("drm/amdgpu: Add show_fdinfo() interface"), using the
>> infrastructure added in previous patches, we add basic client info
>> and GPU engine utilisation for msm.
>>
>> Example output:
>>
>>          # cat /proc/`pgrep glmark2`/fdinfo/6
>>          pos:    0
>>          flags:  02400002
>>          mnt_id: 21
>>          ino:    162
>>          drm-driver:     msm
>>          drm-client-id:  7
>>          drm-engine-gpu: 1734371319 ns
>>          drm-cycles-gpu: 1153645024

Nice, so my vendor agnostic actually worked (with that single fixup of 
accounting for the fact pdev tag is optional)?

> Note that it might be useful to have a standardized way to report # of
> cycles and max freq, so userspace tool can derive %utilization in
> addition to just %busy

How do you define %utilisation vs %busy - I don't exactly follow since I 
see the two as same?

Looking at your patch I guess I don't understand the difference between 
'elapsed' and 'cycles' inside your retire_submit(). Both are scoped to a 
single context and are not global? If 'elapsed' is time context has 
spent on the GPU, cycles isn't the same just in a different unit?

Regards,

Tvrtko

> BR,
> -R
> 
>>
>> See also: https://patchwork.freedesktop.org/patch/468505/
>>
>> Signed-off-by: Rob Clark <robdclark@chromium.org>
>> ---
>>   drivers/gpu/drm/msm/msm_drv.c | 17 ++++++++++++++++-
>>   drivers/gpu/drm/msm/msm_gpu.c | 20 ++++++++++++++++++--
>>   drivers/gpu/drm/msm/msm_gpu.h | 19 +++++++++++++++++++
>>   3 files changed, 53 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
>> index 16f37f3d9061..fdf401e6f09e 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.c
>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>> @@ -911,7 +911,22 @@ static const struct drm_ioctl_desc msm_ioctls[] = {
>>          DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_QUERY, msm_ioctl_submitqueue_query, DRM_RENDER_ALLOW),
>>   };
>>
>> -DEFINE_DRM_GEM_FOPS(fops);
>> +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 drm_printer p = drm_seq_file_printer(m);
>> +
>> +       if (!priv->gpu)
>> +               return;
>> +
>> +       msm_gpu_show_fdinfo(priv->gpu, file->driver_priv, &p);
>> +}
>> +
>> +DEFINE_DRM_GEM_FOPS(fops,
>> +       .show_fdinfo = msm_fop_show_fdinfo,
>> +);
>>
>>   static const struct drm_driver msm_driver = {
>>          .driver_features    = DRIVER_GEM |
>> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
>> index 173ebd449f2f..6302f3fe564b 100644
>> --- a/drivers/gpu/drm/msm/msm_gpu.c
>> +++ b/drivers/gpu/drm/msm/msm_gpu.c
>> @@ -4,6 +4,8 @@
>>    * Author: Rob Clark <robdclark@gmail.com>
>>    */
>>
>> +#include "drm/drm_drv.h"
>> +
>>   #include "msm_gpu.h"
>>   #include "msm_gem.h"
>>   #include "msm_mmu.h"
>> @@ -146,6 +148,15 @@ int msm_gpu_pm_suspend(struct msm_gpu *gpu)
>>          return 0;
>>   }
>>
>> +void msm_gpu_show_fdinfo(struct msm_gpu *gpu, struct msm_file_private *ctx,
>> +                        struct drm_printer *p)
>> +{
>> +       drm_printf(p, "drm-driver:\t%s\n", gpu->dev->driver->name);
>> +       drm_printf(p, "drm-client-id:\t%u\n", ctx->seqno);
>> +       drm_printf(p, "drm-engine-gpu:\t%llu ns\n", ctx->elapsed_ns);
>> +       drm_printf(p, "drm-cycles-gpu:\t%llu\n", ctx->cycles);
>> +}
>> +
>>   int msm_gpu_hw_init(struct msm_gpu *gpu)
>>   {
>>          int ret;
>> @@ -643,7 +654,7 @@ static void retire_submit(struct msm_gpu *gpu, struct msm_ringbuffer *ring,
>>   {
>>          int index = submit->seqno % MSM_GPU_SUBMIT_STATS_COUNT;
>>          volatile struct msm_gpu_submit_stats *stats;
>> -       u64 elapsed, clock = 0;
>> +       u64 elapsed, clock = 0, cycles;
>>          unsigned long flags;
>>
>>          stats = &ring->memptrs->stats[index];
>> @@ -651,12 +662,17 @@ static void retire_submit(struct msm_gpu *gpu, struct msm_ringbuffer *ring,
>>          elapsed = (stats->alwayson_end - stats->alwayson_start) * 10000;
>>          do_div(elapsed, 192);
>>
>> +       cycles = stats->cpcycles_end - stats->cpcycles_start;
>> +
>>          /* Calculate the clock frequency from the number of CP cycles */
>>          if (elapsed) {
>> -               clock = (stats->cpcycles_end - stats->cpcycles_start) * 1000;
>> +               clock = cycles * 1000;
>>                  do_div(clock, elapsed);
>>          }
>>
>> +       submit->queue->ctx->elapsed_ns += elapsed;
>> +       submit->queue->ctx->cycles     += cycles;
>> +
>>          trace_msm_gpu_submit_retired(submit, elapsed, clock,
>>                  stats->alwayson_start, stats->alwayson_end);
>>
>> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
>> index 696e2ed8a236..ad4fe05dee03 100644
>> --- a/drivers/gpu/drm/msm/msm_gpu.h
>> +++ b/drivers/gpu/drm/msm/msm_gpu.h
>> @@ -328,6 +328,22 @@ struct msm_file_private {
>>          struct kref ref;
>>          int seqno;
>>
>> +       /**
>> +        * elapsed:
>> +        *
>> +        * The total (cumulative) elapsed time GPU was busy with rendering
>> +        * from this context in ns.
>> +        */
>> +       uint64_t elapsed_ns;
>> +
>> +       /**
>> +        * cycles:
>> +        *
>> +        * The total (cumulative) GPU cycles elapsed attributed to this
>> +        * context.
>> +        */
>> +       uint64_t cycles;
>> +
>>          /**
>>           * entities:
>>           *
>> @@ -511,6 +527,9 @@ static inline void gpu_write64(struct msm_gpu *gpu, u32 lo, u32 hi, u64 val)
>>   int msm_gpu_pm_suspend(struct msm_gpu *gpu);
>>   int msm_gpu_pm_resume(struct msm_gpu *gpu);
>>
>> +void msm_gpu_show_fdinfo(struct msm_gpu *gpu, struct msm_file_private *ctx,
>> +                        struct drm_printer *p);
>> +
>>   int msm_submitqueue_init(struct drm_device *drm, struct msm_file_private *ctx);
>>   struct msm_gpu_submitqueue *msm_submitqueue_get(struct msm_file_private *ctx,
>>                  u32 id);
>> --
>> 2.35.1
>>

Re: [PATCH 3/3] drm/msm: Expose client engine utilization via fdinfo

[Rob Clark]

On Mon, Feb 28, 2022 at 6:33 AM Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> On 25/02/2022 22:14, Rob Clark wrote:
> > On Fri, Feb 25, 2022 at 12:25 PM Rob Clark <robdclark@gmail.com> wrote:
> >>
> >> From: Rob Clark <robdclark@chromium.org>
> >>
> >> Similar to AMD commit
> >> 874442541133 ("drm/amdgpu: Add show_fdinfo() interface"), using the
> >> infrastructure added in previous patches, we add basic client info
> >> and GPU engine utilisation for msm.
> >>
> >> Example output:
> >>
> >>          # cat /proc/`pgrep glmark2`/fdinfo/6
> >>          pos:    0
> >>          flags:  02400002
> >>          mnt_id: 21
> >>          ino:    162
> >>          drm-driver:     msm
> >>          drm-client-id:  7
> >>          drm-engine-gpu: 1734371319 ns
> >>          drm-cycles-gpu: 1153645024
>
> Nice, so my vendor agnostic actually worked (with that single fixup of
> accounting for the fact pdev tag is optional)?
>
> > Note that it might be useful to have a standardized way to report # of
> > cycles and max freq, so userspace tool can derive %utilization in
> > addition to just %busy
>
> How do you define %utilisation vs %busy - I don't exactly follow since I
> see the two as same?

so, say you are running at 50% of max clk, and gpu is busy 70% of the
time.  The utilization is only 35% because the gpu could scale up the
clk to get more work done.

> Looking at your patch I guess I don't understand the difference between
> 'elapsed' and 'cycles' inside your retire_submit(). Both are scoped to a
> single context and are not global? If 'elapsed' is time context has
> spent on the GPU, cycles isn't the same just in a different unit?

Correct, we capture (from GPU cmdstream) two counters both before and
after a submit (aka execbuf) runs, one is a fixed-rate counter, which
gives us elapsed time.  The second is a counter that increments every
clk cycle, which gives us the # of cycles.  With the two values, we
can calculate GPU frequency.

BR,
-R

> Regards,
>
> Tvrtko
>

Re: [PATCH 3/3] drm/msm: Expose client engine utilization via fdinfo

[Tvrtko Ursulin]

On 28/02/2022 16:01, Rob Clark wrote:
> On Mon, Feb 28, 2022 at 6:33 AM Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>>
>>
>> On 25/02/2022 22:14, Rob Clark wrote:
>>> On Fri, Feb 25, 2022 at 12:25 PM Rob Clark <robdclark@gmail.com> wrote:
>>>>
>>>> From: Rob Clark <robdclark@chromium.org>
>>>>
>>>> Similar to AMD commit
>>>> 874442541133 ("drm/amdgpu: Add show_fdinfo() interface"), using the
>>>> infrastructure added in previous patches, we add basic client info
>>>> and GPU engine utilisation for msm.
>>>>
>>>> Example output:
>>>>
>>>>           # cat /proc/`pgrep glmark2`/fdinfo/6
>>>>           pos:    0
>>>>           flags:  02400002
>>>>           mnt_id: 21
>>>>           ino:    162
>>>>           drm-driver:     msm
>>>>           drm-client-id:  7
>>>>           drm-engine-gpu: 1734371319 ns
>>>>           drm-cycles-gpu: 1153645024
>>
>> Nice, so my vendor agnostic actually worked (with that single fixup of
>> accounting for the fact pdev tag is optional)?
>>
>>> Note that it might be useful to have a standardized way to report # of
>>> cycles and max freq, so userspace tool can derive %utilization in
>>> addition to just %busy
>>
>> How do you define %utilisation vs %busy - I don't exactly follow since I
>> see the two as same?
> 
> so, say you are running at 50% of max clk, and gpu is busy 70% of the
> time.  The utilization is only 35% because the gpu could scale up the
> clk to get more work done.

Got it, thanks. I don't think we have the equivalent on the i915 side 
(we do have global frequency reporting via perf/PMU). In general things 
like these I imagined would be defined as driver specific tags. If you 
look at the drm-usage-stats.rst in my series, there is a "Driver 
specific implementations" section in there which links to the i915 doc 
section.

I've also put a "big fat" comment into the i915 fdinfo fops vfunc 
pointing back to drm-usage-stats.rst which I think is useful when large 
teams work on a driver. Not sure if that applies to msm so just mentioning.

Since this all works for you, would you mind applying your ack against 
20220222140422.1121163-9-tvrtko.ursulin@linux.intel.com?

I need to get some updates r-b's for my series and then I submit it 
again to Dave and Daniel for final acks.

After that, for a 2nd/follow-up phase, I plan to re-surrect the amdgpu 
patch I had to make it compliant to common format, plus document the 
option of engine utilisation tag being in percentage units as exposed 
from that driver. And extending gputop to support that as well.

Regards,

Tvrtko

>> Looking at your patch I guess I don't understand the difference between
>> 'elapsed' and 'cycles' inside your retire_submit(). Both are scoped to a
>> single context and are not global? If 'elapsed' is time context has
>> spent on the GPU, cycles isn't the same just in a different unit?
> 
> Correct, we capture (from GPU cmdstream) two counters both before and
> after a submit (aka execbuf) runs, one is a fixed-rate counter, which
> gives us elapsed time.  The second is a counter that increments every
> clk cycle, which gives us the # of cycles.  With the two values, we
> can calculate GPU frequency.
> 
> BR,
> -R
> 
>> Regards,
>>
>> Tvrtko
>>

Re: [PATCH 1/3] drm: Extend DEFINE_DRM_GEM_FOPS() for optional fops

[Ville Syrjälä]

On Fri, Feb 25, 2022 at 12:26:12PM -0800, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> Extend the helper macro so we don't have to throw it away if driver adds
> support for optional fops, like show_fdinfo().
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>  include/drm/drm_gem.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index 35e7f44c2a75..987e78b18244 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -327,7 +327,7 @@ struct drm_gem_object {
>   * non-static version of this you're probably doing it wrong and will break the
>   * THIS_MODULE reference by accident.
>   */
> -#define DEFINE_DRM_GEM_FOPS(name) \
> +#define DEFINE_DRM_GEM_FOPS(name, ...) \
>  	static const struct file_operations name = {\
>  		.owner		= THIS_MODULE,\
>  		.open		= drm_open,\
> @@ -338,6 +338,7 @@ struct drm_gem_object {
>  		.read		= drm_read,\
>  		.llseek		= noop_llseek,\
>  		.mmap		= drm_gem_mmap,\
> +		##__VA_ARGS__\
>  	}

Would it not be less convoluted to make the macro only provide
the initializers? So you'd get something like:

static const struct file_operations foo = {
	DRM_GEM_FOPS,
	.my_stuff = whatever,
};

>  
>  void drm_gem_object_release(struct drm_gem_object *obj);
> -- 
> 2.35.1

-- 
Ville Syrjälä
Intel

Re: [Freedreno] [PATCH 1/3] drm: Extend DEFINE_DRM_GEM_FOPS() for optional fops

[Rob Clark]

On Fri, Feb 25, 2022 at 12:36 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Fri, Feb 25, 2022 at 12:26:12PM -0800, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > Extend the helper macro so we don't have to throw it away if driver adds
> > support for optional fops, like show_fdinfo().
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> >  include/drm/drm_gem.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > index 35e7f44c2a75..987e78b18244 100644
> > --- a/include/drm/drm_gem.h
> > +++ b/include/drm/drm_gem.h
> > @@ -327,7 +327,7 @@ struct drm_gem_object {
> >   * non-static version of this you're probably doing it wrong and will break the
> >   * THIS_MODULE reference by accident.
> >   */
> > -#define DEFINE_DRM_GEM_FOPS(name) \
> > +#define DEFINE_DRM_GEM_FOPS(name, ...) \
> >       static const struct file_operations name = {\
> >               .owner          = THIS_MODULE,\
> >               .open           = drm_open,\
> > @@ -338,6 +338,7 @@ struct drm_gem_object {
> >               .read           = drm_read,\
> >               .llseek         = noop_llseek,\
> >               .mmap           = drm_gem_mmap,\
> > +             ##__VA_ARGS__\
> >       }
>
> Would it not be less convoluted to make the macro only provide
> the initializers? So you'd get something like:
>
> static const struct file_operations foo = {
>         DRM_GEM_FOPS,
>         .my_stuff = whatever,
> };
>

Hmm, I like my color of the bikeshed, but I guess it is a matter of opinion ;-)

BR,
-R

Re: [Freedreno] [PATCH 1/3] drm: Extend DEFINE_DRM_GEM_FOPS() for optional fops

[Sam Ravnborg]

Hi Rob,

> > > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > > index 35e7f44c2a75..987e78b18244 100644
> > > --- a/include/drm/drm_gem.h
> > > +++ b/include/drm/drm_gem.h
> > > @@ -327,7 +327,7 @@ struct drm_gem_object {
> > >   * non-static version of this you're probably doing it wrong and will break the
> > >   * THIS_MODULE reference by accident.
> > >   */
> > > -#define DEFINE_DRM_GEM_FOPS(name) \
> > > +#define DEFINE_DRM_GEM_FOPS(name, ...) \
> > >       static const struct file_operations name = {\
> > >               .owner          = THIS_MODULE,\
> > >               .open           = drm_open,\
> > > @@ -338,6 +338,7 @@ struct drm_gem_object {
> > >               .read           = drm_read,\
> > >               .llseek         = noop_llseek,\
> > >               .mmap           = drm_gem_mmap,\
> > > +             ##__VA_ARGS__\
> > >       }
> >
> > Would it not be less convoluted to make the macro only provide
> > the initializers? So you'd get something like:
> >
> > static const struct file_operations foo = {
> >         DRM_GEM_FOPS,
> >         .my_stuff = whatever,
> > };
> >
> 
> Hmm, I like my color of the bikeshed, but I guess it is a matter of opinion ;-)
Or less surprise. Most similar macros provides initializers only.

Try "git grep DRM_.*OPS  | grep define" in include/drm
and take a look at the hits.

	Sam