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

[Tvrtko Ursulin]

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