[igt-dev] [PATCH i-g-t 2/3] tests/i915_load: Add 'load' subtest

[Arkadiusz Hiler]

On Wed, Oct 17, 2018 at 01:31:56PM +0100, Chris Wilson wrote:
> Quoting Arkadiusz Hiler (2018-10-17 13:15:44)
> > This subtest is meant to be run as a first thing on your system, making
> > it a catch-all for all the issues we can hit during the HW init, so the
> > runner can do its magic by looking for dragons in the dmesg and
> > overriding the final result if any were spotted.
> > 
> > To have full "normal boot" experience it also probes all the connectors.
> > 
> > The subtest warns if the driver is already loaded, so we know that
> > there's something off with the environment configuration.
> > 
> > It's already blacklisted for Intel's CI, along the other
> > module-wrangling subtests.
> > 
> > Cc: Petri Latvala <petri.latvala at intel.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler at intel.com>
> > ---
> >  tests/i915_load.c | 39 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> > 
> > diff --git a/tests/i915_load.c b/tests/i915_load.c
> > index cf4aea80..4f61d280 100644
> > --- a/tests/i915_load.c
> > +++ b/tests/i915_load.c
> > @@ -35,6 +35,7 @@
> >  #include <signal.h>
> >  #include <sys/ioctl.h>
> >  #include <fcntl.h>
> > +#include <xf86drmMode.h>
> >  
> >  
> >  #define LOCAL_I915_EXEC_BSD_SHIFT      (13)
> > @@ -324,8 +325,46 @@ hda_dynamic_debug(bool enable)
> >         fclose(fp);
> >  }
> >  
> > +static void
> > +reprobe_connectors(int fd)
> > +{
> > +       int i;
> > +       drmModeConnectorPtr conn;
> > +
> > +       drmModeResPtr res = drmModeGetResources(fd);
> > +       if (!res)
> > +               return;
> 
> You can be really cheeky and do this entirely from the stack.

Care to elaborate?

> > +       for (i = 0; i < res->count_connectors; ++i) {
> > +               /* drmModeGetConnector forces probe */
> > +               conn = drmModeGetConnector(fd, res->connectors[i]);
> > +               igt_assert(conn);
> 
> And doing so would allow you to inspect the error directly; as this
> assert can mean many, many things. It could even be legitimate for DP-MST.

Fair point.

> > +               drmModeFreeConnector(conn);
> > +       }
> > +
> > +       drmModeFreeResources(res);
> > +}
> > +
> >  igt_main
> >  {
> > +       igt_subtest("load") {
> 
> load?  This is more of a "probe", I would say.

It loads the module and probes the connectors, but I think that "load"
is more descriptive of the intention here.

> > +               int fd, load_error;
> > +               bool i915_already_loaded;
> > +
> > +               i915_already_loaded = igt_kmod_is_loaded("i915");
> > +               igt_warn_on(i915_already_loaded);
> 
> Warn? You didn't unload so randomly warning based on run order.

Not randomly, always in the same scenarios. I think the warn is
descriptive enough.

Warning here will also notify us about any issues with the testing
configuration - if we run the test when we are not supposed to, or if we
modprobe i915 (e.g. due to broken blacklisting), rendering the test
useless.

We can go with a better name here than "load", to be even clearer.
-Arek

> > +               if (!i915_already_loaded) {
> > +                       load_error = igt_i915_driver_load(NULL);
> > +                       igt_assert_eq(load_error, 0);
> > +               }
> > +
> > +               fd = __drm_open_driver(DRIVER_INTEL);
> 
> __drm_open_driver -> performs a default modprobe of the driver.
> 
> > +               igt_assert(fd > 0);
> > +               reprobe_connectors(fd);
> > +               close(fd);
> > +       }
> > +
> >         igt_subtest("basic-reload") {
> >                 int load_error;
> >  
> > -- 
> > 2.17.2
> > 
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev