* [PATCH] drm/i915/execlists: Refactor common engine setup
@ 2016-04-28 13:47 Chris Wilson
2016-04-28 14:17 ` ✗ Fi.CI.BAT: warning for " Patchwork
` (3 more replies)
0 siblings, 4 replies; 24+ messages in thread
From: Chris Wilson @ 2016-04-28 13:47 UTC (permalink / raw)
To: intel-gfx
Move all of the constant assignments up front and into a common
function. This is primarily to ensure the backpointers are set as early
as possible for later use during initialisation.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
---
I'm not too happy about using the function parameters with lots of
similar types, very easy to muddle them up. I'm sending this all because
it places one initialiser in a common location rather than per-engine!
-Chris
---
drivers/gpu/drm/i915/intel_lrc.c | 157 +++++++++++++++++++--------------------
1 file changed, 78 insertions(+), 79 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 874c2515f9d4..a4a4f8eaf000 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1921,8 +1921,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine)
}
static void
-logical_ring_default_vfuncs(struct drm_device *dev,
- struct intel_engine_cs *engine)
+logical_ring_default_vfuncs(struct intel_engine_cs *engine)
{
/* Default vfuncs which can be overriden by each engine. */
engine->init_hw = gen8_init_common_ring;
@@ -1933,7 +1932,7 @@ logical_ring_default_vfuncs(struct drm_device *dev,
engine->emit_bb_start = gen8_emit_bb_start;
engine->get_seqno = gen8_get_seqno;
engine->set_seqno = gen8_set_seqno;
- if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) {
+ if (IS_BXT_REVID(engine->dev, 0, BXT_REVID_A1)) {
engine->irq_seqno_barrier = bxt_a_seqno_barrier;
engine->set_seqno = bxt_a_set_seqno;
}
@@ -1944,6 +1943,7 @@ logical_ring_default_irqs(struct intel_engine_cs *engine, unsigned shift)
{
engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << shift;
engine->irq_keep_mask = GT_CONTEXT_SWITCH_INTERRUPT << shift;
+ init_waitqueue_head(&engine->irq_queue);
}
static int
@@ -1964,31 +1964,28 @@ lrc_setup_hws(struct intel_engine_cs *engine,
return 0;
}
-static int
-logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
+static struct intel_engine_cs *
+logical_ring_setup(struct drm_device *dev, int id,
+ const char *name,
+ unsigned exec_id,
+ unsigned guc_id,
+ unsigned mmio_base,
+ unsigned irq)
{
struct drm_i915_private *dev_priv = to_i915(dev);
- struct intel_context *dctx = dev_priv->kernel_context;
+ struct intel_engine_cs *engine = &dev_priv->engine[id];
enum forcewake_domains fw_domains;
- int ret;
-
- /* Intentionally left blank. */
- engine->buffer = NULL;
engine->dev = dev;
- INIT_LIST_HEAD(&engine->active_list);
- INIT_LIST_HEAD(&engine->request_list);
- i915_gem_batch_pool_init(dev, &engine->batch_pool);
- init_waitqueue_head(&engine->irq_queue);
-
- INIT_LIST_HEAD(&engine->buffers);
- INIT_LIST_HEAD(&engine->execlist_queue);
- spin_lock_init(&engine->execlist_lock);
- tasklet_init(&engine->irq_tasklet,
- intel_lrc_irq_handler, (unsigned long)engine);
+ engine->name = name,
+ engine->id = id;
+ engine->exec_id = exec_id;
+ engine->guc_id = guc_id;
+ engine->mmio_base = mmio_base;
- logical_ring_init_platform_invariants(engine);
+ /* Intentionally left blank. */
+ engine->buffer = NULL;
fw_domains = intel_uncore_forcewake_for_reg(dev_priv,
RING_ELSP(engine),
@@ -2004,6 +2001,31 @@ logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
engine->fw_domains = fw_domains;
+ INIT_LIST_HEAD(&engine->active_list);
+ INIT_LIST_HEAD(&engine->request_list);
+ INIT_LIST_HEAD(&engine->buffers);
+ INIT_LIST_HEAD(&engine->execlist_queue);
+ spin_lock_init(&engine->execlist_lock);
+
+ tasklet_init(&engine->irq_tasklet,
+ intel_lrc_irq_handler, (unsigned long)engine);
+
+ logical_ring_init_platform_invariants(engine);
+ logical_ring_default_vfuncs(engine);
+ logical_ring_default_irqs(engine, irq);
+
+ intel_engine_init_hangcheck(engine);
+ i915_gem_batch_pool_init(engine->dev, &engine->batch_pool);
+
+ return engine;
+}
+
+static int
+logical_ring_init(struct intel_engine_cs *engine)
+{
+ struct intel_context *dctx = to_i915(engine->dev)->kernel_context;
+ int ret;
+
ret = i915_cmd_parser_init_ring(engine);
if (ret)
goto error;
@@ -2036,22 +2058,17 @@ error:
static int logical_render_ring_init(struct drm_device *dev)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_engine_cs *engine = &dev_priv->engine[RCS];
+ struct intel_engine_cs *engine;
int ret;
- engine->name = "render ring";
- engine->id = RCS;
- engine->exec_id = I915_EXEC_RENDER;
- engine->guc_id = GUC_RENDER_ENGINE;
- engine->mmio_base = RENDER_RING_BASE;
-
- logical_ring_default_irqs(engine, GEN8_RCS_IRQ_SHIFT);
+ engine = logical_ring_setup(dev, RCS, "render ring",
+ I915_EXEC_RENDER,
+ GUC_RENDER_ENGINE,
+ RENDER_RING_BASE,
+ GEN8_RCS_IRQ_SHIFT);
if (HAS_L3_DPF(dev))
engine->irq_keep_mask |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
- logical_ring_default_vfuncs(dev, engine);
-
/* Override some for render ring. */
if (INTEL_INFO(dev)->gen >= 9)
engine->init_hw = gen9_init_render_ring;
@@ -2062,8 +2079,6 @@ static int logical_render_ring_init(struct drm_device *dev)
engine->emit_flush = gen8_emit_flush_render;
engine->emit_request = gen8_emit_request_render;
- engine->dev = dev;
-
ret = intel_init_pipe_control(engine);
if (ret)
return ret;
@@ -2079,7 +2094,7 @@ static int logical_render_ring_init(struct drm_device *dev)
ret);
}
- ret = logical_ring_init(dev, engine);
+ ret = logical_ring_init(engine);
if (ret) {
lrc_destroy_wa_ctx_obj(engine);
}
@@ -2089,70 +2104,54 @@ static int logical_render_ring_init(struct drm_device *dev)
static int logical_bsd_ring_init(struct drm_device *dev)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_engine_cs *engine = &dev_priv->engine[VCS];
-
- engine->name = "bsd ring";
- engine->id = VCS;
- engine->exec_id = I915_EXEC_BSD;
- engine->guc_id = GUC_VIDEO_ENGINE;
- engine->mmio_base = GEN6_BSD_RING_BASE;
+ struct intel_engine_cs *engine;
- logical_ring_default_irqs(engine, GEN8_VCS1_IRQ_SHIFT);
- logical_ring_default_vfuncs(dev, engine);
+ engine = logical_ring_setup(dev, VCS, "bsd ring",
+ I915_EXEC_BSD,
+ GUC_VIDEO_ENGINE,
+ GEN6_BSD_RING_BASE,
+ GEN8_VCS1_IRQ_SHIFT);
- return logical_ring_init(dev, engine);
+ return logical_ring_init(engine);
}
static int logical_bsd2_ring_init(struct drm_device *dev)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_engine_cs *engine = &dev_priv->engine[VCS2];
-
- engine->name = "bsd2 ring";
- engine->id = VCS2;
- engine->exec_id = I915_EXEC_BSD;
- engine->guc_id = GUC_VIDEO_ENGINE2;
- engine->mmio_base = GEN8_BSD2_RING_BASE;
+ struct intel_engine_cs *engine;
- logical_ring_default_irqs(engine, GEN8_VCS2_IRQ_SHIFT);
- logical_ring_default_vfuncs(dev, engine);
+ engine = logical_ring_setup(dev, VCS2, "bsd2 ring",
+ I915_EXEC_BSD,
+ GUC_VIDEO_ENGINE2,
+ GEN8_BSD2_RING_BASE,
+ GEN8_VCS2_IRQ_SHIFT);
- return logical_ring_init(dev, engine);
+ return logical_ring_init(engine);
}
static int logical_blt_ring_init(struct drm_device *dev)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_engine_cs *engine = &dev_priv->engine[BCS];
-
- engine->name = "blitter ring";
- engine->id = BCS;
- engine->exec_id = I915_EXEC_BLT;
- engine->guc_id = GUC_BLITTER_ENGINE;
- engine->mmio_base = BLT_RING_BASE;
+ struct intel_engine_cs *engine;
- logical_ring_default_irqs(engine, GEN8_BCS_IRQ_SHIFT);
- logical_ring_default_vfuncs(dev, engine);
+ engine = logical_ring_setup(dev, BCS, "blitter ring",
+ I915_EXEC_BLT,
+ GUC_BLITTER_ENGINE,
+ BLT_RING_BASE,
+ GEN8_BCS_IRQ_SHIFT);
- return logical_ring_init(dev, engine);
+ return logical_ring_init(engine);
}
static int logical_vebox_ring_init(struct drm_device *dev)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_engine_cs *engine = &dev_priv->engine[VECS];
-
- engine->name = "video enhancement ring";
- engine->id = VECS;
- engine->exec_id = I915_EXEC_VEBOX;
- engine->guc_id = GUC_VIDEOENHANCE_ENGINE;
- engine->mmio_base = VEBOX_RING_BASE;
+ struct intel_engine_cs *engine;
- logical_ring_default_irqs(engine, GEN8_VECS_IRQ_SHIFT);
- logical_ring_default_vfuncs(dev, engine);
+ engine = logical_ring_setup(dev, VECS, "video enhancement ring",
+ I915_EXEC_VEBOX,
+ GUC_VIDEOENHANCE_ENGINE,
+ VEBOX_RING_BASE,
+ GEN8_VECS_IRQ_SHIFT);
- return logical_ring_init(dev, engine);
+ return logical_ring_init(engine);
}
/**
--
2.8.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 24+ messages in thread
* ✗ Fi.CI.BAT: warning for drm/i915/execlists: Refactor common engine setup
2016-04-28 13:47 [PATCH] drm/i915/execlists: Refactor common engine setup Chris Wilson
@ 2016-04-28 14:17 ` Patchwork
2016-04-28 15:10 ` [PATCH] " Tvrtko Ursulin
` (2 subsequent siblings)
3 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2016-04-28 14:17 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/execlists: Refactor common engine setup
URL : https://patchwork.freedesktop.org/series/6480/
State : warning
== Summary ==
Series 6480v1 drm/i915/execlists: Refactor common engine setup
http://patchwork.freedesktop.org/api/1.0/series/6480/revisions/1/mbox/
Test gem_close_race:
Subgroup basic-threads:
pass -> DMESG-WARN (ilk-hp8440p)
Test gem_exec_suspend:
Subgroup basic-s3:
pass -> DMESG-WARN (ilk-hp8440p)
Test gem_sync:
Subgroup basic-all:
dmesg-warn -> PASS (ilk-hp8440p)
Test kms_pipe_crc_basic:
Subgroup hang-read-crc-pipe-a:
pass -> DMESG-WARN (snb-dellxps)
bdw-nuci7-2 total:201 pass:189 dwarn:0 dfail:0 fail:0 skip:12
bdw-ultra total:201 pass:176 dwarn:0 dfail:0 fail:0 skip:25
bsw-nuc-2 total:200 pass:159 dwarn:0 dfail:0 fail:0 skip:41
byt-nuc total:200 pass:159 dwarn:0 dfail:0 fail:0 skip:41
hsw-brixbox total:201 pass:175 dwarn:0 dfail:0 fail:0 skip:26
hsw-gt2 total:201 pass:179 dwarn:0 dfail:0 fail:1 skip:21
ilk-hp8440p total:201 pass:138 dwarn:2 dfail:0 fail:0 skip:61
ivb-t430s total:201 pass:170 dwarn:0 dfail:0 fail:0 skip:31
skl-i7k-2 total:201 pass:174 dwarn:0 dfail:0 fail:0 skip:27
skl-nuci5 total:201 pass:190 dwarn:0 dfail:0 fail:0 skip:11
snb-dellxps total:201 pass:158 dwarn:1 dfail:0 fail:0 skip:42
snb-x220t total:201 pass:159 dwarn:0 dfail:0 fail:1 skip:41
Results at /archive/results/CI_IGT_test/Patchwork_2103/
4887074a38f11f6b5be255e54c97e7d0b2a0368b drm-intel-nightly: 2016y-04m-28d-11h-21m-01s UTC integration manifest
89ae322 drm/i915/execlists: Refactor common engine setup
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/i915/execlists: Refactor common engine setup
2016-04-28 13:47 [PATCH] drm/i915/execlists: Refactor common engine setup Chris Wilson
2016-04-28 14:17 ` ✗ Fi.CI.BAT: warning for " Patchwork
@ 2016-04-28 15:10 ` Tvrtko Ursulin
2016-04-28 15:26 ` Chris Wilson
2016-04-28 16:12 ` Dave Gordon
2016-04-28 17:35 ` [PATCH v2] " Chris Wilson
3 siblings, 1 reply; 24+ messages in thread
From: Tvrtko Ursulin @ 2016-04-28 15:10 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 28/04/16 14:47, Chris Wilson wrote:
> Move all of the constant assignments up front and into a common
> function. This is primarily to ensure the backpointers are set as early
> as possible for later use during initialisation.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
>
> I'm not too happy about using the function parameters with lots of
> similar types, very easy to muddle them up. I'm sending this all because
> it places one initialiser in a common location rather than per-engine!
> -Chris
>
> ---
> drivers/gpu/drm/i915/intel_lrc.c | 157 +++++++++++++++++++--------------------
> 1 file changed, 78 insertions(+), 79 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 874c2515f9d4..a4a4f8eaf000 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1921,8 +1921,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine)
> }
>
> static void
> -logical_ring_default_vfuncs(struct drm_device *dev,
> - struct intel_engine_cs *engine)
> +logical_ring_default_vfuncs(struct intel_engine_cs *engine)
> {
> /* Default vfuncs which can be overriden by each engine. */
> engine->init_hw = gen8_init_common_ring;
> @@ -1933,7 +1932,7 @@ logical_ring_default_vfuncs(struct drm_device *dev,
> engine->emit_bb_start = gen8_emit_bb_start;
> engine->get_seqno = gen8_get_seqno;
> engine->set_seqno = gen8_set_seqno;
> - if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) {
> + if (IS_BXT_REVID(engine->dev, 0, BXT_REVID_A1)) {
This must have been hard to swallow. ;D
> engine->irq_seqno_barrier = bxt_a_seqno_barrier;
> engine->set_seqno = bxt_a_set_seqno;
> }
> @@ -1944,6 +1943,7 @@ logical_ring_default_irqs(struct intel_engine_cs *engine, unsigned shift)
> {
> engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << shift;
> engine->irq_keep_mask = GT_CONTEXT_SWITCH_INTERRUPT << shift;
> + init_waitqueue_head(&engine->irq_queue);
> }
>
> static int
> @@ -1964,31 +1964,28 @@ lrc_setup_hws(struct intel_engine_cs *engine,
> return 0;
> }
>
> -static int
> -logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
> +static struct intel_engine_cs *
> +logical_ring_setup(struct drm_device *dev, int id,
enum intel_engine_id id
> + const char *name,
> + unsigned exec_id,
> + unsigned guc_id,
If guc_if was an enum it could even be typesafe here. Structure for
parameters is probably an overkill.Don't know.
> + unsigned mmio_base,
This is u32 elsewhere I think.
> + unsigned irq)
irq_shift would be OK.
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
> - struct intel_context *dctx = dev_priv->kernel_context;
> + struct intel_engine_cs *engine = &dev_priv->engine[id];
> enum forcewake_domains fw_domains;
> - int ret;
> -
> - /* Intentionally left blank. */
> - engine->buffer = NULL;
>
> engine->dev = dev;
Setting engine->dev was last in sequence before and now it is ahead of
lot of other things. I am not sure how important that was and if it
would cause any trouble now? Especially since using engine->dev for
intel_ring_initialized is so terribly annoying and bites unexpectedly.
> - INIT_LIST_HEAD(&engine->active_list);
> - INIT_LIST_HEAD(&engine->request_list);
> - i915_gem_batch_pool_init(dev, &engine->batch_pool);
> - init_waitqueue_head(&engine->irq_queue);
> -
> - INIT_LIST_HEAD(&engine->buffers);
> - INIT_LIST_HEAD(&engine->execlist_queue);
> - spin_lock_init(&engine->execlist_lock);
>
> - tasklet_init(&engine->irq_tasklet,
> - intel_lrc_irq_handler, (unsigned long)engine);
> + engine->name = name,
> + engine->id = id;
> + engine->exec_id = exec_id;
> + engine->guc_id = guc_id;
> + engine->mmio_base = mmio_base;
>
> - logical_ring_init_platform_invariants(engine);
> + /* Intentionally left blank. */
> + engine->buffer = NULL;
>
> fw_domains = intel_uncore_forcewake_for_reg(dev_priv,
> RING_ELSP(engine),
> @@ -2004,6 +2001,31 @@ logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
>
> engine->fw_domains = fw_domains;
>
> + INIT_LIST_HEAD(&engine->active_list);
> + INIT_LIST_HEAD(&engine->request_list);
> + INIT_LIST_HEAD(&engine->buffers);
> + INIT_LIST_HEAD(&engine->execlist_queue);
> + spin_lock_init(&engine->execlist_lock);
> +
> + tasklet_init(&engine->irq_tasklet,
> + intel_lrc_irq_handler, (unsigned long)engine);
> +
> + logical_ring_init_platform_invariants(engine);
> + logical_ring_default_vfuncs(engine);
> + logical_ring_default_irqs(engine, irq);
> +
> + intel_engine_init_hangcheck(engine);
> + i915_gem_batch_pool_init(engine->dev, &engine->batch_pool);
> +
> + return engine;
> +}
> +
> +static int
> +logical_ring_init(struct intel_engine_cs *engine)
> +{
> + struct intel_context *dctx = to_i915(engine->dev)->kernel_context;
> + int ret;
> +
> ret = i915_cmd_parser_init_ring(engine);
> if (ret)
> goto error;
> @@ -2036,22 +2058,17 @@ error:
>
> static int logical_render_ring_init(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_engine_cs *engine = &dev_priv->engine[RCS];
> + struct intel_engine_cs *engine;
> int ret;
>
> - engine->name = "render ring";
> - engine->id = RCS;
> - engine->exec_id = I915_EXEC_RENDER;
> - engine->guc_id = GUC_RENDER_ENGINE;
> - engine->mmio_base = RENDER_RING_BASE;
> -
> - logical_ring_default_irqs(engine, GEN8_RCS_IRQ_SHIFT);
> + engine = logical_ring_setup(dev, RCS, "render ring",
> + I915_EXEC_RENDER,
> + GUC_RENDER_ENGINE,
> + RENDER_RING_BASE,
> + GEN8_RCS_IRQ_SHIFT);
> if (HAS_L3_DPF(dev))
> engine->irq_keep_mask |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
>
> - logical_ring_default_vfuncs(dev, engine);
> -
> /* Override some for render ring. */
> if (INTEL_INFO(dev)->gen >= 9)
> engine->init_hw = gen9_init_render_ring;
> @@ -2062,8 +2079,6 @@ static int logical_render_ring_init(struct drm_device *dev)
> engine->emit_flush = gen8_emit_flush_render;
> engine->emit_request = gen8_emit_request_render;
>
> - engine->dev = dev;
> -
> ret = intel_init_pipe_control(engine);
> if (ret)
> return ret;
> @@ -2079,7 +2094,7 @@ static int logical_render_ring_init(struct drm_device *dev)
> ret);
> }
>
> - ret = logical_ring_init(dev, engine);
> + ret = logical_ring_init(engine);
> if (ret) {
> lrc_destroy_wa_ctx_obj(engine);
> }
> @@ -2089,70 +2104,54 @@ static int logical_render_ring_init(struct drm_device *dev)
>
> static int logical_bsd_ring_init(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_engine_cs *engine = &dev_priv->engine[VCS];
> -
> - engine->name = "bsd ring";
> - engine->id = VCS;
> - engine->exec_id = I915_EXEC_BSD;
> - engine->guc_id = GUC_VIDEO_ENGINE;
> - engine->mmio_base = GEN6_BSD_RING_BASE;
> + struct intel_engine_cs *engine;
>
> - logical_ring_default_irqs(engine, GEN8_VCS1_IRQ_SHIFT);
> - logical_ring_default_vfuncs(dev, engine);
> + engine = logical_ring_setup(dev, VCS, "bsd ring",
> + I915_EXEC_BSD,
> + GUC_VIDEO_ENGINE,
> + GEN6_BSD_RING_BASE,
> + GEN8_VCS1_IRQ_SHIFT);
>
> - return logical_ring_init(dev, engine);
> + return logical_ring_init(engine);
> }
>
> static int logical_bsd2_ring_init(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_engine_cs *engine = &dev_priv->engine[VCS2];
> -
> - engine->name = "bsd2 ring";
> - engine->id = VCS2;
> - engine->exec_id = I915_EXEC_BSD;
> - engine->guc_id = GUC_VIDEO_ENGINE2;
> - engine->mmio_base = GEN8_BSD2_RING_BASE;
> + struct intel_engine_cs *engine;
>
> - logical_ring_default_irqs(engine, GEN8_VCS2_IRQ_SHIFT);
> - logical_ring_default_vfuncs(dev, engine);
> + engine = logical_ring_setup(dev, VCS2, "bsd2 ring",
> + I915_EXEC_BSD,
> + GUC_VIDEO_ENGINE2,
> + GEN8_BSD2_RING_BASE,
> + GEN8_VCS2_IRQ_SHIFT);
>
> - return logical_ring_init(dev, engine);
> + return logical_ring_init(engine);
> }
>
> static int logical_blt_ring_init(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_engine_cs *engine = &dev_priv->engine[BCS];
> -
> - engine->name = "blitter ring";
> - engine->id = BCS;
> - engine->exec_id = I915_EXEC_BLT;
> - engine->guc_id = GUC_BLITTER_ENGINE;
> - engine->mmio_base = BLT_RING_BASE;
> + struct intel_engine_cs *engine;
>
> - logical_ring_default_irqs(engine, GEN8_BCS_IRQ_SHIFT);
> - logical_ring_default_vfuncs(dev, engine);
> + engine = logical_ring_setup(dev, BCS, "blitter ring",
> + I915_EXEC_BLT,
> + GUC_BLITTER_ENGINE,
> + BLT_RING_BASE,
> + GEN8_BCS_IRQ_SHIFT);
>
> - return logical_ring_init(dev, engine);
> + return logical_ring_init(engine);
> }
>
> static int logical_vebox_ring_init(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_engine_cs *engine = &dev_priv->engine[VECS];
> -
> - engine->name = "video enhancement ring";
> - engine->id = VECS;
> - engine->exec_id = I915_EXEC_VEBOX;
> - engine->guc_id = GUC_VIDEOENHANCE_ENGINE;
> - engine->mmio_base = VEBOX_RING_BASE;
> + struct intel_engine_cs *engine;
>
> - logical_ring_default_irqs(engine, GEN8_VECS_IRQ_SHIFT);
> - logical_ring_default_vfuncs(dev, engine);
> + engine = logical_ring_setup(dev, VECS, "video enhancement ring",
> + I915_EXEC_VEBOX,
> + GUC_VIDEOENHANCE_ENGINE,
> + VEBOX_RING_BASE,
> + GEN8_VECS_IRQ_SHIFT);
>
> - return logical_ring_init(dev, engine);
> + return logical_ring_init(engine);
> }
>
> /**
>
I still not see a point, maybe it just need to be explained to me. :)
But anyway will not block it after the above points are addressed and
discussed.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/i915/execlists: Refactor common engine setup
2016-04-28 15:10 ` [PATCH] " Tvrtko Ursulin
@ 2016-04-28 15:26 ` Chris Wilson
0 siblings, 0 replies; 24+ messages in thread
From: Chris Wilson @ 2016-04-28 15:26 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
On Thu, Apr 28, 2016 at 04:10:54PM +0100, Tvrtko Ursulin wrote:
> >- /* Intentionally left blank. */
> >- engine->buffer = NULL;
> >
> > engine->dev = dev;
>
> Setting engine->dev was last in sequence before and now it is ahead
> of lot of other things. I am not sure how important that was and if
> it would cause any trouble now? Especially since using engine->dev
> for intel_ring_initialized is so terribly annoying and bites
> unexpectedly.
Yeah, setting it early allows us to us it earlier! Some of the early
initialisation would like to know engine->dev.
> I still not see a point, maybe it just need to be explained to me.
> :) But anyway will not block it after the above points are addressed
> and discussed.
There wasn't much. All this was so that I could add a couple of lines in
the one place rather than in each caller. I did think it looked a bit
neater and matched the init func well.
-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] 24+ messages in thread
* Re: [PATCH] drm/i915/execlists: Refactor common engine setup
2016-04-28 13:47 [PATCH] drm/i915/execlists: Refactor common engine setup Chris Wilson
2016-04-28 14:17 ` ✗ Fi.CI.BAT: warning for " Patchwork
2016-04-28 15:10 ` [PATCH] " Tvrtko Ursulin
@ 2016-04-28 16:12 ` Dave Gordon
2016-04-28 17:04 ` Chris Wilson
2016-04-28 17:35 ` [PATCH v2] " Chris Wilson
3 siblings, 1 reply; 24+ messages in thread
From: Dave Gordon @ 2016-04-28 16:12 UTC (permalink / raw)
To: intel-gfx
On 28/04/16 14:47, Chris Wilson wrote:
> Move all of the constant assignments up front and into a common
> function. This is primarily to ensure the backpointers are set as early
> as possible for later use during initialisation.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
>
> I'm not too happy about using the function parameters with lots of
> similar types, very easy to muddle them up. I'm sending this all because
> it places one initialiser in a common location rather than per-engine!
> -Chris
>
> ---
> drivers/gpu/drm/i915/intel_lrc.c | 157 +++++++++++++++++++--------------------
> 1 file changed, 78 insertions(+), 79 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 874c2515f9d4..a4a4f8eaf000 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1921,8 +1921,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine)
> }
>
> static void
> -logical_ring_default_vfuncs(struct drm_device *dev,
> - struct intel_engine_cs *engine)
> +logical_ring_default_vfuncs(struct intel_engine_cs *engine)
> {
> /* Default vfuncs which can be overriden by each engine. */
> engine->init_hw = gen8_init_common_ring;
> @@ -1933,7 +1932,7 @@ logical_ring_default_vfuncs(struct drm_device *dev,
> engine->emit_bb_start = gen8_emit_bb_start;
> engine->get_seqno = gen8_get_seqno;
> engine->set_seqno = gen8_set_seqno;
> - if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) {
> + if (IS_BXT_REVID(engine->dev, 0, BXT_REVID_A1)) {
> engine->irq_seqno_barrier = bxt_a_seqno_barrier;
> engine->set_seqno = bxt_a_set_seqno;
> }
> @@ -1944,6 +1943,7 @@ logical_ring_default_irqs(struct intel_engine_cs *engine, unsigned shift)
> {
> engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << shift;
> engine->irq_keep_mask = GT_CONTEXT_SWITCH_INTERRUPT << shift;
> + init_waitqueue_head(&engine->irq_queue);
> }
>
> static int
> @@ -1964,31 +1964,28 @@ lrc_setup_hws(struct intel_engine_cs *engine,
> return 0;
> }
>
> -static int
> -logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
> +static struct intel_engine_cs *
> +logical_ring_setup(struct drm_device *dev, int id,
> + const char *name,
> + unsigned exec_id,
> + unsigned guc_id,
> + unsigned mmio_base,
> + unsigned irq)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
> - struct intel_context *dctx = dev_priv->kernel_context;
> + struct intel_engine_cs *engine = &dev_priv->engine[id];
> enum forcewake_domains fw_domains;
> - int ret;
> -
> - /* Intentionally left blank. */
> - engine->buffer = NULL;
>
> engine->dev = dev;
Eeep! Isn't this the "engine initialised and ready for use" flag?
> - INIT_LIST_HEAD(&engine->active_list);
> - INIT_LIST_HEAD(&engine->request_list);
> - i915_gem_batch_pool_init(dev, &engine->batch_pool);
> - init_waitqueue_head(&engine->irq_queue);
> -
> - INIT_LIST_HEAD(&engine->buffers);
> - INIT_LIST_HEAD(&engine->execlist_queue);
> - spin_lock_init(&engine->execlist_lock);
>
> - tasklet_init(&engine->irq_tasklet,
> - intel_lrc_irq_handler, (unsigned long)engine);
> + engine->name = name,
> + engine->id = id;
> + engine->exec_id = exec_id;
> + engine->guc_id = guc_id;
> + engine->mmio_base = mmio_base;
>
> - logical_ring_init_platform_invariants(engine);
> + /* Intentionally left blank. */
> + engine->buffer = NULL;
>
> fw_domains = intel_uncore_forcewake_for_reg(dev_priv,
> RING_ELSP(engine),
> @@ -2004,6 +2001,31 @@ logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
>
> engine->fw_domains = fw_domains;
>
> + INIT_LIST_HEAD(&engine->active_list);
> + INIT_LIST_HEAD(&engine->request_list);
> + INIT_LIST_HEAD(&engine->buffers);
> + INIT_LIST_HEAD(&engine->execlist_queue);
> + spin_lock_init(&engine->execlist_lock);
> +
> + tasklet_init(&engine->irq_tasklet,
> + intel_lrc_irq_handler, (unsigned long)engine);
> +
> + logical_ring_init_platform_invariants(engine);
> + logical_ring_default_vfuncs(engine);
> + logical_ring_default_irqs(engine, irq);
> +
> + intel_engine_init_hangcheck(engine);
> + i915_gem_batch_pool_init(engine->dev, &engine->batch_pool);
> +
> + return engine;
> +}
> +
> +static int
> +logical_ring_init(struct intel_engine_cs *engine)
> +{
> + struct intel_context *dctx = to_i915(engine->dev)->kernel_context;
> + int ret;
> +
> ret = i915_cmd_parser_init_ring(engine);
> if (ret)
> goto error;
> @@ -2036,22 +2058,17 @@ error:
>
> static int logical_render_ring_init(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_engine_cs *engine = &dev_priv->engine[RCS];
> + struct intel_engine_cs *engine;
> int ret;
>
> - engine->name = "render ring";
> - engine->id = RCS;
> - engine->exec_id = I915_EXEC_RENDER;
> - engine->guc_id = GUC_RENDER_ENGINE;
> - engine->mmio_base = RENDER_RING_BASE;
> -
> - logical_ring_default_irqs(engine, GEN8_RCS_IRQ_SHIFT);
> + engine = logical_ring_setup(dev, RCS, "render ring",
> + I915_EXEC_RENDER,
> + GUC_RENDER_ENGINE,
> + RENDER_RING_BASE,
> + GEN8_RCS_IRQ_SHIFT);
We could turn this collection of arguments into a struct, and create a
static readonly table of them. That would eliminate the risk of mixing
them up!
const struct engine_info engines[] =
{ /* name ID execflag GuC ID MMIO
base IRQ shift */
{ "render", RCS, I915_EXEC_RENDER, GUC_RENDER_ENGINE,
RENDER_RING_BASE, GEN8_RCS_IRQ_SHIFT },
{ "bsd", BSD, I915_EXEC_BSD, GUC_VIDEO_ENGINE,
GEN6_BSD_RING_BASE, GEN8_VCS1_IRQ_SHIFT),
...
};
And since all the correspondences would be defined in one table, it
would be much easier to check than hunting around for multiple function
calls.
.Dave.
> if (HAS_L3_DPF(dev))
> engine->irq_keep_mask |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
>
> - logical_ring_default_vfuncs(dev, engine);
> -
> /* Override some for render ring. */
> if (INTEL_INFO(dev)->gen >= 9)
> engine->init_hw = gen9_init_render_ring;
> @@ -2062,8 +2079,6 @@ static int logical_render_ring_init(struct drm_device *dev)
> engine->emit_flush = gen8_emit_flush_render;
> engine->emit_request = gen8_emit_request_render;
>
> - engine->dev = dev;
> -
> ret = intel_init_pipe_control(engine);
> if (ret)
> return ret;
> @@ -2079,7 +2094,7 @@ static int logical_render_ring_init(struct drm_device *dev)
> ret);
> }
>
> - ret = logical_ring_init(dev, engine);
> + ret = logical_ring_init(engine);
> if (ret) {
> lrc_destroy_wa_ctx_obj(engine);
> }
> @@ -2089,70 +2104,54 @@ static int logical_render_ring_init(struct drm_device *dev)
>
> static int logical_bsd_ring_init(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_engine_cs *engine = &dev_priv->engine[VCS];
> -
> - engine->name = "bsd ring";
> - engine->id = VCS;
> - engine->exec_id = I915_EXEC_BSD;
> - engine->guc_id = GUC_VIDEO_ENGINE;
> - engine->mmio_base = GEN6_BSD_RING_BASE;
> + struct intel_engine_cs *engine;
>
> - logical_ring_default_irqs(engine, GEN8_VCS1_IRQ_SHIFT);
> - logical_ring_default_vfuncs(dev, engine);
> + engine = logical_ring_setup(dev, VCS, "bsd ring",
> + I915_EXEC_BSD,
> + GUC_VIDEO_ENGINE,
> + GEN6_BSD_RING_BASE,
> + GEN8_VCS1_IRQ_SHIFT);
>
> - return logical_ring_init(dev, engine);
> + return logical_ring_init(engine);
> }
>
> static int logical_bsd2_ring_init(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_engine_cs *engine = &dev_priv->engine[VCS2];
> -
> - engine->name = "bsd2 ring";
> - engine->id = VCS2;
> - engine->exec_id = I915_EXEC_BSD;
> - engine->guc_id = GUC_VIDEO_ENGINE2;
> - engine->mmio_base = GEN8_BSD2_RING_BASE;
> + struct intel_engine_cs *engine;
>
> - logical_ring_default_irqs(engine, GEN8_VCS2_IRQ_SHIFT);
> - logical_ring_default_vfuncs(dev, engine);
> + engine = logical_ring_setup(dev, VCS2, "bsd2 ring",
> + I915_EXEC_BSD,
> + GUC_VIDEO_ENGINE2,
> + GEN8_BSD2_RING_BASE,
> + GEN8_VCS2_IRQ_SHIFT);
>
> - return logical_ring_init(dev, engine);
> + return logical_ring_init(engine);
> }
>
> static int logical_blt_ring_init(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_engine_cs *engine = &dev_priv->engine[BCS];
> -
> - engine->name = "blitter ring";
> - engine->id = BCS;
> - engine->exec_id = I915_EXEC_BLT;
> - engine->guc_id = GUC_BLITTER_ENGINE;
> - engine->mmio_base = BLT_RING_BASE;
> + struct intel_engine_cs *engine;
>
> - logical_ring_default_irqs(engine, GEN8_BCS_IRQ_SHIFT);
> - logical_ring_default_vfuncs(dev, engine);
> + engine = logical_ring_setup(dev, BCS, "blitter ring",
> + I915_EXEC_BLT,
> + GUC_BLITTER_ENGINE,
> + BLT_RING_BASE,
> + GEN8_BCS_IRQ_SHIFT);
>
> - return logical_ring_init(dev, engine);
> + return logical_ring_init(engine);
> }
>
> static int logical_vebox_ring_init(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_engine_cs *engine = &dev_priv->engine[VECS];
> -
> - engine->name = "video enhancement ring";
> - engine->id = VECS;
> - engine->exec_id = I915_EXEC_VEBOX;
> - engine->guc_id = GUC_VIDEOENHANCE_ENGINE;
> - engine->mmio_base = VEBOX_RING_BASE;
> + struct intel_engine_cs *engine;
>
> - logical_ring_default_irqs(engine, GEN8_VECS_IRQ_SHIFT);
> - logical_ring_default_vfuncs(dev, engine);
> + engine = logical_ring_setup(dev, VECS, "video enhancement ring",
> + I915_EXEC_VEBOX,
> + GUC_VIDEOENHANCE_ENGINE,
> + VEBOX_RING_BASE,
> + GEN8_VECS_IRQ_SHIFT);
>
> - return logical_ring_init(dev, engine);
> + return logical_ring_init(engine);
> }
>
> /**
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] drm/i915/execlists: Refactor common engine setup
2016-04-28 16:12 ` Dave Gordon
@ 2016-04-28 17:04 ` Chris Wilson
0 siblings, 0 replies; 24+ messages in thread
From: Chris Wilson @ 2016-04-28 17:04 UTC (permalink / raw)
To: Dave Gordon; +Cc: intel-gfx
On Thu, Apr 28, 2016 at 05:12:28PM +0100, Dave Gordon wrote:
> On 28/04/16 14:47, Chris Wilson wrote:
> >- /* Intentionally left blank. */
> >- engine->buffer = NULL;
> >
> > engine->dev = dev;
>
> Eeep! Isn't this the "engine initialised and ready for use" flag?
The same that we set to NULL if the initialisation subsequently fails.
-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] 24+ messages in thread
* [PATCH v2] drm/i915/execlists: Refactor common engine setup
2016-04-28 13:47 [PATCH] drm/i915/execlists: Refactor common engine setup Chris Wilson
` (2 preceding siblings ...)
2016-04-28 16:12 ` Dave Gordon
@ 2016-04-28 17:35 ` Chris Wilson
2016-04-29 9:04 ` Tvrtko Ursulin
3 siblings, 1 reply; 24+ messages in thread
From: Chris Wilson @ 2016-04-28 17:35 UTC (permalink / raw)
To: intel-gfx
Move all of the constant assignments up front and into a common
function. This is primarily to ensure the backpointers are set as early
as possible for later use during initialisation.
v2: Use a constant struct so that all the similar values are set
together.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Dave Gordon <david.s.gordon@intel.com>
---
drivers/gpu/drm/i915/intel_lrc.c | 176 +++++++++++++++++++++------------------
1 file changed, 93 insertions(+), 83 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 874c2515f9d4..2e0eaa9fa240 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1921,8 +1921,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine)
}
static void
-logical_ring_default_vfuncs(struct drm_device *dev,
- struct intel_engine_cs *engine)
+logical_ring_default_vfuncs(struct intel_engine_cs *engine)
{
/* Default vfuncs which can be overriden by each engine. */
engine->init_hw = gen8_init_common_ring;
@@ -1933,7 +1932,7 @@ logical_ring_default_vfuncs(struct drm_device *dev,
engine->emit_bb_start = gen8_emit_bb_start;
engine->get_seqno = gen8_get_seqno;
engine->set_seqno = gen8_set_seqno;
- if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) {
+ if (IS_BXT_REVID(engine->dev, 0, BXT_REVID_A1)) {
engine->irq_seqno_barrier = bxt_a_seqno_barrier;
engine->set_seqno = bxt_a_set_seqno;
}
@@ -1944,6 +1943,7 @@ logical_ring_default_irqs(struct intel_engine_cs *engine, unsigned shift)
{
engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << shift;
engine->irq_keep_mask = GT_CONTEXT_SWITCH_INTERRUPT << shift;
+ init_waitqueue_head(&engine->irq_queue);
}
static int
@@ -1964,31 +1964,68 @@ lrc_setup_hws(struct intel_engine_cs *engine,
return 0;
}
-static int
-logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
+static const struct logical_ring_info {
+ const char *name;
+ unsigned exec_id;
+ unsigned guc_id;
+ u32 mmio_base;
+ unsigned irq_shift;
+} logical_rings[] = {
+ [RCS] = {
+ .name = "render ring",
+ .exec_id = I915_EXEC_RENDER,
+ .guc_id = GUC_RENDER_ENGINE,
+ .mmio_base = RENDER_RING_BASE,
+ .irq_shift = GEN8_RCS_IRQ_SHIFT,
+ },
+ [BCS] = {
+ .name = "blitter ring",
+ .exec_id = I915_EXEC_BLT,
+ .guc_id = GUC_BLITTER_ENGINE,
+ .mmio_base = BLT_RING_BASE,
+ .irq_shift = GEN8_BCS_IRQ_SHIFT,
+ },
+ [VCS] = {
+ .name = "bsd ring",
+ .exec_id = I915_EXEC_BSD,
+ .guc_id = GUC_VIDEO_ENGINE,
+ .mmio_base = GEN6_BSD_RING_BASE,
+ .irq_shift = GEN8_VCS1_IRQ_SHIFT,
+ },
+ [VCS2] = {
+ .name = "bsd2 ring",
+ .exec_id = I915_EXEC_BSD,
+ .guc_id = GUC_VIDEO_ENGINE2,
+ .mmio_base = GEN8_BSD2_RING_BASE,
+ .irq_shift = GEN8_VCS2_IRQ_SHIFT,
+ },
+ [VECS] = {
+ .name = "video enhancement ring",
+ .exec_id = I915_EXEC_VEBOX,
+ .guc_id = GUC_VIDEOENHANCE_ENGINE,
+ .mmio_base = VEBOX_RING_BASE,
+ .irq_shift = GEN8_VECS_IRQ_SHIFT,
+ },
+};
+
+static struct intel_engine_cs *
+logical_ring_setup(struct drm_device *dev, enum intel_engine_id id)
{
+ const struct logical_ring_info *info = &logical_rings[id];
struct drm_i915_private *dev_priv = to_i915(dev);
- struct intel_context *dctx = dev_priv->kernel_context;
+ struct intel_engine_cs *engine = &dev_priv->engine[id];
enum forcewake_domains fw_domains;
- int ret;
-
- /* Intentionally left blank. */
- engine->buffer = NULL;
engine->dev = dev;
- INIT_LIST_HEAD(&engine->active_list);
- INIT_LIST_HEAD(&engine->request_list);
- i915_gem_batch_pool_init(dev, &engine->batch_pool);
- init_waitqueue_head(&engine->irq_queue);
- INIT_LIST_HEAD(&engine->buffers);
- INIT_LIST_HEAD(&engine->execlist_queue);
- spin_lock_init(&engine->execlist_lock);
+ engine->id = id;
+ engine->name = info->name;
+ engine->exec_id = info->exec_id;
+ engine->guc_id = info->guc_id;
+ engine->mmio_base = info->mmio_base;
- tasklet_init(&engine->irq_tasklet,
- intel_lrc_irq_handler, (unsigned long)engine);
-
- logical_ring_init_platform_invariants(engine);
+ /* Intentionally left blank. */
+ engine->buffer = NULL;
fw_domains = intel_uncore_forcewake_for_reg(dev_priv,
RING_ELSP(engine),
@@ -2004,6 +2041,31 @@ logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
engine->fw_domains = fw_domains;
+ INIT_LIST_HEAD(&engine->active_list);
+ INIT_LIST_HEAD(&engine->request_list);
+ INIT_LIST_HEAD(&engine->buffers);
+ INIT_LIST_HEAD(&engine->execlist_queue);
+ spin_lock_init(&engine->execlist_lock);
+
+ tasklet_init(&engine->irq_tasklet,
+ intel_lrc_irq_handler, (unsigned long)engine);
+
+ logical_ring_init_platform_invariants(engine);
+ logical_ring_default_vfuncs(engine);
+ logical_ring_default_irqs(engine, info->irq_shift);
+
+ intel_engine_init_hangcheck(engine);
+ i915_gem_batch_pool_init(engine->dev, &engine->batch_pool);
+
+ return engine;
+}
+
+static int
+logical_ring_init(struct intel_engine_cs *engine)
+{
+ struct intel_context *dctx = to_i915(engine->dev)->kernel_context;
+ int ret;
+
ret = i915_cmd_parser_init_ring(engine);
if (ret)
goto error;
@@ -2036,22 +2098,12 @@ error:
static int logical_render_ring_init(struct drm_device *dev)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_engine_cs *engine = &dev_priv->engine[RCS];
+ struct intel_engine_cs *engine = logical_ring_setup(dev, RCS);
int ret;
- engine->name = "render ring";
- engine->id = RCS;
- engine->exec_id = I915_EXEC_RENDER;
- engine->guc_id = GUC_RENDER_ENGINE;
- engine->mmio_base = RENDER_RING_BASE;
-
- logical_ring_default_irqs(engine, GEN8_RCS_IRQ_SHIFT);
if (HAS_L3_DPF(dev))
engine->irq_keep_mask |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
- logical_ring_default_vfuncs(dev, engine);
-
/* Override some for render ring. */
if (INTEL_INFO(dev)->gen >= 9)
engine->init_hw = gen9_init_render_ring;
@@ -2062,8 +2114,6 @@ static int logical_render_ring_init(struct drm_device *dev)
engine->emit_flush = gen8_emit_flush_render;
engine->emit_request = gen8_emit_request_render;
- engine->dev = dev;
-
ret = intel_init_pipe_control(engine);
if (ret)
return ret;
@@ -2079,7 +2129,7 @@ static int logical_render_ring_init(struct drm_device *dev)
ret);
}
- ret = logical_ring_init(dev, engine);
+ ret = logical_ring_init(engine);
if (ret) {
lrc_destroy_wa_ctx_obj(engine);
}
@@ -2089,70 +2139,30 @@ static int logical_render_ring_init(struct drm_device *dev)
static int logical_bsd_ring_init(struct drm_device *dev)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_engine_cs *engine = &dev_priv->engine[VCS];
-
- engine->name = "bsd ring";
- engine->id = VCS;
- engine->exec_id = I915_EXEC_BSD;
- engine->guc_id = GUC_VIDEO_ENGINE;
- engine->mmio_base = GEN6_BSD_RING_BASE;
+ struct intel_engine_cs *engine = logical_ring_setup(dev, VCS);
- logical_ring_default_irqs(engine, GEN8_VCS1_IRQ_SHIFT);
- logical_ring_default_vfuncs(dev, engine);
-
- return logical_ring_init(dev, engine);
+ return logical_ring_init(engine);
}
static int logical_bsd2_ring_init(struct drm_device *dev)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_engine_cs *engine = &dev_priv->engine[VCS2];
-
- engine->name = "bsd2 ring";
- engine->id = VCS2;
- engine->exec_id = I915_EXEC_BSD;
- engine->guc_id = GUC_VIDEO_ENGINE2;
- engine->mmio_base = GEN8_BSD2_RING_BASE;
+ struct intel_engine_cs *engine = logical_ring_setup(dev, VCS2);
- logical_ring_default_irqs(engine, GEN8_VCS2_IRQ_SHIFT);
- logical_ring_default_vfuncs(dev, engine);
-
- return logical_ring_init(dev, engine);
+ return logical_ring_init(engine);
}
static int logical_blt_ring_init(struct drm_device *dev)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_engine_cs *engine = &dev_priv->engine[BCS];
-
- engine->name = "blitter ring";
- engine->id = BCS;
- engine->exec_id = I915_EXEC_BLT;
- engine->guc_id = GUC_BLITTER_ENGINE;
- engine->mmio_base = BLT_RING_BASE;
-
- logical_ring_default_irqs(engine, GEN8_BCS_IRQ_SHIFT);
- logical_ring_default_vfuncs(dev, engine);
+ struct intel_engine_cs *engine = logical_ring_setup(dev, BCS);
- return logical_ring_init(dev, engine);
+ return logical_ring_init(engine);
}
static int logical_vebox_ring_init(struct drm_device *dev)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_engine_cs *engine = &dev_priv->engine[VECS];
-
- engine->name = "video enhancement ring";
- engine->id = VECS;
- engine->exec_id = I915_EXEC_VEBOX;
- engine->guc_id = GUC_VIDEOENHANCE_ENGINE;
- engine->mmio_base = VEBOX_RING_BASE;
-
- logical_ring_default_irqs(engine, GEN8_VECS_IRQ_SHIFT);
- logical_ring_default_vfuncs(dev, engine);
+ struct intel_engine_cs *engine = logical_ring_setup(dev, VECS);
- return logical_ring_init(dev, engine);
+ return logical_ring_init(engine);
}
/**
--
2.8.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2] drm/i915/execlists: Refactor common engine setup
2016-04-28 17:35 ` [PATCH v2] " Chris Wilson
@ 2016-04-29 9:04 ` Tvrtko Ursulin
2016-04-29 9:15 ` Chris Wilson
0 siblings, 1 reply; 24+ messages in thread
From: Tvrtko Ursulin @ 2016-04-29 9:04 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
On 28/04/16 18:35, Chris Wilson wrote:
> Move all of the constant assignments up front and into a common
> function. This is primarily to ensure the backpointers are set as early
> as possible for later use during initialisation.
>
> v2: Use a constant struct so that all the similar values are set
> together.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Dave Gordon <david.s.gordon@intel.com>
> ---
> drivers/gpu/drm/i915/intel_lrc.c | 176 +++++++++++++++++++++------------------
> 1 file changed, 93 insertions(+), 83 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 874c2515f9d4..2e0eaa9fa240 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1921,8 +1921,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine)
> }
>
> static void
> -logical_ring_default_vfuncs(struct drm_device *dev,
> - struct intel_engine_cs *engine)
> +logical_ring_default_vfuncs(struct intel_engine_cs *engine)
> {
> /* Default vfuncs which can be overriden by each engine. */
> engine->init_hw = gen8_init_common_ring;
> @@ -1933,7 +1932,7 @@ logical_ring_default_vfuncs(struct drm_device *dev,
> engine->emit_bb_start = gen8_emit_bb_start;
> engine->get_seqno = gen8_get_seqno;
> engine->set_seqno = gen8_set_seqno;
> - if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) {
> + if (IS_BXT_REVID(engine->dev, 0, BXT_REVID_A1)) {
> engine->irq_seqno_barrier = bxt_a_seqno_barrier;
> engine->set_seqno = bxt_a_set_seqno;
> }
> @@ -1944,6 +1943,7 @@ logical_ring_default_irqs(struct intel_engine_cs *engine, unsigned shift)
> {
> engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << shift;
> engine->irq_keep_mask = GT_CONTEXT_SWITCH_INTERRUPT << shift;
> + init_waitqueue_head(&engine->irq_queue);
> }
>
> static int
> @@ -1964,31 +1964,68 @@ lrc_setup_hws(struct intel_engine_cs *engine,
> return 0;
> }
>
> -static int
> -logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
> +static const struct logical_ring_info {
> + const char *name;
> + unsigned exec_id;
> + unsigned guc_id;
> + u32 mmio_base;
> + unsigned irq_shift;
> +} logical_rings[] = {
> + [RCS] = {
> + .name = "render ring",
> + .exec_id = I915_EXEC_RENDER,
> + .guc_id = GUC_RENDER_ENGINE,
> + .mmio_base = RENDER_RING_BASE,
> + .irq_shift = GEN8_RCS_IRQ_SHIFT,
> + },
> + [BCS] = {
> + .name = "blitter ring",
> + .exec_id = I915_EXEC_BLT,
> + .guc_id = GUC_BLITTER_ENGINE,
> + .mmio_base = BLT_RING_BASE,
> + .irq_shift = GEN8_BCS_IRQ_SHIFT,
> + },
> + [VCS] = {
> + .name = "bsd ring",
> + .exec_id = I915_EXEC_BSD,
> + .guc_id = GUC_VIDEO_ENGINE,
> + .mmio_base = GEN6_BSD_RING_BASE,
> + .irq_shift = GEN8_VCS1_IRQ_SHIFT,
> + },
> + [VCS2] = {
> + .name = "bsd2 ring",
> + .exec_id = I915_EXEC_BSD,
> + .guc_id = GUC_VIDEO_ENGINE2,
> + .mmio_base = GEN8_BSD2_RING_BASE,
> + .irq_shift = GEN8_VCS2_IRQ_SHIFT,
> + },
> + [VECS] = {
> + .name = "video enhancement ring",
> + .exec_id = I915_EXEC_VEBOX,
> + .guc_id = GUC_VIDEOENHANCE_ENGINE,
> + .mmio_base = VEBOX_RING_BASE,
> + .irq_shift = GEN8_VECS_IRQ_SHIFT,
> + },
> +};
> +
> +static struct intel_engine_cs *
> +logical_ring_setup(struct drm_device *dev, enum intel_engine_id id)
> {
Would dev_priv be better? Just to gradually move towards the correct
state of things.
> + const struct logical_ring_info *info = &logical_rings[id];
> struct drm_i915_private *dev_priv = to_i915(dev);
> - struct intel_context *dctx = dev_priv->kernel_context;
> + struct intel_engine_cs *engine = &dev_priv->engine[id];
> enum forcewake_domains fw_domains;
> - int ret;
> -
> - /* Intentionally left blank. */
> - engine->buffer = NULL;
>
> engine->dev = dev;
Looking at usages of intel_engine_initialized... one potential danger
scenario would be interrupt noise during driver load end in notify ring
and explodes. Sounds very unlikely but theoretically it is a regression
compared to where engine->dev initialization was before.
We should really move away from engine->dev for this and just add an
explicit flag.
> - INIT_LIST_HEAD(&engine->active_list);
> - INIT_LIST_HEAD(&engine->request_list);
> - i915_gem_batch_pool_init(dev, &engine->batch_pool);
> - init_waitqueue_head(&engine->irq_queue);
>
> - INIT_LIST_HEAD(&engine->buffers);
> - INIT_LIST_HEAD(&engine->execlist_queue);
> - spin_lock_init(&engine->execlist_lock);
> + engine->id = id;
> + engine->name = info->name;
> + engine->exec_id = info->exec_id;
> + engine->guc_id = info->guc_id;
> + engine->mmio_base = info->mmio_base;
>
> - tasklet_init(&engine->irq_tasklet,
> - intel_lrc_irq_handler, (unsigned long)engine);
> -
> - logical_ring_init_platform_invariants(engine);
> + /* Intentionally left blank. */
> + engine->buffer = NULL;
>
> fw_domains = intel_uncore_forcewake_for_reg(dev_priv,
> RING_ELSP(engine),
> @@ -2004,6 +2041,31 @@ logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
>
> engine->fw_domains = fw_domains;
>
> + INIT_LIST_HEAD(&engine->active_list);
> + INIT_LIST_HEAD(&engine->request_list);
> + INIT_LIST_HEAD(&engine->buffers);
> + INIT_LIST_HEAD(&engine->execlist_queue);
> + spin_lock_init(&engine->execlist_lock);
> +
> + tasklet_init(&engine->irq_tasklet,
> + intel_lrc_irq_handler, (unsigned long)engine);
> +
> + logical_ring_init_platform_invariants(engine);
> + logical_ring_default_vfuncs(engine);
> + logical_ring_default_irqs(engine, info->irq_shift);
> +
> + intel_engine_init_hangcheck(engine);
> + i915_gem_batch_pool_init(engine->dev, &engine->batch_pool);
> +
> + return engine;
> +}
> +
> +static int
> +logical_ring_init(struct intel_engine_cs *engine)
> +{
> + struct intel_context *dctx = to_i915(engine->dev)->kernel_context;
> + int ret;
> +
> ret = i915_cmd_parser_init_ring(engine);
> if (ret)
> goto error;
> @@ -2036,22 +2098,12 @@ error:
>
> static int logical_render_ring_init(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_engine_cs *engine = &dev_priv->engine[RCS];
> + struct intel_engine_cs *engine = logical_ring_setup(dev, RCS);
> int ret;
>
> - engine->name = "render ring";
> - engine->id = RCS;
> - engine->exec_id = I915_EXEC_RENDER;
> - engine->guc_id = GUC_RENDER_ENGINE;
> - engine->mmio_base = RENDER_RING_BASE;
> -
> - logical_ring_default_irqs(engine, GEN8_RCS_IRQ_SHIFT);
> if (HAS_L3_DPF(dev))
> engine->irq_keep_mask |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
>
> - logical_ring_default_vfuncs(dev, engine);
> -
> /* Override some for render ring. */
> if (INTEL_INFO(dev)->gen >= 9)
> engine->init_hw = gen9_init_render_ring;
> @@ -2062,8 +2114,6 @@ static int logical_render_ring_init(struct drm_device *dev)
> engine->emit_flush = gen8_emit_flush_render;
> engine->emit_request = gen8_emit_request_render;
>
> - engine->dev = dev;
> -
> ret = intel_init_pipe_control(engine);
> if (ret)
> return ret;
> @@ -2079,7 +2129,7 @@ static int logical_render_ring_init(struct drm_device *dev)
> ret);
> }
>
> - ret = logical_ring_init(dev, engine);
> + ret = logical_ring_init(engine);
> if (ret) {
> lrc_destroy_wa_ctx_obj(engine);
> }
> @@ -2089,70 +2139,30 @@ static int logical_render_ring_init(struct drm_device *dev)
>
> static int logical_bsd_ring_init(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_engine_cs *engine = &dev_priv->engine[VCS];
> -
> - engine->name = "bsd ring";
> - engine->id = VCS;
> - engine->exec_id = I915_EXEC_BSD;
> - engine->guc_id = GUC_VIDEO_ENGINE;
> - engine->mmio_base = GEN6_BSD_RING_BASE;
> + struct intel_engine_cs *engine = logical_ring_setup(dev, VCS);
>
> - logical_ring_default_irqs(engine, GEN8_VCS1_IRQ_SHIFT);
> - logical_ring_default_vfuncs(dev, engine);
> -
> - return logical_ring_init(dev, engine);
> + return logical_ring_init(engine);
> }
>
> static int logical_bsd2_ring_init(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_engine_cs *engine = &dev_priv->engine[VCS2];
> -
> - engine->name = "bsd2 ring";
> - engine->id = VCS2;
> - engine->exec_id = I915_EXEC_BSD;
> - engine->guc_id = GUC_VIDEO_ENGINE2;
> - engine->mmio_base = GEN8_BSD2_RING_BASE;
> + struct intel_engine_cs *engine = logical_ring_setup(dev, VCS2);
>
> - logical_ring_default_irqs(engine, GEN8_VCS2_IRQ_SHIFT);
> - logical_ring_default_vfuncs(dev, engine);
> -
> - return logical_ring_init(dev, engine);
> + return logical_ring_init(engine);
> }
>
> static int logical_blt_ring_init(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_engine_cs *engine = &dev_priv->engine[BCS];
> -
> - engine->name = "blitter ring";
> - engine->id = BCS;
> - engine->exec_id = I915_EXEC_BLT;
> - engine->guc_id = GUC_BLITTER_ENGINE;
> - engine->mmio_base = BLT_RING_BASE;
> -
> - logical_ring_default_irqs(engine, GEN8_BCS_IRQ_SHIFT);
> - logical_ring_default_vfuncs(dev, engine);
> + struct intel_engine_cs *engine = logical_ring_setup(dev, BCS);
>
> - return logical_ring_init(dev, engine);
> + return logical_ring_init(engine);
> }
>
> static int logical_vebox_ring_init(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_engine_cs *engine = &dev_priv->engine[VECS];
> -
> - engine->name = "video enhancement ring";
> - engine->id = VECS;
> - engine->exec_id = I915_EXEC_VEBOX;
> - engine->guc_id = GUC_VIDEOENHANCE_ENGINE;
> - engine->mmio_base = VEBOX_RING_BASE;
> -
> - logical_ring_default_irqs(engine, GEN8_VECS_IRQ_SHIFT);
> - logical_ring_default_vfuncs(dev, engine);
> + struct intel_engine_cs *engine = logical_ring_setup(dev, VECS);
>
> - return logical_ring_init(dev, engine);
> + return logical_ring_init(engine);
> }
>
> /**
>
Otherwise I like how this looks, just engine->dev bugging me!
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2] drm/i915/execlists: Refactor common engine setup
2016-04-29 9:04 ` Tvrtko Ursulin
@ 2016-04-29 9:15 ` Chris Wilson
2016-04-29 9:25 ` Tvrtko Ursulin
0 siblings, 1 reply; 24+ messages in thread
From: Chris Wilson @ 2016-04-29 9:15 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
On Fri, Apr 29, 2016 at 10:04:35AM +0100, Tvrtko Ursulin wrote:
>
> On 28/04/16 18:35, Chris Wilson wrote:
> >Move all of the constant assignments up front and into a common
> >function. This is primarily to ensure the backpointers are set as early
> >as possible for later use during initialisation.
> >
> >v2: Use a constant struct so that all the similar values are set
> >together.
> >
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> >Cc: Dave Gordon <david.s.gordon@intel.com>
> >---
> > drivers/gpu/drm/i915/intel_lrc.c | 176 +++++++++++++++++++++------------------
> > 1 file changed, 93 insertions(+), 83 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >index 874c2515f9d4..2e0eaa9fa240 100644
> >--- a/drivers/gpu/drm/i915/intel_lrc.c
> >+++ b/drivers/gpu/drm/i915/intel_lrc.c
> >@@ -1921,8 +1921,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine)
> > }
> >
> > static void
> >-logical_ring_default_vfuncs(struct drm_device *dev,
> >- struct intel_engine_cs *engine)
> >+logical_ring_default_vfuncs(struct intel_engine_cs *engine)
> > {
> > /* Default vfuncs which can be overriden by each engine. */
> > engine->init_hw = gen8_init_common_ring;
> >@@ -1933,7 +1932,7 @@ logical_ring_default_vfuncs(struct drm_device *dev,
> > engine->emit_bb_start = gen8_emit_bb_start;
> > engine->get_seqno = gen8_get_seqno;
> > engine->set_seqno = gen8_set_seqno;
> >- if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) {
> >+ if (IS_BXT_REVID(engine->dev, 0, BXT_REVID_A1)) {
> > engine->irq_seqno_barrier = bxt_a_seqno_barrier;
> > engine->set_seqno = bxt_a_set_seqno;
> > }
> >@@ -1944,6 +1943,7 @@ logical_ring_default_irqs(struct intel_engine_cs *engine, unsigned shift)
> > {
> > engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << shift;
> > engine->irq_keep_mask = GT_CONTEXT_SWITCH_INTERRUPT << shift;
> >+ init_waitqueue_head(&engine->irq_queue);
> > }
> >
> > static int
> >@@ -1964,31 +1964,68 @@ lrc_setup_hws(struct intel_engine_cs *engine,
> > return 0;
> > }
> >
> >-static int
> >-logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
> >+static const struct logical_ring_info {
> >+ const char *name;
> >+ unsigned exec_id;
> >+ unsigned guc_id;
> >+ u32 mmio_base;
> >+ unsigned irq_shift;
> >+} logical_rings[] = {
> >+ [RCS] = {
> >+ .name = "render ring",
> >+ .exec_id = I915_EXEC_RENDER,
> >+ .guc_id = GUC_RENDER_ENGINE,
> >+ .mmio_base = RENDER_RING_BASE,
> >+ .irq_shift = GEN8_RCS_IRQ_SHIFT,
> >+ },
> >+ [BCS] = {
> >+ .name = "blitter ring",
> >+ .exec_id = I915_EXEC_BLT,
> >+ .guc_id = GUC_BLITTER_ENGINE,
> >+ .mmio_base = BLT_RING_BASE,
> >+ .irq_shift = GEN8_BCS_IRQ_SHIFT,
> >+ },
> >+ [VCS] = {
> >+ .name = "bsd ring",
> >+ .exec_id = I915_EXEC_BSD,
> >+ .guc_id = GUC_VIDEO_ENGINE,
> >+ .mmio_base = GEN6_BSD_RING_BASE,
> >+ .irq_shift = GEN8_VCS1_IRQ_SHIFT,
> >+ },
> >+ [VCS2] = {
> >+ .name = "bsd2 ring",
> >+ .exec_id = I915_EXEC_BSD,
> >+ .guc_id = GUC_VIDEO_ENGINE2,
> >+ .mmio_base = GEN8_BSD2_RING_BASE,
> >+ .irq_shift = GEN8_VCS2_IRQ_SHIFT,
> >+ },
> >+ [VECS] = {
> >+ .name = "video enhancement ring",
> >+ .exec_id = I915_EXEC_VEBOX,
> >+ .guc_id = GUC_VIDEOENHANCE_ENGINE,
> >+ .mmio_base = VEBOX_RING_BASE,
> >+ .irq_shift = GEN8_VECS_IRQ_SHIFT,
> >+ },
> >+};
> >+
> >+static struct intel_engine_cs *
> >+logical_ring_setup(struct drm_device *dev, enum intel_engine_id id)
> > {
>
> Would dev_priv be better? Just to gradually move towards the correct
> state of things.
I have a patch queued up to do engine->i915 (1 KiB in object code
reduction) next.
> >+ const struct logical_ring_info *info = &logical_rings[id];
> > struct drm_i915_private *dev_priv = to_i915(dev);
> >- struct intel_context *dctx = dev_priv->kernel_context;
> >+ struct intel_engine_cs *engine = &dev_priv->engine[id];
> > enum forcewake_domains fw_domains;
> >- int ret;
> >-
> >- /* Intentionally left blank. */
> >- engine->buffer = NULL;
> >
> > engine->dev = dev;
>
> Looking at usages of intel_engine_initialized... one potential
> danger scenario would be interrupt noise during driver load end in
> notify ring and explodes. Sounds very unlikely but theoretically it
> is a regression compared to where engine->dev initialization was
> before.
>
> We should really move away from engine->dev for this and just add an
> explicit flag.
Hmm. not that but I think we really should be sanitizing the irq here
and enabling them last.
Like:
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 2e0eaa9fa240..2c94072ab085 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2016,14 +2016,17 @@ logical_ring_setup(struct drm_device *dev, enum intel_engine_id id)
struct intel_engine_cs *engine = &dev_priv->engine[id];
enum forcewake_domains fw_domains;
- engine->dev = dev;
-
engine->id = id;
engine->name = info->name;
engine->exec_id = info->exec_id;
engine->guc_id = info->guc_id;
engine->mmio_base = info->mmio_base;
+ /* disable interrupts to this engine before we install ourselves*/
+ I915_WRITE_IMR(engine, ~0);
+
+ engine->dev = dev;
+
/* Intentionally left blank. */
engine->buffer = NULL;
Make sense?
-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 related [flat|nested] 24+ messages in thread
* Re: [PATCH v2] drm/i915/execlists: Refactor common engine setup
2016-04-29 9:15 ` Chris Wilson
@ 2016-04-29 9:25 ` Tvrtko Ursulin
2016-04-29 9:39 ` Chris Wilson
2016-04-29 9:42 ` Chris Wilson
0 siblings, 2 replies; 24+ messages in thread
From: Tvrtko Ursulin @ 2016-04-29 9:25 UTC (permalink / raw)
To: Chris Wilson, intel-gfx, Dave Gordon
On 29/04/16 10:15, Chris Wilson wrote:
> On Fri, Apr 29, 2016 at 10:04:35AM +0100, Tvrtko Ursulin wrote:
>>
>> On 28/04/16 18:35, Chris Wilson wrote:
>>> Move all of the constant assignments up front and into a common
>>> function. This is primarily to ensure the backpointers are set as early
>>> as possible for later use during initialisation.
>>>
>>> v2: Use a constant struct so that all the similar values are set
>>> together.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>> Cc: Dave Gordon <david.s.gordon@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/intel_lrc.c | 176 +++++++++++++++++++++------------------
>>> 1 file changed, 93 insertions(+), 83 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>>> index 874c2515f9d4..2e0eaa9fa240 100644
>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>> @@ -1921,8 +1921,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine)
>>> }
>>>
>>> static void
>>> -logical_ring_default_vfuncs(struct drm_device *dev,
>>> - struct intel_engine_cs *engine)
>>> +logical_ring_default_vfuncs(struct intel_engine_cs *engine)
>>> {
>>> /* Default vfuncs which can be overriden by each engine. */
>>> engine->init_hw = gen8_init_common_ring;
>>> @@ -1933,7 +1932,7 @@ logical_ring_default_vfuncs(struct drm_device *dev,
>>> engine->emit_bb_start = gen8_emit_bb_start;
>>> engine->get_seqno = gen8_get_seqno;
>>> engine->set_seqno = gen8_set_seqno;
>>> - if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) {
>>> + if (IS_BXT_REVID(engine->dev, 0, BXT_REVID_A1)) {
>>> engine->irq_seqno_barrier = bxt_a_seqno_barrier;
>>> engine->set_seqno = bxt_a_set_seqno;
>>> }
>>> @@ -1944,6 +1943,7 @@ logical_ring_default_irqs(struct intel_engine_cs *engine, unsigned shift)
>>> {
>>> engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << shift;
>>> engine->irq_keep_mask = GT_CONTEXT_SWITCH_INTERRUPT << shift;
>>> + init_waitqueue_head(&engine->irq_queue);
>>> }
>>>
>>> static int
>>> @@ -1964,31 +1964,68 @@ lrc_setup_hws(struct intel_engine_cs *engine,
>>> return 0;
>>> }
>>>
>>> -static int
>>> -logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
>>> +static const struct logical_ring_info {
>>> + const char *name;
>>> + unsigned exec_id;
>>> + unsigned guc_id;
>>> + u32 mmio_base;
>>> + unsigned irq_shift;
>>> +} logical_rings[] = {
>>> + [RCS] = {
>>> + .name = "render ring",
>>> + .exec_id = I915_EXEC_RENDER,
>>> + .guc_id = GUC_RENDER_ENGINE,
>>> + .mmio_base = RENDER_RING_BASE,
>>> + .irq_shift = GEN8_RCS_IRQ_SHIFT,
>>> + },
>>> + [BCS] = {
>>> + .name = "blitter ring",
>>> + .exec_id = I915_EXEC_BLT,
>>> + .guc_id = GUC_BLITTER_ENGINE,
>>> + .mmio_base = BLT_RING_BASE,
>>> + .irq_shift = GEN8_BCS_IRQ_SHIFT,
>>> + },
>>> + [VCS] = {
>>> + .name = "bsd ring",
>>> + .exec_id = I915_EXEC_BSD,
>>> + .guc_id = GUC_VIDEO_ENGINE,
>>> + .mmio_base = GEN6_BSD_RING_BASE,
>>> + .irq_shift = GEN8_VCS1_IRQ_SHIFT,
>>> + },
>>> + [VCS2] = {
>>> + .name = "bsd2 ring",
>>> + .exec_id = I915_EXEC_BSD,
>>> + .guc_id = GUC_VIDEO_ENGINE2,
>>> + .mmio_base = GEN8_BSD2_RING_BASE,
>>> + .irq_shift = GEN8_VCS2_IRQ_SHIFT,
>>> + },
>>> + [VECS] = {
>>> + .name = "video enhancement ring",
>>> + .exec_id = I915_EXEC_VEBOX,
>>> + .guc_id = GUC_VIDEOENHANCE_ENGINE,
>>> + .mmio_base = VEBOX_RING_BASE,
>>> + .irq_shift = GEN8_VECS_IRQ_SHIFT,
>>> + },
>>> +};
>>> +
>>> +static struct intel_engine_cs *
>>> +logical_ring_setup(struct drm_device *dev, enum intel_engine_id id)
>>> {
>>
>> Would dev_priv be better? Just to gradually move towards the correct
>> state of things.
>
> I have a patch queued up to do engine->i915 (1 KiB in object code
> reduction) next.
>
>>> + const struct logical_ring_info *info = &logical_rings[id];
>>> struct drm_i915_private *dev_priv = to_i915(dev);
>>> - struct intel_context *dctx = dev_priv->kernel_context;
>>> + struct intel_engine_cs *engine = &dev_priv->engine[id];
>>> enum forcewake_domains fw_domains;
>>> - int ret;
>>> -
>>> - /* Intentionally left blank. */
>>> - engine->buffer = NULL;
>>>
>>> engine->dev = dev;
>>
>> Looking at usages of intel_engine_initialized... one potential
>> danger scenario would be interrupt noise during driver load end in
>> notify ring and explodes. Sounds very unlikely but theoretically it
>> is a regression compared to where engine->dev initialization was
>> before.
>>
>> We should really move away from engine->dev for this and just add an
>> explicit flag.
>
> Hmm. not that but I think we really should be sanitizing the irq here
> and enabling them last.
>
> Like:
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 2e0eaa9fa240..2c94072ab085 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -2016,14 +2016,17 @@ logical_ring_setup(struct drm_device *dev, enum intel_engine_id id)
> struct intel_engine_cs *engine = &dev_priv->engine[id];
> enum forcewake_domains fw_domains;
>
> - engine->dev = dev;
> -
> engine->id = id;
> engine->name = info->name;
> engine->exec_id = info->exec_id;
> engine->guc_id = info->guc_id;
> engine->mmio_base = info->mmio_base;
>
> + /* disable interrupts to this engine before we install ourselves*/
> + I915_WRITE_IMR(engine, ~0);
> +
> + engine->dev = dev;
> +
> /* Intentionally left blank. */
> engine->buffer = NULL;
>
> Make sense?
Not the most elegant because all the hw access we have so far is in
engine->init_hw. Why can't we just make intel_engine_initialized return
false until the very last thing in engine constructors?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2] drm/i915/execlists: Refactor common engine setup
2016-04-29 9:25 ` Tvrtko Ursulin
@ 2016-04-29 9:39 ` Chris Wilson
2016-04-29 9:50 ` Tvrtko Ursulin
2016-04-29 9:42 ` Chris Wilson
1 sibling, 1 reply; 24+ messages in thread
From: Chris Wilson @ 2016-04-29 9:39 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
On Fri, Apr 29, 2016 at 10:25:41AM +0100, Tvrtko Ursulin wrote:
> On 29/04/16 10:15, Chris Wilson wrote:
> >diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >index 2e0eaa9fa240..2c94072ab085 100644
> >--- a/drivers/gpu/drm/i915/intel_lrc.c
> >+++ b/drivers/gpu/drm/i915/intel_lrc.c
> >@@ -2016,14 +2016,17 @@ logical_ring_setup(struct drm_device *dev, enum intel_engine_id id)
> > struct intel_engine_cs *engine = &dev_priv->engine[id];
> > enum forcewake_domains fw_domains;
> >
> >- engine->dev = dev;
> >-
> > engine->id = id;
> > engine->name = info->name;
> > engine->exec_id = info->exec_id;
> > engine->guc_id = info->guc_id;
> > engine->mmio_base = info->mmio_base;
> >
> >+ /* disable interrupts to this engine before we install ourselves*/
> >+ I915_WRITE_IMR(engine, ~0);
> >+
> >+ engine->dev = dev;
> >+
> > /* Intentionally left blank. */
> > engine->buffer = NULL;
> >
> >Make sense?
>
> Not the most elegant because all the hw access we have so far is in
> engine->init_hw. Why can't we just make intel_engine_initialized
> return false until the very last thing in engine constructors?
In my defence sanitizing the hw before we are ready is common practice
across the driver. The unfun part is that irq install is before gem_init
(because modeset init wants irq enabled for GMBUS/dp-aux). gem init
itself could be split up and moved around so that the setup and init_hw
phases are separate (which would be next on the init ordering hitlist I
hope).
I want engine->dev/engine->i915 set early so we can use it during setup
and init-hw and so that for_each_engine() works as expected in that
time.
-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] 24+ messages in thread
* Re: [PATCH v2] drm/i915/execlists: Refactor common engine setup
2016-04-29 9:25 ` Tvrtko Ursulin
2016-04-29 9:39 ` Chris Wilson
@ 2016-04-29 9:42 ` Chris Wilson
1 sibling, 0 replies; 24+ messages in thread
From: Chris Wilson @ 2016-04-29 9:42 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
On Fri, Apr 29, 2016 at 10:25:41AM +0100, Tvrtko Ursulin wrote:
> Not the most elegant because all the hw access we have so far is in
> engine->init_hw. Why can't we just make intel_engine_initialized
> return false until the very last thing in engine constructors?
The other thing I've been mulling over is just making for_each_engine
iterate over the ring_mask. Means the reinstatement of temporary
variables (at least for the macros I've sketched) :(
-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] 24+ messages in thread
* Re: [PATCH v2] drm/i915/execlists: Refactor common engine setup
2016-04-29 9:39 ` Chris Wilson
@ 2016-04-29 9:50 ` Tvrtko Ursulin
2016-04-29 10:00 ` Chris Wilson
0 siblings, 1 reply; 24+ messages in thread
From: Tvrtko Ursulin @ 2016-04-29 9:50 UTC (permalink / raw)
To: Chris Wilson, intel-gfx, Dave Gordon
On 29/04/16 10:39, Chris Wilson wrote:
> On Fri, Apr 29, 2016 at 10:25:41AM +0100, Tvrtko Ursulin wrote:
>> On 29/04/16 10:15, Chris Wilson wrote:
>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>>> index 2e0eaa9fa240..2c94072ab085 100644
>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>> @@ -2016,14 +2016,17 @@ logical_ring_setup(struct drm_device *dev, enum intel_engine_id id)
>>> struct intel_engine_cs *engine = &dev_priv->engine[id];
>>> enum forcewake_domains fw_domains;
>>>
>>> - engine->dev = dev;
>>> -
>>> engine->id = id;
>>> engine->name = info->name;
>>> engine->exec_id = info->exec_id;
>>> engine->guc_id = info->guc_id;
>>> engine->mmio_base = info->mmio_base;
>>>
>>> + /* disable interrupts to this engine before we install ourselves*/
>>> + I915_WRITE_IMR(engine, ~0);
>>> +
>>> + engine->dev = dev;
>>> +
>>> /* Intentionally left blank. */
>>> engine->buffer = NULL;
>>>
>>> Make sense?
>>
>> Not the most elegant because all the hw access we have so far is in
>> engine->init_hw. Why can't we just make intel_engine_initialized
>> return false until the very last thing in engine constructors?
>
> In my defence sanitizing the hw before we are ready is common practice
> across the driver. The unfun part is that irq install is before gem_init
> (because modeset init wants irq enabled for GMBUS/dp-aux). gem init
> itself could be split up and moved around so that the setup and init_hw
> phases are separate (which would be next on the init ordering hitlist I
> hope).
>
> I want engine->dev/engine->i915 set early so we can use it during setup
> and init-hw and so that for_each_engine() works as expected in that
> time.
Why wouldn't an explicit engine->initialized flag solve that? You could
keep setting engine->dev early (as it should be) and then set
engine->initialized at the end of per-engine constructors.
Engine setup would then work fine - I don't see it needed
for_each_engine or intel_engine_initialized ?
And problem of hw sanitation could then be left out of this
patch/discussion, no?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2] drm/i915/execlists: Refactor common engine setup
2016-04-29 9:50 ` Tvrtko Ursulin
@ 2016-04-29 10:00 ` Chris Wilson
2016-04-29 10:11 ` Tvrtko Ursulin
0 siblings, 1 reply; 24+ messages in thread
From: Chris Wilson @ 2016-04-29 10:00 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
On Fri, Apr 29, 2016 at 10:50:02AM +0100, Tvrtko Ursulin wrote:
>
> On 29/04/16 10:39, Chris Wilson wrote:
> >On Fri, Apr 29, 2016 at 10:25:41AM +0100, Tvrtko Ursulin wrote:
> >>On 29/04/16 10:15, Chris Wilson wrote:
> >>>diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >>>index 2e0eaa9fa240..2c94072ab085 100644
> >>>--- a/drivers/gpu/drm/i915/intel_lrc.c
> >>>+++ b/drivers/gpu/drm/i915/intel_lrc.c
> >>>@@ -2016,14 +2016,17 @@ logical_ring_setup(struct drm_device *dev, enum intel_engine_id id)
> >>> struct intel_engine_cs *engine = &dev_priv->engine[id];
> >>> enum forcewake_domains fw_domains;
> >>>
> >>>- engine->dev = dev;
> >>>-
> >>> engine->id = id;
> >>> engine->name = info->name;
> >>> engine->exec_id = info->exec_id;
> >>> engine->guc_id = info->guc_id;
> >>> engine->mmio_base = info->mmio_base;
> >>>
> >>>+ /* disable interrupts to this engine before we install ourselves*/
> >>>+ I915_WRITE_IMR(engine, ~0);
> >>>+
> >>>+ engine->dev = dev;
> >>>+
> >>> /* Intentionally left blank. */
> >>> engine->buffer = NULL;
> >>>
> >>>Make sense?
> >>
> >>Not the most elegant because all the hw access we have so far is in
> >>engine->init_hw. Why can't we just make intel_engine_initialized
> >>return false until the very last thing in engine constructors?
> >
> >In my defence sanitizing the hw before we are ready is common practice
> >across the driver. The unfun part is that irq install is before gem_init
> >(because modeset init wants irq enabled for GMBUS/dp-aux). gem init
> >itself could be split up and moved around so that the setup and init_hw
> >phases are separate (which would be next on the init ordering hitlist I
> >hope).
> >
> >I want engine->dev/engine->i915 set early so we can use it during setup
> >and init-hw and so that for_each_engine() works as expected in that
> >time.
>
> Why wouldn't an explicit engine->initialized flag solve that? You
> could keep setting engine->dev early (as it should be) and then set
> engine->initialized at the end of per-engine constructors.
Because it becomes irrelevant very shortly. The only interesting
question remaining is whether or not we should be sanitizing the IMR
first. It has been suggested elsewhere (in Ville's review of the GT
interrupt handling) that doing the sanitization would be useful.
-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] 24+ messages in thread
* Re: [PATCH v2] drm/i915/execlists: Refactor common engine setup
2016-04-29 10:00 ` Chris Wilson
@ 2016-04-29 10:11 ` Tvrtko Ursulin
2016-04-29 10:22 ` Chris Wilson
0 siblings, 1 reply; 24+ messages in thread
From: Tvrtko Ursulin @ 2016-04-29 10:11 UTC (permalink / raw)
To: Chris Wilson, intel-gfx, Dave Gordon
On 29/04/16 11:00, Chris Wilson wrote:
> On Fri, Apr 29, 2016 at 10:50:02AM +0100, Tvrtko Ursulin wrote:
>>
>> On 29/04/16 10:39, Chris Wilson wrote:
>>> On Fri, Apr 29, 2016 at 10:25:41AM +0100, Tvrtko Ursulin wrote:
>>>> On 29/04/16 10:15, Chris Wilson wrote:
>>>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>>>>> index 2e0eaa9fa240..2c94072ab085 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>>>> @@ -2016,14 +2016,17 @@ logical_ring_setup(struct drm_device *dev, enum intel_engine_id id)
>>>>> struct intel_engine_cs *engine = &dev_priv->engine[id];
>>>>> enum forcewake_domains fw_domains;
>>>>>
>>>>> - engine->dev = dev;
>>>>> -
>>>>> engine->id = id;
>>>>> engine->name = info->name;
>>>>> engine->exec_id = info->exec_id;
>>>>> engine->guc_id = info->guc_id;
>>>>> engine->mmio_base = info->mmio_base;
>>>>>
>>>>> + /* disable interrupts to this engine before we install ourselves*/
>>>>> + I915_WRITE_IMR(engine, ~0);
>>>>> +
>>>>> + engine->dev = dev;
>>>>> +
>>>>> /* Intentionally left blank. */
>>>>> engine->buffer = NULL;
>>>>>
>>>>> Make sense?
>>>>
>>>> Not the most elegant because all the hw access we have so far is in
>>>> engine->init_hw. Why can't we just make intel_engine_initialized
>>>> return false until the very last thing in engine constructors?
>>>
>>> In my defence sanitizing the hw before we are ready is common practice
>>> across the driver. The unfun part is that irq install is before gem_init
>>> (because modeset init wants irq enabled for GMBUS/dp-aux). gem init
>>> itself could be split up and moved around so that the setup and init_hw
>>> phases are separate (which would be next on the init ordering hitlist I
>>> hope).
>>>
>>> I want engine->dev/engine->i915 set early so we can use it during setup
>>> and init-hw and so that for_each_engine() works as expected in that
>>> time.
>>
>> Why wouldn't an explicit engine->initialized flag solve that? You
>> could keep setting engine->dev early (as it should be) and then set
>> engine->initialized at the end of per-engine constructors.
>
> Because it becomes irrelevant very shortly. The only interesting
> question remaining is whether or not we should be sanitizing the IMR
> first. It has been suggested elsewhere (in Ville's review of the GT
> interrupt handling) that doing the sanitization would be useful.
How come it becomes irrelevant? Will there not be
intel_engine_initialized? Because as long as there is, imho it makes
sense not to use engine->dev for it.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2] drm/i915/execlists: Refactor common engine setup
2016-04-29 10:11 ` Tvrtko Ursulin
@ 2016-04-29 10:22 ` Chris Wilson
2016-05-02 8:51 ` Daniel Vetter
0 siblings, 1 reply; 24+ messages in thread
From: Chris Wilson @ 2016-04-29 10:22 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
On Fri, Apr 29, 2016 at 11:11:20AM +0100, Tvrtko Ursulin wrote:
>
> On 29/04/16 11:00, Chris Wilson wrote:
> >On Fri, Apr 29, 2016 at 10:50:02AM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 29/04/16 10:39, Chris Wilson wrote:
> >>>On Fri, Apr 29, 2016 at 10:25:41AM +0100, Tvrtko Ursulin wrote:
> >>>>On 29/04/16 10:15, Chris Wilson wrote:
> >>>>>diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >>>>>index 2e0eaa9fa240..2c94072ab085 100644
> >>>>>--- a/drivers/gpu/drm/i915/intel_lrc.c
> >>>>>+++ b/drivers/gpu/drm/i915/intel_lrc.c
> >>>>>@@ -2016,14 +2016,17 @@ logical_ring_setup(struct drm_device *dev, enum intel_engine_id id)
> >>>>> struct intel_engine_cs *engine = &dev_priv->engine[id];
> >>>>> enum forcewake_domains fw_domains;
> >>>>>
> >>>>>- engine->dev = dev;
> >>>>>-
> >>>>> engine->id = id;
> >>>>> engine->name = info->name;
> >>>>> engine->exec_id = info->exec_id;
> >>>>> engine->guc_id = info->guc_id;
> >>>>> engine->mmio_base = info->mmio_base;
> >>>>>
> >>>>>+ /* disable interrupts to this engine before we install ourselves*/
> >>>>>+ I915_WRITE_IMR(engine, ~0);
> >>>>>+
> >>>>>+ engine->dev = dev;
> >>>>>+
> >>>>> /* Intentionally left blank. */
> >>>>> engine->buffer = NULL;
> >>>>>
> >>>>>Make sense?
> >>>>
> >>>>Not the most elegant because all the hw access we have so far is in
> >>>>engine->init_hw. Why can't we just make intel_engine_initialized
> >>>>return false until the very last thing in engine constructors?
> >>>
> >>>In my defence sanitizing the hw before we are ready is common practice
> >>>across the driver. The unfun part is that irq install is before gem_init
> >>>(because modeset init wants irq enabled for GMBUS/dp-aux). gem init
> >>>itself could be split up and moved around so that the setup and init_hw
> >>>phases are separate (which would be next on the init ordering hitlist I
> >>>hope).
> >>>
> >>>I want engine->dev/engine->i915 set early so we can use it during setup
> >>>and init-hw and so that for_each_engine() works as expected in that
> >>>time.
> >>
> >>Why wouldn't an explicit engine->initialized flag solve that? You
> >>could keep setting engine->dev early (as it should be) and then set
> >>engine->initialized at the end of per-engine constructors.
> >
> >Because it becomes irrelevant very shortly. The only interesting
> >question remaining is whether or not we should be sanitizing the IMR
> >first. It has been suggested elsewhere (in Ville's review of the GT
> >interrupt handling) that doing the sanitization would be useful.
>
> How come it becomes irrelevant? Will there not be
> intel_engine_initialized? Because as long as there is, imho it makes
> sense not to use engine->dev for it.
The only argument here is how to handle an interrupt left over from
before the driver loads. At all other times engine->dev means precisely
that. I do not agree that you need to duplicate the state.
-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] 24+ messages in thread
* Re: [PATCH v2] drm/i915/execlists: Refactor common engine setup
2016-04-29 10:22 ` Chris Wilson
@ 2016-05-02 8:51 ` Daniel Vetter
2016-05-02 10:58 ` Chris Wilson
0 siblings, 1 reply; 24+ messages in thread
From: Daniel Vetter @ 2016-05-02 8:51 UTC (permalink / raw)
To: Chris Wilson, Tvrtko Ursulin, intel-gfx, Dave Gordon
On Fri, Apr 29, 2016 at 11:22:59AM +0100, Chris Wilson wrote:
> On Fri, Apr 29, 2016 at 11:11:20AM +0100, Tvrtko Ursulin wrote:
> >
> > On 29/04/16 11:00, Chris Wilson wrote:
> > >On Fri, Apr 29, 2016 at 10:50:02AM +0100, Tvrtko Ursulin wrote:
> > >>
> > >>On 29/04/16 10:39, Chris Wilson wrote:
> > >>>On Fri, Apr 29, 2016 at 10:25:41AM +0100, Tvrtko Ursulin wrote:
> > >>>>On 29/04/16 10:15, Chris Wilson wrote:
> > >>>>>diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > >>>>>index 2e0eaa9fa240..2c94072ab085 100644
> > >>>>>--- a/drivers/gpu/drm/i915/intel_lrc.c
> > >>>>>+++ b/drivers/gpu/drm/i915/intel_lrc.c
> > >>>>>@@ -2016,14 +2016,17 @@ logical_ring_setup(struct drm_device *dev, enum intel_engine_id id)
> > >>>>> struct intel_engine_cs *engine = &dev_priv->engine[id];
> > >>>>> enum forcewake_domains fw_domains;
> > >>>>>
> > >>>>>- engine->dev = dev;
> > >>>>>-
> > >>>>> engine->id = id;
> > >>>>> engine->name = info->name;
> > >>>>> engine->exec_id = info->exec_id;
> > >>>>> engine->guc_id = info->guc_id;
> > >>>>> engine->mmio_base = info->mmio_base;
> > >>>>>
> > >>>>>+ /* disable interrupts to this engine before we install ourselves*/
> > >>>>>+ I915_WRITE_IMR(engine, ~0);
> > >>>>>+
> > >>>>>+ engine->dev = dev;
> > >>>>>+
> > >>>>> /* Intentionally left blank. */
> > >>>>> engine->buffer = NULL;
> > >>>>>
> > >>>>>Make sense?
> > >>>>
> > >>>>Not the most elegant because all the hw access we have so far is in
> > >>>>engine->init_hw. Why can't we just make intel_engine_initialized
> > >>>>return false until the very last thing in engine constructors?
> > >>>
> > >>>In my defence sanitizing the hw before we are ready is common practice
> > >>>across the driver. The unfun part is that irq install is before gem_init
> > >>>(because modeset init wants irq enabled for GMBUS/dp-aux). gem init
> > >>>itself could be split up and moved around so that the setup and init_hw
> > >>>phases are separate (which would be next on the init ordering hitlist I
> > >>>hope).
> > >>>
> > >>>I want engine->dev/engine->i915 set early so we can use it during setup
> > >>>and init-hw and so that for_each_engine() works as expected in that
> > >>>time.
> > >>
> > >>Why wouldn't an explicit engine->initialized flag solve that? You
> > >>could keep setting engine->dev early (as it should be) and then set
> > >>engine->initialized at the end of per-engine constructors.
> > >
> > >Because it becomes irrelevant very shortly. The only interesting
> > >question remaining is whether or not we should be sanitizing the IMR
> > >first. It has been suggested elsewhere (in Ville's review of the GT
> > >interrupt handling) that doing the sanitization would be useful.
> >
> > How come it becomes irrelevant? Will there not be
> > intel_engine_initialized? Because as long as there is, imho it makes
> > sense not to use engine->dev for it.
>
> The only argument here is how to handle an interrupt left over from
> before the driver loads. At all other times engine->dev means precisely
> that. I do not agree that you need to duplicate the state.
Imo the low-level irq clearing should all be done in the relevant irq
setup code in i915_irq.c. Atm we just forgot to do that. I guess you can
have a bikeshed whether the enginer IMR enable/disable functions should be
together with the clearing or not, placing them in either file is fine.
But since we already clear the higher-level IMR registers in i915_irq.c we
might as well clear the low-level ones too in i915_irq.c.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2] drm/i915/execlists: Refactor common engine setup
2016-05-02 8:51 ` Daniel Vetter
@ 2016-05-02 10:58 ` Chris Wilson
2016-05-09 7:02 ` Daniel Vetter
0 siblings, 1 reply; 24+ messages in thread
From: Chris Wilson @ 2016-05-02 10:58 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Mon, May 02, 2016 at 10:51:31AM +0200, Daniel Vetter wrote:
> Imo the low-level irq clearing should all be done in the relevant irq
> setup code in i915_irq.c. Atm we just forgot to do that. I guess you can
> have a bikeshed whether the enginer IMR enable/disable functions should be
> together with the clearing or not, placing them in either file is fine.
> But since we already clear the higher-level IMR registers in i915_irq.c we
> might as well clear the low-level ones too in i915_irq.c.
That's tricky since that is done before the engines - so how do we get
the various bases to i915_irq.c without duplication? Enabling the irq
for the engines is part of init_hw, so correspondingly putting the
early disable into init looks fine to me.
-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] 24+ messages in thread
* Re: [PATCH v2] drm/i915/execlists: Refactor common engine setup
2016-05-02 10:58 ` Chris Wilson
@ 2016-05-09 7:02 ` Daniel Vetter
2016-05-09 7:45 ` Chris Wilson
0 siblings, 1 reply; 24+ messages in thread
From: Daniel Vetter @ 2016-05-09 7:02 UTC (permalink / raw)
To: Chris Wilson, Daniel Vetter, Tvrtko Ursulin, intel-gfx, Dave Gordon
On Mon, May 02, 2016 at 11:58:38AM +0100, Chris Wilson wrote:
> On Mon, May 02, 2016 at 10:51:31AM +0200, Daniel Vetter wrote:
> > Imo the low-level irq clearing should all be done in the relevant irq
> > setup code in i915_irq.c. Atm we just forgot to do that. I guess you can
> > have a bikeshed whether the enginer IMR enable/disable functions should be
> > together with the clearing or not, placing them in either file is fine.
> > But since we already clear the higher-level IMR registers in i915_irq.c we
> > might as well clear the low-level ones too in i915_irq.c.
>
> That's tricky since that is done before the engines - so how do we get
> the various bases to i915_irq.c without duplication? Enabling the irq
> for the engines is part of init_hw, so correspondingly putting the
> early disable into init looks fine to me.
I just don't particularly like that we have hw access in a function that
thus far (and at a glance still after this patch) only sets up software
state. I'll probably lead to some ugly surprise. That's why I'd like to
move this either to engine->init_hw or to i915_irq.c.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2] drm/i915/execlists: Refactor common engine setup
2016-05-09 7:02 ` Daniel Vetter
@ 2016-05-09 7:45 ` Chris Wilson
2016-05-09 7:58 ` Daniel Vetter
0 siblings, 1 reply; 24+ messages in thread
From: Chris Wilson @ 2016-05-09 7:45 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Mon, May 09, 2016 at 09:02:33AM +0200, Daniel Vetter wrote:
> On Mon, May 02, 2016 at 11:58:38AM +0100, Chris Wilson wrote:
> > On Mon, May 02, 2016 at 10:51:31AM +0200, Daniel Vetter wrote:
> > > Imo the low-level irq clearing should all be done in the relevant irq
> > > setup code in i915_irq.c. Atm we just forgot to do that. I guess you can
> > > have a bikeshed whether the enginer IMR enable/disable functions should be
> > > together with the clearing or not, placing them in either file is fine.
> > > But since we already clear the higher-level IMR registers in i915_irq.c we
> > > might as well clear the low-level ones too in i915_irq.c.
> >
> > That's tricky since that is done before the engines - so how do we get
> > the various bases to i915_irq.c without duplication? Enabling the irq
> > for the engines is part of init_hw, so correspondingly putting the
> > early disable into init looks fine to me.
>
> I just don't particularly like that we have hw access in a function that
> thus far (and at a glance still after this patch) only sets up software
> state. I'll probably lead to some ugly surprise. That's why I'd like to
> move this either to engine->init_hw or to i915_irq.c.
This is sanitize. We do enable it in engine->init_hw(), but the point
raised by Ville earlier in his review of GT irq handling is that nobody
currently disables the ring IMR before use. Here we have a
chicken-and-egg problem, do we duplicate knowledge of available engines
(and their mmio_base) in irq preinstall/sanitize or do we do the engine
specific mmio in engine initialisation? The problem Turslin was raising
was that on future enabling, somebody had enabled the engine IRQ before
the engines were initialised (i.e. had completely disregarded the
current init_hw sequence). Plonking it in i915_irq.c is not foolproof
either!
-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] 24+ messages in thread
* Re: [PATCH v2] drm/i915/execlists: Refactor common engine setup
2016-05-09 7:45 ` Chris Wilson
@ 2016-05-09 7:58 ` Daniel Vetter
2016-05-09 10:41 ` Chris Wilson
0 siblings, 1 reply; 24+ messages in thread
From: Daniel Vetter @ 2016-05-09 7:58 UTC (permalink / raw)
To: Chris Wilson, Daniel Vetter, Tvrtko Ursulin, intel-gfx, Dave Gordon
On Mon, May 09, 2016 at 08:45:16AM +0100, Chris Wilson wrote:
> On Mon, May 09, 2016 at 09:02:33AM +0200, Daniel Vetter wrote:
> > On Mon, May 02, 2016 at 11:58:38AM +0100, Chris Wilson wrote:
> > > On Mon, May 02, 2016 at 10:51:31AM +0200, Daniel Vetter wrote:
> > > > Imo the low-level irq clearing should all be done in the relevant irq
> > > > setup code in i915_irq.c. Atm we just forgot to do that. I guess you can
> > > > have a bikeshed whether the enginer IMR enable/disable functions should be
> > > > together with the clearing or not, placing them in either file is fine.
> > > > But since we already clear the higher-level IMR registers in i915_irq.c we
> > > > might as well clear the low-level ones too in i915_irq.c.
> > >
> > > That's tricky since that is done before the engines - so how do we get
> > > the various bases to i915_irq.c without duplication? Enabling the irq
> > > for the engines is part of init_hw, so correspondingly putting the
> > > early disable into init looks fine to me.
> >
> > I just don't particularly like that we have hw access in a function that
> > thus far (and at a glance still after this patch) only sets up software
> > state. I'll probably lead to some ugly surprise. That's why I'd like to
> > move this either to engine->init_hw or to i915_irq.c.
>
> This is sanitize. We do enable it in engine->init_hw(), but the point
> raised by Ville earlier in his review of GT irq handling is that nobody
> currently disables the ring IMR before use. Here we have a
> chicken-and-egg problem, do we duplicate knowledge of available engines
> (and their mmio_base) in irq preinstall/sanitize or do we do the engine
> specific mmio in engine initialisation? The problem Turslin was raising
> was that on future enabling, somebody had enabled the engine IRQ before
> the engines were initialised (i.e. had completely disregarded the
> current init_hw sequence). Plonking it in i915_irq.c is not foolproof
> either!
Hm, couldn't we put it into init_hw? i915_irq.c sets up the top-level
interrupts, but for GT stuff all masked. In init_hw we could clear that
then, and before init_hw no one should call ring->get_irq to enable it and
potentially cause havoc. Or still too fragile in your opinion?
Indeed putting it into i915_irq.c seems to not be great since it splits
the gt per-engine mask reg handling too far.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2] drm/i915/execlists: Refactor common engine setup
2016-05-09 7:58 ` Daniel Vetter
@ 2016-05-09 10:41 ` Chris Wilson
2016-05-10 7:46 ` Daniel Vetter
0 siblings, 1 reply; 24+ messages in thread
From: Chris Wilson @ 2016-05-09 10:41 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Mon, May 09, 2016 at 09:58:20AM +0200, Daniel Vetter wrote:
> On Mon, May 09, 2016 at 08:45:16AM +0100, Chris Wilson wrote:
> > This is sanitize. We do enable it in engine->init_hw(), but the point
> > raised by Ville earlier in his review of GT irq handling is that nobody
> > currently disables the ring IMR before use. Here we have a
> > chicken-and-egg problem, do we duplicate knowledge of available engines
> > (and their mmio_base) in irq preinstall/sanitize or do we do the engine
> > specific mmio in engine initialisation? The problem Turslin was raising
> > was that on future enabling, somebody had enabled the engine IRQ before
> > the engines were initialised (i.e. had completely disregarded the
> > current init_hw sequence). Plonking it in i915_irq.c is not foolproof
> > either!
>
> Hm, couldn't we put it into init_hw? i915_irq.c sets up the top-level
> interrupts, but for GT stuff all masked. In init_hw we could clear that
> then, and before init_hw no one should call ring->get_irq to enable it and
> potentially cause havoc. Or still too fragile in your opinion?
The race is if we get an interrupt inside init_engine, after we set
engine->dev but before we setup the state for the irq handler. (Note the
race isn't strictly just dev, everything we touch inside the irq handler
gives arise to a potential ordering issue.)
> Indeed putting it into i915_irq.c seems to not be great since it splits
> the gt per-engine mask reg handling too far.
Yeah.
-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] 24+ messages in thread
* Re: [PATCH v2] drm/i915/execlists: Refactor common engine setup
2016-05-09 10:41 ` Chris Wilson
@ 2016-05-10 7:46 ` Daniel Vetter
2016-05-10 7:50 ` Chris Wilson
0 siblings, 1 reply; 24+ messages in thread
From: Daniel Vetter @ 2016-05-10 7:46 UTC (permalink / raw)
To: Chris Wilson, Daniel Vetter, Tvrtko Ursulin, intel-gfx, Dave Gordon
On Mon, May 09, 2016 at 11:41:41AM +0100, Chris Wilson wrote:
> On Mon, May 09, 2016 at 09:58:20AM +0200, Daniel Vetter wrote:
> > On Mon, May 09, 2016 at 08:45:16AM +0100, Chris Wilson wrote:
> > > This is sanitize. We do enable it in engine->init_hw(), but the point
> > > raised by Ville earlier in his review of GT irq handling is that nobody
> > > currently disables the ring IMR before use. Here we have a
> > > chicken-and-egg problem, do we duplicate knowledge of available engines
> > > (and their mmio_base) in irq preinstall/sanitize or do we do the engine
> > > specific mmio in engine initialisation? The problem Turslin was raising
> > > was that on future enabling, somebody had enabled the engine IRQ before
> > > the engines were initialised (i.e. had completely disregarded the
> > > current init_hw sequence). Plonking it in i915_irq.c is not foolproof
> > > either!
> >
> > Hm, couldn't we put it into init_hw? i915_irq.c sets up the top-level
> > interrupts, but for GT stuff all masked. In init_hw we could clear that
> > then, and before init_hw no one should call ring->get_irq to enable it and
> > potentially cause havoc. Or still too fragile in your opinion?
>
> The race is if we get an interrupt inside init_engine, after we set
> engine->dev but before we setup the state for the irq handler. (Note the
> race isn't strictly just dev, everything we touch inside the irq handler
> gives arise to a potential ordering issue.)
But how does this happen? Assuming we did mask all the higher bits
correctly beforehand ... Is this just theoretical (in which case I think
cleanup in init_hw is totally fine), or did it go kaboom already?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2] drm/i915/execlists: Refactor common engine setup
2016-05-10 7:46 ` Daniel Vetter
@ 2016-05-10 7:50 ` Chris Wilson
0 siblings, 0 replies; 24+ messages in thread
From: Chris Wilson @ 2016-05-10 7:50 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Tue, May 10, 2016 at 09:46:05AM +0200, Daniel Vetter wrote:
> On Mon, May 09, 2016 at 11:41:41AM +0100, Chris Wilson wrote:
> > On Mon, May 09, 2016 at 09:58:20AM +0200, Daniel Vetter wrote:
> > > On Mon, May 09, 2016 at 08:45:16AM +0100, Chris Wilson wrote:
> > > > This is sanitize. We do enable it in engine->init_hw(), but the point
> > > > raised by Ville earlier in his review of GT irq handling is that nobody
> > > > currently disables the ring IMR before use. Here we have a
> > > > chicken-and-egg problem, do we duplicate knowledge of available engines
> > > > (and their mmio_base) in irq preinstall/sanitize or do we do the engine
> > > > specific mmio in engine initialisation? The problem Turslin was raising
> > > > was that on future enabling, somebody had enabled the engine IRQ before
> > > > the engines were initialised (i.e. had completely disregarded the
> > > > current init_hw sequence). Plonking it in i915_irq.c is not foolproof
> > > > either!
> > >
> > > Hm, couldn't we put it into init_hw? i915_irq.c sets up the top-level
> > > interrupts, but for GT stuff all masked. In init_hw we could clear that
> > > then, and before init_hw no one should call ring->get_irq to enable it and
> > > potentially cause havoc. Or still too fragile in your opinion?
> >
> > The race is if we get an interrupt inside init_engine, after we set
> > engine->dev but before we setup the state for the irq handler. (Note the
> > race isn't strictly just dev, everything we touch inside the irq handler
> > gives arise to a potential ordering issue.)
>
> But how does this happen? Assuming we did mask all the higher bits
> correctly beforehand ... Is this just theoretical (in which case I think
> cleanup in init_hw is totally fine), or did it go kaboom already?
We haven't masked all the bits correctly beforehand. We don't today, and
tomorrow someone thought it would be fun to enable the GT interrupts
before the engines were initialised. Still, it can only go bang if the
interrupt was asserted - so a particularly interesting bios or kexec /
hibernation scenario.
-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] 24+ messages in thread
end of thread, other threads:[~2016-05-10 7:50 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-28 13:47 [PATCH] drm/i915/execlists: Refactor common engine setup Chris Wilson
2016-04-28 14:17 ` ✗ Fi.CI.BAT: warning for " Patchwork
2016-04-28 15:10 ` [PATCH] " Tvrtko Ursulin
2016-04-28 15:26 ` Chris Wilson
2016-04-28 16:12 ` Dave Gordon
2016-04-28 17:04 ` Chris Wilson
2016-04-28 17:35 ` [PATCH v2] " Chris Wilson
2016-04-29 9:04 ` Tvrtko Ursulin
2016-04-29 9:15 ` Chris Wilson
2016-04-29 9:25 ` Tvrtko Ursulin
2016-04-29 9:39 ` Chris Wilson
2016-04-29 9:50 ` Tvrtko Ursulin
2016-04-29 10:00 ` Chris Wilson
2016-04-29 10:11 ` Tvrtko Ursulin
2016-04-29 10:22 ` Chris Wilson
2016-05-02 8:51 ` Daniel Vetter
2016-05-02 10:58 ` Chris Wilson
2016-05-09 7:02 ` Daniel Vetter
2016-05-09 7:45 ` Chris Wilson
2016-05-09 7:58 ` Daniel Vetter
2016-05-09 10:41 ` Chris Wilson
2016-05-10 7:46 ` Daniel Vetter
2016-05-10 7:50 ` Chris Wilson
2016-04-29 9:42 ` Chris Wilson
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.