* [PATCH 2/5] drm/i915: Prepare for engine init unification
2016-07-01 16:47 [PATCH 1/5] drm/i915: unify first-stage engine struct setup Tvrtko Ursulin
@ 2016-07-01 16:47 ` Tvrtko Ursulin
2016-07-01 16:47 ` [PATCH 3/5] drm/i915: Unify engine init loop Tvrtko Ursulin
` (4 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2016-07-01 16:47 UTC (permalink / raw)
To: Intel-gfx
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Move the execlist engine setup to vfuncs so that the engine
init loop is clearly split into the mode agnostic and
specific steps.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/intel_lrc.c | 103 ++++++++++++++++++++-------------------
1 file changed, 54 insertions(+), 49 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index ed017f1a07a2..abb165019af7 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2020,6 +2020,46 @@ lrc_setup_hws(struct intel_engine_cs *engine,
return 0;
}
+static void
+logical_ring_setup(struct intel_engine_cs *engine)
+{
+ struct drm_i915_private *dev_priv = engine->i915;
+ enum forcewake_domains fw_domains;
+
+ /* Intentionally left blank. */
+ engine->buffer = NULL;
+
+ fw_domains = intel_uncore_forcewake_for_reg(dev_priv,
+ RING_ELSP(engine),
+ FW_REG_WRITE);
+
+ fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
+ RING_CONTEXT_STATUS_PTR(engine),
+ FW_REG_READ | FW_REG_WRITE);
+
+ fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
+ RING_CONTEXT_STATUS_BUF_BASE(engine),
+ FW_REG_READ);
+
+ 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);
+
+ intel_engine_init_hangcheck(engine);
+ i915_gem_batch_pool_init(dev_priv->dev, &engine->batch_pool);
+}
+
static int
logical_ring_init(struct intel_engine_cs *engine)
{
@@ -2061,6 +2101,8 @@ static int logical_render_ring_init(struct intel_engine_cs *engine)
struct drm_i915_private *dev_priv = engine->i915;
int ret;
+ logical_ring_setup(engine);
+
if (HAS_L3_DPF(dev_priv))
engine->irq_keep_mask |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
@@ -2097,6 +2139,13 @@ static int logical_render_ring_init(struct intel_engine_cs *engine)
return ret;
}
+static int logical_xcs_ring_init(struct intel_engine_cs *engine)
+{
+ logical_ring_setup(engine);
+
+ return logical_ring_init(engine);
+}
+
static const struct engine_info {
const char *name;
unsigned exec_id;
@@ -2119,7 +2168,7 @@ static const struct engine_info {
.guc_id = GUC_BLITTER_ENGINE,
.mmio_base = BLT_RING_BASE,
.irq_shift = GEN8_BCS_IRQ_SHIFT,
- .init = logical_ring_init,
+ .init = logical_xcs_ring_init,
},
[VCS] = {
.name = "bsd ring",
@@ -2127,7 +2176,7 @@ static const struct engine_info {
.guc_id = GUC_VIDEO_ENGINE,
.mmio_base = GEN6_BSD_RING_BASE,
.irq_shift = GEN8_VCS1_IRQ_SHIFT,
- .init = logical_ring_init,
+ .init = logical_xcs_ring_init,
},
[VCS2] = {
.name = "bsd2 ring",
@@ -2135,7 +2184,7 @@ static const struct engine_info {
.guc_id = GUC_VIDEO_ENGINE2,
.mmio_base = GEN8_BSD2_RING_BASE,
.irq_shift = GEN8_VCS2_IRQ_SHIFT,
- .init = logical_ring_init,
+ .init = logical_xcs_ring_init,
},
[VECS] = {
.name = "video enhancement ring",
@@ -2143,7 +2192,7 @@ static const struct engine_info {
.guc_id = GUC_VIDEOENHANCE_ENGINE,
.mmio_base = VEBOX_RING_BASE,
.irq_shift = GEN8_VECS_IRQ_SHIFT,
- .init = logical_ring_init,
+ .init = logical_xcs_ring_init,
},
};
@@ -2165,50 +2214,6 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
return engine;
}
-static struct intel_engine_cs *
-logical_ring_setup(struct drm_i915_private *dev_priv, enum intel_engine_id id)
-{
- struct intel_engine_cs *engine;
- enum forcewake_domains fw_domains;
-
- engine = intel_engine_setup(dev_priv, id);
-
- /* Intentionally left blank. */
- engine->buffer = NULL;
-
- fw_domains = intel_uncore_forcewake_for_reg(dev_priv,
- RING_ELSP(engine),
- FW_REG_WRITE);
-
- fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
- RING_CONTEXT_STATUS_PTR(engine),
- FW_REG_READ | FW_REG_WRITE);
-
- fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
- RING_CONTEXT_STATUS_BUF_BASE(engine),
- FW_REG_READ);
-
- 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);
-
- intel_engine_init_hangcheck(engine);
- i915_gem_batch_pool_init(dev_priv->dev, &engine->batch_pool);
-
- return engine;
-}
-
/**
* intel_logical_rings_init() - allocate, populate and init the Engine Command Streamers
* @dev: DRM device.
@@ -2237,7 +2242,7 @@ int intel_logical_rings_init(struct drm_device *dev)
if (!intel_engines[i].init)
continue;
- ret = intel_engines[i].init(logical_ring_setup(dev_priv, i));
+ ret = intel_engines[i].init(intel_engine_setup(dev_priv, i));
if (ret)
goto cleanup;
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 3/5] drm/i915: Unify engine init loop
2016-07-01 16:47 [PATCH 1/5] drm/i915: unify first-stage engine struct setup Tvrtko Ursulin
2016-07-01 16:47 ` [PATCH 2/5] drm/i915: Prepare for engine init unification Tvrtko Ursulin
@ 2016-07-01 16:47 ` Tvrtko Ursulin
2016-07-01 16:47 ` [PATCH 4/5] drm/i915: Make more use of the shared engine irq setup Tvrtko Ursulin
` (3 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2016-07-01 16:47 UTC (permalink / raw)
To: Intel-gfx
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
With the unified common engine setup done, and the execlist engine
initialization loop clearly split into two phases, we can eliminate
the separate legacy engine initialization code.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 2 --
drivers/gpu/drm/i915/i915_gem.c | 51 +--------------------------------
drivers/gpu/drm/i915/intel_lrc.c | 25 ++++++++++------
drivers/gpu/drm/i915/intel_lrc.h | 2 +-
drivers/gpu/drm/i915/intel_ringbuffer.c | 47 +++++++++++-------------------
drivers/gpu/drm/i915/intel_ringbuffer.h | 10 +++----
6 files changed, 39 insertions(+), 98 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 485ab1148181..c058cb8150be 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2030,7 +2030,6 @@ struct drm_i915_private {
int (*execbuf_submit)(struct i915_execbuffer_params *params,
struct drm_i915_gem_execbuffer2 *args,
struct list_head *vmas);
- int (*init_engines)(struct drm_device *dev);
void (*cleanup_engine)(struct intel_engine_cs *engine);
void (*stop_engine)(struct intel_engine_cs *engine);
} gt;
@@ -3342,7 +3341,6 @@ static inline bool i915_stop_ring_allow_warn(struct drm_i915_private *dev_priv)
void i915_gem_reset(struct drm_device *dev);
bool i915_gem_clflush_object(struct drm_i915_gem_object *obj, bool force);
int __must_check i915_gem_init(struct drm_device *dev);
-int i915_gem_init_engines(struct drm_device *dev);
int __must_check i915_gem_init_hw(struct drm_device *dev);
void i915_gem_init_swizzling(struct drm_device *dev);
void i915_gem_cleanup_engines(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1d9878258103..3b5c99833ddd 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5029,53 +5029,6 @@ static void init_unused_rings(struct drm_device *dev)
}
}
-int i915_gem_init_engines(struct drm_device *dev)
-{
- struct drm_i915_private *dev_priv = dev->dev_private;
- int ret;
-
- ret = intel_init_render_ring_buffer(dev);
- if (ret)
- return ret;
-
- if (HAS_BSD(dev)) {
- ret = intel_init_bsd_ring_buffer(dev);
- if (ret)
- goto cleanup_render_ring;
- }
-
- if (HAS_BLT(dev)) {
- ret = intel_init_blt_ring_buffer(dev);
- if (ret)
- goto cleanup_bsd_ring;
- }
-
- if (HAS_VEBOX(dev)) {
- ret = intel_init_vebox_ring_buffer(dev);
- if (ret)
- goto cleanup_blt_ring;
- }
-
- if (HAS_BSD2(dev)) {
- ret = intel_init_bsd2_ring_buffer(dev);
- if (ret)
- goto cleanup_vebox_ring;
- }
-
- return 0;
-
-cleanup_vebox_ring:
- intel_cleanup_engine(&dev_priv->engine[VECS]);
-cleanup_blt_ring:
- intel_cleanup_engine(&dev_priv->engine[BCS]);
-cleanup_bsd_ring:
- intel_cleanup_engine(&dev_priv->engine[VCS]);
-cleanup_render_ring:
- intel_cleanup_engine(&dev_priv->engine[RCS]);
-
- return ret;
-}
-
int
i915_gem_init_hw(struct drm_device *dev)
{
@@ -5157,12 +5110,10 @@ int i915_gem_init(struct drm_device *dev)
if (!i915.enable_execlists) {
dev_priv->gt.execbuf_submit = i915_gem_ringbuffer_submission;
- dev_priv->gt.init_engines = i915_gem_init_engines;
dev_priv->gt.cleanup_engine = intel_cleanup_engine;
dev_priv->gt.stop_engine = intel_stop_engine;
} else {
dev_priv->gt.execbuf_submit = intel_execlists_submission;
- dev_priv->gt.init_engines = intel_logical_rings_init;
dev_priv->gt.cleanup_engine = intel_logical_ring_cleanup;
dev_priv->gt.stop_engine = intel_logical_ring_stop;
}
@@ -5182,7 +5133,7 @@ int i915_gem_init(struct drm_device *dev)
if (ret)
goto out_unlock;
- ret = dev_priv->gt.init_engines(dev);
+ ret = intel_engines_init(dev);
if (ret)
goto out_unlock;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index abb165019af7..bf8f4a67c75b 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2153,6 +2153,7 @@ static const struct engine_info {
u32 mmio_base;
unsigned irq_shift;
int (*init)(struct intel_engine_cs *engine);
+ int (*init_ringbuf)(struct intel_engine_cs *engine);
} intel_engines[] = {
[RCS] = {
.name = "render ring",
@@ -2161,6 +2162,7 @@ static const struct engine_info {
.mmio_base = RENDER_RING_BASE,
.irq_shift = GEN8_RCS_IRQ_SHIFT,
.init = logical_render_ring_init,
+ .init_ringbuf = intel_init_render_ring_buffer,
},
[BCS] = {
.name = "blitter ring",
@@ -2169,6 +2171,7 @@ static const struct engine_info {
.mmio_base = BLT_RING_BASE,
.irq_shift = GEN8_BCS_IRQ_SHIFT,
.init = logical_xcs_ring_init,
+ .init_ringbuf = intel_init_blt_ring_buffer,
},
[VCS] = {
.name = "bsd ring",
@@ -2177,6 +2180,7 @@ static const struct engine_info {
.mmio_base = GEN6_BSD_RING_BASE,
.irq_shift = GEN8_VCS1_IRQ_SHIFT,
.init = logical_xcs_ring_init,
+ .init_ringbuf = intel_init_bsd_ring_buffer,
},
[VCS2] = {
.name = "bsd2 ring",
@@ -2185,6 +2189,7 @@ static const struct engine_info {
.mmio_base = GEN8_BSD2_RING_BASE,
.irq_shift = GEN8_VCS2_IRQ_SHIFT,
.init = logical_xcs_ring_init,
+ .init_ringbuf = intel_init_bsd2_ring_buffer,
},
[VECS] = {
.name = "video enhancement ring",
@@ -2193,6 +2198,7 @@ static const struct engine_info {
.mmio_base = VEBOX_RING_BASE,
.irq_shift = GEN8_VECS_IRQ_SHIFT,
.init = logical_xcs_ring_init,
+ .init_ringbuf = intel_init_vebox_ring_buffer,
},
};
@@ -2215,20 +2221,16 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
}
/**
- * intel_logical_rings_init() - allocate, populate and init the Engine Command Streamers
+ * intel_engines_init() - allocate, populate and init the Engine Command Streamers
* @dev: DRM device.
*
- * This function inits the engines for an Execlists submission style (the
- * equivalent in the legacy ringbuffer submission world would be
- * i915_gem_init_engines). It does it only for those engines that are present in
- * the hardware.
- *
* Return: non-zero if the initialization failed.
*/
-int intel_logical_rings_init(struct drm_device *dev)
+int intel_engines_init(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
unsigned int mask = 0;
+ int (*init)(struct intel_engine_cs *engine);
unsigned int i;
int ret;
@@ -2239,10 +2241,15 @@ int intel_logical_rings_init(struct drm_device *dev)
if (!HAS_ENGINE(dev_priv, i))
continue;
- if (!intel_engines[i].init)
+ if (i915.enable_execlists)
+ init = intel_engines[i].init;
+ else
+ init = intel_engines[i].init_ringbuf;
+
+ if (!init)
continue;
- ret = intel_engines[i].init(intel_engine_setup(dev_priv, i));
+ ret = init(intel_engine_setup(dev_priv, i));
if (ret)
goto cleanup;
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 2b8255c19dcc..aa8905c42de7 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -67,7 +67,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
int intel_logical_ring_reserve_space(struct drm_i915_gem_request *request);
void intel_logical_ring_stop(struct intel_engine_cs *engine);
void intel_logical_ring_cleanup(struct intel_engine_cs *engine);
-int intel_logical_rings_init(struct drm_device *dev);
+int intel_engines_init(struct drm_device *dev);
int logical_ring_flush_all_caches(struct drm_i915_gem_request *req);
/**
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 215424efe05c..a00adc3438f3 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -3033,15 +3033,12 @@ static void intel_ring_default_vfuncs(struct drm_i915_private *dev_priv,
intel_ring_init_semaphores(dev_priv, engine);
}
-int intel_init_render_ring_buffer(struct drm_device *dev)
+int intel_init_render_ring_buffer(struct intel_engine_cs *engine)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_engine_cs *engine;
+ struct drm_i915_private *dev_priv = engine->i915;
struct drm_i915_gem_object *obj;
int ret;
- engine = intel_engine_setup(dev_priv, RCS);
-
intel_ring_default_vfuncs(dev_priv, engine);
if (INTEL_GEN(dev_priv) >= 8) {
@@ -3080,7 +3077,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
/* Workaround batchbuffer to combat CS tlb bug. */
if (HAS_BROKEN_CS_TLB(dev_priv)) {
- obj = i915_gem_object_create(dev, I830_WA_SIZE);
+ obj = i915_gem_object_create(dev_priv->dev, I830_WA_SIZE);
if (IS_ERR(obj)) {
DRM_ERROR("Failed to allocate batch bo\n");
return PTR_ERR(obj);
@@ -3097,7 +3094,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
engine->scratch.gtt_offset = i915_gem_obj_ggtt_offset(obj);
}
- ret = intel_init_ring_buffer(dev, engine);
+ ret = intel_init_ring_buffer(dev_priv->dev, engine);
if (ret)
return ret;
@@ -3110,12 +3107,9 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
return 0;
}
-int intel_init_bsd_ring_buffer(struct drm_device *dev)
+int intel_init_bsd_ring_buffer(struct intel_engine_cs *engine)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_engine_cs *engine;
-
- engine = intel_engine_setup(dev_priv, VCS);
+ struct drm_i915_private *dev_priv = engine->i915;
intel_ring_default_vfuncs(dev_priv, engine);
@@ -3138,18 +3132,15 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
engine->irq_enable_mask = I915_BSD_USER_INTERRUPT;
}
- return intel_init_ring_buffer(dev, engine);
+ return intel_init_ring_buffer(dev_priv->dev, engine);
}
/**
* Initialize the second BSD ring (eg. Broadwell GT3, Skylake GT3)
*/
-int intel_init_bsd2_ring_buffer(struct drm_device *dev)
+int intel_init_bsd2_ring_buffer(struct intel_engine_cs *engine)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_engine_cs *engine;
-
- engine = intel_engine_setup(dev_priv, VCS2);
+ struct drm_i915_private *dev_priv = engine->i915;
intel_ring_default_vfuncs(dev_priv, engine);
@@ -3157,15 +3148,12 @@ int intel_init_bsd2_ring_buffer(struct drm_device *dev)
engine->irq_enable_mask =
GT_RENDER_USER_INTERRUPT << GEN8_VCS2_IRQ_SHIFT;
- return intel_init_ring_buffer(dev, engine);
+ return intel_init_ring_buffer(dev_priv->dev, engine);
}
-int intel_init_blt_ring_buffer(struct drm_device *dev)
+int intel_init_blt_ring_buffer(struct intel_engine_cs *engine)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_engine_cs *engine;
-
- engine = intel_engine_setup(dev_priv, BCS);
+ struct drm_i915_private *dev_priv = engine->i915;
intel_ring_default_vfuncs(dev_priv, engine);
@@ -3176,15 +3164,12 @@ int intel_init_blt_ring_buffer(struct drm_device *dev)
else
engine->irq_enable_mask = GT_BLT_USER_INTERRUPT;
- return intel_init_ring_buffer(dev, engine);
+ return intel_init_ring_buffer(dev_priv->dev, engine);
}
-int intel_init_vebox_ring_buffer(struct drm_device *dev)
+int intel_init_vebox_ring_buffer(struct intel_engine_cs *engine)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_engine_cs *engine;
-
- engine = intel_engine_setup(dev_priv, VECS);
+ struct drm_i915_private *dev_priv = engine->i915;
intel_ring_default_vfuncs(dev_priv, engine);
@@ -3199,7 +3184,7 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev)
engine->irq_put = hsw_vebox_put_irq;
}
- return intel_init_ring_buffer(dev, engine);
+ return intel_init_ring_buffer(dev_priv->dev, engine);
}
int
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 1aeb00cba9e2..a843164ab386 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -459,11 +459,11 @@ int intel_ring_invalidate_all_caches(struct drm_i915_gem_request *req);
void intel_fini_pipe_control(struct intel_engine_cs *engine);
int intel_init_pipe_control(struct intel_engine_cs *engine);
-int intel_init_render_ring_buffer(struct drm_device *dev);
-int intel_init_bsd_ring_buffer(struct drm_device *dev);
-int intel_init_bsd2_ring_buffer(struct drm_device *dev);
-int intel_init_blt_ring_buffer(struct drm_device *dev);
-int intel_init_vebox_ring_buffer(struct drm_device *dev);
+int intel_init_render_ring_buffer(struct intel_engine_cs *engine);
+int intel_init_bsd_ring_buffer(struct intel_engine_cs *engine);
+int intel_init_bsd2_ring_buffer(struct intel_engine_cs *engine);
+int intel_init_blt_ring_buffer(struct intel_engine_cs *engine);
+int intel_init_vebox_ring_buffer(struct intel_engine_cs *engine);
u64 intel_ring_get_active_head(struct intel_engine_cs *engine);
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 4/5] drm/i915: Make more use of the shared engine irq setup
2016-07-01 16:47 [PATCH 1/5] drm/i915: unify first-stage engine struct setup Tvrtko Ursulin
2016-07-01 16:47 ` [PATCH 2/5] drm/i915: Prepare for engine init unification Tvrtko Ursulin
2016-07-01 16:47 ` [PATCH 3/5] drm/i915: Unify engine init loop Tvrtko Ursulin
@ 2016-07-01 16:47 ` Tvrtko Ursulin
2016-07-13 12:30 ` Daniel Vetter
2016-07-01 16:47 ` [PATCH 5/5] drm/i915: Simplify intel_init_ring_buffer prototype Tvrtko Ursulin
` (2 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2016-07-01 16:47 UTC (permalink / raw)
To: Intel-gfx
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Use it for legacy engine initialization by adding a
intel_ring_default_irqs helper used by individual engines.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/intel_ringbuffer.c | 36 +++++++++++++++------------------
1 file changed, 16 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index a00adc3438f3..964776bb181c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2394,8 +2394,6 @@ static int intel_init_ring_buffer(struct drm_device *dev,
memset(engine->semaphore.sync_seqno, 0,
sizeof(engine->semaphore.sync_seqno));
- init_waitqueue_head(&engine->irq_queue);
-
/* We may need to do things with the shrinker which
* require us to immediately switch back to the default
* context. This can cause a problem as pinning the
@@ -3033,6 +3031,13 @@ static void intel_ring_default_vfuncs(struct drm_i915_private *dev_priv,
intel_ring_init_semaphores(dev_priv, engine);
}
+static void
+intel_ring_default_irqs(struct intel_engine_cs *engine)
+{
+ engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << engine->irq_shift;
+ init_waitqueue_head(&engine->irq_queue);
+}
+
int intel_init_render_ring_buffer(struct intel_engine_cs *engine)
{
struct drm_i915_private *dev_priv = engine->i915;
@@ -3040,12 +3045,12 @@ int intel_init_render_ring_buffer(struct intel_engine_cs *engine)
int ret;
intel_ring_default_vfuncs(dev_priv, engine);
+ intel_ring_default_irqs(engine);
if (INTEL_GEN(dev_priv) >= 8) {
engine->init_context = intel_rcs_ctx_init;
engine->add_request = gen8_render_add_request;
engine->flush = gen8_render_ring_flush;
- engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT;
if (i915_semaphore_is_enabled(dev_priv))
engine->semaphore.signal = gen8_rcs_signal;
} else if (INTEL_GEN(dev_priv) >= 6) {
@@ -3053,14 +3058,12 @@ int intel_init_render_ring_buffer(struct intel_engine_cs *engine)
engine->flush = gen7_render_ring_flush;
if (IS_GEN6(dev_priv))
engine->flush = gen6_render_ring_flush;
- engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT;
} else if (IS_GEN5(dev_priv)) {
engine->add_request = pc_render_add_request;
engine->flush = gen4_render_ring_flush;
engine->get_seqno = pc_render_get_seqno;
engine->set_seqno = pc_render_set_seqno;
- engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT |
- GT_RENDER_PIPECTL_NOTIFY_INTERRUPT;
+ engine->irq_enable_mask |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT;
} else {
if (INTEL_GEN(dev_priv) < 4)
engine->flush = gen2_render_ring_flush;
@@ -3112,16 +3115,14 @@ int intel_init_bsd_ring_buffer(struct intel_engine_cs *engine)
struct drm_i915_private *dev_priv = engine->i915;
intel_ring_default_vfuncs(dev_priv, engine);
+ intel_ring_default_irqs(engine);
if (INTEL_GEN(dev_priv) >= 6) {
/* gen6 bsd needs a special wa for tail updates */
if (IS_GEN6(dev_priv))
engine->write_tail = gen6_bsd_ring_write_tail;
engine->flush = gen6_bsd_ring_flush;
- if (INTEL_GEN(dev_priv) >= 8)
- engine->irq_enable_mask =
- GT_RENDER_USER_INTERRUPT << GEN8_VCS1_IRQ_SHIFT;
- else
+ if (INTEL_GEN(dev_priv) < 8)
engine->irq_enable_mask = GT_BSD_USER_INTERRUPT;
} else {
engine->mmio_base = BSD_RING_BASE;
@@ -3143,10 +3144,9 @@ int intel_init_bsd2_ring_buffer(struct intel_engine_cs *engine)
struct drm_i915_private *dev_priv = engine->i915;
intel_ring_default_vfuncs(dev_priv, engine);
+ intel_ring_default_irqs(engine);
engine->flush = gen6_bsd_ring_flush;
- engine->irq_enable_mask =
- GT_RENDER_USER_INTERRUPT << GEN8_VCS2_IRQ_SHIFT;
return intel_init_ring_buffer(dev_priv->dev, engine);
}
@@ -3156,12 +3156,10 @@ int intel_init_blt_ring_buffer(struct intel_engine_cs *engine)
struct drm_i915_private *dev_priv = engine->i915;
intel_ring_default_vfuncs(dev_priv, engine);
+ intel_ring_default_irqs(engine);
engine->flush = gen6_ring_flush;
- if (INTEL_GEN(dev_priv) >= 8)
- engine->irq_enable_mask =
- GT_RENDER_USER_INTERRUPT << GEN8_BCS_IRQ_SHIFT;
- else
+ if (INTEL_GEN(dev_priv) < 8)
engine->irq_enable_mask = GT_BLT_USER_INTERRUPT;
return intel_init_ring_buffer(dev_priv->dev, engine);
@@ -3172,13 +3170,11 @@ int intel_init_vebox_ring_buffer(struct intel_engine_cs *engine)
struct drm_i915_private *dev_priv = engine->i915;
intel_ring_default_vfuncs(dev_priv, engine);
+ intel_ring_default_irqs(engine);
engine->flush = gen6_ring_flush;
- if (INTEL_GEN(dev_priv) >= 8) {
- engine->irq_enable_mask =
- GT_RENDER_USER_INTERRUPT << GEN8_VECS_IRQ_SHIFT;
- } else {
+ if (INTEL_GEN(dev_priv) < 8) {
engine->irq_enable_mask = PM_VEBOX_USER_INTERRUPT;
engine->irq_get = hsw_vebox_get_irq;
engine->irq_put = hsw_vebox_put_irq;
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 4/5] drm/i915: Make more use of the shared engine irq setup
2016-07-01 16:47 ` [PATCH 4/5] drm/i915: Make more use of the shared engine irq setup Tvrtko Ursulin
@ 2016-07-13 12:30 ` Daniel Vetter
2016-07-13 13:19 ` Tvrtko Ursulin
0 siblings, 1 reply; 15+ messages in thread
From: Daniel Vetter @ 2016-07-13 12:30 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
On Fri, Jul 01, 2016 at 05:47:14PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Use it for legacy engine initialization by adding a
> intel_ring_default_irqs helper used by individual engines.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Not sure this is worth it. irq handling is fairly gen specific, and the
overlap between lrc and legacy rings is just gen8. For preceeding two
patches 2&3:
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/i915/intel_ringbuffer.c | 36 +++++++++++++++------------------
> 1 file changed, 16 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index a00adc3438f3..964776bb181c 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2394,8 +2394,6 @@ static int intel_init_ring_buffer(struct drm_device *dev,
> memset(engine->semaphore.sync_seqno, 0,
> sizeof(engine->semaphore.sync_seqno));
>
> - init_waitqueue_head(&engine->irq_queue);
> -
> /* We may need to do things with the shrinker which
> * require us to immediately switch back to the default
> * context. This can cause a problem as pinning the
> @@ -3033,6 +3031,13 @@ static void intel_ring_default_vfuncs(struct drm_i915_private *dev_priv,
> intel_ring_init_semaphores(dev_priv, engine);
> }
>
> +static void
> +intel_ring_default_irqs(struct intel_engine_cs *engine)
> +{
> + engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << engine->irq_shift;
> + init_waitqueue_head(&engine->irq_queue);
> +}
> +
> int intel_init_render_ring_buffer(struct intel_engine_cs *engine)
> {
> struct drm_i915_private *dev_priv = engine->i915;
> @@ -3040,12 +3045,12 @@ int intel_init_render_ring_buffer(struct intel_engine_cs *engine)
> int ret;
>
> intel_ring_default_vfuncs(dev_priv, engine);
> + intel_ring_default_irqs(engine);
>
> if (INTEL_GEN(dev_priv) >= 8) {
> engine->init_context = intel_rcs_ctx_init;
> engine->add_request = gen8_render_add_request;
> engine->flush = gen8_render_ring_flush;
> - engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT;
> if (i915_semaphore_is_enabled(dev_priv))
> engine->semaphore.signal = gen8_rcs_signal;
> } else if (INTEL_GEN(dev_priv) >= 6) {
> @@ -3053,14 +3058,12 @@ int intel_init_render_ring_buffer(struct intel_engine_cs *engine)
> engine->flush = gen7_render_ring_flush;
> if (IS_GEN6(dev_priv))
> engine->flush = gen6_render_ring_flush;
> - engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT;
> } else if (IS_GEN5(dev_priv)) {
> engine->add_request = pc_render_add_request;
> engine->flush = gen4_render_ring_flush;
> engine->get_seqno = pc_render_get_seqno;
> engine->set_seqno = pc_render_set_seqno;
> - engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT |
> - GT_RENDER_PIPECTL_NOTIFY_INTERRUPT;
> + engine->irq_enable_mask |= GT_RENDER_PIPECTL_NOTIFY_INTERRUPT;
> } else {
> if (INTEL_GEN(dev_priv) < 4)
> engine->flush = gen2_render_ring_flush;
> @@ -3112,16 +3115,14 @@ int intel_init_bsd_ring_buffer(struct intel_engine_cs *engine)
> struct drm_i915_private *dev_priv = engine->i915;
>
> intel_ring_default_vfuncs(dev_priv, engine);
> + intel_ring_default_irqs(engine);
>
> if (INTEL_GEN(dev_priv) >= 6) {
> /* gen6 bsd needs a special wa for tail updates */
> if (IS_GEN6(dev_priv))
> engine->write_tail = gen6_bsd_ring_write_tail;
> engine->flush = gen6_bsd_ring_flush;
> - if (INTEL_GEN(dev_priv) >= 8)
> - engine->irq_enable_mask =
> - GT_RENDER_USER_INTERRUPT << GEN8_VCS1_IRQ_SHIFT;
> - else
> + if (INTEL_GEN(dev_priv) < 8)
> engine->irq_enable_mask = GT_BSD_USER_INTERRUPT;
> } else {
> engine->mmio_base = BSD_RING_BASE;
> @@ -3143,10 +3144,9 @@ int intel_init_bsd2_ring_buffer(struct intel_engine_cs *engine)
> struct drm_i915_private *dev_priv = engine->i915;
>
> intel_ring_default_vfuncs(dev_priv, engine);
> + intel_ring_default_irqs(engine);
>
> engine->flush = gen6_bsd_ring_flush;
> - engine->irq_enable_mask =
> - GT_RENDER_USER_INTERRUPT << GEN8_VCS2_IRQ_SHIFT;
>
> return intel_init_ring_buffer(dev_priv->dev, engine);
> }
> @@ -3156,12 +3156,10 @@ int intel_init_blt_ring_buffer(struct intel_engine_cs *engine)
> struct drm_i915_private *dev_priv = engine->i915;
>
> intel_ring_default_vfuncs(dev_priv, engine);
> + intel_ring_default_irqs(engine);
>
> engine->flush = gen6_ring_flush;
> - if (INTEL_GEN(dev_priv) >= 8)
> - engine->irq_enable_mask =
> - GT_RENDER_USER_INTERRUPT << GEN8_BCS_IRQ_SHIFT;
> - else
> + if (INTEL_GEN(dev_priv) < 8)
> engine->irq_enable_mask = GT_BLT_USER_INTERRUPT;
>
> return intel_init_ring_buffer(dev_priv->dev, engine);
> @@ -3172,13 +3170,11 @@ int intel_init_vebox_ring_buffer(struct intel_engine_cs *engine)
> struct drm_i915_private *dev_priv = engine->i915;
>
> intel_ring_default_vfuncs(dev_priv, engine);
> + intel_ring_default_irqs(engine);
>
> engine->flush = gen6_ring_flush;
>
> - if (INTEL_GEN(dev_priv) >= 8) {
> - engine->irq_enable_mask =
> - GT_RENDER_USER_INTERRUPT << GEN8_VECS_IRQ_SHIFT;
> - } else {
> + if (INTEL_GEN(dev_priv) < 8) {
> engine->irq_enable_mask = PM_VEBOX_USER_INTERRUPT;
> engine->irq_get = hsw_vebox_get_irq;
> engine->irq_put = hsw_vebox_put_irq;
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
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] 15+ messages in thread* Re: [PATCH 4/5] drm/i915: Make more use of the shared engine irq setup
2016-07-13 12:30 ` Daniel Vetter
@ 2016-07-13 13:19 ` Tvrtko Ursulin
0 siblings, 0 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2016-07-13 13:19 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel-gfx
On 13/07/16 13:30, Daniel Vetter wrote:
> On Fri, Jul 01, 2016 at 05:47:14PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Use it for legacy engine initialization by adding a
>> intel_ring_default_irqs helper used by individual engines.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Not sure this is worth it. irq handling is fairly gen specific, and the
> overlap between lrc and legacy rings is just gen8. For preceeding two
In the latest rebase it is:
1 file changed, 5 insertions(+), 15 deletions(-)
So it does manage to use the shared static state significantly. So on
the basis of that vs the rebase work dropping this one would cause,
maybe change your mind? :)
> patches 2&3:
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Thanks!
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 5/5] drm/i915: Simplify intel_init_ring_buffer prototype
2016-07-01 16:47 [PATCH 1/5] drm/i915: unify first-stage engine struct setup Tvrtko Ursulin
` (2 preceding siblings ...)
2016-07-01 16:47 ` [PATCH 4/5] drm/i915: Make more use of the shared engine irq setup Tvrtko Ursulin
@ 2016-07-01 16:47 ` Tvrtko Ursulin
2016-07-13 12:30 ` Daniel Vetter
2016-07-01 17:08 ` ✗ Ro.CI.BAT: warning for series starting with [1/5] drm/i915: unify first-stage engine struct setup Patchwork
2016-07-13 12:23 ` [PATCH 1/5] " Daniel Vetter
5 siblings, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2016-07-01 16:47 UTC (permalink / raw)
To: Intel-gfx
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Engine contains dev_priv so need to pass it in.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/intel_ringbuffer.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 964776bb181c..760c6b53f063 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2376,21 +2376,19 @@ static void intel_ring_context_unpin(struct i915_gem_context *ctx,
i915_gem_context_unreference(ctx);
}
-static int intel_init_ring_buffer(struct drm_device *dev,
- struct intel_engine_cs *engine)
+static int intel_init_ring_buffer(struct intel_engine_cs *engine)
{
- struct drm_i915_private *dev_priv = to_i915(dev);
+ struct drm_i915_private *dev_priv = engine->i915;
struct intel_ringbuffer *ringbuf;
int ret;
WARN_ON(engine->buffer);
- engine->i915 = dev_priv;
INIT_LIST_HEAD(&engine->active_list);
INIT_LIST_HEAD(&engine->request_list);
INIT_LIST_HEAD(&engine->execlist_queue);
INIT_LIST_HEAD(&engine->buffers);
- i915_gem_batch_pool_init(dev, &engine->batch_pool);
+ i915_gem_batch_pool_init(dev_priv->dev, &engine->batch_pool);
memset(engine->semaphore.sync_seqno, 0,
sizeof(engine->semaphore.sync_seqno));
@@ -3097,7 +3095,7 @@ int intel_init_render_ring_buffer(struct intel_engine_cs *engine)
engine->scratch.gtt_offset = i915_gem_obj_ggtt_offset(obj);
}
- ret = intel_init_ring_buffer(dev_priv->dev, engine);
+ ret = intel_init_ring_buffer(engine);
if (ret)
return ret;
@@ -3133,7 +3131,7 @@ int intel_init_bsd_ring_buffer(struct intel_engine_cs *engine)
engine->irq_enable_mask = I915_BSD_USER_INTERRUPT;
}
- return intel_init_ring_buffer(dev_priv->dev, engine);
+ return intel_init_ring_buffer(engine);
}
/**
@@ -3148,7 +3146,7 @@ int intel_init_bsd2_ring_buffer(struct intel_engine_cs *engine)
engine->flush = gen6_bsd_ring_flush;
- return intel_init_ring_buffer(dev_priv->dev, engine);
+ return intel_init_ring_buffer(engine);
}
int intel_init_blt_ring_buffer(struct intel_engine_cs *engine)
@@ -3162,7 +3160,7 @@ int intel_init_blt_ring_buffer(struct intel_engine_cs *engine)
if (INTEL_GEN(dev_priv) < 8)
engine->irq_enable_mask = GT_BLT_USER_INTERRUPT;
- return intel_init_ring_buffer(dev_priv->dev, engine);
+ return intel_init_ring_buffer(engine);
}
int intel_init_vebox_ring_buffer(struct intel_engine_cs *engine)
@@ -3180,7 +3178,7 @@ int intel_init_vebox_ring_buffer(struct intel_engine_cs *engine)
engine->irq_put = hsw_vebox_put_irq;
}
- return intel_init_ring_buffer(dev_priv->dev, engine);
+ return intel_init_ring_buffer(engine);
}
int
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 5/5] drm/i915: Simplify intel_init_ring_buffer prototype
2016-07-01 16:47 ` [PATCH 5/5] drm/i915: Simplify intel_init_ring_buffer prototype Tvrtko Ursulin
@ 2016-07-13 12:30 ` Daniel Vetter
0 siblings, 0 replies; 15+ messages in thread
From: Daniel Vetter @ 2016-07-13 12:30 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
On Fri, Jul 01, 2016 at 05:47:15PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Engine contains dev_priv so need to pass it in.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/i915/intel_ringbuffer.c | 18 ++++++++----------
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 964776bb181c..760c6b53f063 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2376,21 +2376,19 @@ static void intel_ring_context_unpin(struct i915_gem_context *ctx,
> i915_gem_context_unreference(ctx);
> }
>
> -static int intel_init_ring_buffer(struct drm_device *dev,
> - struct intel_engine_cs *engine)
> +static int intel_init_ring_buffer(struct intel_engine_cs *engine)
> {
> - struct drm_i915_private *dev_priv = to_i915(dev);
> + struct drm_i915_private *dev_priv = engine->i915;
> struct intel_ringbuffer *ringbuf;
> int ret;
>
> WARN_ON(engine->buffer);
>
> - engine->i915 = dev_priv;
> INIT_LIST_HEAD(&engine->active_list);
> INIT_LIST_HEAD(&engine->request_list);
> INIT_LIST_HEAD(&engine->execlist_queue);
> INIT_LIST_HEAD(&engine->buffers);
> - i915_gem_batch_pool_init(dev, &engine->batch_pool);
> + i915_gem_batch_pool_init(dev_priv->dev, &engine->batch_pool);
> memset(engine->semaphore.sync_seqno, 0,
> sizeof(engine->semaphore.sync_seqno));
>
> @@ -3097,7 +3095,7 @@ int intel_init_render_ring_buffer(struct intel_engine_cs *engine)
> engine->scratch.gtt_offset = i915_gem_obj_ggtt_offset(obj);
> }
>
> - ret = intel_init_ring_buffer(dev_priv->dev, engine);
> + ret = intel_init_ring_buffer(engine);
> if (ret)
> return ret;
>
> @@ -3133,7 +3131,7 @@ int intel_init_bsd_ring_buffer(struct intel_engine_cs *engine)
> engine->irq_enable_mask = I915_BSD_USER_INTERRUPT;
> }
>
> - return intel_init_ring_buffer(dev_priv->dev, engine);
> + return intel_init_ring_buffer(engine);
> }
>
> /**
> @@ -3148,7 +3146,7 @@ int intel_init_bsd2_ring_buffer(struct intel_engine_cs *engine)
>
> engine->flush = gen6_bsd_ring_flush;
>
> - return intel_init_ring_buffer(dev_priv->dev, engine);
> + return intel_init_ring_buffer(engine);
> }
>
> int intel_init_blt_ring_buffer(struct intel_engine_cs *engine)
> @@ -3162,7 +3160,7 @@ int intel_init_blt_ring_buffer(struct intel_engine_cs *engine)
> if (INTEL_GEN(dev_priv) < 8)
> engine->irq_enable_mask = GT_BLT_USER_INTERRUPT;
>
> - return intel_init_ring_buffer(dev_priv->dev, engine);
> + return intel_init_ring_buffer(engine);
> }
>
> int intel_init_vebox_ring_buffer(struct intel_engine_cs *engine)
> @@ -3180,7 +3178,7 @@ int intel_init_vebox_ring_buffer(struct intel_engine_cs *engine)
> engine->irq_put = hsw_vebox_put_irq;
> }
>
> - return intel_init_ring_buffer(dev_priv->dev, engine);
> + return intel_init_ring_buffer(engine);
> }
>
> int
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
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] 15+ messages in thread
* ✗ Ro.CI.BAT: warning for series starting with [1/5] drm/i915: unify first-stage engine struct setup
2016-07-01 16:47 [PATCH 1/5] drm/i915: unify first-stage engine struct setup Tvrtko Ursulin
` (3 preceding siblings ...)
2016-07-01 16:47 ` [PATCH 5/5] drm/i915: Simplify intel_init_ring_buffer prototype Tvrtko Ursulin
@ 2016-07-01 17:08 ` Patchwork
2016-07-13 12:23 ` [PATCH 1/5] " Daniel Vetter
5 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2016-07-01 17:08 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/5] drm/i915: unify first-stage engine struct setup
URL : https://patchwork.freedesktop.org/series/9391/
State : warning
== Summary ==
Series 9391v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/9391/revisions/1/mbox
Test kms_pipe_crc_basic:
Subgroup nonblocking-crc-pipe-c:
pass -> SKIP (fi-skl-i5-6260u)
Subgroup suspend-read-crc-pipe-a:
skip -> DMESG-WARN (ro-bdw-i5-5250u)
Subgroup suspend-read-crc-pipe-c:
dmesg-warn -> SKIP (ro-bdw-i5-5250u)
skip -> PASS (fi-skl-i5-6260u)
fi-hsw-i7-4770k total:229 pass:196 dwarn:0 dfail:0 fail:0 skip:33
fi-kbl-qkkr total:229 pass:160 dwarn:28 dfail:1 fail:0 skip:40
fi-skl-i5-6260u total:229 pass:203 dwarn:0 dfail:0 fail:0 skip:26
fi-skl-i7-6700k total:229 pass:190 dwarn:0 dfail:0 fail:0 skip:39
ro-bdw-i5-5250u total:229 pass:204 dwarn:3 dfail:1 fail:0 skip:21
ro-bdw-i7-5557U total:229 pass:204 dwarn:1 dfail:1 fail:0 skip:23
ro-bdw-i7-5600u total:229 pass:190 dwarn:0 dfail:1 fail:0 skip:38
ro-bsw-n3050 total:229 pass:176 dwarn:0 dfail:1 fail:3 skip:49
ro-byt-n2820 total:229 pass:180 dwarn:0 dfail:1 fail:3 skip:45
ro-hsw-i3-4010u total:229 pass:197 dwarn:0 dfail:1 fail:0 skip:31
ro-hsw-i7-4770r total:229 pass:197 dwarn:0 dfail:1 fail:0 skip:31
ro-ilk-i7-620lm total:229 pass:157 dwarn:0 dfail:1 fail:1 skip:70
ro-ilk1-i5-650 total:224 pass:157 dwarn:0 dfail:1 fail:1 skip:65
ro-ivb-i7-3770 total:229 pass:188 dwarn:0 dfail:1 fail:0 skip:40
ro-skl3-i5-6260u total:229 pass:208 dwarn:1 dfail:1 fail:0 skip:19
ro-snb-i7-2620M total:229 pass:179 dwarn:0 dfail:1 fail:1 skip:48
Results at /archive/results/CI_IGT_test/RO_Patchwork_1368/
a755d6c drm-intel-nightly: 2016y-07m-01d-13h-54m-24s UTC integration manifest
b43d558 drm/i915: Simplify intel_init_ring_buffer prototype
845bf70 drm/i915: Make more use of the shared engine irq setup
d9744b9 drm/i915: Unify engine init loop
2df5cf5 drm/i915: Prepare for engine init unification
b8131fa drm/i915: unify first-stage engine struct setup
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/5] drm/i915: unify first-stage engine struct setup
2016-07-01 16:47 [PATCH 1/5] drm/i915: unify first-stage engine struct setup Tvrtko Ursulin
` (4 preceding siblings ...)
2016-07-01 17:08 ` ✗ Ro.CI.BAT: warning for series starting with [1/5] drm/i915: unify first-stage engine struct setup Patchwork
@ 2016-07-13 12:23 ` Daniel Vetter
2016-07-13 13:16 ` Tvrtko Ursulin
5 siblings, 1 reply; 15+ messages in thread
From: Daniel Vetter @ 2016-07-13 12:23 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
On Fri, Jul 01, 2016 at 05:47:11PM +0100, Tvrtko Ursulin wrote:
> From: Dave Gordon <david.s.gordon@intel.com>
>
> intel_lrc.c has a table of "logical rings" (meaning engines), while
> intel_ringbuffer.c has separately open-coded initialisation for each
> engine. We can deduplicate this somewhat by using the same first-stage
> engine-setup function for both modes.
>
> So here we expose the function that transfers information from the
> static table of (all) known engines to the dev_priv->engine array of
> engines available on this device (adjusting the names along the way)
> and then embed calls to it in both the LRC and the legacy-mode setup.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
> drivers/gpu/drm/i915/intel_lrc.c | 40 +++++++++++++++++++++------------
> drivers/gpu/drm/i915/intel_ringbuffer.c | 40 +++++++++------------------------
> drivers/gpu/drm/i915/intel_ringbuffer.h | 5 +++++
> 3 files changed, 41 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 339d8041075f..ed017f1a07a2 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1994,8 +1994,9 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine)
> }
>
> static inline void
> -logical_ring_default_irqs(struct intel_engine_cs *engine, unsigned shift)
> +logical_ring_default_irqs(struct intel_engine_cs *engine)
> {
> + unsigned shift = engine->irq_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);
> @@ -2096,14 +2097,14 @@ static int logical_render_ring_init(struct intel_engine_cs *engine)
> return ret;
> }
>
> -static const struct logical_ring_info {
> +static const struct engine_info {
> const char *name;
> unsigned exec_id;
> unsigned guc_id;
> u32 mmio_base;
> unsigned irq_shift;
> int (*init)(struct intel_engine_cs *engine);
> -} logical_rings[] = {
> +} intel_engines[] = {
> [RCS] = {
> .name = "render ring",
> .exec_id = I915_EXEC_RENDER,
> @@ -2146,20 +2147,31 @@ static const struct logical_ring_info {
> },
> };
>
> -static struct intel_engine_cs *
> -logical_ring_setup(struct drm_i915_private *dev_priv, enum intel_engine_id id)
> +struct intel_engine_cs *
> +intel_engine_setup(struct drm_i915_private *dev_priv,
> + enum intel_engine_id id)
Kerneldoc for this would be nice. Also, we now have a mess between
intel_lrc.c and intel_ringbuffer.c. Extracting intel_engine.c with the
shared bits or something similar, plus cleanup up all the docs would be
awesome as a follow up.
With the kerneldoc added:
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> {
> - const struct logical_ring_info *info = &logical_rings[id];
> + const struct engine_info *info = &intel_engines[id];
> struct intel_engine_cs *engine = &dev_priv->engine[id];
> - enum forcewake_domains fw_domains;
>
> engine->id = id;
> + engine->i915 = dev_priv;
> engine->name = info->name;
> engine->exec_id = info->exec_id;
> - engine->guc_id = info->guc_id;
> + engine->hw_id = engine->guc_id = info->guc_id;
Optional bikeshed: s/info->guc_id/info->hw_id/ makes sense imo in the new
context. Or nuking engine->guc_id.
> engine->mmio_base = info->mmio_base;
> + engine->irq_shift = info->irq_shift;
>
> - engine->i915 = dev_priv;
> + return engine;
> +}
> +
> +static struct intel_engine_cs *
> +logical_ring_setup(struct drm_i915_private *dev_priv, enum intel_engine_id id)
> +{
> + struct intel_engine_cs *engine;
> + enum forcewake_domains fw_domains;
> +
> + engine = intel_engine_setup(dev_priv, id);
>
> /* Intentionally left blank. */
> engine->buffer = NULL;
> @@ -2189,7 +2201,7 @@ logical_ring_setup(struct drm_i915_private *dev_priv, enum intel_engine_id id)
>
> logical_ring_init_platform_invariants(engine);
> logical_ring_default_vfuncs(engine);
> - logical_ring_default_irqs(engine, info->irq_shift);
> + logical_ring_default_irqs(engine);
>
> intel_engine_init_hangcheck(engine);
> i915_gem_batch_pool_init(dev_priv->dev, &engine->batch_pool);
> @@ -2218,14 +2230,14 @@ int intel_logical_rings_init(struct drm_device *dev)
> WARN_ON(INTEL_INFO(dev_priv)->ring_mask &
> GENMASK(sizeof(mask) * BITS_PER_BYTE - 1, I915_NUM_ENGINES));
>
> - for (i = 0; i < ARRAY_SIZE(logical_rings); i++) {
> + for (i = 0; i < ARRAY_SIZE(intel_engines); i++) {
> if (!HAS_ENGINE(dev_priv, i))
> continue;
>
> - if (!logical_rings[i].init)
> + if (!intel_engines[i].init)
> continue;
>
> - ret = logical_rings[i].init(logical_ring_setup(dev_priv, i));
> + ret = intel_engines[i].init(logical_ring_setup(dev_priv, i));
> if (ret)
> goto cleanup;
>
> @@ -2233,7 +2245,7 @@ int intel_logical_rings_init(struct drm_device *dev)
> }
>
> /*
> - * Catch failures to update logical_rings table when the new engines
> + * Catch failures to update intel_engines table when the new engines
> * are added to the driver by a warning and disabling the forgotten
> * engines.
> */
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 24cdc920f4b4..215424efe05c 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -3036,15 +3036,11 @@ static void intel_ring_default_vfuncs(struct drm_i915_private *dev_priv,
> int intel_init_render_ring_buffer(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;
> struct drm_i915_gem_object *obj;
> int ret;
>
> - engine->name = "render ring";
> - engine->id = RCS;
> - engine->exec_id = I915_EXEC_RENDER;
> - engine->hw_id = 0;
> - engine->mmio_base = RENDER_RING_BASE;
> + engine = intel_engine_setup(dev_priv, RCS);
>
> intel_ring_default_vfuncs(dev_priv, engine);
>
> @@ -3117,17 +3113,13 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
> int intel_init_bsd_ring_buffer(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_engine_cs *engine = &dev_priv->engine[VCS];
> + struct intel_engine_cs *engine;
>
> - engine->name = "bsd ring";
> - engine->id = VCS;
> - engine->exec_id = I915_EXEC_BSD;
> - engine->hw_id = 1;
> + engine = intel_engine_setup(dev_priv, VCS);
>
> intel_ring_default_vfuncs(dev_priv, engine);
>
> if (INTEL_GEN(dev_priv) >= 6) {
> - engine->mmio_base = GEN6_BSD_RING_BASE;
> /* gen6 bsd needs a special wa for tail updates */
> if (IS_GEN6(dev_priv))
> engine->write_tail = gen6_bsd_ring_write_tail;
> @@ -3155,13 +3147,9 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
> int intel_init_bsd2_ring_buffer(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_engine_cs *engine = &dev_priv->engine[VCS2];
> + struct intel_engine_cs *engine;
>
> - engine->name = "bsd2 ring";
> - engine->id = VCS2;
> - engine->exec_id = I915_EXEC_BSD;
> - engine->hw_id = 4;
> - engine->mmio_base = GEN8_BSD2_RING_BASE;
> + engine = intel_engine_setup(dev_priv, VCS2);
>
> intel_ring_default_vfuncs(dev_priv, engine);
>
> @@ -3175,13 +3163,9 @@ int intel_init_bsd2_ring_buffer(struct drm_device *dev)
> int intel_init_blt_ring_buffer(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_engine_cs *engine = &dev_priv->engine[BCS];
> + struct intel_engine_cs *engine;
>
> - engine->name = "blitter ring";
> - engine->id = BCS;
> - engine->exec_id = I915_EXEC_BLT;
> - engine->hw_id = 2;
> - engine->mmio_base = BLT_RING_BASE;
> + engine = intel_engine_setup(dev_priv, BCS);
>
> intel_ring_default_vfuncs(dev_priv, engine);
>
> @@ -3198,13 +3182,9 @@ int intel_init_blt_ring_buffer(struct drm_device *dev)
> int intel_init_vebox_ring_buffer(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_engine_cs *engine = &dev_priv->engine[VECS];
> + struct intel_engine_cs *engine;
>
> - engine->name = "video enhancement ring";
> - engine->id = VECS;
> - engine->exec_id = I915_EXEC_VEBOX;
> - engine->hw_id = 3;
> - engine->mmio_base = VEBOX_RING_BASE;
> + engine = intel_engine_setup(dev_priv, VECS);
>
> intel_ring_default_vfuncs(dev_priv, engine);
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 113d5230a6de..1aeb00cba9e2 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -145,6 +145,7 @@ struct intel_engine_cs {
> unsigned int hw_id;
> unsigned int guc_id; /* XXX same as hw_id? */
> u32 mmio_base;
> + unsigned int irq_shift;
> struct intel_ringbuffer *buffer;
> struct list_head buffers;
>
> @@ -335,6 +336,10 @@ struct intel_engine_cs {
> u32 (*get_cmd_length_mask)(u32 cmd_header);
> };
>
> +struct intel_engine_cs *
> +intel_engine_setup(struct drm_i915_private *dev_priv,
> + enum intel_engine_id id);
> +
> static inline bool
> intel_engine_initialized(struct intel_engine_cs *engine)
> {
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
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] 15+ messages in thread* Re: [PATCH 1/5] drm/i915: unify first-stage engine struct setup
2016-07-13 12:23 ` [PATCH 1/5] " Daniel Vetter
@ 2016-07-13 13:16 ` Tvrtko Ursulin
2016-07-13 13:24 ` Tvrtko Ursulin
2016-07-14 11:33 ` Dave Gordon
0 siblings, 2 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2016-07-13 13:16 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel-gfx
On 13/07/16 13:23, Daniel Vetter wrote:
> On Fri, Jul 01, 2016 at 05:47:11PM +0100, Tvrtko Ursulin wrote:
>> From: Dave Gordon <david.s.gordon@intel.com>
>>
>> intel_lrc.c has a table of "logical rings" (meaning engines), while
>> intel_ringbuffer.c has separately open-coded initialisation for each
>> engine. We can deduplicate this somewhat by using the same first-stage
>> engine-setup function for both modes.
>>
>> So here we expose the function that transfers information from the
>> static table of (all) known engines to the dev_priv->engine array of
>> engines available on this device (adjusting the names along the way)
>> and then embed calls to it in both the LRC and the legacy-mode setup.
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_lrc.c | 40 +++++++++++++++++++++------------
>> drivers/gpu/drm/i915/intel_ringbuffer.c | 40 +++++++++------------------------
>> drivers/gpu/drm/i915/intel_ringbuffer.h | 5 +++++
>> 3 files changed, 41 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index 339d8041075f..ed017f1a07a2 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -1994,8 +1994,9 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine)
>> }
>>
>> static inline void
>> -logical_ring_default_irqs(struct intel_engine_cs *engine, unsigned shift)
>> +logical_ring_default_irqs(struct intel_engine_cs *engine)
>> {
>> + unsigned shift = engine->irq_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);
>> @@ -2096,14 +2097,14 @@ static int logical_render_ring_init(struct intel_engine_cs *engine)
>> return ret;
>> }
>>
>> -static const struct logical_ring_info {
>> +static const struct engine_info {
>> const char *name;
>> unsigned exec_id;
>> unsigned guc_id;
>> u32 mmio_base;
>> unsigned irq_shift;
>> int (*init)(struct intel_engine_cs *engine);
>> -} logical_rings[] = {
>> +} intel_engines[] = {
>> [RCS] = {
>> .name = "render ring",
>> .exec_id = I915_EXEC_RENDER,
>> @@ -2146,20 +2147,31 @@ static const struct logical_ring_info {
>> },
>> };
>>
>> -static struct intel_engine_cs *
>> -logical_ring_setup(struct drm_i915_private *dev_priv, enum intel_engine_id id)
>> +struct intel_engine_cs *
>> +intel_engine_setup(struct drm_i915_private *dev_priv,
>> + enum intel_engine_id id)
>
> Kerneldoc for this would be nice. Also, we now have a mess between
> intel_lrc.c and intel_ringbuffer.c. Extracting intel_engine.c with the
Yes that was already suggested by Chris and I am trybotting the
additions today, again on his explicit request to progress this series.
> shared bits or something similar, plus cleanup up all the docs would be
> awesome as a follow up.
What do you mean "all the docs"? :D
>
> With the kerneldoc added:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Thanks, will add.
>> {
>> - const struct logical_ring_info *info = &logical_rings[id];
>> + const struct engine_info *info = &intel_engines[id];
>> struct intel_engine_cs *engine = &dev_priv->engine[id];
>> - enum forcewake_domains fw_domains;
>>
>> engine->id = id;
>> + engine->i915 = dev_priv;
>> engine->name = info->name;
>> engine->exec_id = info->exec_id;
>> - engine->guc_id = info->guc_id;
>> + engine->hw_id = engine->guc_id = info->guc_id;
>
> Optional bikeshed: s/info->guc_id/info->hw_id/ makes sense imo in the new
> context. Or nuking engine->guc_id.
Someone said we cannot be sure they will be the same in the future. So
maybe just rename to hw_id for now.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/5] drm/i915: unify first-stage engine struct setup
2016-07-13 13:16 ` Tvrtko Ursulin
@ 2016-07-13 13:24 ` Tvrtko Ursulin
2016-07-14 14:29 ` Daniel Vetter
2016-07-14 11:33 ` Dave Gordon
1 sibling, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2016-07-13 13:24 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel-gfx
On 13/07/16 14:16, Tvrtko Ursulin wrote:
>
> On 13/07/16 13:23, Daniel Vetter wrote:
>> On Fri, Jul 01, 2016 at 05:47:11PM +0100, Tvrtko Ursulin wrote:
>>> From: Dave Gordon <david.s.gordon@intel.com>
>>>
>>> intel_lrc.c has a table of "logical rings" (meaning engines), while
>>> intel_ringbuffer.c has separately open-coded initialisation for each
>>> engine. We can deduplicate this somewhat by using the same first-stage
>>> engine-setup function for both modes.
>>>
>>> So here we expose the function that transfers information from the
>>> static table of (all) known engines to the dev_priv->engine array of
>>> engines available on this device (adjusting the names along the way)
>>> and then embed calls to it in both the LRC and the legacy-mode setup.
>>>
>>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/intel_lrc.c | 40
>>> +++++++++++++++++++++------------
>>> drivers/gpu/drm/i915/intel_ringbuffer.c | 40
>>> +++++++++------------------------
>>> drivers/gpu/drm/i915/intel_ringbuffer.h | 5 +++++
>>> 3 files changed, 41 insertions(+), 44 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c
>>> b/drivers/gpu/drm/i915/intel_lrc.c
>>> index 339d8041075f..ed017f1a07a2 100644
>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>> @@ -1994,8 +1994,9 @@ logical_ring_default_vfuncs(struct
>>> intel_engine_cs *engine)
>>> }
>>>
>>> static inline void
>>> -logical_ring_default_irqs(struct intel_engine_cs *engine, unsigned
>>> shift)
>>> +logical_ring_default_irqs(struct intel_engine_cs *engine)
>>> {
>>> + unsigned shift = engine->irq_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);
>>> @@ -2096,14 +2097,14 @@ static int logical_render_ring_init(struct
>>> intel_engine_cs *engine)
>>> return ret;
>>> }
>>>
>>> -static const struct logical_ring_info {
>>> +static const struct engine_info {
>>> const char *name;
>>> unsigned exec_id;
>>> unsigned guc_id;
>>> u32 mmio_base;
>>> unsigned irq_shift;
>>> int (*init)(struct intel_engine_cs *engine);
>>> -} logical_rings[] = {
>>> +} intel_engines[] = {
>>> [RCS] = {
>>> .name = "render ring",
>>> .exec_id = I915_EXEC_RENDER,
>>> @@ -2146,20 +2147,31 @@ static const struct logical_ring_info {
>>> },
>>> };
>>>
>>> -static struct intel_engine_cs *
>>> -logical_ring_setup(struct drm_i915_private *dev_priv, enum
>>> intel_engine_id id)
>>> +struct intel_engine_cs *
>>> +intel_engine_setup(struct drm_i915_private *dev_priv,
>>> + enum intel_engine_id id)
>>
>> Kerneldoc for this would be nice. Also, we now have a mess between
>> intel_lrc.c and intel_ringbuffer.c. Extracting intel_engine.c with the
>
> Yes that was already suggested by Chris and I am trybotting the
> additions today, again on his explicit request to progress this series.
>
>> shared bits or something similar, plus cleanup up all the docs would be
>> awesome as a follow up.
>
> What do you mean "all the docs"? :D
>
>>
>> With the kerneldoc added:
>>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Thanks, will add.
Actually the function becomes static later in the series, when the
common stuff is moved into intel_engine_cs.c.
Can I keep the r-b without adding kernel doc for it then?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/5] drm/i915: unify first-stage engine struct setup
2016-07-13 13:24 ` Tvrtko Ursulin
@ 2016-07-14 14:29 ` Daniel Vetter
0 siblings, 0 replies; 15+ messages in thread
From: Daniel Vetter @ 2016-07-14 14:29 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
On Wed, Jul 13, 2016 at 02:24:06PM +0100, Tvrtko Ursulin wrote:
>
> On 13/07/16 14:16, Tvrtko Ursulin wrote:
> >
> > On 13/07/16 13:23, Daniel Vetter wrote:
> > > On Fri, Jul 01, 2016 at 05:47:11PM +0100, Tvrtko Ursulin wrote:
> > > > From: Dave Gordon <david.s.gordon@intel.com>
> > > >
> > > > intel_lrc.c has a table of "logical rings" (meaning engines), while
> > > > intel_ringbuffer.c has separately open-coded initialisation for each
> > > > engine. We can deduplicate this somewhat by using the same first-stage
> > > > engine-setup function for both modes.
> > > >
> > > > So here we expose the function that transfers information from the
> > > > static table of (all) known engines to the dev_priv->engine array of
> > > > engines available on this device (adjusting the names along the way)
> > > > and then embed calls to it in both the LRC and the legacy-mode setup.
> > > >
> > > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/intel_lrc.c | 40
> > > > +++++++++++++++++++++------------
> > > > drivers/gpu/drm/i915/intel_ringbuffer.c | 40
> > > > +++++++++------------------------
> > > > drivers/gpu/drm/i915/intel_ringbuffer.h | 5 +++++
> > > > 3 files changed, 41 insertions(+), 44 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> > > > b/drivers/gpu/drm/i915/intel_lrc.c
> > > > index 339d8041075f..ed017f1a07a2 100644
> > > > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > > > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > > > @@ -1994,8 +1994,9 @@ logical_ring_default_vfuncs(struct
> > > > intel_engine_cs *engine)
> > > > }
> > > >
> > > > static inline void
> > > > -logical_ring_default_irqs(struct intel_engine_cs *engine, unsigned
> > > > shift)
> > > > +logical_ring_default_irqs(struct intel_engine_cs *engine)
> > > > {
> > > > + unsigned shift = engine->irq_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);
> > > > @@ -2096,14 +2097,14 @@ static int logical_render_ring_init(struct
> > > > intel_engine_cs *engine)
> > > > return ret;
> > > > }
> > > >
> > > > -static const struct logical_ring_info {
> > > > +static const struct engine_info {
> > > > const char *name;
> > > > unsigned exec_id;
> > > > unsigned guc_id;
> > > > u32 mmio_base;
> > > > unsigned irq_shift;
> > > > int (*init)(struct intel_engine_cs *engine);
> > > > -} logical_rings[] = {
> > > > +} intel_engines[] = {
> > > > [RCS] = {
> > > > .name = "render ring",
> > > > .exec_id = I915_EXEC_RENDER,
> > > > @@ -2146,20 +2147,31 @@ static const struct logical_ring_info {
> > > > },
> > > > };
> > > >
> > > > -static struct intel_engine_cs *
> > > > -logical_ring_setup(struct drm_i915_private *dev_priv, enum
> > > > intel_engine_id id)
> > > > +struct intel_engine_cs *
> > > > +intel_engine_setup(struct drm_i915_private *dev_priv,
> > > > + enum intel_engine_id id)
> > >
> > > Kerneldoc for this would be nice. Also, we now have a mess between
> > > intel_lrc.c and intel_ringbuffer.c. Extracting intel_engine.c with the
> >
> > Yes that was already suggested by Chris and I am trybotting the
> > additions today, again on his explicit request to progress this series.
> >
> > > shared bits or something similar, plus cleanup up all the docs would be
> > > awesome as a follow up.
> >
> > What do you mean "all the docs"? :D
> >
> > >
> > > With the kerneldoc added:
> > >
> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > Thanks, will add.
>
> Actually the function becomes static later in the series, when the common
> stuff is moved into intel_engine_cs.c.
>
> Can I keep the r-b without adding kernel doc for it then?
Sure.
-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] 15+ messages in thread
* Re: [PATCH 1/5] drm/i915: unify first-stage engine struct setup
2016-07-13 13:16 ` Tvrtko Ursulin
2016-07-13 13:24 ` Tvrtko Ursulin
@ 2016-07-14 11:33 ` Dave Gordon
1 sibling, 0 replies; 15+ messages in thread
From: Dave Gordon @ 2016-07-14 11:33 UTC (permalink / raw)
To: intel-gfx
On 13/07/16 14:16, Tvrtko Ursulin wrote:
>
> On 13/07/16 13:23, Daniel Vetter wrote:
>> On Fri, Jul 01, 2016 at 05:47:11PM +0100, Tvrtko Ursulin wrote:
>>> From: Dave Gordon <david.s.gordon@intel.com>
>>>
[snip]
>>> {
>>> - const struct logical_ring_info *info = &logical_rings[id];
>>> + const struct engine_info *info = &intel_engines[id];
>>> struct intel_engine_cs *engine = &dev_priv->engine[id];
>>> - enum forcewake_domains fw_domains;
>>>
>>> engine->id = id;
>>> + engine->i915 = dev_priv;
>>> engine->name = info->name;
>>> engine->exec_id = info->exec_id;
>>> - engine->guc_id = info->guc_id;
>>> + engine->hw_id = engine->guc_id = info->guc_id;
>>
>> Optional bikeshed: s/info->guc_id/info->hw_id/ makes sense imo in the new
>> context. Or nuking engine->guc_id.
>
> Someone said we cannot be sure they will be the same in the future. So
> maybe just rename to hw_id for now.
>
> Regards,
> Tvrtko
The GuC firmware *could* use a completely different enumeration of the
engines, but why would it? Since it's closely tied to the hardware, it
*ought* to use the same naming scheme as the h/w. So I have no objection
to getting rid of guc_id entirely, and changing existing uses to use
hw_id instead.
OTOH it seems little benefit and would certainly involve more work to
reverse. The firmware team might, after all, decide that they too want
to decouple the logical-engine-numbers used for the KMD interface from
whatever the hardware team decide is the best way to number engines --
which might after all change between generations or even different SKUs
of the same device!
SO, I think the "best" version of that line is probably:
+ engine->hw_id = info->hw_id;
+
+ /* Current GuC f/w uses hw_id not driver id */
+ engine->guc_id = info->hw_id;
and we'll add "info->guc_id" back again if it ever becomes necessary.
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread