[igt-dev] [PATCH i-g-t 4/4] lib/kms: warn if we commit without outputs

[Arkadiusz Hiler]

On Fri, Oct 12, 2018 at 02:32:44PM +0200, Daniel Vetter wrote:
> On Fri, Oct 12, 2018 at 2:02 PM Arkadiusz Hiler
> <arkadiusz.hiler at intel.com> wrote:
> >
> > On Fri, Oct 05, 2018 at 08:48:40PM +0200, Daniel Vetter wrote:
> > > On Fri, Oct 05, 2018 at 04:40:14PM +0100, Chris Wilson wrote:
> > > > Quoting Daniel Vetter (2018-10-05 16:07:38)
> > > > > On Thu, Oct 04, 2018 at 03:21:28PM +0200, Daniel Vetter wrote:
> > > > > > With the high-level helpers requiring outputs there's not point
> > > > > > in silently ignoring issues anymore. Complain about that if it
> > > > > > ever happens.
> > > > > >
> > > > > > Cc: Antonio Argenziano <antonio.argenziano at intel.com>
> > > > > > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > > > > > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > > > >
> > > > > I guess my motivation with fumbling around with the igt_display_* api
> > > > > wasn't entirely clear: It's this patch here, respectively Chris' patch
> > > > > which added the silent short circuit.
> > > > >
> > > > > Imo that's very brittle api, asking for trouble, and me not recognizing
> > > > > right away what's happening in the debugfs is kinda proving the point.
> > > > > Silently throwing a request away from the testcase is at least very
> > > > > surprising. And inconsistent with both more explicit igt_require/assert
> > > > > checks in drivers, and (the style I favour personally, but really not the
> > > > > issue here) putting igt_require/assert into the helper library.
> >
> > So basically what you want to do here:
> >
> > 1. make sure that tests are using the correct one:
> >  * igt_display_init()    - display disabled safe/aware tests
> >  * igt_display_require() - if you need a functioning display
> >
> > 2. Make sure that we don't hide/silence errors coming out of kms
> >    unnecessarily.
> >
> > Sounds sensible to me.
> >
> > > > I'd argue the opposite. The test case requested us to configure 0
> > > > outputs across 0 pipes, and we are obeying it. Not second guessing what
> > > > the test case intended.
> >
> > | if (!display->n_pipes || !display->n_outputs)
> > |       return 0; /* nothing to do */
> >
> > Then if the test does requests that, why do we return 0 here, instead of
> > doing the IOCTL, especially that the doc states:
> > "This function should be used to commit changes that are expected to fail"?
> >
> > How is that less second guessing? TBH I would not even warn here.
> 
> Afaiui this fails for the tests (like the debugfs one) that want to
> keep running even if no display is present. That's at least my
> understanding. Just committing is what we originally had, and would
> amount to a revert of
> 
> commit 212b71372bfbb73663d872df31118d6b396ada4f
> Author: Chris Wilson <chris at chris-wilson.co.uk>
> Date:   Fri Sep 14 21:03:38 2018 +0100
> 
>     lib/kms: Skip no-op display updates
> 
> which would break some tests with i915.disable_display=1 I think (but
> the commit message isn't terribly clear on what exactly). I'm also ok
> with reverting that one instead of the igt_warn_on patch here, both
> are imo fine options.

I wonder what breaks in there.

If we have some hard asserts inside that are failing us with display
disabled, maybe this is the thing that needs fixing.

I think I have a NUC I can use for testing somewhere around.
Let me find it.

> > > We have 8 cases of igt_display_init now, and slightly shy of 70 for
> > > igt_display_require. There's almost 10x testcases you can silently break
> > > for the handful that don't break with this.
> > >
> > > Furthermore, there seems exactly 1 testcase which actually asked for this,
> > > and I'm argueing that that testcase is much cleaner if we'd split the
> > > no-display test from the all-displays-off test.
> > >
> > > So we have 1 vs. ~70 testcases. If your api makes the exceedingly common
> > > case brittle, then I really don't see why that's good api design.
> > >
> > > And there's tons other options, without any brittleness. E.g. we could add
> > > this igt_warn_on here, but add explicit control flow to the debugfs test.
> > > That:
> > > - Keeps all the igt require checks exactly where you want them.
> > > - Gives us a non-brittle api that doesn't hide surprises.
> > >
> > > There's probably metric piles of other approaches, with varying shades of
> > > paint applied. I don't see any reason why the api should silently eat
> > > errors - and errors does it eat, since that's why you pushed the patch.
> > >
> > > All I'm arguing for is to make that more explicit.
> >
> > Again, sounds like a sensible thing to do and being explicit helps with
> > making the codebase more digestible. This is less of a two-liner hax and
> > more of a proper rethinking. I agree that we are at the point where it
> > has to be done, so we won't let our APIs rot any further.
> 
> So the question is, what do do. We have 2 options for this function
> here (revert or igt_warn), and we have a bunch of options for how to
> make things more explicit in the tests (either my patch series here,
> with v2 of patch 1, or maybe more explicit control flow with a
> __must_check annotation for igt_display_init). Imo all combinations
> would give us a solid api in the end, I just prefer not to implement
> them all first, but decide first :-)
> -Daniel

IMO:

1. revert and either:
 a) fixing try_commit so that it does not assert
 b) moving the if n_pipes == 0 || n_outputs == 0 logic to debugfs tests
    (or a block igt_with_display_on(display) { } ?)
2. respin of your patch series

But let's see what others have to say.

-Arek