Re: [PATCH v2 2/2] drm/panfrost: Add cycle counter job requirement

[Steven Price <steven.price@arm.com>]

On 14/08/2024 12:21, 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.
> 
> 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.
> 
> v2:
> - Rework commit message
> - Squash uAPI changes and implementation in this commit
> - Simplify changes based on Steven Price comments
> 
> Signed-off-by: Mary Guillemard <mary.guillemard@collabora.com>

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

Thanks,
Steve

> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c |  8 +++++--
>  drivers/gpu/drm/panfrost/panfrost_job.c | 28 +++++++++++++++----------
>  include/uapi/drm/panfrost_drm.h         |  1 +
>  3 files changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 83696d06d697..07a09f32c32e 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -25,6 +25,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);
>  
> @@ -280,7 +282,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) {
> @@ -619,6 +621,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,
> @@ -632,7 +636,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..e5e62ee356ef 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -159,16 +159,17 @@ panfrost_dequeue_job(struct panfrost_device *pfdev, int slot)
>  	struct panfrost_job *job = pfdev->jobs[slot][0];
>  
>  	WARN_ON(!job);
> -	if (job->is_profiled) {
> -		if (job->engine_usage) {
> -			job->engine_usage->elapsed_ns[slot] +=
> -				ktime_to_ns(ktime_sub(ktime_get(), job->start_time));
> -			job->engine_usage->cycles[slot] +=
> -				panfrost_cycle_counter_read(pfdev) - job->start_cycles;
> -		}
> -		panfrost_cycle_counter_put(job->pfdev);
> +
> +	if (job->is_profiled && job->engine_usage) {
> +		job->engine_usage->elapsed_ns[slot] +=
> +			ktime_to_ns(ktime_sub(ktime_get(), job->start_time));
> +		job->engine_usage->cycles[slot] +=
> +			panfrost_cycle_counter_read(pfdev) - job->start_cycles;
>  	}
>  
> +	if (job->requirements & PANFROST_JD_REQ_CYCLE_COUNT || job->is_profiled)
> +		panfrost_cycle_counter_put(pfdev);
> +
>  	pfdev->jobs[slot][0] = pfdev->jobs[slot][1];
>  	pfdev->jobs[slot][1] = NULL;
>  
> @@ -243,9 +244,13 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
>  	subslot = panfrost_enqueue_job(pfdev, js, job);
>  	/* Don't queue the job if a reset is in progress */
>  	if (!atomic_read(&pfdev->reset.pending)) {
> -		if (pfdev->profile_mode) {
> +		job->is_profiled = pfdev->profile_mode;
> +
> +		if (job->requirements & PANFROST_JD_REQ_CYCLE_COUNT ||
> +		    job->is_profiled)
>  			panfrost_cycle_counter_get(pfdev);
> -			job->is_profiled = true;
> +
> +		if (job->is_profiled) {
>  			job->start_time = ktime_get();
>  			job->start_cycles = panfrost_cycle_counter_read(pfdev);
>  		}
> @@ -693,7 +698,8 @@ 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]->is_profiled)
> +			if (pfdev->jobs[i][j]->requirements & PANFROST_JD_REQ_CYCLE_COUNT ||
> +				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);
> 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.