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.
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
