Thanks fore reviewing Tvrtko, below are my responses.
I'll rerev without generalized func ptr and only for the subtests that need it.
...alan
On Thu, 2023-06-29 at 22:44 +0100, Tvrtko Ursulin wrote:
> On 29/06/2023 21:42, 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
> > +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
> > + */
>
> How is a CI timeout value relevant?
>
> Plus from the commit message it sounds like the point of wait is so
> submission to gsc does not fail if loading is still in progress, not
> that the CI times out. So what is the main problem?
Alan: The comment was meant to explain why we override the CI selftest timeout (an input param
to the generalized func ptr loop) to something much larger specially for gsc-proxy-waiting.
However, since your other review comment below is to remove the generalization, this comment
therefore will not make sense so I'll remove it accordingly. The point was that CI might
have a system level selftest timeout of something much smaller like 500 milisecs (used to
have some control over the execution time), but for the gsc-proxy waiting, its not in i915's
control but depends on the kernel component driver loading flow (and in rare occasions, after
a fresh IFWI was flashed which causes a 1-time longer period for fw-proxy flows to complete).
In any case, I'll remove the comment as per your direction.
>
> > + if (timeout_ms < 8000)
> > + timeout_ms = 8000;
> > +
>
alan:snip
> > +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;
>
> Ugh.
>
> If it ever becomes an empty array just zap this whole code and not have
> these checks.
Alan: Okay sure - will remove these check except the i915 - see below.
>
> Also, no i915 is a possibility?
Alan: i915_mock_selftests passes in NULL for i915. This checking of the i915
aligns with the existing __run_selftests code - but in that function the
param is called "data" eventhough in all callers of __run_selftests, that
"data" is actually i915 when its not null.
>
> But actually.. please don't add the function table generalization unless
> it is already known something else is coming to be plugged into it.
Alan: Okay- I'll remove it.
alan:snip
>
> > @@ -134,6 +193,8 @@ static int __run_selftests(const char *name,
> > {
> > int err = 0;
> >
> > + __wait_on_all_system_dependencies(data);
>
> Why does this need to be top level selftests and not just a wait for
> intel_gsc_uc_fw_proxy_init_done in the tests where it is relevant, via
> some helper or something?
Alan: it was an offline decision because we didn't want to repeat
the same check for all permutations of selftests' subtests (i.e. considering
module params can dictate to skip some subtests but execute others).
Anyways, let me get back to you on how how many selftests' subtests actually excercise the
need for proxy-init to complete - if its just 1-to-2 subtest I'll move the remove the code
from here and move them into the individual subtests.
alan:snip
