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)
> > {
>
