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

[Daniel Vetter]

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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch