On 12/07/2023 18:49, Teres Alexis, Alan Previn wrote:
On Wed, 2023-07-12 at 10:19 +0100, Tvrtko Ursulin wrote:
On 11/07/2023 23:02, Alan Previn wrote:
On MTL, if the GSC Proxy init flows haven't completed, submissions to the
GSC engine will fail. Those init flows are dependent on the mei's
gsc_proxy component that is loaded in parallel with i915 and a
worker that could potentially start after i915 driver init is done.
That said, all subsytems that access the GSC engine today does check
for such init flow completion before using the GSC engine. However,
selftests currently don't wait on anything before starting.
alan:snip
+ /*
+ * The gsc proxy component depends on the kernel component driver load ordering
+ * and in corner cases (the first time after an IFWI flash), init-completion
+ * firmware flows take longer.
+ */
+ unsigned long timeout_ms = 8000;
+
+ if (need_to_wait &&
+ (wait_for(intel_gsc_uc_fw_proxy_init_done(&i915->media_gt->uc.gsc, true),
+ timeout_ms)))
+ pr_info(DRIVER_NAME "Timed out waiting for gsc_proxy_completion!\n");
Would it make sense to error out here? Or at least upgrade to pr_warn or
something?
alan: agree on pr_warn (especially since need_for_wait being true and we tried for 8 secs - this should never happen).
I didn't quite understand the points Daniele raised about engine loops
and resets - in my mind GSC engine is this special thing exercised for
highly specialized operations and not touched in random for_each_engine
loop tests, but I also did not really look so might be totally wrong.
alan: after consulting with Daniele further, the concern in the case of
having gsc-proxy-init mid-execution while other selttests
are running (when thinking of tests that have nothing to do with GSC
but has indirect effect like memory-pressure, engine-resets,
etc) is that the proxy-init will bail-out print an error but
that would result in CI reporting a false-negative. We can't skip
that error since CONFIG_INTEL_MEI_GSC_PROXY tells us that the kernel
owner wants GSC-PROXY working.
In any case, v4 reads clear - no confusing comments and not
over-engineered so is acceptable to me.
alan: thanks Tvrtko.
P.S. Maybe the check *could* be moved to i915_live_selftests, where hw
dependencies conceptually fit better, and maybe i915_perf_selftests
would need it too then (?), but it is up to you.
alan: i can do this quickly as a rev5 (after a bit of manual check if perf needs it)
Maybe even in the array selftests/i915_live_selftests.h if we could add
a facility to make unskippable tests and add this one after the sanity
check. Which would then achieve the same generalized thing you had in
the previous version without needing to add a new array/loop.
alan: i rather not attempt this as part of the current patch but will
consider it as a separate patch if you are okay with that?
Yes that is fine.
Regards,
Tvrtko
