[PATCH v2 1/3] drm/scheduler: track GPU active time per entity

[PATCH v2 1/3] drm/scheduler: track GPU active time per entity

[Lucas Stach]

Track the accumulated time that jobs from this entity were active
on the GPU. This allows drivers using the scheduler to trivially
implement the DRM fdinfo when the hardware doesn't provide more
specific information than signalling job completion anyways.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
Reviewed-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 6 ++++++
 include/drm/gpu_scheduler.h            | 7 +++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 68317d3a7a27..5dbe826d498d 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -852,6 +852,12 @@ drm_sched_get_cleanup_job(struct drm_gpu_scheduler *sched)
 
 	spin_unlock(&sched->job_list_lock);
 
+	if (job) {
+		job->entity->elapsed_ns += ktime_to_ns(
+			ktime_sub(job->s_fence->finished.timestamp,
+				  job->s_fence->scheduled.timestamp));
+	}
+
 	return job;
 }
 
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index addb135eeea6..573bef640664 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -196,6 +196,13 @@ struct drm_sched_entity {
 	 * drm_sched_entity_fini().
 	 */
 	struct completion		entity_idle;
+	/**
+	 * @elapsed_ns
+	 *
+	 * Records the amount of time where jobs from this entity were active
+	 * on the GPU.
+	 */
+	uint64_t elapsed_ns;
 };
 
 /**
-- 
2.30.2

[PATCH v2 2/3] drm/etnaviv: allocate unique ID per drm_file

[Lucas Stach]

Allows to easily track if several fd are pointing to the same
execution context due to being dup'ed.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_drv.c | 3 +++
 drivers/gpu/drm/etnaviv/etnaviv_drv.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 1d2b4fb4bcf8..b69edb40ae2a 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -49,6 +49,7 @@ static void load_gpu(struct drm_device *dev)
 static int etnaviv_open(struct drm_device *dev, struct drm_file *file)
 {
 	struct etnaviv_drm_private *priv = dev->dev_private;
+	static atomic_t ident = ATOMIC_INIT(0);
 	struct etnaviv_file_private *ctx;
 	int ret, i;
 
@@ -56,6 +57,8 @@ static int etnaviv_open(struct drm_device *dev, struct drm_file *file)
 	if (!ctx)
 		return -ENOMEM;
 
+	ctx->id = atomic_inc_return(&ident);
+
 	ctx->mmu = etnaviv_iommu_context_init(priv->mmu_global,
 					      priv->cmdbuf_suballoc);
 	if (!ctx->mmu) {
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
index f32f4771dada..851b4b4ef146 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
@@ -27,6 +27,7 @@ struct etnaviv_iommu_global;
 #define ETNAVIV_SOFTPIN_START_ADDRESS	SZ_4M /* must be >= SUBALLOC_SIZE */
 
 struct etnaviv_file_private {
+	int id;
 	struct etnaviv_iommu_context	*mmu;
 	struct drm_sched_entity		sched_entity[ETNA_MAX_PIPES];
 };
-- 
2.30.2

Re: [PATCH v2 2/3] drm/etnaviv: allocate unique ID per drm_file

[Lucas Stach]

Am Freitag, dem 16.09.2022 um 17:12 +0200 schrieb Lucas Stach:
> Allows to easily track if several fd are pointing to the same
> execution context due to being dup'ed.
> 
I would appreciate it if someone could review path 2 and 3 of this
series. I would like to include it in my next upstream PR.

Regards,
Lucas

> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_drv.c | 3 +++
>  drivers/gpu/drm/etnaviv/etnaviv_drv.h | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index 1d2b4fb4bcf8..b69edb40ae2a 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -49,6 +49,7 @@ static void load_gpu(struct drm_device *dev)
>  static int etnaviv_open(struct drm_device *dev, struct drm_file *file)
>  {
>  	struct etnaviv_drm_private *priv = dev->dev_private;
> +	static atomic_t ident = ATOMIC_INIT(0);
>  	struct etnaviv_file_private *ctx;
>  	int ret, i;
>  
> @@ -56,6 +57,8 @@ static int etnaviv_open(struct drm_device *dev, struct drm_file *file)
>  	if (!ctx)
>  		return -ENOMEM;
>  
> +	ctx->id = atomic_inc_return(&ident);
> +
>  	ctx->mmu = etnaviv_iommu_context_init(priv->mmu_global,
>  					      priv->cmdbuf_suballoc);
>  	if (!ctx->mmu) {
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> index f32f4771dada..851b4b4ef146 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> @@ -27,6 +27,7 @@ struct etnaviv_iommu_global;
>  #define ETNAVIV_SOFTPIN_START_ADDRESS	SZ_4M /* must be >= SUBALLOC_SIZE */
>  
>  struct etnaviv_file_private {
> +	int id;
>  	struct etnaviv_iommu_context	*mmu;
>  	struct drm_sched_entity		sched_entity[ETNA_MAX_PIPES];
>  };

Re: [PATCH v2 2/3] drm/etnaviv: allocate unique ID per drm_file

[Philipp Zabel]

On Fri, Sep 16, 2022 at 05:12:04PM +0200, Lucas Stach wrote:
> Allows to easily track if several fd are pointing to the same
> execution context due to being dup'ed.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_drv.c | 3 +++
>  drivers/gpu/drm/etnaviv/etnaviv_drv.h | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index 1d2b4fb4bcf8..b69edb40ae2a 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -49,6 +49,7 @@ static void load_gpu(struct drm_device *dev)
>  static int etnaviv_open(struct drm_device *dev, struct drm_file *file)
>  {
>  	struct etnaviv_drm_private *priv = dev->dev_private;
> +	static atomic_t ident = ATOMIC_INIT(0);
>  	struct etnaviv_file_private *ctx;
>  	int ret, i;
>  
> @@ -56,6 +57,8 @@ static int etnaviv_open(struct drm_device *dev, struct drm_file *file)
>  	if (!ctx)
>  		return -ENOMEM;
>  
> +	ctx->id = atomic_inc_return(&ident);

I suppose we can ignore that this could theoretically wrap around.

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

Re: [PATCH v2 2/3] drm/etnaviv: allocate unique ID per drm_file

[Tvrtko Ursulin]

On 16/11/2022 13:18, Philipp Zabel wrote:
> On Fri, Sep 16, 2022 at 05:12:04PM +0200, Lucas Stach wrote:
>> Allows to easily track if several fd are pointing to the same
>> execution context due to being dup'ed.
>>
>> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
>> ---
>>   drivers/gpu/drm/etnaviv/etnaviv_drv.c | 3 +++
>>   drivers/gpu/drm/etnaviv/etnaviv_drv.h | 1 +
>>   2 files changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> index 1d2b4fb4bcf8..b69edb40ae2a 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> @@ -49,6 +49,7 @@ static void load_gpu(struct drm_device *dev)
>>   static int etnaviv_open(struct drm_device *dev, struct drm_file *file)
>>   {
>>   	struct etnaviv_drm_private *priv = dev->dev_private;
>> +	static atomic_t ident = ATOMIC_INIT(0);
>>   	struct etnaviv_file_private *ctx;
>>   	int ret, i;
>>   
>> @@ -56,6 +57,8 @@ static int etnaviv_open(struct drm_device *dev, struct drm_file *file)
>>   	if (!ctx)
>>   		return -ENOMEM;
>>   
>> +	ctx->id = atomic_inc_return(&ident);
> 
> I suppose we can ignore that this could theoretically wrap around.

Depends on your usecases - if you can envisage a long running client, 
say the compositor, while other clients come and go then eventually 
these will not be unique and will break the fdinfo spec. Hence I used a 
cyclic xarray in i915 (aka idr). I would recommend you just do that and 
remove future debug sessions around the area of "why does gputop show 
nonsense all of a sudden".

Regards,

Tvrtko

> 
> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> 
> regards
> Philipp

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

[Lucas Stach]

This exposes a accumulated GPU active time per client via the
fdinfo infrastructure.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
v2:
- fix code style
- switch to raw seq_printf
- leave some breadcrumbs about the output format
---
 drivers/gpu/drm/etnaviv/etnaviv_drv.c | 40 ++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index b69edb40ae2a..c08748472f74 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,44 @@ 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;
+
+	/*
+	 * For a description of the text output format used here, see
+	 * Documentation/gpu/drm-usage-stats.rst.
+	 */
+	seq_printf(m, "drm-driver:\t%s\n", dev->driver->name);
+	seq_printf(m, "drm-client-id:\t%u\n", ctx->id);
+
+	for (int i = 0; i < ETNA_MAX_PIPES; i++) {
+		struct etnaviv_gpu *gpu = priv->gpu[i];
+		char engine[8];
+		int cur = 0;
+
+		if (!gpu)
+			continue;
+
+		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 ? "/" : "");
+
+		seq_printf(m, "drm-engine-%s:\t%llu ns\n", engine,
+			   ctx->sched_entity[i].elapsed_ns);
+	}
+}
+
+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,
-- 
2.30.2

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

[Philipp Zabel]

On Fri, Sep 16, 2022 at 05:12:05PM +0200, 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>
> ---
> v2:
> - fix code style
> - switch to raw seq_printf
> - leave some breadcrumbs about the output format
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_drv.c | 40 ++++++++++++++++++++++++++-
>  1 file changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index b69edb40ae2a..c08748472f74 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,44 @@ 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;
> +
> +	/*
> +	 * For a description of the text output format used here, see
> +	 * Documentation/gpu/drm-usage-stats.rst.
> +	 */
> +	seq_printf(m, "drm-driver:\t%s\n", dev->driver->name);
> +	seq_printf(m, "drm-client-id:\t%u\n", ctx->id);
> +
> +	for (int i = 0; i < ETNA_MAX_PIPES; i++) {
> +		struct etnaviv_gpu *gpu = priv->gpu[i];
> +		char engine[8];

Maybe initialize this as well? See below.

> +		int cur = 0;
> +
> +		if (!gpu)
> +			continue;
> +
> +		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 ? "/" : "");

Does the NPU have either bit set? If not, this must not be forgotten to
be changed when NPU support is added, to avoid uninitalized use of the
engine variable.

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Tested-by: Philipp Zabel <p.zabel@pengutronix.de>

with gputop [1].

[1] https://lore.kernel.org/dri-devel/20221111155844.3290531-1-tvrtko.ursulin@linux.intel.com/

regards
Philipp