Re: [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 <nfrapr...@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