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

[Arkadiusz Hiler]

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.

> 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.

- Arek