Re: [igt-dev] [PATCH i-g-t v2 2/3] tests/kms_plane: Test all YCbCr encodings/ranges

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Jul 02, 2019 at 06:07:24PM +0000, Kazlauskas, Nicholas wrote:
> On 7/2/19 1:57 PM, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Test all support YCbCr encoding/range combinations. So far
> > we've only been testing one particular combo.
> > 
> > v2: Use igt_create_fb_with_bo_size() and hand roll the
> >      solid fills
> > 
> > Cc: Uma Shankar <uma.shankar@intel.com>
> > Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> #v1
> > Reviewed-by: Uma Shankar <uma.shankar@intel.com> #v1
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   tests/kms_plane.c | 164 ++++++++++++++++++++++++++++++++++++----------
> >   1 file changed, 129 insertions(+), 35 deletions(-)
> > 
> > diff --git a/tests/kms_plane.c b/tests/kms_plane.c
> > index 59d5f1e80cd2..ce4bf9e1c21d 100644
> > --- a/tests/kms_plane.c
> > +++ b/tests/kms_plane.c
> > @@ -427,27 +427,36 @@ static void test_format_plane_color(data_t *data, enum pipe pipe,
> >   				    igt_plane_t *plane,
> >   				    uint32_t format, uint64_t modifier,
> >   				    int width, int height,
> > +				    enum igt_color_encoding color_encoding,
> > +				    enum igt_color_range color_range,
> >   				    int color, igt_crc_t *crc, struct igt_fb *fb)
> >   {
> >   	const color_t *c = &colors[color];
> >   	struct igt_fb old_fb = *fb;
> > +	cairo_t *cr;
> >   
> >   	if (data->crop == 0 || format == DRM_FORMAT_XRGB8888) {
> > -		igt_create_color_fb(data->drm_fd, width, height,
> > -				    format, modifier,
> > -				    c->red, c->green, c->blue, fb);
> > +		igt_create_fb_with_bo_size(data->drm_fd, width, height,
> > +					   format, modifier, color_encoding,
> > +					   color_range, fb, 0, 0);
> > +
> > +		cr = igt_get_cairo_ctx(data->drm_fd, fb);
> > +
> > +		igt_paint_color(cr, 0, 0, width, height,
> > +				c->red, c->green, c->blue);
> > +
> > +		igt_put_cairo_ctx(data->drm_fd, fb, cr);
> >   	} else {
> > +		igt_create_fb_with_bo_size(data->drm_fd,
> > +					   width + data->crop * 2,
> > +					   height + data->crop * 2,
> > +					   format, modifier, color_encoding,
> > +					   color_range, fb, 0, 0);
> > +
> >   		/*
> >   		 * paint border in inverted color, then visible area in middle
> >   		 * with correct color for clamping test
> >   		 */
> > -		cairo_t *cr;
> > -
> > -		igt_create_fb(data->drm_fd,
> > -			      width + data->crop * 2,
> > -			      height + data->crop * 2,
> > -			      format, modifier, fb);
> > -
> >   		cr = igt_get_cairo_ctx(data->drm_fd, fb);
> >   
> >   		igt_paint_color(cr, 0, 0,
> > @@ -500,6 +509,104 @@ static int num_unique_crcs(const igt_crc_t crc[], int num_crc)
> >   	return num_unique_crc;
> >   }
> >   
> > +static bool test_format_plane_colors(data_t *data, enum pipe pipe,
> > +				     igt_plane_t *plane,
> > +				     uint32_t format, uint64_t modifier,
> > +				     int width, int height,
> > +				     enum igt_color_encoding encoding,
> > +				     enum igt_color_range range,
> > +				     igt_crc_t ref_crc[ARRAY_SIZE(colors)],
> > +				     struct igt_fb *fb)
> > +{
> > +	int crc_mismatch_count = 0;
> > +	unsigned int crc_mismatch_mask = 0;
> > +	bool result = true;
> > +
> > +	for (int i = 0; i < ARRAY_SIZE(colors); i++) {
> > +		igt_crc_t crc;
> > +
> > +		test_format_plane_color(data, pipe, plane,
> > +					format, modifier,
> > +					width, height,
> > +					encoding, range,
> > +					i, &crc, fb);
> > +
> > +		if (!igt_check_crc_equal(&crc, &ref_crc[i])) {
> > +			crc_mismatch_count++;
> > +			crc_mismatch_mask |= (1 << i);
> > +			result = false;
> > +		}
> > +	}
> > +
> > +	if (crc_mismatch_count)
> > +		igt_warn("CRC mismatches with format " IGT_FORMAT_FMT " on %s.%u with %d/%d solid colors tested (0x%X)\n",
> > +			 IGT_FORMAT_ARGS(format), kmstest_pipe_name(pipe),
> > +			 plane->index, crc_mismatch_count, (int)ARRAY_SIZE(colors), crc_mismatch_mask);
> > +
> > +	return result;
> > +}
> > +
> > +static bool test_format_plane_rgb(data_t *data, enum pipe pipe,
> > +				  igt_plane_t *plane,
> > +				  uint32_t format, uint64_t modifier,
> > +				  int width, int height,
> > +				  igt_crc_t ref_crc[ARRAY_SIZE(colors)],
> > +				  struct igt_fb *fb)
> > +{
> > +	igt_info("Testing format " IGT_FORMAT_FMT " / modifier 0x%" PRIx64 " on %s.%u\n",
> > +		 IGT_FORMAT_ARGS(format), modifier,
> > +		 kmstest_pipe_name(pipe), plane->index);
> > +
> > +	return test_format_plane_colors(data, pipe, plane,
> > +					format, modifier,
> > +					width, height,
> > +					IGT_COLOR_YCBCR_BT601,
> > +					IGT_COLOR_YCBCR_LIMITED_RANGE,
> > +					ref_crc, fb);
> > +}
> > +
> > +static bool test_format_plane_yuv(data_t *data, enum pipe pipe,
> > +				  igt_plane_t *plane,
> > +				  uint32_t format, uint64_t modifier,
> > +				  int width, int height,
> > +				  igt_crc_t ref_crc[ARRAY_SIZE(colors)],
> > +				  struct igt_fb *fb)
> > +{
> > +	bool result = true;
> > +
> > +	if (!igt_plane_has_prop(plane, IGT_PLANE_COLOR_ENCODING))
> > +		return true;
> > +	if (!igt_plane_has_prop(plane, IGT_PLANE_COLOR_RANGE))
> > +		return true;
> 
> Skimmed over this in my first pass.
> 
> Do we really want to be returning true here if the driver doesn't 
> support encoding/range? It's probably not a good idea to really expose 
> YUV formats with them so it might be best to just assert here instead.

I suspect there are still quite a few drivers that don't expose
these props. Asserting/skipping here would mean skipping the entire
subtest which is not so great. So IMO better to just skip the YUV
formats.

> 
> The rest of P1+P2 lgtm. I don't see P3 but that still only needs the 
> title fix.
> 
> Nicholas Kazlauskas
> 
> > +
> > +	for (enum igt_color_encoding e = 0; e < IGT_NUM_COLOR_ENCODINGS; e++) {
> > +		if (!igt_plane_try_prop_enum(plane,
> > +					     IGT_PLANE_COLOR_ENCODING,
> > +					     igt_color_encoding_to_str(e)))
> > +			continue;
> > +
> > +		for (enum igt_color_range r = 0; r < IGT_NUM_COLOR_RANGES; r++) {
> > +			if (!igt_plane_try_prop_enum(plane,
> > +						     IGT_PLANE_COLOR_RANGE,
> > +						     igt_color_range_to_str(r)))
> > +				continue;
> > +
> > +			igt_info("Testing format " IGT_FORMAT_FMT " / modifier 0x%" PRIx64 " (%s, %s) on %s.%u\n",
> > +				 IGT_FORMAT_ARGS(format), modifier,
> > +				 igt_color_encoding_to_str(e),
> > +				 igt_color_range_to_str(r),
> > +				 kmstest_pipe_name(pipe), plane->index);
> > +
> > +			result &= test_format_plane_colors(data, pipe, plane,
> > +							   format, modifier,
> > +							   width, height,
> > +							   e, r, ref_crc, fb);
> > +		}
> > +	}
> > +
> > +	return result;
> > +}
> > +
> >   static bool test_format_plane(data_t *data, enum pipe pipe,
> >   			      igt_output_t *output, igt_plane_t *plane)
> >   {
> > @@ -569,6 +676,8 @@ static bool test_format_plane(data_t *data, enum pipe pipe,
> >   		test_format_plane_color(data, pipe, plane,
> >   					format, modifier,
> >   					width, height,
> > +					IGT_COLOR_YCBCR_BT709,
> > +					IGT_COLOR_YCBCR_LIMITED_RANGE,
> >   					i, &ref_crc[i], &fb);
> >   	}
> >   
> > @@ -580,10 +689,6 @@ static bool test_format_plane(data_t *data, enum pipe pipe,
> >   	igt_require(num_unique_crcs(ref_crc, ARRAY_SIZE(colors)) > 1);
> >   
> >   	for (int i = 0; i < plane->format_mod_count; i++) {
> > -		int crc_mismatch_count = 0;
> > -		int crc_mismatch_mask = 0;
> > -		igt_crc_t crc;
> > -
> >   		format = plane->formats[i];
> >   		modifier = plane->modifiers[i];
> >   
> > @@ -599,30 +704,19 @@ static bool test_format_plane(data_t *data, enum pipe pipe,
> >   				continue;
> >   		}
> >   
> > -		igt_info("Testing format " IGT_FORMAT_FMT " / modifier 0x%" PRIx64 " on %s.%u\n",
> > -			 IGT_FORMAT_ARGS(format), modifier,
> > -			 kmstest_pipe_name(pipe), plane->index);
> > -
> > -		for (int j = 0; j < ARRAY_SIZE(colors); j++) {
> > -			test_format_plane_color(data, pipe, plane,
> > -						format, modifier,
> > -						width, height,
> > -						j, &crc, &fb);
> > -
> > -			if (!igt_check_crc_equal(&crc, &ref_crc[j])) {
> > -				crc_mismatch_count++;
> > -				crc_mismatch_mask |= (1 << j);
> > -				result = false;
> > -			}
> > -		}
> > +		if (igt_format_is_yuv(format))
> > +			result &= test_format_plane_yuv(data, pipe, plane,
> > +							format, modifier,
> > +							width, height,
> > +							ref_crc, &fb);
> > +		else
> > +			result &= test_format_plane_rgb(data, pipe, plane,
> > +							format, modifier,
> > +							width, height,
> > +							ref_crc, &fb);
> >   
> >   		if (format == DRM_FORMAT_C8)
> >   			set_legacy_lut(data, pipe, 0xfc00);
> > -
> > -		if (crc_mismatch_count)
> > -			igt_warn("CRC mismatches with format " IGT_FORMAT_FMT " on %s.%u with %d/%d solid colors tested (0x%X)\n",
> > -				 IGT_FORMAT_ARGS(format), kmstest_pipe_name(pipe),
> > -				 plane->index, crc_mismatch_count, (int)ARRAY_SIZE(colors), crc_mismatch_mask);
> >   	}
> >   
> >   	igt_pipe_crc_stop(data->pipe_crc);
> > 
> 

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev