Re: [PATCH v4 02/11] drm/vkms: Fix limited-range YCbCr to RGB conversion scaling
Harry Wentland <[email protected]> Fri, 31 Jul 2026 11:30:48 -0400
| Newsgroups | org.freedesktop.lists.amd-gfx,org.freedesktop.lists.dri-devel |
|---|---|
| Message-ID | <[email protected]> |
On 2026-07-30 09:46, Pekka Paalanen wrote: > On Wed, 22 Jul 2026 09:45:58 -0400 > Harry Wentland <[email protected]> wrote: > >> The limited-/studio-range YCbCr to RGB conversion matrices were generated >> with colour.matrix_YCbCr(is_legal=True, bits=8), which normalises the >> narrow range by 2^bits (256 for 8-bit) rather than by the full-range >> maximum 2^bits - 1 (255). As a result the luma scale was 256/219 and the >> chroma scale 256/224 instead of the correct 255/219 and 255/224. > > Hi Harry, > > this is an interesting find. For the previous patch "drm/colorop: Add > DRM_COLOROP_FIXED_MATRIX" I verified that the matrices match > colour.matrix_YCbCr(), but this here seems to say all the > limited range cases are wrong? That patch would need the numbers > corrected too. > Interestingly this came out of asking Claude LLM to add documentation to the fixed matrix enums. I asked it to confirm whether the matrix documentation matches what amdgpu and VKMS do. It then pointed out that VKMS scales luma against 256, not 255. So the documented matrices in the "Add DRM_COLOROP_FIXED_MATRIX" patch are correct, but VKMS's matrices didn't match. This patch fixes it up. Before this patch I had to increase VKMS's 8 bpc bracket to [-1, 3] in our IGT tests. Now a [-1, 2] bracket suffices. >> >> This over-scales every limited-range conversion by a factor of 256/255, >> producing an error of up to ~3/255 (8-bit) and causing IGT tests that >> compare software-computed surfaces against VKMS-processed ones to fail. >> >> Recompute the three limited-range matrices from first principles rather >> than relying on colour's is_legal scaling: >> >> 1. Start from the standard ITU-R YCbCr -> RGB relations for the luma >> weights Kr, Kb (Kg = 1 - Kr - Kb) of each encoding (BT.601, BT.709 >> and BT.2020 non-constant luminance). These are the same relations >> that produce the existing full-range matrices. >> 2. Expand the studio input range to full range relative to a full-range >> maximum of 2^n - 1: the luma coefficient by 255/(235 - 16) and the >> chroma coefficients by 255/(240 - 16). This matches the DRM UAPI >> definition and IGT's igt_ycbcr_to_rgb_matrix(). >> 3. Convert each coefficient to S31.32 fixed point (round(coeff * 2^32)). > > I verified the new matrix values assuming that > colour.matrix_YCbCr(K=colour.WEIGHTS_YCBCR["ITU-R BT.709"], > is_legal=False) is correct, and the same with 601 and 2020. > Thanks for verifying. These things are easy to get wrong and I really appreciate the cross-check. >> >> The full-range matrices (is_legal=False) are unaffected as they use exact >> 1.0/2.0 scaling. > > A what scaling now? > I should've done a better job checking the LLM output here. This line is obvious nonsense and will be dropped in v5. I think the LLM ended up confusing similar but unrelated concepts and then came up with this nonsense. >> Update the limited-range reference values in the vkms-format KUnit test >> accordingly: the expected YCbCr inputs now use the studio-range encoding >> consistent with the corrected matrices (8-bit value v placed at v * 257, >> i.e. relative to a full-range maximum of 255). > > H.273 describes how to expand the values to any bit size. It maintains > full precision rather than scales up 8-bit values. The results are > different than recorded in this patch. > The tests are modified based on VKMS's YUV handling, not H.273. VKMS scales any 8-bpc RGB and YUV value up to 16-bit unorm by multiplying it by 257 [1]. I'll submit the python script to generate the kunit test values with v5. [1] https://gitlab.freedesktop.org/drm/misc/kernel/-/blob/drm-misc-next/drivers/gpu/drm/vkms/vkms_formats.c?ref_type=heads#L280 Harry > > Thanks, > pq > >> Fixes: fe22d21e9342 ("drm/vkms: Add YUV support") >> Assisted-by: Copilot:claude-opus-4.8 >> Signed-off-by: Harry Wentland <[email protected]> >> --- >> drivers/gpu/drm/vkms/tests/vkms_format_test.c | 36 ++++++------ >> drivers/gpu/drm/vkms/vkms_formats.c | 55 +++++++++++++------ >> 2 files changed, 55 insertions(+), 36 deletions(-) >> >> diff --git a/drivers/gpu/drm/vkms/tests/vkms_format_test.c b/drivers/gpu/drm/vkms/tests/vkms_format_test.c >> index a7788fbc45dc..789c59d07ffb 100644 >> --- a/drivers/gpu/drm/vkms/tests/vkms_format_test.c >> +++ b/drivers/gpu/drm/vkms/tests/vkms_format_test.c >> @@ -101,12 +101,12 @@ static struct yuv_u16_to_argb_u16_case yuv_u16_to_argb_u16_cases[] = { >> .range = DRM_COLOR_YCBCR_LIMITED_RANGE, >> .n_colors = 6, >> .colors = { >> - { "white", { 0xeb00, 0x8000, 0x8000 }, { 0xffff, 0xffff, 0xffff, 0xffff }}, >> - { "gray", { 0x7dee, 0x8000, 0x8000 }, { 0xffff, 0x8080, 0x8080, 0x8080 }}, >> - { "black", { 0x1000, 0x8000, 0x8000 }, { 0xffff, 0x0000, 0x0000, 0x0000 }}, >> - { "red", { 0x517b, 0x5a34, 0xf000 }, { 0xffff, 0xffff, 0x0000, 0x0000 }}, >> - { "green", { 0x908e, 0x35cc, 0x2237 }, { 0xffff, 0x0000, 0xffff, 0x0000 }}, >> - { "blue", { 0x28f7, 0xf000, 0x6dc9 }, { 0xffff, 0x0000, 0x0000, 0xffff }}, >> + { "white", { 0xebeb, 0x8080, 0x8080 }, { 0xffff, 0xffff, 0xffff, 0xffff }}, >> + { "gray", { 0x7e6c, 0x8080, 0x8080 }, { 0xffff, 0x8080, 0x8080, 0x8080 }}, >> + { "black", { 0x1010, 0x8080, 0x8080 }, { 0xffff, 0x0000, 0x0000, 0x0000 }}, >> + { "red", { 0x51cd, 0x5a8e, 0xf0f0 }, { 0xffff, 0xffff, 0x0000, 0x0000 }}, >> + { "green", { 0x911e, 0x3602, 0x2259 }, { 0xffff, 0x0000, 0xffff, 0x0000 }}, >> + { "blue", { 0x2920, 0xf0f0, 0x6e37 }, { 0xffff, 0x0000, 0x0000, 0xffff }}, >> } >> }, >> /* >> @@ -151,12 +151,12 @@ static struct yuv_u16_to_argb_u16_case yuv_u16_to_argb_u16_cases[] = { >> .range = DRM_COLOR_YCBCR_LIMITED_RANGE, >> .n_colors = 6, >> .colors = { >> - { "white", { 0xeb00, 0x8000, 0x8000 }, { 0xffff, 0xffff, 0xffff, 0xffff }}, >> - { "gray", { 0x7dee, 0x8000, 0x8000 }, { 0xffff, 0x8080, 0x8080, 0x8080 }}, >> - { "black", { 0x1000, 0x8000, 0x8000 }, { 0xffff, 0x0000, 0x0000, 0x0000 }}, >> - { "red", { 0x3e8f, 0x6656, 0xf000 }, { 0xffff, 0xffff, 0x0000, 0x0000 }}, >> - { "green", { 0xaca1, 0x29aa, 0x1a45 }, { 0xffff, 0x0000, 0xffff, 0x0000 }}, >> - { "blue", { 0x1fd0, 0xf000, 0x75bb }, { 0xffff, 0x0000, 0x0000, 0xffff }}, >> + { "white", { 0xebeb, 0x8080, 0x8080 }, { 0xffff, 0xffff, 0xffff, 0xffff }}, >> + { "gray", { 0x7e6c, 0x8080, 0x8080 }, { 0xffff, 0x8080, 0x8080, 0x8080 }}, >> + { "black", { 0x1010, 0x8080, 0x8080 }, { 0xffff, 0x0000, 0x0000, 0x0000 }}, >> + { "red", { 0x3ece, 0x66bc, 0xf0f0 }, { 0xffff, 0xffff, 0x0000, 0x0000 }}, >> + { "green", { 0xad4e, 0x29d4, 0x1a5f }, { 0xffff, 0x0000, 0xffff, 0x0000 }}, >> + { "blue", { 0x1ff0, 0xf0f0, 0x7631 }, { 0xffff, 0x0000, 0x0000, 0xffff }}, >> } >> }, >> /* >> @@ -201,12 +201,12 @@ static struct yuv_u16_to_argb_u16_case yuv_u16_to_argb_u16_cases[] = { >> .range = DRM_COLOR_YCBCR_LIMITED_RANGE, >> .n_colors = 6, >> .colors = { >> - { "white", { 0xeb00, 0x8000, 0x8000 }, { 0xffff, 0xffff, 0xffff, 0xffff }}, >> - { "gray", { 0x7dee, 0x8000, 0x8000 }, { 0xffff, 0x8080, 0x8080, 0x8080 }}, >> - { "black", { 0x1000, 0x8000, 0x8000 }, { 0xffff, 0x0000, 0x0000, 0x0000 }}, >> - { "red", { 0x4988, 0x60b9, 0xf000 }, { 0xffff, 0xffff, 0x0000, 0x0000 }}, >> - { "green", { 0xa47b, 0x2f47, 0x1902 }, { 0xffff, 0x0000, 0xffff, 0x0000 }}, >> - { "blue", { 0x1cfd, 0xf000, 0x76fe }, { 0xffff, 0x0000, 0x0000, 0xffff }}, >> + { "white", { 0xebeb, 0x8080, 0x8080 }, { 0xffff, 0xffff, 0xffff, 0xffff }}, >> + { "gray", { 0x7e6c, 0x8080, 0x8080 }, { 0xffff, 0x8080, 0x8080, 0x8080 }}, >> + { "black", { 0x1010, 0x8080, 0x8080 }, { 0xffff, 0x0000, 0x0000, 0x0000 }}, >> + { "red", { 0x49d2, 0x611a, 0xf0f0 }, { 0xffff, 0xffff, 0x0000, 0x0000 }}, >> + { "green", { 0xa520, 0x2f76, 0x191b }, { 0xffff, 0x0000, 0xffff, 0x0000 }}, >> + { "blue", { 0x1d1a, 0xf0f0, 0x7775 }, { 0xffff, 0x0000, 0x0000, 0xffff }}, >> } >> }, >> }; >> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c >> index dfb8e13cba87..4d5fcaeb82c5 100644 >> --- a/drivers/gpu/drm/vkms/vkms_formats.c >> +++ b/drivers/gpu/drm/vkms/vkms_formats.c >> @@ -788,15 +788,36 @@ static const struct conversion_matrix yuv_bt601_full = { >> }; >> >> /* >> - * numpy.around(colour.matrix_YCbCr(K=colour.WEIGHTS_YCBCR["ITU-R BT.601"], >> - * is_legal = True, >> - * bits = 8) * 2**32).astype(int) >> + * BT.601 limited-/studio-range YCbCr to full-range RGB. >> + * >> + * The coefficients are derived as follows: >> + * >> + * 1. Take the standard ITU-R YCbCr -> RGB relations for luma weights >> + * Kr, Kb (Kg = 1 - Kr - Kb), with Y in [0, 1] and Cb, Cr in >> + * [-0.5, 0.5]. For BT.601 Kr = 0.299 and Kb = 0.114: >> + * >> + * R = Y + 2 * (1 - Kr) * Cr >> + * G = Y - 2 * (1 - Kb) * Kb / Kg * Cb - 2 * (1 - Kr) * Kr / Kg * Cr >> + * B = Y + 2 * (1 - Kb) * Cb >> + * >> + * These are exactly the yuv_bt601_full coefficients above. >> + * >> + * 2. Expand the studio input range to full range, relative to a >> + * full-range maximum of 2^n - 1 (255 for 8-bit): the luma >> + * coefficient is scaled by 255/(235 - 16) and the chroma >> + * coefficients by 255/(240 - 16). This matches the DRM UAPI >> + * definition and IGT's igt_ycbcr_to_rgb_matrix(). Note this differs >> + * from colour.matrix_YCbCr(is_legal=True), which normalises by 2^n >> + * and is thus off by a factor of 256/255. >> + * >> + * 3. Convert each coefficient to S31.32 fixed point, i.e. >> + * round(coeff * 2^32). >> */ >> static const struct conversion_matrix yuv_bt601_limited = { >> .matrix = { >> - { 5020601039, 0, 6881764740 }, >> - { 5020601039, -1689204679, -3505362278 }, >> - { 5020601039, 8697922339, 0 }, >> + { 5000989317, 0, 6854882848 }, >> + { 5000989317, -1682606224, -3491669458 }, >> + { 5000989317, 8663946082, 0 }, >> }, >> .y_offset = 16, >> }; >> @@ -816,15 +837,14 @@ static const struct conversion_matrix yuv_bt709_full = { >> }; >> >> /* >> - * numpy.around(colour.matrix_YCbCr(K=colour.WEIGHTS_YCBCR["ITU-R BT.709"], >> - * is_legal = True, >> - * bits = 8) * 2**32).astype(int) >> + * BT.709 limited-range YCbCr to full-range RGB (Kr = 0.2126, Kb = 0.0722). >> + * Derived as described for yuv_bt601_limited. >> */ >> static const struct conversion_matrix yuv_bt709_limited = { >> .matrix = { >> - { 5020601039, 0, 7729959424 }, >> - { 5020601039, -919487572, -2297803934 }, >> - { 5020601039, 9108275786, 0 }, >> + { 5000989317, 0, 7699764272 }, >> + { 5000989317, -915895824, -2288828138 }, >> + { 5000989317, 9072696586, 0 }, >> }, >> .y_offset = 16, >> }; >> @@ -844,15 +864,14 @@ static const struct conversion_matrix yuv_bt2020_full = { >> }; >> >> /* >> - * numpy.around(colour.matrix_YCbCr(K=colour.WEIGHTS_YCBCR["ITU-R BT.2020"], >> - * is_legal = True, >> - * bits = 8) * 2**32).astype(int) >> + * BT.2020 non-constant-luminance limited-range YCbCr to full-range RGB >> + * (Kr = 0.2627, Kb = 0.0593). Derived as described for yuv_bt601_limited. >> */ >> static const struct conversion_matrix yuv_bt2020_limited = { >> .matrix = { >> - { 5020601039, 0, 7238124312 }, >> - { 5020601039, -807714626, -2804506279 }, >> - { 5020601039, 9234915964, 0 }, >> + { 5000989317, 0, 7209850391 }, >> + { 5000989317, -804559491, -2793551177 }, >> + { 5000989317, 9198842076, 0 }, >> }, >> .y_offset = 16, >> }; >