See my comments below.
> -----Original Message-----
> From: Alan Previn <alan.previn.teres.ale...@intel.com>
> Sent: May 30, 2023 1:01 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: dri-de...@lists.freedesktop.org; Alan Previn
> <alan.previn.teres.ale...@intel.com>
> Subject: [v2] drm/i915/selftest/gsc: Ensure GSC Proxy init completes before
> selftests
>
> 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. While implementing this,
> use an table of function pointers so its scalable to add additional
> waiter functions for future such "wait on dependency" cases that.
>
> Difference from prior versions:
> v1: Based on internal testing, increase the timeout for gsc-proxy
> specific case to 8 seconds.
>
> Signed-off-by: Alan Previn <alan.previn.teres.ale...@intel.com>
> ---
> .../gpu/drm/i915/selftests/i915_selftest.c | 61 +++++++++++++++++++
> 1 file changed, 61 insertions(+)
>
>
> base-commit: 0ae4ee2c735979030a0219218081eee661606921
>
> diff --git a/drivers/gpu/drm/i915/selftests/i915_selftest.c
> b/drivers/gpu/drm/i915/selftests/i915_selftest.c
> index 39da0fb0d6d2..77168a7a7e3f 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,63 @@ static void set_default_test_all(struct selftest *st,
> unsigned int count)
> st[i].enabled = true;
> }
>
> +static int
> +__wait_gsc_proxy_completed(struct drm_i915_private *i915,
> + unsigned long timeout_ms)
> +{
> + 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));
> +
> + /*
> + * For gsc proxy component loading + init, we need a much longer
> timeout
> + * than what CI selftest infrastrucutre currently uses. This longer wait
> + * period depends on the kernel config and component driver load
> ordering
> + */
> + if (timeout_ms < 8000)
> + timeout_ms = 8000;
Lgtm, just an concern about the fixed number here, shall we set the minimal
here, or let i915_selftest.timeout_ms take control? Thus no longer need coding
change here in the future.
Reviewed-by: Zhanjun Dong <zhanjun.d...@intel.com>
> +
> + if (need_to_wait &&
> + (wait_for(intel_gsc_uc_fw_proxy_init_done(&i915->media_gt-
> >uc.gsc),
> + timeout_ms)))
> + return -ETIME;
> +
> + return 0;
> +}
> +
> +struct __startup_waiter {
> + const char *name;
> + int (*wait_to_completed)(struct drm_i915_private *i915, unsigned
> long timeout_ms);
> +};
> +
> +static struct __startup_waiter all_startup_waiters[] = { \
> + {"gsc_proxy", __wait_gsc_proxy_completed} \
> + };
> +
> +static int __wait_on_all_system_dependencies(struct drm_i915_private
> *i915)
> +{
> + struct __startup_waiter *waiter = all_startup_waiters;
> + int count = ARRAY_SIZE(all_startup_waiters);
> + int ret;
> +
> + if (!waiter || !count || !i915)
> + return 0;
> +
> + for (; count--; waiter++) {
> + if (!waiter->wait_to_completed)
> + continue;
> + ret = waiter->wait_to_completed(i915,
> i915_selftest.timeout_ms);
> + if (ret) {
> + pr_info(DRIVER_NAME ": Pre-selftest waiter %s failed
> with %d\n",
> + waiter->name, ret);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> static int __run_selftests(const char *name,
> struct selftest *st,
> unsigned int count,
> @@ -134,6 +193,8 @@ static int __run_selftests(const char *name,
> {
> int err = 0;
>
> + __wait_on_all_system_dependencies(data);
> +
> while (!i915_selftest.random_seed)
> i915_selftest.random_seed = get_random_u32();
>
