Re: [PATCH] drm/i915: Check for timeout completion when waiting for the rq to submitted

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 08/02/2017 17:54, Chris Wilson wrote:
> On Wed, Feb 08, 2017 at 05:28:39PM +0000, Tvrtko Ursulin wrote:
>>
>> On 08/02/2017 16:54, Chris Wilson wrote:
>>> We first wait for a request to be submitted to hw and assigned a seqno,
>>> before we can wait for the hw to signal completion (otherwise we don't
>>> know the hw id we need to wait upon). Whilst waiting for the request to
>>> be submitted, we may exceed the user's timeout and need to propagate the
>>> error back.
>>>
>>> Reported-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Fixes: 4680816be336 ("drm/i915: Wait first for submission, before waiting for request completion")
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.10-rc1+
>>> ---
>>> drivers/gpu/drm/i915/i915_gem_request.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
>>> index 72b7f7d9461d..69aff559cf8e 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_request.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
>>> @@ -1084,6 +1084,9 @@ long i915_wait_request(struct drm_i915_gem_request *req,
>>> 		if (timeout < 0)
>>> 			goto complete;
>>>
>>> +		if (!timeout)
>>> +			return -ETIME;
>>> +
>>> 		GEM_BUG_ON(!i915_sw_fence_done(&req->execute));
>>> 	}
>>> 	GEM_BUG_ON(!i915_sw_fence_done(&req->submit));
>>>
>>
>> Perhaps "else if" would be more typical, but still OK for a fix.
>
> What I did later on in the series, was
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> index 72b7f7d9461d..c33f537f02b3 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -1026,7 +1026,11 @@ __i915_request_wait_for_execute(struct drm_i915_gem_request *request,
>                 }
>
>                 timeout = io_schedule_timeout(timeout);
> -       } while (timeout);
> +               if (!timeout) {
> +                       timeout = -ETIME;
> +                       break;
> +               }
> +       } while (1);
>         finish_wait(&request->execute.wait, &wait);
>
>         if (flags & I915_WAIT_LOCKED)
>
> That seemed like a more consistent pattern to use. Care to consider that as a v2?

Don't like to see "while (1)", how about:

	return timeout == 0 ? -ETIME : timeout;

For the exit of __i915_request_wait_for_execute?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx