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

[Steven Price]

On 27/05/2021 21:38, alyssa.rosenzweig at collabora.com wrote:
> From: Alyssa Rosenzweig <alyssa.rosenzweig at 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 at 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);
>  }
>  
>