On Thu, Jan 09, 2025 at 02:37:10PM +0100, Philipp Stanner wrote:
> From: Philipp Stanner <pstanner@xxxxxxxxxx>
>
> drm_sched_backend_ops.run_job() returns a dma_fence for the scheduler.
> That fence is signalled by the driver once the hardware completed the
> associated job. The scheduler does not increment the reference count on
> that fence, but implicitly expects to inherit this fence from run_job().
>
> This is relatively subtle and prone to misunderstandings.
>
> This implies that, to keep a reference for itself, a driver needs to
> call dma_fence_get() in addition to dma_fence_init() in that callback.
>
> It's further complicated by the fact that the scheduler even decrements
> the refcount in drm_sched_run_job_work() since it created a new
> reference in drm_sched_fence_scheduled(). It does, however, still use
> its pointer to the fence after calling dma_fence_put() - which is safe
> because of the aforementioned new reference, but actually still violates
> the refcounting rules.
>
> Improve the explanatory comment for that decrement.
>
> Move the call to dma_fence_put() to the position behind the last usage
> of the fence.
>
> Document the necessity to increment the reference count in
> drm_sched_backend_ops.run_job().
>
> Signed-off-by: Philipp Stanner <pstanner@xxxxxxxxxx>
> ---
> drivers/gpu/drm/scheduler/sched_main.c | 10 +++++++---
> include/drm/gpu_scheduler.h | 19 +++++++++++++++----
> 2 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 57da84908752..5f46c01eb01e 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -1218,15 +1218,19 @@ static void drm_sched_run_job_work(struct work_struct *w)
> drm_sched_fence_scheduled(s_fence, fence);
>
> if (!IS_ERR_OR_NULL(fence)) {
> - /* Drop for original kref_init of the fence */
> - dma_fence_put(fence);
> -
> r = dma_fence_add_callback(fence, &sched_job->cb,
> drm_sched_job_done_cb);
> if (r == -ENOENT)
> drm_sched_job_done(sched_job, fence->error);
> else if (r)
> DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n", r);
> +
> + /*
> + * s_fence took a new reference to fence in the call to
> + * drm_sched_fence_scheduled() above. The reference passed by
I think mentioning that in this context is a bit misleading. The reason we can
put the fence here, is because we stop using the local fence pointer we have a
reference for (from run_job()).
This has nothing to do with the fact that drm_sched_fence_scheduled() took its
own reference when it stored a copy of this fence pointer in a separate data
structure.
With that fixed,
Reviewed-by: Danilo Krummrich <dakr@xxxxxxxxxx>
> + * run_job() above is now not needed any longer. Drop it.
> + */
> + dma_fence_put(fence);
> } else {
> drm_sched_job_done(sched_job, IS_ERR(fence) ?
> PTR_ERR(fence) : 0);
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 95e17504e46a..d5cd2a78f27c 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -420,10 +420,21 @@ struct drm_sched_backend_ops {
> struct drm_sched_entity *s_entity);
>
> /**
> - * @run_job: Called to execute the job once all of the dependencies
> - * have been resolved. This may be called multiple times, if
> - * timedout_job() has happened and drm_sched_job_recovery()
> - * decides to try it again.
> + * @run_job: Called to execute the job once all of the dependencies
> + * have been resolved. This may be called multiple times, if
> + * timedout_job() has happened and drm_sched_job_recovery() decides to
> + * try it again.
> + *
> + * @sched_job: the job to run
> + *
> + * Returns: dma_fence the driver must signal once the hardware has
> + * completed the job ("hardware fence").
> + *
> + * Note that the scheduler expects to 'inherit' its own reference to
> + * this fence from the callback. It does not invoke an extra
> + * dma_fence_get() on it. Consequently, this callback must take a
> + * reference for the scheduler, and additional ones for the driver's
> + * respective needs.
> */
> struct dma_fence *(*run_job)(struct drm_sched_job *sched_job);
>
> --
> 2.47.1
>
