[CI 1/2] drm/i915: Add enum for hardware engine identifiers

[CI 1/2] drm/i915: Add enum for hardware engine identifiers

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Put the engine hardware id in the common header so they are
not only associated with the GuC since they are needed for
the legacy semaphores implementation.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_engine_cs.c  | 14 +++++++-------
 drivers/gpu/drm/i915/intel_ringbuffer.h | 10 ++++++++--
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 5ec8a10fd18b..8a27bb9f6bc1 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -29,7 +29,7 @@
 static const struct engine_info {
 	const char *name;
 	unsigned exec_id;
-	unsigned guc_id;
+	enum intel_engine_hw_id hw_id;
 	u32 mmio_base;
 	unsigned irq_shift;
 	int (*init_legacy)(struct intel_engine_cs *engine);
@@ -38,7 +38,7 @@ static const struct engine_info {
 	[RCS] = {
 		.name = "render ring",
 		.exec_id = I915_EXEC_RENDER,
-		.guc_id = GUC_RENDER_ENGINE,
+		.hw_id = RCS_HW,
 		.mmio_base = RENDER_RING_BASE,
 		.irq_shift = GEN8_RCS_IRQ_SHIFT,
 		.init_execlists = logical_render_ring_init,
@@ -47,7 +47,7 @@ static const struct engine_info {
 	[BCS] = {
 		.name = "blitter ring",
 		.exec_id = I915_EXEC_BLT,
-		.guc_id = GUC_BLITTER_ENGINE,
+		.hw_id = BCS_HW,
 		.mmio_base = BLT_RING_BASE,
 		.irq_shift = GEN8_BCS_IRQ_SHIFT,
 		.init_execlists = logical_xcs_ring_init,
@@ -56,7 +56,7 @@ static const struct engine_info {
 	[VCS] = {
 		.name = "bsd ring",
 		.exec_id = I915_EXEC_BSD,
-		.guc_id = GUC_VIDEO_ENGINE,
+		.hw_id = VCS_HW,
 		.mmio_base = GEN6_BSD_RING_BASE,
 		.irq_shift = GEN8_VCS1_IRQ_SHIFT,
 		.init_execlists = logical_xcs_ring_init,
@@ -65,7 +65,7 @@ static const struct engine_info {
 	[VCS2] = {
 		.name = "bsd2 ring",
 		.exec_id = I915_EXEC_BSD,
-		.guc_id = GUC_VIDEO_ENGINE2,
+		.hw_id = VCS2_HW,
 		.mmio_base = GEN8_BSD2_RING_BASE,
 		.irq_shift = GEN8_VCS2_IRQ_SHIFT,
 		.init_execlists = logical_xcs_ring_init,
@@ -74,7 +74,7 @@ static const struct engine_info {
 	[VECS] = {
 		.name = "video enhancement ring",
 		.exec_id = I915_EXEC_VEBOX,
-		.guc_id = GUC_VIDEOENHANCE_ENGINE,
+		.hw_id = VECS_HW,
 		.mmio_base = VEBOX_RING_BASE,
 		.irq_shift = GEN8_VECS_IRQ_SHIFT,
 		.init_execlists = logical_xcs_ring_init,
@@ -93,7 +93,7 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
 	engine->i915 = dev_priv;
 	engine->name = info->name;
 	engine->exec_id = info->exec_id;
-	engine->hw_id = engine->guc_id = info->guc_id;
+	engine->hw_id = engine->guc_id = info->hw_id;
 	engine->mmio_base = info->mmio_base;
 	engine->irq_shift = info->irq_shift;
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index e3777572c70e..9d723c24eeff 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -146,8 +146,14 @@ struct intel_engine_cs {
 #define I915_NUM_ENGINES 5
 #define _VCS(n) (VCS + (n))
 	unsigned int exec_id;
-	unsigned int hw_id;
-	unsigned int guc_id; /* XXX same as hw_id? */
+	enum intel_engine_hw_id {
+		RCS_HW = 0,
+		VCS_HW,
+		BCS_HW,
+		VECS_HW,
+		VCS2_HW
+	} hw_id;
+	enum intel_engine_hw_id guc_id; /* XXX same as hw_id? */
 	u64 fence_context;
 	u32		mmio_base;
 	unsigned int irq_shift;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Build the legacy semaphore initialisation array using the engine
hardware ids instead of driver internal ones. This makes the
static array size dependent only on the number of gen6 semaphore
engines.

Also makes the per-engine semaphore wait and signal tables
hardware id indexed saving some more space.

v2: Refactor I915_GEN6_NUM_ENGINES to GEN6_SEMAPHORE_LAST. (Chris Wilson)
v3: More polish. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 55 +++++++++++++++++----------------
 drivers/gpu/drm/i915/intel_ringbuffer.h |  7 +++--
 2 files changed, 34 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index fa22bd87bab0..12703ea27259 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1337,8 +1337,7 @@ static int gen6_signal(struct drm_i915_gem_request *req)
 {
 	struct intel_ring *ring = req->ring;
 	struct drm_i915_private *dev_priv = req->i915;
-	struct intel_engine_cs *useless;
-	enum intel_engine_id id;
+	struct intel_engine_cs *engine;
 	int ret, num_rings;
 
 	num_rings = INTEL_INFO(dev_priv)->num_rings;
@@ -1346,9 +1345,13 @@ static int gen6_signal(struct drm_i915_gem_request *req)
 	if (ret)
 		return ret;
 
-	for_each_engine_id(useless, dev_priv, id) {
-		i915_reg_t mbox_reg = req->engine->semaphore.mbox.signal[id];
+	for_each_engine(engine, dev_priv) {
+		i915_reg_t mbox_reg;
+
+		if (!(BIT(engine->hw_id) & GEN6_SEMAPHORES_MASK))
+			continue;
 
+		mbox_reg = req->engine->semaphore.mbox.signal[engine->hw_id];
 		if (i915_mmio_reg_valid(mbox_reg)) {
 			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
 			intel_ring_emit_reg(ring, mbox_reg);
@@ -1495,7 +1498,7 @@ gen6_ring_sync_to(struct drm_i915_gem_request *req,
 	u32 dw1 = MI_SEMAPHORE_MBOX |
 		  MI_SEMAPHORE_COMPARE |
 		  MI_SEMAPHORE_REGISTER;
-	u32 wait_mbox = signal->engine->semaphore.mbox.wait[req->engine->id];
+	u32 wait_mbox = signal->engine->semaphore.mbox.wait[req->engine->hw_id];
 	int ret;
 
 	WARN_ON(wait_mbox == MI_SEMAPHORE_SYNC_INVALID);
@@ -2569,41 +2572,41 @@ static void intel_ring_init_semaphores(struct drm_i915_private *dev_priv,
 		 * initialized as INVALID.  Gen8 will initialize the
 		 * sema between VCS2 and RCS later.
 		 */
-		for (i = 0; i < I915_NUM_ENGINES; i++) {
+		for (i = 0; i < GEN6_NUM_SEMAPHORES; i++) {
 			static const struct {
 				u32 wait_mbox;
 				i915_reg_t mbox_reg;
-			} sem_data[I915_NUM_ENGINES][I915_NUM_ENGINES] = {
-				[RCS] = {
-					[VCS] =  { .wait_mbox = MI_SEMAPHORE_SYNC_RV,  .mbox_reg = GEN6_VRSYNC },
-					[BCS] =  { .wait_mbox = MI_SEMAPHORE_SYNC_RB,  .mbox_reg = GEN6_BRSYNC },
-					[VECS] = { .wait_mbox = MI_SEMAPHORE_SYNC_RVE, .mbox_reg = GEN6_VERSYNC },
+			} sem_data[GEN6_NUM_SEMAPHORES][GEN6_NUM_SEMAPHORES] = {
+				[RCS_HW] = {
+					[VCS_HW] =  { .wait_mbox = MI_SEMAPHORE_SYNC_RV,  .mbox_reg = GEN6_VRSYNC },
+					[BCS_HW] =  { .wait_mbox = MI_SEMAPHORE_SYNC_RB,  .mbox_reg = GEN6_BRSYNC },
+					[VECS_HW] = { .wait_mbox = MI_SEMAPHORE_SYNC_RVE, .mbox_reg = GEN6_VERSYNC },
 				},
-				[VCS] = {
-					[RCS] =  { .wait_mbox = MI_SEMAPHORE_SYNC_VR,  .mbox_reg = GEN6_RVSYNC },
-					[BCS] =  { .wait_mbox = MI_SEMAPHORE_SYNC_VB,  .mbox_reg = GEN6_BVSYNC },
-					[VECS] = { .wait_mbox = MI_SEMAPHORE_SYNC_VVE, .mbox_reg = GEN6_VEVSYNC },
+				[VCS_HW] = {
+					[RCS_HW] =  { .wait_mbox = MI_SEMAPHORE_SYNC_VR,  .mbox_reg = GEN6_RVSYNC },
+					[BCS_HW] =  { .wait_mbox = MI_SEMAPHORE_SYNC_VB,  .mbox_reg = GEN6_BVSYNC },
+					[VECS_HW] = { .wait_mbox = MI_SEMAPHORE_SYNC_VVE, .mbox_reg = GEN6_VEVSYNC },
 				},
-				[BCS] = {
-					[RCS] =  { .wait_mbox = MI_SEMAPHORE_SYNC_BR,  .mbox_reg = GEN6_RBSYNC },
-					[VCS] =  { .wait_mbox = MI_SEMAPHORE_SYNC_BV,  .mbox_reg = GEN6_VBSYNC },
-					[VECS] = { .wait_mbox = MI_SEMAPHORE_SYNC_BVE, .mbox_reg = GEN6_VEBSYNC },
+				[BCS_HW] = {
+					[RCS_HW] =  { .wait_mbox = MI_SEMAPHORE_SYNC_BR,  .mbox_reg = GEN6_RBSYNC },
+					[VCS_HW] =  { .wait_mbox = MI_SEMAPHORE_SYNC_BV,  .mbox_reg = GEN6_VBSYNC },
+					[VECS_HW] = { .wait_mbox = MI_SEMAPHORE_SYNC_BVE, .mbox_reg = GEN6_VEBSYNC },
 				},
-				[VECS] = {
-					[RCS] =  { .wait_mbox = MI_SEMAPHORE_SYNC_VER, .mbox_reg = GEN6_RVESYNC },
-					[VCS] =  { .wait_mbox = MI_SEMAPHORE_SYNC_VEV, .mbox_reg = GEN6_VVESYNC },
-					[BCS] =  { .wait_mbox = MI_SEMAPHORE_SYNC_VEB, .mbox_reg = GEN6_BVESYNC },
+				[VECS_HW] = {
+					[RCS_HW] =  { .wait_mbox = MI_SEMAPHORE_SYNC_VER, .mbox_reg = GEN6_RVESYNC },
+					[VCS_HW] =  { .wait_mbox = MI_SEMAPHORE_SYNC_VEV, .mbox_reg = GEN6_VVESYNC },
+					[BCS_HW] =  { .wait_mbox = MI_SEMAPHORE_SYNC_VEB, .mbox_reg = GEN6_BVESYNC },
 				},
 			};
 			u32 wait_mbox;
 			i915_reg_t mbox_reg;
 
-			if (i == engine->id || i == VCS2) {
+			if (i == engine->hw_id) {
 				wait_mbox = MI_SEMAPHORE_SYNC_INVALID;
 				mbox_reg = GEN6_NOSYNC;
 			} else {
-				wait_mbox = sem_data[engine->id][i].wait_mbox;
-				mbox_reg = sem_data[engine->id][i].mbox_reg;
+				wait_mbox = sem_data[engine->hw_id][i].wait_mbox;
+				mbox_reg = sem_data[engine->hw_id][i].mbox_reg;
 			}
 
 			engine->semaphore.mbox.wait[i] = wait_mbox;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 9d723c24eeff..86612d502c65 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -278,11 +278,14 @@ struct intel_engine_cs {
 		u32	sync_seqno[I915_NUM_ENGINES-1];
 
 		union {
+#define GEN6_SEMAPHORE_LAST	VECS_HW
+#define GEN6_NUM_SEMAPHORES	(GEN6_SEMAPHORE_LAST + 1)
+#define GEN6_SEMAPHORES_MASK	GENMASK(GEN6_SEMAPHORE_LAST, 0)
 			struct {
 				/* our mbox written by others */
-				u32		wait[I915_NUM_ENGINES];
+				u32		wait[GEN6_NUM_SEMAPHORES];
 				/* mboxes this ring signals to */
-				i915_reg_t	signal[I915_NUM_ENGINES];
+				i915_reg_t	signal[GEN6_NUM_SEMAPHORES];
 			} mbox;
 			u64		signal_ggtt[I915_NUM_ENGINES];
 		};
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array

[Chris Wilson]

On Tue, Aug 16, 2016 at 05:04:21PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Build the legacy semaphore initialisation array using the engine
> hardware ids instead of driver internal ones. This makes the
> static array size dependent only on the number of gen6 semaphore
> engines.
> 
> Also makes the per-engine semaphore wait and signal tables
> hardware id indexed saving some more space.
> 
> v2: Refactor I915_GEN6_NUM_ENGINES to GEN6_SEMAPHORE_LAST. (Chris Wilson)
> v3: More polish. (Chris Wilson)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 55 +++++++++++++++++----------------
>  drivers/gpu/drm/i915/intel_ringbuffer.h |  7 +++--
>  2 files changed, 34 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index fa22bd87bab0..12703ea27259 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1337,8 +1337,7 @@ static int gen6_signal(struct drm_i915_gem_request *req)
>  {
>  	struct intel_ring *ring = req->ring;
>  	struct drm_i915_private *dev_priv = req->i915;
> -	struct intel_engine_cs *useless;
> -	enum intel_engine_id id;
> +	struct intel_engine_cs *engine;
>  	int ret, num_rings;
>  
>  	num_rings = INTEL_INFO(dev_priv)->num_rings;
> @@ -1346,9 +1345,13 @@ static int gen6_signal(struct drm_i915_gem_request *req)
>  	if (ret)
>  		return ret;
>  
> -	for_each_engine_id(useless, dev_priv, id) {
> -		i915_reg_t mbox_reg = req->engine->semaphore.mbox.signal[id];
> +	for_each_engine(engine, dev_priv) {
> +		i915_reg_t mbox_reg;
> +
> +		if (!(BIT(engine->hw_id) & GEN6_SEMAPHORES_MASK))
> +			continue;

Later on, it would be nice if this used
	for_each_engine_masked()
instead (presupposing we have an efficient iterator for a sparse mask).
The issue is in defining GEN6_SEMAPHORES_MASK

#define GEN6_SEMAPHORES_MASK \
	(RENDER_RING |
	 BSD_RING |
	 BSD2_RING |
	 BLT_RING |
	 VECS_RING)

Defnitely pencil that in for when otherwise we would iterate over 10
empty slots in dev_priv->engines[].

I'm thinking that we should make for_each_engine_mask() the default, say
	for_each_engine(engine, dev_priv, ALL_ENGINES, id)
-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

✗ Ro.CI.BAT: failure for series starting with [CI,1/2] drm/i915: Add enum for hardware engine identifiers

[Patchwork]

== Series Details ==

Series: series starting with [CI,1/2] drm/i915: Add enum for hardware engine identifiers
URL   : https://patchwork.freedesktop.org/series/11172/
State : failure

== Summary ==

Series 11172v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/11172/revisions/1/mbox

Test kms_cursor_legacy:
        Subgroup basic-flip-vs-cursor-legacy:
                fail       -> PASS       (ro-byt-n2820)
                pass       -> FAIL       (fi-hsw-i7-4770k)
                fail       -> PASS       (ro-bdw-i5-5250u)
        Subgroup basic-flip-vs-cursor-varying-size:
                pass       -> FAIL       (fi-hsw-i7-4770k)
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-a-frame-sequence:
                fail       -> PASS       (ro-byt-n2820)
        Subgroup suspend-read-crc-pipe-a:
                dmesg-warn -> PASS       (ro-bdw-i7-5600u)
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)

fi-hsw-i7-4770k  total:244  pass:220  dwarn:0   dfail:0   fail:2   skip:22 
fi-kbl-qkkr      total:244  pass:186  dwarn:28  dfail:0   fail:3   skip:27 
fi-skl-i7-6700k  total:244  pass:208  dwarn:4   dfail:2   fail:2   skip:28 
fi-snb-i7-2600   total:244  pass:202  dwarn:0   dfail:0   fail:0   skip:42 
ro-bdw-i5-5250u  total:240  pass:219  dwarn:1   dfail:0   fail:1   skip:19 
ro-bdw-i7-5600u  total:240  pass:207  dwarn:0   dfail:0   fail:1   skip:32 
ro-bsw-n3050     total:240  pass:195  dwarn:0   dfail:0   fail:3   skip:42 
ro-byt-n2820     total:240  pass:198  dwarn:0   dfail:0   fail:2   skip:40 
ro-hsw-i3-4010u  total:240  pass:214  dwarn:0   dfail:0   fail:0   skip:26 
ro-hsw-i7-4770r  total:240  pass:185  dwarn:0   dfail:0   fail:0   skip:55 
ro-ilk1-i5-650   total:235  pass:174  dwarn:0   dfail:0   fail:1   skip:60 
ro-ivb-i7-3770   total:240  pass:205  dwarn:0   dfail:0   fail:0   skip:35 
ro-ivb2-i7-3770  total:240  pass:209  dwarn:0   dfail:0   fail:0   skip:31 
ro-skl3-i5-6260u total:240  pass:222  dwarn:0   dfail:0   fail:4   skip:14 
ro-bdw-i7-5557U failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1894/

b5e8283 drm-intel-nightly: 2016y-08m-16d-15h-40m-39s UTC integration manifest
d5e3403 drm/i915: Initialize legacy semaphores from engine hw id indexed array
79308b3 drm/i915: Add enum for hardware engine identifiers

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array

[Tvrtko Ursulin]

On 16/08/16 17:18, Chris Wilson wrote:
> On Tue, Aug 16, 2016 at 05:04:21PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Build the legacy semaphore initialisation array using the engine
>> hardware ids instead of driver internal ones. This makes the
>> static array size dependent only on the number of gen6 semaphore
>> engines.
>>
>> Also makes the per-engine semaphore wait and signal tables
>> hardware id indexed saving some more space.
>>
>> v2: Refactor I915_GEN6_NUM_ENGINES to GEN6_SEMAPHORE_LAST. (Chris Wilson)
>> v3: More polish. (Chris Wilson)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   drivers/gpu/drm/i915/intel_ringbuffer.c | 55 +++++++++++++++++----------------
>>   drivers/gpu/drm/i915/intel_ringbuffer.h |  7 +++--
>>   2 files changed, 34 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index fa22bd87bab0..12703ea27259 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -1337,8 +1337,7 @@ static int gen6_signal(struct drm_i915_gem_request *req)
>>   {
>>   	struct intel_ring *ring = req->ring;
>>   	struct drm_i915_private *dev_priv = req->i915;
>> -	struct intel_engine_cs *useless;
>> -	enum intel_engine_id id;
>> +	struct intel_engine_cs *engine;
>>   	int ret, num_rings;
>>
>>   	num_rings = INTEL_INFO(dev_priv)->num_rings;
>> @@ -1346,9 +1345,13 @@ static int gen6_signal(struct drm_i915_gem_request *req)
>>   	if (ret)
>>   		return ret;
>>
>> -	for_each_engine_id(useless, dev_priv, id) {
>> -		i915_reg_t mbox_reg = req->engine->semaphore.mbox.signal[id];
>> +	for_each_engine(engine, dev_priv) {
>> +		i915_reg_t mbox_reg;
>> +
>> +		if (!(BIT(engine->hw_id) & GEN6_SEMAPHORES_MASK))
>> +			continue;
>
> Later on, it would be nice if this used
> 	for_each_engine_masked()
> instead (presupposing we have an efficient iterator for a sparse mask).
> The issue is in defining GEN6_SEMAPHORES_MASK
>
> #define GEN6_SEMAPHORES_MASK \
> 	(RENDER_RING |
> 	 BSD_RING |
> 	 BSD2_RING |
> 	 BLT_RING |
> 	 VECS_RING)
>
> Defnitely pencil that in for when otherwise we would iterate over 10
> empty slots in dev_priv->engines[].
>
> I'm thinking that we should make for_each_engine_mask() the default, say
> 	for_each_engine(engine, dev_priv, ALL_ENGINES, id)

Or add an initialized engine array to dev_priv, in addition to the 
existing map for best of both worlds.

That would prevent churn in for_each_engine users.

Or just give up and add something smarter, like a typdef 
intel_engine_it, to each of them and be done with churn once for all.

But we said there aren't any iterators on fast paths anyway so I would 
rather we chose to do nothing. :)

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array

[Chris Wilson]

On Wed, Aug 17, 2016 at 10:34:18AM +0100, Tvrtko Ursulin wrote:
> 
> On 16/08/16 17:18, Chris Wilson wrote:
> >On Tue, Aug 16, 2016 at 05:04:21PM +0100, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >>Build the legacy semaphore initialisation array using the engine
> >>hardware ids instead of driver internal ones. This makes the
> >>static array size dependent only on the number of gen6 semaphore
> >>engines.
> >>
> >>Also makes the per-engine semaphore wait and signal tables
> >>hardware id indexed saving some more space.
> >>
> >>v2: Refactor I915_GEN6_NUM_ENGINES to GEN6_SEMAPHORE_LAST. (Chris Wilson)
> >>v3: More polish. (Chris Wilson)
> >>
> >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>---
> >>  drivers/gpu/drm/i915/intel_ringbuffer.c | 55 +++++++++++++++++----------------
> >>  drivers/gpu/drm/i915/intel_ringbuffer.h |  7 +++--
> >>  2 files changed, 34 insertions(+), 28 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>index fa22bd87bab0..12703ea27259 100644
> >>--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>@@ -1337,8 +1337,7 @@ static int gen6_signal(struct drm_i915_gem_request *req)
> >>  {
> >>  	struct intel_ring *ring = req->ring;
> >>  	struct drm_i915_private *dev_priv = req->i915;
> >>-	struct intel_engine_cs *useless;
> >>-	enum intel_engine_id id;
> >>+	struct intel_engine_cs *engine;
> >>  	int ret, num_rings;
> >>
> >>  	num_rings = INTEL_INFO(dev_priv)->num_rings;
> >>@@ -1346,9 +1345,13 @@ static int gen6_signal(struct drm_i915_gem_request *req)
> >>  	if (ret)
> >>  		return ret;
> >>
> >>-	for_each_engine_id(useless, dev_priv, id) {
> >>-		i915_reg_t mbox_reg = req->engine->semaphore.mbox.signal[id];
> >>+	for_each_engine(engine, dev_priv) {
> >>+		i915_reg_t mbox_reg;
> >>+
> >>+		if (!(BIT(engine->hw_id) & GEN6_SEMAPHORES_MASK))
> >>+			continue;
> >
> >Later on, it would be nice if this used
> >	for_each_engine_masked()
> >instead (presupposing we have an efficient iterator for a sparse mask).
> >The issue is in defining GEN6_SEMAPHORES_MASK
> >
> >#define GEN6_SEMAPHORES_MASK \
> >	(RENDER_RING |
> >	 BSD_RING |
> >	 BSD2_RING |
> >	 BLT_RING |
> >	 VECS_RING)
> >
> >Defnitely pencil that in for when otherwise we would iterate over 10
> >empty slots in dev_priv->engines[].
> >
> >I'm thinking that we should make for_each_engine_mask() the default, say
> >	for_each_engine(engine, dev_priv, ALL_ENGINES, id)
> 
> Or add an initialized engine array to dev_priv, in addition to the
> existing map for best of both worlds.

We have the ring_mask which already tells us that mapping, so I think
the second array is overkill.
 
> That would prevent churn in for_each_engine users.
> 
> Or just give up and add something smarter, like a typdef
> intel_engine_it, to each of them and be done with churn once for
> all.
> 
> But we said there aren't any iterators on fast paths anyway so I
> would rather we chose to do nothing. :)

gen6_semaphore_signal is called uncomfortably often, I'd be tempted to
keep it trim. We should also contemplate delayed semaphore signaling
again.
-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

Re: [CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array

[Tvrtko Ursulin]

On 17/08/16 10:41, Chris Wilson wrote:
> On Wed, Aug 17, 2016 at 10:34:18AM +0100, Tvrtko Ursulin wrote:
>> On 16/08/16 17:18, Chris Wilson wrote:
>>> On Tue, Aug 16, 2016 at 05:04:21PM +0100, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> Build the legacy semaphore initialisation array using the engine
>>>> hardware ids instead of driver internal ones. This makes the
>>>> static array size dependent only on the number of gen6 semaphore
>>>> engines.
>>>>
>>>> Also makes the per-engine semaphore wait and signal tables
>>>> hardware id indexed saving some more space.
>>>>
>>>> v2: Refactor I915_GEN6_NUM_ENGINES to GEN6_SEMAPHORE_LAST. (Chris Wilson)
>>>> v3: More polish. (Chris Wilson)
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>> ---
>>>>   drivers/gpu/drm/i915/intel_ringbuffer.c | 55 +++++++++++++++++----------------
>>>>   drivers/gpu/drm/i915/intel_ringbuffer.h |  7 +++--
>>>>   2 files changed, 34 insertions(+), 28 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>>>> index fa22bd87bab0..12703ea27259 100644
>>>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>>>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>>>> @@ -1337,8 +1337,7 @@ static int gen6_signal(struct drm_i915_gem_request *req)
>>>>   {
>>>>   	struct intel_ring *ring = req->ring;
>>>>   	struct drm_i915_private *dev_priv = req->i915;
>>>> -	struct intel_engine_cs *useless;
>>>> -	enum intel_engine_id id;
>>>> +	struct intel_engine_cs *engine;
>>>>   	int ret, num_rings;
>>>>
>>>>   	num_rings = INTEL_INFO(dev_priv)->num_rings;
>>>> @@ -1346,9 +1345,13 @@ static int gen6_signal(struct drm_i915_gem_request *req)
>>>>   	if (ret)
>>>>   		return ret;
>>>>
>>>> -	for_each_engine_id(useless, dev_priv, id) {
>>>> -		i915_reg_t mbox_reg = req->engine->semaphore.mbox.signal[id];
>>>> +	for_each_engine(engine, dev_priv) {
>>>> +		i915_reg_t mbox_reg;
>>>> +
>>>> +		if (!(BIT(engine->hw_id) & GEN6_SEMAPHORES_MASK))
>>>> +			continue;
>>>
>>> Later on, it would be nice if this used
>>> 	for_each_engine_masked()
>>> instead (presupposing we have an efficient iterator for a sparse mask).
>>> The issue is in defining GEN6_SEMAPHORES_MASK
>>>
>>> #define GEN6_SEMAPHORES_MASK \
>>> 	(RENDER_RING |
>>> 	 BSD_RING |
>>> 	 BSD2_RING |
>>> 	 BLT_RING |
>>> 	 VECS_RING)
>>>
>>> Defnitely pencil that in for when otherwise we would iterate over 10
>>> empty slots in dev_priv->engines[].
>>>
>>> I'm thinking that we should make for_each_engine_mask() the default, say
>>> 	for_each_engine(engine, dev_priv, ALL_ENGINES, id)
>>
>> Or add an initialized engine array to dev_priv, in addition to the
>> existing map for best of both worlds.
>
> We have the ring_mask which already tells us that mapping, so I think
> the second array is overkill.

Yes, I said "in addition to the existing map". In addition we could have 
an array of only initialized engines to avoid any skipping at runtime. 
Since iterators end with intel_engine_initialized anyway.

>> That would prevent churn in for_each_engine users.
>>
>> Or just give up and add something smarter, like a typdef
>> intel_engine_it, to each of them and be done with churn once for
>> all.
>>
>> But we said there aren't any iterators on fast paths anyway so I
>> would rather we chose to do nothing. :)
>
> gen6_semaphore_signal is called uncomfortably often, I'd be tempted to
> keep it trim. We should also contemplate delayed semaphore signaling
> again.

The list/array idea would solve that AFAICT.

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array

[Chris Wilson]

On Wed, Aug 17, 2016 at 10:57:34AM +0100, Tvrtko Ursulin wrote:
> 
> On 17/08/16 10:41, Chris Wilson wrote:
> >On Wed, Aug 17, 2016 at 10:34:18AM +0100, Tvrtko Ursulin wrote:
> >>Or add an initialized engine array to dev_priv, in addition to the
> >>existing map for best of both worlds.
> >
> >We have the ring_mask which already tells us that mapping, so I think
> >the second array is overkill.
> 
> Yes, I said "in addition to the existing map". In addition we could
> have an array of only initialized engines to avoid any skipping at
> runtime. Since iterators end with intel_engine_initialized anyway.

And I'm saying we already have that information in ring_mask. 
-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

Re: ✗ Ro.CI.BAT: failure for series starting with [CI,1/2] drm/i915: Add enum for hardware engine identifiers

[Tvrtko Ursulin]

On 16/08/16 17:35, Patchwork wrote:
> == Series Details ==
>
> Series: series starting with [CI,1/2] drm/i915: Add enum for hardware engine identifiers
> URL   : https://patchwork.freedesktop.org/series/11172/
> State : failure
>
> == Summary ==
>
> Series 11172v1 Series without cover letter
> http://patchwork.freedesktop.org/api/1.0/series/11172/revisions/1/mbox
>
> Test kms_cursor_legacy:
>          Subgroup basic-flip-vs-cursor-legacy:
>                  fail       -> PASS       (ro-byt-n2820)
>                  pass       -> FAIL       (fi-hsw-i7-4770k)

https://bugs.freedesktop.org/show_bug.cgi?id=97188

>                  fail       -> PASS       (ro-bdw-i5-5250u)
>          Subgroup basic-flip-vs-cursor-varying-size:
>                  pass       -> FAIL       (fi-hsw-i7-4770k)

https://bugs.freedesktop.org/show_bug.cgi?id=97188

> Test kms_pipe_crc_basic:
>          Subgroup nonblocking-crc-pipe-a-frame-sequence:
>                  fail       -> PASS       (ro-byt-n2820)
>          Subgroup suspend-read-crc-pipe-a:
>                  dmesg-warn -> PASS       (ro-bdw-i7-5600u)
>          Subgroup suspend-read-crc-pipe-b:
>                  dmesg-warn -> SKIP       (ro-bdw-i5-5250u)
>
> fi-hsw-i7-4770k  total:244  pass:220  dwarn:0   dfail:0   fail:2   skip:22
> fi-kbl-qkkr      total:244  pass:186  dwarn:28  dfail:0   fail:3   skip:27
> fi-skl-i7-6700k  total:244  pass:208  dwarn:4   dfail:2   fail:2   skip:28
> fi-snb-i7-2600   total:244  pass:202  dwarn:0   dfail:0   fail:0   skip:42
> ro-bdw-i5-5250u  total:240  pass:219  dwarn:1   dfail:0   fail:1   skip:19
> ro-bdw-i7-5600u  total:240  pass:207  dwarn:0   dfail:0   fail:1   skip:32
> ro-bsw-n3050     total:240  pass:195  dwarn:0   dfail:0   fail:3   skip:42
> ro-byt-n2820     total:240  pass:198  dwarn:0   dfail:0   fail:2   skip:40
> ro-hsw-i3-4010u  total:240  pass:214  dwarn:0   dfail:0   fail:0   skip:26
> ro-hsw-i7-4770r  total:240  pass:185  dwarn:0   dfail:0   fail:0   skip:55
> ro-ilk1-i5-650   total:235  pass:174  dwarn:0   dfail:0   fail:1   skip:60
> ro-ivb-i7-3770   total:240  pass:205  dwarn:0   dfail:0   fail:0   skip:35
> ro-ivb2-i7-3770  total:240  pass:209  dwarn:0   dfail:0   fail:0   skip:31
> ro-skl3-i5-6260u total:240  pass:222  dwarn:0   dfail:0   fail:4   skip:14
> ro-bdw-i7-5557U failed to connect after reboot
>
> Results at /archive/results/CI_IGT_test/RO_Patchwork_1894/
>
> b5e8283 drm-intel-nightly: 2016y-08m-16d-15h-40m-39s UTC integration manifest
> d5e3403 drm/i915: Initialize legacy semaphores from engine hw id indexed array
> 79308b3 drm/i915: Add enum for hardware engine identifiers

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

Re: [CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array

[Tvrtko Ursulin]

On 17/08/16 11:05, Chris Wilson wrote:
> On Wed, Aug 17, 2016 at 10:57:34AM +0100, Tvrtko Ursulin wrote:
>>
>> On 17/08/16 10:41, Chris Wilson wrote:
>>> On Wed, Aug 17, 2016 at 10:34:18AM +0100, Tvrtko Ursulin wrote:
>>>> Or add an initialized engine array to dev_priv, in addition to the
>>>> existing map for best of both worlds.
>>>
>>> We have the ring_mask which already tells us that mapping, so I think
>>> the second array is overkill.
>>
>> Yes, I said "in addition to the existing map". In addition we could
>> have an array of only initialized engines to avoid any skipping at
>> runtime. Since iterators end with intel_engine_initialized anyway.
>
> And I'm saying we already have that information in ring_mask.

The ffs smarts you've been developing? I am not sure devising very smart 
macros and expanding all that code to all the call sites is that great. 
It is effectively just re-implementing arrays at runtime using CPU 
instructions.

What would be the big deal of just building the array when engines are 
initialized for simplicity? Just the allure of getting away with no 
iterator variable?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array

[Chris Wilson]

On Wed, Aug 17, 2016 at 03:36:51PM +0100, Tvrtko Ursulin wrote:
> 
> On 17/08/16 11:05, Chris Wilson wrote:
> >On Wed, Aug 17, 2016 at 10:57:34AM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 17/08/16 10:41, Chris Wilson wrote:
> >>>On Wed, Aug 17, 2016 at 10:34:18AM +0100, Tvrtko Ursulin wrote:
> >>>>Or add an initialized engine array to dev_priv, in addition to the
> >>>>existing map for best of both worlds.
> >>>
> >>>We have the ring_mask which already tells us that mapping, so I think
> >>>the second array is overkill.
> >>
> >>Yes, I said "in addition to the existing map". In addition we could
> >>have an array of only initialized engines to avoid any skipping at
> >>runtime. Since iterators end with intel_engine_initialized anyway.
> >
> >And I'm saying we already have that information in ring_mask.
> 
> The ffs smarts you've been developing? I am not sure devising very
> smart macros and expanding all that code to all the call sites is
> that great. It is effectively just re-implementing arrays at runtime
> using CPU instructions.
> 
> What would be the big deal of just building the array when engines
> are initialized for simplicity? Just the allure of getting away with
> no iterator variable?

It's just the redundant information. We definitely want the (sparse)
id->engine lookup table just for convenience in execbuf and friends.
Given that, having a second array is overkill. A list may be a
reasonable compromise, and I guess pushing for using an ffs iterator
where it matters (where we know we have a sparse set).
-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

Re: [CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array

[Tvrtko Ursulin]

On 17/08/16 15:44, Chris Wilson wrote:
> On Wed, Aug 17, 2016 at 03:36:51PM +0100, Tvrtko Ursulin wrote:
>> On 17/08/16 11:05, Chris Wilson wrote:
>>> On Wed, Aug 17, 2016 at 10:57:34AM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 17/08/16 10:41, Chris Wilson wrote:
>>>>> On Wed, Aug 17, 2016 at 10:34:18AM +0100, Tvrtko Ursulin wrote:
>>>>>> Or add an initialized engine array to dev_priv, in addition to the
>>>>>> existing map for best of both worlds.
>>>>>
>>>>> We have the ring_mask which already tells us that mapping, so I think
>>>>> the second array is overkill.
>>>>
>>>> Yes, I said "in addition to the existing map". In addition we could
>>>> have an array of only initialized engines to avoid any skipping at
>>>> runtime. Since iterators end with intel_engine_initialized anyway.
>>>
>>> And I'm saying we already have that information in ring_mask.
>>
>> The ffs smarts you've been developing? I am not sure devising very
>> smart macros and expanding all that code to all the call sites is
>> that great. It is effectively just re-implementing arrays at runtime
>> using CPU instructions.
>>
>> What would be the big deal of just building the array when engines
>> are initialized for simplicity? Just the allure of getting away with
>> no iterator variable?
>
> It's just the redundant information. We definitely want the (sparse)
> id->engine lookup table just for convenience in execbuf and friends.
> Given that, having a second array is overkill. A list may be a
> reasonable compromise, and I guess pushing for using an ffs iterator
> where it matters (where we know we have a sparse set).

Don't get what you consider a list vs array? I was talking about an 
contiguous array of engine pointers (only initialized ones).

Can't see that it is redundant or overkill since it is not that uncommon 
to have two separate data structure/indices pointing the the same thing 
for ease of manipulation/use.

As I said, you build the list once at init time, or you build it 
effectively at the every iterator site. When you call ffs() you make the 
CPU do the skipping just on a lower level than the current C code does.

Regards,

Tvrtko


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array

[Chris Wilson]

On Wed, Aug 17, 2016 at 04:10:00PM +0100, Tvrtko Ursulin wrote:
> 
> On 17/08/16 15:44, Chris Wilson wrote:
> >On Wed, Aug 17, 2016 at 03:36:51PM +0100, Tvrtko Ursulin wrote:
> >>On 17/08/16 11:05, Chris Wilson wrote:
> >>>On Wed, Aug 17, 2016 at 10:57:34AM +0100, Tvrtko Ursulin wrote:
> >>>>
> >>>>On 17/08/16 10:41, Chris Wilson wrote:
> >>>>>On Wed, Aug 17, 2016 at 10:34:18AM +0100, Tvrtko Ursulin wrote:
> >>>>>>Or add an initialized engine array to dev_priv, in addition to the
> >>>>>>existing map for best of both worlds.
> >>>>>
> >>>>>We have the ring_mask which already tells us that mapping, so I think
> >>>>>the second array is overkill.
> >>>>
> >>>>Yes, I said "in addition to the existing map". In addition we could
> >>>>have an array of only initialized engines to avoid any skipping at
> >>>>runtime. Since iterators end with intel_engine_initialized anyway.
> >>>
> >>>And I'm saying we already have that information in ring_mask.
> >>
> >>The ffs smarts you've been developing? I am not sure devising very
> >>smart macros and expanding all that code to all the call sites is
> >>that great. It is effectively just re-implementing arrays at runtime
> >>using CPU instructions.
> >>
> >>What would be the big deal of just building the array when engines
> >>are initialized for simplicity? Just the allure of getting away with
> >>no iterator variable?
> >
> >It's just the redundant information. We definitely want the (sparse)
> >id->engine lookup table just for convenience in execbuf and friends.
> >Given that, having a second array is overkill. A list may be a
> >reasonable compromise, and I guess pushing for using an ffs iterator
> >where it matters (where we know we have a sparse set).
> 
> Don't get what you consider a list vs array? I was talking about an
> contiguous array of engine pointers (only initialized ones).
> 
> Can't see that it is redundant or overkill since it is not that
> uncommon to have two separate data structure/indices pointing the
> the same thing for ease of manipulation/use.
> 
> As I said, you build the list once at init time, or you build it
> effectively at the every iterator site. When you call ffs() you make
> the CPU do the skipping just on a lower level than the current C
> code does.

All I am saying is that makes having the dense array of engines
redundant.

There are only a few places where iterating over the engines is anything
other than a rare event (init, reset, etc) and where we want a sparse
set we typically have a mask of engines to iterate over already. In that
regard just the list of engines would suffice to make the common iterator
very simple.
-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