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
> */
