Re: [PATCH v2 01/13] drm/colorop: Add DRM_COLOROP_FIXED_MATRIX

["Borah, Chaitanya Kumar" <chaitanya.kumar.borah@intel.com>]

On 4/23/2026 3:00 AM, Melissa Wen wrote:
> 
> 
> On 21/04/2026 11:57, Borah, Chaitanya Kumar wrote:
>>
>>
>> On 4/21/2026 6:40 PM, Melissa Wen wrote:
>>>
>>>
>>> On 08/04/2026 02:15, Chaitanya Kumar Borah wrote:
>>>> 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)
>>>>
>>>> Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/drm_atomic.c      |   4 ++
>>>>   drivers/gpu/drm/drm_atomic_uapi.c |   4 ++
>>>>   drivers/gpu/drm/drm_colorop.c     | 107 ++++++++++++++++++++++++++ 
>>>> ++++
>>>>   include/drm/drm_colorop.h         |  84 +++++++++++++++++++++++
>>>>   include/uapi/drm/drm_mode.h       |  12 ++++
>>>>   5 files changed, 211 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/ 
>>>> drm_atomic.c
>>>> index 41c57063f3b4..16a2183b11bf 100644
>>>> --- a/drivers/gpu/drm/drm_atomic.c
>>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>>> @@ -845,6 +845,10 @@ static void 
>>>> drm_atomic_colorop_print_state(struct drm_printer *p,
>>>> drm_get_colorop_lut3d_interpolation_name(colorop- 
>>>> >lut3d_interpolation));
>>>>           drm_printf(p, "\tdata blob id=%d\n", state->data ? state- 
>>>> >data->base.id : 0);
>>>>           break;
>>>> +    case DRM_COLOROP_FIXED_MATRIX:
>>>> +        drm_printf(p, "\tfixed_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 5bd5bf6661df..d69406b08a0f 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 566816e3c6f0..dd385e8dacbe 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_FULL_RGB_NC]  = "YCbCr 2020 Full to 
>>>> RGB NC",
>>>> +    [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,81 @@ 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 +605,13 @@ static void __drm_colorop_state_reset(struct 
>>>> drm_colorop_state *colorop_state,
>>>>                                 &val);
>>>>           colorop_state->curve_1d_type = val;
>>>>       }
>>>> +
>>>> +    if (colorop->fixed_matrix_type_property) {
>>>> + drm_object_property_get_default_value(&colorop->base,
>>>> + colorop->fixed_matrix_type_property,
>>>> +                              &val);
>>>> +        colorop_state->fixed_matrix_type = val;
>>>> +    }
>>>>   }
>>>>   /**
>>>> @@ -561,6 +652,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[] = {
>>>> @@ -617,6 +709,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 bd082854ca74..c13b4b045fff 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 Full to RGB NC"
>>> Nit: I think you mean "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_FULL_RGB_NC,
>>> ... and here ^ DRM_COLOROP_FM_YCBCR_NC_FULL_RGB
>>
>> Ack.
>>
>>>> +
>>>> +    /**
>>>> +     * @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,
>>> I didn't full understand how this is going to work.
>>> Looks like it's merging two properties (COLOR_ENCODING and 
>>> COLOR_RANGE) in a single colorop.
>>> But we can only select one enum value. So how to communicate, for 
>>> example, a "YCbCr 709 Limited to RGB" conversion?
>>> The driver will have to define a coloro pipeline with two fixed 
>>> matrix colorop in a row, one with only "Limited to Full" and another 
>>> with "YCbCr Full to RGB" ?
>>>
>>
>> It depends on what the HW supports. Let's say a fixed function HW 
>> block can itself support "YCbCr Limited to RGB" then we just need a 
>> single colorop that exposes this enum. These are being added in the 
>> series that Harry published.[1]
>>
>> In Intel's case, we have two separate blocks to do this. One that does 
>> the range correction (Limited to Full) and then a fixed function CSC 
>> block that does only "YCbCr Full to RGB" conversion. If they are 
>> adjacent in the pipeline, they can very well be represent by a single 
>> colorop described in the previous paragraph but we have a unique 
>> situation where there is a 1D LUT in between the two blocks.
>>
>> [YUV Limited to Full] -> [1D LUT] -> [YCbCr XXX Full to RGB] -> [1D LUT].
> 
> I see. Thanks for clarifying.
> 
> Apart from the DRM_COLOROP_FM_YCBCR_NC_FULL_RGB fix above-mentioned, 
> this patch LGTM.
> It was missing IGT tests, but I see Harry provided it in the series you 
> linked.
> 
> Reviewed-by: Melissa Wen <mwen@igalia.com>
> 

Thank you, Melissa, for the review.

Regarding the IGTs they are yet to be upstreamed with this version of 
the series as we are still working out how to use CRC matching for pass 
criteria. (As i915/xe does not have writeback hooked up yet)

==
Chaitanya

>>
>> ==
>> Chaitanya
>>
>> [1] https://lore.kernel.org/dri-devel/20260330153451.99472-3- 
>> harry.wentland@amd.com/
>>
>>> Melissa
>>>> +
>>>> +    /**
>>>> +     * @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_state */
>>>>       struct drm_atomic_state *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_duplicate_state(struct drm_colorop 
>>>> *colorop);
>>>> @@ -480,6 +563,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 a4bdc4bd11bc..dc4b8566fec8 100644
>>>> --- a/include/uapi/drm/drm_mode.h
>>>> +++ b/include/uapi/drm/drm_mode.h
>>>> @@ -971,6 +971,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,
>>>> +
>>>>   };
>>>>   /**
>>>
>>
>