* [CI 1/2] drm/i915: Add enum for hardware engine identifiers
@ 2016-08-16 16:04 Tvrtko Ursulin
2016-08-16 16:04 ` [CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array Tvrtko Ursulin
2016-08-16 16:35 ` ✗ Ro.CI.BAT: failure for series starting with [CI,1/2] drm/i915: Add enum for hardware engine identifiers Patchwork
0 siblings, 2 replies; 13+ messages in thread
From: Tvrtko Ursulin @ 2016-08-16 16:04 UTC (permalink / raw)
To: Intel-gfx
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array
2016-08-16 16:04 [CI 1/2] drm/i915: Add enum for hardware engine identifiers Tvrtko Ursulin
@ 2016-08-16 16:04 ` Tvrtko Ursulin
2016-08-16 16:18 ` Chris Wilson
2016-08-16 16:35 ` ✗ Ro.CI.BAT: failure for series starting with [CI,1/2] drm/i915: Add enum for hardware engine identifiers Patchwork
1 sibling, 1 reply; 13+ messages in thread
From: Tvrtko Ursulin @ 2016-08-16 16:04 UTC (permalink / raw)
To: Intel-gfx
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array
2016-08-16 16:04 ` [CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array Tvrtko Ursulin
@ 2016-08-16 16:18 ` Chris Wilson
2016-08-17 9:34 ` Tvrtko Ursulin
0 siblings, 1 reply; 13+ messages in thread
From: Chris Wilson @ 2016-08-16 16:18 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* ✗ Ro.CI.BAT: failure for series starting with [CI,1/2] drm/i915: Add enum for hardware engine identifiers
2016-08-16 16:04 [CI 1/2] drm/i915: Add enum for hardware engine identifiers Tvrtko Ursulin
2016-08-16 16:04 ` [CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array Tvrtko Ursulin
@ 2016-08-16 16:35 ` Patchwork
2016-08-17 10:31 ` Tvrtko Ursulin
1 sibling, 1 reply; 13+ messages in thread
From: Patchwork @ 2016-08-16 16:35 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
== 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array
2016-08-16 16:18 ` Chris Wilson
@ 2016-08-17 9:34 ` Tvrtko Ursulin
2016-08-17 9:41 ` Chris Wilson
0 siblings, 1 reply; 13+ messages in thread
From: Tvrtko Ursulin @ 2016-08-17 9:34 UTC (permalink / raw)
To: Chris Wilson, Intel-gfx
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array
2016-08-17 9:34 ` Tvrtko Ursulin
@ 2016-08-17 9:41 ` Chris Wilson
2016-08-17 9:57 ` Tvrtko Ursulin
0 siblings, 1 reply; 13+ messages in thread
From: Chris Wilson @ 2016-08-17 9:41 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array
2016-08-17 9:41 ` Chris Wilson
@ 2016-08-17 9:57 ` Tvrtko Ursulin
2016-08-17 10:05 ` Chris Wilson
0 siblings, 1 reply; 13+ messages in thread
From: Tvrtko Ursulin @ 2016-08-17 9:57 UTC (permalink / raw)
To: Chris Wilson, Intel-gfx
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array
2016-08-17 9:57 ` Tvrtko Ursulin
@ 2016-08-17 10:05 ` Chris Wilson
2016-08-17 14:36 ` Tvrtko Ursulin
0 siblings, 1 reply; 13+ messages in thread
From: Chris Wilson @ 2016-08-17 10:05 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: ✗ Ro.CI.BAT: failure for series starting with [CI,1/2] drm/i915: Add enum for hardware engine identifiers
2016-08-16 16:35 ` ✗ Ro.CI.BAT: failure for series starting with [CI,1/2] drm/i915: Add enum for hardware engine identifiers Patchwork
@ 2016-08-17 10:31 ` Tvrtko Ursulin
0 siblings, 0 replies; 13+ messages in thread
From: Tvrtko Ursulin @ 2016-08-17 10:31 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array
2016-08-17 10:05 ` Chris Wilson
@ 2016-08-17 14:36 ` Tvrtko Ursulin
2016-08-17 14:44 ` Chris Wilson
0 siblings, 1 reply; 13+ messages in thread
From: Tvrtko Ursulin @ 2016-08-17 14:36 UTC (permalink / raw)
To: Chris Wilson, Intel-gfx
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array
2016-08-17 14:36 ` Tvrtko Ursulin
@ 2016-08-17 14:44 ` Chris Wilson
2016-08-17 15:10 ` Tvrtko Ursulin
0 siblings, 1 reply; 13+ messages in thread
From: Chris Wilson @ 2016-08-17 14:44 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array
2016-08-17 14:44 ` Chris Wilson
@ 2016-08-17 15:10 ` Tvrtko Ursulin
2016-08-17 15:23 ` Chris Wilson
0 siblings, 1 reply; 13+ messages in thread
From: Tvrtko Ursulin @ 2016-08-17 15:10 UTC (permalink / raw)
To: Chris Wilson, Intel-gfx
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array
2016-08-17 15:10 ` Tvrtko Ursulin
@ 2016-08-17 15:23 ` Chris Wilson
0 siblings, 0 replies; 13+ messages in thread
From: Chris Wilson @ 2016-08-17 15:23 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-08-17 15:23 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-16 16:04 [CI 1/2] drm/i915: Add enum for hardware engine identifiers Tvrtko Ursulin
2016-08-16 16:04 ` [CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array Tvrtko Ursulin
2016-08-16 16:18 ` Chris Wilson
2016-08-17 9:34 ` Tvrtko Ursulin
2016-08-17 9:41 ` Chris Wilson
2016-08-17 9:57 ` Tvrtko Ursulin
2016-08-17 10:05 ` Chris Wilson
2016-08-17 14:36 ` Tvrtko Ursulin
2016-08-17 14:44 ` Chris Wilson
2016-08-17 15:10 ` Tvrtko Ursulin
2016-08-17 15:23 ` Chris Wilson
2016-08-16 16:35 ` ✗ Ro.CI.BAT: failure for series starting with [CI,1/2] drm/i915: Add enum for hardware engine identifiers Patchwork
2016-08-17 10:31 ` 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.