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 <[email protected]>
> > ---
> > 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
> >
