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?
