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

[PATCH 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>
---
 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 76fd2904c7c6..24c77a6a157f 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -847,6 +847,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 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

[PATCH 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>
---
 drivers/gpu/drm/etnaviv/etnaviv_drv.c | 38 ++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index b69edb40ae2a..11b1f11fcb58 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,42 @@ 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;
+	struct drm_printer p = drm_seq_file_printer(m);
+	int i;
+
+	drm_printf(&p, "drm-driver:\t%s\n", dev->driver->name);
+	drm_printf(&p, "drm-client-id:\t%u\n", ctx->id);
+
+	for (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 ? "/" : "");
+
+		drm_printf(&p, "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 1/3] drm/scheduler: track GPU active time per entity

[Andrey Grodzovsky]

On 2022-09-08 14:10, Lucas Stach wrote:
> 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>
> ---
>   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 76fd2904c7c6..24c77a6a157f 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -847,6 +847,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;


Looks like you making as assumption that drm_sched_entity will always be 
allocated using kzalloc ? Isn't it a bit dangerous assumption ?

Andrey


>   }
>   
> 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;
>   };
>   
>   /**

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

[Lucas Stach]

Am Donnerstag, dem 08.09.2022 um 14:33 -0400 schrieb Andrey Grodzovsky:
> On 2022-09-08 14:10, Lucas Stach wrote:
> > 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>
> > ---
> >   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 76fd2904c7c6..24c77a6a157f 100644
> > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > @@ -847,6 +847,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;
> 
> 
> Looks like you making as assumption that drm_sched_entity will always be 
> allocated using kzalloc ? Isn't it a bit dangerous assumption ?
> 
No, drm_sched_entity_init() memsets the whole struct to 0 before
initializing any members that need more specific init values.

Regards,
Lucas

> Andrey
> 
> 
> >   }
> >   
> > 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;
> >   };
> >   
> >   /**

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

[Tvrtko Ursulin]

Hi Lucas,

On 08/09/2022 19:10, 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>
> ---
>   drivers/gpu/drm/etnaviv/etnaviv_drv.c | 38 ++++++++++++++++++++++++++-
>   1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index b69edb40ae2a..11b1f11fcb58 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,42 @@ 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;
> +	struct drm_printer p = drm_seq_file_printer(m);

Any specific reason not to use seq_printf directly? (May be my ignorance.)

> +	int i;
> +
> +	drm_printf(&p, "drm-driver:\t%s\n", dev->driver->name);
> +	drm_printf(&p, "drm-client-id:\t%u\n", ctx->id);
> +
> +	for (i = 0; i < ETNA_MAX_PIPES; i++) {
> +                struct etnaviv_gpu *gpu = priv->gpu[i];
> +		char engine[8];
> +		int cur = 0;

Alignment renders odd in my client.

> +
> +		if (!gpu)
> +			continue;

I'd stick a comment in here to the effect of "For text output format 
description please see drm-usage-stats.rst!".

Just to leave a breadcrumb putting some restraint on adding vendor 
specific things which may be already covered by the common spec. To help 
with common tools in the future as much as possible.

> +
> +		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 ? "/" : "");
> +
> +		drm_printf(&p, "drm-engine-%s:\t%llu ns\n", engine,
> +			   ctx->sched_entity[i].elapsed_ns);

Two questions:

1)
So you have max four pipes, each can be either only 2d, 3d, or 2d/3d? 
Can you have multiple of the same like 2x 3D? If you do, have you 
considered exporting one drm-engine-% together with drm-engine-capacity- 
for it?

2)
Have you tried my, yet unmerged, vendor agnostic gputop tool with your 
changes?

https://patchwork.freedesktop.org/series/102175/

It would be interesting to know if it works.

Regards,

Tvrtko

> +	}
> +}
> +
> +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,

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

[Lucas Stach]

Hi Tvrtko,

Am Freitag, dem 16.09.2022 um 10:31 +0100 schrieb Tvrtko Ursulin:
> Hi Lucas,
> 
> On 08/09/2022 19:10, 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>
> > ---
> >   drivers/gpu/drm/etnaviv/etnaviv_drv.c | 38 ++++++++++++++++++++++++++-
> >   1 file changed, 37 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> > index b69edb40ae2a..11b1f11fcb58 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,42 @@ 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;
> > +	struct drm_printer p = drm_seq_file_printer(m);
> 
> Any specific reason not to use seq_printf directly? (May be my ignorance.)
> 
Not really, I just followed what msm was doing here.

> > +	int i;
> > +
> > +	drm_printf(&p, "drm-driver:\t%s\n", dev->driver->name);
> > +	drm_printf(&p, "drm-client-id:\t%u\n", ctx->id);
> > +
> > +	for (i = 0; i < ETNA_MAX_PIPES; i++) {
> > +                struct etnaviv_gpu *gpu = priv->gpu[i];
> > +		char engine[8];
> > +		int cur = 0;
> 
> Alignment renders odd in my client.

I'll check that, might have messed up here.
> 
> > +
> > +		if (!gpu)
> > +			continue;
> 
> I'd stick a comment in here to the effect of "For text output format 
> description please see drm-usage-stats.rst!".
> 
> Just to leave a breadcrumb putting some restraint on adding vendor 
> specific things which may be already covered by the common spec. To help 
> with common tools in the future as much as possible.

Yea, it was pretty to clear to me that we want the common format as
much as possible when writing the patches, but it's a good idea to add
a pointer for the future reader.

> 
> > +
> > +		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 ? "/" : "");
> > +
> > +		drm_printf(&p, "drm-engine-%s:\t%llu ns\n", engine,
> > +			   ctx->sched_entity[i].elapsed_ns);
> 
> Two questions:
> 
> 1)
> So you have max four pipes, each can be either only 2d, 3d, or 2d/3d? 
> Can you have multiple of the same like 2x 3D? If you do, have you 
> considered exporting one drm-engine-% together with drm-engine-capacity- 
> for it?
> 
The four pipes is a arbitrary driver limit. Etnaviv is a bit special in
that it collects all Vivante GPUs present in a system into a single DRM
device, each of those GPUs can be either 2D, 3D or a combined core with
both 2D and 3D capabilities. In theory there could be multiple GPUs of
each kind, but for now all real SoC designs we've come across only had
a single one of each kind.

When we add support for a SoC that has multiple GPUs of one kind, I
think exposing drm-engine-capacity, together with hooking them up to
the load balancing in the scheduler is the right thing to do.

> 2)
> Have you tried my, yet unmerged, vendor agnostic gputop tool with your 
> changes?
> 
> https://patchwork.freedesktop.org/series/102175/
> 
> It would be interesting to know if it works.
> 
Yes, I did when working on this series. I had some crashes related to
(I believe) double frees in the DRM client discovery, which I hadn't
had time to investigate further. Seems there is a race, as I couldn't
reproduce the crash when running with valgrind.

Other than that, the tool works for exposing the per-client GPU load on
etnaviv.

Regards,
Lucas

> Regards,
> 
> Tvrtko
> 
> > +	}
> > +}
> > +
> > +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,

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

[Tvrtko Ursulin]

On 16/09/2022 10:50, Lucas Stach wrote:
> Hi Tvrtko,
> 
> Am Freitag, dem 16.09.2022 um 10:31 +0100 schrieb Tvrtko Ursulin:
>> Hi Lucas,
>>
>> On 08/09/2022 19:10, 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>
>>> ---
>>>    drivers/gpu/drm/etnaviv/etnaviv_drv.c | 38 ++++++++++++++++++++++++++-
>>>    1 file changed, 37 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>>> index b69edb40ae2a..11b1f11fcb58 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,42 @@ 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;
>>> +	struct drm_printer p = drm_seq_file_printer(m);
>>
>> Any specific reason not to use seq_printf directly? (May be my ignorance.)
>>
> Not really, I just followed what msm was doing here.

Right, no strong feelings either way I just did not see the need to wrap 
it for this use case.

>>> +	int i;
>>> +
>>> +	drm_printf(&p, "drm-driver:\t%s\n", dev->driver->name);
>>> +	drm_printf(&p, "drm-client-id:\t%u\n", ctx->id);
>>> +
>>> +	for (i = 0; i < ETNA_MAX_PIPES; i++) {
>>> +                struct etnaviv_gpu *gpu = priv->gpu[i];
>>> +		char engine[8];
>>> +		int cur = 0;
>>
>> Alignment renders odd in my client.
> 
> I'll check that, might have messed up here.
>>
>>> +
>>> +		if (!gpu)
>>> +			continue;
>>
>> I'd stick a comment in here to the effect of "For text output format
>> description please see drm-usage-stats.rst!".
>>
>> Just to leave a breadcrumb putting some restraint on adding vendor
>> specific things which may be already covered by the common spec. To help
>> with common tools in the future as much as possible.
> 
> Yea, it was pretty to clear to me that we want the common format as
> much as possible when writing the patches, but it's a good idea to add
> a pointer for the future reader.

Thanks!

>>> +
>>> +		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 ? "/" : "");
>>> +
>>> +		drm_printf(&p, "drm-engine-%s:\t%llu ns\n", engine,
>>> +			   ctx->sched_entity[i].elapsed_ns);
>>
>> Two questions:
>>
>> 1)
>> So you have max four pipes, each can be either only 2d, 3d, or 2d/3d?
>> Can you have multiple of the same like 2x 3D? If you do, have you
>> considered exporting one drm-engine-% together with drm-engine-capacity-
>> for it?
>>
> The four pipes is a arbitrary driver limit. Etnaviv is a bit special in
> that it collects all Vivante GPUs present in a system into a single DRM
> device, each of those GPUs can be either 2D, 3D or a combined core with
> both 2D and 3D capabilities. In theory there could be multiple GPUs of
> each kind, but for now all real SoC designs we've come across only had
> a single one of each kind.
> 
> When we add support for a SoC that has multiple GPUs of one kind, I
> think exposing drm-engine-capacity, together with hooking them up to
> the load balancing in the scheduler is the right thing to do.
> 
>> 2)
>> Have you tried my, yet unmerged, vendor agnostic gputop tool with your
>> changes?
>>
>> https://patchwork.freedesktop.org/series/102175/
>>
>> It would be interesting to know if it works.
>>
> Yes, I did when working on this series. I had some crashes related to
> (I believe) double frees in the DRM client discovery, which I hadn't
> had time to investigate further. Seems there is a race, as I couldn't
> reproduce the crash when running with valgrind.
> 
> Other than that, the tool works for exposing the per-client GPU load on
> etnaviv.

Cool, at least some success.

Out of curiosity what is the planned consumer in etnaviv landscape?

Regards,

Tvrtko

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

[Andrey Grodzovsky]

On 2022-09-16 05:12, Lucas Stach wrote:
> Am Donnerstag, dem 08.09.2022 um 14:33 -0400 schrieb Andrey Grodzovsky:
>> On 2022-09-08 14:10, Lucas Stach wrote:
>>> 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>
>>> ---
>>>    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 76fd2904c7c6..24c77a6a157f 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>> @@ -847,6 +847,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;
>>
>> Looks like you making as assumption that drm_sched_entity will always be
>> allocated using kzalloc ? Isn't it a bit dangerous assumption ?
>>
> No, drm_sched_entity_init() memsets the whole struct to 0 before
> initializing any members that need more specific init values.
>
> Regards,
> Lucas


Missed the memset, in that case Reviewed-by: Andrey Grodzovsky 
<andrey.grodzovsky@amd.com>

I assume you can push that change yourself with the rest of your patchset ?

Andrey

>
>> Andrey
>>
>>
>>>    }
>>>    
>>> 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;
>>>    };
>>>    
>>>    /**
>