[Intel-gfx] [PATCH 1/4] drm/i915: Move execlists setup out of common

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-11-28 17:29:25)
> 
> 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 at 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 at 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. 

Keep looking.

> That could be made conditional,

No. It already is conditional on the derived state.

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

It's wrong to initialised the shared structure?

> 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?

Don't break it in this patch, and you won't have to do either?
-Chris