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

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

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.
> 
> To fix this, add a waiter function at the start of __run_selftests
> that waits for gsc-proxy init flows to complete.
> 
> Difference from prior versions:
>     v4: - Remove generalized waiters function table framework (Tvrtko).
>         - Remove mention of CI-framework-timeout from comments (Tvrtko).
>     v3: - Rebase to latest drm-tip.
>     v2: - Based on internal testing, increase the timeout for gsc-proxy
>           specific case to 8 seconds.
> 
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>   .../gpu/drm/i915/selftests/i915_selftest.c    | 25 +++++++++++++++++++
>   1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/selftests/i915_selftest.c b/drivers/gpu/drm/i915/selftests/i915_selftest.c
> index 39da0fb0d6d2..bbfaaaeef505 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_selftest.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_selftest.c
> @@ -24,6 +24,8 @@
>   #include <linux/random.h>
>   
>   #include "gt/intel_gt_pm.h"
> +#include "gt/uc/intel_gsc_fw.h"
> +
>   #include "i915_driver.h"
>   #include "i915_drv.h"
>   #include "i915_selftest.h"
> @@ -127,6 +129,26 @@ static void set_default_test_all(struct selftest *st, unsigned int count)
>   		st[i].enabled = true;
>   }
>   
> +static void
> +__wait_gsc_proxy_completed(struct drm_i915_private *i915)
> +{
> +	bool need_to_wait = (IS_ENABLED(CONFIG_INTEL_MEI_GSC_PROXY) &&
> +			     i915->media_gt &&
> +			     HAS_ENGINE(i915->media_gt, GSC0) &&
> +			     intel_uc_fw_is_loadable(&i915->media_gt->uc.gsc.fw));
> +	/*
> +	 * 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?

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.

In any case, v4 reads clear - no confusing comments and not 
over-engineered so is acceptable to me.

Regards,

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.

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.

> +}
> +
>   static int __run_selftests(const char *name,
>   			   struct selftest *st,
>   			   unsigned int count,
> @@ -134,6 +156,9 @@ static int __run_selftests(const char *name,
>   {
>   	int err = 0;
>   
> +	if (data)
> +		__wait_gsc_proxy_completed(data);
> +
>   	while (!i915_selftest.random_seed)
>   		i915_selftest.random_seed = get_random_u32();
>   
> 
> base-commit: 01c4678ab6c623c621a1dea438133e39711291d4