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.koe...@amd.com>
> Cc: Rob Herring <r...@kernel.org>
> Cc: Tomeu Vizoso <tomeu.viz...@collabora.com>
> Cc: Steven Price <steven.pr...@arm.com>
> Cc: Alyssa Rosenzweig <alyssa.rosenzw...@collabora.com>
> ---
> 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;
> }
