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@xxxxxxxxx>
> >>>>
> >>>> 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@xxxxxxxxx>
> >>>> ---
> >>>> +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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
