Re: [PATCH v4 03/11] drm/vkms: Add KUnit test for YCbCr to RGB conversion matrices

[Pekka Paalanen <pekka.paalanen@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 7547 bytes --]

On Tue, 28 Jul 2026 17:17:34 +0200
Robert Mader <robert.mader@collabora.com> wrote:

> TBH this test feels a bit redundant to me, given that it just repeats 
> the exact values, which again just get memcpy'd. I suggest to drop it.

I agree.

If these reference matrices were produced by completely different means
instead of being checked in as "binaries", e.g. by floating-point
calculations from first principles at test time, they might have some
use.

What would actually be useful is to record the script used to produce
these "binary" numbers in the first place. It's much easier to review
such a script, than try to come up with one's own in order to check if
the "binary" numbers are correct (like I did when reviewing these
patches).

I've been wondering, what is the source code - the preferential format
for modification - the numbers recorded here, or the scripts producing
them.


Thanks,
pq

> On 22.07.26 15:45, Harry Wentland wrote:
> > The existing yuv_u16_to_argb_u16 test performs a full RGB->YCbCr->RGB
> > round trip with a tolerance of 0x1ff. That tolerance is required because
> > fully saturated primaries clamp their chroma at encode time and cannot
> > round-trip exactly, but it also makes the test unable to detect small
> > scaling errors in the conversion matrices (such as using a 2^n instead
> > of a 2^n - 1 full-range maximum for limited range, an error of ~129 at
> > gray - below the tolerance).
> >
> > Add a parameterised test that checks the exact S31.32 fixed-point
> > coefficients and luma offset returned by
> > get_conversion_matrix_to_argb_u16() for each encoding and range against
> > reference values. This directly pins the matrices and catches
> > sub-tolerance regressions that the round-trip test cannot.
> >
> > Assisted-by: Copilot:claude-opus-4.8
> > Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> > ---
> >   drivers/gpu/drm/vkms/tests/vkms_format_test.c | 122 ++++++++++++++++++
> >   1 file changed, 122 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/vkms/tests/vkms_format_test.c b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
> > index 789c59d07ffb..e746b18bf37b 100644
> > --- a/drivers/gpu/drm/vkms/tests/vkms_format_test.c
> > +++ b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
> > @@ -263,8 +263,130 @@ KUNIT_ARRAY_PARAM(yuv_u16_to_argb_u16, yuv_u16_to_argb_u16_cases,
> >   		  vkms_format_test_yuv_u16_to_argb_u16_case_desc
> >   );
> >   
> > +/*
> > + * struct conversion_matrix_case - Reference matrix to test the YUV to RGB
> > + * conversion matrices returned by get_conversion_matrix_to_argb_u16()
> > + *
> > + * @encoding: Encoding of the conversion matrix under test
> > + * @range: Range of the conversion matrix under test
> > + * @expected: Expected S31.32 fixed-point matrix and luma offset
> > + *
> > + * The limited-range coefficients use the studio-range scaling mandated by the
> > + * DRM UAPI and IGT's igt_ycbcr_to_rgb_matrix(): the narrow range is expanded
> > + * relative to a full-range maximum of 2^n - 1 (255 for 8-bit), i.e. luma by
> > + * 255/(235 - 16) and chroma by 255/(240 - 128). Generating them with the
> > + * common 2^n normalisation (as colour.matrix_YCbCr(is_legal=True) does) is off
> > + * by a factor of 256/255. See the DRM_COLOROP_FM_YCBCR*_LIMITED_RGB
> > + * documentation in <drm/drm_colorop.h>.
> > + */
> > +struct conversion_matrix_case {
> > +	enum drm_color_encoding encoding;
> > +	enum drm_color_range range;
> > +	struct conversion_matrix expected;
> > +};
> > +
> > +static struct conversion_matrix_case conversion_matrix_cases[] = {
> > +	{
> > +		.encoding = DRM_COLOR_YCBCR_BT601,
> > +		.range = DRM_COLOR_YCBCR_FULL_RANGE,
> > +		.expected = { .matrix = {
> > +			{ 4294967296, 0,           6021544149 },
> > +			{ 4294967296, -1478054095, -3067191994 },
> > +			{ 4294967296, 7610682049,  0 },
> > +		}, .y_offset = 0 },
> > +	},
> > +	{
> > +		.encoding = DRM_COLOR_YCBCR_BT601,
> > +		.range = DRM_COLOR_YCBCR_LIMITED_RANGE,
> > +		.expected = { .matrix = {
> > +			{ 5000989317, 0,           6854882848 },
> > +			{ 5000989317, -1682606224, -3491669458 },
> > +			{ 5000989317, 8663946082,  0 },
> > +		}, .y_offset = 16 },
> > +	},
> > +	{
> > +		.encoding = DRM_COLOR_YCBCR_BT709,
> > +		.range = DRM_COLOR_YCBCR_FULL_RANGE,
> > +		.expected = { .matrix = {
> > +			{ 4294967296, 0,          6763714498 },
> > +			{ 4294967296, -804551626, -2010578443 },
> > +			{ 4294967296, 7969741314, 0 },
> > +		}, .y_offset = 0 },
> > +	},
> > +	{
> > +		.encoding = DRM_COLOR_YCBCR_BT709,
> > +		.range = DRM_COLOR_YCBCR_LIMITED_RANGE,
> > +		.expected = { .matrix = {
> > +			{ 5000989317, 0,          7699764272 },
> > +			{ 5000989317, -915895824, -2288828138 },
> > +			{ 5000989317, 9072696586, 0 },
> > +		}, .y_offset = 16 },
> > +	},
> > +	{
> > +		.encoding = DRM_COLOR_YCBCR_BT2020,
> > +		.range = DRM_COLOR_YCBCR_FULL_RANGE,
> > +		.expected = { .matrix = {
> > +			{ 4294967296, 0,          6333358775 },
> > +			{ 4294967296, -706750298, -2453942994 },
> > +			{ 4294967296, 8080551471, 0 },
> > +		}, .y_offset = 0 },
> > +	},
> > +	{
> > +		.encoding = DRM_COLOR_YCBCR_BT2020,
> > +		.range = DRM_COLOR_YCBCR_LIMITED_RANGE,
> > +		.expected = { .matrix = {
> > +			{ 5000989317, 0,          7209850391 },
> > +			{ 5000989317, -804559491, -2793551177 },
> > +			{ 5000989317, 9198842076, 0 },
> > +		}, .y_offset = 16 },
> > +	},
> > +};
> > +
> > +/*
> > + * vkms_format_test_conversion_matrix - Verify the YUV to RGB conversion matrices
> > + *
> > + * This test checks that get_conversion_matrix_to_argb_u16() returns the exact
> > + * fixed-point coefficients expected for each encoding and range. Unlike the
> > + * round-trip test above, it is sensitive to small (sub-tolerance) scaling
> > + * errors such as using a 2^n instead of a 2^n - 1 full-range maximum for the
> > + * limited-range matrices.
> > + */
> > +static void vkms_format_test_conversion_matrix(struct kunit *test)
> > +{
> > +	const struct conversion_matrix_case *param = test->param_value;
> > +	struct conversion_matrix matrix;
> > +
> > +	get_conversion_matrix_to_argb_u16(DRM_FORMAT_NV12, param->encoding,
> > +					  param->range, false, &matrix);
> > +
> > +	for (size_t i = 0; i < 3; i++) {
> > +		for (size_t j = 0; j < 3; j++) {
> > +			KUNIT_EXPECT_EQ_MSG(test, matrix.matrix[i][j],
> > +					    param->expected.matrix[i][j],
> > +					    "matrix[%zu][%zu] mismatch for %s - %s",
> > +					    i, j,
> > +					    drm_get_color_encoding_name(param->encoding),
> > +					    drm_get_color_range_name(param->range));
> > +		}
> > +	}
> > +
> > +	KUNIT_EXPECT_EQ(test, matrix.y_offset, param->expected.y_offset);
> > +}
> > +
> > +static void vkms_format_test_conversion_matrix_case_desc(struct conversion_matrix_case *t,
> > +							 char *desc)
> > +{
> > +	snprintf(desc, KUNIT_PARAM_DESC_SIZE, "%s - %s",
> > +		 drm_get_color_encoding_name(t->encoding), drm_get_color_range_name(t->range));
> > +}
> > +
> > +KUNIT_ARRAY_PARAM(conversion_matrix, conversion_matrix_cases,
> > +		  vkms_format_test_conversion_matrix_case_desc
> > +);
> > +
> >   static struct kunit_case vkms_format_test_cases[] = {
> >   	KUNIT_CASE_PARAM(vkms_format_test_yuv_u16_to_argb_u16, yuv_u16_to_argb_u16_gen_params),
> > +	KUNIT_CASE_PARAM(vkms_format_test_conversion_matrix, conversion_matrix_gen_params),
> >   	{}
> >   };
> >     
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]