Re: [PATCH] drm/sched: Mandate usage of drm_sched_job_cleanup()

[Matthew Brost <matthew.brost@intel.com>]

On Mon, Sep 29, 2025 at 10:12:31AM -0700, Matthew Brost wrote:
> On Fri, Sep 26, 2025 at 02:36:31PM +0200, Philipp Stanner wrote:
> > drm_sched_job_cleanup()'s documentation so far uses relatively soft
> > language, only "recommending" usage of the function. To avoid memory
> > leaks and, potentiall, other bugs, however, the function has to be used.
> > 
> > Demand usage of the function explicitly.
> > 
> > Signed-off-by: Philipp Stanner <phasta@kernel.org>
> > ---
> >  drivers/gpu/drm/scheduler/sched_main.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> > index 46119aacb809..0a9df9e61963 100644
> > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > @@ -1030,7 +1030,7 @@ EXPORT_SYMBOL(drm_sched_job_has_dependency);
> >   *
> >   * Cleans up the resources allocated with drm_sched_job_init().
> >   *
> > - * Drivers should call this from their error unwind code if @job is aborted
> > + * Drivers need to call this from their error unwind code if @job is aborted
> >   * before drm_sched_job_arm() is called.
> >   *
> >   * drm_sched_job_arm() is a point of no return since it initializes the fences
> > @@ -1038,7 +1038,7 @@ EXPORT_SYMBOL(drm_sched_job_has_dependency);
> >   * submit it with drm_sched_entity_push_job() and cannot simply abort it by
> >   * calling drm_sched_job_cleanup().
> >   *
> > - * This function should be called in the &drm_sched_backend_ops.free_job callback.
> > + * This function must be called in the &drm_sched_backend_ops.free_job callback.
> 
> I believe both the existing documentation and the new version are
> incorrect.
> 
> Also, the free_job callback is probably misnamed. This ties into a
> statement made during your XDC presentation today that confused me. The
> DRM scheduler only holds a reference to the driver-allocated job from
> the time run_job is called (i.e., once drm_sched_job_arm is invoked)
> until the job’s fence signals. It does not own the responsibility of
> freeing the job — the driver should manage the job’s reference count and
> free it as appropriate.
> 
> For most drivers, the only reference is the creation reference, which is
> transferred to the DRM scheduler after drm_sched_job_arm and released in
> free_job.
> 
> So, I think what really needs to be documented is this reference
> counting transfer. drm_sched_job_cleanup is actually associated with
> drm_sched_job_init — meaning drm_sched_job_cleanup must be called if
> drm_sched_job_init was called. This is typically handled by the driver
> through reference counting of the job.
> 
> Finally, I think free_job should probably be renamed to something like
> sched_job_put to better reflect its purpose.

Jetlagged - s/sched_job_put/put_job/

Matt

> 
> We can discuss this more in person on Wednesday at the workshop or grab
> in the hallway.
> 
> Matt
> 
> >   */
> >  void drm_sched_job_cleanup(struct drm_sched_job *job)
> >  {
> > -- 
> > 2.49.0
> >