drm/i915: Seal busy-ioctl uABI and prevent leaking of internal ids

Tvrtko was looking through the execbuffer-ioctl and noticed that the
uABI was tightly coupled to our internal engine identifiers. Close
inspection also revealed that we leak those internal engine identifiers
through the busy-ioctl, and those internal identifiers already do not
match the user identifiers. Fortuitiously, there is only one user of the
set of busy rings from the busy-ioctl, and they only wish to choose
between the RENDER and the BLT engines.

Let's fix the userspace ABI while we still can.

v2: Update the uAPI documentation to explain the identifiers.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Testcase: igt/gem_busy
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Link: http://patchwork.freedesktop.org/patch/msgid/1452876706-21620-1-git-send-email-chris@chris-wilson.co.uk

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0ed731ed697643887efa49955427881d4801c91d..371bbb28c4713c52dc3a1ebed72ea344559697ae 100644 (file)
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4358,10 +4358,20 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
        if (ret)
                goto unref;
 
-       BUILD_BUG_ON(I915_NUM_RINGS > 16);
-       args->busy = obj->active << 16;
-       if (obj->last_write_req)
-               args->busy |= obj->last_write_req->ring->id;
+       args->busy = 0;
+       if (obj->active) {
+               int i;
+
+               for (i = 0; i < I915_NUM_RINGS; i++) {
+                       struct drm_i915_gem_request *req;
+
+                       req = obj->last_read_req[i];
+                       if (req)
+                               args->busy |= 1 << (16 + req->ring->exec_id);
+               }
+               if (obj->last_write_req)
+                       args->busy |= obj->last_write_req->ring->exec_id;
+       }
 
 unref:
        drm_gem_object_unreference(&obj->base);

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 7f47948d5c404fd4cd60f4db75a0f95a064af362..73d4347429dfd3881f6d4fb2d4d5504186b819ca 100644 (file)
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2085,6 +2085,7 @@ static int logical_render_ring_init(struct drm_device *dev)
 
        ring->name = "render ring";
        ring->id = RCS;
+       ring->exec_id = I915_EXEC_RENDER;
        ring->mmio_base = RENDER_RING_BASE;
 
        logical_ring_default_irqs(ring, GEN8_RCS_IRQ_SHIFT);
@@ -2135,6 +2136,7 @@ static int logical_bsd_ring_init(struct drm_device *dev)
 
        ring->name = "bsd ring";
        ring->id = VCS;
+       ring->exec_id = I915_EXEC_BSD;
        ring->mmio_base = GEN6_BSD_RING_BASE;
 
        logical_ring_default_irqs(ring, GEN8_VCS1_IRQ_SHIFT);
@@ -2150,6 +2152,7 @@ static int logical_bsd2_ring_init(struct drm_device *dev)
 
        ring->name = "bsd2 ring";
        ring->id = VCS2;
+       ring->exec_id = I915_EXEC_BSD;
        ring->mmio_base = GEN8_BSD2_RING_BASE;
 
        logical_ring_default_irqs(ring, GEN8_VCS2_IRQ_SHIFT);
@@ -2165,6 +2168,7 @@ static int logical_blt_ring_init(struct drm_device *dev)
 
        ring->name = "blitter ring";
        ring->id = BCS;
+       ring->exec_id = I915_EXEC_BLT;
        ring->mmio_base = BLT_RING_BASE;
 
        logical_ring_default_irqs(ring, GEN8_BCS_IRQ_SHIFT);
@@ -2180,6 +2184,7 @@ static int logical_vebox_ring_init(struct drm_device *dev)
 
        ring->name = "video enhancement ring";
        ring->id = VECS;
+       ring->exec_id = I915_EXEC_VEBOX;
        ring->mmio_base = VEBOX_RING_BASE;
 
        logical_ring_default_irqs(ring, GEN8_VECS_IRQ_SHIFT);

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index d4e33ac02efa9413f501b51cf67ecb464915a4ac..9030e2bca0c0fcc56af6cca35780d48cad5ecca9 100644 (file)
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2683,6 +2683,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 
        ring->name = "render ring";
        ring->id = RCS;
+       ring->exec_id = I915_EXEC_RENDER;
        ring->mmio_base = RENDER_RING_BASE;
 
        if (INTEL_INFO(dev)->gen >= 8) {
@@ -2831,6 +2832,7 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
 
        ring->name = "bsd ring";
        ring->id = VCS;
+       ring->exec_id = I915_EXEC_BSD;
 
        ring->write_tail = ring_write_tail;
        if (INTEL_INFO(dev)->gen >= 6) {
@@ -2907,6 +2909,7 @@ int intel_init_bsd2_ring_buffer(struct drm_device *dev)
 
        ring->name = "bsd2 ring";
        ring->id = VCS2;
+       ring->exec_id = I915_EXEC_BSD;
 
        ring->write_tail = ring_write_tail;
        ring->mmio_base = GEN8_BSD2_RING_BASE;
@@ -2937,6 +2940,7 @@ int intel_init_blt_ring_buffer(struct drm_device *dev)
 
        ring->name = "blitter ring";
        ring->id = BCS;
+       ring->exec_id = I915_EXEC_BLT;
 
        ring->mmio_base = BLT_RING_BASE;
        ring->write_tail = ring_write_tail;
@@ -2994,6 +2998,7 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev)
 
        ring->name = "video enhancement ring";
        ring->id = VECS;
+       ring->exec_id = I915_EXEC_VEBOX;
 
        ring->mmio_base = VEBOX_RING_BASE;
        ring->write_tail = ring_write_tail;

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 5b44ca63405d83b6b7c168501acd62433b8080b5..b12f2aabd1045ca31cc98bee23c982143954d467 100644 (file)
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -157,6 +157,7 @@ struct  intel_engine_cs {
        } id;
 #define I915_NUM_RINGS 5
 #define _VCS(n) (VCS + (n))
+       unsigned int exec_id;
        u32             mmio_base;
        struct          drm_device *dev;
        struct intel_ringbuffer *buffer;

diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index acf21026c78acb625b67f78c9fdf21914dc8e4a2..6a19371391fab40770634a36378e3c52426d8baa 100644 (file)
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -812,10 +812,35 @@ struct drm_i915_gem_busy {
        /** Handle of the buffer to check for busy */
        __u32 handle;
 
-       /** Return busy status (1 if busy, 0 if idle).
-        * The high word is used to indicate on which rings the object
-        * currently resides:
-        *  16:31 - busy (r or r/w) rings (16 render, 17 bsd, 18 blt, etc)
+       /** Return busy status
+        *
+        * A return of 0 implies that the object is idle (after
+        * having flushed any pending activity), and a non-zero return that
+        * the object is still in-flight on the GPU. (The GPU has not yet
+        * signaled completion for all pending requests that reference the
+        * object.)
+        *
+        * The returned dword is split into two fields to indicate both
+        * the engines on which the object is being read, and the
+        * engine on which it is currently being written (if any).
+        *
+        * The low word (bits 0:15) indicate if the object is being written
+        * to by any engine (there can only be one, as the GEM implicit
+        * synchronisation rules force writes to be serialised). Only the
+        * engine for the last write is reported.
+        *
+        * The high word (bits 16:31) are a bitmask of which engines are
+        * currently reading from the object. Multiple engines may be
+        * reading from the object simultaneously.
+        *
+        * The value of each engine is the same as specified in the
+        * EXECBUFFER2 ioctl, i.e. I915_EXEC_RENDER, I915_EXEC_BSD etc.
+        * Note I915_EXEC_DEFAULT is a symbolic value and is mapped to
+        * the I915_EXEC_RENDER engine for execution, and so it is never
+        * reported as active itself. Some hardware may have parallel
+        * execution engines, e.g. multiple media engines, which are
+        * mapped to the same identifier in the EXECBUFFER2 ioctl and
+        * so are not separately reported for busyness.
         */
        __u32 busy;
 };