Re: [Intel-gfx] ✗ Fi.CI.IGT: failure for Introduce multitile support

[Andi Shyti <andi.shyti@linux.intel.com>]

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@linux.intel.com> 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(&gt->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