Re: [igt-dev] [PATCH i-g-t] tests/kms_available_modes_crc: Fix handling of test comparisons

[Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>]

On Fri, 2019-01-25 at 14:30 +0200, Juha-Pekka Heikkila wrote:
> Fixed handling of single plane modes so used bpp never get to
> be 0.
> 
> Reduce verboseness if there's no errors.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106701
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> ---
>  tests/kms_available_modes_crc.c | 123 ++++++++++++++++++++--------
> ------------
>  1 file changed, 61 insertions(+), 62 deletions(-)
> 
> diff --git a/tests/kms_available_modes_crc.c
> b/tests/kms_available_modes_crc.c
> index 7ff385f..432c084 100644
> --- a/tests/kms_available_modes_crc.c
> +++ b/tests/kms_available_modes_crc.c
> @@ -54,6 +54,12 @@ typedef struct {
>  
>  	igt_crc_t cursor_crc;
>  	igt_crc_t fullscreen_crc;
> +
> +	/*
> +	 * If there's unknown format setting up mode can fail
> +	 * without failing entire test.
> +	 */
> +	bool allow_fail;
>  } data_t;
>  
>  
> @@ -120,7 +126,6 @@ static void generate_comparison_crc_list(data_t
> *data, igt_output_t *output)
>  
>  static const struct {
>  	uint32_t	fourcc;
> -	char		zeropadding;
>  	enum		{ BYTES_PP_1=1,
>  				BYTES_PP_2=2,
>  				BYTES_PP_4=4,
> @@ -129,10 +134,10 @@ static const struct {
>  				SKIP4 } bpp;
>  	uint32_t	value;
>  } fillers[] = {
> -	{ DRM_FORMAT_C8, 0, BYTES_PP_1, 0xff},
> -	{ DRM_FORMAT_RGB565, 0, BYTES_PP_2, 0xffff},
> -	{ DRM_FORMAT_XRGB8888, 0, BYTES_PP_4, 0xffffffff},
> -	{ DRM_FORMAT_XBGR8888, 0, BYTES_PP_4, 0xffffffff},
> +	{ DRM_FORMAT_C8, BYTES_PP_1, 0xff},
> +	{ DRM_FORMAT_RGB565, BYTES_PP_2, 0xffff},
> +	{ DRM_FORMAT_XRGB8888, BYTES_PP_4, 0xffffffff},
> +	{ DRM_FORMAT_XBGR8888, BYTES_PP_4, 0xffffffff},
>  
>  	/*
>  	 * following two are skipped because blending seems to work
> @@ -140,31 +145,31 @@ static const struct {
>  	 * Test still creates the planes, just filling plane
>  	 * and getting crc is skipped.
>  	 */
> -	{ DRM_FORMAT_ARGB8888, 0, SKIP4, 0xffffffff},
> -	{ DRM_FORMAT_ABGR8888, 0, SKIP4, 0x00ffffff},
> +	{ DRM_FORMAT_ARGB8888, SKIP4, 0xffffffff},
> +	{ DRM_FORMAT_ABGR8888, SKIP4, 0x00ffffff},
>  
> -	{ DRM_FORMAT_XRGB2101010, 0, BYTES_PP_4, 0xffffffff},
> -	{ DRM_FORMAT_XBGR2101010, 0, BYTES_PP_4, 0xffffffff},
> +	{ DRM_FORMAT_XRGB2101010, BYTES_PP_4, 0xffffffff},
> +	{ DRM_FORMAT_XBGR2101010, BYTES_PP_4, 0xffffffff},
>  
> -	{ DRM_FORMAT_YUYV, 0, BYTES_PP_4, 0x80eb80eb},
> -	{ DRM_FORMAT_YVYU, 0, BYTES_PP_4, 0x80eb80eb},
> -	{ DRM_FORMAT_VYUY, 0, BYTES_PP_4, 0xeb80eb80},
> -	{ DRM_FORMAT_UYVY, 0, BYTES_PP_4, 0xeb80eb80},
> +	{ DRM_FORMAT_YUYV, BYTES_PP_4, 0x80eb80eb},
> +	{ DRM_FORMAT_YVYU, BYTES_PP_4, 0x80eb80eb},
> +	{ DRM_FORMAT_VYUY, BYTES_PP_4, 0xeb80eb80},
> +	{ DRM_FORMAT_UYVY, BYTES_PP_4, 0xeb80eb80},
>  
>  	/*
>  	 * (semi-)planar formats
>  	 */
> -	{ DRM_FORMAT_NV12, 0, NV12, 0x80eb},
> +	{ DRM_FORMAT_NV12, NV12, 0x80eb},
>  #ifdef DRM_FORMAT_P010
> -	{ DRM_FORMAT_P010, 0, P010, 0x8000eb00},
> +	{ DRM_FORMAT_P010, P010, 0x8000eb00},
>  #endif
>  #ifdef DRM_FORMAT_P012
> -	{ DRM_FORMAT_P012, 0, P010, 0x8000eb00},
> +	{ DRM_FORMAT_P012, P010, 0x8000eb00},
>  #endif
>  #ifdef DRM_FORMAT_P016
> -	{ DRM_FORMAT_P016, 0, P010, 0x8000eb00},
> +	{ DRM_FORMAT_P016, P010, 0x8000eb00},

Doesn't the library implement this stuff already? I see similar code in
create_bo_for_fb(). Even if new formats are needed, I think it's best
to put that in the lib/igt_fb.


>  #endif
> -	{ 0, 0, 0, 0 }
> +	{ 0, SKIP4, 0 }
>  };
>  
>  /*
> @@ -233,13 +238,10 @@ static bool fill_in_fb(data_t *data,
> igt_output_t *output, igt_plane_t *plane,
>  			writesize = data->size;
>  			break;
>  		}
> -		igt_info("Format %s CRC comparison skipped by
> design.\n",
> -			 (char*)&fillers[i].fourcc);
> -
> -		return false;
> +	/* Fall through */
>  	default:
> -		igt_info("Unsupported mode for test %s\n",
> -			 (char*)&fillers[i].fourcc);
> +		igt_info("Unsupported mode for testing CRC %.4s\n",
> +			 (char *)&format);
>  		return false;
>  	}
>  
> @@ -260,6 +262,8 @@ static bool setup_fb(data_t *data, igt_output_t
> *output, igt_plane_t *plane,
>  	int bpp = 0;
>  	int i;
>  
> +	data->allow_fail = false;
> +
>  	mode = igt_output_get_mode(output);
>  	if (plane->type != DRM_PLANE_TYPE_CURSOR) {
>  		w = mode->hdisplay;
> @@ -288,6 +292,8 @@ static bool setup_fb(data_t *data, igt_output_t
> *output, igt_plane_t *plane,
>  		break;
>  
>  	case SKIP4:
> +		data->allow_fail = true;
> +		/* fall through */
Wouldn't this prevent CRC testing for 
(fillers[i].fourcc == DRM_FORMAT_ARGB8888 && plane->type ==
DRM_PLANE_TYPE_CURSOR) ?


>  	case BYTES_PP_4:
>  		bpp = 32;
>  		break;
> @@ -324,7 +330,7 @@ static bool setup_fb(data_t *data, igt_output_t
> *output, igt_plane_t *plane,
>  
>  	if(ret < 0) {
>  		igt_info("Creating fb for format %s failed, return code
> %d\n",
> -			 (char*)&data->format.name, ret);
> +			 (char *)&data->format.name, ret);
>  
>  		return false;
>  	}
> @@ -385,13 +391,14 @@ static bool prepare_crtc(data_t *data,
> igt_output_t *output,
>  
>  
>  static int
> -test_one_mode(data_t* data, igt_output_t *output, igt_plane_t* 
Side note not related to the changes you are making, "mode" is quite
misleading. Shouldn't the test be called kms_pixel_formats_crc or
something? I thought this was about testing different connector modes.


> plane,
> -	      int mode)
> +test_one_mode(data_t *data, igt_output_t *output, igt_plane_t
> *plane,
> +	      enum pipe pipe, int mode)
>  {
>  	igt_crc_t current_crc;
>  	signed rVal = 0;
>  	bool do_crc;
> -	char* crccompare[2];
> +	char *crccompare[2];
> +	igt_crc_t *p_crc;
>  
>  	if (prepare_crtc(data, output, plane, mode)){
>  		/*
> @@ -409,32 +416,32 @@ test_one_mode(data_t* data, igt_output_t
> *output, igt_plane_t* plane,
>  		if (do_crc) {
>  			igt_pipe_crc_get_current(data->gfx_fd, data-
> >pipe_crc, &current_crc);
>  
> -			if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> -				if (!igt_check_crc_equal(&current_crc,
> -					&data->fullscreen_crc)) {
> -					crccompare[0] =
> igt_crc_to_string(&current_crc);
> -					crccompare[1] =
> igt_crc_to_string(&data->fullscreen_crc);
> -					igt_warn("crc mismatch. target
> %.8s, result %.8s.\n", crccompare[0], crccompare[1]);
> -					free(crccompare[0]);
> -					free(crccompare[1]);
> -					rVal++;
> -				}
> -			} else {
> -				if (!igt_check_crc_equal(&current_crc,
> -					&data->cursor_crc)) {
> -					crccompare[0] =
> igt_crc_to_string(&current_crc);
> -					crccompare[1] =
> igt_crc_to_string(&data->cursor_crc);
> -					igt_warn("crc mismatch. target
> %.8s, result %.8s.\n", crccompare[0], crccompare[1]);
> -					free(crccompare[0]);
> -					free(crccompare[1]);
> +			if (plane->type != DRM_PLANE_TYPE_CURSOR)
> +				p_crc = &data->fullscreen_crc;
> +			else
> +				p_crc = &data->cursor_crc;
> +
> +			if (!igt_check_crc_equal(&current_crc, p_crc))
> {
> +				crccompare[0] =
> igt_crc_to_string(&current_crc);
> +				crccompare[1] =
> igt_crc_to_string(p_crc);
> +				igt_warn("crc mismatch on %s PIPE %s
> plane %d format %.4s. target %.8s, result %.8s.\n",
> +					 igt_output_name(output),
> +					 kmstest_pipe_name(pipe),
> +					 plane->index,
> +					 (char *)&data->format.name,
> +					 crccompare[0],
> +					 crccompare[1]);
This is the only hunk I was hoping you would be able to split into a
separate patch.

> +
> +				free(crccompare[0]);
> +				free(crccompare[1]);
> +				if (!data->allow_fail)
Is there a point of reading CRCs if failures are allowed?

>  					rVal++;
> -				}
>  			}
>  		}
>  		remove_fb(data, output, plane);
>  		return rVal;
>  	}
> -	return 1;
> +	return data->allow_fail ? 0 : 1;
>  }
>  
>  
> @@ -445,11 +452,11 @@ test_available_modes(data_t* data)
>  	igt_plane_t *plane;
>  	int modeindex;
>  	enum pipe pipe;
> -	int invalids = 0;
> +	int failed_crcs = 0;
>  	drmModePlane *modePlane;
> -	char planetype[3][8] = {"OVERLAY\0", "PRIMARY\0", "CURSOR\0" };
>  
>  	for_each_pipe_with_valid_output(&data->display, pipe, output) {
> +		igt_display_reset(&data->display);
>  		igt_output_set_pipe(output, pipe);
>  		igt_display_commit2(&data->display, data->commit);
>  
> @@ -472,17 +479,9 @@ test_available_modes(data_t* data)
>  			     modeindex++) {
>  				data->format.dword = modePlane-
> >formats[modeindex];
>  
> -				igt_info("Testing connector %s using
> pipe %s" \
> -					 " plane index %d type %s mode
> %s\n",
> -					 igt_output_name(output),
> -					 kmstest_pipe_name(pipe),
> -					 plane->index,
> -					 planetype[plane->type],
> -					 (char*)&data->format.name);
> -
> -				invalids += test_one_mode(data, output,
> -							  plane,
> -							  modePlane-
> >formats[modeindex]);
> +				failed_crcs += test_one_mode(data,
> output,
> +							     plane,
> pipe,
> +							     modePlane-
> >formats[modeindex]);
>  			}
>  			drmModeFreePlane(modePlane);
>  		}
> @@ -491,7 +490,7 @@ test_available_modes(data_t* data)
>  		igt_display_commit2(&data->display, data->commit);
>  		igt_output_set_pipe(output, PIPE_NONE);
>  	}
> -	igt_assert(invalids == 0);
> +	igt_assert_f((failed_crcs == 0), "There was %d modes with
> invalid crc\n", failed_crcs);
>  }
>  
>  

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev