[igt-dev] [PATCH i-g-t 1/4] lib/igt_kms: Fix test_init() crash when <6 pipes

[Petri Latvala]

On Fri, Jun 18, 2021 at 10:28:52AM -0400, Anson Jacob wrote:
> From: Victor Lu <victorchengchi.lu at amd.com>
> 
> [why & how]
> An upstream change is causing the common amdgpu test_init() to crash on
> ASICs with <6 pipes.
> 
> Signed-off-by: Victor Lu <victorchengchi.lu at amd.com>
> Acked-by: Anson Jacob <Anson.Jacob at amd.com>
> Cc: Petri Latvala <petri.latvala at intel.com>
> Cc: Rodrigo Siqueira <Rodrigo.Siqueira at amd.com>
> Cc: Harry Wentland <harry.wentland at amd.com>
> Cc: Mark Yacoub <markyacoub at google.com>
> ---
>  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 c7c69b6ea0eb..26c51a384918 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -2164,8 +2164,8 @@ void igt_display_require(igt_display_t *display, int drm_fd)
>  	 * pipe display and reading pipe enum for a crtc using GET_PIPE_FROM_CRTC_ID ioctl
>  	 * for a pipe to do pipe ordering with respect to crtc list.
>  	 */
> -	display->n_pipes = IGT_MAX_PIPES;
> -	display->pipes = calloc(sizeof(igt_pipe_t), display->n_pipes);
> +	display->n_pipes = resources->count_crtcs;
> +	display->pipes = calloc(sizeof(igt_pipe_t), IGT_MAX_PIPES);
>  	igt_assert_f(display->pipes, "Failed to allocate memory for %d pipes\n", display->n_pipes);


NAK on this, this will break i915 testing.

A little background: The IGT interface for CRTCs has been changed to
accomodate Intel HW change where some pipes are disabled leaving
holes. The resources->crtcs array still has the crtcs that actually
exist, but it could be for example

count_crtcs = 2
crtcs[0] with PIPE_A
crtcs[1] with PIPE_C

Not all pipes are created equal so we need to track which one is which
so we know which pipe is broken when failures happen.

Now, the IGT interface should be fairly invisible except in
cornercases. Every access to a pipe needs to check pipe->enabled
before doing anything to it, and that's done automatically with things
like for_each_pipe(). Directly selecting a pipe should be the only
case where one needs to do the pipe->enabled check themselves.

Where does it crash for you? I tried to look at the test_init()
functions in tests/amdgpu/*.c but couldn't spot anything that touches
n_pipes. Or even uses anything other than PIPE_A.

I wonder if fixing this for you is as simple as doing

 if (!i915_device) display->n_pipes = i;

after the for loop you're changing here...


-- 
Petri Latvala