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

[Chris Wilson]

On Wed, Feb 08, 2017 at 06:08:43PM +0000, Tvrtko Ursulin wrote:
> 
> 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 at intel.com>
> >>>Fixes: 4680816be336 ("drm/i915: Wait first for submission, before waiting for request completion")
> >>>Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >>>Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>>Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> >>>Cc: <drm-intel-fixes at 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:

for (;;) ? I prefer do while (1) personally.

Actually, the patch should be doing the if (!timeout) before the schedule
so that we check the fence condition before declaring a TIMEOUT.

Have a look at the v2 patch and see what you think.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre