[igt-dev] [PATCH i-g-t, 1/2] tests/kms_cursor_crc.c: Improve test_cursor_alpha()

[Rodrigo Siqueira]

Hi,

I checked your patch in two environments:

1. My computer with i915 and kernel 2.20;
2. In a QEMU machine with a patched version of VKMS and running the
   Kernel 5.0.

All the tested completed with success \o/

I Just have some tiny comments inline.

On 01/24, Mamta Shukla wrote:
> Changes in test_cursor_alpha() to fix CRC mismatch error
> [1] Set cursor plane in HW test using drmModeSetCursor()
> [2] Remove igt_display_commit()
> [3] Add igt_remove_fb() after disabling the cursor plane in HW test.
> 
> This function is aligned with test_cursor_size.
> With the above changes,got passing results for alpha blending support
> added in VKMS CRC API.

Add some additional information about the problem in the commit body.
Additionally, try to improve the commit message to better describe the
problem that you solved here.
 
> Signed-off-by: Mamta Shukla <mamtashukla555 at gmail.com>
> ---
>  tests/kms_cursor_crc.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c
> index 3c9856d9..c1ea7faf 100644
> --- a/tests/kms_cursor_crc.c
> +++ b/tests/kms_cursor_crc.c
> @@ -413,24 +413,26 @@ static void test_cursor_alpha(data_t *data, double a)
>  	uint32_t fb_id;
>  	int curw=data->curw;
>  	int curh=data->curh;
> +	int ret;
>  
>  	/*alpha cursor fb*/
> -	fb_id = igt_create_color_fb(data->drm_fd, curw, curh,
> +	fb_id = igt_create_fb(data->drm_fd, curw, curh,
>  				    DRM_FORMAT_ARGB8888,
>  				    LOCAL_DRM_FORMAT_MOD_NONE,
> -				    1.0, 1.0, 1.0,
>  				    &data->fb);
>  	igt_assert(fb_id);
>  	cr = igt_get_cairo_ctx(data->drm_fd, &data->fb);
> -	draw_cursor(cr, 0, 0, curw, curh, a);
> +	igt_paint_color_alpha(cr, 0, 0, curw, curh, 1.0, 1.0, 1.0, a);
>  	igt_put_cairo_ctx(data->drm_fd, &data->fb, cr);
>  
>  	/*Hardware Test*/
>  	cursor_enable(data);
> -	igt_display_commit(display);
> +	ret=drmModeSetCursor(data->drm_fd, data->output->config.crtc->crtc_id, data->fb.gem_handle, curw, curh);

Add spaces between '='.

Finally, IMHO patch 1 and 2 could be merged in a single one.

Best Regards
Rodrigo Siqueira

> +	igt_assert_eq(ret, 0);
>  	igt_wait_for_vblank(data->drm_fd, data->pipe);
>  	igt_pipe_crc_collect_crc(pipe_crc, &crc);
>  	cursor_disable(data);
> +	igt_remove_fb(data->drm_fd, &data->fb);
>  
>  	/*Software Test*/
>  	cr = igt_get_cairo_ctx(data->drm_fd, &data->primary_fb);
> @@ -447,7 +449,6 @@ static void test_cursor_alpha(data_t *data, double a)
>  	igt_paint_color(cr, 0, 0, data->screenw, data->screenh,
>  			0.0, 0.0, 0.0);
>  	igt_put_cairo_ctx(data->drm_fd, &data->primary_fb, cr);
> -	igt_remove_fb(data->drm_fd, &data->fb);
>  }
>  
>  static void test_cursor_transparent(data_t *data)
> -- 
> 2.17.1
> 

-- 
Rodrigo Siqueira
https://siqueira.tech
Graduate Student
Department of Computer Science
University of São Paulo