Hi Matt and Tvrtko,
> On 18/03/2022 13:25, Matthew Auld wrote:
> > On Fri, 18 Mar 2022 at 08:18, Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx> wrote:
> > >
> > > > • igt@i915_selftest@mock@requests:
> > > >
> > > > □ shard-kbl: PASS -> DMESG-FAIL
> > > >
> > > > □ shard-tglb: PASS -> DMESG-FAIL
> > > >
> > > > □ shard-apl: PASS -> DMESG-FAIL
> > > >
> > > > □ shard-glk: PASS -> DMESG-FAIL
> > > >
> > > > □ shard-skl: PASS -> DMESG-FAIL
> > > >
> > > > □ shard-snb: PASS -> DMESG-FAIL
> > > >
> > > > □ shard-iclb: PASS -> DMESG-FAIL
> > >
> > > I don't see how these failures can be related to the series I
> > > sent.
> > >
> > > Maybe a false positive?
> >
> > AFAICT these look new. Did we forget to do something for the
> > mock_device? Maybe something in patch 3? Nothing is jumping out at
> > me...
it was of course suspicious, but I spent quite a lot of time at
fixing the mock selftests, until I got all greens on trybot. But
then, another refactoring happened...
> Yeah to "sus" :)
>
> [I like so don't recognise much of that patch I am supposed to be author of... I think it moved on a lot since my version. Anyway.. onto the bug.]
>
> Module init (executed in order):
>
> { .init = i915_mock_selftests }, -> this is the part which runs mock selftests
> ...
> { .init = i915_pci_register_driver, -> this is the part which sets up i915->gt[0]
>
> It happens via i915_pci_register_driver -> i915_pci_probe -> i915_driver_probe -> intel_gt_probe_all.
>
> Mock cleanup does:
>
> mock_device_release
>
> + intel_gt_driver_late_release(i915);
>
> ->
>
> + for_each_gt(gt, i915, id) {
> + intel_uc_driver_late_release(>->uc);
> + intel_gt_fini_requests(gt);
> + intel_gt_fini_reset(gt);
> + intel_gt_fini_timelines(gt);
> + intel_engines_free(gt);
> + }
>
> Hence I think for_each_gt does not see i915->gt[0] being set ergo does nothing.
goot point, I'm missing a
i915->gt[0] = gt;
somewhere, that is supposed to happen in the probe_all. Thanks!
> I also don't like the signature changes like:
>
> -void intel_gt_driver_late_release(struct intel_gt *gt)
> +void intel_gt_driver_late_release(struct drm_i915_private *i915)
>
> If it has to live in intel_gt.c, maybe at least use the "_all" suffix more consistently?
yes... not nice indeed. Also Michal complained. I will add the
"_all" suffix. Didn't want to make very long function names at
first.
Andi
