Re: [PATCH v5 5/6] drm/atomic: track individual colorop updates

["Borah, Chaitanya Kumar" <chaitanya.kumar.borah@intel.com>]

On 5/7/2026 12:53 AM, Melissa Wen wrote:
> As we do for CRTC color mgmt properties, use color_mgmt_changed flag to
> track any value changes in the color pipeline of a given plane, so that
> drivers can update color blocks as soon as plane color pipeline or
> individual colorop values change.
> 
> Reviewed-by: Harry Wentland <harry.wentland@amd.com> #v1
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> --
> 
> v3:
> - track lut1d/3d_interpolation changes (Chaitanya)
> 
> v2: add linux types to provide bool for MSM driver (kernel bot)
> ---
>   drivers/gpu/drm/drm_atomic_uapi.c | 55 +++++++++++++++++++++++--------
>   include/drm/drm_atomic_uapi.h     |  4 ++-
>   2 files changed, 45 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 25fe94410af7..2bac16c9855a 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -265,13 +265,19 @@ EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
>    *
>    * Helper function to select the color pipeline on a plane by setting
>    * it to the first drm_colorop element of the pipeline.
> + *
> + * Return: true if plane color pipeline value changed, false otherwise.
>    */
> -void
> +bool
>   drm_atomic_set_colorop_for_plane(struct drm_plane_state *plane_state,
>   				 struct drm_colorop *colorop)
>   {
>   	struct drm_plane *plane = plane_state->plane;
>   
> +	/* Color pipeline didn't change */
> +	if (plane_state->color_pipeline == colorop)
> +		return false;
> +
>   	if (colorop)
>   		drm_dbg_atomic(plane->dev,
>   			       "Set [COLOROP:%d] for [PLANE:%d:%s] state %p\n",
> @@ -283,6 +289,8 @@ drm_atomic_set_colorop_for_plane(struct drm_plane_state *plane_state,
>   			       plane->base.id, plane->name, plane_state);
>   
>   	plane_state->color_pipeline = colorop;
> +
> +	return true;
>   }
>   EXPORT_SYMBOL(drm_atomic_set_colorop_for_plane);
>   
> @@ -604,7 +612,7 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>   		if (val && !colorop)
>   			return -EACCES;
>   
> -		drm_atomic_set_colorop_for_plane(state, colorop);
> +		state->color_mgmt_changed |= drm_atomic_set_colorop_for_plane(state, colorop);
>   	} else if (property == config->prop_fb_damage_clips) {
>   		ret = drm_property_replace_blob_from_id(dev,
>   					&state->fb_damage_clips,
> @@ -713,11 +721,11 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>   static int drm_atomic_color_set_data_property(struct drm_colorop *colorop,
>   					      struct drm_colorop_state *state,
>   					      struct drm_property *property,
> -					      uint64_t val)
> +					      uint64_t val,
> +					      bool *replaced)
>   {
>   	ssize_t elem_size = -1;
>   	ssize_t size = -1;
> -	bool replaced = false;
>   
>   	switch (colorop->type) {
>   	case DRM_COLOROP_1D_LUT:
> @@ -739,28 +747,45 @@ static int drm_atomic_color_set_data_property(struct drm_colorop *colorop,
>   						 &state->data,
>   						 val,
>   						 -1, size, elem_size,
> -						 &replaced);
> +						 replaced);
>   }
>   
>   static int drm_atomic_colorop_set_property(struct drm_colorop *colorop,
>   					   struct drm_colorop_state *state,
>   					   struct drm_file *file_priv,
>   					   struct drm_property *property,
> -					   uint64_t val)
> +					   uint64_t val,
> +					   bool *replaced)
>   {
>   	if (property == colorop->bypass_property) {
> -		state->bypass = val;
> +		if (state->bypass != val) {
> +			state->bypass = val;
> +			*replaced = true;
> +		}
>   	} else if (property == colorop->lut1d_interpolation_property) {
> -		state->lut1d_interpolation = val;
> +		if (state->lut1d_interpolation != val) {
> +			state->lut1d_interpolation = val;
> +			*replaced = true;
> +		}
>   	} else if (property == colorop->curve_1d_type_property) {
> -		state->curve_1d_type = val;
> +		if (state->curve_1d_type != val) {
> +			state->curve_1d_type = val;
> +			*replaced = true;
> +		}
>   	} else if (property == colorop->multiplier_property) {
> -		state->multiplier = val;
> +		if (state->multiplier != val) {
> +			state->multiplier = val;
> +			*replaced = true;
> +		}
>   	} else if (property == colorop->lut3d_interpolation_property) {
> -		state->lut3d_interpolation = val;
> +		if (state->lut3d_interpolation != val) {
> +			state->lut3d_interpolation = val;
> +			*replaced = true;
> +		}
>   	} else if (property == colorop->data_property) {
>   		return drm_atomic_color_set_data_property(colorop, state,
> -							  property, val);
> +							  property, val,
> +							  replaced);
>   	} else {
>   		drm_dbg_atomic(colorop->dev,
>   			       "[COLOROP:%d:%d] unknown property [PROP:%d:%s]\n",
> @@ -1278,6 +1303,7 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>   		struct drm_colorop *active_colorop, *colorop = obj_to_colorop(obj);
>   		struct drm_colorop_state *colorop_state = NULL;
>   		struct drm_plane_state *plane_state;
> +		bool replaced = false;
>   
>   		plane_state = drm_atomic_get_plane_state(state, colorop->plane);
>   		if (IS_ERR(plane_state)) {
> @@ -1308,7 +1334,10 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
>   		}
>   
>   		ret = drm_atomic_colorop_set_property(colorop, colorop_state,
> -						      file_priv, prop, prop_value);
> +						      file_priv, prop, prop_value,
> +						      &replaced);
> +		if (!ret && replaced)
> +			plane_state->color_mgmt_changed = true;

Nit: For consistency with the other color_mgmt_changed updates in this 
file and to make the intent explicit, perhaps change it to 
plane_state->color_mgmt_changed |= replaced;

Otherwise, LGTM

Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>


>   		break;
>   	}
>   	default:
> diff --git a/include/drm/drm_atomic_uapi.h b/include/drm/drm_atomic_uapi.h
> index 436315523326..4e7e78f711e2 100644
> --- a/include/drm/drm_atomic_uapi.h
> +++ b/include/drm/drm_atomic_uapi.h
> @@ -29,6 +29,8 @@
>   #ifndef DRM_ATOMIC_UAPI_H_
>   #define DRM_ATOMIC_UAPI_H_
>   
> +#include <linux/types.h>
> +
>   struct drm_crtc_state;
>   struct drm_display_mode;
>   struct drm_property_blob;
> @@ -50,7 +52,7 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
>   			      struct drm_crtc *crtc);
>   void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
>   				 struct drm_framebuffer *fb);
> -void drm_atomic_set_colorop_for_plane(struct drm_plane_state *plane_state,
> +bool drm_atomic_set_colorop_for_plane(struct drm_plane_state *plane_state,
>   				      struct drm_colorop *colorop);
>   int __must_check
>   drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,