Re: [PATCH 5/7] drm/panfrost: switch to using drm_exec

[Steven Price <steven.price@xxxxxxx>]

On 12/07/2023 13:47, Christian König wrote:
> Just a straightforward conversion without any optimization.
> 
> Only compile tested for now.
> 
> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> Cc: Rob Herring <robh@xxxxxxxxxx>
> Cc: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx>
> Cc: Steven Price <steven.price@xxxxxxx>
> Cc: Alyssa Rosenzweig <alyssa.rosenzweig@xxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/panfrost/Kconfig        |  1 +
>  drivers/gpu/drm/panfrost/panfrost_job.c | 12 +++++++-----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/Kconfig b/drivers/gpu/drm/panfrost/Kconfig
> index e6403a9d66ad..e86a1a2fd8e1 100644
> --- a/drivers/gpu/drm/panfrost/Kconfig
> +++ b/drivers/gpu/drm/panfrost/Kconfig
> @@ -7,6 +7,7 @@ config DRM_PANFROST
>  	depends on !GENERIC_ATOMIC64    # for IOMMU_IO_PGTABLE_LPAE
>  	depends on MMU
>  	select DRM_SCHED
> +	select DRM_EXEC
>  	select IOMMU_SUPPORT
>  	select IOMMU_IO_PGTABLE_LPAE
>  	select DRM_GEM_SHMEM_HELPER
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index dbc597ab46fb..8b9206e910b5 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -8,6 +8,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/dma-resv.h>
> +#include <drm/drm_exec.h>
>  #include <drm/gpu_scheduler.h>
>  #include <drm/panfrost_drm.h>
>  
> @@ -275,13 +276,14 @@ static void panfrost_attach_object_fences(struct drm_gem_object **bos,
>  int panfrost_job_push(struct panfrost_job *job)
>  {
>  	struct panfrost_device *pfdev = job->pfdev;
> -	struct ww_acquire_ctx acquire_ctx;
> +	struct drm_exec exec;
>  	int ret = 0;
>  
> -	ret = drm_gem_lock_reservations(job->bos, job->bo_count,
> -					    &acquire_ctx);
> +	drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT);
> +	drm_exec_until_all_locked(&exec)
> +		ret = drm_exec_prepare_array(&exec, job->bos, job->bo_count, 1);

This loop bothers me - the return value is ignored until
drm_exec_until_all_locked() decides we can exit. It also reads badly
without the braces and with the "if" unindented below. The documentation
("a typical usage pattern") suggests this should be something like:

	drm_exec_until_all_locked(&exec) {
		ret = drm_exec_prepare_array(...);
		drm_exec_retry_on_contention(&exec);
		if (ret)
			goto unlock;
	}

Although I'm not sure if that's actually different because if there's no
contention that drm_exec_until_all_locked() looks like it should exit.

Perhaps it's just the name drm_exec_until_all_locked() which perhaps
should be drm_exec_until_not_contended()...?

Anyway I gave it a quick spin on a Firefly-RK3288 and didn't see any
problems, but I don't think I've got any tests which would create
contention on the BOs.

Steve

>  	if (ret)
> -		return ret;
> +		goto unlock;
>  
>  	mutex_lock(&pfdev->sched_lock);
>  	drm_sched_job_arm(&job->base);
> @@ -305,7 +307,7 @@ int panfrost_job_push(struct panfrost_job *job)
>  				      job->render_done_fence);
>  
>  unlock:
> -	drm_gem_unlock_reservations(job->bos, job->bo_count, &acquire_ctx);
> +	drm_exec_fini(&exec);
>  
>  	return ret;
>  }