[igt-dev] [PATCH i-g-t] tests/kms_cursor_crc: refactoring cursor-alpha subtests

[Arkadiusz Hiler]

On Wed, Aug 26, 2020 at 08:17:33AM -0300, Melissa Wen wrote:
> Considering just a fully opaque or fully transparent cursor is not enough
> to check the composition of an ARGB cursor plane. For example, the cairo
> ARGB32 format uses pre-multiplied alpha, and this representation may only
> be evident when testing a translucent cursor.
> 
> Therefore, this patch refactors the cursor-alpha-opaque and
> cursor-alpha-transparent subtests into just one subtest (cursor-alpha)
> that checks the alpha blending of a white cursor, with different alpha
> values, in the primary plane. This refactoring also generates some setup
> stuffs savings.
> 
> Signed-off-by: Melissa Wen <melissa.srw at gmail.com>
> ---
>  tests/kms_cursor_crc.c | 82 ++++++++++++++++++++----------------------
>  1 file changed, 39 insertions(+), 43 deletions(-)
> 
> diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c
> index e9491847..e200b0b5 100644
> --- a/tests/kms_cursor_crc.c
> +++ b/tests/kms_cursor_crc.c
> @@ -478,65 +478,66 @@ static void prepare_crtc(data_t *data, igt_output_t *output,
>  	igt_pipe_crc_start(data->pipe_crc);
>  }
>  
> -static void test_cursor_alpha(data_t *data, double a)
> +static void test_cursor_alpha(data_t *data)
>  {
>  	igt_display_t *display = &data->display;
>  	igt_pipe_crc_t *pipe_crc = data->pipe_crc;
> -	igt_crc_t crc, ref_crc;
> +	igt_crc_t crc[5], ref_crc;
>  	cairo_t *cr;
>  	uint32_t fb_id;
>  	int curw = data->curw;
>  	int curh = data->curh;
> +	int i;
> +	double alpha;
>  
> -	/* Alpha cursor fb with white color */
> +	/* Alpha cursor fb */
>  	fb_id = igt_create_fb(data->drm_fd, curw, curh,
>  				    DRM_FORMAT_ARGB8888,
>  				    LOCAL_DRM_FORMAT_MOD_NONE,
>  				    &data->fb);
>  	igt_assert(fb_id);
> +
> +	/* 
> +	 * Hardware Test
> +	 * With the cursor enabled, get the PF CRCs from the composition with a
> +	 * white cursor with different alpha values.
> +	 */
>  	cr = igt_get_cairo_ctx(data->drm_fd, &data->fb);
> -	igt_paint_color_alpha(cr, 0, 0, curw, curh, 1.0, 1.0, 1.0, a);
> -	igt_put_cairo_ctx(cr);
> +	cairo_set_operator (cr, CAIRO_OPERATOR_SOURCE);
>  
> -	/* Hardware Test - enable cursor and get PF CRC */
>  	cursor_enable(data);
> -	igt_display_commit(display);
> -	igt_wait_for_vblank(data->drm_fd,
> -			display->pipes[data->pipe].crtc_offset);
> -	igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &crc);
> +	for (i = 0, alpha = 1.0; alpha >= 0.0; alpha -= 0.25, i++) {
> +		igt_paint_color_alpha(cr, 0, 0, curw, curh, 1.0, 1.0, 1.0, alpha);
> +
> +		igt_display_commit(display);
> +		igt_wait_for_vblank(data->drm_fd,
> +				    display->pipes[data->pipe].crtc_offset);
> +		igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &crc[i]);
> +	}
> +	igt_put_cairo_ctx(cr);
>  
>  	cursor_disable(data);
>  	igt_remove_fb(data->drm_fd, &data->fb);
>  
> -	/* Software Test - render cursor in software, drawn it directly on PF */
> +	/* Software Test - render cursor in software, drawn it directly on PF*/
>  	cr = igt_get_cairo_ctx(data->drm_fd, &data->primary_fb[FRONTBUFFER]);
> -	igt_paint_color_alpha(cr, 0, 0, curw, curh, 1.0, 1.0, 1.0, a);
> -	igt_put_cairo_ctx(cr);
>  
> -	igt_display_commit(display);
> -	igt_wait_for_vblank(data->drm_fd,
> -			display->pipes[data->pipe].crtc_offset);
> -	igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &ref_crc);
> +	for (i = 0, alpha = 1.0; alpha >= 0.0; alpha -= 0.25, i++) {
> +		igt_paint_color_alpha(cr, 0, 0, curw, curh, 1.0, 1.0, 1.0, alpha);
>  
> -	/* Compare CRC from Hardware/Software tests */
> -	igt_assert_crc_equal(&crc, &ref_crc);
> -
> -	/*Clear Screen*/
> -	cr = igt_get_cairo_ctx(data->drm_fd, &data->primary_fb[FRONTBUFFER]);
> -	igt_paint_color(cr, 0, 0, data->screenw, data->screenh,
> -			0.0, 0.0, 0.0);
> -	igt_put_cairo_ctx(cr);
> -}
> -
> -static void test_cursor_transparent(data_t *data)
> -{
> -	test_cursor_alpha(data, 0.0);
> +		igt_display_commit(display);
> +		igt_wait_for_vblank(data->drm_fd,
> +				display->pipes[data->pipe].crtc_offset);
> +		igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &ref_crc);
>  
> -}
> +		/* Compare CRC from Hardware/Software tests */
> +		igt_assert_crc_equal(&crc[i], &ref_crc);

Hey,

Sorry it took so long for me to review this. I was taking a bit of a
break from DRM work.

The change is sound, checking multiple alpha levels makes sense, and
looks like the test mostly passes on real HW - I was a bit afraid that
there may be some discrepancies between SW and HW implementation of
alpha blending.

On Intel's CI side - GLK is misbehaving a bit:
https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4907/shards-all.html?testfilter=cursor-alpha

But that looks like it's a HW problem. It's for alpha=0.5, which wasn't
tested before and other generations of hardware are passing the test just
fine.

It would be nice to improve logging in the test a bit:
 * don't fail the test early, always go through all of the alpha levels
 * clearly state for which alpha levels we have failed, right now you
   I have to count the number of "commits" in the debug logs

We've been doing similar improvements in other tests.

-- 
Cheers,
Arek