From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave Gordon Subject: Re: [RFC PATCH] drm/i915: fix potential dangling else problems in for_each_ macros Date: Wed, 2 Dec 2015 18:18:43 +0000 Message-ID: <565F3603.3060804@intel.com> 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> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------060407080100020906010809" Return-path: 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) In-Reply-To: <20151202145801.GD13583@nuc-i3427.alporthouse.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org This is a multi-part message in MIME format. --------------060407080100020906010809 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit 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. --------------060407080100020906010809 Content-Type: text/x-patch; name="0001-drm-i915-intel_ring_initialized-must-be-simple-and-i.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0001-drm-i915-intel_ring_initialized-must-be-simple-and-i.pa"; filename*1="tch" >>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 --------------060407080100020906010809 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9pbnRlbC1nZngK --------------060407080100020906010809--