Re: [PATCH 1/5] drm/sched: Create wrapper to add a syncobj dependency to job

[Christian König <christian.koenig@xxxxxxx>]

Am 08.02.23 um 20:48 schrieb Maíra Canal:
> In order to add a syncobj's fence as a dependency to a job, it is
> necessary to call drm_syncobj_find_fence() to find the fence and then
> add the dependency with drm_sched_job_add_dependency(). So, wrap these
> steps in one single function, drm_sched_job_add_syncobj_dependency().
>
> Signed-off-by: Maíra Canal <mcanal@xxxxxxxxxx>

Just one nit pick below, with that fixed Reviewed-by: Christian König 
<christian.koenig@xxxxxxx>

I'm pretty sure we have the exact same function now in amdgpu cause I 
cleaned that up just a few weeks ago to look the same as the other drivers.

Would be nice to have that new function applied there as well.

Thanks,
Christian.

> ---
>   drivers/gpu/drm/scheduler/sched_main.c | 29 ++++++++++++++++++++++++++
>   include/drm/gpu_scheduler.h            |  6 ++++++
>   2 files changed, 35 insertions(+)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 0e4378420271..d5331b1877a3 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -53,6 +53,7 @@
>   
>   #include <drm/drm_print.h>
>   #include <drm/drm_gem.h>
> +#include <drm/drm_syncobj.h>
>   #include <drm/gpu_scheduler.h>
>   #include <drm/spsc_queue.h>
>   
> @@ -718,6 +719,34 @@ int drm_sched_job_add_dependency(struct drm_sched_job *job,
>   }
>   EXPORT_SYMBOL(drm_sched_job_add_dependency);
>   
> +/**
> + * drm_sched_job_add_syncobj_dependency - adds a syncobj's fence as a job dependency
> + * @job: scheduler job to add the dependencies to
> + * @file_private: drm file private pointer
> + * @handle: syncobj handle to lookup
> + * @point: timeline point
> + *
> + * This adds the fence matching the given syncobj to @job.
> + *
> + * Returns:
> + * 0 on success, or an error on failing to expand the array.
> + */
> +int drm_sched_job_add_syncobj_dependency(struct drm_sched_job *job,
> +					 struct drm_file *file,
> +					 u32 handle,
> +					 u32 point)
> +{
> +	struct dma_fence *fence;
> +	int ret = 0;

Please don't initialize any local return variables if it isn't necessary.

This just suppresses uninitialized variables from the compiler which 
quite often have helped finding more wider bugs.

Regards,
Christian.

> +
> +	ret = drm_syncobj_find_fence(file, handle, point, 0, &fence);
> +	if (ret)
> +		return ret;
> +
> +	return drm_sched_job_add_dependency(job, fence);
> +}
> +EXPORT_SYMBOL(drm_sched_job_add_syncobj_dependency);
> +
>   /**
>    * drm_sched_job_add_resv_dependencies - add all fences from the resv to the job
>    * @job: scheduler job to add the dependencies to
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 9935d1e2ff69..4cc54f8b57b4 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -48,6 +48,8 @@ struct drm_gem_object;
>   struct drm_gpu_scheduler;
>   struct drm_sched_rq;
>   
> +struct drm_file;
> +
>   /* These are often used as an (initial) index
>    * to an array, and as such should start at 0.
>    */
> @@ -515,6 +517,10 @@ int drm_sched_job_init(struct drm_sched_job *job,
>   void drm_sched_job_arm(struct drm_sched_job *job);
>   int drm_sched_job_add_dependency(struct drm_sched_job *job,
>   				 struct dma_fence *fence);
> +int drm_sched_job_add_syncobj_dependency(struct drm_sched_job *job,
> +					 struct drm_file *file,
> +					 u32 handle,
> +					 u32 point);
>   int drm_sched_job_add_resv_dependencies(struct drm_sched_job *job,
>   					struct dma_resv *resv,
>   					enum dma_resv_usage usage);