Re: [PATCH v3 01/11] drm/colorop: Add DRM_COLOROP_FIXED_MATRIX

[Harry Wentland <harry.wentland@amd.com>]

On 2026-07-03 06:52, Pekka Paalanen wrote:
> On Tue, 23 Jun 2026 12:48:02 -0400
> Harry Wentland <harry.wentland@amd.com> wrote:
> 
>> 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.
>>
>> v2:
>>  - Naming changes (Pekka)
>>
>> v3:
>>  - Fix NC matrix enum name and string (Melissa)
>>  - Rebase
>>
>> Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
>> Reviewed-by: Melissa Wen <mwen@igalia.com>
>> Reviewed-by: Harry Wentland <harry.wentland@amd.com>
>> ---
>>  drivers/gpu/drm/drm_atomic.c      |   4 ++
>>  drivers/gpu/drm/drm_atomic_uapi.c |   4 ++
>>  drivers/gpu/drm/drm_colorop.c     | 106 ++++++++++++++++++++++++++++++
>>  include/drm/drm_colorop.h         |  84 +++++++++++++++++++++++
>>  include/uapi/drm/drm_mode.h       |  12 ++++
>>  5 files changed, 210 insertions(+)
>>
> 
> Hi,
> 
> the UAPI looks good.
> 
> Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> 
> It's a little bit unfortunate that the colorop UAPI documentation is
> embedded into kernel-private enum types' documentation.
> 

Agreed. This goes beyond this patchset. I'll have a look at a separate
series to see if we can fix that.

> Is RGB always full-range? The documentation could be more explicit
> about that. Maybe the documentation should spell out the matrix
> contents just to be clear? There are more than one form of limited
> range IIRC (HDMI, JPEG, SDI). Or maybe JPEG was a different type of
> full-range?
> 

It's always full-range. Agreed about spelling out the matrix contents.

>> +	 * enum string "RGB709 to RGB2020"
>> +	 *
>> +	 * Selects the fixed-function CSC preset that converts RGB
>> +	 * (BT.709) colorimetry to RGB (BT.2020).
> 
> Why is this called a "CSC preset" rather than a matrix? This confuses
> me.
> 

Will fix that up.

I was taking Chaitanya's patch as-is and adding my changes in Patch 2
but with all these changes I'll end up squashing the two commits
together.

> Was there a conclusion from discussions around compatible pixel formats
> for a specific choice of a fixed matrix? How will userspace discover
> that, or will it be based on documentation and enforced as a common
> requirement over all drivers?
> 
> ISTR some hardware requiring an YUV pixel format to be able to use the
> YCbCr conversion matrices, or vice versa. Otherwise there is no reason
> to have that limitation.
> 

We talked about this at the Hackfest (offline most likely) and the
consensus is that there's no reason to limit this to certain pixel formats.
We should treat it as a simple matrix that operates on the 3 input channels.
If userspace decides to select a CSC matrix for an RGB buffer it's assumed
that that's intentional and userspace knows what it's doing.

Harry

> 
> Thanks,
> pq
> 
> 
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 3af1b9cc9a06..ced591c4a0bd 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(colorop->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 c7f80d90794c..cee4550ffdbd 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -761,6 +761,8 @@ static int drm_atomic_colorop_set_property(struct drm_colorop *colorop,
>>  	} else if (property == colorop->data_property) {
>>  		return drm_atomic_color_set_data_property(colorop, state,
>>  							  property, val);
>> +	} else if (property == colorop->fixed_matrix_type_property) {
>> +		state->fixed_matrix_type = val;
>>  	} else {
>>  		drm_dbg_atomic(colorop->dev,
>>  			       "[COLOROP:%d:%d] unknown property [PROP:%d:%s]\n",
>> @@ -793,6 +795,8 @@ drm_atomic_colorop_get_property(struct drm_colorop *colorop,
>>  		*val = colorop->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 c0eecde8c176..c11c3012fcc5 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,14 @@ 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_YCBCR709_FULL_RGB] = "YCbCr 709 Full to RGB",
>> +	[DRM_COLOROP_FM_YCBCR2020_NC_FULL_RGB] = "YCbCr 2020 NC Full 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,
>> @@ -455,6 +464,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)
>>  {
>> @@ -521,6 +604,13 @@ static void __drm_colorop_state_init(struct drm_colorop_state *colorop_state,
>>  							   &val))
>>  			colorop_state->curve_1d_type = 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;
>> +	}
>>  }
>>  
>>  /**
>> @@ -584,6 +674,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[] = {
>> @@ -640,6 +731,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 b4b9e4f558ab..88933d5b4d8b 100644
>> --- a/include/drm/drm_colorop.h
>> +++ b/include/drm/drm_colorop.h
>> @@ -134,6 +134,71 @@ 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"
>> +	 *
>> +	 * This selects the matrix that converts full range YCbCr into RGB
>> +	 * according to the BT.601 coefficients.
>> +	 */
>> +	DRM_COLOROP_FM_YCBCR601_FULL_RGB,
>> +
>> +	/**
>> +	 * @DRM_COLOROP_FM_YCBCR709_FULL_RGB:
>> +	 *
>> +	 * enum string "YCbCr 709 Full to RGB"
>> +	 *
>> +	 * This selects the matrix that converts full range YCbCr into RGB
>> +	 * according to the BT.709 coefficients.
>> +	 */
>> +	DRM_COLOROP_FM_YCBCR709_FULL_RGB,
>> +
>> +	/**
>> +	 * @DRM_COLOROP_FM_YCBCR2020_NC_FULL_RGB:
>> +	 *
>> +	 * enum string "YCbCr 2020 NC Full to RGB"
>> +	 *
>> +	 * This selects the matrix that converts full range YCbCr into RGB
>> +	 * according to the BT.2020 non-constant luminance coefficients.
>> +	 */
>> +	DRM_COLOROP_FM_YCBCR2020_NC_FULL_RGB,
>> +
>> +	/**
>> +	 * @DRM_COLOROP_FM_YCBCR_LIMITED_FULL:
>> +	 *
>> +	 * enum string "YCbCr limited to full"
>> +	 *
>> +	 * This selects the matrix that converts limited range YCbCr into
>> +	 * full range YCbCr. Though not strictly a matrix operation but
>> +	 * can be represented as one.
>> +	 */
>> +	DRM_COLOROP_FM_YCBCR_LIMITED_FULL,
>> +
>> +	/**
>> +	 * @DRM_COLOROP_FM_RGB709_RGB2020:
>> +	 *
>> +	 * enum string "RGB709 to RGB2020"
>> +	 *
>> +	 * Selects the fixed-function CSC preset that converts RGB
>> +	 * (BT.709) colorimetry to RGB (BT.2020).
>> +	 */
>> +	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
>>   */
>> @@ -183,6 +248,13 @@ struct drm_colorop_state {
>>  	 */
>>  	struct drm_property_blob *data;
>>  
>> +	/**
>> +	 * @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;
>>  };
>> @@ -368,6 +440,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:
>>  	 *
>> @@ -424,6 +503,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);
>> @@ -482,6 +565,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 381a3e857d4e..bff6dba08d50 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -954,6 +954,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,
>> +
>>  };
>>  
>>  /**
>