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

[Daniel Vetter <daniel@ffwll.ch>]

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@intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@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.
> 
> 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.

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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev