The documentation for drm_sched_backend_ops.run_job() mentions a certain
function called drm_sched_job_recovery(). This function does not exist.
What's actually meant is drm_sched_resubmit_jobs(), which is by now also
deprecated.
Furthermore, the scheduler expects to "inherit" a reference on the fence
from the run_job() callback. This, so far, is also not documented.
Remove the mention of the removed function.
Discourage the behavior of drm_sched_backend_ops.run_job() being called
multiple times for the same job.
Document the necessity of incrementing the refcount in run_job().
Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
include/drm/gpu_scheduler.h | 34 ++++++++++++++++++++++++++++++----
1 file changed, 30 insertions(+), 4 deletions(-)
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 50928a7ae98e..6381baae8024 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -410,10 +410,36 @@ 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.
+ *
+ * @sched_job: the job to run
+ *
+ * The deprecated drm_sched_resubmit_jobs() (called by &struct
+ * drm_sched_backend_ops.timedout_job) can invoke this again with the
+ * same parameters. Using this is discouraged because it violates
+ * dma_fence rules, notably dma_fence_init() has to be called on
+ * already initialized fences for a second time. Moreover, this is
+ * dangerous because attempts to allocate memory might deadlock with
+ * memory management code waiting for the reset to complete.
+ *
+ * TODO: Document what drivers should do / use instead.
+ *
+ * This method is called in a workqueue context - either from the
+ * submit_wq the driver passed through drm_sched_init(), or, if the
+ * driver passed NULL, a separate, ordered workqueue the scheduler
+ * allocated.
+ *
+ * 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.
+ *
+ * Return:
+ * * On success: dma_fence the driver must signal once the hardware has
+ * completed the job ("hardware fence").
+ * * On failure: NULL or an ERR_PTR.
*/
struct dma_fence *(*run_job)(struct drm_sched_job *sched_job);
--
2.48.1Hi Philipp,
On 05/03/25 10:05, Philipp Stanner wrote:
> The documentation for drm_sched_backend_ops.run_job() mentions a certain
> function called drm_sched_job_recovery(). This function does not exist.
> What's actually meant is drm_sched_resubmit_jobs(), which is by now also
> deprecated.
>
> Furthermore, the scheduler expects to "inherit" a reference on the fence
> from the run_job() callback. This, so far, is also not documented.
>
> Remove the mention of the removed function.
>
> Discourage the behavior of drm_sched_backend_ops.run_job() being called
> multiple times for the same job.
>
> Document the necessity of incrementing the refcount in run_job().
>
> Signed-off-by: Philipp Stanner <phasta@kernel.org>
> ---
> include/drm/gpu_scheduler.h | 34 ++++++++++++++++++++++++++++++----
> 1 file changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 50928a7ae98e..6381baae8024 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -410,10 +410,36 @@ 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.
> + *
> + * @sched_job: the job to run
> + *
> + * The deprecated drm_sched_resubmit_jobs() (called by &struct
> + * drm_sched_backend_ops.timedout_job) can invoke this again with the
> + * same parameters. Using this is discouraged because it violates
> + * dma_fence rules, notably dma_fence_init() has to be called on
> + * already initialized fences for a second time. Moreover, this is
> + * dangerous because attempts to allocate memory might deadlock with
> + * memory management code waiting for the reset to complete.
Thanks for adding this paragraph! Also, thanks Christian for providing
this explanation in v5. It really helped clarify the reasoning behind
deprecating drm_sched_resubmit_jobs().
Best Regards,
- Maíra
> + *
> + * TODO: Document what drivers should do / use instead.
> + *
> + * This method is called in a workqueue context - either from the
> + * submit_wq the driver passed through drm_sched_init(), or, if the
> + * driver passed NULL, a separate, ordered workqueue the scheduler
> + * allocated.
> + *
> + * 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.
> + *
> + * Return:
> + * * On success: dma_fence the driver must signal once the hardware has
> + * completed the job ("hardware fence").
> + * * On failure: NULL or an ERR_PTR.
> */
> struct dma_fence *(*run_job)(struct drm_sched_job *sched_job);
>
On Fri, 2025-03-07 at 15:09 -0300, Maíra Canal wrote:
> Hi Philipp,
>
> On 05/03/25 10:05, Philipp Stanner wrote:
> > The documentation for drm_sched_backend_ops.run_job() mentions a
> > certain
> > function called drm_sched_job_recovery(). This function does not
> > exist.
> > What's actually meant is drm_sched_resubmit_jobs(), which is by now
> > also
> > deprecated.
> >
> > Furthermore, the scheduler expects to "inherit" a reference on the
> > fence
> > from the run_job() callback. This, so far, is also not documented.
> >
> > Remove the mention of the removed function.
> >
> > Discourage the behavior of drm_sched_backend_ops.run_job() being
> > called
> > multiple times for the same job.
> >
> > Document the necessity of incrementing the refcount in run_job().
> >
> > Signed-off-by: Philipp Stanner <phasta@kernel.org>
> > ---
> > include/drm/gpu_scheduler.h | 34 ++++++++++++++++++++++++++++++--
> > --
> > 1 file changed, 30 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/drm/gpu_scheduler.h
> > b/include/drm/gpu_scheduler.h
> > index 50928a7ae98e..6381baae8024 100644
> > --- a/include/drm/gpu_scheduler.h
> > +++ b/include/drm/gpu_scheduler.h
> > @@ -410,10 +410,36 @@ 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.
> > + *
> > + * @sched_job: the job to run
> > + *
> > + * The deprecated drm_sched_resubmit_jobs() (called by
> > &struct
> > + * drm_sched_backend_ops.timedout_job) can invoke this
> > again with the
> > + * same parameters. Using this is discouraged because it
> > violates
> > + * dma_fence rules, notably dma_fence_init() has to be
> > called on
> > + * already initialized fences for a second time. Moreover,
> > this is
> > + * dangerous because attempts to allocate memory might
> > deadlock with
> > + * memory management code waiting for the reset to
> > complete.
>
> Thanks for adding this paragraph!
You're welcome
> Also, thanks Christian for providing
> this explanation in v5. It really helped clarify the reasoning behind
> deprecating drm_sched_resubmit_jobs().
I thought a bit more about it the last days and think that you are
right and we definitely have to tell drivers with hardware scheduler
how they can achieve that without using drm_sched_resubmit_jobs().
Unfortunately, I discovered that this is quite complicated and
certainly difficult to do right.
So I'd only feel comfortable writing more docu about that once we got
more input from Christian or someone else who's got a hardware
scheduler about how they're currently doing it
Cheers
P.
>
> Best Regards,
> - Maíra
>
> > + *
> > + * TODO: Document what drivers should do / use instead.
> > + *
> > + * This method is called in a workqueue context - either
> > from the
> > + * submit_wq the driver passed through drm_sched_init(),
> > or, if the
> > + * driver passed NULL, a separate, ordered workqueue the
> > scheduler
> > + * allocated.
> > + *
> > + * 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.
> > + *
> > + * Return:
> > + * * On success: dma_fence the driver must signal once the
> > hardware has
> > + * completed the job ("hardware fence").
> > + * * On failure: NULL or an ERR_PTR.
> > */
> > struct dma_fence *(*run_job)(struct drm_sched_job
> > *sched_job);
> >
>
On Wed, Mar 05, 2025 at 02:05:50PM +0100, Philipp Stanner wrote: > /** > - * @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. > + * > + * @sched_job: the job to run > + * > + * The deprecated drm_sched_resubmit_jobs() (called by &struct > + * drm_sched_backend_ops.timedout_job) can invoke this again with the > + * same parameters. Using this is discouraged because it violates > + * dma_fence rules, notably dma_fence_init() has to be called on > + * already initialized fences for a second time. Moreover, this is > + * dangerous because attempts to allocate memory might deadlock with > + * memory management code waiting for the reset to complete. > + * > + * TODO: Document what drivers should do / use instead. No replacement? Or bespoke/roll-your-own functionality as a must? Confused... -- An old man doll... just what I always wanted! - Clara
On Wed, 2025-03-05 at 20:45 +0700, Bagas Sanjaya wrote: > On Wed, Mar 05, 2025 at 02:05:50PM +0100, Philipp Stanner wrote: > > /** > > - * @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. > > + * > > + * @sched_job: the job to run > > + * > > + * The deprecated drm_sched_resubmit_jobs() (called by > > &struct > > + * drm_sched_backend_ops.timedout_job) can invoke this > > again with the > > + * same parameters. Using this is discouraged because it > > violates > > + * dma_fence rules, notably dma_fence_init() has to be > > called on > > + * already initialized fences for a second time. Moreover, > > this is > > + * dangerous because attempts to allocate memory might > > deadlock with > > + * memory management code waiting for the reset to > > complete. > > + * > > + * TODO: Document what drivers should do / use instead. > > No replacement? Or bespoke/roll-your-own functionality as a must? > > Confused... We will document this in a follow-up. I'm trying for 2 months now [1] just to fix up some broken, outdated documentation – and that in a component that *I* am maintaining. It's very difficult to reach the relevant stakeholders, and I really want to unblock this series. Feel free to provide a proposal for the TODO based on this series or jump into the discussion here [2]. Otherwise I will propose a fix for the TODO some time the next weeks. P. [1] https://lore.kernel.org/dri-devel/20250109133710.39404-2-phasta@kernel.org/ [2] https://lore.kernel.org/dri-devel/688b5665-496d-470d-9835-0c6eadfa5569@gmail.com/
© 2016 - 2025 Red Hat, Inc.