[PATCH] drm/i915: Ensure that the wait ioctl return timeout is valid

[PATCH] drm/i915: Ensure that the wait ioctl return timeout is valid

[Chris Wilson]

Due to a discrepancy between the sleep time and the amount of time we
spend waiting inside the wait_event_timeout, it is possible that we end
up computing that negative time remains in the wait_ioctl. This is
obviously a bogus result to return to userspace, and triggers a WARN, so
we need to fix up the value before propagating it back.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index cdcf19d..8e43a4f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1050,6 +1050,8 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
 	if (timeout) {
 		struct timespec sleep_time = timespec_sub(now, before);
 		*timeout = timespec_sub(*timeout, sleep_time);
+		if (end == 0 || !timespec_valid(timeout))
+			set_normalized_timespec(timeout, 0, 0);
 	}
 
 	switch (end) {
@@ -1058,8 +1060,6 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno,
 	case -ERESTARTSYS: /* Signal */
 		return (int)end;
 	case 0: /* Timeout */
-		if (timeout)
-			set_normalized_timespec(timeout, 0, 0);
 		return -ETIME;
 	default: /* Completed */
 		WARN_ON(end < 0); /* We're not aware of other errors */
@@ -2335,10 +2335,9 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 	mutex_unlock(&dev->struct_mutex);
 
 	ret = __wait_seqno(ring, seqno, true, timeout);
-	if (timeout) {
-		WARN_ON(!timespec_valid(timeout));
+	if (timeout)
 		args->timeout_ns = timespec_to_ns(timeout);
-	}
+
 	return ret;
 
 out:
-- 
1.7.10.4

Re: [PATCH] drm/i915: Ensure that the wait ioctl return timeout is valid

[Daniel Vetter]

On Wed, Nov 14, 2012 at 07:11:24PM +0000, Chris Wilson wrote:
> Due to a discrepancy between the sleep time and the amount of time we
> spend waiting inside the wait_event_timeout, it is possible that we end
> up computing that negative time remains in the wait_ioctl. This is
> obviously a bogus result to return to userspace, and triggers a WARN, so
> we need to fix up the value before propagating it back.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>

Ben, can you pls take a look at this and smash an r-b on it if you're ok?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Ensure that the wait ioctl return timeout is valid

[Ben Widawsky]

On Thu, 29 Nov 2012 13:42:17 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Nov 14, 2012 at 07:11:24PM +0000, Chris Wilson wrote:
> > Due to a discrepancy between the sleep time and the amount of time
> > we spend waiting inside the wait_event_timeout, it is possible that
> > we end up computing that negative time remains in the wait_ioctl.
> > This is obviously a bogus result to return to userspace, and
> > triggers a WARN, so we need to fix up the value before propagating
> > it back.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ben Widawsky <ben@bwidawsk.net>
> 
> Ben, can you pls take a look at this and smash an r-b on it if you're
> ok?
> 
> Thanks, Daniel

I want the real bug fixed! I do suspect it's a test bug, but I'd
prefer not to slap an r-b on until I can pass the original
problematic test. This just fixes an annoying WARN, which isn't super
important to me because it's not a regression (since it existed since
day 1), and no userspace really cares yet other than IGT.

I did review the patch previously, and it looks fine. So, the r-b is
pending.