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

[Daniel Vetter <daniel@ffwll.ch>]

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.

Now my proposal to get us back some robustness seems not met with huge
enthusiams, so what's it going to be? I'd be ok with sprinkling more
explicit checks over tests (and probably more explicit control flow), plus
proper documentation for igt_display_require, too. But this here doesn't
look like a good idea long-term, and that's why I'm not happy with how
that bugfixing was rushed in. Fixing disable_display=1 isn't _that_ high
of a priority that we can't take the time to get the igt library api
somewhat right.

Cheers, Daniel

> ---
>  lib/igt_kms.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 454ab7481cde..867eaa7a971c 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -3269,7 +3269,7 @@ static int do_display_commit(igt_display_t *display,
>  	enum pipe pipe;
>  	LOG_INDENT(display, "commit");
>  
> -	if (!display->n_pipes || !display->n_outputs)
> +	if (igt_warn_on(!display->n_pipes || !display->n_outputs))
>  		return 0; /* nothing to do */
>  
>  	igt_display_refresh(display);
> @@ -3322,7 +3322,7 @@ int igt_display_try_commit_atomic(igt_display_t *display, uint32_t flags, void *
>  {
>  	int ret;
>  
> -	if (!display->n_pipes || !display->n_outputs)
> +	if (igt_warn_on(!display->n_pipes || !display->n_outputs))
>  		return 0; /* nothing to do */
>  
>  	LOG_INDENT(display, "commit");
> -- 
> 2.19.0.rc2
> 

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