| Message ID | 1466610951-3482-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 22, 2016 at 04:55:51PM +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Effectively removes one layer of indirection between the mask of > possible engines and the engine constructors. Instead of spelling > out in code the mapping of HAS_<engine> to constructors, makes > more use of the recently added data driven approach by putting > engine constructor vfuncs into the table as well. > > Effect is fewer lines of source and smaller binary. > > At the same time simplify the error handling since engine > destructors can run on unitialized engines anyway. > > Similar approach could be done for legacy submission is wanted. Yup, long term plan is to reduce as much as the needless duplication between the two/three (and kill of the dev_priv->gt.init_rings and friends). Muttering was even afoot to seperate the legacy submission code from the ring handling. > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > /** > * intel_logical_rings_init() - allocate, populate and init the Engine Command Streamers > * @dev: DRM device. > * > - * This function inits the engines for an Execlists submission style (the equivalent in the > - * legacy ringbuffer submission world would be i915_gem_init_engines). It does it only for > - * those engines that are present in the hardware. > + * This function inits the engines for an Execlists submission style (the > + * equivalent in the legacy ringbuffer submission world would be > + * i915_gem_init_engines). It does it only for those engines that are present in > + * the hardware. > * > * Return: non-zero if the initialization failed. > */ > int intel_logical_rings_init(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_engine_cs *engine; > + unsigned int i; > int ret; > > - ret = logical_render_ring_init(dev); > - if (ret) > - return ret; > + BUILD_BUG_ON((1 << RCS) != RENDER_RING); > + BUILD_BUG_ON((1 << BCS) != BLT_RING); > + BUILD_BUG_ON((1 << VCS) != BSD_RING); > + BUILD_BUG_ON((1 << VCS2) != BSD2_RING); > + BUILD_BUG_ON((1 << VECS) != VEBOX_RING); Heh, isn't that the very definition of those in the header. Planning for some array compaction? -Chris
On 22/06/16 17:10, Chris Wilson wrote: > On Wed, Jun 22, 2016 at 04:55:51PM +0100, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Effectively removes one layer of indirection between the mask of >> possible engines and the engine constructors. Instead of spelling >> out in code the mapping of HAS_<engine> to constructors, makes >> more use of the recently added data driven approach by putting >> engine constructor vfuncs into the table as well. >> >> Effect is fewer lines of source and smaller binary. >> >> At the same time simplify the error handling since engine >> destructors can run on unitialized engines anyway. >> >> Similar approach could be done for legacy submission is wanted. > > Yup, long term plan is to reduce as much as the needless duplication > between the two/three (and kill of the dev_priv->gt.init_rings and > friends). Muttering was even afoot to seperate the legacy submission > code from the ring handling. > >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > >> --- >> /** >> * intel_logical_rings_init() - allocate, populate and init the Engine Command Streamers >> * @dev: DRM device. >> * >> - * This function inits the engines for an Execlists submission style (the equivalent in the >> - * legacy ringbuffer submission world would be i915_gem_init_engines). It does it only for >> - * those engines that are present in the hardware. >> + * This function inits the engines for an Execlists submission style (the >> + * equivalent in the legacy ringbuffer submission world would be >> + * i915_gem_init_engines). It does it only for those engines that are present in >> + * the hardware. >> * >> * Return: non-zero if the initialization failed. >> */ >> int intel_logical_rings_init(struct drm_device *dev) >> { >> struct drm_i915_private *dev_priv = dev->dev_private; >> + struct intel_engine_cs *engine; >> + unsigned int i; >> int ret; >> >> - ret = logical_render_ring_init(dev); >> - if (ret) >> - return ret; >> + BUILD_BUG_ON((1 << RCS) != RENDER_RING); >> + BUILD_BUG_ON((1 << BCS) != BLT_RING); >> + BUILD_BUG_ON((1 << VCS) != BSD_RING); >> + BUILD_BUG_ON((1 << VCS2) != BSD2_RING); >> + BUILD_BUG_ON((1 << VECS) != VEBOX_RING); > > Heh, isn't that the very definition of those in the header. > Planning for some array compaction? No I was trying to protect against someone changing the definitions of RENDER_RING & co since the loop below this depends on that. Maybe it was too paranoid. Or maybe better, I could add HAS_ENGINE(id) and cement that in one place instead of this many BUILD_BUG_ONs. I'll respin anyway to remove forward decls which can be avoided with some reshuffle. Regards, Tvrtko
On Wed, Jun 22, 2016 at 05:21:01PM +0100, Tvrtko Ursulin wrote: > > On 22/06/16 17:10, Chris Wilson wrote: > >On Wed, Jun 22, 2016 at 04:55:51PM +0100, Tvrtko Ursulin wrote: > >>+ BUILD_BUG_ON((1 << RCS) != RENDER_RING); > >>+ BUILD_BUG_ON((1 << BCS) != BLT_RING); > >>+ BUILD_BUG_ON((1 << VCS) != BSD_RING); > >>+ BUILD_BUG_ON((1 << VCS2) != BSD2_RING); > >>+ BUILD_BUG_ON((1 << VECS) != VEBOX_RING); > > > >Heh, isn't that the very definition of those in the header. > >Planning for some array compaction? > > No I was trying to protect against someone changing the definitions > of RENDER_RING & co since the loop below this depends on that. Maybe > it was too paranoid. Or maybe better, I could add HAS_ENGINE(id) and > cement that in one place instead of this many BUILD_BUG_ONs. Hmm, the logical_rings[] table is ordered by id, so it should be in the same order as the mask. It is probably going to be safer to remove the RENDER_RING et al and replace them with BIT(RCS) when making the masks. -Chris
On 24/06/16 11:11, Patchwork wrote: > == Series Details == > > Series: drm/i915: Small compaction of the engine init code (rev4) > URL : https://patchwork.freedesktop.org/series/9053/ > State : failure > > == Summary == > > Series 9053v4 drm/i915: Small compaction of the engine init code > http://patchwork.freedesktop.org/api/1.0/series/9053/revisions/4/mbox > > Test gem_exec_suspend: > Subgroup basic-s3: > pass -> INCOMPLETE (fi-hsw-i7-4770k) Patch doesn't touch HSW code paths and it happened before sporadically. > Test kms_pipe_crc_basic: > Subgroup suspend-read-crc-pipe-a: > skip -> DMESG-WARN (ro-bdw-i5-5250u) > Subgroup suspend-read-crc-pipe-b: > dmesg-warn -> SKIP (ro-bdw-i5-5250u) > Subgroup suspend-read-crc-pipe-c: > skip -> DMESG-WARN (ro-bdw-i5-5250u) Unrelated https://bugs.freedesktop.org/show_bug.cgi?id=96614 > Test vgem_basic: > Subgroup create: > skip -> PASS (ro-ilk1-i5-650) > skip -> PASS (ro-hsw-i3-4010u) > skip -> PASS (ro-bdw-i7-5600u) > skip -> PASS (ro-bdw-i5-5250u) > skip -> PASS (ro-hsw-i7-4770r) > skip -> PASS (ro-ivb-i7-3770) > skip -> PASS (ro-ivb2-i7-3770) > skip -> PASS (ro-byt-n2820) > skip -> PASS (ro-skl3-i5-6260u) > Subgroup debugfs: > skip -> PASS (ro-ilk1-i5-650) > skip -> PASS (ro-hsw-i3-4010u) > skip -> PASS (ro-bdw-i7-5600u) > skip -> PASS (ro-bdw-i5-5250u) > skip -> PASS (ro-hsw-i7-4770r) > skip -> PASS (ro-ivb-i7-3770) > skip -> PASS (ro-ivb2-i7-3770) > skip -> PASS (ro-byt-n2820) > skip -> PASS (ro-skl3-i5-6260u) > Subgroup mmap: > skip -> DMESG-FAIL (ro-ilk1-i5-650) > skip -> DMESG-FAIL (ro-hsw-i3-4010u) > skip -> DMESG-FAIL (ro-bdw-i7-5600u) > skip -> DMESG-FAIL (ro-bdw-i5-5250u) > skip -> DMESG-FAIL (ro-hsw-i7-4770r) > skip -> DMESG-FAIL (ro-ivb-i7-3770) > skip -> DMESG-FAIL (ro-ivb2-i7-3770) > skip -> DMESG-FAIL (ro-byt-n2820) > skip -> DMESG-FAIL (ro-skl3-i5-6260u) > Subgroup sysfs: > skip -> PASS (ro-ilk1-i5-650) > skip -> PASS (ro-hsw-i3-4010u) > skip -> PASS (ro-bdw-i7-5600u) > skip -> PASS (ro-bdw-i5-5250u) > skip -> PASS (ro-hsw-i7-4770r) > skip -> PASS (ro-ivb-i7-3770) > skip -> PASS (ro-ivb2-i7-3770) > skip -> PASS (ro-byt-n2820) > skip -> PASS (ro-skl3-i5-6260u) > Test vgem_reload_basic: > skip -> PASS (ro-ilk1-i5-650) > skip -> PASS (ro-hsw-i3-4010u) > skip -> PASS (ro-bdw-i7-5600u) > skip -> PASS (ro-bdw-i5-5250u) > skip -> PASS (ro-hsw-i7-4770r) > skip -> PASS (ro-ivb-i7-3770) > skip -> PASS (ro-ivb2-i7-3770) > skip -> PASS (ro-byt-n2820) > skip -> PASS (ro-skl3-i5-6260u) Unrelated https://bugs.freedesktop.org/show_bug.cgi?id=96603 > fi-hsw-i7-4770k total:103 pass:86 dwarn:0 dfail:0 fail:0 skip:16 > fi-kbl-qkkr total:227 pass:160 dwarn:29 dfail:0 fail:0 skip:38 > fi-skl-i5-6260u total:227 pass:202 dwarn:0 dfail:0 fail:1 skip:24 > fi-skl-i7-6700k total:227 pass:188 dwarn:0 dfail:0 fail:1 skip:38 > fi-snb-i7-2600 total:227 pass:174 dwarn:0 dfail:0 fail:1 skip:52 > ro-bdw-i5-5250u total:227 pass:201 dwarn:3 dfail:1 fail:1 skip:21 > ro-bdw-i7-5600u total:227 pass:189 dwarn:0 dfail:1 fail:0 skip:37 > ro-bsw-n3050 total:227 pass:176 dwarn:0 dfail:1 fail:2 skip:48 > ro-byt-n2820 total:227 pass:177 dwarn:0 dfail:1 fail:4 skip:45 > ro-hsw-i3-4010u total:227 pass:194 dwarn:0 dfail:1 fail:1 skip:31 > ro-hsw-i7-4770r total:227 pass:194 dwarn:0 dfail:1 fail:1 skip:31 > ro-ilk1-i5-650 total:222 pass:154 dwarn:0 dfail:1 fail:2 skip:65 > ro-ivb-i7-3770 total:227 pass:185 dwarn:0 dfail:1 fail:1 skip:40 > ro-ivb2-i7-3770 total:227 pass:189 dwarn:0 dfail:1 fail:1 skip:36 > ro-skl3-i5-6260u total:227 pass:205 dwarn:1 dfail:1 fail:1 skip:19 > ro-bdw-i7-5557U failed to connect after reboot > > Results at /archive/results/CI_IGT_test/RO_Patchwork_1288/ > > 8e5ac92 drm-intel-nightly: 2016y-06m-22d-18h-10m-30s UTC integration manifest > 7b807e8 drm/i915: Small compaction of the engine init code Merged to dinq, thanks for the review. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index debed011a958..619be2a9d5d1 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -2016,12 +2016,16 @@ lrc_setup_hws(struct intel_engine_cs *engine, return 0; } +static int logical_render_ring_init(struct intel_engine_cs *engine); +static int logical_ring_init(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; + int (*init)(struct intel_engine_cs *engine); } logical_rings[] = { [RCS] = { .name = "render ring", @@ -2029,6 +2033,7 @@ static const struct logical_ring_info { .guc_id = GUC_RENDER_ENGINE, .mmio_base = RENDER_RING_BASE, .irq_shift = GEN8_RCS_IRQ_SHIFT, + .init = logical_render_ring_init, }, [BCS] = { .name = "blitter ring", @@ -2036,6 +2041,7 @@ static const struct logical_ring_info { .guc_id = GUC_BLITTER_ENGINE, .mmio_base = BLT_RING_BASE, .irq_shift = GEN8_BCS_IRQ_SHIFT, + .init = logical_ring_init, }, [VCS] = { .name = "bsd ring", @@ -2043,6 +2049,7 @@ static const struct logical_ring_info { .guc_id = GUC_VIDEO_ENGINE, .mmio_base = GEN6_BSD_RING_BASE, .irq_shift = GEN8_VCS1_IRQ_SHIFT, + .init = logical_ring_init, }, [VCS2] = { .name = "bsd2 ring", @@ -2050,6 +2057,7 @@ static const struct logical_ring_info { .guc_id = GUC_VIDEO_ENGINE2, .mmio_base = GEN8_BSD2_RING_BASE, .irq_shift = GEN8_VCS2_IRQ_SHIFT, + .init = logical_ring_init, }, [VECS] = { .name = "video enhancement ring", @@ -2057,14 +2065,14 @@ static const struct logical_ring_info { .guc_id = GUC_VIDEOENHANCE_ENGINE, .mmio_base = VEBOX_RING_BASE, .irq_shift = GEN8_VECS_IRQ_SHIFT, + .init = logical_ring_init, }, }; static struct intel_engine_cs * -logical_ring_setup(struct drm_device *dev, enum intel_engine_id id) +logical_ring_setup(struct drm_i915_private *dev_priv, enum intel_engine_id id) { const struct logical_ring_info *info = &logical_rings[id]; - struct drm_i915_private *dev_priv = to_i915(dev); struct intel_engine_cs *engine = &dev_priv->engine[id]; enum forcewake_domains fw_domains; @@ -2107,7 +2115,7 @@ logical_ring_setup(struct drm_device *dev, enum intel_engine_id id) logical_ring_default_irqs(engine, info->irq_shift); intel_engine_init_hangcheck(engine); - i915_gem_batch_pool_init(dev, &engine->batch_pool); + i915_gem_batch_pool_init(dev_priv->dev, &engine->batch_pool); return engine; } @@ -2148,16 +2156,16 @@ error: return ret; } -static int logical_render_ring_init(struct drm_device *dev) +static int logical_render_ring_init(struct intel_engine_cs *engine) { - struct intel_engine_cs *engine = logical_ring_setup(dev, RCS); + struct drm_i915_private *dev_priv = engine->i915; int ret; - if (HAS_L3_DPF(dev)) + if (HAS_L3_DPF(dev_priv)) engine->irq_keep_mask |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT; /* Override some for render ring. */ - if (INTEL_INFO(dev)->gen >= 9) + if (INTEL_GEN(dev_priv) >= 9) engine->init_hw = gen9_init_render_ring; else engine->init_hw = gen8_init_render_ring; @@ -2189,87 +2197,44 @@ static int logical_render_ring_init(struct drm_device *dev) return ret; } -static int logical_bsd_ring_init(struct drm_device *dev) -{ - struct intel_engine_cs *engine = logical_ring_setup(dev, VCS); - - return logical_ring_init(engine); -} - -static int logical_bsd2_ring_init(struct drm_device *dev) -{ - struct intel_engine_cs *engine = logical_ring_setup(dev, VCS2); - - return logical_ring_init(engine); -} - -static int logical_blt_ring_init(struct drm_device *dev) -{ - struct intel_engine_cs *engine = logical_ring_setup(dev, BCS); - - return logical_ring_init(engine); -} - -static int logical_vebox_ring_init(struct drm_device *dev) -{ - struct intel_engine_cs *engine = logical_ring_setup(dev, VECS); - - return logical_ring_init(engine); -} - /** * intel_logical_rings_init() - allocate, populate and init the Engine Command Streamers * @dev: DRM device. * - * This function inits the engines for an Execlists submission style (the equivalent in the - * legacy ringbuffer submission world would be i915_gem_init_engines). It does it only for - * those engines that are present in the hardware. + * This function inits the engines for an Execlists submission style (the + * equivalent in the legacy ringbuffer submission world would be + * i915_gem_init_engines). It does it only for those engines that are present in + * the hardware. * * Return: non-zero if the initialization failed. */ int intel_logical_rings_init(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_engine_cs *engine; + unsigned int i; int ret; - ret = logical_render_ring_init(dev); - if (ret) - return ret; + BUILD_BUG_ON((1 << RCS) != RENDER_RING); + BUILD_BUG_ON((1 << BCS) != BLT_RING); + BUILD_BUG_ON((1 << VCS) != BSD_RING); + BUILD_BUG_ON((1 << VCS2) != BSD2_RING); + BUILD_BUG_ON((1 << VECS) != VEBOX_RING); - if (HAS_BSD(dev)) { - ret = logical_bsd_ring_init(dev); - if (ret) - goto cleanup_render_ring; - } - - if (HAS_BLT(dev)) { - ret = logical_blt_ring_init(dev); - if (ret) - goto cleanup_bsd_ring; - } - - if (HAS_VEBOX(dev)) { - ret = logical_vebox_ring_init(dev); - if (ret) - goto cleanup_blt_ring; - } - - if (HAS_BSD2(dev)) { - ret = logical_bsd2_ring_init(dev); - if (ret) - goto cleanup_vebox_ring; + for (i = 0; i < I915_NUM_ENGINES; i++) { + if (dev_priv->info.ring_mask & (1 << i)) { + engine = logical_ring_setup(dev_priv, i); + ret = logical_rings[i].init(engine); + if (ret) + goto cleanup; + } } return 0; -cleanup_vebox_ring: - intel_logical_ring_cleanup(&dev_priv->engine[VECS]); -cleanup_blt_ring: - intel_logical_ring_cleanup(&dev_priv->engine[BCS]); -cleanup_bsd_ring: - intel_logical_ring_cleanup(&dev_priv->engine[VCS]); -cleanup_render_ring: - intel_logical_ring_cleanup(&dev_priv->engine[RCS]); +cleanup: + for (i = 0; i < I915_NUM_ENGINES; i++) + intel_logical_ring_cleanup(&dev_priv->engine[i]); return ret; }