Skip to contentSkip to conversation

drm/i915: fix wait ioctl with negative timeout

PATCH In review Chia-I Wu 2 participants 3 messages Oct 11, 2014 – Oct 21, 2014

Subsystems (2)
lore
Share & tools
lore

Chia-I Wu

author
#0 lore raw

PATCH 1/1 drm/i915: fix wait ioctl with negative timeout

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.

---
 drivers/gpu/drm/i915/i915_gem.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/gpu/drm/i915/i915_gem.c +4 -3
index ad55b06..3da2d62 100644
@@ -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:
Signature · 1 line
2.1.1
author
#1 lore raw
Quoted 6 lines · 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.

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.

Quoted 42 lines
---
 drivers/gpu/drm/i915/i915_gem.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/gpu/drm/i915/i915_gem.c +0 -0
index ad55b06..3da2d62 100644
@@ -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

drivers/gpu/drm/i915/i915_gem.c +4 -3
-        * 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

Signature · 1 line
[email protected]

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

#2 lore raw
Quoted 8 lines · Chia-I Wu
Quoted 6 lines · 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.

Quoted 7 lines

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

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