Re: [igt-dev] [PATCH] tests/kms_pipe_crc_basic: Sanity check for CRC mismatches

[Karthik B S <karthik.b.s@intel.com>]

On 7/30/2020 3:59 AM, Bhanuprakash Modem wrote:
> We’ve seen multiple CRC related issues on older platforms and
> pre-silicon environment, most of the time we end up with debugging.
> 
> This patch works as a crc-sanity test, which can verify that the
> CRC mechanism is clean from the platform side before debugging the
> actual CRC mismatches or other CRC related issues.
> 

Please add the test details as well in the commit message.
> Cc: Swati Sharma <swati2.sharma@intel.com>
> Cc: Karthik B S <karthik.b.s@intel.com>
> Cc: Jeevan B <jeevan.b@intel.com>
> Cc: Petri Latvala <petri.latvala@intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> ---
>   tests/kms_pipe_crc_basic.c | 77 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 77 insertions(+)
> 
> diff --git a/tests/kms_pipe_crc_basic.c b/tests/kms_pipe_crc_basic.c
> index cb93c1ad..a59c6bf1 100644
> --- a/tests/kms_pipe_crc_basic.c
> +++ b/tests/kms_pipe_crc_basic.c
> @@ -154,6 +154,80 @@ static void test_read_crc(data_t *data, enum pipe pipe, unsigned flags)
>   	}
>   }
>   
> +/*
> + * CRC-sanity test, to make sure there would be no CRC mismatches
> + *
> + * - Create two framebuffers (FB0 & FB1) with same color info
> + * - Flip FB0 with the Primary plane & collect the CRC as ref CRC.
> + * - Flip FB1, collect CRC & compare with the ref CRC
Nitpick, but its better if both sentences are consistent.
In the first, primary plane is mentioned, but in the second
nothing is mentioned. Little ambiguous for me.
> + *
> + *  No CRC mismatch should happen
> + */
> +static void test_check_crc(data_t *data)
> +{
> +	igt_display_t *display = &data->display;
> +	igt_output_t *output;
> +	enum pipe pipe;
> +	drmModeModeInfo *mode;
> +	struct igt_fb fb0, fb1;
> +	igt_crc_t ref_crc, crc;
> +	igt_pipe_crc_t *pipe_crc;
> +	igt_plane_t *primary;
> +
> +	for_each_pipe_with_valid_output(display, pipe, output) {

Can we use 'igt_get_single_output_for_pipe' for the intended pipe 
instead of using loop when we're only using it for one iteration always.
> +		igt_output_set_pipe(output, pipe);
> +		mode = igt_output_get_mode(output);
> +
> +		/* Create two framebuffers with the same color info. */
> +		igt_create_color_fb(data->drm_fd,
> +				mode->hdisplay, mode->vdisplay,
> +				DRM_FORMAT_XRGB8888,
> +				LOCAL_DRM_FORMAT_MOD_NONE,
> +				1.0, 1.0, 1.0,
> +				&fb0);
> +		igt_create_color_fb(data->drm_fd,
> +				mode->hdisplay, mode->vdisplay,
> +				DRM_FORMAT_XRGB8888,
> +				LOCAL_DRM_FORMAT_MOD_NONE,
> +				1.0, 1.0, 1.0,
> +				&fb1);
> +
> +		/* Flip FB0 with the Primary plane & collect the CRC as ref CRC. */
> +		primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
> +		igt_plane_set_fb(primary, &fb0);
> +		igt_display_commit(display);
> +
> +		if (pipe_crc)
> +			igt_pipe_crc_free(pipe_crc);

Is this check required?
> +
> +		pipe_crc = igt_pipe_crc_new(data->drm_fd, pipe,
> +					    INTEL_PIPE_CRC_SOURCE_AUTO);
> +		igt_pipe_crc_collect_crc(pipe_crc, &ref_crc);
> +
> +		/* Flip FB1 with the Primary plane & compare the CRC with ref CRC. */
> +		igt_plane_set_fb(primary, &fb1);
> +		igt_display_commit(display);
> +
> +		igt_pipe_crc_collect_crc(pipe_crc, &crc);
> +		igt_assert_crc_equal(&crc, &ref_crc);
> +
> +		/* Clean-up */
> +		igt_pipe_crc_free(pipe_crc);
> +		pipe_crc = NULL;
> +		igt_plane_set_fb(primary, NULL);
> +		igt_output_set_pipe(output, PIPE_NONE);
> +		igt_display_commit(display);
> +
> +		igt_remove_fb(data->drm_fd, &fb0);
> +		igt_remove_fb(data->drm_fd, &fb1);
> +
> +		/* once is enough */
> +		return;
> +	}
> +
> +	igt_skip("no valid crtc/connector combinations found\n");

Based on the comments above, we can remove the return and igt_skip();
> +}
> +'
>   data_t data = {0, };
>   
>   igt_main
> @@ -211,6 +285,9 @@ igt_main
>   		}
>   	}
>   
> +	igt_subtest("compare-crc-basic")
> +                test_check_crc(&data);

Should we have this at a per pipe level like the other subtests?
May be dynamic-subtests, but not sure because other tests in this IGT 
are not dynamic.

Thanks,
Karthik.B.S
> +
>   	igt_fixture {
>   		igt_display_fini(&data.display);
>   	}
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev