On Fri, 2025-09-05 at 19:55 +0200, Louis Chauvet wrote:

Le 22/08/2025 à 20:36, Nícolas F. R. A. Prado a écrit :

quoted

Introduce support for a post-blend color pipeline API analogous to
the
pre-blend color pipeline API. While the pre-blend color pipeline
was
configured through a COLOR_PIPELINE property attached to a
drm_plane,
the post-blend color pipeline is configured through a
COLOR_PIPELINE
property on the drm_crtc.

Since colorops can now be attached to either a drm_plane or a
drm_crtc,
rework the helpers to account for both cases.

Also introduce a new cap, DRM_CLIENT_CAP_POST_BLEND_COLOR_PIPELINE,
to
enable support for post-blend color pipelines, and prevent the now
legacy GAMMA_LUT, DEGAMMA_LUT, GAMMA_LUT_SIZE and CTM properties
from
being exposed.

Signed-off-by: Nícolas F. R. A. Prado <redacted>
---
  drivers/gpu/drm/drm_atomic.c        |  32 ++++++--
  drivers/gpu/drm/drm_atomic_uapi.c   |  50 ++++++++++++-
  drivers/gpu/drm/drm_colorop.c       | 144
+++++++++++++++++++++++++++++-------
  drivers/gpu/drm/drm_connector.c     |   1 +
  drivers/gpu/drm/drm_crtc.c          |  77 +++++++++++++++++++
  drivers/gpu/drm/drm_crtc_internal.h |   6 ++
  drivers/gpu/drm/drm_ioctl.c         |   7 ++
  drivers/gpu/drm/drm_mode_object.c   |  20 +++++
  drivers/gpu/drm/drm_plane.c         |  36 ++-------
  include/drm/drm_atomic.h            |  20 +++++
  include/drm/drm_atomic_uapi.h       |   2 +
  include/drm/drm_colorop.h           |  16 +++-
  include/drm/drm_crtc.h              |  19 +++++
  include/drm/drm_file.h              |   7 ++
  include/uapi/drm/drm.h              |  16 ++++
  15 files changed, 383 insertions(+), 70 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c

b/drivers/gpu/drm/drm_atomic.c
index
3ab32fe7fe1cbf9057c3763d979638dce013d82b..558d389d59d9a44d3cd1048ed
365848f62b4d62d 100644

--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c

@@ -472,6 +472,8 @@ static void drm_atomic_crtc_print_state(struct

drm_printer *p,
  	drm_printf(p, "\tplane_mask=%x\n", state->plane_mask);
  	drm_printf(p, "\tconnector_mask=%x\n", state-

quoted

connector_mask);

  	drm_printf(p, "\tencoder_mask=%x\n", state->encoder_mask);
+	drm_printf(p, "\tcolor-pipeline=%d\n",
+		   state->color_pipeline ? state->color_pipeline-

quoted

base.id : 0);

This could be in a separate patch / suggested to the initial series.

Right, I'll make sure to split the changes in this commit further for
v2.

quoted

  	drm_printf(p, "\tmode: " DRM_MODE_FMT "\n",
DRM_MODE_ARG(&state->mode));
  
  	if (crtc->funcs->atomic_print_state)

@@ -617,9 +619,15 @@ drm_atomic_get_colorop_state(struct

drm_atomic_state *state,
  	if (colorop_state)
  		return colorop_state;
  
-	ret = drm_modeset_lock(&colorop->plane->mutex, state-

quoted

acquire_ctx);

-	if (ret)
-		return ERR_PTR(ret);
+	if (colorop->plane) {
+		ret = drm_modeset_lock(&colorop->plane->mutex,
state->acquire_ctx);
+		if (ret)
+			return ERR_PTR(ret);
+	} else {
+		ret = drm_modeset_lock(&colorop->crtc->mutex,
state->acquire_ctx);
+		if (ret)
+			return ERR_PTR(ret);
+	}

Two suggestions here:

Maybe you can create `colorop_modeset_lock/unlock` helpers to avoid
code 
repetition.

Can you also change it to

	if (colorop->plane)
		...
	else if (colorop->crtc)
		...
	else
		drm_err("Dangling colorop, it must be attached to a
plane or a CRTC")
		return ERR_PTR

?

This way it will avoid issues if someone add support to attach
colorop 
to other drm parts and forgot to update locking in some places.

Yes, both suggestions sound good, I'll apply them for v2.

quoted

diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h

index
27cc159c1d275c7a7fe057840ef792f30a582bb7..1191b142ebaa5478376308f16
9f9ce8591580d9d 100644

--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h

@@ -921,6 +921,22 @@ struct drm_get_cap {

   */
  #define DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE	7
  
+/**
+ * DRM_CLIENT_CAP_POST_BLEND_COLOR_PIPELINE
+ *
+ * If set to 1 the DRM core will allow setting the COLOR_PIPELINE
+ * property on a &drm_crtc, as well as drm_colorop properties.
+ *
+ * Setting of these crtc properties will be rejected when this
client
+ * cap is set:

I don't know enough the uAPI of DRM, but if I understand your patch 
correctly, it will not reject GAMMA_LUT/DEGAMMA_LUT/CTM, only unlist 
them from the get_properties syscall. Did I overlooked something?

You're right. This was originally based on v10 of the pre-blend series
(https://lore.kernel.org/dri-devel/20250617041746.2884343-12-alex.hung@amd.com/ (local)
), which did in fact return an error when the properties were set with
the cap enabled, but v11
(https://lore.kernel.org/all/20250815035047.3319284-12-alex.hung@amd.com/ (local)
) switched to just unlisting the properties, so I also updated the code
here accordingly for this RFC, but forgot to update this text, and
apparently so did the original series.

From the discussion in another thread
(https://lore.kernel.org/dri-devel/20250822-mtk-post-blend-color-pipeline-v1-0-a9446d4aca82@collabora.com/T/#m1e8ba77ef54453b26ae70e1f1699ed17afec91e3 (local)
) we might make these properties read-only, and in which case we'll
have to make them listed but error when set again, so I'll wait for
that discussion to be resolved before updating this and forwarding that
feedback to the pre-blend series. But thanks for pointing out the
inconsistency anyway.

quoted

+ * - GAMMA_LUT
+ * - DEGAMMA_LUT
+ * - CTM
+ *
+ * The client must enable &DRM_CLIENT_CAP_ATOMIC first.
+ */
+#define DRM_CLIENT_CAP_POST_BLEND_COLOR_PIPELINE	8
+
  /* DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
  struct drm_set_client_cap {
  	__u64 capability;

-- 
Thanks,

Nícolas