[PATCH] drm/i915: fix wait ioctl with negative timeout

[PATCH] drm/i915: fix wait ioctl with negative timeout

[Chia-I Wu]

When timeout_ns is negative, it really means to wait indefinitely instead of
returning immediately.  But since userspace can no longer rely on that, I am
not sure if there is any point fixing it.

Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ad55b06..3da2d62 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2787,9 +2787,9 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 		 goto out;
 
 	/* Do this after OLR check to make sure we make forward progress polling
-	 * on this IOCTL with a timeout <=0 (like busy ioctl)
+	 * on this IOCTL with a timeout == 0 (like busy ioctl)
 	 */
-	if (args->timeout_ns <= 0) {
+	if (args->timeout_ns == 0) {
 		ret = -ETIME;
 		goto out;
 	}
@@ -2798,7 +2798,8 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 	reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
 	mutex_unlock(&dev->struct_mutex);
 
-	return __wait_seqno(ring, seqno, reset_counter, true, &args->timeout_ns,
+	return __wait_seqno(ring, seqno, reset_counter, true,
+			    (args->timeout_ns > 0) ? &args->timeout_ns : NULL,
 			    file->driver_priv);
 
 out:
-- 
2.1.1

Re: [PATCH] drm/i915: fix wait ioctl with negative timeout

[Chia-I Wu]

[-- Attachment #1.1: Type: text/plain, Size: 2001 bytes --]

On Sun, Oct 12, 2014 at 1:21 AM, Chia-I Wu <olvaffe@gmail.com> wrote:

> When timeout_ns is negative, it really means to wait indefinitely instead
> of
> returning immediately.  But since userspace can no longer rely on that, I
> am
> not sure if there is any point fixing it.
>
Note that userspace may use GL_TIMEOUT_IGNORED for timeout_ns to wait
indefinitely.  The macro is defined to

#define GL_TIMEOUT_IGNORED                0xFFFFFFFFFFFFFFFFull

Prior to 3.17, the kernel would behave as expected.  But on 3.17, it would
return immediately with -ETIME if the bo is busy.


>
> Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c
> index ad55b06..3da2d62 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2787,9 +2787,9 @@ i915_gem_wait_ioctl(struct drm_device *dev, void
> *data, struct drm_file *file)
>                  goto out;
>
>         /* Do this after OLR check to make sure we make forward progress
> polling
> -        * on this IOCTL with a timeout <=0 (like busy ioctl)
> +        * on this IOCTL with a timeout == 0 (like busy ioctl)
>          */
> -       if (args->timeout_ns <= 0) {
> +       if (args->timeout_ns == 0) {
>                 ret = -ETIME;
>                 goto out;
>         }
> @@ -2798,7 +2798,8 @@ i915_gem_wait_ioctl(struct drm_device *dev, void
> *data, struct drm_file *file)
>         reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
>         mutex_unlock(&dev->struct_mutex);
>
> -       return __wait_seqno(ring, seqno, reset_counter, true,
> &args->timeout_ns,
> +       return __wait_seqno(ring, seqno, reset_counter, true,
> +                           (args->timeout_ns > 0) ? &args->timeout_ns :
> NULL,
>                             file->driver_priv);
>
>  out:
> --
> 2.1.1
>
>


-- 
olv@LunarG.com

[-- Attachment #1.2: Type: text/html, Size: 2882 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: fix wait ioctl with negative timeout

[Daniel Vetter]

On Mon, Oct 13, 2014 at 12:40:48AM +0800, Chia-I Wu wrote:
> On Sun, Oct 12, 2014 at 1:21 AM, Chia-I Wu <olvaffe@gmail.com> wrote:
> 
> > When timeout_ns is negative, it really means to wait indefinitely instead
> > of
> > returning immediately.  But since userspace can no longer rely on that, I
> > am
> > not sure if there is any point fixing it.
> >
> Note that userspace may use GL_TIMEOUT_IGNORED for timeout_ns to wait
> indefinitely.  The macro is defined to
> 
> #define GL_TIMEOUT_IGNORED                0xFFFFFFFFFFFFFFFFull
> 
> Prior to 3.17, the kernel would behave as expected.  But on 3.17, it would
> return immediately with -ETIME if the bo is busy.

That sounds like a regression, for which we need a testcase in igt and a
special-case in our wait ioctl to make sure that we have an infinite
timeout for this input value. Can you please take care of both?

I guess we could also restore the bevahiour for negative timeouts, just
for the sake of it. Would again need a testcase though. Also note that
I've recently refactored the wait ioctl testcase, so adding new subtests
should be a lot easier now.

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