From 426960bed3217f72a1b7bb94f084d79cc616ec0f Mon Sep 17 00:00:00 2001 From: Chris Wilson Date: Fri, 15 Jan 2016 16:51:46 +0000 Subject: [PATCH] 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 Testcase: igt/gem_busy Cc: Tvrtko Ursulin Cc: Daniel Vetter Reviewed-by: Tvrtko Ursulin Acked-by: Daniel Vetter Link: http://patchwork.freedesktop.org/patch/msgid/1452876706-21620-1-git-send-email-chris@chris-wilson.co.uk --- drivers/gpu/drm/i915/i915_gem.c | 18 ++++++++++++++---- drivers/gpu/drm/i915/intel_lrc.c | 5 +++++ drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++++ drivers/gpu/drm/i915/intel_ringbuffer.h | 1 + include/uapi/drm/i915_drm.h | 33 +++++++++++++++++++++++++++++---- 5 files changed, 54 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 0ed731ed6976..371bbb28c471 100644 --- 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 7f47948d5c40..73d4347429df 100644 --- 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 d4e33ac02efa..9030e2bca0c0 100644 --- 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 5b44ca63405d..b12f2aabd104 100644 --- 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 acf21026c78a..6a19371391fa 100644 --- 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; }; -- 2.11.0