Re: [PATCH 1/4] drm/i915: Move execlists setup out of common

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 28/11/2017 16:04, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-11-28 13:07:54)
>>
>> On 28/11/2017 12:48, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2017-11-28 12:41:27)
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> Move the execlists specific setup out of intel_engine_setup_common. This
>>>> was supposed to be only for backend agnostic bits. At the same time rename
>>>> it to intel_engine_setup_execlist to follow the setup vs init naming
>>>> convetion we have.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> ---
>>>> +static void
>>>> +intel_engine_setup_execlist(struct intel_engine_cs *engine)
>>>> +{
>>>> +       struct intel_engine_execlists * const execlists = &engine->execlists;
>>>> +
>>>> +       execlists->csb_use_mmio = csb_force_mmio(engine->i915);
>>>> +
>>>> +       execlists->port_mask = 1;
>>>> +       BUILD_BUG_ON_NOT_POWER_OF_2(execlists_num_ports(execlists));
>>>> +       GEM_BUG_ON(execlists_num_ports(execlists) > EXECLIST_MAX_PORTS);
>>>> +
>>>> +       execlists->queue = RB_ROOT;
>>>> +       execlists->first = NULL;
>>>> +}
>>>
>>> The only problem here was that we wanted to be sure that some fields
>>> were initialised for the common paths, i.e. so we could iterate over the
>>> queue without worrying first if it was execlists (if it wasn't execlists
>>> the queue would be empty).
>>>
>>> Now, I think we could just rely on zero initialisation, but that was the
>>> rationale for it ending up early. Now we could split it between
>>> setup_execlists and init_execlists if we want the pedantry.
>>
>> Common paths as in ringbuffer submission? I grepped around and don't see
>> it used there.
> 
> See the reset code, the debug code, etc; in the common layer, above the
> backends, we want to be neutral.
> 
>> Then about setup vs init, we said init is for hw access so I don't
>> follow how you would split the above?
> 
> init_hw is for initialising hw. Better names for the phases is still
> open :)

Okay I found execlists->first usage in intel_engine_dump, so one so far. 
That could be made conditional, or if there are other places abstracted 
out to the backend implementation. It could be that I just did not find 
more due too much context switching?

This way or the other, I did not want to put a code like "if 
(HAS_EXECLISTS(i915)) ..."  in the function called 
intel_engine_init_execlists. That's just wrong.

And I'd say it's equally wrong to call intel_engine_init_execlists from 
_intel_engine_setup_common_, because the spiritual starting point for 
this common setup refactoring was to only put there bits _common_ to 
both backends.

If you want to keep this approach of letting the higher layers just 
assume they can access backend specific parts then simplest would be I 
just drop this patch and put that ugly "if HAS_EXECLISTS" where I don't 
want to put it. Can view it as interim and fixup later?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx