drm ioctl woes

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

drm ioctl woes

Mark Kettenis
Annying implementation details between ioctls in OpenBSD and Linux.
On OpenBSD we don't copyout the ioctl arguments back to userland if
the ioctl fails, whereas Linux does.  And of course the Intel
engineers designed an ioctl (DRM_IOCTL_I915_GEM_WAIT) that relies on
this :(.  If interrupted (so the ioctl fails with EINTR) it is
supposed to return the time remaining, such that userland can restart
without running the risk of waiting until the death of the universe.
Not entirely unreasonable, but nevertheless rather annoying.

Easiest way around this is to do the copyin/copyout ourselves in the
implementation of this ioctl.  Unfortunately that means a kernel ABI
break as we have to change the definition of the ioctl.  And our
i915_drm.h diverges a bit more from the Linux one.

ok?


Index: i915_drm.h
===================================================================
RCS file: /cvs/src/sys/dev/pci/drm/i915_drm.h,v
retrieving revision 1.19
diff -u -p -r1.19 i915_drm.h
--- i915_drm.h 15 Jun 2013 11:27:59 -0000 1.19
+++ i915_drm.h 19 Dec 2013 11:31:23 -0000
@@ -235,7 +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_GET_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)
+#define DRM_IOCTL_I915_GEM_WAIT DRM_IO(  DRM_COMMAND_BASE + DRM_I915_GEM_WAIT /*, struct drm_i915_gem_wait */)
 #define DRM_IOCTL_I915_GEM_CONTEXT_CREATE DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_CREATE, struct drm_i915_gem_context_create)
 #define DRM_IOCTL_I915_GEM_CONTEXT_DESTROY DRM_IOW (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_DESTROY, struct drm_i915_gem_context_destroy)
 #define DRM_IOCTL_I915_REG_READ DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_REG_READ, struct drm_i915_reg_read)
Index: i915/i915_gem.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/drm/i915/i915_gem.c,v
retrieving revision 1.60
diff -u -p -r1.60 i915_gem.c
--- i915/i915_gem.c 15 Dec 2013 11:42:10 -0000 1.60
+++ i915/i915_gem.c 19 Dec 2013 11:31:23 -0000
@@ -2501,13 +2509,17 @@ i915_gem_object_flush_active(struct drm_
 int
 i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 {
- struct drm_i915_gem_wait *args = data;
+ struct drm_i915_gem_wait stkbuf;
+ struct drm_i915_gem_wait *args = &stkbuf;
  struct drm_i915_gem_object *obj;
  struct intel_ring_buffer *ring = NULL;
  struct timespec timeout_stack, *timeout = NULL;
  u32 seqno = 0;
  int ret = 0;
 
+ if (copyin(*(caddr_t *)data, &stkbuf, sizeof(stkbuf)))
+ return -EFAULT;
+
  if (args->timeout_ns >= 0) {
  timeout_stack = ns_to_timespec(args->timeout_ns);
  timeout = &timeout_stack;
@@ -2552,6 +2564,8 @@ i915_gem_wait_ioctl(struct drm_device *d
  WARN_ON(!timespec_valid(timeout));
  args->timeout_ns = timespec_to_ns(timeout);
  }
+ if (copyout(&stkbuf, *(caddr_t *)data, sizeof(stkbuf)))
+ return -EFAULT;
  return ret;
 
 out:

Reply | Threaded
Open this post in threaded view
|

Re: drm ioctl woes

Philip Guenther-2
On Thu, Dec 19, 2013 at 4:56 AM, Mark Kettenis <[hidden email]> wrote:
> Annying implementation details between ioctls in OpenBSD and Linux.
> On OpenBSD we don't copyout the ioctl arguments back to userland if
> the ioctl fails, whereas Linux does.  And of course the Intel
> engineers designed an ioctl (DRM_IOCTL_I915_GEM_WAIT) that relies on
> this :(.  If interrupted (so the ioctl fails with EINTR) it is
> supposed to return the time remaining, such that userland can restart
> without running the risk of waiting until the death of the universe.
> Not entirely unreasonable, but nevertheless rather annoying.

I guess they took a different lesson from the mess that resulted from
Linux's select() actually updating the supplied timeout.
And they also ignored how pthread_cond_timedwait() uses an absolute
timeout to solve this.  <sigh>


Philip Guenther