* [PATCH v4 01/11] drm/colorop: Add DRM_COLOROP_FIXED_MATRIX
2026-07-22 13:45 [PATCH v4 00/11] YUV conversion colorop with amdgpu and VKMS Harry Wentland
@ 2026-07-22 13:45 ` Harry Wentland
2026-07-22 13:55 ` sashiko-bot
2026-07-22 13:45 ` [PATCH v4 02/11] drm/vkms: Fix limited-range YCbCr to RGB conversion scaling Harry Wentland
` (9 subsequent siblings)
10 siblings, 1 reply; 19+ messages in thread
From: Harry Wentland @ 2026-07-22 13:45 UTC (permalink / raw)
To: dri-devel, amd-gfx
Cc: Chaitanya Kumar Borah, Harry Wentland, Melissa Wen, Robert Mader
From: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
Introduce DRM_COLOROP_FIXED_MATRIX, a new colorop type representing a
hardware that performs a fixed matrix operation.
Unlike CTM-based colorops, this block does not expose programmable
coefficients. Instead, userspace selects one of the predefined
hardware modes via a new FIXED_MATRIX_TYPE enum property. Supported modes
include common YCbCr->RGB and RGB709->RGB2020 conversions.
v4:
- Add limited-range YCbCr-RGB conversion matrix enums (Robert)
- Document matrix values (Pekka)
- Clarify RGB709 to RGB2020 is a full-range matrix (Pekka)
- Fix confusing "CSC preset" doc (Pekka)
v2:
- Naming changes (Pekka)
v3:
- Fix NC matrix enum name and string (Melissa)
- Rebase
Co-developed-by: Harry Wentland <harry.wentland@amd.com>
Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Reviewed-by: Melissa Wen <mwen@igalia.com>
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
Reviewed-by: Robert Mader <robert.mader@collabora.com>
---
drivers/gpu/drm/drm_atomic.c | 4 +
drivers/gpu/drm/drm_atomic_uapi.c | 7 ++
drivers/gpu/drm/drm_colorop.c | 109 ++++++++++++++++++++
include/drm/drm_colorop.h | 159 ++++++++++++++++++++++++++++++
include/uapi/drm/drm_mode.h | 12 +++
5 files changed, 291 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index e5c8ef06caed..45df6376f408 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -925,6 +925,10 @@ static void drm_atomic_colorop_print_state(struct drm_printer *p,
drm_get_colorop_lut3d_interpolation_name(state->lut3d_interpolation));
drm_printf_indent(p, 1, "data blob id=%d\n", state->data ? state->data->base.id : 0);
break;
+ case DRM_COLOROP_FIXED_MATRIX:
+ drm_printf_indent(p, 1, "fixed_matrix_type=%s\n",
+ drm_get_colorop_fixed_matrix_type_name(state->fixed_matrix_type));
+ break;
default:
break;
}
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 1050dddadb17..d52504d823bc 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -786,6 +786,11 @@ static int drm_atomic_colorop_set_property(struct drm_colorop *colorop,
return drm_atomic_color_set_data_property(colorop, state,
property, val,
replaced);
+ } else if (property == colorop->fixed_matrix_type_property) {
+ if (state->fixed_matrix_type != val) {
+ state->fixed_matrix_type = val;
+ *replaced = true;
+ }
} else {
drm_dbg_atomic(colorop->dev,
"[COLOROP:%d:%d] unknown property [PROP:%d:%s]\n",
@@ -818,6 +823,8 @@ drm_atomic_colorop_get_property(struct drm_colorop *colorop,
*val = state->lut3d_interpolation;
else if (property == colorop->data_property)
*val = (state->data) ? state->data->base.id : 0;
+ else if (property == colorop->fixed_matrix_type_property)
+ *val = state->fixed_matrix_type;
else
return -EINVAL;
diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
index 682fcc651525..e7ccf7d053b8 100644
--- a/drivers/gpu/drm/drm_colorop.c
+++ b/drivers/gpu/drm/drm_colorop.c
@@ -68,6 +68,7 @@ static const struct drm_prop_enum_list drm_colorop_type_enum_list[] = {
{ DRM_COLOROP_CTM_3X4, "3x4 Matrix"},
{ DRM_COLOROP_MULTIPLIER, "Multiplier"},
{ DRM_COLOROP_3D_LUT, "3D LUT"},
+ { DRM_COLOROP_FIXED_MATRIX, "Fixed Matrix"},
};
static const char * const colorop_curve_1d_type_names[] = {
@@ -90,6 +91,17 @@ static const struct drm_prop_enum_list drm_colorop_lut3d_interpolation_list[] =
{ DRM_COLOROP_LUT3D_INTERPOLATION_TETRAHEDRAL, "Tetrahedral" },
};
+static const char * const colorop_fixed_matrix_type_names[] = {
+ [DRM_COLOROP_FM_YCBCR601_FULL_RGB] = "YCbCr 601 Full to RGB",
+ [DRM_COLOROP_FM_YCBCR601_LIMITED_RGB] = "YCbCr 601 Limited to RGB",
+ [DRM_COLOROP_FM_YCBCR709_FULL_RGB] = "YCbCr 709 Full to RGB",
+ [DRM_COLOROP_FM_YCBCR709_LIMITED_RGB] = "YCbCr 709 Limited to RGB",
+ [DRM_COLOROP_FM_YCBCR2020_NC_FULL_RGB] = "YCbCr 2020 NC Full to RGB",
+ [DRM_COLOROP_FM_YCBCR2020_NC_LIMITED_RGB] = "YCbCr 2020 NC Limited to RGB",
+ [DRM_COLOROP_FM_YCBCR_LIMITED_FULL] = "YCbCr limited to full",
+ [DRM_COLOROP_FM_RGB709_RGB2020] = "RGB709 to RGB2020",
+};
+
/* Init Helpers */
static int drm_plane_colorop_init(struct drm_device *dev, struct drm_colorop *colorop,
@@ -453,6 +465,80 @@ int drm_plane_colorop_3dlut_init(struct drm_device *dev, struct drm_colorop *col
}
EXPORT_SYMBOL(drm_plane_colorop_3dlut_init);
+/**
+ * drm_plane_colorop_fixed_matrix_init - Initialize a DRM_COLOROP_FIXED_MATRIX
+ *
+ * @dev: DRM device
+ * @colorop: The drm_colorop object to initialize
+ * @plane: The associated drm_plane
+ * @funcs: control functions for the new colorop
+ * @supported_fm: A bitfield of supported drm_colorop_fixed_matrix_type enum values,
+ * created using BIT(fixed_matrix_type) and combined with the OR '|'
+ * operator.
+ * @flags: bitmask of misc, see DRM_COLOROP_FLAG_* defines.
+ * @return zero on success, -E value on failure
+ */
+int drm_plane_colorop_fixed_matrix_init(struct drm_device *dev, struct drm_colorop *colorop,
+ struct drm_plane *plane,
+ const struct drm_colorop_funcs *funcs,
+ u64 supported_fm, uint32_t flags)
+{
+ struct drm_prop_enum_list enum_list[DRM_COLOROP_FM_COUNT];
+ int i, len;
+ struct drm_property *prop;
+ int ret;
+
+ if (!supported_fm) {
+ drm_err(dev,
+ "No supported FM type op for new Fixed Matrix colorop on [PLANE:%d:%s]\n",
+ plane->base.id, plane->name);
+ return -EINVAL;
+ }
+
+ if ((supported_fm & -BIT(DRM_COLOROP_FM_COUNT)) != 0) {
+ drm_err(dev, "Unknown Fixed Matrix provided on [PLANE:%d:%s]\n",
+ plane->base.id, plane->name);
+ return -EINVAL;
+ }
+
+ ret = drm_plane_colorop_init(dev, colorop, plane, funcs, DRM_COLOROP_FIXED_MATRIX, flags);
+ if (ret)
+ return ret;
+
+ len = 0;
+ for (i = 0; i < DRM_COLOROP_FM_COUNT; i++) {
+ if ((supported_fm & BIT(i)) == 0)
+ continue;
+
+ enum_list[len].type = i;
+ enum_list[len].name = colorop_fixed_matrix_type_names[i];
+ len++;
+ }
+
+ if (WARN_ON(len <= 0))
+ return -EINVAL;
+
+ prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC, "FIXED_MATRIX_TYPE",
+ enum_list, len);
+
+ if (!prop)
+ return -ENOMEM;
+
+ colorop->fixed_matrix_type_property = prop;
+ /*
+ * Default to the first supported CSC mode as provided by the driver.
+ * Intuitively this should be something that keeps the colorop in pixel bypass
+ * mode but that is already handled via the standard colorop bypass
+ * property.
+ */
+ drm_object_attach_property(&colorop->base, colorop->fixed_matrix_type_property,
+ enum_list[0].type);
+ drm_colorop_reset(colorop);
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_plane_colorop_fixed_matrix_init);
+
static void __drm_atomic_helper_colorop_duplicate_state(struct drm_colorop *colorop,
struct drm_colorop_state *state)
{
@@ -533,6 +619,13 @@ static void __drm_colorop_state_init(struct drm_colorop_state *colorop_state,
&val))
colorop_state->lut3d_interpolation = val;
}
+
+ if (colorop->fixed_matrix_type_property) {
+ if (!drm_object_property_get_default_value(&colorop->base,
+ colorop->fixed_matrix_type_property,
+ &val))
+ colorop_state->fixed_matrix_type = val;
+ }
}
/**
@@ -596,6 +689,7 @@ static const char * const colorop_type_name[] = {
[DRM_COLOROP_CTM_3X4] = "3x4 Matrix",
[DRM_COLOROP_MULTIPLIER] = "Multiplier",
[DRM_COLOROP_3D_LUT] = "3D LUT",
+ [DRM_COLOROP_FIXED_MATRIX] = "Fixed Matrix",
};
static const char * const colorop_lu3d_interpolation_name[] = {
@@ -652,6 +746,21 @@ const char *drm_get_colorop_lut3d_interpolation_name(enum drm_colorop_lut3d_inte
return colorop_lu3d_interpolation_name[type];
}
+/**
+ * drm_get_colorop_fixed_matrix_type_name: return a string for fixed matrix type
+ * @type: fixed matrix type to compute name of
+ *
+ * In contrast to the other drm_get_*_name functions this one here returns a
+ * const pointer and hence is threadsafe.
+ */
+const char *drm_get_colorop_fixed_matrix_type_name(enum drm_colorop_fixed_matrix_type type)
+{
+ if (WARN_ON(type >= ARRAY_SIZE(colorop_fixed_matrix_type_names)))
+ return "unknown";
+
+ return colorop_fixed_matrix_type_names[type];
+}
+
/**
* drm_colorop_set_next_property - sets the next pointer
* @colorop: drm colorop
diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
index 224fae40ed2b..92eb6463c39f 100644
--- a/include/drm/drm_colorop.h
+++ b/include/drm/drm_colorop.h
@@ -134,6 +134,146 @@ enum drm_colorop_curve_1d_type {
DRM_COLOROP_1D_CURVE_COUNT
};
+/**
+ * enum drm_colorop_fixed_matrix_type - type of Fixed Matrix
+ *
+ * Describes a Fixed Matrix operation to be applied by the DRM_COLOROP_FIXED_MATRIX
+ */
+enum drm_colorop_fixed_matrix_type {
+ /**
+ * @DRM_COLOROP_FM_YCBCR601_FULL_RGB:
+ *
+ * enum string "YCbCr 601 Full to RGB"
+ *
+ * Converts full-range YCbCr into full-range RGB using the BT.601
+ * coefficients. Y is normalized to [0, 1] and Cb, Cr are centered
+ * at 0 (the nominal 0.5 offset removed) before the matrix::
+ *
+ * | R | | 1.0 0.000000 1.402000 | | Y |
+ * | G | = | 1.0 -0.344136 -0.714136 | x | Cb |
+ * | B | | 1.0 1.772000 0.000000 | | Cr |
+ */
+ DRM_COLOROP_FM_YCBCR601_FULL_RGB,
+
+ /**
+ * @DRM_COLOROP_FM_YCBCR601_LIMITED_RGB:
+ *
+ * enum string "YCbCr 601 Limited to RGB"
+ *
+ * Converts limited- (narrow-) range YCbCr into full-range RGB using
+ * the BT.601 coefficients. Before the matrix Y is offset by 16/255
+ * and Cb, Cr are centered at 0, following the studio-range
+ * convention of ITU-R BT.601::
+ *
+ * | R | | 1.164384 0.000000 1.596027 | | Y - 16/255 |
+ * | G | = | 1.164384 -0.391762 -0.812968 | x | Cb |
+ * | B | | 1.164384 2.017232 0.000000 | | Cr |
+ */
+ DRM_COLOROP_FM_YCBCR601_LIMITED_RGB,
+
+ /**
+ * @DRM_COLOROP_FM_YCBCR709_FULL_RGB:
+ *
+ * enum string "YCbCr 709 Full to RGB"
+ *
+ * Converts full-range YCbCr into full-range RGB using the BT.709
+ * coefficients. Y is normalized to [0, 1] and Cb, Cr are centered
+ * at 0 (the nominal 0.5 offset removed) before the matrix::
+ *
+ * | R | | 1.0 0.000000 1.574800 | | Y |
+ * | G | = | 1.0 -0.187324 -0.468124 | x | Cb |
+ * | B | | 1.0 1.855600 0.000000 | | Cr |
+ */
+ DRM_COLOROP_FM_YCBCR709_FULL_RGB,
+
+ /**
+ * @DRM_COLOROP_FM_YCBCR709_LIMITED_RGB:
+ *
+ * enum string "YCbCr 709 Limited to RGB"
+ *
+ * Converts limited- (narrow-) range YCbCr into full-range RGB using
+ * the BT.709 coefficients. Before the matrix Y is offset by 16/255
+ * and Cb, Cr are centered at 0, following the studio-range
+ * convention of ITU-R BT.709::
+ *
+ * | R | | 1.164384 0.000000 1.792741 | | Y - 16/255 |
+ * | G | = | 1.164384 -0.213249 -0.532909 | x | Cb |
+ * | B | | 1.164384 2.112402 0.000000 | | Cr |
+ */
+ DRM_COLOROP_FM_YCBCR709_LIMITED_RGB,
+
+ /**
+ * @DRM_COLOROP_FM_YCBCR2020_NC_FULL_RGB:
+ *
+ * enum string "YCbCr 2020 NC Full to RGB"
+ *
+ * Converts full-range YCbCr into full-range RGB using the BT.2020
+ * non-constant luminance coefficients. Y is normalized to [0, 1]
+ * and Cb, Cr are centered at 0 (the nominal 0.5 offset removed)
+ * before the matrix::
+ *
+ * | R | | 1.0 0.000000 1.474600 | | Y |
+ * | G | = | 1.0 -0.164553 -0.571353 | x | Cb |
+ * | B | | 1.0 1.881400 0.000000 | | Cr |
+ */
+ DRM_COLOROP_FM_YCBCR2020_NC_FULL_RGB,
+
+ /**
+ * @DRM_COLOROP_FM_YCBCR2020_NC_LIMITED_RGB:
+ *
+ * enum string "YCbCr 2020 NC Limited to RGB"
+ *
+ * Converts limited- (narrow-) range YCbCr into full-range RGB using
+ * the BT.2020 non-constant luminance coefficients. Before the matrix
+ * Y is offset by 16/255 and Cb, Cr are centered at 0, following the
+ * studio-range convention of ITU-R BT.2020::
+ *
+ * | R | | 1.164384 0.000000 1.678674 | | Y - 16/255 |
+ * | G | = | 1.164384 -0.187326 -0.650424 | x | Cb |
+ * | B | | 1.164384 2.141772 0.000000 | | Cr |
+ */
+ DRM_COLOROP_FM_YCBCR2020_NC_LIMITED_RGB,
+
+ /**
+ * @DRM_COLOROP_FM_YCBCR_LIMITED_FULL:
+ *
+ * enum string "YCbCr limited to full"
+ *
+ * Converts limited- (narrow-) range YCbCr into full-range YCbCr.
+ * Though not strictly a matrix operation it can be represented as
+ * one. Luma is expanded by 255/219 and chroma by 255/224, keeping
+ * the black level (16/255) and chroma neutral point (0.5) fixed::
+ *
+ * | Y' | | 1.164384 0.000000 0.000000 | | Y - 16/255 |
+ * | Cb'-0.5 | = | 0.000000 1.138393 0.000000 | x | Cb - 0.5 |
+ * | Cr'-0.5 | | 0.000000 0.000000 1.138393 | | Cr - 0.5 |
+ */
+ DRM_COLOROP_FM_YCBCR_LIMITED_FULL,
+
+ /**
+ * @DRM_COLOROP_FM_RGB709_RGB2020:
+ *
+ * enum string "RGB709 to RGB2020"
+ *
+ * Selects the matrix that converts full-range RGB with BT.709
+ * primaries to full-range RGB with BT.2020 primaries. The
+ * coefficients match the RGB-to-RGB conversion defined in
+ * ITU-R BT.2087::
+ *
+ * | R2020 | | 0.6274 0.3293 0.0433 | | R709 |
+ * | G2020 | = | 0.0691 0.9195 0.0114 | x | G709 |
+ * | B2020 | | 0.0164 0.0880 0.8956 | | B709 |
+ */
+ DRM_COLOROP_FM_RGB709_RGB2020,
+
+ /**
+ * @DRM_COLOROP_FM_COUNT:
+ *
+ * enum value denoting the size of the enum
+ */
+ DRM_COLOROP_FM_COUNT
+};
+
/**
* struct drm_colorop_state - mutable colorop state
*/
@@ -197,6 +337,13 @@ struct drm_colorop_state {
*/
enum drm_colorop_lut3d_interpolation_type lut3d_interpolation;
+ /**
+ * @fixed_matrix_type:
+ *
+ * Type of Fixed Matrix operation.
+ */
+ enum drm_colorop_fixed_matrix_type fixed_matrix_type;
+
/** @state: backpointer to global drm_atomic_commit */
struct drm_atomic_commit *state;
};
@@ -366,6 +513,13 @@ struct drm_colorop {
*/
struct drm_property *data_property;
+ /**
+ * @fixed_matrix_type_property:
+ *
+ * Sub-type for DRM_COLOROP_FIXED_MATRIX type.
+ */
+ struct drm_property *fixed_matrix_type_property;
+
/**
* @next_property:
*
@@ -422,6 +576,10 @@ int drm_plane_colorop_3dlut_init(struct drm_device *dev, struct drm_colorop *col
uint32_t lut_size,
enum drm_colorop_lut3d_interpolation_type interpolation,
uint32_t flags);
+int drm_plane_colorop_fixed_matrix_init(struct drm_device *dev, struct drm_colorop *colorop,
+ struct drm_plane *plane,
+ const struct drm_colorop_funcs *funcs,
+ u64 supported_fm, uint32_t flags);
struct drm_colorop_state *
drm_atomic_helper_colorop_create_state(struct drm_colorop *colorop);
@@ -480,6 +638,7 @@ drm_get_colorop_lut1d_interpolation_name(enum drm_colorop_lut1d_interpolation_ty
const char *
drm_get_colorop_lut3d_interpolation_name(enum drm_colorop_lut3d_interpolation_type type);
+const char *drm_get_colorop_fixed_matrix_type_name(enum drm_colorop_fixed_matrix_type type);
void drm_colorop_set_next_property(struct drm_colorop *colorop, struct drm_colorop *next);
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index bd435effdcee..ed1a660a3dfd 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -955,6 +955,18 @@ enum drm_colorop_type {
* color = lut3d[index]
*/
DRM_COLOROP_3D_LUT,
+
+ /**
+ * @DRM_COLOROP_FIXED_MATRIX:
+ *
+ * enum string "Fixed Matrix"
+ *
+ * A Colorop block that performs a pre-defined matrix operation selected
+ * via the FIXED_MATRIX_TYPE enum property. The driver advertises the supported
+ * operations through this property.
+ */
+ DRM_COLOROP_FIXED_MATRIX,
+
};
/**
--
2.55.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4 01/11] drm/colorop: Add DRM_COLOROP_FIXED_MATRIX
2026-07-22 13:45 ` [PATCH v4 01/11] drm/colorop: Add DRM_COLOROP_FIXED_MATRIX Harry Wentland
@ 2026-07-22 13:55 ` sashiko-bot
0 siblings, 0 replies; 19+ messages in thread
From: sashiko-bot @ 2026-07-22 13:55 UTC (permalink / raw)
To: Harry Wentland; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Validation check for a 64-bit bitmask on 32-bit platforms incorrectly uses the 32-bit `BIT()` macro, causing upper bits to be silently ignored.
--
--- Patch [1]: [PATCH v4 01/11] drm/colorop: Add DRM_COLOROP_FIXED_MATRIX ---
commit e2a1cc91ee8bca2062d87fc6c62ee5b47046aaf8
Author: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
Subject: drm/colorop: Add DRM_COLOROP_FIXED_MATRIX
This commit introduces DRM_COLOROP_FIXED_MATRIX, a new colorop type
representing hardware that performs a fixed matrix operation. It adds a new
FIXED_MATRIX_TYPE enum property to select predefined hardware modes instead
of using programmable coefficients.
> diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
> index 682fcc6515259..e7ccf7d053b89 100644
> --- a/drivers/gpu/drm/drm_colorop.c
> +++ b/drivers/gpu/drm/drm_colorop.c
[ ... ]
> +int drm_plane_colorop_fixed_matrix_init(struct drm_device *dev, struct drm_colorop *colorop,
> + struct drm_plane *plane,
> + const struct drm_colorop_funcs *funcs,
> + u64 supported_fm, uint32_t flags)
> +{
[ ... ]
> + if (!supported_fm) {
> + drm_err(dev,
> + "No supported FM type op for new Fixed Matrix colorop on [PLANE:%d:%s]\n",
> + plane->base.id, plane->name);
> + return -EINVAL;
> + }
> +
> + if ((supported_fm & -BIT(DRM_COLOROP_FM_COUNT)) != 0) {
[Severity: Medium]
Since supported_fm is a u64, will this check properly mask the upper 32 bits on
32-bit platforms where BIT() evaluates to a 32-bit unsigned long?
When -BIT(DRM_COLOROP_FM_COUNT) is promoted to 64 bits for the bitwise AND, it
will be zero-extended, causing the upper 32 bits of the mask to be zero. This
could allow invalid upper bits in supported_fm to pass validation silently.
Should this use BIT_ULL() instead?
> + drm_err(dev, "Unknown Fixed Matrix provided on [PLANE:%d:%s]\n",
> + plane->base.id, plane->name);
> + return -EINVAL;
> + }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260722134607.136293-1-harry.wentland@amd.com?part=1
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4 02/11] drm/vkms: Fix limited-range YCbCr to RGB conversion scaling
2026-07-22 13:45 [PATCH v4 00/11] YUV conversion colorop with amdgpu and VKMS Harry Wentland
2026-07-22 13:45 ` [PATCH v4 01/11] drm/colorop: Add DRM_COLOROP_FIXED_MATRIX Harry Wentland
@ 2026-07-22 13:45 ` Harry Wentland
2026-07-22 13:45 ` [PATCH v4 03/11] drm/vkms: Add KUnit test for YCbCr to RGB conversion matrices Harry Wentland
` (8 subsequent siblings)
10 siblings, 0 replies; 19+ messages in thread
From: Harry Wentland @ 2026-07-22 13:45 UTC (permalink / raw)
To: dri-devel, amd-gfx; +Cc: Harry Wentland
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.
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)).
The full-range matrices (is_legal=False) are unaffected as they use exact
1.0/2.0 scaling.
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).
Fixes: fe22d21e9342 ("drm/vkms: Add YUV support")
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 | 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,
};
--
2.55.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v4 03/11] drm/vkms: Add KUnit test for YCbCr to RGB conversion matrices
2026-07-22 13:45 [PATCH v4 00/11] YUV conversion colorop with amdgpu and VKMS Harry Wentland
2026-07-22 13:45 ` [PATCH v4 01/11] drm/colorop: Add DRM_COLOROP_FIXED_MATRIX Harry Wentland
2026-07-22 13:45 ` [PATCH v4 02/11] drm/vkms: Fix limited-range YCbCr to RGB conversion scaling Harry Wentland
@ 2026-07-22 13:45 ` Harry Wentland
2026-07-22 14:01 ` sashiko-bot
2026-07-22 13:46 ` [PATCH v4 04/11] drm/vkms: Add fixed matrix colorop to color pipeline Harry Wentland
` (7 subsequent siblings)
10 siblings, 1 reply; 19+ messages in thread
From: Harry Wentland @ 2026-07-22 13:45 UTC (permalink / raw)
To: dri-devel, amd-gfx; +Cc: Harry Wentland
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),
{}
};
--
2.55.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4 03/11] drm/vkms: Add KUnit test for YCbCr to RGB conversion matrices
2026-07-22 13:45 ` [PATCH v4 03/11] drm/vkms: Add KUnit test for YCbCr to RGB conversion matrices Harry Wentland
@ 2026-07-22 14:01 ` sashiko-bot
0 siblings, 0 replies; 19+ messages in thread
From: sashiko-bot @ 2026-07-22 14:01 UTC (permalink / raw)
To: Harry Wentland; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Compilation error due to function signature mismatch in KUnit test
--
--- Patch [3]: [PATCH v4 03/11] drm/vkms: Add KUnit test for YCbCr to RGB conversion matrices ---
commit dd6b42b44f3cdb2524b236663ae684d8ead8a59a
Author: Harry Wentland <harry.wentland@amd.com>
drm/vkms: Add KUnit test for YCbCr to RGB conversion matrices
The existing yuv_u16_to_argb_u16 test performs a full RGB->YCbCr->RGB round
trip with a tolerance of 0x1ff. This patch adds a parameterised test that
checks the exact S31.32 fixed-point coefficients and luma offset returned by
get_conversion_matrix_to_argb_u16() against reference values.
> diff --git a/drivers/gpu/drm/vkms/tests/vkms_format_test.c b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
> index 789c59d07ffb6..e746b18bf37b0 100644
> --- a/drivers/gpu/drm/vkms/tests/vkms_format_test.c
> +++ b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
[ ... ]
> +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);
[Severity: Low]
Will this compile when the VKMS KUnit tests are enabled?
The newly added test code calls get_conversion_matrix_to_argb_u16() with 5
arguments, passing false as the 4th argument. However, the function signature
in vkms_formats.h only accepts 4 arguments, which will cause a 'too many
arguments to function' compiler error.
> +
> + 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);
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260722134607.136293-1-harry.wentland@amd.com?part=3
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4 04/11] drm/vkms: Add fixed matrix colorop to color pipeline
2026-07-22 13:45 [PATCH v4 00/11] YUV conversion colorop with amdgpu and VKMS Harry Wentland
` (2 preceding siblings ...)
2026-07-22 13:45 ` [PATCH v4 03/11] drm/vkms: Add KUnit test for YCbCr to RGB conversion matrices Harry Wentland
@ 2026-07-22 13:46 ` Harry Wentland
2026-07-22 13:46 ` [PATCH v4 05/11] drm/vkms: Add atomic check and matrix handling for fixed matrix colorop Harry Wentland
` (6 subsequent siblings)
10 siblings, 0 replies; 19+ messages in thread
From: Harry Wentland @ 2026-07-22 13:46 UTC (permalink / raw)
To: dri-devel, amd-gfx; +Cc: Harry Wentland, Alex Hung, Robert Mader
Insert a fixed matrix colorop as the first operation in the
VKMS color pipeline, before the existing 1D curve. This allows
userspace to select YUV-to-RGB conversion via the color pipeline
for YCbCr framebuffers.
The CSC Fixed Matrix colorop advertises support for all six
YUV-to-RGB conversion presets:
- YUV601/709/2020 full-range to RGB
- YUV601/709/2020 limited-range to RGB
Bump MAX_COLOR_PIPELINE_OPS from 4 to 5 to accommodate the new op.
Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Reviewed-by: Alex Hung <alex.hung@amd.com>
Reviewed-by: Robert Mader <robert.mader@collabora.com>
---
drivers/gpu/drm/vkms/vkms_colorop.c | 66 ++++++++++++++++++++---------
1 file changed, 46 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_colorop.c b/drivers/gpu/drm/vkms/vkms_colorop.c
index 071f3a8d2e7c..9424b1994239 100644
--- a/drivers/gpu/drm/vkms/vkms_colorop.c
+++ b/drivers/gpu/drm/vkms/vkms_colorop.c
@@ -12,11 +12,19 @@ static const u64 supported_tfs =
BIT(DRM_COLOROP_1D_CURVE_SRGB_EOTF) |
BIT(DRM_COLOROP_1D_CURVE_SRGB_INV_EOTF);
+static const u64 supported_fm =
+ BIT(DRM_COLOROP_FM_YCBCR601_FULL_RGB) |
+ BIT(DRM_COLOROP_FM_YCBCR601_LIMITED_RGB) |
+ BIT(DRM_COLOROP_FM_YCBCR709_FULL_RGB) |
+ BIT(DRM_COLOROP_FM_YCBCR709_LIMITED_RGB) |
+ BIT(DRM_COLOROP_FM_YCBCR2020_NC_FULL_RGB) |
+ BIT(DRM_COLOROP_FM_YCBCR2020_NC_LIMITED_RGB);
+
static const struct drm_colorop_funcs vkms_colorop_funcs = {
.destroy = drm_colorop_destroy,
};
-#define MAX_COLOR_PIPELINE_OPS 4
+#define MAX_COLOR_PIPELINE_OPS 5
static int vkms_initialize_color_pipeline(struct drm_plane *plane, struct drm_prop_enum_list *list)
{
@@ -27,7 +35,25 @@ static int vkms_initialize_color_pipeline(struct drm_plane *plane, struct drm_pr
memset(ops, 0, sizeof(ops));
- /* 1st op: 1d curve */
+ /* 1st op: Fixed Matrix (YUV to RGB) */
+ ops[i] = kzalloc_obj(*ops[i]);
+ if (!ops[i]) {
+ drm_err(dev, "KMS: Failed to allocate colorop\n");
+ ret = -ENOMEM;
+ goto cleanup;
+ }
+
+ ret = drm_plane_colorop_fixed_matrix_init(dev, ops[i], plane, &vkms_colorop_funcs,
+ supported_fm,
+ DRM_COLOROP_FLAG_ALLOW_BYPASS);
+ if (ret)
+ goto cleanup;
+
+ list->type = ops[i]->base.id;
+
+ i++;
+
+ /* 2nd op: 1d curve */
ops[i] = kzalloc_obj(*ops[i]);
if (!ops[i]) {
drm_err(dev, "KMS: Failed to allocate colorop\n");
@@ -41,23 +67,6 @@ static int vkms_initialize_color_pipeline(struct drm_plane *plane, struct drm_pr
if (ret)
goto cleanup;
- list->type = ops[i]->base.id;
-
- i++;
-
- /* 2nd op: 3x4 matrix */
- ops[i] = kzalloc_obj(*ops[i]);
- if (!ops[i]) {
- drm_err(dev, "KMS: Failed to allocate colorop\n");
- ret = -ENOMEM;
- goto cleanup;
- }
-
- ret = drm_plane_colorop_ctm_3x4_init(dev, ops[i], plane, &vkms_colorop_funcs,
- DRM_COLOROP_FLAG_ALLOW_BYPASS);
- if (ret)
- goto cleanup;
-
drm_colorop_set_next_property(ops[i - 1], ops[i]);
i++;
@@ -79,7 +88,24 @@ static int vkms_initialize_color_pipeline(struct drm_plane *plane, struct drm_pr
i++;
- /* 4th op: 1d curve */
+ /* 4th op: 3x4 matrix */
+ ops[i] = kzalloc_obj(*ops[i]);
+ if (!ops[i]) {
+ drm_err(dev, "KMS: Failed to allocate colorop\n");
+ ret = -ENOMEM;
+ goto cleanup;
+ }
+
+ ret = drm_plane_colorop_ctm_3x4_init(dev, ops[i], plane, &vkms_colorop_funcs,
+ DRM_COLOROP_FLAG_ALLOW_BYPASS);
+ if (ret)
+ goto cleanup;
+
+ drm_colorop_set_next_property(ops[i - 1], ops[i]);
+
+ i++;
+
+ /* 5th op: 1d curve */
ops[i] = kzalloc_obj(*ops[i]);
if (!ops[i]) {
drm_err(dev, "KMS: Failed to allocate colorop\n");
--
2.55.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v4 05/11] drm/vkms: Add atomic check and matrix handling for fixed matrix colorop
2026-07-22 13:45 [PATCH v4 00/11] YUV conversion colorop with amdgpu and VKMS Harry Wentland
` (3 preceding siblings ...)
2026-07-22 13:46 ` [PATCH v4 04/11] drm/vkms: Add fixed matrix colorop to color pipeline Harry Wentland
@ 2026-07-22 13:46 ` Harry Wentland
2026-07-22 13:59 ` sashiko-bot
2026-07-22 13:46 ` [PATCH v4 06/11] drm/amd/display: Add fixed matrix colorop to color pipeline Harry Wentland
` (5 subsequent siblings)
10 siblings, 1 reply; 19+ messages in thread
From: Harry Wentland @ 2026-07-22 13:46 UTC (permalink / raw)
To: dri-devel, amd-gfx; +Cc: Harry Wentland, Alex Hung, Robert Mader
In vkms_plane_atomic_check(), validate that when a color pipeline
is active and the framebuffer format is YUV:
- The first colorop must be DRM_COLOROP_FIXED_MATRIX
- The fixed matrix colorop must not be bypassed
In vkms_plane_atomic_update(), when a color pipeline is active,
read the fixed_matrix_type from the first colorop state and map
it to the appropriate (color_encoding, color_range) pair for the
existing get_conversion_matrix_to_argb_u16() function.
In apply_colorop(), add a no-op case for DRM_COLOROP_FIXED_MATRIX
since the actual YUV-to-RGB conversion is already performed at
format read time via the conversion matrix.
Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Reviewed-by: Alex Hung <alex.hung@amd.com>
Reviewed-by: Robert Mader <robert.mader@collabora.com>
---
drivers/gpu/drm/vkms/tests/vkms_format_test.c | 2 +-
drivers/gpu/drm/vkms/vkms_composer.c | 6 ++
drivers/gpu/drm/vkms/vkms_formats.c | 9 +++
drivers/gpu/drm/vkms/vkms_formats.h | 2 +-
drivers/gpu/drm/vkms/vkms_plane.c | 55 ++++++++++++++++++-
5 files changed, 71 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vkms/tests/vkms_format_test.c b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
index e746b18bf37b..e931cdfafae7 100644
--- a/drivers/gpu/drm/vkms/tests/vkms_format_test.c
+++ b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
@@ -232,7 +232,7 @@ static void vkms_format_test_yuv_u16_to_argb_u16(struct kunit *test)
struct conversion_matrix matrix;
get_conversion_matrix_to_argb_u16
- (DRM_FORMAT_NV12, param->encoding, param->range, &matrix);
+ (DRM_FORMAT_NV12, param->encoding, param->range, false, &matrix);
argb = argb_u16_from_yuv161616(&matrix, color->yuv.y, color->yuv.u,
color->yuv.v);
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 83d217085ad0..899120cd07ac 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -187,6 +187,12 @@ static void apply_colorop(struct pixel_argb_s32 *pixel, struct drm_colorop *colo
if (colorop_state->data)
apply_3x4_matrix(pixel,
(struct drm_color_ctm_3x4 *)colorop_state->data->data);
+ } else if (colorop->type == DRM_COLOROP_FIXED_MATRIX) {
+ /*
+ * Fixed Matrix is a no-op here. YUV-to-RGB conversion
+ * is handled at format read time via the
+ * conversion_matrix set in vkms_plane_atomic_update().
+ */
}
}
diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index 4d5fcaeb82c5..964b574d9ed7 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -895,16 +895,25 @@ static void swap_uv_columns(struct conversion_matrix *matrix)
* @format: DRM_FORMAT_* value for which to obtain a conversion function (see [drm_fourcc.h])
* @encoding: DRM_COLOR_* value for which to obtain a conversion matrix
* @range: DRM_COLOR_*_RANGE value for which to obtain a conversion matrix
+ * @bypass: If true, return an identity (no-op) matrix that passes the samples
+ * through unchanged, ignoring @encoding and @range. Used when a fixed
+ * matrix colorop is present but bypassed.
* @matrix: Pointer to store the value into
*/
void get_conversion_matrix_to_argb_u16(u32 format,
enum drm_color_encoding encoding,
enum drm_color_range range,
+ bool bypass,
struct conversion_matrix *matrix)
{
const struct conversion_matrix *matrix_to_copy;
bool limited_range;
+ if (bypass) {
+ memcpy(matrix, &no_operation, sizeof(no_operation));
+ return;
+ }
+
switch (range) {
case DRM_COLOR_YCBCR_LIMITED_RANGE:
limited_range = true;
diff --git a/drivers/gpu/drm/vkms/vkms_formats.h b/drivers/gpu/drm/vkms/vkms_formats.h
index eeb208cdd6b1..d969cc669a5e 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.h
+++ b/drivers/gpu/drm/vkms/vkms_formats.h
@@ -10,7 +10,7 @@ pixel_read_line_t get_pixel_read_line_function(u32 format);
pixel_write_t get_pixel_write_function(u32 format);
void get_conversion_matrix_to_argb_u16(u32 format, enum drm_color_encoding encoding,
- enum drm_color_range range,
+ enum drm_color_range range, bool bypass,
struct conversion_matrix *matrix);
#if IS_ENABLED(CONFIG_KUNIT)
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 68cb2a3335e6..6ee5c3f3207c 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -6,6 +6,7 @@
#include <drm/drm_atomic.h>
#include <drm/drm_atomic_helper.h>
#include <drm/drm_blend.h>
+#include <drm/drm_colorop.h>
#include <drm/drm_fourcc.h>
#include <drm/drm_gem_atomic_helper.h>
#include <drm/drm_gem_framebuffer_helper.h>
@@ -131,6 +132,9 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
struct drm_framebuffer *fb = new_state->fb;
struct vkms_frame_info *frame_info;
u32 fmt;
+ enum drm_color_encoding encoding = new_state->color_encoding;
+ enum drm_color_range range = new_state->color_range;
+ bool bypass = false;
if (!new_state->crtc || !fb)
return;
@@ -148,7 +152,49 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
frame_info->rotation = new_state->rotation;
vkms_plane_state->pixel_read_line = get_pixel_read_line_function(fmt);
- get_conversion_matrix_to_argb_u16(fmt, new_state->color_encoding, new_state->color_range,
+
+ if (new_state->color_pipeline) {
+ struct drm_colorop *colorop = new_state->color_pipeline;
+ struct drm_colorop_state *colorop_state;
+
+ colorop_state = drm_atomic_get_new_colorop_state(state, colorop);
+ bypass = !colorop_state || colorop_state->bypass;
+
+ if (!bypass) {
+ switch (colorop_state->fixed_matrix_type) {
+ case DRM_COLOROP_FM_YCBCR601_FULL_RGB:
+ encoding = DRM_COLOR_YCBCR_BT601;
+ range = DRM_COLOR_YCBCR_FULL_RANGE;
+ break;
+ case DRM_COLOROP_FM_YCBCR601_LIMITED_RGB:
+ encoding = DRM_COLOR_YCBCR_BT601;
+ range = DRM_COLOR_YCBCR_LIMITED_RANGE;
+ break;
+ case DRM_COLOROP_FM_YCBCR709_FULL_RGB:
+ encoding = DRM_COLOR_YCBCR_BT709;
+ range = DRM_COLOR_YCBCR_FULL_RANGE;
+ break;
+ case DRM_COLOROP_FM_YCBCR709_LIMITED_RGB:
+ encoding = DRM_COLOR_YCBCR_BT709;
+ range = DRM_COLOR_YCBCR_LIMITED_RANGE;
+ break;
+ case DRM_COLOROP_FM_YCBCR2020_NC_FULL_RGB:
+ encoding = DRM_COLOR_YCBCR_BT2020;
+ range = DRM_COLOR_YCBCR_FULL_RANGE;
+ break;
+ case DRM_COLOROP_FM_YCBCR2020_NC_LIMITED_RGB:
+ encoding = DRM_COLOR_YCBCR_BT2020;
+ range = DRM_COLOR_YCBCR_LIMITED_RANGE;
+ break;
+ default:
+ encoding = DRM_COLOR_YCBCR_BT709;
+ range = DRM_COLOR_YCBCR_LIMITED_RANGE;
+ break;
+ }
+ }
+ }
+
+ get_conversion_matrix_to_argb_u16(fmt, encoding, range, bypass,
&vkms_plane_state->conversion_matrix);
}
@@ -175,6 +221,13 @@ static int vkms_plane_atomic_check(struct drm_plane *plane,
if (ret != 0)
return ret;
+ if (new_plane_state->color_pipeline && new_plane_state->fb->format->is_yuv) {
+ struct drm_colorop *colorop = new_plane_state->color_pipeline;
+
+ if (colorop->type != DRM_COLOROP_FIXED_MATRIX)
+ return -EINVAL;
+ }
+
return 0;
}
--
2.55.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4 05/11] drm/vkms: Add atomic check and matrix handling for fixed matrix colorop
2026-07-22 13:46 ` [PATCH v4 05/11] drm/vkms: Add atomic check and matrix handling for fixed matrix colorop Harry Wentland
@ 2026-07-22 13:59 ` sashiko-bot
0 siblings, 0 replies; 19+ messages in thread
From: sashiko-bot @ 2026-07-22 13:59 UTC (permalink / raw)
To: Harry Wentland; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues:
- [High] Page flips that do not modify the color pipeline incorrectly bypass the YUV-to-RGB conversion matrix.
- [Medium] vkms_plane_atomic_check fails to validate the bypass state for the fixed matrix colorop.
- [Medium] FIXED_MATRIX colorop is silently ignored for RGB formats, and state is unconditionally interpreted as fixed_matrix.
Pre-existing issues:
- [High] The background composer accesses live DRM object state (`colorop->state`), risking Use-After-Free.
--
--- Patch [5]: [PATCH v4 05/11] drm/vkms: Add atomic check and matrix handling for fixed matrix colorop ---
commit 7c8489284a67e8c66fd1ead26966ba84d8380366
Author: Harry Wentland <harry.wentland@amd.com>
drm/vkms: Add atomic check and matrix handling for fixed matrix colorop
This commit validates that YUV framebuffers use a non-bypassed fixed matrix
colorop when a color pipeline is active. It also maps the fixed matrix type
to color encoding and range for YUV-to-RGB conversion, while treating the
fixed matrix colorop as a no-op during composition.
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 83d217085ad09..899120cd07ac5 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -187,6 +187,12 @@ static void apply_colorop(struct pixel_argb_s32 *pixel, struct drm_colorop *colo
> if (colorop_state->data)
> apply_3x4_matrix(pixel,
> (struct drm_color_ctm_3x4 *)colorop_state->data->data);
> + } else if (colorop->type == DRM_COLOROP_FIXED_MATRIX) {
> + /*
> + * Fixed Matrix is a no-op here. YUV-to-RGB conversion
> + * is handled at format read time via the
> + * conversion_matrix set in vkms_plane_atomic_update().
> + */
> }
[Severity: Medium]
If userspace applies a DRM_COLOROP_FIXED_MATRIX to an RGB plane, will this
silently ignore the operation instead of properly rejecting it in the atomic
check?
[Severity: High]
This isn't a bug introduced by this patch, but doesn't reading the live
colorop->state pointer asynchronously in the worker risk a use-after-free
if a concurrent atomic commit frees the old state?
Looking at the beginning of apply_colorop() in vkms_composer.c:
static void apply_colorop(struct pixel_argb_s32 *pixel, struct drm_colorop *colorop)
{
struct drm_colorop_state *colorop_state = colorop->state;
Should VKMS snapshot the required properties into vkms_plane_state during
atomic_update instead of dereferencing the live DRM object state asynchronously?
[ ... ]
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 68cb2a3335e64..6ee5c3f3207c5 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
[ ... ]
> @@ -148,7 +152,49 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
> frame_info->rotation = new_state->rotation;
>
> vkms_plane_state->pixel_read_line = get_pixel_read_line_function(fmt);
> - get_conversion_matrix_to_argb_u16(fmt, new_state->color_encoding, new_state->color_range,
> +
> + if (new_state->color_pipeline) {
> + struct drm_colorop *colorop = new_state->color_pipeline;
> + struct drm_colorop_state *colorop_state;
> +
> + colorop_state = drm_atomic_get_new_colorop_state(state, colorop);
> + bypass = !colorop_state || colorop_state->bypass;
[Severity: High]
Could this logic inadvertently break YUV decoding on standard page flips?
If a page flip does not include the colorop in the atomic commit,
drm_atomic_get_new_colorop_state() returns NULL, which forces bypass to true.
Will this reset the conversion matrix to a no-op on the second frame?
> +
> + if (!bypass) {
> + switch (colorop_state->fixed_matrix_type) {
[Severity: Medium]
When the active colorop on an RGB plane is a 1D_CURVE instead of a
FIXED_MATRIX, will this unconditionally misinterpret the state struct and read
an invalid fixed_matrix_type?
> + case DRM_COLOROP_FM_YCBCR601_FULL_RGB:
> + encoding = DRM_COLOR_YCBCR_BT601;
> + range = DRM_COLOR_YCBCR_FULL_RANGE;
> + break;
[ ... ]
> @@ -175,6 +221,13 @@ static int vkms_plane_atomic_check(struct drm_plane *plane,
> if (ret != 0)
> return ret;
>
> + if (new_plane_state->color_pipeline && new_plane_state->fb->format->is_yuv) {
> + struct drm_colorop *colorop = new_plane_state->color_pipeline;
> +
> + if (colorop->type != DRM_COLOROP_FIXED_MATRIX)
> + return -EINVAL;
[Severity: Medium]
The commit message states that "The fixed matrix colorop must not be bypassed"
when the framebuffer format is YUV.
Does this atomic check fail to enforce that rule by not retrieving the
colorop state to check its bypass property?
> + }
> +
> return 0;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260722134607.136293-1-harry.wentland@amd.com?part=5
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4 06/11] drm/amd/display: Add fixed matrix colorop to color pipeline
2026-07-22 13:45 [PATCH v4 00/11] YUV conversion colorop with amdgpu and VKMS Harry Wentland
` (4 preceding siblings ...)
2026-07-22 13:46 ` [PATCH v4 05/11] drm/vkms: Add atomic check and matrix handling for fixed matrix colorop Harry Wentland
@ 2026-07-22 13:46 ` Harry Wentland
2026-07-22 14:03 ` sashiko-bot
2026-07-22 13:46 ` [PATCH v4 07/11] drm/amd/display: Implement fixed matrix colorop color space mapping Harry Wentland
` (4 subsequent siblings)
10 siblings, 1 reply; 19+ messages in thread
From: Harry Wentland @ 2026-07-22 13:46 UTC (permalink / raw)
To: dri-devel, amd-gfx; +Cc: Harry Wentland, Alex Hung
Insert a fixed matrix colorop as the first operation in the
amdgpu color pipeline, before the existing DEGAM 1D curve. This
allows userspace to select YUV-to-RGB conversion via the color
pipeline for YCbCr framebuffers.
The fixed matrix colorop advertises support for all six YUV-to-RGB
conversion presets:
- YUV601/709/2020 full-range to RGB
- YUV601/709/2020 limited-range to RGB
Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Reviewed-by: Alex Hung <alex.hung@amd.com>
---
.../amd/display/amdgpu_dm/amdgpu_dm_colorop.c | 27 ++++++++++++++++++-
.../amd/display/amdgpu_dm/amdgpu_dm_colorop.h | 1 +
2 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c
index 056a76b88f43..1ed12cdce317 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c
@@ -55,6 +55,14 @@ const u64 amdgpu_dm_supported_blnd_tfs =
BIT(DRM_COLOROP_1D_CURVE_GAMMA22);
EXPORT_IF_KUNIT(amdgpu_dm_supported_blnd_tfs);
+const u64 amdgpu_dm_supported_fm =
+ BIT(DRM_COLOROP_FM_YCBCR601_FULL_RGB) |
+ BIT(DRM_COLOROP_FM_YCBCR601_LIMITED_RGB) |
+ BIT(DRM_COLOROP_FM_YCBCR709_FULL_RGB) |
+ BIT(DRM_COLOROP_FM_YCBCR709_LIMITED_RGB) |
+ BIT(DRM_COLOROP_FM_YCBCR2020_NC_FULL_RGB) |
+ BIT(DRM_COLOROP_FM_YCBCR2020_NC_LIMITED_RGB);
+
#define MAX_COLOR_PIPELINE_OPS 10
#define LUT3D_SIZE 17
@@ -73,6 +81,23 @@ amdgpu_dm_build_default_pipeline(struct drm_device *dev, struct drm_plane *plane
memset(ops, 0, sizeof(ops));
+ /* Fixed Matrix (YUV to RGB) */
+ ops[i] = kzalloc_obj(*ops[0]);
+ if (!ops[i]) {
+ ret = -ENOMEM;
+ goto cleanup;
+ }
+
+ ret = drm_plane_colorop_fixed_matrix_init(dev, ops[i], plane, &dm_colorop_funcs,
+ amdgpu_dm_supported_fm,
+ DRM_COLOROP_FLAG_ALLOW_BYPASS);
+ if (ret)
+ goto cleanup;
+
+ list->type = ops[i]->base.id;
+
+ i++;
+
/* 1D curve - DEGAM TF */
ops[i] = kzalloc_obj(*ops[0]);
if (!ops[i]) {
@@ -86,7 +111,7 @@ amdgpu_dm_build_default_pipeline(struct drm_device *dev, struct drm_plane *plane
if (ret)
goto cleanup;
- list->type = ops[i]->base.id;
+ drm_colorop_set_next_property(ops[i - 1], ops[i]);
i++;
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.h
index 77364d954d3b..7802efa5312d 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.h
@@ -30,6 +30,7 @@
extern const u64 amdgpu_dm_supported_degam_tfs;
extern const u64 amdgpu_dm_supported_shaper_tfs;
extern const u64 amdgpu_dm_supported_blnd_tfs;
+extern const u64 amdgpu_dm_supported_fm;
int amdgpu_dm_initialize_default_pipeline(struct drm_plane *plane, struct drm_prop_enum_list *list);
--
2.55.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4 06/11] drm/amd/display: Add fixed matrix colorop to color pipeline
2026-07-22 13:46 ` [PATCH v4 06/11] drm/amd/display: Add fixed matrix colorop to color pipeline Harry Wentland
@ 2026-07-22 14:03 ` sashiko-bot
0 siblings, 0 replies; 19+ messages in thread
From: sashiko-bot @ 2026-07-22 14:03 UTC (permalink / raw)
To: Harry Wentland; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The newly added Fixed Matrix (FM) colorop is unhandled by the AMD atomic check/commit logic, breaking the entire AMD color pipeline feature.
--
--- Patch [6]: [PATCH v4 06/11] drm/amd/display: Add fixed matrix colorop to color pipeline ---
commit d87e1fc27fd29f8b37f2f841d69915be8eea3f32
Author: Harry Wentland <harry.wentland@amd.com>
drm/amd/display: Add fixed matrix colorop to color pipeline
This commit inserts a fixed matrix colorop as the first operation in the amdgpu
color pipeline, before the DEGAM 1D curve. This enables userspace to select
YUV-to-RGB conversion via the color pipeline for YCbCr framebuffers.
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c
> index 056a76b88f438..1ed12cdce3172 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_colorop.c
[ ... ]
> @@ -73,6 +81,23 @@ amdgpu_dm_build_default_pipeline(struct drm_device *dev, struct drm_plane *plane
>
> memset(ops, 0, sizeof(ops));
>
> + /* Fixed Matrix (YUV to RGB) */
> + ops[i] = kzalloc_obj(*ops[0]);
> + if (!ops[i]) {
> + ret = -ENOMEM;
> + goto cleanup;
> + }
> +
> + ret = drm_plane_colorop_fixed_matrix_init(dev, ops[i], plane, &dm_colorop_funcs,
> + amdgpu_dm_supported_fm,
> + DRM_COLOROP_FLAG_ALLOW_BYPASS);
> + if (ret)
> + goto cleanup;
> +
> + list->type = ops[i]->base.id;
> +
> + i++;
[Severity: High]
Will this cause atomic checks to unconditionally fail for the color pipeline?
Because the fixed matrix colorop is now the first operation in the pipeline,
userspace will pass it in plane_state->color_pipeline.
However, looking at amdgpu_dm_plane_set_colorop_properties(), it expects the
first operation to be the DEGAM TF:
amdgpu_dm_color.c:amdgpu_dm_plane_set_colorop_properties() {
...
/* 1D Curve - DEGAM TF */
if (!colorop)
return -EINVAL;
ret = __set_dm_plane_colorop_degamma(plane_state, dc_plane_state,
colorop);
if (ret)
return ret;
...
}
When this passes the new fixed matrix colorop to
__set_dm_plane_colorop_degamma(), it ultimately calls
__set_colorop_in_tf_1d_curve() which has a strict type check:
amdgpu_dm_color.c:__set_colorop_in_tf_1d_curve() {
...
if (colorop->type != DRM_COLOROP_1D_CURVE)
return -EINVAL;
...
}
Since the first colorop is now DRM_COLOROP_FIXED_MATRIX, this type check will
fail and return -EINVAL.
Does amdgpu_dm_plane_set_colorop_properties() need to be updated to parse and
handle the new fixed matrix colorop before processing the DEGAM TF?
> +
> /* 1D curve - DEGAM TF */
> ops[i] = kzalloc_obj(*ops[0]);
> if (!ops[i]) {
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260722134607.136293-1-harry.wentland@amd.com?part=6
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4 07/11] drm/amd/display: Implement fixed matrix colorop color space mapping
2026-07-22 13:45 [PATCH v4 00/11] YUV conversion colorop with amdgpu and VKMS Harry Wentland
` (5 preceding siblings ...)
2026-07-22 13:46 ` [PATCH v4 06/11] drm/amd/display: Add fixed matrix colorop to color pipeline Harry Wentland
@ 2026-07-22 13:46 ` Harry Wentland
2026-07-22 14:14 ` sashiko-bot
2026-07-22 13:46 ` [PATCH v4 08/11] drm/amd/display: Use GAMCOR for first TF if YUV conversion is needed Harry Wentland
` (3 subsequent siblings)
10 siblings, 1 reply; 19+ messages in thread
From: Harry Wentland @ 2026-07-22 13:46 UTC (permalink / raw)
To: dri-devel, amd-gfx; +Cc: Harry Wentland, Alex Hung
Add __set_dm_plane_colorop_fixed_matrix() which maps the fixed matrix
type enum to the appropriate dc_color_space for DC programming:
YCBCR601_FULL_RGB -> COLOR_SPACE_YCBCR601
YCBCR601_LIMITED_RGB -> COLOR_SPACE_YCBCR601_LIMITED
YCBCR709_FULL_RGB -> COLOR_SPACE_YCBCR709
YCBCR709_LIMITED_RGB -> COLOR_SPACE_YCBCR709_LIMITED
YCBCR2020_FULL_RGB_NC -> COLOR_SPACE_2020_YCBCR_FULL
YCBCR2020_LIMITED_RGB_NC -> COLOR_SPACE_2020_YCBCR_LIMITED
When the fixed matrix is bypassed, color_space is set to
COLOR_SPACE_UNKNOWN.
Update amdgpu_dm_plane_set_colorop_properties() to process the
fixed matrix colorop first (before DEGAM), matching the new pipeline
order.
Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Reviewed-by: Alex Hung <alex.hung@amd.com>
---
.../amd/display/amdgpu_dm/amdgpu_dm_color.c | 68 ++++++++++++++++++-
1 file changed, 67 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
index 357c7c5c85cf..62791077ceef 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
@@ -1545,6 +1545,61 @@ __set_colorop_in_tf_1d_curve(struct dc_plane_state *dc_plane_state,
}
EXPORT_IF_KUNIT(__set_colorop_in_tf_1d_curve);
+static int
+__set_dm_plane_colorop_fixed_matrix(struct drm_plane_state *plane_state,
+ struct dc_plane_state *dc_plane_state,
+ struct drm_colorop *colorop)
+{
+ struct drm_colorop *old_colorop;
+ struct drm_colorop_state *colorop_state = NULL, *new_colorop_state;
+ struct drm_atomic_commit *state = plane_state->state;
+ int i = 0;
+
+ old_colorop = colorop;
+
+ for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
+ if (new_colorop_state->colorop == old_colorop) {
+ colorop_state = new_colorop_state;
+ break;
+ }
+ }
+
+ if (!colorop_state)
+ return -EINVAL;
+
+ if (colorop_state->bypass) {
+ dc_plane_state->color_space = COLOR_SPACE_SRGB;
+ return 0;
+ }
+
+ switch (colorop_state->fixed_matrix_type) {
+ case DRM_COLOROP_FM_YCBCR601_FULL_RGB:
+ dc_plane_state->color_space = COLOR_SPACE_YCBCR601;
+ break;
+ case DRM_COLOROP_FM_YCBCR601_LIMITED_RGB:
+ dc_plane_state->color_space = COLOR_SPACE_YCBCR601_LIMITED;
+ break;
+ case DRM_COLOROP_FM_YCBCR709_FULL_RGB:
+ dc_plane_state->color_space = COLOR_SPACE_YCBCR709;
+ break;
+ case DRM_COLOROP_FM_YCBCR709_LIMITED_RGB:
+ dc_plane_state->color_space = COLOR_SPACE_YCBCR709_LIMITED;
+ break;
+ case DRM_COLOROP_FM_YCBCR2020_NC_FULL_RGB:
+ dc_plane_state->color_space = COLOR_SPACE_2020_YCBCR_FULL;
+ break;
+ case DRM_COLOROP_FM_YCBCR2020_NC_LIMITED_RGB:
+ dc_plane_state->color_space = COLOR_SPACE_2020_YCBCR_LIMITED;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ dc_plane_state->update_bits.full_update = 1;
+
+ return 0;
+}
+
static int
__set_dm_plane_colorop_degamma(struct drm_plane_state *plane_state,
struct dc_plane_state *dc_plane_state,
@@ -1946,10 +2001,21 @@ amdgpu_dm_plane_set_colorop_properties(struct drm_plane_state *plane_state,
bool has_3dlut = adev->dm.dc->caps.color.dpp.hw_3d_lut || adev->dm.dc->caps.color.mpc.preblend;
int ret;
- /* 1D Curve - DEGAM TF */
+ /* Fixed Matrix (YUV to RGB) */
if (!colorop)
return -EINVAL;
+ ret = __set_dm_plane_colorop_fixed_matrix(plane_state, dc_plane_state, colorop);
+ if (ret)
+ return ret;
+
+ /* 1D Curve - DEGAM TF */
+ colorop = colorop->next;
+ if (!colorop) {
+ drm_dbg(dev, "no degamma colorop found\n");
+ return -EINVAL;
+ }
+
ret = __set_dm_plane_colorop_degamma(plane_state, dc_plane_state, colorop);
if (ret)
return ret;
--
2.55.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4 07/11] drm/amd/display: Implement fixed matrix colorop color space mapping
2026-07-22 13:46 ` [PATCH v4 07/11] drm/amd/display: Implement fixed matrix colorop color space mapping Harry Wentland
@ 2026-07-22 14:14 ` sashiko-bot
0 siblings, 0 replies; 19+ messages in thread
From: sashiko-bot @ 2026-07-22 14:14 UTC (permalink / raw)
To: Harry Wentland; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues:
- [High] `vkms_plane_atomic_update` unintentionally disables the fixed matrix colorop during simple page flips.
- [Medium] `drm_plane_colorop_fixed_matrix_init` uses `-BIT(DRM_COLOROP_FM_COUNT)` which zero-extends on 32-bit platforms, failing to validate bits 32-63 of the `u64` `supported_fm` mask.
- [Medium] `__set_dm_plane_colorop_fixed_matrix` unconditionally sets `full_update = 1`, which can unnecessarily destroy fast page flip performance.
- [Low] Mismatch between commit message documentation and code implementation for bypass color space.
Pre-existing issues:
- [High] `__set_dm_plane_colorop_fixed_matrix` and `amdgpu_dm_update_plane_color_mgmt` silently drop the entire color pipeline on page flips.
--
--- Patch [7]: [PATCH v4 07/11] drm/amd/display: Implement fixed matrix colorop color space mapping ---
commit b2189c95d176aaf780d252979cb22287548aef19
Author: Harry Wentland <harry.wentland@amd.com>
drm/amd/display: Implement fixed matrix colorop color space mapping
Add __set_dm_plane_colorop_fixed_matrix() which maps the fixed matrix type
enum to the appropriate dc_color_space for DC programming. It also updates
the plane properties processing to handle the fixed matrix before degamma.
> When the fixed matrix is bypassed, color_space is set to
> COLOR_SPACE_UNKNOWN.
[Severity: Low]
This isn't a bug, but there is a mismatch between the documentation and the
implementation. The commit message states COLOR_SPACE_UNKNOWN is used when
bypassed, but the code sets it to COLOR_SPACE_SRGB.
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> index 357c7c5c85cff..62791077ceef8 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> @@ -1545,6 +1545,61 @@ __set_colorop_in_tf_1d_curve(struct dc_plane_state *dc_plane_state,
> }
> EXPORT_IF_KUNIT(__set_colorop_in_tf_1d_curve);
>
> +static int
> +__set_dm_plane_colorop_fixed_matrix(struct drm_plane_state *plane_state,
> + struct dc_plane_state *dc_plane_state,
> + struct drm_colorop *colorop)
> +{
> + struct drm_colorop *old_colorop;
> + struct drm_colorop_state *colorop_state = NULL, *new_colorop_state;
> + struct drm_atomic_commit *state = plane_state->state;
> + int i = 0;
> +
> + old_colorop = colorop;
> +
> + for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
> + if (new_colorop_state->colorop == old_colorop) {
> + colorop_state = new_colorop_state;
> + break;
> + }
> + }
> +
> + if (!colorop_state)
> + return -EINVAL;
[Severity: High]
This isn't a regression introduced by this patch, but does returning -EINVAL
here cause the entire hardware color pipeline configuration to be silently
discarded?
When userspace performs a fast page flip without modifying the color pipeline,
the DRM core does not add the colorop states to the atomic commit.
Since for_each_new_colorop_in_state() only searches the current transaction,
it will return -EINVAL if the state is missing. Can this lead to
amdgpu_dm_update_plane_color_mgmt() catching this -EINVAL and reverting to
legacy DRM color properties on every simple page flip?
> +
> + if (colorop_state->bypass) {
> + dc_plane_state->color_space = COLOR_SPACE_SRGB;
> + return 0;
> + }
> +
> + switch (colorop_state->fixed_matrix_type) {
> + case DRM_COLOROP_FM_YCBCR601_FULL_RGB:
> + dc_plane_state->color_space = COLOR_SPACE_YCBCR601;
> + break;
[ ... ]
> + case DRM_COLOROP_FM_YCBCR2020_NC_LIMITED_RGB:
> + dc_plane_state->color_space = COLOR_SPACE_2020_YCBCR_LIMITED;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + dc_plane_state->update_bits.full_update = 1;
> +
> + return 0;
> +}
[Severity: Medium]
Does setting full_update to 1 unconditionally here unnecessarily impact fast
page flip performance?
If the fixed matrix colorop is explicitly included in the atomic commit, this
forces a full update even if the state didn't actually change from the
previous commit.
Should this only force a full update if the color_space or bypass state
actively transitioned, to avoid hardware sequencer overhead and potential
stuttering?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260722134607.136293-1-harry.wentland@amd.com?part=7
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4 08/11] drm/amd/display: Use GAMCOR for first TF if YUV conversion is needed
2026-07-22 13:45 [PATCH v4 00/11] YUV conversion colorop with amdgpu and VKMS Harry Wentland
` (6 preceding siblings ...)
2026-07-22 13:46 ` [PATCH v4 07/11] drm/amd/display: Implement fixed matrix colorop color space mapping Harry Wentland
@ 2026-07-22 13:46 ` Harry Wentland
2026-07-22 13:46 ` [PATCH v4 09/11] drm/amd/display: Check actual state during commit_tail Harry Wentland
` (2 subsequent siblings)
10 siblings, 0 replies; 19+ messages in thread
From: Harry Wentland @ 2026-07-22 13:46 UTC (permalink / raw)
To: dri-devel, amd-gfx; +Cc: Harry Wentland
For subsampled formats we need to use GAMCOR instead of
the DEGAM block. The color module can create a LUT for
that if we set map_user_ramp to true. So do that when
we have subsampled formats.
Assisted-by: Claude:claude-sonnet-4.5
Signed-off-by: Harry Wentland <harry.wentland@amd.com>
---
.../drm/amd/display/amdgpu_dm/amdgpu_dm_color.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
index 62791077ceef..8913bc9f0f06 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
@@ -1524,6 +1524,8 @@ __set_colorop_in_tf_1d_curve(struct dc_plane_state *dc_plane_state,
struct dc_transfer_func *tf = &dc_plane_state->in_transfer_func;
struct drm_colorop *colorop = colorop_state->colorop;
struct drm_device *drm = colorop->dev;
+ struct dc_color_caps *color_caps = NULL;
+ bool is_subsampled_format;
if (colorop->type != DRM_COLOROP_1D_CURVE)
return -EINVAL;
@@ -1537,10 +1539,23 @@ __set_colorop_in_tf_1d_curve(struct dc_plane_state *dc_plane_state,
}
drm_dbg(drm, "Degamma colorop with ID: %d\n", colorop->base.id);
-
tf->type = TF_TYPE_PREDEFINED;
+
+ /* Check if format requires post-scale color processing (subsampled formats) */
+ is_subsampled_format = (dc_plane_state->format >= SURFACE_PIXEL_FORMAT_VIDEO_BEGIN &&
+ dc_plane_state->format < SURFACE_PIXEL_FORMAT_SUBSAMPLE_END);
+
tf->tf = amdgpu_colorop_tf_to_dc_tf(colorop_state->curve_1d_type);
+ if (dc_plane_state->ctx && dc_plane_state->ctx->dc)
+ color_caps = &dc_plane_state->ctx->dc->caps.color;
+
+ if (!mod_color_calculate_degamma_params(color_caps, tf, NULL,
+ is_subsampled_format)) {
+ drm_err(drm, "Failed to calculate degamma params\n");
+ return -EINVAL;
+ }
+
return 0;
}
EXPORT_IF_KUNIT(__set_colorop_in_tf_1d_curve);
--
2.55.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v4 09/11] drm/amd/display: Check actual state during commit_tail
2026-07-22 13:45 [PATCH v4 00/11] YUV conversion colorop with amdgpu and VKMS Harry Wentland
` (7 preceding siblings ...)
2026-07-22 13:46 ` [PATCH v4 08/11] drm/amd/display: Use GAMCOR for first TF if YUV conversion is needed Harry Wentland
@ 2026-07-22 13:46 ` Harry Wentland
2026-07-22 14:08 ` sashiko-bot
2026-07-22 13:46 ` [PATCH v4 10/11] drm/amd/display: Set color_space to plane_infos Harry Wentland
2026-07-22 13:46 ` [PATCH v4 11/11] drm/amd/display: Force GAMCOR for subsampled surfaces with PQ/Gamma22/HLG Harry Wentland
10 siblings, 1 reply; 19+ messages in thread
From: Harry Wentland @ 2026-07-22 13:46 UTC (permalink / raw)
To: dri-devel, amd-gfx; +Cc: Harry Wentland, Alex Hung
Previously we checked the plane_state->state's plane_color_pipeline
client config when determining whether fill_plane_color_attributes
should run, i.e., whether we had a plane color pipeline and should
use that code for handling fixed matrix properties, or whether we
should look at legacy plane encoding and range.
The problem is that we also call this during commit_tail, during
which plane_state doesn't have the state backpointer. This meant
that during commit_tail we'd look at the legacy plane COLOR_RANGE
and COLOR_ENCODING values and overwrite what we pulled from the
fixed matrix colorop in atomic_check previously.
Instead pass the drm_atomic_state in explicitly and check that.
Assisted-by:Claude:claude-sonnet-4.5
Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Reviewed-by: Alex Hung <alex.hung@amd.com>
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 67b825cbb88f..b7d815997753 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2984,7 +2984,8 @@ STATIC_IFN_KUNIT bool modereset_required(struct drm_crtc_state *crtc_state)
EXPORT_IF_KUNIT(modereset_required);
STATIC_IFN_KUNIT int
-fill_plane_color_attributes(const struct drm_plane_state *plane_state,
+fill_plane_color_attributes(struct drm_atomic_commit *state,
+ const struct drm_plane_state *plane_state,
const enum surface_pixel_format format,
enum dc_color_space *color_space)
{
@@ -2993,7 +2994,7 @@ fill_plane_color_attributes(const struct drm_plane_state *plane_state,
*color_space = COLOR_SPACE_SRGB;
/* Ignore properties when DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE is set */
- if (plane_state->state && plane_state->state->plane_color_pipeline)
+ if (state && state->plane_color_pipeline)
return 0;
/* DRM color properties only affect non-RGB formats. */
@@ -3034,6 +3035,7 @@ EXPORT_IF_KUNIT(fill_plane_color_attributes);
static int
fill_dc_plane_info_and_addr(struct amdgpu_device *adev,
+ struct drm_atomic_commit *state,
const struct drm_plane_state *plane_state,
const u64 tiling_flags,
struct dc_plane_info *plane_info,
@@ -3127,7 +3129,7 @@ fill_dc_plane_info_and_addr(struct amdgpu_device *adev,
plane_info->layer_index = plane_state->normalized_zpos;
- ret = fill_plane_color_attributes(plane_state, plane_info->format,
+ ret = fill_plane_color_attributes(state, plane_state, plane_info->format,
&plane_info->color_space);
if (ret)
return ret;
@@ -3168,7 +3170,7 @@ static int fill_dc_plane_attributes(struct amdgpu_device *adev,
dc_plane_state->clip_rect = scaling_info.clip_rect;
dc_plane_state->scaling_quality = scaling_info.scaling_quality;
- ret = fill_dc_plane_info_and_addr(adev, plane_state,
+ ret = fill_dc_plane_info_and_addr(adev, plane_state->state, plane_state,
afb->tiling_flags,
&plane_info,
&dc_plane_state->address,
@@ -4226,7 +4228,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_commit *state,
}
fill_dc_plane_info_and_addr(
- dm->adev, new_plane_state,
+ dm->adev, state, new_plane_state,
afb->tiling_flags,
&bundle->plane_infos[planes_count],
&bundle->flip_addrs[planes_count].address,
--
2.55.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4 09/11] drm/amd/display: Check actual state during commit_tail
2026-07-22 13:46 ` [PATCH v4 09/11] drm/amd/display: Check actual state during commit_tail Harry Wentland
@ 2026-07-22 14:08 ` sashiko-bot
0 siblings, 0 replies; 19+ messages in thread
From: sashiko-bot @ 2026-07-22 14:08 UTC (permalink / raw)
To: Harry Wentland; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Build Breakage / Function Prototype Mismatch
- [High] Logic Error overwriting correctly calculated color_space with SRGB during commit_tail
--
--- Patch [9]: [PATCH v4 09/11] drm/amd/display: Check actual state during commit_tail ---
commit 7fa51e59a15d532394f2e3959287098fefb48b8d
Author: Harry Wentland <harry.wentland@amd.com>
drm/amd/display: Check actual state during commit_tail
Previously we checked the plane_state->state's plane_color_pipeline
client config when determining whether fill_plane_color_attributes
should run, i.e., whether we had a plane color pipeline and should
use that code for handling fixed matrix properties, or whether we
should look at legacy plane encoding and range.
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index e7080880b2211..4f07fe5688262 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3008,7 +3008,8 @@ STATIC_IFN_KUNIT bool modereset_required(struct drm_crtc_state *crtc_state)
> EXPORT_IF_KUNIT(modereset_required);
>
> STATIC_IFN_KUNIT int
> -fill_plane_color_attributes(const struct drm_plane_state *plane_state,
> +fill_plane_color_attributes(struct drm_atomic_commit *state,
> + const struct drm_plane_state *plane_state,
> const enum surface_pixel_format format,
> enum dc_color_space *color_space)
> {
[Severity: High]
Will this break the build when CONFIG_DRM_AMD_DC_KUNIT_TEST is enabled?
The old prototype remains in drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:
int fill_plane_color_attributes(const struct drm_plane_state *plane_state,
const enum surface_pixel_format format,
enum dc_color_space *color_space);
And the KUnit tests still call the function with 3 arguments:
drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_test.c:dm_test_fill_color_attr_rgb_format() {
...
ret = fill_plane_color_attributes(&plane_state,
SURFACE_PIXEL_FORMAT_GRPH_ARGB8888,
&color_space);
...
}
> @@ -3017,7 +3018,7 @@ fill_plane_color_attributes(const struct drm_plane_state *plane_state,
> *color_space = COLOR_SPACE_SRGB;
>
> /* Ignore properties when DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE is set */
> - if (plane_state->state && plane_state->state->plane_color_pipeline)
> + if (state && state->plane_color_pipeline)
> return 0;
[Severity: High]
Could this unintentionally overwrite the correctly calculated color_space with
COLOR_SPACE_SRGB during commit_tail?
Because *color_space is initialized to COLOR_SPACE_SRGB before the early
return, when the color pipeline is active, fill_plane_color_attributes() sets
plane_info->color_space to COLOR_SPACE_SRGB.
Later, DC core's copy_surface_update_to_plane() unconditionally copies this
value to surface->color_space:
drivers/gpu/drm/amd/display/dc/core/dc.c:copy_surface_update_to_plane() {
...
if (srf_update->plane_info) {
surface->color_space =
srf_update->plane_info->color_space;
...
}
Can this clobber the fixed matrix value calculated during atomic check,
causing YUV-to-RGB conversion to be bypassed for YUV framebuffers?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260722134607.136293-1-harry.wentland@amd.com?part=9
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4 10/11] drm/amd/display: Set color_space to plane_infos
2026-07-22 13:45 [PATCH v4 00/11] YUV conversion colorop with amdgpu and VKMS Harry Wentland
` (8 preceding siblings ...)
2026-07-22 13:46 ` [PATCH v4 09/11] drm/amd/display: Check actual state during commit_tail Harry Wentland
@ 2026-07-22 13:46 ` Harry Wentland
2026-07-22 14:13 ` sashiko-bot
2026-07-22 13:46 ` [PATCH v4 11/11] drm/amd/display: Force GAMCOR for subsampled surfaces with PQ/Gamma22/HLG Harry Wentland
10 siblings, 1 reply; 19+ messages in thread
From: Harry Wentland @ 2026-07-22 13:46 UTC (permalink / raw)
To: dri-devel, amd-gfx; +Cc: Harry Wentland, Alex Hung
DC looks at plane_info structs.
For legacy drm_plane COLOR_ENCODING and COLOR_RANGE this was
done in fill_dc_plane_info_and_addr's call to
fill_plane_color_attributes.
Assisted-by: Claude:claude-sonnet-4.5
Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Reviewed-by: Alex Hung <alex.hung@amd.com>
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index b7d815997753..f5515ba64ca0 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4234,6 +4234,8 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_commit *state,
&bundle->flip_addrs[planes_count].address,
afb->tmz_surface);
+ bundle->plane_infos[planes_count].color_space = dc_plane->color_space;
+
drm_dbg_state(state->dev, "plane: id=%d dcc_en=%d\n",
new_plane_state->plane->index,
bundle->plane_infos[planes_count].dcc.enable);
--
2.55.0
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4 10/11] drm/amd/display: Set color_space to plane_infos
2026-07-22 13:46 ` [PATCH v4 10/11] drm/amd/display: Set color_space to plane_infos Harry Wentland
@ 2026-07-22 14:13 ` sashiko-bot
0 siblings, 0 replies; 19+ messages in thread
From: sashiko-bot @ 2026-07-22 14:13 UTC (permalink / raw)
To: Harry Wentland; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Changes to DRM plane color pipeline properties (drm_colorop) do not trigger a DC plane state reset, causing the hardware to use stale color space and colorop properties.
--
--- Patch [10]: [PATCH v4 10/11] drm/amd/display: Set color_space to plane_infos ---
commit d69c24f2afbe9ab20f934e3e3173c2c3d2c7b892
Author: Harry Wentland <harry.wentland@amd.com>
drm/amd/display: Set color_space to plane_infos
This commit assigns the color space from the dc_plane directly to the
plane_infos array in the update bundle during plane commits. This replaces
the legacy behavior that handled COLOR_ENCODING and COLOR_RANGE inside
fill_plane_color_attributes.
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 4f07fe5688262..779f98e8d2d7d 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -4288,6 +4288,8 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_commit *state,
> &bundle->flip_addrs[planes_count].address,
> afb->tmz_surface);
>
> + bundle->plane_infos[planes_count].color_space = dc_plane->color_space;
> +
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
Could this assignment use a stale color_space value?
When userspace dynamically updates a property on an active drm_colorop
pipeline, DRM core sets new_plane_state->color_mgmt_changed = true.
However, should_reset_plane() incorrectly omits checking
new_other_state->color_mgmt_changed or iterating over drm_colorop_state
property changes:
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:should_reset_plane() {
...
/* Colorspace changes. */
if (old_other_state->color_range != new_other_state->color_range ||
old_other_state->color_encoding != new_other_state->color_encoding)
return true;
...
}
Because should_reset_plane() returns false when only colorop pipeline
properties are modified, dm_update_plane_state() skips recreating the
dc_plane_state and bypasses amdgpu_dm_update_plane_color_mgmt().
As a result, stale color space parameters are passed to DC here. Should
should_reset_plane() be updated to properly check for color management changes
so the hardware color pipeline does not retain stale configuration values?
> drm_dbg_state(state->dev, "plane: id=%d dcc_en=%d\n",
> new_plane_state->plane->index,
> bundle->plane_infos[planes_count].dcc.enable);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260722134607.136293-1-harry.wentland@amd.com?part=10
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4 11/11] drm/amd/display: Force GAMCOR for subsampled surfaces with PQ/Gamma22/HLG
2026-07-22 13:45 [PATCH v4 00/11] YUV conversion colorop with amdgpu and VKMS Harry Wentland
` (9 preceding siblings ...)
2026-07-22 13:46 ` [PATCH v4 10/11] drm/amd/display: Set color_space to plane_infos Harry Wentland
@ 2026-07-22 13:46 ` Harry Wentland
10 siblings, 0 replies; 19+ messages in thread
From: Harry Wentland @ 2026-07-22 13:46 UTC (permalink / raw)
To: dri-devel, amd-gfx; +Cc: Harry Wentland, stable
The ROM early-return checks for PQ, Gamma 2.2, and HLG in
mod_color_calculate_degamma_params() do not consider the map_user_ramp
parameter. When map_user_ramp is true (indicating a subsampled surface
that requires post-scaler degamma via GAMCOR), the function still takes
the ROM path, programming PRE_DEGAM instead.
For subsampled formats the chroma channels must be upsampled by the
scaler before degamma is applied. PRE_DEGAM sits before the scaler in
the pipeline (CNVC domain), while GAMCOR sits after it (CM domain).
Using the ROM for subsampled surfaces means degamma is applied to
subsampled chroma before upsampling, producing incorrect results.
Fix by gating the dcn_arch ROM capability checks on !map_user_ramp,
forcing PQ/Gamma22/HLG through the GAMCOR LUT path when the surface
is subsampled.
Fixes: a8bf71649088 ("drm/amd/display: Internal refactoring to abstract color caps")
Cc: stable@vger.kernel.org
Assisted-by: Copilot:claude-opus-4.6
Signed-off-by: Harry Wentland <harry.wentland@amd.com>
---
drivers/gpu/drm/amd/display/modules/color/color_gamma.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/display/modules/color/color_gamma.c b/drivers/gpu/drm/amd/display/modules/color/color_gamma.c
index 03d88e78165d..5786ef5787ac 100644
--- a/drivers/gpu/drm/amd/display/modules/color/color_gamma.c
+++ b/drivers/gpu/drm/amd/display/modules/color/color_gamma.c
@@ -1696,7 +1696,8 @@ bool mod_color_calculate_degamma_params(struct dc_color_caps *dc_caps,
return true;
if (dc_caps != NULL &&
- dc_caps->dpp.dcn_arch == 1) {
+ dc_caps->dpp.dcn_arch == 1 &&
+ !map_user_ramp) {
if (input_tf->tf == TRANSFER_FUNCTION_PQ &&
dc_caps->dpp.dgam_rom_caps.pq == 1)
--
2.55.0
^ permalink raw reply related [flat|nested] 19+ messages in thread