Re: [PATCH 2/2] drm/sched: Fix docu of drm_sched_entity_flush()

[Philipp Stanner]

On Tue, 2024-11-19 at 15:27 +0100, Christian König wrote:
> Am 19.11.24 um 14:41 schrieb Philipp Stanner:
> > drm_sched_entity_flush()'s documentation states that an error is
> > being
> > returned when "the process was killed". That is not what the
> > function
> > actually does.
> > 
> > Furthermore, it contains an inprecise statement about how the
> > function
> > is part of a convenience wrapper.
> > 
> > Move that statement to drm_sched_entity_destroy().
> > 
> > Correct drm_sched_entity_flush()'s documentation.
> > 
> > Cc: Christian König <christian.koenig@xxxxxxx>
> > Signed-off-by: Philipp Stanner <pstanner@xxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/scheduler/sched_entity.c | 18 +++++++++---------
> >   1 file changed, 9 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
> > b/drivers/gpu/drm/scheduler/sched_entity.c
> > index 16b172aee453..7af7b448ad06 100644
> > --- a/drivers/gpu/drm/scheduler/sched_entity.c
> > +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> > @@ -270,15 +270,12 @@ static void drm_sched_entity_kill(struct
> > drm_sched_entity *entity)
> >   
> >   /**
> >    * drm_sched_entity_flush - Flush a context entity
> > - *
> >    * @entity: scheduler entity
> > - * @timeout: time to wait in for Q to become empty in jiffies.
> > - *
> > - * Splitting drm_sched_entity_fini() into two functions, The first
> > one does the
> > - * waiting, removes the entity from the runqueue and returns an
> > error when the
> > - * process was killed.
> > + * @timeout: time to wait in jiffies
> >    *
> >    * Returns: 0 if the timeout ellapsed, the remaining time
> > otherwise.
> > +
> > + * Waits at most @timeout jiffies for the entity's job queue to
> > become empty.
> >    */
> >   long drm_sched_entity_flush(struct drm_sched_entity *entity, long
> > timeout)
> >   {
> > @@ -290,7 +287,7 @@ long drm_sched_entity_flush(struct
> > drm_sched_entity *entity, long timeout)
> >   		return 0;
> >   
> >   	sched = entity->rq->sched;
> > -	/**
> > +	/*
> >   	 * The client will not queue more IBs during this fini,
> > consume existing
> >   	 * queued IBs or discard them on SIGKILL
> 
> That comment is actually not correct either.
> 
> drm_sched_entity_flush() should be used from the file_operations-
> >flush 
> function and that one can be used even without destroying the entity.
> 
> So it is perfectly possible that more and more IBs are pumped into
> the 
> entity while we wait for it to become idle.

Which would just result in drm_sched_entity_flush() timing out and
effectively not having done anything, right?

I guess we could touch that topic again when writing some docu for
scheduler teardown.

Would it be the best to just remove the comment, what do you think?

P.

> 
> Regards,
> Christian.
> 
> >   	 */
> > @@ -359,8 +356,11 @@ EXPORT_SYMBOL(drm_sched_entity_fini);
> >    * drm_sched_entity_destroy - Destroy a context entity
> >    * @entity: scheduler entity
> >    *
> > - * Calls drm_sched_entity_flush() and drm_sched_entity_fini() as a
> > - * convenience wrapper.
> > + * Convenience wrapper for entity teardown.
> > + *
> > + * Teardown of entities is split into two functions. The first
> > one,
> > + * drm_sched_entity_flush(), waits for the entity to become empty.
> > The second
> > + * one, drm_sched_entity_fini(), does the actual cleanup of the
> > entity object.
> >    */
> >   void drm_sched_entity_destroy(struct drm_sched_entity *entity)
> >   {
>