The kernel patches have now landed in Daniel's -next-queued tree.
Begin forwarded message:
Date: Fri, 11 May 2012 13:54:11 -0700 From: Ben Widawsky ben@bwidawsk.net To: intel-gfx@lists.freedesktop.org Cc: Ben Widawsky ben@bwidawsk.net Subject: [Intel-gfx] [PATCH] intel: add a timed wait function
drm_intel_gem_bo_wait(bo, &timeout in ns)
Signed-off-by: Ben Widawsky ben@bwidawsk.net --- include/drm/i915_drm.h | 10 ++++++++++ intel/intel_bufmgr.h | 1 + intel/intel_bufmgr_gem.c | 26 ++++++++++++++++++++++++++ 3 files changed, 37 insertions(+)
diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h index af3ce17..5dc5f12 100644 --- a/include/drm/i915_drm.h +++ b/include/drm/i915_drm.h @@ -191,6 +191,7 @@ typedef struct _drm_i915_sarea { #define DRM_I915_GEM_EXECBUFFER2 0x29 #define DRM_I915_GET_SPRITE_COLORKEY 0x2a #define DRM_I915_SET_SPRITE_COLORKEY 0x2b +#define DRM_I915_GEM_WAIT 0x2c
#define DRM_IOCTL_I915_INIT DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t) #define DRM_IOCTL_I915_FLUSH DRM_IO ( DRM_COMMAND_BASE + DRM_I915_FLUSH) @@ -234,6 +235,7 @@ typedef struct _drm_i915_sarea { #define DRM_IOCTL_I915_OVERLAY_ATTRS DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_OVERLAY_ATTRS, struct drm_intel_overlay_attrs) #define DRM_IOCTL_I915_SET_SPRITE_COLORKEY DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_SET_SPRITE_COLORKEY, struct drm_intel_sprite_colorkey) #define DRM_IOCTL_I915_GET_SPRITE_COLORKEY DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_SET_SPRITE_COLORKEY, struct drm_intel_sprite_colorkey) +#define DRM_IOCTL_I915_GEM_WAIT DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_WAIT, struct drm_i915_gem_wait) /* Allow drivers to submit batchbuffers directly to hardware, relying * on the security mechanisms provided by hardware. @@ -876,4 +878,12 @@ struct drm_intel_sprite_colorkey { __u32 flags; };
+struct drm_i915_gem_wait { + /** Handle of BO we shall wait on */ + __u32 bo_handle; + __u32 flags; + /** Number of nanoseconds to wait, Returns time remaining. */ + __u64 timeout_ns; +}; + #endif /* _I915_DRM_H_ */ diff --git a/intel/intel_bufmgr.h b/intel/intel_bufmgr.h index c197abc..d78884e 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, uint64_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..695a449 100644 --- a/intel/intel_bufmgr_gem.c +++ b/intel/intel_bufmgr_gem.c @@ -1478,6 +1478,32 @@ drm_intel_gem_bo_wait_rendering(drm_intel_bo *bo) drm_intel_gem_bo_start_gtt_access(bo, 1); }
+int drm_intel_gem_bo_wait(drm_intel_bo *bo, uint64_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; + int ret; + + if (!timeout_ns) + return -EINVAL; + + wait.bo_handle = bo_gem->gem_handle; + wait.timeout_ns = *timeout_ns; + wait.flags = 0; + ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GEM_WAIT, &wait); + if (ret) + return ret; + + if (wait.timeout_ns == 0) { + DBG("Wait timed out on buffer %d\n", bo_gem->gem_handle); + *timeout_ns = 0; + } else + *timeout_ns = wait.timeout_ns; + + return ret; +} + /** * 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().
On Sun, 27 May 2012 13:16:54 -0700, Ben Widawsky ben@bwidawsk.net wrote:
diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c index b776d2f..695a449 100644 --- a/intel/intel_bufmgr_gem.c +++ b/intel/intel_bufmgr_gem.c @@ -1478,6 +1478,32 @@ drm_intel_gem_bo_wait_rendering(drm_intel_bo *bo) drm_intel_gem_bo_start_gtt_access(bo, 1); }
+int drm_intel_gem_bo_wait(drm_intel_bo *bo, uint64_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;
- int ret;
- if (!timeout_ns)
return -EINVAL;
At least for the GL case, timeout of 0 ns wants to turn into GL_TIMEOUT_EXPIRED or GL_ALREADY_SIGNALED. -EINVAL doesn't sound like translating into either of those -- are you thinking that GL will special case 0 ns to not call this function?
- wait.bo_handle = bo_gem->gem_handle;
- wait.timeout_ns = *timeout_ns;
- wait.flags = 0;
- ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GEM_WAIT, &wait);
- if (ret)
return ret;
- if (wait.timeout_ns == 0) {
DBG("Wait timed out on buffer %d\n",
bo_gem->gem_handle);
*timeout_ns = 0;
- } else
*timeout_ns = wait.timeout_ns;
- return ret;
+}
Do we see any consumers wanting the unslept time? GL doesn't care, and not passing a pointer would be more convenient for the caller.
I guess GL_ALREADY_SIGNALED handling will be done using a check for bo_busy() before calling this.
On Wed, 30 May 2012 10:41:20 -0700 Eric Anholt eric@anholt.net wrote:
On Sun, 27 May 2012 13:16:54 -0700, Ben Widawsky ben@bwidawsk.net wrote:
diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c index b776d2f..695a449 100644 --- a/intel/intel_bufmgr_gem.c +++ b/intel/intel_bufmgr_gem.c @@ -1478,6 +1478,32 @@ drm_intel_gem_bo_wait_rendering(drm_intel_bo *bo) drm_intel_gem_bo_start_gtt_access(bo, 1); }
+int drm_intel_gem_bo_wait(drm_intel_bo *bo, uint64_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;
- int ret;
- if (!timeout_ns)
return -EINVAL;
At least for the GL case, timeout of 0 ns wants to turn into GL_TIMEOUT_EXPIRED or GL_ALREADY_SIGNALED. -EINVAL doesn't sound like translating into either of those -- are you thinking that GL will special case 0 ns to not call this function?
Well, it's timeout of NULL, not 0. 0 should do what you want. I can turn NULL into 0 just as easily, if you want?
- wait.bo_handle = bo_gem->gem_handle;
- wait.timeout_ns = *timeout_ns;
- wait.flags = 0;
- ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GEM_WAIT, &wait);
- if (ret)
return ret;
- if (wait.timeout_ns == 0) {
DBG("Wait timed out on buffer %d\n",
bo_gem->gem_handle);
*timeout_ns = 0;
- } else
*timeout_ns = wait.timeout_ns;
- return ret;
+}
Do we see any consumers wanting the unslept time? GL doesn't care, and not passing a pointer would be more convenient for the caller.
That is how I originally had it, but Daniel Vetter requested otherwise. I don't care either way. This interacts with your earlier comment as well.
I guess GL_ALREADY_SIGNALED handling will be done using a check for bo_busy() before calling this.
It shouldn't have to.
I think the outcome is either, drop the return time, or convert NULLs to 0, and everything should be fine, right?
On Wed, May 30, 2012 at 7:41 PM, Eric Anholt eric@anholt.net wrote:
I guess GL_ALREADY_SIGNALED handling will be done using a check for bo_busy() before calling this.
I've just read through the mesa code and gl_already_signalled seems to be handled already by core mesa code in _mesa_ClientWaitSync (if the driver sets syncObject->Status correctly). So I guess the current kernel code should work as-is and only the libdrm interface needs some colour adjustments around the timeout parameter. -Daniel
On Wed, 30 May 2012 21:07:57 +0200 Daniel Vetter daniel@ffwll.ch wrote:
On Wed, May 30, 2012 at 7:41 PM, Eric Anholt eric@anholt.net wrote:
I guess GL_ALREADY_SIGNALED handling will be done using a check for bo_busy() before calling this.
I've just read through the mesa code and gl_already_signalled seems to be handled already by core mesa code in _mesa_ClientWaitSync (if the driver sets syncObject->Status correctly). So I guess the current kernel code should work as-is and only the libdrm interface needs some colour adjustments around the timeout parameter. -Daniel
What have we all agreed on for the color?
On Wed, 30 May 2012 21:07:57 +0200, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, May 30, 2012 at 7:41 PM, Eric Anholt eric@anholt.net wrote:
I guess GL_ALREADY_SIGNALED handling will be done using a check for bo_busy() before calling this.
I've just read through the mesa code and gl_already_signalled seems to be handled already by core mesa code in _mesa_ClientWaitSync (if the driver sets syncObject->Status correctly). So I guess the current kernel code should work as-is and only the libdrm interface needs some colour adjustments around the timeout parameter.
Yeah, matches what I found.
Did you want pointer for timeout in the userspace api? I don't feel strongly about it, I just didn't see a use. The equivalent API I could think of was select(), where apparently linux returns time unwaited, while "everyone else" doesn't. I don't see a strong recommendation either way from that.
On Thu, May 31, 2012 at 12:35 AM, Eric Anholt eric@anholt.net wrote:
Did you want pointer for timeout in the userspace api? I don't feel strongly about it, I just didn't see a use. The equivalent API I could think of was select(), where apparently linux returns time unwaited, while "everyone else" doesn't. I don't see a strong recommendation either way from that.
I wanted the kernel to return the unwaited time (or at least a upper bound to it, with coarse clocks that's the best we can do) so that ioctl restarting after a signal does the right thing. Exposing that any further than libdrm doesn't make much sense imo. -Daniel
dri-devel@lists.freedesktop.org