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

[Daniel Vetter]

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.

> > 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch