From patchwork Fri Apr 29 09:15:04 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chris Wilson X-Patchwork-Id: 8978711 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 476409F46D for ; Fri, 29 Apr 2016 09:15:30 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 4976B2021A for ; Fri, 29 Apr 2016 09:15:29 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 416DC20219 for ; Fri, 29 Apr 2016 09:15:28 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id D84CF6EE76; Fri, 29 Apr 2016 09:15:26 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from fireflyinternet.com (mail.fireflyinternet.com [87.106.93.118]) by gabe.freedesktop.org (Postfix) with ESMTP id 0D2316EE77 for ; Fri, 29 Apr 2016 09:15:23 +0000 (UTC) X-Default-Received-SPF: pass (skip=forwardok (res=PASS)) x-ip-name=78.156.65.138; Received: from nuc-i3427.alporthouse.com (unverified [78.156.65.138]) by fireflyinternet.com (Firefly Internet (M1)) with ESMTP id 58986768-1500048 for multiple; Fri, 29 Apr 2016 10:15:25 +0100 Received: by nuc-i3427.alporthouse.com (sSMTP sendmail emulation); Fri, 29 Apr 2016 10:15:04 +0100 Date: Fri, 29 Apr 2016 10:15:04 +0100 From: Chris Wilson To: Tvrtko Ursulin Message-ID: <20160429091504.GE30680@nuc-i3427.alporthouse.com> Mail-Followup-To: Chris Wilson , Tvrtko Ursulin , intel-gfx@lists.freedesktop.org, Dave Gordon References: <1461851222-20557-1-git-send-email-chris@chris-wilson.co.uk> <1461864932-26230-1-git-send-email-chris@chris-wilson.co.uk> <572323A3.2000902@linux.intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <572323A3.2000902@linux.intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH v2] drm/i915/execlists: Refactor common engine setup 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=-5.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, 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 Fri, Apr 29, 2016 at 10:04:35AM +0100, Tvrtko Ursulin wrote: > > On 28/04/16 18:35, Chris Wilson wrote: > >Move all of the constant assignments up front and into a common > >function. This is primarily to ensure the backpointers are set as early > >as possible for later use during initialisation. > > > >v2: Use a constant struct so that all the similar values are set > >together. > > > >Signed-off-by: Chris Wilson > >Cc: Tvrtko Ursulin > >Cc: Dave Gordon > >--- > > drivers/gpu/drm/i915/intel_lrc.c | 176 +++++++++++++++++++++------------------ > > 1 file changed, 93 insertions(+), 83 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > >index 874c2515f9d4..2e0eaa9fa240 100644 > >--- a/drivers/gpu/drm/i915/intel_lrc.c > >+++ b/drivers/gpu/drm/i915/intel_lrc.c > >@@ -1921,8 +1921,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine) > > } > > > > static void > >-logical_ring_default_vfuncs(struct drm_device *dev, > >- struct intel_engine_cs *engine) > >+logical_ring_default_vfuncs(struct intel_engine_cs *engine) > > { > > /* Default vfuncs which can be overriden by each engine. */ > > engine->init_hw = gen8_init_common_ring; > >@@ -1933,7 +1932,7 @@ logical_ring_default_vfuncs(struct drm_device *dev, > > engine->emit_bb_start = gen8_emit_bb_start; > > engine->get_seqno = gen8_get_seqno; > > engine->set_seqno = gen8_set_seqno; > >- if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) { > >+ if (IS_BXT_REVID(engine->dev, 0, BXT_REVID_A1)) { > > engine->irq_seqno_barrier = bxt_a_seqno_barrier; > > engine->set_seqno = bxt_a_set_seqno; > > } > >@@ -1944,6 +1943,7 @@ logical_ring_default_irqs(struct intel_engine_cs *engine, unsigned shift) > > { > > engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << shift; > > engine->irq_keep_mask = GT_CONTEXT_SWITCH_INTERRUPT << shift; > >+ init_waitqueue_head(&engine->irq_queue); > > } > > > > static int > >@@ -1964,31 +1964,68 @@ lrc_setup_hws(struct intel_engine_cs *engine, > > return 0; > > } > > > >-static int > >-logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine) > >+static const struct logical_ring_info { > >+ const char *name; > >+ unsigned exec_id; > >+ unsigned guc_id; > >+ u32 mmio_base; > >+ unsigned irq_shift; > >+} logical_rings[] = { > >+ [RCS] = { > >+ .name = "render ring", > >+ .exec_id = I915_EXEC_RENDER, > >+ .guc_id = GUC_RENDER_ENGINE, > >+ .mmio_base = RENDER_RING_BASE, > >+ .irq_shift = GEN8_RCS_IRQ_SHIFT, > >+ }, > >+ [BCS] = { > >+ .name = "blitter ring", > >+ .exec_id = I915_EXEC_BLT, > >+ .guc_id = GUC_BLITTER_ENGINE, > >+ .mmio_base = BLT_RING_BASE, > >+ .irq_shift = GEN8_BCS_IRQ_SHIFT, > >+ }, > >+ [VCS] = { > >+ .name = "bsd ring", > >+ .exec_id = I915_EXEC_BSD, > >+ .guc_id = GUC_VIDEO_ENGINE, > >+ .mmio_base = GEN6_BSD_RING_BASE, > >+ .irq_shift = GEN8_VCS1_IRQ_SHIFT, > >+ }, > >+ [VCS2] = { > >+ .name = "bsd2 ring", > >+ .exec_id = I915_EXEC_BSD, > >+ .guc_id = GUC_VIDEO_ENGINE2, > >+ .mmio_base = GEN8_BSD2_RING_BASE, > >+ .irq_shift = GEN8_VCS2_IRQ_SHIFT, > >+ }, > >+ [VECS] = { > >+ .name = "video enhancement ring", > >+ .exec_id = I915_EXEC_VEBOX, > >+ .guc_id = GUC_VIDEOENHANCE_ENGINE, > >+ .mmio_base = VEBOX_RING_BASE, > >+ .irq_shift = GEN8_VECS_IRQ_SHIFT, > >+ }, > >+}; > >+ > >+static struct intel_engine_cs * > >+logical_ring_setup(struct drm_device *dev, enum intel_engine_id id) > > { > > Would dev_priv be better? Just to gradually move towards the correct > state of things. I have a patch queued up to do engine->i915 (1 KiB in object code reduction) next. > >+ const struct logical_ring_info *info = &logical_rings[id]; > > struct drm_i915_private *dev_priv = to_i915(dev); > >- struct intel_context *dctx = dev_priv->kernel_context; > >+ struct intel_engine_cs *engine = &dev_priv->engine[id]; > > enum forcewake_domains fw_domains; > >- int ret; > >- > >- /* Intentionally left blank. */ > >- engine->buffer = NULL; > > > > engine->dev = dev; > > Looking at usages of intel_engine_initialized... one potential > danger scenario would be interrupt noise during driver load end in > notify ring and explodes. Sounds very unlikely but theoretically it > is a regression compared to where engine->dev initialization was > before. > > We should really move away from engine->dev for this and just add an > explicit flag. Hmm. not that but I think we really should be sanitizing the irq here and enabling them last. Like: Make sense? -Chris diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 2e0eaa9fa240..2c94072ab085 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -2016,14 +2016,17 @@ logical_ring_setup(struct drm_device *dev, enum intel_engine_id id) struct intel_engine_cs *engine = &dev_priv->engine[id]; enum forcewake_domains fw_domains; - engine->dev = dev; - engine->id = id; engine->name = info->name; engine->exec_id = info->exec_id; engine->guc_id = info->guc_id; engine->mmio_base = info->mmio_base; + /* disable interrupts to this engine before we install ourselves*/ + I915_WRITE_IMR(engine, ~0); + + engine->dev = dev; + /* Intentionally left blank. */ engine->buffer = NULL;