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

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