[igt-dev] [PATCH v7 1/2] tests: Add a new test for driver/device hot reload

[Janusz Krzysztofik]

On Wednesday, April 17, 2019 10:53:05 AM CEST Katarzyna Dec wrote:
> On Tue, Apr 16, 2019 at 09:17:21AM +0200, Janusz Krzysztofik wrote:
> > ...
> > On Friday, April 12, 2019 10:16:41 AM CEST Katarzyna Dec wrote:
> > > On Thu, Apr 11, 2019 at 02:26:28PM +0200, Janusz Krzysztofik wrote:
> > > > ...
> > > > +/*
> > > > + * Actions
> > > > + *
> > > > + * Purpose: make the device disappear
> > > > + *
> > > > + * @dir: file descriptor of an open device sysfs directory
> > > > + *
> > > > + * Return value: file descriptor of an open device bus' sysfs
> > > > directory
> > > > + * 		 or -1 if no bus rescan is needed
> > > > + */
> > > > +
> > > 
> > > I went though your answers on my quesions. It is more then ok to have a
> > > wider overview on what is going on in whole binary, although I would
> > > prefer
> > > to have some summary doc at the begining and small docs aboe functions
> > > (if
> > > needed).
> > 
> > Let's try to agree on that before I submit another version.
> > 
> > There are now 3 multiline comments, each of them open one of three
> > sections of the code:
> > 1) a section with different action functions,
> > 2) a section with different workload functions,
> > 3) a section with common code, I called it skeleton.
> > 
> > If I move all those three comments at the top, they will loose their
> > purpose of opening the sections.  That's why I propose to keep them where
> > they are> 
> > now, and to add another comment at the top.  Here is how it may look like:
> >  /**
> >  * This binary includes subtests for performing the following two actions:
> >  * - unbinding the driver from a device under load,
> >  * - virtually unplugging the device under load from its bus.
> >  * Both actions are implemented as separate functions sharing the same
> >  API,
> >  * wrapped with a common skeleton.  A similar approach has been taken for
> >  * defining different workloads.  Even if only one type of workload is
> >  currently * implemented, the code is ready for extending it with other
> >  workloads defined * as additional workload functions which follow the
> >  same API.
> >  * Since the unplug action actually removes the device instance from the
> >  system, * a bus rescan operation is needed to reinstantiate the device
> >  after completion * of the test.  For that purpose, the action passes
> >  back a file descriptor of * an open sysfs directory representing the bus
> >  on which the rescan is then * performed.  In case of the unbind action,
> >  it always returns an invalid file * descriptor to inform the skeleton
> >  that no bus rescan is needed.
> >  * Since a workload is expected to crash as a result of an action, its
> >  * invocation is performed from a helper process run from the igt_fixture
> >  block * in order to avoid direct influence of that crash on a test
> >  result. */
> > 
> > What do you think?
> 
> It looks a little too long, but it is ok :).

OK, I'll see if I can make it shorter.

> > > > ...
> > > > +static void healthcheck(int chipset)
> > > > +{
> > > > +	if (chipset == DRIVER_INTEL) {
> > > > +		/*
> > > > +		 * We have it perfectly implemented in i915_module_load,
> > > > +		 * just use it.
> > > > +		 */
> > > > +		igt_assert(igt_system_quiet("i915_module_load --run-
subtest
> > > > reload")
> > > > +			   == IGT_EXIT_SUCCESS);
> > > > ...
> 
> We have a discussion at the office together with knr about this binary.

Yes, I know, and I've been waiting for some response from Michał with results 
of that discussion, as he has promised, before going any further.  I'm 
assuming this message provides that information.

> The
> outcome was:
> * we should not run test from inside this one, so should avoid cases when we
> run 'i915_module_load' inside healthcheck, you should add test code -
> exactly the functions that you need.

With my still not enough experience with i915 I don't really know what I need 
but I've already had a suggestion on that from Daniel to follow:

On Tuesday, April 2, 2019 10:47:20 AM CEST Daniel Vetter wrote:
> ... good sanity check is to issue a batch with an MI write and check it 
> still completes and does something ...

so I'll try to implement it instead of calling i915_module_load.

> * It would be nice to have bind/unbind scenario with DRIVER_ANY located
> somewhere in igt/tests/* (vendor agnostic)

I didn't know about igt/tests/ being the right place to put vendor agnostic 
tests, I thought that tests/ was the right location, with subdirs dedicated to 
vendors.  But anyway, does that mean you prefer a separate source file for 
each vendor specific (or agnostic) implementation, regardless of possibly high 
code redundancy?  If that's the case then maybe I should consider moving some 
of that under lib/ for reuse by those separate implementations (that would 
take additional time, of course).

> * we have also doubts about second patch. Even if it looks cool, it cannot
> be probably used by CI. And if CI is not using it, most probably developers
> will not be using it too.

OK, I'll drop it from the series if you think it's useless.  Since I think I 
may be using it anyway, I'll keep it in my private branch :-)

Regarding additional workloads, not mentioned here but a few times before by 
Daniel and Chris, I fully understand they are needed (that's why I proposed 
that workaround with optional external workloads as a quick win).  However, 
for them to be implemented in a way consistent with subtest naming convention 
as I see it, I first need to move workload invocation back to subtests.  For 
that, I need to reimplement some of existing library functions now containing 
igt_assert()s etc. so obviously expected workload crashes inside subtests 
won't affect test results in any way.  While working on that, I'd like to put 
possibly reusable modified functions inside libraries, not only in this 
particular test sources.  That will certainly take still more time.

> For more questions/clarifications or concerns, please start discussion on
> intel-gfx pointing people that you need :) It is the fastest way to have
> feedback.

OK

Thanks,
Janusz

> 
> Kasia :)