All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] drm/panfrost: Plumb cycle counters to userspace
@ 2021-05-27 20:38 alyssa.rosenzweig
  2021-05-27 20:38 ` [PATCH 1/4] drm/panfrost: Add cycle counter job requirement alyssa.rosenzweig
                   ` (5 more replies)
  0 siblings, 6 replies; 12+ messages in thread
From: alyssa.rosenzweig @ 2021-05-27 20:38 UTC (permalink / raw)
  To: dri-devel; +Cc: tomeu.vizoso, airlied, steven.price, 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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/4] drm/panfrost: Add cycle counter job requirement
  2021-05-27 20:38 [PATCH 0/4] drm/panfrost: Plumb cycle counters to userspace alyssa.rosenzweig
@ 2021-05-27 20:38 ` alyssa.rosenzweig
  2021-06-02 11:50   ` Steven Price
  2021-05-27 20:38 ` [PATCH 2/4] drm/panfrost: Add CYCLE_COUNT_START/STOP commands alyssa.rosenzweig
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 12+ messages in thread
From: alyssa.rosenzweig @ 2021-05-27 20:38 UTC (permalink / raw)
  To: dri-devel; +Cc: tomeu.vizoso, airlied, steven.price, 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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/4] drm/panfrost: Add CYCLE_COUNT_START/STOP commands
  2021-05-27 20:38 [PATCH 0/4] drm/panfrost: Plumb cycle counters to userspace alyssa.rosenzweig
  2021-05-27 20:38 ` [PATCH 1/4] drm/panfrost: Add cycle counter job requirement alyssa.rosenzweig
@ 2021-05-27 20:38 ` alyssa.rosenzweig
  2021-06-02 11:50   ` Steven Price
  2021-05-27 20:38 ` [PATCH 3/4] drm/panfrost: Add permon acquire/release helpers alyssa.rosenzweig
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 12+ messages in thread
From: alyssa.rosenzweig @ 2021-05-27 20:38 UTC (permalink / raw)
  To: dri-devel; +Cc: tomeu.vizoso, airlied, steven.price, 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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/4] drm/panfrost: Add permon acquire/release helpers
  2021-05-27 20:38 [PATCH 0/4] drm/panfrost: Plumb cycle counters to userspace alyssa.rosenzweig
  2021-05-27 20:38 ` [PATCH 1/4] drm/panfrost: Add cycle counter job requirement alyssa.rosenzweig
  2021-05-27 20:38 ` [PATCH 2/4] drm/panfrost: Add CYCLE_COUNT_START/STOP commands alyssa.rosenzweig
@ 2021-05-27 20:38 ` alyssa.rosenzweig
  2021-06-02 11:50   ` Steven Price
  2021-05-27 20:38 ` [PATCH 4/4] drm/panfrost: Handle PANFROST_JD_REQ_PERMON alyssa.rosenzweig
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 12+ messages in thread
From: alyssa.rosenzweig @ 2021-05-27 20:38 UTC (permalink / raw)
  To: dri-devel; +Cc: tomeu.vizoso, airlied, steven.price, 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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 4/4] drm/panfrost: Handle PANFROST_JD_REQ_PERMON
  2021-05-27 20:38 [PATCH 0/4] drm/panfrost: Plumb cycle counters to userspace alyssa.rosenzweig
                   ` (2 preceding siblings ...)
  2021-05-27 20:38 ` [PATCH 3/4] drm/panfrost: Add permon acquire/release helpers alyssa.rosenzweig
@ 2021-05-27 20:38 ` alyssa.rosenzweig
  2021-06-02 11:50   ` Steven Price
  2021-05-27 21:14 ` [PATCH 0/4] drm/panfrost: Plumb cycle counters to userspace Alyssa Rosenzweig
  2021-05-28  6:07 ` Tomeu Vizoso
  5 siblings, 1 reply; 12+ messages in thread
From: alyssa.rosenzweig @ 2021-05-27 20:38 UTC (permalink / raw)
  To: dri-devel; +Cc: tomeu.vizoso, airlied, steven.price, 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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/4] drm/panfrost: Plumb cycle counters to userspace
  2021-05-27 20:38 [PATCH 0/4] drm/panfrost: Plumb cycle counters to userspace alyssa.rosenzweig
                   ` (3 preceding siblings ...)
  2021-05-27 20:38 ` [PATCH 4/4] drm/panfrost: Handle PANFROST_JD_REQ_PERMON alyssa.rosenzweig
@ 2021-05-27 21:14 ` Alyssa Rosenzweig
  2021-05-28  6:07 ` Tomeu Vizoso
  5 siblings, 0 replies; 12+ messages in thread
From: Alyssa Rosenzweig @ 2021-05-27 21:14 UTC (permalink / raw)
  To: alyssa.rosenzweig; +Cc: tomeu.vizoso, airlied, dri-devel, steven.price

> 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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/4] drm/panfrost: Plumb cycle counters to userspace
  2021-05-27 20:38 [PATCH 0/4] drm/panfrost: Plumb cycle counters to userspace alyssa.rosenzweig
                   ` (4 preceding siblings ...)
  2021-05-27 21:14 ` [PATCH 0/4] drm/panfrost: Plumb cycle counters to userspace Alyssa Rosenzweig
@ 2021-05-28  6:07 ` Tomeu Vizoso
  2021-05-28 13:31   ` Alyssa Rosenzweig
  5 siblings, 1 reply; 12+ messages in thread
From: Tomeu Vizoso @ 2021-05-28  6:07 UTC (permalink / raw)
  To: alyssa.rosenzweig, dri-devel; +Cc: airlied, steven.price

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(-)
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/4] drm/panfrost: Plumb cycle counters to userspace
  2021-05-28  6:07 ` Tomeu Vizoso
@ 2021-05-28 13:31   ` Alyssa Rosenzweig
  0 siblings, 0 replies; 12+ messages in thread
From: Alyssa Rosenzweig @ 2021-05-28 13:31 UTC (permalink / raw)
  To: Tomeu Vizoso; +Cc: airlied, dri-devel, steven.price, 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/4] drm/panfrost: Add cycle counter job requirement
  2021-05-27 20:38 ` [PATCH 1/4] drm/panfrost: Add cycle counter job requirement alyssa.rosenzweig
@ 2021-06-02 11:50   ` Steven Price
  0 siblings, 0 replies; 12+ messages in thread
From: Steven Price @ 2021-06-02 11:50 UTC (permalink / raw)
  To: alyssa.rosenzweig, dri-devel; +Cc: airlied, tomeu.vizoso

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.
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/4] drm/panfrost: Add CYCLE_COUNT_START/STOP commands
  2021-05-27 20:38 ` [PATCH 2/4] drm/panfrost: Add CYCLE_COUNT_START/STOP commands alyssa.rosenzweig
@ 2021-06-02 11:50   ` Steven Price
  0 siblings, 0 replies; 12+ messages in thread
From: Steven Price @ 2021-06-02 11:50 UTC (permalink / raw)
  To: alyssa.rosenzweig, dri-devel; +Cc: airlied, tomeu.vizoso

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
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] drm/panfrost: Add permon acquire/release helpers
  2021-05-27 20:38 ` [PATCH 3/4] drm/panfrost: Add permon acquire/release helpers alyssa.rosenzweig
@ 2021-06-02 11:50   ` Steven Price
  0 siblings, 0 replies; 12+ messages in thread
From: Steven Price @ 2021-06-02 11:50 UTC (permalink / raw)
  To: alyssa.rosenzweig, dri-devel; +Cc: airlied, tomeu.vizoso

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
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/4] drm/panfrost: Handle PANFROST_JD_REQ_PERMON
  2021-05-27 20:38 ` [PATCH 4/4] drm/panfrost: Handle PANFROST_JD_REQ_PERMON alyssa.rosenzweig
@ 2021-06-02 11:50   ` Steven Price
  0 siblings, 0 replies; 12+ messages in thread
From: Steven Price @ 2021-06-02 11:50 UTC (permalink / raw)
  To: alyssa.rosenzweig, dri-devel; +Cc: airlied, tomeu.vizoso

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);
>  }
>  
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-06-02 11:50 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-27 20:38 [PATCH 0/4] drm/panfrost: Plumb cycle counters to userspace alyssa.rosenzweig
2021-05-27 20:38 ` [PATCH 1/4] drm/panfrost: Add cycle counter job requirement alyssa.rosenzweig
2021-06-02 11:50   ` Steven Price
2021-05-27 20:38 ` [PATCH 2/4] drm/panfrost: Add CYCLE_COUNT_START/STOP commands alyssa.rosenzweig
2021-06-02 11:50   ` Steven Price
2021-05-27 20:38 ` [PATCH 3/4] drm/panfrost: Add permon acquire/release helpers alyssa.rosenzweig
2021-06-02 11:50   ` Steven Price
2021-05-27 20:38 ` [PATCH 4/4] drm/panfrost: Handle PANFROST_JD_REQ_PERMON alyssa.rosenzweig
2021-06-02 11:50   ` Steven Price
2021-05-27 21:14 ` [PATCH 0/4] drm/panfrost: Plumb cycle counters to userspace Alyssa Rosenzweig
2021-05-28  6:07 ` Tomeu Vizoso
2021-05-28 13:31   ` Alyssa Rosenzweig

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.