| Message ID | 1352920285-26149-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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:
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(-)