| Message ID | 1448786893-2522-8-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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);
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(-)