Re: [Intel-gfx] [PATCH v4] drm/i915/selftest/gsc: Ensure GSC Proxy init completes before selftests

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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