Re: [PATCH RFC 1/5] drm: Support post-blend color pipeline API

["Nícolas F. R. A. Prado" <nfraprado@collabora.com>]

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 :
> > 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 <nfraprado@collabora.com>
> > ---
> >   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-
> > >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-
> > >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.

> 
> >   	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-
> > >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.

> > 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/
), 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/
) 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
) 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.

> 
> > + * - 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