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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 05-11-18 om 12:32 schreef Juha-Pekka Heikkila:
> Fixed handling of single plane modes so used bpp never get to
> be 0 and ease off crc comparison a bit with gamma tables as
> idea of this test is to see found modes are able to initialize.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106641
> 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 | 78 ++++++++++++++++++++++++-----------------
>  1 file changed, 45 insertions(+), 33 deletions(-)
>
> diff --git a/tests/kms_available_modes_crc.c b/tests/kms_available_modes_crc.c
> index 45a4775..69b3306 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;
>  
>  
> @@ -133,19 +139,10 @@ static const struct {
>  	{ DRM_FORMAT_RGB565, 0, BYTES_PP_2, 0xffff},
>  	{ DRM_FORMAT_XRGB8888, 0, BYTES_PP_4, 0xffffffff},
>  	{ DRM_FORMAT_XBGR8888, 0, BYTES_PP_4, 0xffffffff},
> -
> -	/*
> -	 * following two are skipped because blending seems to work
> -	 * incorrectly with exception of AR24 on cursor plane.
> -	 * 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, 0, BYTES_PP_4, 0xffffffff},
> +	{ DRM_FORMAT_ABGR8888, 0, BYTES_PP_4, 0x00ffffff},
>  	{ DRM_FORMAT_XRGB2101010, 0, BYTES_PP_4, 0xffffffff},
>  	{ DRM_FORMAT_XBGR2101010, 0, 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},
> @@ -164,7 +161,7 @@ static const struct {
>  #ifdef DRM_FORMAT_P016
>  	{ DRM_FORMAT_P016, 0, P010, 0x8000eb00},
>  #endif
> -	{ 0, 0, 0, 0 }
> +	{ 0, 0, SKIP4, 0 }
>  };
>  
>  /*
> @@ -221,24 +218,9 @@ static bool fill_in_fb(data_t *data, igt_output_t *output, igt_plane_t *plane,
>  		writesize = data->size+data->size/2;
>  		break;
>  	case SKIP4:
> -		if (fillers[i].fourcc == DRM_FORMAT_ARGB8888 &&
> -		    plane->type == DRM_PLANE_TYPE_CURSOR) {
> -		/*
> -		 * special for cursor plane where blending works correctly.
> -		 */
> -			ptemp_32_buf = (unsigned int*)data->buf;
> -			for (c = 0; c < data->size/4; c++)
> -				ptemp_32_buf[c] = fillers[i].value;
> -			writesize = data->size;
> -			break;
> -		}
> -		igt_info("Format %s CRC comparison skipped by design.\n",
> -			 (char*)&fillers[i].fourcc);
> -
> -		return false;
>  	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;
>  	}
>  
> @@ -261,6 +243,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;
> @@ -289,6 +273,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 */
>  	case BYTES_PP_4:
>  		bpp = 32;
>  		break;
> @@ -412,7 +398,8 @@ test_one_mode(data_t* data, igt_output_t *output, igt_plane_t* plane,
>  					igt_warn("crc mismatch. target %.8s, result %.8s.\n", crccompare[0], crccompare[1]);
>  					free(crccompare[0]);
>  					free(crccompare[1]);
> -					rVal++;
> +					if (!data->allow_fail)
> +						rVal++;
>  				}
>  			} else {
>  				if (!igt_check_crc_equal(&current_crc,
> @@ -422,14 +409,15 @@ test_one_mode(data_t* data, igt_output_t *output, igt_plane_t* plane,
>  					igt_warn("crc mismatch. target %.8s, result %.8s.\n", crccompare[0], crccompare[1]);
>  					free(crccompare[0]);
>  					free(crccompare[1]);
> -					rVal++;
> +					if (!data->allow_fail)
> +						rVal++;
>  				}
>  			}
>  		}
>  		remove_fb(data, output, plane);
>  		return rVal;
>  	}
> -	return 1;
> +	return data->allow_fail?0:1;
>  }
>  
>  
> @@ -438,16 +426,31 @@ test_available_modes(data_t* data)
>  {
>  	igt_output_t *output;
>  	igt_plane_t *plane;
> +	igt_pipe_t *pipe_obj;
>  	int modeindex;
>  	enum pipe pipe;
> -	int invalids = 0;
> +	int invalids = 0, i, lut_size;
> +	uint16_t *lut;
>  	drmModePlane *modePlane;
> +	drmModeCrtc *crtc;
>  	char planetype[3][8] = {"OVERLAY\0", "PRIMARY\0", "CURSOR\0" };
>  
>  	for_each_pipe_with_valid_output(&data->display, pipe, output) {
>  		igt_output_set_pipe(output, pipe);
>  		igt_display_commit2(&data->display, data->commit);
>  
> +		pipe_obj = &data->display.pipes[pipe];
> +		crtc = drmModeGetCrtc(data->gfx_fd, pipe_obj->crtc_id);
> +		lut_size = crtc->gamma_size;
> +		drmModeFreeCrtc(crtc);

if (igt_pipe_obj_get_prop(IGT_CRTC_GAMMA_LUT_SIZE)) {
lut_size = igt_pipe_get_prop(IGT_CRTC_GAMMA_LUT_SIZE)
etc?
}
> +		lut = malloc(sizeof(uint16_t) * lut_size);
> +
> +		for (i = 0; i < lut_size; i++)
> +			lut[i] = (i * 0xffff / (lut_size - 1)) & 0xfc00;
> +
> +		drmModeCrtcSetGamma(data->gfx_fd, pipe_obj->crtc_id,
> +				    lut_size, lut, lut, lut);
And call igt_pipe_replace_prop_blob() with IGT_CRTC_GAMMA_LUT instead, it will be committed on next igt_display_commit2()
>  		data->pipe_crc = igt_pipe_crc_new(data->gfx_fd, pipe,
>  						  INTEL_PIPE_CRC_SOURCE_AUTO);
>  
> @@ -484,6 +487,15 @@ test_available_modes(data_t* data)
>  		igt_pipe_crc_stop(data->pipe_crc);
>  		igt_pipe_crc_free(data->pipe_crc);
>  		igt_display_commit2(&data->display, data->commit);
> +
> +		for (i = 0; i < lut_size; i++)
> +			lut[i] = i * 0xffff / (lut_size - 1);
> +
> +		drmModeCrtcSetGamma(data->gfx_fd, pipe_obj->crtc_id,
> +				    lut_size, lut, lut, lut);
> +		free(lut);
> +		igt_output_set_pipe(output, PIPE_ANY);
Same here. We're trying to keep out the legacy crap where possible. :)

Hm, is the igt_display_commit2() at the end needed, or could it be an igt_display_reset at the start?
If you can't put in a reset, then igt_pipe_replace_prop_blob(IGT_CRTC_GAMMA_LUT, NULL, 0)
>  	}
>  	igt_assert(invalids == 0);
>  }


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