From patchwork Fri Jan 15 16:51:46 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chris Wilson X-Patchwork-Id: 8043371 Return-Path: X-Original-To: patchwork-intel-gfx@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 1302FBEEE5 for ; Fri, 15 Jan 2016 16:52:02 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id F14E920461 for ; Fri, 15 Jan 2016 16:52:00 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 6C42F2044C for ; Fri, 15 Jan 2016 16:51:59 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id EC8926EBC7; Fri, 15 Jan 2016 08:51:58 -0800 (PST) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mail-wm0-f67.google.com (mail-wm0-f67.google.com [74.125.82.67]) by gabe.freedesktop.org (Postfix) with ESMTPS id 68BCB6EBC7 for ; Fri, 15 Jan 2016 08:51:57 -0800 (PST) Received: by mail-wm0-f67.google.com with SMTP id u188so3759407wmu.0 for ; Fri, 15 Jan 2016 08:51:57 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:from:to:cc:subject:date:message-id:in-reply-to:references; bh=GuhuKxwcsCVpj5WoKphS/9mlJo98Xk8bqHUag2CLz+I=; b=I7XBZIMZUxQuhaf8z4DFVqHvmdqLkY506PWnqVkBxA/Ab8eqEZMVoDYr7ePHBWHqsH Pn8lGDmmkgNP0pgiM0ZRbMifi/6sgC1C1QRlfQhyHab499HriG6xAziWEaLyQ3yMBNh8 XO7nDkmKyuAoOG7OcENJAzArszbyP+mgydC7FS74+00RqMgGjz22jODAqctNrmSsiI5D oLpzCryttNpleL9T6OimP0ePo7c1VSUCcRgzk6/ZjroBjZWFP8UDNH1WU9DWPZNo3rdI JEP9/16kQJNknXwG3R7TrIsIK8kYY5hC4BDs4YRqiKEoKbKG7X2LXpCIxfQ6TYIqCNhJ Wu8w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:sender:from:to:cc:subject:date:message-id :in-reply-to:references; bh=GuhuKxwcsCVpj5WoKphS/9mlJo98Xk8bqHUag2CLz+I=; b=F/db2hemQudyfk/ZXYPJWbNHj2l/zBTACYV1FW9Px5WoaEGdXLpnUQHKmdZLQaOXix 1FVNqvTIdeiuTdvfab/Iif7VXlzJKSylBTtYOdPVtL3kEsBr4+whVe9OTmc5m4lgfVr3 45u8AAZ1Se/jOxcvsAvU/KPpqx78luaP2ggEpGr0ziQH6sKBeA9+T6qNSLF6CyLEdVse 9MUXRH+8iafCs/JHi82HJIWecv9LgtBkH2tgVjvW+ThddWEipnDQNjd2sZB95RhlnO9Q ZSaJ3T35wG+RHUXM1mpK2p4+kPAXqbVJiAW+IoMvivcAYnn3DvHTuTYWRhv15e6WuQhs MmOw== X-Gm-Message-State: ALoCoQli/4c5tdN8NlbLZsrzjcKKz7vDWlLAT2/xt9+adqBWQ9pYhOhIJyNybpkXV0ZLRQVYA6N+mzXwrG3Fkbx9NVQn/McZBg== X-Received: by 10.194.20.105 with SMTP id m9mr10866497wje.161.1452876715568; Fri, 15 Jan 2016 08:51:55 -0800 (PST) Received: from haswell.alporthouse.com ([78.156.65.138]) by smtp.gmail.com with ESMTPSA id g187sm3291384wmf.8.2016.01.15.08.51.53 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Fri, 15 Jan 2016 08:51:54 -0800 (PST) From: Chris Wilson To: intel-gfx@lists.freedesktop.org Date: Fri, 15 Jan 2016 16:51:46 +0000 Message-Id: <1452876706-21620-1-git-send-email-chris@chris-wilson.co.uk> X-Mailer: git-send-email 2.7.0.rc3 In-Reply-To: <1452856013-29728-1-git-send-email-chris@chris-wilson.co.uk> References: <1452856013-29728-1-git-send-email-chris@chris-wilson.co.uk> Cc: Daniel Vetter Subject: [Intel-gfx] [PATCH v2] drm/i915: Seal busy-ioctl uABI and prevent leaking of internal ids X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,RP_MATCHES_RCVD,T_DKIM_INVALID,UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 Cc: Tvrtko Ursulin Cc: Daniel Vetter Reviewed-by: Tvrtko Ursulin Acked-by: Daniel Vetter --- 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 bb44bad15403..85797813a3de 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4328,10 +4328,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 f5d89c845ede..4aa209483237 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -2024,6 +2024,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); @@ -2073,6 +2074,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); @@ -2088,6 +2090,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); @@ -2103,6 +2106,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); @@ -2118,6 +2122,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 8cd8aabcc3ff..310d151c0db2 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -2680,6 +2680,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) { @@ -2828,6 +2829,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) { @@ -2904,6 +2906,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; @@ -2934,6 +2937,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; @@ -2991,6 +2995,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 7349d9258191..2067f4700caa 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -156,6 +156,7 @@ struct intel_engine_cs { } id; #define I915_NUM_RINGS 5 #define LAST_USER_RING (VECS + 1) + 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; };