public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] drm/panfrost: Wire cycle counters and timestamp info to userspace
@ 2024-08-07 16:08 Mary Guillemard
  2024-08-07 16:08 ` [PATCH 1/3] drm/panfrost: Add SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY parameters Mary Guillemard
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Mary Guillemard @ 2024-08-07 16:08 UTC (permalink / raw)
  To: linux-kernel; +Cc: dri-devel, kernel, Mary Guillemard

Mali has hardware cycle counters and GPU timestamps available for
profiling.

This patch series adds support for cycle counters propagation and
also new timestamp info parameters.

Those new changes to the uAPI will be used in Mesa to implement
timestamp queries for OpenGL and Vulkan.

Mary Guillemard (3):
  drm/panfrost: Add SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY
    parameters
  drm/panfrost: Add cycle counter job requirement
  drm/panfrost: Handle JD_REQ_CYCLE_COUNT

 drivers/gpu/drm/panfrost/panfrost_drv.c | 25 +++++++++++++++++++++++--
 drivers/gpu/drm/panfrost/panfrost_job.c | 10 ++++++++++
 include/uapi/drm/panfrost_drm.h         |  3 +++
 3 files changed, 36 insertions(+), 2 deletions(-)


base-commit: f7f3ddb6e5c8dc7b621fd8c0903ea42190d67452
-- 
2.45.2


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

* [PATCH 1/3] drm/panfrost: Add SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY parameters
  2024-08-07 16:08 [PATCH 0/3] drm/panfrost: Wire cycle counters and timestamp info to userspace Mary Guillemard
@ 2024-08-07 16:08 ` Mary Guillemard
  2024-08-08 10:23   ` Steven Price
  2024-08-08 13:06   ` kernel test robot
  2024-08-07 16:08 ` [PATCH 2/3] drm/panfrost: Add cycle counter job requirement Mary Guillemard
  2024-08-07 16:08 ` [PATCH 3/3] drm/panfrost: Handle JD_REQ_CYCLE_COUNT Mary Guillemard
  2 siblings, 2 replies; 9+ messages in thread
From: Mary Guillemard @ 2024-08-07 16:08 UTC (permalink / raw)
  To: linux-kernel
  Cc: dri-devel, kernel, Mary Guillemard, Boris Brezillon, Rob Herring,
	Steven Price, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter

Expose system timestamp and frequency supported by the GPU.

Mali uses the generic arch timer as GPU system time so we currently
wire cntvct_el0 and cntfrq_el0 respectively to those parameters.
We could have directly read those values from userland but handling
this here allows us to be future proof in case of errata related to
timers for example.

This new uAPI will be used in Mesa to implement timestamp queries and
VK_KHR_calibrated_timestamps.

Signed-off-by: Mary Guillemard <mary.guillemard@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 17 +++++++++++++++++
 include/uapi/drm/panfrost_drm.h         |  2 ++
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 671eed4ad890..d94c9bf5a7f9 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -69,6 +69,23 @@ static int panfrost_ioctl_get_param(struct drm_device *ddev, void *data, struct
 		PANFROST_FEATURE_ARRAY(JS_FEATURES, js_features, 15);
 		PANFROST_FEATURE(NR_CORE_GROUPS, nr_core_groups);
 		PANFROST_FEATURE(THREAD_TLS_ALLOC, thread_tls_alloc);
+
+	case DRM_PANFROST_PARAM_SYSTEM_TIMESTAMP:
+#ifdef CONFIG_ARM_ARCH_TIMER
+		param->value = __arch_counter_get_cntvct();
+#else
+		param->value = 0;
+#endif
+		break;
+
+	case DRM_PANFROST_PARAM_SYSTEM_TIMESTAMP_FREQUENCY:
+#ifdef CONFIG_ARM_ARCH_TIMER
+		param->value = arch_timer_get_cntfrq();
+#else
+		param->value = 0;
+#endif
+		break;
+
 	default:
 		return -EINVAL;
 	}
diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h
index 9f231d40a146..52b050e2b660 100644
--- a/include/uapi/drm/panfrost_drm.h
+++ b/include/uapi/drm/panfrost_drm.h
@@ -172,6 +172,8 @@ enum drm_panfrost_param {
 	DRM_PANFROST_PARAM_NR_CORE_GROUPS,
 	DRM_PANFROST_PARAM_THREAD_TLS_ALLOC,
 	DRM_PANFROST_PARAM_AFBC_FEATURES,
+	DRM_PANFROST_PARAM_SYSTEM_TIMESTAMP,
+	DRM_PANFROST_PARAM_SYSTEM_TIMESTAMP_FREQUENCY,
 };
 
 struct drm_panfrost_get_param {
-- 
2.45.2


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

* [PATCH 2/3] drm/panfrost: Add cycle counter job requirement
  2024-08-07 16:08 [PATCH 0/3] drm/panfrost: Wire cycle counters and timestamp info to userspace Mary Guillemard
  2024-08-07 16:08 ` [PATCH 1/3] drm/panfrost: Add SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY parameters Mary Guillemard
@ 2024-08-07 16:08 ` Mary Guillemard
  2024-08-08 10:23   ` Steven Price
  2024-08-07 16:08 ` [PATCH 3/3] drm/panfrost: Handle JD_REQ_CYCLE_COUNT Mary Guillemard
  2 siblings, 1 reply; 9+ messages in thread
From: Mary Guillemard @ 2024-08-07 16:08 UTC (permalink / raw)
  To: linux-kernel
  Cc: dri-devel, kernel, Mary Guillemard, Boris Brezillon, Rob Herring,
	Steven Price, David Airlie, Daniel Vetter, Maarten Lankhorst,
	Maxime Ripard, Thomas Zimmermann

Extend the uAPI with a new job requirement flag for cycle
counters. This requirement is used by userland to indicate that a job
requires cycle counters or system timestamp to be propagated. (for use
with write value timestamp jobs)

We cannot enable cycle counters unconditionally as this would result in
an increase of GPU power consumption. As a result, they should be left
off unless required by the application.

This new uAPI will be used in Mesa to implement timestamp queries and
VK_KHR_shader_clock.

Signed-off-by: Mary Guillemard <mary.guillemard@collabora.com>
---
 include/uapi/drm/panfrost_drm.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h
index 52b050e2b660..568724be6628 100644
--- a/include/uapi/drm/panfrost_drm.h
+++ b/include/uapi/drm/panfrost_drm.h
@@ -40,6 +40,7 @@ extern "C" {
 #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_CYCLE_COUNT (1 << 1)
 /**
  * struct drm_panfrost_submit - ioctl argument for submitting commands to the 3D
  * engine.
-- 
2.45.2


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

* [PATCH 3/3] drm/panfrost: Handle JD_REQ_CYCLE_COUNT
  2024-08-07 16:08 [PATCH 0/3] drm/panfrost: Wire cycle counters and timestamp info to userspace Mary Guillemard
  2024-08-07 16:08 ` [PATCH 1/3] drm/panfrost: Add SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY parameters Mary Guillemard
  2024-08-07 16:08 ` [PATCH 2/3] drm/panfrost: Add cycle counter job requirement Mary Guillemard
@ 2024-08-07 16:08 ` Mary Guillemard
  2024-08-08 10:23   ` Steven Price
  2 siblings, 1 reply; 9+ messages in thread
From: Mary Guillemard @ 2024-08-07 16:08 UTC (permalink / raw)
  To: linux-kernel
  Cc: dri-devel, kernel, Mary Guillemard, Boris Brezillon, Rob Herring,
	Steven Price, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter

If a job requires cycle counters or system timestamps propagation, we
must enable cycle counting before issuing a job and disable it right
after the job completes.

Since this extends the uAPI and because userland needs a way to advertise
features like VK_KHR_shader_clock conditionally, we bumps the driver
minor version.

Signed-off-by: Mary Guillemard <mary.guillemard@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c |  8 ++++++--
 drivers/gpu/drm/panfrost/panfrost_job.c | 10 ++++++++++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index d94c9bf5a7f9..fe983defdfdf 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -21,6 +21,8 @@
 #include "panfrost_gpu.h"
 #include "panfrost_perfcnt.h"
 
+#define JOB_REQUIREMENTS (PANFROST_JD_REQ_FS | PANFROST_JD_REQ_CYCLE_COUNT)
+
 static bool unstable_ioctls;
 module_param_unsafe(unstable_ioctls, bool, 0600);
 
@@ -262,7 +264,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 && args->requirements & ~JOB_REQUIREMENTS)
 		return -EINVAL;
 
 	if (args->out_sync > 0) {
@@ -601,6 +603,8 @@ static const struct file_operations panfrost_drm_driver_fops = {
  * - 1.0 - initial interface
  * - 1.1 - adds HEAP and NOEXEC flags for CREATE_BO
  * - 1.2 - adds AFBC_FEATURES query
+ * - 1.3 - adds JD_REQ_CYCLE_COUNT job requirement for SUBMIT
+ *       - adds SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY queries
  */
 static const struct drm_driver panfrost_drm_driver = {
 	.driver_features	= DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ,
@@ -614,7 +618,7 @@ static const struct drm_driver panfrost_drm_driver = {
 	.desc			= "panfrost DRM",
 	.date			= "20180908",
 	.major			= 1,
-	.minor			= 2,
+	.minor			= 3,
 
 	.gem_create_object	= panfrost_gem_create_object,
 	.gem_prime_import_sg_table = panfrost_gem_prime_import_sg_table,
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index df49d37d0e7e..d8c215c0c672 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -159,6 +159,9 @@ panfrost_dequeue_job(struct panfrost_device *pfdev, int slot)
 	struct panfrost_job *job = pfdev->jobs[slot][0];
 
 	WARN_ON(!job);
+	if (job->requirements & PANFROST_JD_REQ_CYCLE_COUNT)
+		panfrost_cycle_counter_put(pfdev);
+
 	if (job->is_profiled) {
 		if (job->engine_usage) {
 			job->engine_usage->elapsed_ns[slot] +=
@@ -219,6 +222,9 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
 
 	panfrost_job_write_affinity(pfdev, job->requirements, js);
 
+	if (job->requirements & PANFROST_JD_REQ_CYCLE_COUNT)
+		panfrost_cycle_counter_get(pfdev);
+
 	/* start MMU, medium priority, cache clean/flush on end, clean/flush on
 	 * start */
 	cfg |= JS_CONFIG_THREAD_PRI(8) |
@@ -693,8 +699,12 @@ panfrost_reset(struct panfrost_device *pfdev,
 	spin_lock(&pfdev->js->job_lock);
 	for (i = 0; i < NUM_JOB_SLOTS; i++) {
 		for (j = 0; j < ARRAY_SIZE(pfdev->jobs[0]) && pfdev->jobs[i][j]; j++) {
+			if (pfdev->jobs[i][j]->requirements & PANFROST_JD_REQ_CYCLE_COUNT)
+				panfrost_cycle_counter_put(pfdev);
+
 			if (pfdev->jobs[i][j]->is_profiled)
 				panfrost_cycle_counter_put(pfdev->jobs[i][j]->pfdev);
+
 			pm_runtime_put_noidle(pfdev->dev);
 			panfrost_devfreq_record_idle(&pfdev->pfdevfreq);
 		}
-- 
2.45.2


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

* Re: [PATCH 1/3] drm/panfrost: Add SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY parameters
  2024-08-07 16:08 ` [PATCH 1/3] drm/panfrost: Add SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY parameters Mary Guillemard
@ 2024-08-08 10:23   ` Steven Price
  2024-08-08 10:46     ` Mary Guillemard
  2024-08-08 13:06   ` kernel test robot
  1 sibling, 1 reply; 9+ messages in thread
From: Steven Price @ 2024-08-08 10:23 UTC (permalink / raw)
  To: Mary Guillemard, linux-kernel
  Cc: dri-devel, kernel, Boris Brezillon, Rob Herring,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
	Daniel Vetter

On 07/08/2024 17:08, Mary Guillemard wrote:
> Expose system timestamp and frequency supported by the GPU.
> 
> Mali uses the generic arch timer as GPU system time so we currently
> wire cntvct_el0 and cntfrq_el0 respectively to those parameters.
> We could have directly read those values from userland but handling
> this here allows us to be future proof in case of errata related to
> timers for example.

I'm not sure what the benefit is for the kernel driver providing these
(and only on some systems)? The user space driver is still going to need
code to deal with the problematic systems.

> This new uAPI will be used in Mesa to implement timestamp queries and
> VK_KHR_calibrated_timestamps.

VK_KHR_calibrated_timestamps says it should query 'quasi simultaneously
from two time domains' - but this is purely providing an interface to
read a single counter at a time.

It _may_ be useful to report the GPU's view of time and at the same time
(or as near as possible) the architectural counters. That can be used to
deal with drift between the GPU's counters and arch counters[1].

In general we try to avoid providing an interface to something which is
unrelated to the GPU, especially when user space already has a mechanism.

Steve

[1] See Mihail's response to the panthor patches for details of
differences that might occur.

> Signed-off-by: Mary Guillemard <mary.guillemard@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c | 17 +++++++++++++++++
>  include/uapi/drm/panfrost_drm.h         |  2 ++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 671eed4ad890..d94c9bf5a7f9 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -69,6 +69,23 @@ static int panfrost_ioctl_get_param(struct drm_device *ddev, void *data, struct
>  		PANFROST_FEATURE_ARRAY(JS_FEATURES, js_features, 15);
>  		PANFROST_FEATURE(NR_CORE_GROUPS, nr_core_groups);
>  		PANFROST_FEATURE(THREAD_TLS_ALLOC, thread_tls_alloc);
> +
> +	case DRM_PANFROST_PARAM_SYSTEM_TIMESTAMP:
> +#ifdef CONFIG_ARM_ARCH_TIMER
> +		param->value = __arch_counter_get_cntvct();
> +#else
> +		param->value = 0;
> +#endif
> +		break;
> +
> +	case DRM_PANFROST_PARAM_SYSTEM_TIMESTAMP_FREQUENCY:
> +#ifdef CONFIG_ARM_ARCH_TIMER
> +		param->value = arch_timer_get_cntfrq();
> +#else
> +		param->value = 0;
> +#endif
> +		break;
> +
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h
> index 9f231d40a146..52b050e2b660 100644
> --- a/include/uapi/drm/panfrost_drm.h
> +++ b/include/uapi/drm/panfrost_drm.h
> @@ -172,6 +172,8 @@ enum drm_panfrost_param {
>  	DRM_PANFROST_PARAM_NR_CORE_GROUPS,
>  	DRM_PANFROST_PARAM_THREAD_TLS_ALLOC,
>  	DRM_PANFROST_PARAM_AFBC_FEATURES,
> +	DRM_PANFROST_PARAM_SYSTEM_TIMESTAMP,
> +	DRM_PANFROST_PARAM_SYSTEM_TIMESTAMP_FREQUENCY,
>  };
>  
>  struct drm_panfrost_get_param {


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

* Re: [PATCH 2/3] drm/panfrost: Add cycle counter job requirement
  2024-08-07 16:08 ` [PATCH 2/3] drm/panfrost: Add cycle counter job requirement Mary Guillemard
@ 2024-08-08 10:23   ` Steven Price
  0 siblings, 0 replies; 9+ messages in thread
From: Steven Price @ 2024-08-08 10:23 UTC (permalink / raw)
  To: Mary Guillemard, linux-kernel
  Cc: dri-devel, kernel, Boris Brezillon, Rob Herring, David Airlie,
	Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann

On 07/08/2024 17:08, Mary Guillemard wrote:
> Extend the uAPI with a new job requirement flag for cycle
> counters. This requirement is used by userland to indicate that a job
> requires cycle counters or system timestamp to be propagated. (for use
> with write value timestamp jobs)
> 
> We cannot enable cycle counters unconditionally as this would result in
> an increase of GPU power consumption. As a result, they should be left
> off unless required by the application.
> 
> This new uAPI will be used in Mesa to implement timestamp queries and
> VK_KHR_shader_clock.
> 
> Signed-off-by: Mary Guillemard <mary.guillemard@collabora.com>

Makes sense, but I'm not sure why this needs to be it's own patch - I'd
squash this with the next one where it actually gets used.

Steve

> ---
>  include/uapi/drm/panfrost_drm.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h
> index 52b050e2b660..568724be6628 100644
> --- a/include/uapi/drm/panfrost_drm.h
> +++ b/include/uapi/drm/panfrost_drm.h
> @@ -40,6 +40,7 @@ extern "C" {
>  #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_CYCLE_COUNT (1 << 1)
>  /**
>   * struct drm_panfrost_submit - ioctl argument for submitting commands to the 3D
>   * engine.


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

* Re: [PATCH 3/3] drm/panfrost: Handle JD_REQ_CYCLE_COUNT
  2024-08-07 16:08 ` [PATCH 3/3] drm/panfrost: Handle JD_REQ_CYCLE_COUNT Mary Guillemard
@ 2024-08-08 10:23   ` Steven Price
  0 siblings, 0 replies; 9+ messages in thread
From: Steven Price @ 2024-08-08 10:23 UTC (permalink / raw)
  To: Mary Guillemard, linux-kernel
  Cc: dri-devel, kernel, Boris Brezillon, Rob Herring,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
	Daniel Vetter

On 07/08/2024 17:08, Mary Guillemard wrote:
> If a job requires cycle counters or system timestamps propagation, we
> must enable cycle counting before issuing a job and disable it right
> after the job completes.
> 
> Since this extends the uAPI and because userland needs a way to advertise
> features like VK_KHR_shader_clock conditionally, we bumps the driver
> minor version.
> 
> Signed-off-by: Mary Guillemard <mary.guillemard@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c |  8 ++++++--
>  drivers/gpu/drm/panfrost/panfrost_job.c | 10 ++++++++++
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index d94c9bf5a7f9..fe983defdfdf 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -21,6 +21,8 @@
>  #include "panfrost_gpu.h"
>  #include "panfrost_perfcnt.h"
>  
> +#define JOB_REQUIREMENTS (PANFROST_JD_REQ_FS | PANFROST_JD_REQ_CYCLE_COUNT)
> +
>  static bool unstable_ioctls;
>  module_param_unsafe(unstable_ioctls, bool, 0600);
>  
> @@ -262,7 +264,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 && args->requirements & ~JOB_REQUIREMENTS)

I think this can be simplified to just:
if (args->requirements & ~JOB_REQUIREMENTS)

>  		return -EINVAL;
>  
>  	if (args->out_sync > 0) {
> @@ -601,6 +603,8 @@ static const struct file_operations panfrost_drm_driver_fops = {
>   * - 1.0 - initial interface
>   * - 1.1 - adds HEAP and NOEXEC flags for CREATE_BO
>   * - 1.2 - adds AFBC_FEATURES query
> + * - 1.3 - adds JD_REQ_CYCLE_COUNT job requirement for SUBMIT
> + *       - adds SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY queries

Obviously needs updating if the first patch is dropped.

>   */
>  static const struct drm_driver panfrost_drm_driver = {
>  	.driver_features	= DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ,
> @@ -614,7 +618,7 @@ static const struct drm_driver panfrost_drm_driver = {
>  	.desc			= "panfrost DRM",
>  	.date			= "20180908",
>  	.major			= 1,
> -	.minor			= 2,
> +	.minor			= 3,
>  
>  	.gem_create_object	= panfrost_gem_create_object,
>  	.gem_prime_import_sg_table = panfrost_gem_prime_import_sg_table,
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index df49d37d0e7e..d8c215c0c672 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -159,6 +159,9 @@ panfrost_dequeue_job(struct panfrost_device *pfdev, int slot)
>  	struct panfrost_job *job = pfdev->jobs[slot][0];
>  
>  	WARN_ON(!job);
> +	if (job->requirements & PANFROST_JD_REQ_CYCLE_COUNT)
> +		panfrost_cycle_counter_put(pfdev);
> +
>  	if (job->is_profiled) {
>  		if (job->engine_usage) {
>  			job->engine_usage->elapsed_ns[slot] +=
> @@ -219,6 +222,9 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
>  
>  	panfrost_job_write_affinity(pfdev, job->requirements, js);
>  
> +	if (job->requirements & PANFROST_JD_REQ_CYCLE_COUNT)
> +		panfrost_cycle_counter_get(pfdev);
> +
>  	/* start MMU, medium priority, cache clean/flush on end, clean/flush on
>  	 * start */
>  	cfg |= JS_CONFIG_THREAD_PRI(8) |
> @@ -693,8 +699,12 @@ panfrost_reset(struct panfrost_device *pfdev,
>  	spin_lock(&pfdev->js->job_lock);
>  	for (i = 0; i < NUM_JOB_SLOTS; i++) {
>  		for (j = 0; j < ARRAY_SIZE(pfdev->jobs[0]) && pfdev->jobs[i][j]; j++) {
> +			if (pfdev->jobs[i][j]->requirements & PANFROST_JD_REQ_CYCLE_COUNT)
> +				panfrost_cycle_counter_put(pfdev);
> +
>  			if (pfdev->jobs[i][j]->is_profiled)
>  				panfrost_cycle_counter_put(pfdev->jobs[i][j]->pfdev);
> +
>  			pm_runtime_put_noidle(pfdev->dev);
>  			panfrost_devfreq_record_idle(&pfdev->pfdevfreq);
>  		}

This looks correct, but it would be nice to combine the is_profiled and
REQ_CYCLE_COUNT paths. We end up with the odd situation of taking two
reference counts (per job) if global profiling is enabled and the flag
is specified.

To be honest it looks as if there could be a bit of a cleanup by
changing panfrost_reset() to use panfrost_dequeue_job(). I'm not sure
why it was written that way.

Steve


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

* Re: [PATCH 1/3] drm/panfrost: Add SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY parameters
  2024-08-08 10:23   ` Steven Price
@ 2024-08-08 10:46     ` Mary Guillemard
  0 siblings, 0 replies; 9+ messages in thread
From: Mary Guillemard @ 2024-08-08 10:46 UTC (permalink / raw)
  To: Steven Price
  Cc: linux-kernel, dri-devel, kernel, Boris Brezillon, Rob Herring,
	Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
	Daniel Vetter

Hello Steve,

> VK_KHR_calibrated_timestamps says it should query 'quasi simultaneously
> from two time domains' - but this is purely providing an interface to
> read a single counter at a time.
> 
> It _may_ be useful to report the GPU's view of time and at the same time
> (or as near as possible) the architectural counters. That can be used to
> deal with drift between the GPU's counters and arch counters[1].
> 
> In general we try to avoid providing an interface to something which is
> unrelated to the GPU, especially when user space already has a mechanism.
> 
> Steve
> 
> [1] See Mihail's response to the panthor patches for details of
> differences that might occur.

I initially didn't saw the register to get the GPU timestamp and
wrongly assumed I would have to query it from the generic arch timer.

I will make SYSTEM_TIMESTAMP returns the value of the timestamp register
on v2 and keep SYSTEM_TIMESTAMP_FREQUENCY the same way as it is at the
moment.

Thanks for the review,

Mary


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

* Re: [PATCH 1/3] drm/panfrost: Add SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY parameters
  2024-08-07 16:08 ` [PATCH 1/3] drm/panfrost: Add SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY parameters Mary Guillemard
  2024-08-08 10:23   ` Steven Price
@ 2024-08-08 13:06   ` kernel test robot
  1 sibling, 0 replies; 9+ messages in thread
From: kernel test robot @ 2024-08-08 13:06 UTC (permalink / raw)
  To: Mary Guillemard, linux-kernel
  Cc: llvm, oe-kbuild-all, dri-devel, kernel, Mary Guillemard,
	Boris Brezillon, Rob Herring, Steven Price, Maarten Lankhorst,
	Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter

Hi Mary,

kernel test robot noticed the following build errors:

[auto build test ERROR on f7f3ddb6e5c8dc7b621fd8c0903ea42190d67452]

url:    https://github.com/intel-lab-lkp/linux/commits/Mary-Guillemard/drm-panfrost-Add-SYSTEM_TIMESTAMP-and-SYSTEM_TIMESTAMP_FREQUENCY-parameters/20240808-032938
base:   f7f3ddb6e5c8dc7b621fd8c0903ea42190d67452
patch link:    https://lore.kernel.org/r/20240807160900.149154-2-mary.guillemard%40collabora.com
patch subject: [PATCH 1/3] drm/panfrost: Add SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY parameters
config: arm-defconfig (https://download.01.org/0day-ci/archive/20240808/202408082014.XKxle32n-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240808/202408082014.XKxle32n-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408082014.XKxle32n-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/panfrost/panfrost_drv.c:75:18: error: implicit declaration of function '__arch_counter_get_cntvct' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
                   param->value = __arch_counter_get_cntvct();
                                  ^
>> drivers/gpu/drm/panfrost/panfrost_drv.c:83:18: error: implicit declaration of function 'arch_timer_get_cntfrq' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
                   param->value = arch_timer_get_cntfrq();
                                  ^
   2 errors generated.


vim +/__arch_counter_get_cntvct +75 drivers/gpu/drm/panfrost/panfrost_drv.c

    26	
    27	static int panfrost_ioctl_get_param(struct drm_device *ddev, void *data, struct drm_file *file)
    28	{
    29		struct drm_panfrost_get_param *param = data;
    30		struct panfrost_device *pfdev = ddev->dev_private;
    31	
    32		if (param->pad != 0)
    33			return -EINVAL;
    34	
    35	#define PANFROST_FEATURE(name, member)			\
    36		case DRM_PANFROST_PARAM_ ## name:		\
    37			param->value = pfdev->features.member;	\
    38			break
    39	#define PANFROST_FEATURE_ARRAY(name, member, max)			\
    40		case DRM_PANFROST_PARAM_ ## name ## 0 ...			\
    41			DRM_PANFROST_PARAM_ ## name ## max:			\
    42			param->value = pfdev->features.member[param->param -	\
    43				DRM_PANFROST_PARAM_ ## name ## 0];		\
    44			break
    45	
    46		switch (param->param) {
    47			PANFROST_FEATURE(GPU_PROD_ID, id);
    48			PANFROST_FEATURE(GPU_REVISION, revision);
    49			PANFROST_FEATURE(SHADER_PRESENT, shader_present);
    50			PANFROST_FEATURE(TILER_PRESENT, tiler_present);
    51			PANFROST_FEATURE(L2_PRESENT, l2_present);
    52			PANFROST_FEATURE(STACK_PRESENT, stack_present);
    53			PANFROST_FEATURE(AS_PRESENT, as_present);
    54			PANFROST_FEATURE(JS_PRESENT, js_present);
    55			PANFROST_FEATURE(L2_FEATURES, l2_features);
    56			PANFROST_FEATURE(CORE_FEATURES, core_features);
    57			PANFROST_FEATURE(TILER_FEATURES, tiler_features);
    58			PANFROST_FEATURE(MEM_FEATURES, mem_features);
    59			PANFROST_FEATURE(MMU_FEATURES, mmu_features);
    60			PANFROST_FEATURE(THREAD_FEATURES, thread_features);
    61			PANFROST_FEATURE(MAX_THREADS, max_threads);
    62			PANFROST_FEATURE(THREAD_MAX_WORKGROUP_SZ,
    63					thread_max_workgroup_sz);
    64			PANFROST_FEATURE(THREAD_MAX_BARRIER_SZ,
    65					thread_max_barrier_sz);
    66			PANFROST_FEATURE(COHERENCY_FEATURES, coherency_features);
    67			PANFROST_FEATURE(AFBC_FEATURES, afbc_features);
    68			PANFROST_FEATURE_ARRAY(TEXTURE_FEATURES, texture_features, 3);
    69			PANFROST_FEATURE_ARRAY(JS_FEATURES, js_features, 15);
    70			PANFROST_FEATURE(NR_CORE_GROUPS, nr_core_groups);
    71			PANFROST_FEATURE(THREAD_TLS_ALLOC, thread_tls_alloc);
    72	
    73		case DRM_PANFROST_PARAM_SYSTEM_TIMESTAMP:
    74	#ifdef CONFIG_ARM_ARCH_TIMER
  > 75			param->value = __arch_counter_get_cntvct();
    76	#else
    77			param->value = 0;
    78	#endif
    79			break;
    80	
    81		case DRM_PANFROST_PARAM_SYSTEM_TIMESTAMP_FREQUENCY:
    82	#ifdef CONFIG_ARM_ARCH_TIMER
  > 83			param->value = arch_timer_get_cntfrq();
    84	#else
    85			param->value = 0;
    86	#endif
    87			break;
    88	
    89		default:
    90			return -EINVAL;
    91		}
    92	
    93		return 0;
    94	}
    95	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

end of thread, other threads:[~2024-08-08 13:08 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-07 16:08 [PATCH 0/3] drm/panfrost: Wire cycle counters and timestamp info to userspace Mary Guillemard
2024-08-07 16:08 ` [PATCH 1/3] drm/panfrost: Add SYSTEM_TIMESTAMP and SYSTEM_TIMESTAMP_FREQUENCY parameters Mary Guillemard
2024-08-08 10:23   ` Steven Price
2024-08-08 10:46     ` Mary Guillemard
2024-08-08 13:06   ` kernel test robot
2024-08-07 16:08 ` [PATCH 2/3] drm/panfrost: Add cycle counter job requirement Mary Guillemard
2024-08-08 10:23   ` Steven Price
2024-08-07 16:08 ` [PATCH 3/3] drm/panfrost: Handle JD_REQ_CYCLE_COUNT Mary Guillemard
2024-08-08 10:23   ` Steven Price

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox