* [PATCH] drm/i915: Small compaction of the engine init code
@ 2016-06-22 15:55 Tvrtko Ursulin
2016-06-22 16:10 ` Chris Wilson
` (4 more replies)
0 siblings, 5 replies; 21+ messages in thread
From: Tvrtko Ursulin @ 2016-06-22 15:55 UTC (permalink / raw)
To: Intel-gfx
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.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/intel_lrc.c | 107 +++++++++++++--------------------------
1 file changed, 36 insertions(+), 71 deletions(-)
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;
}
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] drm/i915: Small compaction of the engine init code
2016-06-22 15:55 [PATCH] drm/i915: Small compaction of the engine init code Tvrtko Ursulin
@ 2016-06-22 16:10 ` Chris Wilson
2016-06-22 16:21 ` Tvrtko Ursulin
2016-06-22 16:18 ` ✓ Ro.CI.BAT: success for " Patchwork
` (3 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Chris Wilson @ 2016-06-22 16:10 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
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
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* ✓ Ro.CI.BAT: success for drm/i915: Small compaction of the engine init code
2016-06-22 15:55 [PATCH] drm/i915: Small compaction of the engine init code Tvrtko Ursulin
2016-06-22 16:10 ` Chris Wilson
@ 2016-06-22 16:18 ` Patchwork
2016-06-22 17:09 ` ✗ Ro.CI.BAT: warning for drm/i915: Small compaction of the engine init code (rev2) Patchwork
` (2 subsequent siblings)
4 siblings, 0 replies; 21+ messages in thread
From: Patchwork @ 2016-06-22 16:18 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Small compaction of the engine init code
URL : https://patchwork.freedesktop.org/series/9053/
State : success
== Summary ==
Series 9053v1 drm/i915: Small compaction of the engine init code
http://patchwork.freedesktop.org/api/1.0/series/9053/revisions/1/mbox
Test gem_exec_flush:
Subgroup basic-batch-kernel-default-cmd:
fail -> PASS (ro-byt-n2820)
Test kms_flip:
Subgroup basic-plain-flip:
dmesg-warn -> PASS (ro-snb-i7-2620M)
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-b:
dmesg-warn -> SKIP (ro-bdw-i5-5250u)
fi-skl-i7-6700k total:226 pass:188 dwarn:0 dfail:0 fail:1 skip:37
ro-bdw-i5-5250u total:226 pass:197 dwarn:1 dfail:0 fail:1 skip:27
ro-bdw-i7-5557U total:226 pass:198 dwarn:0 dfail:0 fail:1 skip:27
ro-bdw-i7-5600u total:226 pass:185 dwarn:0 dfail:0 fail:1 skip:40
ro-bsw-n3050 total:226 pass:172 dwarn:0 dfail:0 fail:3 skip:51
ro-byt-n2820 total:226 pass:174 dwarn:0 dfail:0 fail:3 skip:49
ro-hsw-i3-4010u total:226 pass:190 dwarn:0 dfail:0 fail:1 skip:35
ro-hsw-i7-4770r total:226 pass:190 dwarn:0 dfail:0 fail:1 skip:35
ro-ilk-i7-620lm total:226 pass:150 dwarn:0 dfail:0 fail:2 skip:74
ro-ilk1-i5-650 total:221 pass:150 dwarn:0 dfail:0 fail:2 skip:69
ro-ivb-i7-3770 total:226 pass:181 dwarn:0 dfail:0 fail:1 skip:44
ro-ivb2-i7-3770 total:226 pass:185 dwarn:0 dfail:0 fail:1 skip:40
ro-skl3-i5-6260u total:226 pass:201 dwarn:1 dfail:0 fail:1 skip:23
ro-snb-i7-2620M total:226 pass:174 dwarn:0 dfail:0 fail:2 skip:50
fi-hsw-i7-4770k failed to connect after reboot
fi-kbl-qkkr failed to connect after reboot
fi-skl-i5-6260u failed to connect after reboot
fi-snb-i7-2600 failed to connect after reboot
Results at /archive/results/CI_IGT_test/RO_Patchwork_1271/
0a88ca3 drm-intel-nightly: 2016y-06m-22d-13h-19m-30s UTC integration manifest
2897a9e drm/i915: Small compaction of the engine init code
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] drm/i915: Small compaction of the engine init code
2016-06-22 16:10 ` Chris Wilson
@ 2016-06-22 16:21 ` Tvrtko Ursulin
2016-06-22 16:28 ` Chris Wilson
2016-06-22 16:35 ` [PATCH v2] " Tvrtko Ursulin
0 siblings, 2 replies; 21+ messages in thread
From: Tvrtko Ursulin @ 2016-06-22 16:21 UTC (permalink / raw)
To: Chris Wilson, Intel-gfx, Tvrtko Ursulin
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] drm/i915: Small compaction of the engine init code
2016-06-22 16:21 ` Tvrtko Ursulin
@ 2016-06-22 16:28 ` Chris Wilson
2016-06-22 16:35 ` [PATCH v2] " Tvrtko Ursulin
1 sibling, 0 replies; 21+ messages in thread
From: Chris Wilson @ 2016-06-22 16:28 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
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
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] drm/i915: Small compaction of the engine init code
2016-06-22 16:21 ` Tvrtko Ursulin
2016-06-22 16:28 ` Chris Wilson
@ 2016-06-22 16:35 ` Tvrtko Ursulin
2016-06-22 16:59 ` Chris Wilson
1 sibling, 1 reply; 21+ messages in thread
From: Tvrtko Ursulin @ 2016-06-22 16:35 UTC (permalink / raw)
To: Intel-gfx
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.
v2: Removed ugly BUILD_BUG_ONs in favour of newly introduced
ENGINE_MASK and HAS_ENGINE macros.
Also removed the forward declarations by shuffling functions
around.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_drv.h | 27 +++--
drivers/gpu/drm/i915/intel_lrc.c | 246 ++++++++++++++++-----------------------
2 files changed, 117 insertions(+), 156 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 74d0a61de75a..6d96b14b53c8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2759,17 +2759,22 @@ struct drm_i915_cmd_table {
#define IS_GEN8(dev) (INTEL_INFO(dev)->gen_mask & BIT(7))
#define IS_GEN9(dev) (INTEL_INFO(dev)->gen_mask & BIT(8))
-#define RENDER_RING (1<<RCS)
-#define BSD_RING (1<<VCS)
-#define BLT_RING (1<<BCS)
-#define VEBOX_RING (1<<VECS)
-#define BSD2_RING (1<<VCS2)
-#define ALL_ENGINES (~0)
-
-#define HAS_BSD(dev) (INTEL_INFO(dev)->ring_mask & BSD_RING)
-#define HAS_BSD2(dev) (INTEL_INFO(dev)->ring_mask & BSD2_RING)
-#define HAS_BLT(dev) (INTEL_INFO(dev)->ring_mask & BLT_RING)
-#define HAS_VEBOX(dev) (INTEL_INFO(dev)->ring_mask & VEBOX_RING)
+#define ENGINE_MASK(id) BIT(id)
+#define RENDER_RING ENGINE_MASK(RCS)
+#define BSD_RING ENGINE_MASK(VCS)
+#define BLT_RING ENGINE_MASK(BCS)
+#define VEBOX_RING ENGINE_MASK(VECS)
+#define BSD2_RING ENGINE_MASK(VCS2)
+#define ALL_ENGINES (~0)
+
+#define HAS_ENGINE(dev_priv, id) \
+ (INTEL_INFO(dev_priv)->ring_mask & ENGINE_MASK(id))
+
+#define HAS_BSD(dev_priv) HAS_ENGINE(dev_priv, VCS)
+#define HAS_BSD2(dev_priv) HAS_ENGINE(dev_priv, VCS2)
+#define HAS_BLT(dev_priv) HAS_ENGINE(dev_priv, BCS)
+#define HAS_VEBOX(dev_priv) HAS_ENGINE(dev_priv, VECS)
+
#define HAS_LLC(dev) (INTEL_INFO(dev)->has_llc)
#define HAS_SNOOP(dev) (INTEL_INFO(dev)->has_snoop)
#define HAS_EDRAM(dev) (__I915__(dev)->edram_cap & EDRAM_ENABLED)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index debed011a958..c48b21f79bd2 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2016,12 +2016,90 @@ lrc_setup_hws(struct intel_engine_cs *engine,
return 0;
}
+static int
+logical_ring_init(struct intel_engine_cs *engine)
+{
+ struct i915_gem_context *dctx = engine->i915->kernel_context;
+ int ret;
+
+ ret = i915_cmd_parser_init_ring(engine);
+ if (ret)
+ goto error;
+
+ ret = execlists_context_deferred_alloc(dctx, engine);
+ if (ret)
+ goto error;
+
+ /* As this is the default context, always pin it */
+ ret = intel_lr_context_pin(dctx, engine);
+ if (ret) {
+ DRM_ERROR("Failed to pin context for %s: %d\n",
+ engine->name, ret);
+ goto error;
+ }
+
+ /* And setup the hardware status page. */
+ ret = lrc_setup_hws(engine, dctx->engine[engine->id].state);
+ if (ret) {
+ DRM_ERROR("Failed to set up hws %s: %d\n", engine->name, ret);
+ goto error;
+ }
+
+ return 0;
+
+error:
+ intel_logical_ring_cleanup(engine);
+ return ret;
+}
+
+static int logical_render_ring_init(struct intel_engine_cs *engine)
+{
+ struct drm_i915_private *dev_priv = engine->i915;
+ int ret;
+
+ if (HAS_L3_DPF(dev_priv))
+ engine->irq_keep_mask |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
+
+ /* Override some for render ring. */
+ if (INTEL_GEN(dev_priv) >= 9)
+ engine->init_hw = gen9_init_render_ring;
+ else
+ engine->init_hw = gen8_init_render_ring;
+ engine->init_context = gen8_init_rcs_context;
+ engine->cleanup = intel_fini_pipe_control;
+ engine->emit_flush = gen8_emit_flush_render;
+ engine->emit_request = gen8_emit_request_render;
+
+ ret = intel_init_pipe_control(engine);
+ if (ret)
+ return ret;
+
+ ret = intel_init_workaround_bb(engine);
+ if (ret) {
+ /*
+ * We continue even if we fail to initialize WA batch
+ * because we only expect rare glitches but nothing
+ * critical to prevent us from using GPU
+ */
+ DRM_ERROR("WA batch buffer initialization failed: %d\n",
+ ret);
+ }
+
+ ret = logical_ring_init(engine);
+ if (ret) {
+ lrc_destroy_wa_ctx_obj(engine);
+ }
+
+ return ret;
+}
+
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 +2107,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 +2115,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 +2123,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 +2131,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 +2139,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,169 +2189,43 @@ 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;
}
-static int
-logical_ring_init(struct intel_engine_cs *engine)
-{
- struct i915_gem_context *dctx = engine->i915->kernel_context;
- int ret;
-
- ret = i915_cmd_parser_init_ring(engine);
- if (ret)
- goto error;
-
- ret = execlists_context_deferred_alloc(dctx, engine);
- if (ret)
- goto error;
-
- /* As this is the default context, always pin it */
- ret = intel_lr_context_pin(dctx, engine);
- if (ret) {
- DRM_ERROR("Failed to pin context for %s: %d\n",
- engine->name, ret);
- goto error;
- }
-
- /* And setup the hardware status page. */
- ret = lrc_setup_hws(engine, dctx->engine[engine->id].state);
- if (ret) {
- DRM_ERROR("Failed to set up hws %s: %d\n", engine->name, ret);
- goto error;
- }
-
- return 0;
-
-error:
- intel_logical_ring_cleanup(engine);
- return ret;
-}
-
-static int logical_render_ring_init(struct drm_device *dev)
-{
- struct intel_engine_cs *engine = logical_ring_setup(dev, RCS);
- int ret;
-
- if (HAS_L3_DPF(dev))
- engine->irq_keep_mask |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
-
- /* Override some for render ring. */
- if (INTEL_INFO(dev)->gen >= 9)
- engine->init_hw = gen9_init_render_ring;
- else
- engine->init_hw = gen8_init_render_ring;
- engine->init_context = gen8_init_rcs_context;
- engine->cleanup = intel_fini_pipe_control;
- engine->emit_flush = gen8_emit_flush_render;
- engine->emit_request = gen8_emit_request_render;
-
- ret = intel_init_pipe_control(engine);
- if (ret)
- return ret;
-
- ret = intel_init_workaround_bb(engine);
- if (ret) {
- /*
- * We continue even if we fail to initialize WA batch
- * because we only expect rare glitches but nothing
- * critical to prevent us from using GPU
- */
- DRM_ERROR("WA batch buffer initialization failed: %d\n",
- ret);
- }
-
- ret = logical_ring_init(engine);
- if (ret) {
- lrc_destroy_wa_ctx_obj(engine);
- }
-
- 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;
-
- 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 (HAS_ENGINE(dev_priv, 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;
}
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2] drm/i915: Small compaction of the engine init code
2016-06-22 16:35 ` [PATCH v2] " Tvrtko Ursulin
@ 2016-06-22 16:59 ` Chris Wilson
2016-06-23 10:26 ` Tvrtko Ursulin
0 siblings, 1 reply; 21+ messages in thread
From: Chris Wilson @ 2016-06-22 16:59 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
On Wed, Jun 22, 2016 at 05:35:48PM +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.
>
> v2: Removed ugly BUILD_BUG_ONs in favour of newly introduced
> ENGINE_MASK and HAS_ENGINE macros.
> Also removed the forward declarations by shuffling functions
> around.
>
> 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>
> 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;
> -
> - 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++) {
One more thing to consider is that logical_rings[] has an unspecified
size. Either we set NUM_ENGINES there or use ARRAY_SIZE here, either way
we risk not initialising all engines we expect.
I think we need:
unsigned mask = 0;
> + if (HAS_ENGINE(dev_priv, i)) {
> + engine = logical_ring_setup(dev_priv, i);
> + ret = logical_rings[i].init(engine);
> + if (ret)
> + goto cleanup;
mask |= intel_engine_flag(engine);
> + }
> }
if (WARN_ON(mask != dev_priv->info.rings_mask))
mkwrite_intel_info(dev_priv)->rings_mask = mask;
To catch any future forgotten additions without exploding.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* ✗ Ro.CI.BAT: warning for drm/i915: Small compaction of the engine init code (rev2)
2016-06-22 15:55 [PATCH] drm/i915: Small compaction of the engine init code Tvrtko Ursulin
2016-06-22 16:10 ` Chris Wilson
2016-06-22 16:18 ` ✓ Ro.CI.BAT: success for " Patchwork
@ 2016-06-22 17:09 ` Patchwork
2016-06-24 10:10 ` ✗ Ro.CI.BAT: failure for drm/i915: Small compaction of the engine init code (rev3) Patchwork
2016-06-24 10:11 ` ✗ Ro.CI.BAT: failure for drm/i915: Small compaction of the engine init code (rev4) Patchwork
4 siblings, 0 replies; 21+ messages in thread
From: Patchwork @ 2016-06-22 17:09 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Small compaction of the engine init code (rev2)
URL : https://patchwork.freedesktop.org/series/9053/
State : warning
== Summary ==
Series 9053v2 drm/i915: Small compaction of the engine init code
http://patchwork.freedesktop.org/api/1.0/series/9053/revisions/2/mbox
Test drv_module_reload_basic:
pass -> DMESG-WARN (ro-byt-n2820)
skip -> PASS (ro-hsw-i3-4010u)
pass -> SKIP (fi-skl-i5-6260u)
Test gem_exec_flush:
Subgroup basic-batch-kernel-default-cmd:
fail -> PASS (ro-byt-n2820)
Test kms_pipe_crc_basic:
Subgroup nonblocking-crc-pipe-b:
skip -> PASS (fi-skl-i5-6260u)
Subgroup nonblocking-crc-pipe-c:
pass -> SKIP (fi-skl-i5-6260u)
Subgroup suspend-read-crc-pipe-b:
skip -> PASS (ro-bdw-i7-5557U)
Subgroup suspend-read-crc-pipe-c:
skip -> DMESG-WARN (ro-bdw-i5-5250u)
fi-hsw-i7-4770k total:226 pass:194 dwarn:0 dfail:0 fail:1 skip:31
fi-kbl-qkkr total:226 pass:161 dwarn:29 dfail:0 fail:1 skip:35
fi-skl-i5-6260u total:226 pass:200 dwarn:0 dfail:0 fail:1 skip:25
fi-skl-i7-6700k total:226 pass:188 dwarn:0 dfail:0 fail:1 skip:37
fi-snb-i7-2600 total:226 pass:174 dwarn:0 dfail:0 fail:1 skip:51
ro-bdw-i5-5250u total:226 pass:197 dwarn:2 dfail:0 fail:0 skip:27
ro-bdw-i7-5557U total:226 pass:199 dwarn:1 dfail:0 fail:0 skip:26
ro-bdw-i7-5600u total:226 pass:185 dwarn:0 dfail:0 fail:0 skip:41
ro-bsw-n3050 total:226 pass:171 dwarn:1 dfail:0 fail:2 skip:52
ro-byt-n2820 total:226 pass:173 dwarn:1 dfail:0 fail:2 skip:50
ro-hsw-i3-4010u total:226 pass:190 dwarn:0 dfail:0 fail:0 skip:36
ro-hsw-i7-4770r total:226 pass:190 dwarn:0 dfail:0 fail:0 skip:36
ro-ilk-i7-620lm total:226 pass:150 dwarn:0 dfail:0 fail:1 skip:75
ro-ilk1-i5-650 total:221 pass:150 dwarn:0 dfail:0 fail:1 skip:70
ro-ivb-i7-3770 total:226 pass:181 dwarn:0 dfail:0 fail:0 skip:45
ro-ivb2-i7-3770 total:226 pass:185 dwarn:0 dfail:0 fail:0 skip:41
ro-skl3-i5-6260u total:226 pass:201 dwarn:1 dfail:0 fail:0 skip:24
ro-snb-i7-2620M total:226 pass:174 dwarn:0 dfail:0 fail:1 skip:51
Results at /archive/results/CI_IGT_test/RO_Patchwork_1272/
fb6ffad drm-intel-nightly: 2016y-06m-22d-16h-19m-09s UTC integration manifest
6ddf8f2 drm/i915: Small compaction of the engine init code
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] drm/i915: Small compaction of the engine init code
2016-06-22 16:59 ` Chris Wilson
@ 2016-06-23 10:26 ` Tvrtko Ursulin
2016-06-23 10:47 ` Chris Wilson
0 siblings, 1 reply; 21+ messages in thread
From: Tvrtko Ursulin @ 2016-06-23 10:26 UTC (permalink / raw)
To: Chris Wilson, Intel-gfx, Tvrtko Ursulin
On 22/06/16 17:59, Chris Wilson wrote:
> On Wed, Jun 22, 2016 at 05:35:48PM +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.
>>
>> v2: Removed ugly BUILD_BUG_ONs in favour of newly introduced
>> ENGINE_MASK and HAS_ENGINE macros.
>> Also removed the forward declarations by shuffling functions
>> around.
>>
>> 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>
>
>> 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;
>> -
>> - 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++) {
>
> One more thing to consider is that logical_rings[] has an unspecified
> size. Either we set NUM_ENGINES there or use ARRAY_SIZE here, either way
> we risk not initialising all engines we expect.
>
> I think we need:
> unsigned mask = 0;
>
>> + if (HAS_ENGINE(dev_priv, i)) {
>> + engine = logical_ring_setup(dev_priv, i);
>> + ret = logical_rings[i].init(engine);
>> + if (ret)
>> + goto cleanup;
> mask |= intel_engine_flag(engine);
>> + }
>> }
>
> if (WARN_ON(mask != dev_priv->info.rings_mask))
> mkwrite_intel_info(dev_priv)->rings_mask = mask;
>
> To catch any future forgotten additions without exploding.
Hm, if logical_rings does not contain all required engines it can either
crash or, if somehow it magically manages to initialize it from random
data, still pass your test.
Perhaps we just need:
BUILD_BUG_ON(ARRAY_SIZE(logical_rings) == I915_NUM_ENGINES)
?
What is this mkwrite_intel_info thing? There is a facility nowadays to
write to the rodata section? :)
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] drm/i915: Small compaction of the engine init code
2016-06-23 10:26 ` Tvrtko Ursulin
@ 2016-06-23 10:47 ` Chris Wilson
2016-06-23 11:12 ` [PATCH v3] " Tvrtko Ursulin
0 siblings, 1 reply; 21+ messages in thread
From: Chris Wilson @ 2016-06-23 10:47 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
On Thu, Jun 23, 2016 at 11:26:27AM +0100, Tvrtko Ursulin wrote:
>
> On 22/06/16 17:59, Chris Wilson wrote:
> >On Wed, Jun 22, 2016 at 05:35:48PM +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.
> >>
> >>v2: Removed ugly BUILD_BUG_ONs in favour of newly introduced
> >> ENGINE_MASK and HAS_ENGINE macros.
> >> Also removed the forward declarations by shuffling functions
> >> around.
> >>
> >>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>
> >
> >> 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;
> >>-
> >>- 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++) {
> >
> >One more thing to consider is that logical_rings[] has an unspecified
> >size. Either we set NUM_ENGINES there or use ARRAY_SIZE here, either way
> >we risk not initialising all engines we expect.
> >
> >I think we need:
> >unsigned mask = 0;
> >
> >>+ if (HAS_ENGINE(dev_priv, i)) {
> >>+ engine = logical_ring_setup(dev_priv, i);
> >>+ ret = logical_rings[i].init(engine);
> >>+ if (ret)
> >>+ goto cleanup;
> > mask |= intel_engine_flag(engine);
> >>+ }
> >> }
> >
> >if (WARN_ON(mask != dev_priv->info.rings_mask))
> > mkwrite_intel_info(dev_priv)->rings_mask = mask;
> >
> >To catch any future forgotten additions without exploding.
>
> Hm, if logical_rings does not contain all required engines it can
> either crash or, if somehow it magically manages to initialize it
> from random data, still pass your test.
I was expecting you to use if (!init()) continue or something to stop
the crash and so leave holes in the mask :)
> Perhaps we just need:
>
> BUILD_BUG_ON(ARRAY_SIZE(logical_rings) == I915_NUM_ENGINES)
>
> ?
Yeah, I can't see what more information we can provide other than
pointing out the piece of forgotten code. But what if we add new engines
to a future submission mechanism that we don't need in execlists?
When the build bug fires, would be the time to consider how to fix it.
> What is this mkwrite_intel_info thing? There is a facility nowadays
> to write to the rodata section? :)
It's not rodata, intel_info is copied into dev_priv, modified, then
treated as const.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3] drm/i915: Small compaction of the engine init code
2016-06-23 10:47 ` Chris Wilson
@ 2016-06-23 11:12 ` Tvrtko Ursulin
2016-06-23 11:25 ` Chris Wilson
0 siblings, 1 reply; 21+ messages in thread
From: Tvrtko Ursulin @ 2016-06-23 11:12 UTC (permalink / raw)
To: Intel-gfx
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.
v2: Removed ugly BUILD_BUG_ONs in favour of newly introduced
ENGINE_MASK and HAS_ENGINE macros.
Also removed the forward declarations by shuffling functions
around.
v3: Warn when logical_rings table does not contain enough data
and disable the engines which could not be initialized.
(Chris Wilson)
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_drv.h | 27 +++--
drivers/gpu/drm/i915/intel_lrc.c | 256 +++++++++++++++++----------------------
2 files changed, 129 insertions(+), 154 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 74d0a61de75a..6d96b14b53c8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2759,17 +2759,22 @@ struct drm_i915_cmd_table {
#define IS_GEN8(dev) (INTEL_INFO(dev)->gen_mask & BIT(7))
#define IS_GEN9(dev) (INTEL_INFO(dev)->gen_mask & BIT(8))
-#define RENDER_RING (1<<RCS)
-#define BSD_RING (1<<VCS)
-#define BLT_RING (1<<BCS)
-#define VEBOX_RING (1<<VECS)
-#define BSD2_RING (1<<VCS2)
-#define ALL_ENGINES (~0)
-
-#define HAS_BSD(dev) (INTEL_INFO(dev)->ring_mask & BSD_RING)
-#define HAS_BSD2(dev) (INTEL_INFO(dev)->ring_mask & BSD2_RING)
-#define HAS_BLT(dev) (INTEL_INFO(dev)->ring_mask & BLT_RING)
-#define HAS_VEBOX(dev) (INTEL_INFO(dev)->ring_mask & VEBOX_RING)
+#define ENGINE_MASK(id) BIT(id)
+#define RENDER_RING ENGINE_MASK(RCS)
+#define BSD_RING ENGINE_MASK(VCS)
+#define BLT_RING ENGINE_MASK(BCS)
+#define VEBOX_RING ENGINE_MASK(VECS)
+#define BSD2_RING ENGINE_MASK(VCS2)
+#define ALL_ENGINES (~0)
+
+#define HAS_ENGINE(dev_priv, id) \
+ (INTEL_INFO(dev_priv)->ring_mask & ENGINE_MASK(id))
+
+#define HAS_BSD(dev_priv) HAS_ENGINE(dev_priv, VCS)
+#define HAS_BSD2(dev_priv) HAS_ENGINE(dev_priv, VCS2)
+#define HAS_BLT(dev_priv) HAS_ENGINE(dev_priv, BCS)
+#define HAS_VEBOX(dev_priv) HAS_ENGINE(dev_priv, VECS)
+
#define HAS_LLC(dev) (INTEL_INFO(dev)->has_llc)
#define HAS_SNOOP(dev) (INTEL_INFO(dev)->has_snoop)
#define HAS_EDRAM(dev) (__I915__(dev)->edram_cap & EDRAM_ENABLED)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index debed011a958..c80aa22d3367 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2016,12 +2016,90 @@ lrc_setup_hws(struct intel_engine_cs *engine,
return 0;
}
+static int
+logical_ring_init(struct intel_engine_cs *engine)
+{
+ struct i915_gem_context *dctx = engine->i915->kernel_context;
+ int ret;
+
+ ret = i915_cmd_parser_init_ring(engine);
+ if (ret)
+ goto error;
+
+ ret = execlists_context_deferred_alloc(dctx, engine);
+ if (ret)
+ goto error;
+
+ /* As this is the default context, always pin it */
+ ret = intel_lr_context_pin(dctx, engine);
+ if (ret) {
+ DRM_ERROR("Failed to pin context for %s: %d\n",
+ engine->name, ret);
+ goto error;
+ }
+
+ /* And setup the hardware status page. */
+ ret = lrc_setup_hws(engine, dctx->engine[engine->id].state);
+ if (ret) {
+ DRM_ERROR("Failed to set up hws %s: %d\n", engine->name, ret);
+ goto error;
+ }
+
+ return 0;
+
+error:
+ intel_logical_ring_cleanup(engine);
+ return ret;
+}
+
+static int logical_render_ring_init(struct intel_engine_cs *engine)
+{
+ struct drm_i915_private *dev_priv = engine->i915;
+ int ret;
+
+ if (HAS_L3_DPF(dev_priv))
+ engine->irq_keep_mask |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
+
+ /* Override some for render ring. */
+ if (INTEL_GEN(dev_priv) >= 9)
+ engine->init_hw = gen9_init_render_ring;
+ else
+ engine->init_hw = gen8_init_render_ring;
+ engine->init_context = gen8_init_rcs_context;
+ engine->cleanup = intel_fini_pipe_control;
+ engine->emit_flush = gen8_emit_flush_render;
+ engine->emit_request = gen8_emit_request_render;
+
+ ret = intel_init_pipe_control(engine);
+ if (ret)
+ return ret;
+
+ ret = intel_init_workaround_bb(engine);
+ if (ret) {
+ /*
+ * We continue even if we fail to initialize WA batch
+ * because we only expect rare glitches but nothing
+ * critical to prevent us from using GPU
+ */
+ DRM_ERROR("WA batch buffer initialization failed: %d\n",
+ ret);
+ }
+
+ ret = logical_ring_init(engine);
+ if (ret) {
+ lrc_destroy_wa_ctx_obj(engine);
+ }
+
+ return ret;
+}
+
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 +2107,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 +2115,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 +2123,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 +2131,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 +2139,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,169 +2189,57 @@ 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;
}
-static int
-logical_ring_init(struct intel_engine_cs *engine)
-{
- struct i915_gem_context *dctx = engine->i915->kernel_context;
- int ret;
-
- ret = i915_cmd_parser_init_ring(engine);
- if (ret)
- goto error;
-
- ret = execlists_context_deferred_alloc(dctx, engine);
- if (ret)
- goto error;
-
- /* As this is the default context, always pin it */
- ret = intel_lr_context_pin(dctx, engine);
- if (ret) {
- DRM_ERROR("Failed to pin context for %s: %d\n",
- engine->name, ret);
- goto error;
- }
-
- /* And setup the hardware status page. */
- ret = lrc_setup_hws(engine, dctx->engine[engine->id].state);
- if (ret) {
- DRM_ERROR("Failed to set up hws %s: %d\n", engine->name, ret);
- goto error;
- }
-
- return 0;
-
-error:
- intel_logical_ring_cleanup(engine);
- return ret;
-}
-
-static int logical_render_ring_init(struct drm_device *dev)
-{
- struct intel_engine_cs *engine = logical_ring_setup(dev, RCS);
- int ret;
-
- if (HAS_L3_DPF(dev))
- engine->irq_keep_mask |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
-
- /* Override some for render ring. */
- if (INTEL_INFO(dev)->gen >= 9)
- engine->init_hw = gen9_init_render_ring;
- else
- engine->init_hw = gen8_init_render_ring;
- engine->init_context = gen8_init_rcs_context;
- engine->cleanup = intel_fini_pipe_control;
- engine->emit_flush = gen8_emit_flush_render;
- engine->emit_request = gen8_emit_request_render;
-
- ret = intel_init_pipe_control(engine);
- if (ret)
- return ret;
-
- ret = intel_init_workaround_bb(engine);
- if (ret) {
- /*
- * We continue even if we fail to initialize WA batch
- * because we only expect rare glitches but nothing
- * critical to prevent us from using GPU
- */
- DRM_ERROR("WA batch buffer initialization failed: %d\n",
- ret);
- }
-
- ret = logical_ring_init(engine);
- if (ret) {
- lrc_destroy_wa_ctx_obj(engine);
- }
-
- 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 mask = 0;
+ unsigned int i;
int ret;
- ret = logical_render_ring_init(dev);
- if (ret)
- return ret;
-
- 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;
+ for (i = 0;
+ i < I915_NUM_ENGINES && i < ARRAY_SIZE(logical_rings); i++) {
+ if (HAS_ENGINE(dev_priv, i)) {
+ engine = logical_ring_setup(dev_priv, i);
+ ret = logical_rings[i].init(engine);
+ if (ret)
+ goto cleanup;
+ mask |= ENGINE_MASK(i);
+ }
}
- if (HAS_BSD2(dev)) {
- ret = logical_bsd2_ring_init(dev);
- if (ret)
- goto cleanup_vebox_ring;
+ /*
+ * Catch failures to update logical_rings table when the new engines
+ * are added to the driver by a warning and disabling the forgotten
+ * engines.
+ */
+ if (WARN_ON(mask != INTEL_INFO(dev_priv)->ring_mask)) {
+ struct intel_device_info *info =
+ (struct intel_device_info *)&dev_priv->info;
+ info->ring_mask = mask;
}
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;
}
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v3] drm/i915: Small compaction of the engine init code
2016-06-23 11:12 ` [PATCH v3] " Tvrtko Ursulin
@ 2016-06-23 11:25 ` Chris Wilson
2016-06-23 11:46 ` Tvrtko Ursulin
0 siblings, 1 reply; 21+ messages in thread
From: Chris Wilson @ 2016-06-23 11:25 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
On Thu, Jun 23, 2016 at 12:12:29PM +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.
>
> v2: Removed ugly BUILD_BUG_ONs in favour of newly introduced
> ENGINE_MASK and HAS_ENGINE macros.
> Also removed the forward declarations by shuffling functions
> around.
>
> v3: Warn when logical_rings table does not contain enough data
> and disable the engines which could not be initialized.
> (Chris Wilson)
I was happy with the BUILD_BUG suggestion :)
> + for (i = 0;
> + i < I915_NUM_ENGINES && i < ARRAY_SIZE(logical_rings); i++) {
HAS_ENGINE() == false if i >= I915_NUM_ENGINES
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3] drm/i915: Small compaction of the engine init code
2016-06-23 11:25 ` Chris Wilson
@ 2016-06-23 11:46 ` Tvrtko Ursulin
2016-06-23 12:11 ` Chris Wilson
0 siblings, 1 reply; 21+ messages in thread
From: Tvrtko Ursulin @ 2016-06-23 11:46 UTC (permalink / raw)
To: Chris Wilson, Intel-gfx, Tvrtko Ursulin
On 23/06/16 12:25, Chris Wilson wrote:
> On Thu, Jun 23, 2016 at 12:12:29PM +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.
>>
>> v2: Removed ugly BUILD_BUG_ONs in favour of newly introduced
>> ENGINE_MASK and HAS_ENGINE macros.
>> Also removed the forward declarations by shuffling functions
>> around.
>>
>> v3: Warn when logical_rings table does not contain enough data
>> and disable the engines which could not be initialized.
>> (Chris Wilson)
>
> I was happy with the BUILD_BUG suggestion :)
I've changed my mind later. :)
>> + for (i = 0;
>> + i < I915_NUM_ENGINES && i < ARRAY_SIZE(logical_rings); i++) {
>
> HAS_ENGINE() == false if i >= I915_NUM_ENGINES
Don't follow. :) Why is v3 not good enough?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3] drm/i915: Small compaction of the engine init code
2016-06-23 11:46 ` Tvrtko Ursulin
@ 2016-06-23 12:11 ` Chris Wilson
2016-06-23 13:16 ` Tvrtko Ursulin
0 siblings, 1 reply; 21+ messages in thread
From: Chris Wilson @ 2016-06-23 12:11 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
On Thu, Jun 23, 2016 at 12:46:42PM +0100, Tvrtko Ursulin wrote:
>
> On 23/06/16 12:25, Chris Wilson wrote:
> >On Thu, Jun 23, 2016 at 12:12:29PM +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.
> >>
> >>v2: Removed ugly BUILD_BUG_ONs in favour of newly introduced
> >> ENGINE_MASK and HAS_ENGINE macros.
> >> Also removed the forward declarations by shuffling functions
> >> around.
> >>
> >>v3: Warn when logical_rings table does not contain enough data
> >> and disable the engines which could not be initialized.
> >> (Chris Wilson)
> >
> >I was happy with the BUILD_BUG suggestion :)
>
> I've changed my mind later. :)
>
> >>+ for (i = 0;
> >>+ i < I915_NUM_ENGINES && i < ARRAY_SIZE(logical_rings); i++) {
> >
> >HAS_ENGINE() == false if i >= I915_NUM_ENGINES
>
> Don't follow. :) Why is v3 not good enough?
Both (all three) is overkill.
I feel like HAS_ENGINE() should encompass i < I915_NUM_ENGINES quite
succinctly. For belt and braces,
WARN_ON(dev_priv->intel_info.rings_mask & -(1 << I915_NUM_ENGINES)));
for (i = 0; i < ARRAY_SIZE(); i++) {
if (!HAS_ENGINE(i))
continue;
if (!logical_rings[i].init)
continue;
ret = logical_rings[i].init(logical_rings_engine(i));
if (ret)
goto err;
mask |= ENGINE_MASK(i);
}
WARN_ON(mask != dev_priv->intel_info.rings_mask) ...
?
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3] drm/i915: Small compaction of the engine init code
2016-06-23 12:11 ` Chris Wilson
@ 2016-06-23 13:16 ` Tvrtko Ursulin
2016-06-23 13:25 ` Chris Wilson
0 siblings, 1 reply; 21+ messages in thread
From: Tvrtko Ursulin @ 2016-06-23 13:16 UTC (permalink / raw)
To: Chris Wilson, Intel-gfx, Tvrtko Ursulin
On 23/06/16 13:11, Chris Wilson wrote:
> On Thu, Jun 23, 2016 at 12:46:42PM +0100, Tvrtko Ursulin wrote:
>>
>> On 23/06/16 12:25, Chris Wilson wrote:
>>> On Thu, Jun 23, 2016 at 12:12:29PM +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.
>>>>
>>>> v2: Removed ugly BUILD_BUG_ONs in favour of newly introduced
>>>> ENGINE_MASK and HAS_ENGINE macros.
>>>> Also removed the forward declarations by shuffling functions
>>>> around.
>>>>
>>>> v3: Warn when logical_rings table does not contain enough data
>>>> and disable the engines which could not be initialized.
>>>> (Chris Wilson)
>>>
>>> I was happy with the BUILD_BUG suggestion :)
>>
>> I've changed my mind later. :)
>>
>>>> + for (i = 0;
>>>> + i < I915_NUM_ENGINES && i < ARRAY_SIZE(logical_rings); i++) {
>>>
>>> HAS_ENGINE() == false if i >= I915_NUM_ENGINES
>>
>> Don't follow. :) Why is v3 not good enough?
>
> Both (all three) is overkill.
>
> I feel like HAS_ENGINE() should encompass i < I915_NUM_ENGINES quite
> succinctly. For belt and braces,
>
> WARN_ON(dev_priv->intel_info.rings_mask & -(1 << I915_NUM_ENGINES)));
I don't think this works - you meant testing that bits higher than
BIT(I915_NUM_ENGINES) were not set in ring_mask?
And it probably belongs somewhere else, in common code which initializes
intel_device_info I think.
> for (i = 0; i < ARRAY_SIZE(); i++) {
> if (!HAS_ENGINE(i))
> continue;
>
> if (!logical_rings[i].init)
> continue;
>
> ret = logical_rings[i].init(logical_rings_engine(i));
> if (ret)
> goto err;
>
> mask |= ENGINE_MASK(i);
> }
>
> WARN_ON(mask != dev_priv->intel_info.rings_mask) ...
>
> ?
I like it, will resend when we clarify the above. Also solves one more
issue than the previous versions which is a potentially sparse
logical_rings array if engine ids get renumbered.
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3] drm/i915: Small compaction of the engine init code
2016-06-23 13:16 ` Tvrtko Ursulin
@ 2016-06-23 13:25 ` Chris Wilson
2016-06-23 13:52 ` [PATCH v4] " Tvrtko Ursulin
0 siblings, 1 reply; 21+ messages in thread
From: Chris Wilson @ 2016-06-23 13:25 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
On Thu, Jun 23, 2016 at 02:16:55PM +0100, Tvrtko Ursulin wrote:
>
> On 23/06/16 13:11, Chris Wilson wrote:
> >On Thu, Jun 23, 2016 at 12:46:42PM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 23/06/16 12:25, Chris Wilson wrote:
> >>>On Thu, Jun 23, 2016 at 12:12:29PM +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.
> >>>>
> >>>>v2: Removed ugly BUILD_BUG_ONs in favour of newly introduced
> >>>> ENGINE_MASK and HAS_ENGINE macros.
> >>>> Also removed the forward declarations by shuffling functions
> >>>> around.
> >>>>
> >>>>v3: Warn when logical_rings table does not contain enough data
> >>>> and disable the engines which could not be initialized.
> >>>> (Chris Wilson)
> >>>
> >>>I was happy with the BUILD_BUG suggestion :)
> >>
> >>I've changed my mind later. :)
> >>
> >>>>+ for (i = 0;
> >>>>+ i < I915_NUM_ENGINES && i < ARRAY_SIZE(logical_rings); i++) {
> >>>
> >>>HAS_ENGINE() == false if i >= I915_NUM_ENGINES
> >>
> >>Don't follow. :) Why is v3 not good enough?
> >
> >Both (all three) is overkill.
> >
> >I feel like HAS_ENGINE() should encompass i < I915_NUM_ENGINES quite
> >succinctly. For belt and braces,
> >
> >WARN_ON(dev_priv->intel_info.rings_mask & -(1 << I915_NUM_ENGINES)));
>
> I don't think this works - you meant testing that bits higher than
> BIT(I915_NUM_ENGINES) were not set in ring_mask?
Yes. Higher than BIT(NUM_ENGINES-1), just to make sure we don't end up
setting garbage at some point.
> And it probably belongs somewhere else, in common code which
> initializes intel_device_info I think.
Possibly. I think the engine initialisation path is a good choice
though, since that has the knowlege of what will be setup.
Either here (with the goal of using this as the basis for future
unification), or in the caller.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v4] drm/i915: Small compaction of the engine init code
2016-06-23 13:25 ` Chris Wilson
@ 2016-06-23 13:52 ` Tvrtko Ursulin
2016-06-23 14:03 ` Chris Wilson
0 siblings, 1 reply; 21+ messages in thread
From: Tvrtko Ursulin @ 2016-06-23 13:52 UTC (permalink / raw)
To: Intel-gfx
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.
v2: Removed ugly BUILD_BUG_ONs in favour of newly introduced
ENGINE_MASK and HAS_ENGINE macros.
Also removed the forward declarations by shuffling functions
around.
v3: Warn when logical_rings table does not contain enough data
and disable the engines which could not be initialized.
(Chris Wilson)
v4: Chris Wilson suggested a nicer engine init loop.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_drv.h | 27 +++--
drivers/gpu/drm/i915/intel_lrc.c | 253 ++++++++++++++++++---------------------
2 files changed, 130 insertions(+), 150 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 74d0a61de75a..6d96b14b53c8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2759,17 +2759,22 @@ struct drm_i915_cmd_table {
#define IS_GEN8(dev) (INTEL_INFO(dev)->gen_mask & BIT(7))
#define IS_GEN9(dev) (INTEL_INFO(dev)->gen_mask & BIT(8))
-#define RENDER_RING (1<<RCS)
-#define BSD_RING (1<<VCS)
-#define BLT_RING (1<<BCS)
-#define VEBOX_RING (1<<VECS)
-#define BSD2_RING (1<<VCS2)
-#define ALL_ENGINES (~0)
-
-#define HAS_BSD(dev) (INTEL_INFO(dev)->ring_mask & BSD_RING)
-#define HAS_BSD2(dev) (INTEL_INFO(dev)->ring_mask & BSD2_RING)
-#define HAS_BLT(dev) (INTEL_INFO(dev)->ring_mask & BLT_RING)
-#define HAS_VEBOX(dev) (INTEL_INFO(dev)->ring_mask & VEBOX_RING)
+#define ENGINE_MASK(id) BIT(id)
+#define RENDER_RING ENGINE_MASK(RCS)
+#define BSD_RING ENGINE_MASK(VCS)
+#define BLT_RING ENGINE_MASK(BCS)
+#define VEBOX_RING ENGINE_MASK(VECS)
+#define BSD2_RING ENGINE_MASK(VCS2)
+#define ALL_ENGINES (~0)
+
+#define HAS_ENGINE(dev_priv, id) \
+ (INTEL_INFO(dev_priv)->ring_mask & ENGINE_MASK(id))
+
+#define HAS_BSD(dev_priv) HAS_ENGINE(dev_priv, VCS)
+#define HAS_BSD2(dev_priv) HAS_ENGINE(dev_priv, VCS2)
+#define HAS_BLT(dev_priv) HAS_ENGINE(dev_priv, BCS)
+#define HAS_VEBOX(dev_priv) HAS_ENGINE(dev_priv, VECS)
+
#define HAS_LLC(dev) (INTEL_INFO(dev)->has_llc)
#define HAS_SNOOP(dev) (INTEL_INFO(dev)->has_snoop)
#define HAS_EDRAM(dev) (__I915__(dev)->edram_cap & EDRAM_ENABLED)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index debed011a958..62b0dc6c2642 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2016,12 +2016,90 @@ lrc_setup_hws(struct intel_engine_cs *engine,
return 0;
}
+static int
+logical_ring_init(struct intel_engine_cs *engine)
+{
+ struct i915_gem_context *dctx = engine->i915->kernel_context;
+ int ret;
+
+ ret = i915_cmd_parser_init_ring(engine);
+ if (ret)
+ goto error;
+
+ ret = execlists_context_deferred_alloc(dctx, engine);
+ if (ret)
+ goto error;
+
+ /* As this is the default context, always pin it */
+ ret = intel_lr_context_pin(dctx, engine);
+ if (ret) {
+ DRM_ERROR("Failed to pin context for %s: %d\n",
+ engine->name, ret);
+ goto error;
+ }
+
+ /* And setup the hardware status page. */
+ ret = lrc_setup_hws(engine, dctx->engine[engine->id].state);
+ if (ret) {
+ DRM_ERROR("Failed to set up hws %s: %d\n", engine->name, ret);
+ goto error;
+ }
+
+ return 0;
+
+error:
+ intel_logical_ring_cleanup(engine);
+ return ret;
+}
+
+static int logical_render_ring_init(struct intel_engine_cs *engine)
+{
+ struct drm_i915_private *dev_priv = engine->i915;
+ int ret;
+
+ if (HAS_L3_DPF(dev_priv))
+ engine->irq_keep_mask |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
+
+ /* Override some for render ring. */
+ if (INTEL_GEN(dev_priv) >= 9)
+ engine->init_hw = gen9_init_render_ring;
+ else
+ engine->init_hw = gen8_init_render_ring;
+ engine->init_context = gen8_init_rcs_context;
+ engine->cleanup = intel_fini_pipe_control;
+ engine->emit_flush = gen8_emit_flush_render;
+ engine->emit_request = gen8_emit_request_render;
+
+ ret = intel_init_pipe_control(engine);
+ if (ret)
+ return ret;
+
+ ret = intel_init_workaround_bb(engine);
+ if (ret) {
+ /*
+ * We continue even if we fail to initialize WA batch
+ * because we only expect rare glitches but nothing
+ * critical to prevent us from using GPU
+ */
+ DRM_ERROR("WA batch buffer initialization failed: %d\n",
+ ret);
+ }
+
+ ret = logical_ring_init(engine);
+ if (ret) {
+ lrc_destroy_wa_ctx_obj(engine);
+ }
+
+ return ret;
+}
+
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 +2107,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 +2115,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 +2123,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 +2131,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 +2139,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,169 +2189,62 @@ 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;
}
-static int
-logical_ring_init(struct intel_engine_cs *engine)
-{
- struct i915_gem_context *dctx = engine->i915->kernel_context;
- int ret;
-
- ret = i915_cmd_parser_init_ring(engine);
- if (ret)
- goto error;
-
- ret = execlists_context_deferred_alloc(dctx, engine);
- if (ret)
- goto error;
-
- /* As this is the default context, always pin it */
- ret = intel_lr_context_pin(dctx, engine);
- if (ret) {
- DRM_ERROR("Failed to pin context for %s: %d\n",
- engine->name, ret);
- goto error;
- }
-
- /* And setup the hardware status page. */
- ret = lrc_setup_hws(engine, dctx->engine[engine->id].state);
- if (ret) {
- DRM_ERROR("Failed to set up hws %s: %d\n", engine->name, ret);
- goto error;
- }
-
- return 0;
-
-error:
- intel_logical_ring_cleanup(engine);
- return ret;
-}
-
-static int logical_render_ring_init(struct drm_device *dev)
-{
- struct intel_engine_cs *engine = logical_ring_setup(dev, RCS);
- int ret;
-
- if (HAS_L3_DPF(dev))
- engine->irq_keep_mask |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
-
- /* Override some for render ring. */
- if (INTEL_INFO(dev)->gen >= 9)
- engine->init_hw = gen9_init_render_ring;
- else
- engine->init_hw = gen8_init_render_ring;
- engine->init_context = gen8_init_rcs_context;
- engine->cleanup = intel_fini_pipe_control;
- engine->emit_flush = gen8_emit_flush_render;
- engine->emit_request = gen8_emit_request_render;
-
- ret = intel_init_pipe_control(engine);
- if (ret)
- return ret;
-
- ret = intel_init_workaround_bb(engine);
- if (ret) {
- /*
- * We continue even if we fail to initialize WA batch
- * because we only expect rare glitches but nothing
- * critical to prevent us from using GPU
- */
- DRM_ERROR("WA batch buffer initialization failed: %d\n",
- ret);
- }
-
- ret = logical_ring_init(engine);
- if (ret) {
- lrc_destroy_wa_ctx_obj(engine);
- }
-
- 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;
+ unsigned int mask = 0;
+ unsigned int i;
int ret;
- ret = logical_render_ring_init(dev);
- if (ret)
- return ret;
+ WARN_ON(INTEL_INFO(dev_priv)->ring_mask &
+ GENMASK(sizeof(mask) * BITS_PER_BYTE - 1, I915_NUM_ENGINES));
- if (HAS_BSD(dev)) {
- ret = logical_bsd_ring_init(dev);
- if (ret)
- goto cleanup_render_ring;
- }
+ for (i = 0; i < ARRAY_SIZE(logical_rings); i++) {
+ if (!HAS_ENGINE(dev_priv, i))
+ continue;
- if (HAS_BLT(dev)) {
- ret = logical_blt_ring_init(dev);
- if (ret)
- goto cleanup_bsd_ring;
- }
+ if (!logical_rings[i].init)
+ continue;
- if (HAS_VEBOX(dev)) {
- ret = logical_vebox_ring_init(dev);
+ ret = logical_rings[i].init(logical_ring_setup(dev_priv, i));
if (ret)
- goto cleanup_blt_ring;
+ goto cleanup;
+
+ mask |= ENGINE_MASK(i);
}
- if (HAS_BSD2(dev)) {
- ret = logical_bsd2_ring_init(dev);
- if (ret)
- goto cleanup_vebox_ring;
+ /*
+ * Catch failures to update logical_rings table when the new engines
+ * are added to the driver by a warning and disabling the forgotten
+ * engines.
+ */
+ if (WARN_ON(mask != INTEL_INFO(dev_priv)->ring_mask)) {
+ struct intel_device_info *info =
+ (struct intel_device_info *)&dev_priv->info;
+ info->ring_mask = mask;
}
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;
}
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v4] drm/i915: Small compaction of the engine init code
2016-06-23 13:52 ` [PATCH v4] " Tvrtko Ursulin
@ 2016-06-23 14:03 ` Chris Wilson
0 siblings, 0 replies; 21+ messages in thread
From: Chris Wilson @ 2016-06-23 14:03 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
On Thu, Jun 23, 2016 at 02:52:41PM +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.
>
> v2: Removed ugly BUILD_BUG_ONs in favour of newly introduced
> ENGINE_MASK and HAS_ENGINE macros.
> Also removed the forward declarations by shuffling functions
> around.
>
> v3: Warn when logical_rings table does not contain enough data
> and disable the engines which could not be initialized.
> (Chris Wilson)
>
> v4: Chris Wilson suggested a nicer engine init loop.
>
> 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>
> + WARN_ON(INTEL_INFO(dev_priv)->ring_mask &
> + GENMASK(sizeof(mask) * BITS_PER_BYTE - 1, I915_NUM_ENGINES));
It's computed as a long, so BITS_PER_LONG would suffice
p/x GENMASK(sizeof(mask) * BITS_PER_BYTE - 1, I915_NUM_ENGINES))
= ((~0UL) << I915_NUM_ENGINES)
= 0xffffffc0
p/x -(1 << I915_NUM_ENGINES)
= 0xffffffc0
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* ✗ Ro.CI.BAT: failure for drm/i915: Small compaction of the engine init code (rev3)
2016-06-22 15:55 [PATCH] drm/i915: Small compaction of the engine init code Tvrtko Ursulin
` (2 preceding siblings ...)
2016-06-22 17:09 ` ✗ Ro.CI.BAT: warning for drm/i915: Small compaction of the engine init code (rev2) Patchwork
@ 2016-06-24 10:10 ` Patchwork
2016-06-24 10:11 ` ✗ Ro.CI.BAT: failure for drm/i915: Small compaction of the engine init code (rev4) Patchwork
4 siblings, 0 replies; 21+ messages in thread
From: Patchwork @ 2016-06-24 10:10 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Small compaction of the engine init code (rev3)
URL : https://patchwork.freedesktop.org/series/9053/
State : failure
== Summary ==
Series 9053v3 drm/i915: Small compaction of the engine init code
http://patchwork.freedesktop.org/api/1.0/series/9053/revisions/3/mbox
Test gem_exec_flush:
Subgroup basic-batch-kernel-default-cmd:
fail -> PASS (ro-byt-n2820)
Test kms_flip:
Subgroup basic-flip-vs-wf_vblank:
pass -> FAIL (ro-bdw-i5-5250u)
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-i7-5557U)
Subgroup suspend-read-crc-pipe-c:
skip -> PASS (ro-bdw-i7-5557U)
skip -> DMESG-WARN (ro-bdw-i5-5250u)
Test vgem_basic:
Subgroup create:
skip -> PASS (ro-bdw-i7-5557U)
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-bdw-i7-5557U)
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-bdw-i7-5557U)
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-bdw-i7-5557U)
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-bdw-i7-5557U)
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)
fi-hsw-i7-4770k total:227 pass:194 dwarn:0 dfail:0 fail:1 skip:32
fi-kbl-qkkr total:227 pass:161 dwarn:28 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
ro-bdw-i5-5250u total:227 pass:200 dwarn:4 dfail:1 fail:2 skip:20
ro-bdw-i7-5557U total:227 pass:203 dwarn:0 dfail:1 fail:1 skip:22
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:178 dwarn:0 dfail:1 fail:3 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
Results at /archive/results/CI_IGT_test/RO_Patchwork_1284/
8e5ac92 drm-intel-nightly: 2016y-06m-22d-18h-10m-30s UTC integration manifest
2c52a2d drm/i915: Small compaction of the engine init code
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* ✗ Ro.CI.BAT: failure for drm/i915: Small compaction of the engine init code (rev4)
2016-06-22 15:55 [PATCH] drm/i915: Small compaction of the engine init code Tvrtko Ursulin
` (3 preceding siblings ...)
2016-06-24 10:10 ` ✗ Ro.CI.BAT: failure for drm/i915: Small compaction of the engine init code (rev3) Patchwork
@ 2016-06-24 10:11 ` Patchwork
2016-06-24 11:09 ` Tvrtko Ursulin
4 siblings, 1 reply; 21+ messages in thread
From: Patchwork @ 2016-06-24 10:11 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
== 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)
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)
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)
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: ✗ Ro.CI.BAT: failure for drm/i915: Small compaction of the engine init code (rev4)
2016-06-24 10:11 ` ✗ Ro.CI.BAT: failure for drm/i915: Small compaction of the engine init code (rev4) Patchwork
@ 2016-06-24 11:09 ` Tvrtko Ursulin
0 siblings, 0 replies; 21+ messages in thread
From: Tvrtko Ursulin @ 2016-06-24 11:09 UTC (permalink / raw)
To: intel-gfx
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2016-06-24 11:09 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-22 15:55 [PATCH] drm/i915: Small compaction of the engine init code Tvrtko Ursulin
2016-06-22 16:10 ` Chris Wilson
2016-06-22 16:21 ` Tvrtko Ursulin
2016-06-22 16:28 ` Chris Wilson
2016-06-22 16:35 ` [PATCH v2] " Tvrtko Ursulin
2016-06-22 16:59 ` Chris Wilson
2016-06-23 10:26 ` Tvrtko Ursulin
2016-06-23 10:47 ` Chris Wilson
2016-06-23 11:12 ` [PATCH v3] " Tvrtko Ursulin
2016-06-23 11:25 ` Chris Wilson
2016-06-23 11:46 ` Tvrtko Ursulin
2016-06-23 12:11 ` Chris Wilson
2016-06-23 13:16 ` Tvrtko Ursulin
2016-06-23 13:25 ` Chris Wilson
2016-06-23 13:52 ` [PATCH v4] " Tvrtko Ursulin
2016-06-23 14:03 ` Chris Wilson
2016-06-22 16:18 ` ✓ Ro.CI.BAT: success for " Patchwork
2016-06-22 17:09 ` ✗ Ro.CI.BAT: warning for drm/i915: Small compaction of the engine init code (rev2) Patchwork
2016-06-24 10:10 ` ✗ Ro.CI.BAT: failure for drm/i915: Small compaction of the engine init code (rev3) Patchwork
2016-06-24 10:11 ` ✗ Ro.CI.BAT: failure for drm/i915: Small compaction of the engine init code (rev4) Patchwork
2016-06-24 11:09 ` Tvrtko Ursulin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.