Re: [PATCH i-g-t v3 2/7] lib/igt_kms: Fix off-by-one bug on skip of missing pipe

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Gabriel Krisman Bertazi <krisman@collabora.co.uk>,
	intel-gfx@lists.freedesktop.org
Cc: daniel.vetter@intel.com, daniels@collabora.com,
	tomeu.vizoso@collabora.com
Subject: Re: [PATCH i-g-t v3 2/7] lib/igt_kms: Fix off-by-one bug on skip of missing pipe
Date: Thu, 31 Aug 2017 09:06:32 +0200	[thread overview]
Message-ID: <ca0dd9c3-2423-f8b4-b72e-758cdc89fcae@linux.intel.com> (raw)
In-Reply-To: <20170831061810.11773-3-krisman@collabora.co.uk>

Op 31-08-17 om 08:18 schreef Gabriel Krisman Bertazi:
> display->n_pipes is zero-indexed, so N returned in
> igt_display_get_n_pipes is already not a valid pipe.  This patch
> prevents kms_ccs from going nuts when testing the first unxesting pipe.
>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.co.uk>
> ---
>  lib/igt_kms.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 14e2701c3afd..ce07fcc1fc73 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1864,7 +1864,7 @@ void igt_display_require_output_on_pipe(igt_display_t *display, enum pipe pipe)
>  {
>  	igt_output_t *output;
>  
> -	igt_skip_on_f(igt_display_get_n_pipes(display) < pipe,
> +	igt_skip_on_f(igt_display_get_n_pipes(display) <= pipe,
>  		      "Pipe %s does not exist.\n", kmstest_pipe_name(pipe));
>  
>  	for_each_valid_output_on_pipe(display, pipe, output)

I think the original check is correct?

Although it could be inverted to make it more clear, matching the other similar checks in tests/kms_*.c:

igt_skip_on_f(pipe >= igt_display_get_n_pipes(display),

But your change doesn't match what any user of n_pipes in /tests is doing, so I'm afraid I have to NACK. :)

Kind regards,
Maarten

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx