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

[Melissa Wen <mwen@igalia.com>]

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>

>
> ==
> 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,
>>> +
>>>   };
>>>   /**
>>
>