Re: [PATCH 1/2] drm/sched: Fix drm_sched_entity_flush() return val

From: Philipp Stanner
Date: Tue Nov 19 2024 - 08:48:57 EST


On Tue, 2024-11-19 at 14:41 +0100, Philipp Stanner wrote:
> The documentation of drm_sched_entity_flush() states that the
> function
> shall - always - return the remaining timeout time in jiffies.
>
> However, that is not what the function actually does; in one of its
> if
> branches it simply returns the unchanged timeout value.
>
> Furthermore, the used function wait_event_timeout() doesn't always
> return the remaining timeout time.
>
> Adjust the function so that it actually does what the documentation
> states it shall do.
>
> Cc: Christian König <christian.koenig@xxxxxxx>
> Signed-off-by: Philipp Stanner <pstanner@xxxxxxxxxx>
> ---
> @AMD:
> You guys are the only ones who use the function's return code, so I
> leave it completely up to you to decide what behavior you want.
>
> But we should at least do something, because right now function
> documentation and behavior do not match.
>
> P.
> ---
>  drivers/gpu/drm/scheduler/sched_entity.c | 23 ++++++++++++++++------
> -
>  1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
> b/drivers/gpu/drm/scheduler/sched_entity.c
> index a75eede8bf8d..16b172aee453 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -278,7 +278,7 @@ static void drm_sched_entity_kill(struct
> drm_sched_entity *entity)
>   * waiting, removes the entity from the runqueue and returns an
> error when the
>   * process was killed.
>   *
> - * Returns the remaining time in jiffies left from the input timeout
> + * Returns: 0 if the timeout ellapsed, the remaining time otherwise.
>   */
>  long drm_sched_entity_flush(struct drm_sched_entity *entity, long
> timeout)
>  {
> @@ -294,15 +294,24 @@ long drm_sched_entity_flush(struct
> drm_sched_entity *entity, long timeout)
>   * The client will not queue more IBs during this fini,
> consume existing
>   * queued IBs or discard them on SIGKILL
>   */
> - if (current->flags & PF_EXITING) {
> - if (timeout)
> - ret = wait_event_timeout(
> - sched->job_scheduled,
> -
> drm_sched_entity_is_idle(entity),
> - timeout);
> + if (timeout != 0 && (current->flags & PF_EXITING)) {
> + ret = wait_event_timeout(sched->job_scheduled,
> + drm_sched_entity_is_idle(entity),
> + timeout);
> + /*
> + * wait_event_timeout() returns 1 if it timed out
> but the
> + * condition became true on timeout. We only care
> about whether
> + * it timed out or not.
> + */
> + if (ret == 1)
> + ret = 0;
>   } else {
>   wait_event_killable(sched->job_scheduled,
>      
> drm_sched_entity_is_idle(entity));
> +
> + ret -= (long)get_jiffies_64();

Ah, just recognized that this is probably nonsense – anyways, let's
discuss what we shall do here. I can fix it later.

P.

> + if (ret < 0)
> + ret = 0;
>   }
>  
>   /* For killed process disable any more IBs enqueue right now
> */