[igt-dev] [PATCH i-g-t 2/4] tests: Use igt_display_require

[Daniel Vetter]

On Thu, Oct 04, 2018 at 09:04:09PM +0200, Daniel Vetter wrote:
> On Thu, Oct 04, 2018 at 04:06:53PM +0100, Chris Wilson wrote:
> > Quoting Daniel Vetter (2018-10-04 14:21:26)
> > > diff --git a/tests/kms_force_connector_basic.c b/tests/kms_force_connector_basic.c
> > > index e9325dec9305..b8246e669939 100644
> > > --- a/tests/kms_force_connector_basic.c
> > > +++ b/tests/kms_force_connector_basic.c
> > > @@ -217,7 +217,7 @@ int main(int argc, char **argv)
> > >  
> > >                 /* attempt to use the display */
> > >                 kmstest_set_vt_graphics_mode();
> > > -               igt_display_init(&display, drm_fd);
> > > +               igt_display_require(&display, drm_fd);
> > >                 igt_display_commit(&display);
> > >                 igt_display_fini(&display);
> > 
> > Where is the requirement here? I'd buy that this should be an
> > igt_assert() since we did the forcing earlier.
> > 
> > > diff --git a/tests/kms_getfb.c b/tests/kms_getfb.c
> > > index 07ffd79c4613..ca0b01c05e5c 100644
> > > --- a/tests/kms_getfb.c
> > > +++ b/tests/kms_getfb.c
> > > @@ -116,7 +116,7 @@ static uint32_t get_any_prop_id(int fd)
> > >  {
> > >         igt_display_t display;
> > >  
> > > -       igt_display_init(&display, fd);
> > > +       igt_display_require(&display, fd);
> > 
> > Not required, as we describe the requirement of having the property
> > as an output of this function.
> > 
> > >         for (int i = 0; i < display.n_outputs; i++) {
> > >                 igt_output_t *output = &display.outputs[i];
> > >                 if (output->props[IGT_CONNECTOR_DPMS] != 0)
> > > diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c
> > > index b34bc66ce2c4..21292bf3a2fe 100644
> > > --- a/tests/perf_pmu.c
> > > +++ b/tests/perf_pmu.c
> > > @@ -811,7 +811,7 @@ event_wait(int gem_fd, const struct intel_execution_engine2 *e)
> > >         igt_skip_on(IS_VALLEYVIEW(devid) || IS_CHERRYVIEW(devid));
> > >  
> > >         kmstest_set_vt_graphics_mode();
> > > -       igt_display_init(&data.display, gem_fd);
> > > +       igt_display_require(&data.display, gem_fd);
> > 
> > We do a search for our requirements in the loop below.
> > 
> > >         /**
> > >          * We will use the display to render event forwarind so need to
> > > diff --git a/tests/pm_backlight.c b/tests/pm_backlight.c
> > > index 32808cdf6ca4..054300f6e2e1 100644
> > > --- a/tests/pm_backlight.c
> > > +++ b/tests/pm_backlight.c
> > > @@ -214,7 +214,7 @@ igt_main
> > >                  * try to enable all.
> > >                  */
> > >                 kmstest_set_vt_graphics_mode();
> > > -               igt_display_init(&display, drm_open_driver(DRIVER_INTEL));
> > > +               igt_display_require(&display, drm_open_driver(DRIVER_INTEL));
> > 
> > The exact requirement of having a connected backlight is spelled out
> > below.
> 
> Answering to all 4, since they boil down to the same I think:
> 
> If I understand this correctly, you want to have only 1 igt_require, at
> the very end, and keeping initializing everything? Even if it's pointless
> to keep on going, because we know that if there's no output, you can't
> possibly have a backlight on an output (as an example)?
> 
> My preferred approach is to bail out as soon as we know we can't
> reasonably proceed. Since that tends to give us the option of more
> abuse-proof apis - if I can remove the igt_display_init, then I think
> that's a good goal.

Another upside of checking early (even if you might not catch the full
conditions all in one go): We waste less time on skipped tests with
pointless setup. E.g. on the gem side we also don't open the drm fd,
initialize the batch manager, set up the batch and everything only to then
realize that we're not running on top of i915.ko. I think this here is
similar.

If you want, I could move them a lot further up in the code so they catch
stuff earlier.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch