[PATCH 2/2 v4] intel: wait render timeout implementation

[PATCH 2/2 v4] intel: wait render timeout implementation

[Ben Widawsky]

int drm_intel_gem_bo_wait(drm_intel_bo *bo, uint64_t timeout_ns)

This should bump the libdrm version. We're waiting for context support
so we can do both features in one bump.

v2: don't return remaining timeout amount
use get param and fallback for older kernels

v3: only doing getparam at init
prototypes now have a signed input value

v4: update comments
fall back to correct polling behavior with new userspace and old kernel

Cc: Eric Anholt <eric@anholt.net>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 intel/intel_bufmgr.h     |    1 +
 intel/intel_bufmgr_gem.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/intel/intel_bufmgr.h b/intel/intel_bufmgr.h
index c197abc..fa6c4dd 100644
--- a/intel/intel_bufmgr.h
+++ b/intel/intel_bufmgr.h
@@ -184,6 +184,7 @@ int drm_intel_get_pipe_from_crtc_id(drm_intel_bufmgr *bufmgr, int crtc_id);
 
 int drm_intel_get_aperture_sizes(int fd, size_t *mappable, size_t *total);
 int drm_intel_bufmgr_gem_get_devid(drm_intel_bufmgr *bufmgr);
+int drm_intel_gem_bo_wait(drm_intel_bo *bo, int64_t timeout_ns);
 
 /* drm_intel_bufmgr_fake.c */
 drm_intel_bufmgr *drm_intel_bufmgr_fake_init(int fd,
diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index b776d2f..f8e3706 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -119,6 +119,7 @@ typedef struct _drm_intel_bufmgr_gem {
 	unsigned int has_blt : 1;
 	unsigned int has_relaxed_fencing : 1;
 	unsigned int has_llc : 1;
+	unsigned int has_wait_timeout : 1;
 	unsigned int bo_reuse : 1;
 	unsigned int no_exec : 1;
 	bool fenced_relocs;
@@ -1479,6 +1480,52 @@ drm_intel_gem_bo_wait_rendering(drm_intel_bo *bo)
 }
 
 /**
+ * Waits on a BO for the given amount of time.
+ *
+ * @bo: buffer object to wait for
+ * @timeout_ns: amount of time to wait in nanoseconds.
+ *   If value is less than 0, an infinite wait will occur.
+ *
+ * Returns 0 if the wait was successful ie. the last batch referencing the
+ * object has completed within the allotted time. Otherwise some negative return
+ * value describes the error.
+ *
+ * Similar to drm_intel_gem_bo_wait_rendering except a timeout parameter allows
+ * the operation to give up after a certain amount of time. Another subtle
+ * difference is the internal locking semantics are different (this variant does
+ * not hold the lock for the duration of the wait). This makes the wait subject
+ * to a larger userspace race window.
+ *
+ * The implementation shall wait until the object is no longer actively
+ * referenced within a batch buffer at the time of the call. The wait will
+ * not guarantee that the buffer is re-issued via another thread, or an flinked
+ * handle. Userspace must make sure this race does not occur if such precision
+ * is important.
+ */
+int drm_intel_gem_bo_wait(drm_intel_bo *bo, int64_t timeout_ns)
+{
+	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bo->bufmgr;
+	drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo;
+	struct drm_i915_gem_wait wait;
+
+	if (!bufmgr_gem->has_wait_timeout) {
+		DBG("%s:%d: Timed wait is not supported. Falling back to "
+		    "infinite wait\n", __FILE__, __LINE__);
+		if (timeout_ns) {
+			drm_intel_gem_bo_wait_rendering(bo);
+			return 0;
+		} else {
+			return drm_intel_gem_bo_busy(bo);
+		}
+	}
+
+	wait.bo_handle = bo_gem->gem_handle;
+	wait.timeout_ns = timeout_ns;
+	wait.flags = 0;
+	return drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GEM_WAIT, &wait);
+}
+
+/**
  * Sets the object to the GTT read and possibly write domain, used by the X
  * 2D driver in the absence of kernel support to do drm_intel_gem_bo_map_gtt().
  *
@@ -2898,6 +2945,10 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
 	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GETPARAM, &gp);
 	bufmgr_gem->has_relaxed_fencing = ret == 0;
 
+	gp.param = I915_PARAM_HAS_WAIT_TIMEOUT;
+	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GETPARAM, &gp);
+	bufmgr_gem->has_wait_timeout = ret == 0;
+
 	gp.param = I915_PARAM_HAS_LLC;
 	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GETPARAM, &gp);
 	if (ret != 0) {
-- 
1.7.10.3

Re: [PATCH 2/2 v4] intel: wait render timeout implementation

[Daniel Vetter]

On Wed, Jun 06, 2012 at 04:42:11PM -0700, Ben Widawsky wrote:
> int drm_intel_gem_bo_wait(drm_intel_bo *bo, uint64_t timeout_ns)
> 
> This should bump the libdrm version. We're waiting for context support
> so we can do both features in one bump.
> 
> v2: don't return remaining timeout amount
> use get param and fallback for older kernels
> 
> v3: only doing getparam at init
> prototypes now have a signed input value
> 
> v4: update comments
> fall back to correct polling behavior with new userspace and old kernel
> 
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  intel/intel_bufmgr.h     |    1 +
>  intel/intel_bufmgr_gem.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 52 insertions(+)
> 
> diff --git a/intel/intel_bufmgr.h b/intel/intel_bufmgr.h
> index c197abc..fa6c4dd 100644
> --- a/intel/intel_bufmgr.h
> +++ b/intel/intel_bufmgr.h
> @@ -184,6 +184,7 @@ int drm_intel_get_pipe_from_crtc_id(drm_intel_bufmgr *bufmgr, int crtc_id);
>  
>  int drm_intel_get_aperture_sizes(int fd, size_t *mappable, size_t *total);
>  int drm_intel_bufmgr_gem_get_devid(drm_intel_bufmgr *bufmgr);
> +int drm_intel_gem_bo_wait(drm_intel_bo *bo, int64_t timeout_ns);
>  
>  /* drm_intel_bufmgr_fake.c */
>  drm_intel_bufmgr *drm_intel_bufmgr_fake_init(int fd,
> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> index b776d2f..f8e3706 100644
> --- a/intel/intel_bufmgr_gem.c
> +++ b/intel/intel_bufmgr_gem.c
> @@ -119,6 +119,7 @@ typedef struct _drm_intel_bufmgr_gem {
>  	unsigned int has_blt : 1;
>  	unsigned int has_relaxed_fencing : 1;
>  	unsigned int has_llc : 1;
> +	unsigned int has_wait_timeout : 1;
>  	unsigned int bo_reuse : 1;
>  	unsigned int no_exec : 1;
>  	bool fenced_relocs;
> @@ -1479,6 +1480,52 @@ drm_intel_gem_bo_wait_rendering(drm_intel_bo *bo)
>  }
>  
>  /**
> + * Waits on a BO for the given amount of time.
> + *
> + * @bo: buffer object to wait for
> + * @timeout_ns: amount of time to wait in nanoseconds.
> + *   If value is less than 0, an infinite wait will occur.
> + *
> + * Returns 0 if the wait was successful ie. the last batch referencing the
> + * object has completed within the allotted time. Otherwise some negative return
> + * value describes the error.

I'm not too sure about that being correct yet. Iirc drmIoctl simply
returns -1 on error, with the (positive error number stored in errno),
and the busy function returns 1 when the thing is busy. I think we should
map both properly to the same -ETIME.
-Daniel

> + *
> + * Similar to drm_intel_gem_bo_wait_rendering except a timeout parameter allows
> + * the operation to give up after a certain amount of time. Another subtle
> + * difference is the internal locking semantics are different (this variant does
> + * not hold the lock for the duration of the wait). This makes the wait subject
> + * to a larger userspace race window.
> + *
> + * The implementation shall wait until the object is no longer actively
> + * referenced within a batch buffer at the time of the call. The wait will
> + * not guarantee that the buffer is re-issued via another thread, or an flinked
> + * handle. Userspace must make sure this race does not occur if such precision
> + * is important.
> + */
> +int drm_intel_gem_bo_wait(drm_intel_bo *bo, int64_t timeout_ns)
> +{
> +	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bo->bufmgr;
> +	drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo;
> +	struct drm_i915_gem_wait wait;
> +
> +	if (!bufmgr_gem->has_wait_timeout) {
> +		DBG("%s:%d: Timed wait is not supported. Falling back to "
> +		    "infinite wait\n", __FILE__, __LINE__);
> +		if (timeout_ns) {
> +			drm_intel_gem_bo_wait_rendering(bo);
> +			return 0;
> +		} else {
> +			return drm_intel_gem_bo_busy(bo);
> +		}
> +	}
> +
> +	wait.bo_handle = bo_gem->gem_handle;
> +	wait.timeout_ns = timeout_ns;
> +	wait.flags = 0;
> +	return drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GEM_WAIT, &wait);
> +}
> +
> +/**
>   * Sets the object to the GTT read and possibly write domain, used by the X
>   * 2D driver in the absence of kernel support to do drm_intel_gem_bo_map_gtt().
>   *
> @@ -2898,6 +2945,10 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
>  	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GETPARAM, &gp);
>  	bufmgr_gem->has_relaxed_fencing = ret == 0;
>  
> +	gp.param = I915_PARAM_HAS_WAIT_TIMEOUT;
> +	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +	bufmgr_gem->has_wait_timeout = ret == 0;
> +
>  	gp.param = I915_PARAM_HAS_LLC;
>  	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GETPARAM, &gp);
>  	if (ret != 0) {
> -- 
> 1.7.10.3
> 

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 2/2 v4] intel: wait render timeout implementation

[Ben Widawsky]

On Thu, 7 Jun 2012 09:10:08 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Jun 06, 2012 at 04:42:11PM -0700, Ben Widawsky wrote:
> > int drm_intel_gem_bo_wait(drm_intel_bo *bo, uint64_t timeout_ns)
> > 
> > This should bump the libdrm version. We're waiting for context support
> > so we can do both features in one bump.
> > 
> > v2: don't return remaining timeout amount
> > use get param and fallback for older kernels
> > 
> > v3: only doing getparam at init
> > prototypes now have a signed input value
> > 
> > v4: update comments
> > fall back to correct polling behavior with new userspace and old kernel
> > 
> > Cc: Eric Anholt <eric@anholt.net>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  intel/intel_bufmgr.h     |    1 +
> >  intel/intel_bufmgr_gem.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 52 insertions(+)
> > 
> > diff --git a/intel/intel_bufmgr.h b/intel/intel_bufmgr.h
> > index c197abc..fa6c4dd 100644
> > --- a/intel/intel_bufmgr.h
> > +++ b/intel/intel_bufmgr.h
> > @@ -184,6 +184,7 @@ int drm_intel_get_pipe_from_crtc_id(drm_intel_bufmgr *bufmgr, int crtc_id);
> >  
> >  int drm_intel_get_aperture_sizes(int fd, size_t *mappable, size_t *total);
> >  int drm_intel_bufmgr_gem_get_devid(drm_intel_bufmgr *bufmgr);
> > +int drm_intel_gem_bo_wait(drm_intel_bo *bo, int64_t timeout_ns);
> >  
> >  /* drm_intel_bufmgr_fake.c */
> >  drm_intel_bufmgr *drm_intel_bufmgr_fake_init(int fd,
> > diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> > index b776d2f..f8e3706 100644
> > --- a/intel/intel_bufmgr_gem.c
> > +++ b/intel/intel_bufmgr_gem.c
> > @@ -119,6 +119,7 @@ typedef struct _drm_intel_bufmgr_gem {
> >  	unsigned int has_blt : 1;
> >  	unsigned int has_relaxed_fencing : 1;
> >  	unsigned int has_llc : 1;
> > +	unsigned int has_wait_timeout : 1;
> >  	unsigned int bo_reuse : 1;
> >  	unsigned int no_exec : 1;
> >  	bool fenced_relocs;
> > @@ -1479,6 +1480,52 @@ drm_intel_gem_bo_wait_rendering(drm_intel_bo *bo)
> >  }
> >  
> >  /**
> > + * Waits on a BO for the given amount of time.
> > + *
> > + * @bo: buffer object to wait for
> > + * @timeout_ns: amount of time to wait in nanoseconds.
> > + *   If value is less than 0, an infinite wait will occur.
> > + *
> > + * Returns 0 if the wait was successful ie. the last batch referencing the
> > + * object has completed within the allotted time. Otherwise some negative return
> > + * value describes the error.
> 
> I'm not too sure about that being correct yet. Iirc drmIoctl simply
> returns -1 on error, with the (positive error number stored in errno),
> and the busy function returns 1 when the thing is busy. I think we should
> map both properly to the same -ETIME.
> -Daniel
> 

This was supposed to be solved by patch 1/2 where I changed the
behavior of drmIoctl. It seems Dave didn't like the idea so much.
Unless you want to take up arms with me, there will be a v5 of this
patch.

> > + *
> > + * Similar to drm_intel_gem_bo_wait_rendering except a timeout parameter allows
> > + * the operation to give up after a certain amount of time. Another subtle
> > + * difference is the internal locking semantics are different (this variant does
> > + * not hold the lock for the duration of the wait). This makes the wait subject
> > + * to a larger userspace race window.
> > + *
> > + * The implementation shall wait until the object is no longer actively
> > + * referenced within a batch buffer at the time of the call. The wait will
> > + * not guarantee that the buffer is re-issued via another thread, or an flinked
> > + * handle. Userspace must make sure this race does not occur if such precision
> > + * is important.
> > + */
> > +int drm_intel_gem_bo_wait(drm_intel_bo *bo, int64_t timeout_ns)
> > +{
> > +	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bo->bufmgr;
> > +	drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo;
> > +	struct drm_i915_gem_wait wait;
> > +
> > +	if (!bufmgr_gem->has_wait_timeout) {
> > +		DBG("%s:%d: Timed wait is not supported. Falling back to "
> > +		    "infinite wait\n", __FILE__, __LINE__);
> > +		if (timeout_ns) {
> > +			drm_intel_gem_bo_wait_rendering(bo);
> > +			return 0;
> > +		} else {
> > +			return drm_intel_gem_bo_busy(bo);
> > +		}
> > +	}
> > +
> > +	wait.bo_handle = bo_gem->gem_handle;
> > +	wait.timeout_ns = timeout_ns;
> > +	wait.flags = 0;
> > +	return drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GEM_WAIT, &wait);
> > +}
> > +
> > +/**
> >   * Sets the object to the GTT read and possibly write domain, used by the X
> >   * 2D driver in the absence of kernel support to do drm_intel_gem_bo_map_gtt().
> >   *
> > @@ -2898,6 +2945,10 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
> >  	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GETPARAM, &gp);
> >  	bufmgr_gem->has_relaxed_fencing = ret == 0;
> >  
> > +	gp.param = I915_PARAM_HAS_WAIT_TIMEOUT;
> > +	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GETPARAM, &gp);
> > +	bufmgr_gem->has_wait_timeout = ret == 0;
> > +
> >  	gp.param = I915_PARAM_HAS_LLC;
> >  	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GETPARAM, &gp);
> >  	if (ret != 0) {
> > -- 
> > 1.7.10.3
> > 
>