From patchwork Wed Dec 2 18:18:43 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dave Gordon X-Patchwork-Id: 7750911 Return-Path: X-Original-To: patchwork-intel-gfx@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id DC06C9F350 for ; Wed, 2 Dec 2015 18:18:49 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id BAF12204F6 for ; Wed, 2 Dec 2015 18:18:48 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 8F7BB204EC for ; Wed, 2 Dec 2015 18:18:47 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 0C9C66EA95; Wed, 2 Dec 2015 10:18:47 -0800 (PST) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id F03716EA92 for ; Wed, 2 Dec 2015 10:18:44 -0800 (PST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga102.fm.intel.com with ESMTP; 02 Dec 2015 10:18:44 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,374,1444719600"; d="scan'208,223";a="832855474" Received: from dsgordon-linux2.isw.intel.com (HELO [10.102.226.88]) ([10.102.226.88]) by orsmga001.jf.intel.com with ESMTP; 02 Dec 2015 10:18:44 -0800 To: Chris Wilson References: <1448386585-4144-1-git-send-email-jani.nikula@intel.com> <20151124222601.GB16277@nuc-i3427.alporthouse.com> <20151124234726.GA29196@nuc-i3427.alporthouse.com> <20151125092323.GP17050@phenom.ffwll.local> <565EF231.3020100@intel.com> <20151202134651.GC13583@nuc-i3427.alporthouse.com> <565F0565.5000804@intel.com> <20151202145801.GD13583@nuc-i3427.alporthouse.com> From: Dave Gordon Organization: Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ Message-ID: <565F3603.3060804@intel.com> Date: Wed, 2 Dec 2015 18:18:43 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <20151202145801.GD13583@nuc-i3427.alporthouse.com> Cc: intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [RFC PATCH] drm/i915: fix potential dangling else problems in for_each_ macros 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: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, T_RP_MATCHES_RCVD, 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 On 02/12/15 14:58, Chris Wilson wrote: > On Wed, Dec 02, 2015 at 02:51:17PM +0000, Dave Gordon wrote: >> Or, put the active ones on a linked list, or keep a bitmask of which >> ones have been initialised inside the dev_priv structure, so you >> don't have to even dereference the engine[] array to work out >> whether a particular engine is initialised. Apropos which, wouldn't >> it be much more efficient to do that, because >> intel_ring_initialized() is quite heavyweight and the results surely >> don't change often, if at all, during normal operation. So we should >> only evaluate it when something has changed, and cache the bool >> result for use in all those for_each() loops! > > commit b53cd50c19f9d3c6f3308165b3e26c47b19dd041 > Author: Chris Wilson > Date: Tue Mar 31 00:25:20 2015 +0100 > > drm/i915: intel_ring_initialized() must be simple and inline > > Fixes regression from > commit 48d823878d64f93163f5a949623346748bbce1b4 > Author: Oscar Mateo > Date: Thu Jul 24 17:04:23 2014 +0100 > > drm/i915/bdw: Generic logical ring init and cleanup > > Signed-off-by: Chris Wilson > > ... > > -bool intel_ring_initialized(struct intel_engine_cs *ring); > +static inline bool > +intel_ring_initialized(struct intel_engine_cs *ring) > +{ > + return ring->dev != NULL; > +} > > Just waiting to clear a massive backlog. > > If we can use an array rather than a list (and a static assignment > certainly qualifies), use an array. > -Chris Aha! That looks useful, but it didn't apply cleanly, so I've reworked it somewhat. Here's the patch (UNTESTED) for your comments ... .Dave. From e2cc1e8f65c9b4bc465a3e1097de91d4bb6c13cd Mon Sep 17 00:00:00 2001 From: Dave Gordon Date: Wed, 2 Dec 2015 17:58:33 +0000 Subject: [RFC] drm/i915: intel_ring_initialized() must be simple and inline Organization: Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ Based on Chris Wilson's patch from 6 months ago, rebased and adapted. The idea is to use ring->dev as an indicator showing which engines have been initialised and are therefore to be included in iterations that use for_each_ring(). This allows us to avoid multiple memory references and a (non-inlined) function call on each iteration of each such loop. This version differs from Chris' primarily in the error cleanup paths, where initialisation has failed and we therefore want to mark an engine as NOT initialised. I have made the ring_cleanup() functions callable from the failure path of the ring_init() code, rather than duplicating all the steps to tear down a partially-constructed state. This also increases symmetry; ring->dev is set at the start of ring_init, and cleared at the end of ring_cleanup, in both the normal and error cases. Fixes regression from commit 48d823878d64f93163f5a949623346748bbce1b4 Author: Oscar Mateo Date: Thu Jul 24 17:04:23 2014 +0100 drm/i915/bdw: Generic logical ring init and cleanup Signed-off-by: Chris Wilson Signed-off-by: Dave Gordon --- drivers/gpu/drm/i915/intel_lrc.c | 17 +++++++++----- drivers/gpu/drm/i915/intel_ringbuffer.c | 39 +++++++++++---------------------- drivers/gpu/drm/i915/intel_ringbuffer.h | 6 ++++- 3 files changed, 30 insertions(+), 32 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 4ebafab..7644c48 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1894,8 +1894,10 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring) dev_priv = ring->dev->dev_private; - intel_logical_ring_stop(ring); - WARN_ON((I915_READ_MODE(ring) & MODE_IDLE) == 0); + if (ring->buffer) { + intel_logical_ring_stop(ring); + WARN_ON((I915_READ_MODE(ring) & MODE_IDLE) == 0); + } if (ring->cleanup) ring->cleanup(ring); @@ -1909,6 +1911,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring) } lrc_destroy_wa_ctx_obj(ring); + ring->dev = NULL; } static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *ring) @@ -1931,11 +1934,11 @@ static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin ret = i915_cmd_parser_init_ring(ring); if (ret) - return ret; + goto error; ret = intel_lr_context_deferred_alloc(ring->default_context, ring); if (ret) - return ret; + goto error; /* As this is the default context, always pin it */ ret = intel_lr_context_do_pin( @@ -1946,9 +1949,13 @@ static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin DRM_ERROR( "Failed to pin and map ringbuffer %s: %d\n", ring->name, ret); - return ret; + goto error; } + return 0; + +error: + intel_logical_ring_cleanup(ring); return ret; } diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 57d78f2..921c8a6 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -33,23 +33,6 @@ #include "i915_trace.h" #include "intel_drv.h" -bool -intel_ring_initialized(struct intel_engine_cs *ring) -{ - struct drm_device *dev = ring->dev; - - if (!dev) - return false; - - if (i915.enable_execlists) { - struct intel_context *dctx = ring->default_context; - struct intel_ringbuffer *ringbuf = dctx->engine[ring->id].ringbuf; - - return ringbuf->obj; - } else - return ring->buffer && ring->buffer->obj; -} - int __intel_ring_space(int head, int tail, int size) { int space = head - tail; @@ -2167,8 +2150,10 @@ static int intel_init_ring_buffer(struct drm_device *dev, init_waitqueue_head(&ring->irq_queue); ringbuf = intel_engine_create_ringbuffer(ring, 32 * PAGE_SIZE); - if (IS_ERR(ringbuf)) - return PTR_ERR(ringbuf); + if (IS_ERR(ringbuf)) { + ret = PTR_ERR(ringbuf); + goto error; + } ring->buffer = ringbuf; if (I915_NEED_GFX_HWS(dev)) { @@ -2197,8 +2182,7 @@ static int intel_init_ring_buffer(struct drm_device *dev, return 0; error: - intel_ringbuffer_free(ringbuf); - ring->buffer = NULL; + intel_cleanup_ring_buffer(ring); return ret; } @@ -2211,12 +2195,14 @@ void intel_cleanup_ring_buffer(struct intel_engine_cs *ring) dev_priv = to_i915(ring->dev); - intel_stop_ring_buffer(ring); - WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0); + if (ring->buffer) { + intel_stop_ring_buffer(ring); + WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0); - intel_unpin_ringbuffer_obj(ring->buffer); - intel_ringbuffer_free(ring->buffer); - ring->buffer = NULL; + intel_unpin_ringbuffer_obj(ring->buffer); + intel_ringbuffer_free(ring->buffer); + ring->buffer = NULL; + } if (ring->cleanup) ring->cleanup(ring); @@ -2225,6 +2211,7 @@ void intel_cleanup_ring_buffer(struct intel_engine_cs *ring) i915_cmd_parser_fini_ring(ring); i915_gem_batch_pool_fini(&ring->batch_pool); + ring->dev = NULL; } static int ring_wait_for_space(struct intel_engine_cs *ring, int n) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 5d1eb20..49574ff 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -350,7 +350,11 @@ struct intel_engine_cs { u32 (*get_cmd_length_mask)(u32 cmd_header); }; -bool intel_ring_initialized(struct intel_engine_cs *ring); +static inline bool +intel_ring_initialized(struct intel_engine_cs *ring) +{ + return ring->dev != NULL; +} static inline unsigned intel_ring_flag(struct intel_engine_cs *ring) -- 1.9.1