[07/15] drm/i915: Check the timeout passed to i915_wait_request

Submitted by Chris Wilson on Nov. 29, 2015, 8:48 a.m.

Details

Message ID 1448786893-2522-8-git-send-email-chris@chris-wilson.co.uk
State New
Headers show
Series "Series without cover letter" ( rev: 8 7 6 5 4 3 2 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Chris Wilson Nov. 29, 2015, 8:48 a.m.
We have relied upon the sole caller (wait_ioctl) validating the timeout
argument. However, when waiting for multiple requests I forgot to ensure
that the timeout was still positive on the later requests. This is more
simply done inside __i915_wait_request.

Fixes a minor regression introduced in

commit b47161858ba13c9c7e03333132230d66e008dd55
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Mon Apr 27 13:41:17 2015 +0100

    drm/i915: Implement inter-engine read-read optimisations

where we may end up waiting for an extra jiffie for each active ring
after consuming all of the user's timeout.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Lionel Landwerlin <lionel.g.landwerlin@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f33c35c6130f..65d101b47d8e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1251,8 +1251,16 @@  int __i915_wait_request(struct drm_i915_gem_request *req,
 	if (i915_gem_request_completed(req, true))
 		return 0;
 
-	timeout_expire = timeout ?
-		jiffies + nsecs_to_jiffies_timeout((u64)*timeout) : 0;
+	timeout_expire = 0;
+	if (timeout) {
+		if (WARN_ON(*timeout < 0))
+			return -EINVAL;
+
+		if (*timeout == 0)
+			return -ETIME;
+
+		timeout_expire = jiffies + nsecs_to_jiffies_timeout(*timeout);
+	}
 
 	if (INTEL_INFO(dev_priv)->gen >= 6)
 		gen6_rps_boost(dev_priv, rps, req->emitted_jiffies);

Comments

On 29/11/15 08:48, Chris Wilson wrote:
> We have relied upon the sole caller (wait_ioctl) validating the timeout
> argument. However, when waiting for multiple requests I forgot to ensure
> that the timeout was still positive on the later requests. This is more
> simply done inside __i915_wait_request.

As discussed on IRC please mention that the extra jiffie happens because 
nsecs_to_jiffies_timeout adds it. Otherwise it is not immediately clear 
why would it wait an extra one since __i915_wait_request has explicit 
code to ensure timeout does not go negative already.

With that clarified,

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

> Fixes a minor regression introduced in
>
> commit b47161858ba13c9c7e03333132230d66e008dd55
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Mon Apr 27 13:41:17 2015 +0100
>
>      drm/i915: Implement inter-engine read-read optimisations
>
> where we may end up waiting for an extra jiffie for each active ring
> after consuming all of the user's timeout.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index f33c35c6130f..65d101b47d8e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1251,8 +1251,16 @@ int __i915_wait_request(struct drm_i915_gem_request *req,
>   	if (i915_gem_request_completed(req, true))
>   		return 0;
>
> -	timeout_expire = timeout ?
> -		jiffies + nsecs_to_jiffies_timeout((u64)*timeout) : 0;
> +	timeout_expire = 0;
> +	if (timeout) {
> +		if (WARN_ON(*timeout < 0))
> +			return -EINVAL;
> +
> +		if (*timeout == 0)
> +			return -ETIME;
> +
> +		timeout_expire = jiffies + nsecs_to_jiffies_timeout(*timeout);
> +	}
>
>   	if (INTEL_INFO(dev_priv)->gen >= 6)
>   		gen6_rps_boost(dev_priv, rps, req->emitted_jiffies);
>
On Mon, Nov 30, 2015 at 10:14:04AM +0000, Tvrtko Ursulin wrote:
> 
> On 29/11/15 08:48, Chris Wilson wrote:
> >We have relied upon the sole caller (wait_ioctl) validating the timeout
> >argument. However, when waiting for multiple requests I forgot to ensure
> >that the timeout was still positive on the later requests. This is more
> >simply done inside __i915_wait_request.
> 
> As discussed on IRC please mention that the extra jiffie happens
> because nsecs_to_jiffies_timeout adds it. Otherwise it is not
> immediately clear why would it wait an extra one since
> __i915_wait_request has explicit code to ensure timeout does not go
> negative already.

Sorry, I was under the impression that everyone knew the history of our
*to_jiffies_timeout function variants.
-Chris
On Sun, Nov 29, 2015 at 08:48:05AM +0000, Chris Wilson wrote:
> We have relied upon the sole caller (wait_ioctl) validating the timeout
> argument. However, when waiting for multiple requests I forgot to ensure
> that the timeout was still positive on the later requests. This is more
> simply done inside __i915_wait_request.
> 
> Fixes a minor regression introduced in
> 
> commit b47161858ba13c9c7e03333132230d66e008dd55
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Mon Apr 27 13:41:17 2015 +0100
> 
>     drm/i915: Implement inter-engine read-read optimisations
> 
> where we may end up waiting for an extra jiffie for each active ring
> after consuming all of the user's timeout.

where we may end up waiting for an extra jiffie (added by
nsecs_to_jiffie_timeout to guarantee minimum duration) for each active
ring after consuming all of the user's timeout
-Chris
On 30/11/15 10:19, Chris Wilson wrote:
> On Mon, Nov 30, 2015 at 10:14:04AM +0000, Tvrtko Ursulin wrote:
>>
>> On 29/11/15 08:48, Chris Wilson wrote:
>>> We have relied upon the sole caller (wait_ioctl) validating the timeout
>>> argument. However, when waiting for multiple requests I forgot to ensure
>>> that the timeout was still positive on the later requests. This is more
>>> simply done inside __i915_wait_request.
>>
>> As discussed on IRC please mention that the extra jiffie happens
>> because nsecs_to_jiffies_timeout adds it. Otherwise it is not
>> immediately clear why would it wait an extra one since
>> __i915_wait_request has explicit code to ensure timeout does not go
>> negative already.
>
> Sorry, I was under the impression that everyone knew the history of our
> *to_jiffies_timeout function variants.

I don't know if I am the only one who didn't, but when I ask or suggest 
for more or specific comments, or commit message additions during 
review, it is always genuinely for things which I think would have 
helped me with the review.  Which means I also think they would help 
anyone else getting up to speed with the code base.

Regards,

Tvrtko
On 30/11/15 10:22, Chris Wilson wrote:
> On Sun, Nov 29, 2015 at 08:48:05AM +0000, Chris Wilson wrote:
>> We have relied upon the sole caller (wait_ioctl) validating the timeout
>> argument. However, when waiting for multiple requests I forgot to ensure
>> that the timeout was still positive on the later requests. This is more
>> simply done inside __i915_wait_request.
>>
>> Fixes a minor regression introduced in
>>
>> commit b47161858ba13c9c7e03333132230d66e008dd55
>> Author: Chris Wilson <chris@chris-wilson.co.uk>
>> Date:   Mon Apr 27 13:41:17 2015 +0100
>>
>>      drm/i915: Implement inter-engine read-read optimisations
>>
>> where we may end up waiting for an extra jiffie for each active ring
>> after consuming all of the user's timeout.
>
> where we may end up waiting for an extra jiffie (added by
> nsecs_to_jiffie_timeout to guarantee minimum duration) for each active
> ring after consuming all of the user's timeout

Sounds good! R-b given previously can be applied. :)

Regards,

Tvrtko