[PATCH RFC v2 04/20] drm/crtc: Add COLOR_PIPELINE property

[Ariel D'Alessandro]

Louis,

On 9/19/25 9:43 AM, Louis Chauvet wrote:
> 
> 
> Le 18/09/2025 à 02:43, Nícolas F. R. A. Prado a écrit :
>> Add a COLOR_PIPELINE property to the CRTC to allow userspace to set a
>> post-blend color pipeline analogously to how pre-blend color pipelines
>> are set on planes.
>>
>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
>> ---
>>   drivers/gpu/drm/drm_atomic_uapi.c | 49 +++++++++++++++++++++++++++++ 
>> ++++++----
>>   drivers/gpu/drm/drm_crtc.c        | 33 ++++++++++++++++++++++++++
>>   include/drm/drm_atomic_uapi.h     |  2 ++
>>   include/drm/drm_crtc.h            | 11 +++++++++
>>   4 files changed, 91 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/ 
>> drm_atomic_uapi.c
>> index 
>> b7cc6945864274bedd21dd5b73494f9aae216888..063c142fd9b656e228cfc660d005a3fbb4640d32 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -287,6 +287,33 @@ drm_atomic_set_colorop_for_plane(struct 
>> drm_plane_state *plane_state,
>>   }
>>   EXPORT_SYMBOL(drm_atomic_set_colorop_for_plane);
>> +/**
>> + * drm_atomic_set_colorop_for_crtc - set colorop for crtc
>> + * @crtc_state: atomic state object for the crtc
>> + * @colorop: colorop to use for the crtc
>> + *
>> + * Helper function to select the color pipeline on a crtc by setting
>> + * it to the first drm_colorop element of the pipeline.
>> + */
>> +void
>> +drm_atomic_set_colorop_for_crtc(struct drm_crtc_state *crtc_state,
>> +                 struct drm_colorop *colorop)
>> +{
>> +    struct drm_crtc *crtc = crtc_state->crtc;
>> +
>> +    if (colorop)
>> +        drm_dbg_atomic(crtc->dev,
>> +                   "Set [COLOROP:%d] for [CRTC:%d:%s] state %p\n",
>> +                   colorop->base.id, crtc->base.id, crtc->name,
>> +                   crtc_state);
>> +    else
>> +        drm_dbg_atomic(crtc->dev,
>> +                   "Set [NOCOLOROP] for [CRTC:%d:%s] state %p\n",
>> +                   crtc->base.id, crtc->name, crtc_state);
>> +
>> +    crtc_state->color_pipeline = colorop;
>> +}
>> +EXPORT_SYMBOL(drm_atomic_set_colorop_for_crtc);
>>   /**
>>    * drm_atomic_set_crtc_for_connector - set CRTC for connector
>> @@ -396,8 +423,8 @@ static s32 __user 
>> *get_out_fence_for_connector(struct drm_atomic_state *state,
>>   }
>>   static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>> -        struct drm_crtc_state *state, struct drm_property *property,
>> -        uint64_t val)
>> +        struct drm_crtc_state *state, struct drm_file *file_priv,
>> +        struct drm_property *property, uint64_t val)
>>   {
>>       struct drm_device *dev = crtc->dev;
>>       struct drm_mode_config *config = &dev->mode_config;
>> @@ -406,7 +433,17 @@ static int drm_atomic_crtc_set_property(struct 
>> drm_crtc *crtc,
>>       if (property == config->prop_active)
>>           state->active = val;
>> -    else if (property == config->prop_mode_id) {
>> +    else if (property == crtc->color_pipeline_property) {
>> +        /* find DRM colorop object */
>> +        struct drm_colorop *colorop = NULL;
>> +
>> +        colorop = drm_colorop_find(dev, file_priv, val);
>> +
>> +        if (val && !colorop)
>> +            return -EACCES;
>> +
>> +        drm_atomic_set_colorop_for_crtc(state, colorop);
> 
> I don't know if this is needed, but you added a warning/early return for 
> ctm/degamma_lut if file_priv->post_blend_color_pipeline is true.
> 
> Does it make sense to add this?
> 
>      if (!file_priv->post_blend_color_pipeline) {
>              drm_dbg_atomic(dev,
>                  "Setting COLOR_PIPELINE property not permitted without 
> DRM_CLIENT_CAP_POST_BLEND_COLOR_PIPELINE client cap\n");
>              return -EINVAL;
>          }

Indeed that makes sense. Will add in v3.

Thanks,

-- 
Ariel D'Alessandro
Software Engineer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK 
Registered in England & Wales, no. 5513718