[Intel-gfx] [PATCH v2] drm/i915/execlists: Refactor common engine setup

[Tvrtko Ursulin]

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 at chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
>>> Cc: Dave Gordon <david.s.gordon at 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