[igt-dev] [PATCH i-g-t v3] ts/core_setmaster: new test for drop/set master semantics

[Petri Latvala]

On Fri, Mar 06, 2020 at 02:17:07PM +0000, Emil Velikov wrote:
> On Wed, 4 Mar 2020 at 14:17, Petri Latvala <petri.latvala at intel.com> wrote:
> 
> [snip]
> > > Thinking about it, you're likely talking about the 3rd subtest aka
> > > "master-drop-set-shared-fd". Sure we can use igt_require() there,
> > > although I'd rather keep the__drm_open_driver() + igt_require()
> > > combination.
> > >
> > > Do you agree?
> >
> > For i915, gem_quiescent_gpu and pals are required so pending work
> > failing is reported on the correct test. Without that done, results
> > are hilariously racy.
> >
> Sure I get that, yet there is _no_ work being done in these tests.
> 
> [snip]
> > > Thinking about it close(drm_open_driver(...)) looks like a workaround,
> > > instead of addressing the issue.
> > > Even though it seems like it might work, the modprobe machinery seems partial:
> > >  - in the non i915 case, modprobe failure is a _debug_ message
> >
> > igt_warn("Could not load i915\n");
> >
> > Looks like a warn to me.
> >
> Note: _non_ i915 case.

I need glasses. Yes, the other probes have debug messages.

> > >  - the DRIVER_FOO to module name mapping is partial
> >
> > Yes, we don't have all of them, but we should have all that have
> > module reloading tests.
> >
> Agreed, yet this is not a module reloading test.

I mean that we modprobe all drivers that have module loading tests, or
otherwise special module handling. This test not being a module
reloading test doesn't matter, see example below.

> > >  - for some drivers, the module name differs from the name in
> >      drmGetVersion()
> >
> > Hence the special cased modprobe hook for i915 and the now-removed
> > virtio-gpu.
> >
> Virtio-gpu is a perfect example.

And we don't modprobe that anymore.


> > >  - some drivers have changed their module name
> >
> > The ones we modprobe in IGT don't seem to have?
> >
> >
> > Now, for i915 in particular since we do a load of module reload
> > testing, our required semantics for tests are to leave the state of
> > the system at test exit time in either
> >
> > 1) module is loaded and working
> > 2) module not loaded
> >
> > For i915, not doing drm_open_driver() first means you don't have
> > /dev/dri/card0 if the previously run test is something from
> > i915_module_load.
> >
> > See commit 676d031e6bd9 for a related fix.
> >
> I agree with the point of having reloading tests, so having a modprobe
> machinery, of sorts, makes sense.
> 
> Here we are relying on the _partial_ machinery to workaround a
> extremely unlikely corner-case.

It is _extremely_ likely that we (i915 CI) could sometimes run
igt at i915_module_load@reload right before this test. Or
igt at i915_selftest@live. When we do, there's no /dev/dri/card0 at the
time of chmodding but is at the time of __drm_open_driver().


-- 
Petri Latvala