[PATCH 0/4] drm/panfrost: Plumb cycle counters to userspace

[PATCH 0/4] drm/panfrost: Plumb cycle counters to userspace

[alyssa.rosenzweig]

From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

Mali has hardware cycle counters (and GPU timestamps) available for
profiling. These are exposed in various ways:

- Kernel: As CYCLE_COUNT and TIMESTAMP registers 
- Job chain: As WRITE_VALUE descriptors
- Shader (Midgard): As LD_SPECIAL selectors
- Shader (Bifrost): As the LD_GCLK.u64 instruction

These form building blocks for profiling features, for example the
ARB_shader_clock extension which accesses the counters from an
application's shader.

The counters consume power, so it is recommended to disable the counters
when not in use. To do so, we follow the strategy from mali_kbase: add a
counter requirement to the job, start the counters only when required,
and stop them as quickly as possible.

The new UABI will be used in Mesa. An implementation of ARB_shader_clock
using this UABI is available as a pending upstream merge request [1].
The implementation passes the relevant piglit test, validating both the
kernel and mesa.

The main outstanding questing is the proper name. Performance monitoring
("PERMON") is the name used by kbase, but it's jargon-y and risks
confusion with performance counters, an orthogonal mechanism. Cycle
count is more descriptive and matches the actual hardware name, but
obscures that the same mechanism is required for GPU timestamps. This
bit of bikeshedding aside, I'm pleased with the patches.

[1] https://gitlab.freedesktop.org/mesa/mesa/merge_requests/11051

Alyssa Rosenzweig (4):
  drm/panfrost: Add cycle counter job requirement
  drm/panfrost: Add CYCLE_COUNT_START/STOP commands
  drm/panfrost: Add permon acquire/release helpers
  drm/panfrost: Handle PANFROST_JD_REQ_PERMON

 drivers/gpu/drm/panfrost/panfrost_device.h |  3 +++
 drivers/gpu/drm/panfrost/panfrost_drv.c    | 10 +++++++---
 drivers/gpu/drm/panfrost/panfrost_gpu.c    | 20 ++++++++++++++++++++
 drivers/gpu/drm/panfrost/panfrost_gpu.h    |  3 +++
 drivers/gpu/drm/panfrost/panfrost_job.c    |  6 ++++++
 drivers/gpu/drm/panfrost/panfrost_regs.h   |  2 ++
 include/uapi/drm/panfrost_drm.h            |  3 ++-
 7 files changed, 43 insertions(+), 4 deletions(-)

-- 
2.30.2

[PATCH 1/4] drm/panfrost: Add cycle counter job requirement

[alyssa.rosenzweig]

From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

Extend the Panfrost UABI with a new job requirement for cycle counters
(and GPU timestamps, by extension). This requirement is used in
userspace to implement ARB_shader_clock, an OpenGL extension reporting
the GPU cycle count within a shader. The same mechanism will be required
to implement timestamp queries as a "write value - timestamp" job.

We cannot enable cycle counters unconditionally, as enabling them
increases GPU power consumption. They should be left off unless actually
required by the application for profiling purposes.

Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
---
 include/uapi/drm/panfrost_drm.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h
index ec19db1ee..27e6cb941 100644
--- a/include/uapi/drm/panfrost_drm.h
+++ b/include/uapi/drm/panfrost_drm.h
@@ -39,7 +39,8 @@ extern "C" {
 #define DRM_IOCTL_PANFROST_PERFCNT_ENABLE	DRM_IOW(DRM_COMMAND_BASE + DRM_PANFROST_PERFCNT_ENABLE, struct drm_panfrost_perfcnt_enable)
 #define DRM_IOCTL_PANFROST_PERFCNT_DUMP		DRM_IOW(DRM_COMMAND_BASE + DRM_PANFROST_PERFCNT_DUMP, struct drm_panfrost_perfcnt_dump)
 
-#define PANFROST_JD_REQ_FS (1 << 0)
+#define PANFROST_JD_REQ_FS			(1 << 0)
+#define PANFROST_JD_REQ_PERMON			(1 << 1)
 /**
  * struct drm_panfrost_submit - ioctl argument for submitting commands to the 3D
  * engine.
-- 
2.30.2

Re: [PATCH 1/4] drm/panfrost: Add cycle counter job requirement

[Steven Price]

On 27/05/2021 21:38, alyssa.rosenzweig@collabora.com wrote:
> From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> 
> Extend the Panfrost UABI with a new job requirement for cycle counters
> (and GPU timestamps, by extension). This requirement is used in
> userspace to implement ARB_shader_clock, an OpenGL extension reporting
> the GPU cycle count within a shader. The same mechanism will be required
> to implement timestamp queries as a "write value - timestamp" job.
> 
> We cannot enable cycle counters unconditionally, as enabling them
> increases GPU power consumption. They should be left off unless actually
> required by the application for profiling purposes.
> 
> Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> ---
>  include/uapi/drm/panfrost_drm.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h
> index ec19db1ee..27e6cb941 100644
> --- a/include/uapi/drm/panfrost_drm.h
> +++ b/include/uapi/drm/panfrost_drm.h
> @@ -39,7 +39,8 @@ extern "C" {
>  #define DRM_IOCTL_PANFROST_PERFCNT_ENABLE	DRM_IOW(DRM_COMMAND_BASE + DRM_PANFROST_PERFCNT_ENABLE, struct drm_panfrost_perfcnt_enable)
>  #define DRM_IOCTL_PANFROST_PERFCNT_DUMP		DRM_IOW(DRM_COMMAND_BASE + DRM_PANFROST_PERFCNT_DUMP, struct drm_panfrost_perfcnt_dump)
>  
> -#define PANFROST_JD_REQ_FS (1 << 0)
> +#define PANFROST_JD_REQ_FS			(1 << 0)
> +#define PANFROST_JD_REQ_PERMON			(1 << 1)

As noted PERMON is a bit jargony but matches kbase. Another option to
aid with the bike shedding could be _REQ_CYCLE_COUNT as this is
requesting cycle counters (and timestamp propagation). But I don't mind
the colour of the shed as long as it doesn't leak... ;)

Thanks,

Steve

>  /**
>   * struct drm_panfrost_submit - ioctl argument for submitting commands to the 3D
>   * engine.
> 

[PATCH 2/4] drm/panfrost: Add CYCLE_COUNT_START/STOP commands

[alyssa.rosenzweig]

From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

Add additional values of GPU_COMMAND required to enable and disable the
cycle (and timestamp) counters. Values from mali_kbase.

Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_regs.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_regs.h b/drivers/gpu/drm/panfrost/panfrost_regs.h
index eddaa62ad..8ac60de6f 100644
--- a/drivers/gpu/drm/panfrost/panfrost_regs.h
+++ b/drivers/gpu/drm/panfrost/panfrost_regs.h
@@ -46,6 +46,8 @@
 #define   GPU_CMD_SOFT_RESET		0x01
 #define   GPU_CMD_PERFCNT_CLEAR		0x03
 #define   GPU_CMD_PERFCNT_SAMPLE	0x04
+#define   GPU_CMD_CYCLE_COUNT_START	0x05
+#define   GPU_CMD_CYCLE_COUNT_STOP	0x06
 #define   GPU_CMD_CLEAN_CACHES		0x07
 #define   GPU_CMD_CLEAN_INV_CACHES	0x08
 #define GPU_STATUS			0x34
-- 
2.30.2

Re: [PATCH 2/4] drm/panfrost: Add CYCLE_COUNT_START/STOP commands

[Steven Price]

On 27/05/2021 21:38, alyssa.rosenzweig@collabora.com wrote:
> From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> 
> Add additional values of GPU_COMMAND required to enable and disable the
> cycle (and timestamp) counters. Values from mali_kbase.
> 
> Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  drivers/gpu/drm/panfrost/panfrost_regs.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_regs.h b/drivers/gpu/drm/panfrost/panfrost_regs.h
> index eddaa62ad..8ac60de6f 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_regs.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_regs.h
> @@ -46,6 +46,8 @@
>  #define   GPU_CMD_SOFT_RESET		0x01
>  #define   GPU_CMD_PERFCNT_CLEAR		0x03
>  #define   GPU_CMD_PERFCNT_SAMPLE	0x04
> +#define   GPU_CMD_CYCLE_COUNT_START	0x05
> +#define   GPU_CMD_CYCLE_COUNT_STOP	0x06
>  #define   GPU_CMD_CLEAN_CACHES		0x07
>  #define   GPU_CMD_CLEAN_INV_CACHES	0x08
>  #define GPU_STATUS			0x34
> 

[PATCH 3/4] drm/panfrost: Add permon acquire/release helpers

[alyssa.rosenzweig]

From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

Wrap the underlying CYCLE_COUNT_START/STOP commands in a safe interface
that ensures the commands are only issued where required by guarding
behind an atomic counter. In particular, we need to be careful about
races between multiple in-flight jobs, where only some require cycle
counts.

Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_device.h |  3 +++
 drivers/gpu/drm/panfrost/panfrost_gpu.c    | 20 ++++++++++++++++++++
 drivers/gpu/drm/panfrost/panfrost_gpu.h    |  3 +++
 3 files changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index 597cf1459..8a89aa274 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -117,6 +117,9 @@ struct panfrost_device {
 	struct shrinker shrinker;
 
 	struct panfrost_devfreq pfdevfreq;
+
+	/* Number of active jobs requiring performance monitoring */
+	atomic_t permon_pending;
 };
 
 struct panfrost_mmu {
diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
index 2aae636f1..acacceb15 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
@@ -399,3 +399,23 @@ u32 panfrost_gpu_get_latest_flush_id(struct panfrost_device *pfdev)
 
 	return 0;
 }
+
+void panfrost_acquire_permon(struct panfrost_device *pfdev)
+{
+	/* If another in-flight job enabled permon, we don't have to */
+	if (atomic_inc_return(&pfdev->permon_pending) > 1)
+		return;
+
+	/* Otherwise, we're the first user */
+	gpu_write(pfdev, GPU_CMD, GPU_CMD_CYCLE_COUNT_START);
+}
+
+void panfrost_release_permon(struct panfrost_device *pfdev)
+{
+	/* If another in-flight job needs permon, keep it active */
+	if (atomic_dec_return(&pfdev->permon_pending) > 0)
+		return;
+
+	/* Otherwise, we're the last user */
+	gpu_write(pfdev, GPU_CMD, GPU_CMD_CYCLE_COUNT_STOP);
+}
diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.h b/drivers/gpu/drm/panfrost/panfrost_gpu.h
index 468c51e7e..01a91af09 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gpu.h
+++ b/drivers/gpu/drm/panfrost/panfrost_gpu.h
@@ -18,4 +18,7 @@ void panfrost_gpu_power_off(struct panfrost_device *pfdev);
 
 void panfrost_gpu_amlogic_quirk(struct panfrost_device *pfdev);
 
+void panfrost_acquire_permon(struct panfrost_device *pfdev);
+void panfrost_release_permon(struct panfrost_device *pfdev);
+
 #endif
-- 
2.30.2

Re: [PATCH 3/4] drm/panfrost: Add permon acquire/release helpers

[Steven Price]

On 27/05/2021 21:38, alyssa.rosenzweig@collabora.com wrote:
> From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> 
> Wrap the underlying CYCLE_COUNT_START/STOP commands in a safe interface
> that ensures the commands are only issued where required by guarding
> behind an atomic counter. In particular, we need to be careful about
> races between multiple in-flight jobs, where only some require cycle
> counts.
> 
> Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_device.h |  3 +++
>  drivers/gpu/drm/panfrost/panfrost_gpu.c    | 20 ++++++++++++++++++++
>  drivers/gpu/drm/panfrost/panfrost_gpu.h    |  3 +++
>  3 files changed, 26 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index 597cf1459..8a89aa274 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -117,6 +117,9 @@ struct panfrost_device {
>  	struct shrinker shrinker;
>  
>  	struct panfrost_devfreq pfdevfreq;
> +
> +	/* Number of active jobs requiring performance monitoring */
> +	atomic_t permon_pending;

If the JD_REQ name is going to be changed then this should be renamed to
be consistent.

>  };
>  
>  struct panfrost_mmu {
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> index 2aae636f1..acacceb15 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> @@ -399,3 +399,23 @@ u32 panfrost_gpu_get_latest_flush_id(struct panfrost_device *pfdev)
>  
>  	return 0;
>  }
> +
> +void panfrost_acquire_permon(struct panfrost_device *pfdev)
> +{
> +	/* If another in-flight job enabled permon, we don't have to */
> +	if (atomic_inc_return(&pfdev->permon_pending) > 1)
> +		return;
> +
> +	/* Otherwise, we're the first user */
> +	gpu_write(pfdev, GPU_CMD, GPU_CMD_CYCLE_COUNT_START);
> +}

NIT: The check of atomic_inc_return() is usually of the style '== 1', i.e.:

	/* Only enable permon, if we're the first user */
	if (atomic_inc_return(&pfdev->permon_pending) == 1)
		gpu_write(pfdev, GPU_CMD, GPU_CMD_CYCLE_COUNT_START);

> +
> +void panfrost_release_permon(struct panfrost_device *pfdev)
> +{
> +	/* If another in-flight job needs permon, keep it active */
> +	if (atomic_dec_return(&pfdev->permon_pending) > 0)
> +		return;
> +
> +	/* Otherwise, we're the last user */
> +	gpu_write(pfdev, GPU_CMD, GPU_CMD_CYCLE_COUNT_STOP);
> +}

NIT: Similarly for disabling the usual style is '== 0'

Thanks,

Steve

> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.h b/drivers/gpu/drm/panfrost/panfrost_gpu.h
> index 468c51e7e..01a91af09 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gpu.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.h
> @@ -18,4 +18,7 @@ void panfrost_gpu_power_off(struct panfrost_device *pfdev);
>  
>  void panfrost_gpu_amlogic_quirk(struct panfrost_device *pfdev);
>  
> +void panfrost_acquire_permon(struct panfrost_device *pfdev);
> +void panfrost_release_permon(struct panfrost_device *pfdev);
> +
>  #endif
> 

[PATCH 4/4] drm/panfrost: Handle PANFROST_JD_REQ_PERMON

[alyssa.rosenzweig]

From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

If a job requires cycle counters or timestamps, we must enable cycle
counting just before issuing the job, and disable as soon as the job
completes.

Since this extends the UABI, we bump the driver minor version and date.
That lets userspace detect cycle counter support, and only advertise
features like ARB_shader_clock on kernels with this commit.

Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 10 +++++++---
 drivers/gpu/drm/panfrost/panfrost_job.c |  6 ++++++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index ca07098a6..0f11d2df4 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -20,6 +20,10 @@
 #include "panfrost_gpu.h"
 #include "panfrost_perfcnt.h"
 
+#define JOB_REQUIREMENTS \
+	(PANFROST_JD_REQ_FS | \
+	 PANFROST_JD_REQ_PERMON)
+
 static bool unstable_ioctls;
 module_param_unsafe(unstable_ioctls, bool, 0600);
 
@@ -247,7 +251,7 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
 	if (!args->jc)
 		return -EINVAL;
 
-	if (args->requirements && args->requirements != PANFROST_JD_REQ_FS)
+	if (args->requirements & ~JOB_REQUIREMENTS)
 		return -EINVAL;
 
 	if (args->out_sync > 0) {
@@ -557,9 +561,9 @@ static const struct drm_driver panfrost_drm_driver = {
 	.fops			= &panfrost_drm_driver_fops,
 	.name			= "panfrost",
 	.desc			= "panfrost DRM",
-	.date			= "20180908",
+	.date			= "20210527",
 	.major			= 1,
-	.minor			= 1,
+	.minor			= 2,
 
 	.gem_create_object	= panfrost_gem_create_object,
 	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 6003cfeb1..b78147e3d 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -165,6 +165,9 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
 		return;
 	}
 
+	if (job->requirements & PANFROST_JD_REQ_PERMON)
+		panfrost_acquire_permon(job->pfdev);
+
 	cfg = panfrost_mmu_as_get(pfdev, &job->file_priv->mmu);
 
 	job_write(pfdev, JS_HEAD_NEXT_LO(js), jc_head & 0xFFFFFFFF);
@@ -296,6 +299,9 @@ static void panfrost_job_cleanup(struct kref *ref)
 		kvfree(job->bos);
 	}
 
+	if (job->requirements & PANFROST_JD_REQ_PERMON)
+		panfrost_release_permon(job->pfdev);
+
 	kfree(job);
 }
 
-- 
2.30.2

Re: [PATCH 4/4] drm/panfrost: Handle PANFROST_JD_REQ_PERMON

[Steven Price]

On 27/05/2021 21:38, alyssa.rosenzweig@collabora.com wrote:
> From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> 
> If a job requires cycle counters or timestamps, we must enable cycle
> counting just before issuing the job, and disable as soon as the job
> completes.
> 
> Since this extends the UABI, we bump the driver minor version and date.
> That lets userspace detect cycle counter support, and only advertise
> features like ARB_shader_clock on kernels with this commit.
> 
> Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c | 10 +++++++---
>  drivers/gpu/drm/panfrost/panfrost_job.c |  6 ++++++
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index ca07098a6..0f11d2df4 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -20,6 +20,10 @@
>  #include "panfrost_gpu.h"
>  #include "panfrost_perfcnt.h"
>  
> +#define JOB_REQUIREMENTS \
> +	(PANFROST_JD_REQ_FS | \
> +	 PANFROST_JD_REQ_PERMON)
> +
>  static bool unstable_ioctls;
>  module_param_unsafe(unstable_ioctls, bool, 0600);
>  
> @@ -247,7 +251,7 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
>  	if (!args->jc)
>  		return -EINVAL;
>  
> -	if (args->requirements && args->requirements != PANFROST_JD_REQ_FS)
> +	if (args->requirements & ~JOB_REQUIREMENTS)
>  		return -EINVAL;
>  
>  	if (args->out_sync > 0) {
> @@ -557,9 +561,9 @@ static const struct drm_driver panfrost_drm_driver = {
>  	.fops			= &panfrost_drm_driver_fops,
>  	.name			= "panfrost",
>  	.desc			= "panfrost DRM",
> -	.date			= "20180908",
> +	.date			= "20210527",
>  	.major			= 1,
> -	.minor			= 1,
> +	.minor			= 2,
>  
>  	.gem_create_object	= panfrost_gem_create_object,
>  	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 6003cfeb1..b78147e3d 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -165,6 +165,9 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
>  		return;
>  	}
>  
> +	if (job->requirements & PANFROST_JD_REQ_PERMON)
> +		panfrost_acquire_permon(job->pfdev);
> +

This is leaky - panfrost_job_hw_submit() is currently unconditional - it
pretends to always succeed in submitting the job, so any reference
counting has to always happen. I have a patch fixing this mess:

https://lore.kernel.org/dri-devel/20210512152419.30003-1-steven.price@arm.com/

(Review welcome!)

Basically in the (unlikely) event that the function bails out early
(pm_runtime_get_sync() fails or we hit the WARN_ON) then the job will
still be cleaned up causing the reference count to go negative.

I'm also suspicious that jobs can be cleaned up without ever being
submitted, or submitted more than once (due to a GPU reset). But I
haven't chased that down to be sure that's a problem in reality.

One obvious option would be to add a flag to the job to record whether
we have taken the 'PERMON' reference count.

Thanks,

Steve

>  	cfg = panfrost_mmu_as_get(pfdev, &job->file_priv->mmu);
>  
>  	job_write(pfdev, JS_HEAD_NEXT_LO(js), jc_head & 0xFFFFFFFF);
> @@ -296,6 +299,9 @@ static void panfrost_job_cleanup(struct kref *ref)
>  		kvfree(job->bos);
>  	}
>  
> +	if (job->requirements & PANFROST_JD_REQ_PERMON)
> +		panfrost_release_permon(job->pfdev);
> +
>  	kfree(job);
>  }
>  
> 

Re: [PATCH 0/4] drm/panfrost: Plumb cycle counters to userspace

[Alyssa Rosenzweig]

> The main outstanding questing is the proper name. Performance monitoring
> ("PERMON") is the name used by kbase, but it's jargon-y and risks
> confusion with performance counters, an orthogonal mechanism. Cycle
> count is more descriptive and matches the actual hardware name, but
> obscures that the same mechanism is required for GPU timestamps. This
> bit of bikeshedding aside, I'm pleased with the patches.

PANFROST_JD_REQ_CLOCK might be the clearest.

Re: [PATCH 0/4] drm/panfrost: Plumb cycle counters to userspace

[Tomeu Vizoso]

Hi Alyssa,

Will this be enough to implement GL_TIMESTAMP and GL_TIME_ELAPSED queries?

Guess the DDK implements these as WRITE_VALUE jobs, and there's also a 
soft job BASE_JD_REQ_SOFT_DUMP_CPU_GPU_TIME that I guess is used for 
glGet*(GL_TIMESTAMP). Other DRM drivers use an ioctl for that instead.

Regards,

Tomeu

On 5/27/21 10:38 PM, alyssa.rosenzweig@collabora.com wrote:
> From: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> 
> Mali has hardware cycle counters (and GPU timestamps) available for
> profiling. These are exposed in various ways:
> 
> - Kernel: As CYCLE_COUNT and TIMESTAMP registers
> - Job chain: As WRITE_VALUE descriptors
> - Shader (Midgard): As LD_SPECIAL selectors
> - Shader (Bifrost): As the LD_GCLK.u64 instruction
> 
> These form building blocks for profiling features, for example the
> ARB_shader_clock extension which accesses the counters from an
> application's shader.
> 
> The counters consume power, so it is recommended to disable the counters
> when not in use. To do so, we follow the strategy from mali_kbase: add a
> counter requirement to the job, start the counters only when required,
> and stop them as quickly as possible.
> 
> The new UABI will be used in Mesa. An implementation of ARB_shader_clock
> using this UABI is available as a pending upstream merge request [1].
> The implementation passes the relevant piglit test, validating both the
> kernel and mesa.
> 
> The main outstanding questing is the proper name. Performance monitoring
> ("PERMON") is the name used by kbase, but it's jargon-y and risks
> confusion with performance counters, an orthogonal mechanism. Cycle
> count is more descriptive and matches the actual hardware name, but
> obscures that the same mechanism is required for GPU timestamps. This
> bit of bikeshedding aside, I'm pleased with the patches.
> 
> [1] https://gitlab.freedesktop.org/mesa/mesa/merge_requests/11051
> 
> Alyssa Rosenzweig (4):
>    drm/panfrost: Add cycle counter job requirement
>    drm/panfrost: Add CYCLE_COUNT_START/STOP commands
>    drm/panfrost: Add permon acquire/release helpers
>    drm/panfrost: Handle PANFROST_JD_REQ_PERMON
> 
>   drivers/gpu/drm/panfrost/panfrost_device.h |  3 +++
>   drivers/gpu/drm/panfrost/panfrost_drv.c    | 10 +++++++---
>   drivers/gpu/drm/panfrost/panfrost_gpu.c    | 20 ++++++++++++++++++++
>   drivers/gpu/drm/panfrost/panfrost_gpu.h    |  3 +++
>   drivers/gpu/drm/panfrost/panfrost_job.c    |  6 ++++++
>   drivers/gpu/drm/panfrost/panfrost_regs.h   |  2 ++
>   include/uapi/drm/panfrost_drm.h            |  3 ++-
>   7 files changed, 43 insertions(+), 4 deletions(-)
> 

Re: [PATCH 0/4] drm/panfrost: Plumb cycle counters to userspace

[Alyssa Rosenzweig]

Hi Tomeu,

> Will this be enough to implement GL_TIMESTAMP and GL_TIME_ELAPSED queries?
> 
> Guess the DDK implements these as WRITE_VALUE jobs, and there's also a soft
> job BASE_JD_REQ_SOFT_DUMP_CPU_GPU_TIME that I guess is used for
> glGet*(GL_TIMESTAMP). Other DRM drivers use an ioctl for that instead.

For anything implemented as WRITE_VALUE jobs, this is necessary and
sufficient on the kernel side. If an out-of-band soft job or ioctl is
truly needed (I haven't looked), of course that needs additional piping.

Thanks,

Alyssa