Re: [PATCH v5 1/6] drm/atomic: only add colorop state from active color pipeline

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

On 5/7/2026 12:53 AM, Melissa Wen wrote:
> Instead of adding colorop state of all colorops of a given plane, only
> get those from an active color pipeline of this plane.
> 
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> 
> ---
> v5: fix kernel-doc for plane_state (kernel bot)
> ---
>   drivers/gpu/drm/drm_atomic.c        | 39 ++++++++++++++---------------
>   drivers/gpu/drm/drm_atomic_helper.c |  9 +++----
>   include/drm/drm_atomic.h            |  2 +-
>   3 files changed, 23 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 54bab7e9f935..8eb519673fc5 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1591,26 +1591,25 @@ drm_atomic_add_affected_planes(struct drm_atomic_state *state,
>   		if (IS_ERR(plane_state))
>   			return PTR_ERR(plane_state);
>   
> -		if (plane_state->color_pipeline) {
> -			ret = drm_atomic_add_affected_colorops(state, plane);
> -			if (ret)
> -				return ret;
> -		}
> +		ret = drm_atomic_add_affected_colorops(plane_state, plane);
> +		if (ret)
> +			return ret;
>   	}
>   	return 0;
>   }
>   EXPORT_SYMBOL(drm_atomic_add_affected_planes);
>   
>   /**
> - * drm_atomic_add_affected_colorops - add colorops for plane
> - * @state: atomic state
> + * drm_atomic_add_affected_colorops - add active colorops for plane
> + * @plane_state: DRM plane state
>    * @plane: DRM plane
>    *
>    * This function walks the current configuration and adds all colorops
> - * currently used by @plane to the atomic configuration @state. This is useful
> - * when an atomic commit also needs to check all currently enabled colorop on
> - * @plane, e.g. when changing the mode. It's also useful when re-enabling a plane
> - * to avoid special code to force-enable all colorops.
> + * currently used by an active color pipeline set for a @plane to the atomic
> + * configuration @state.

nit: The documention sounds a bit wonky to me.

Also s/@state/@plane_state

This is useful when an atomic commit also needs to
> + * check all currently enabled colorop on @plane, e.g. when changing the mode.
> + * It's also useful when re-enabling a plane to avoid special code to
> + * force-enable all colorops.
>    *
>    * Since acquiring a colorop state will always also acquire the w/w mutex of the
>    * current plane for that colorop (if there is any) adding all the colorop states for
> @@ -1622,23 +1621,23 @@ EXPORT_SYMBOL(drm_atomic_add_affected_planes);
>    * sequence must be restarted. All other errors are fatal.
>    */
>   int
> -drm_atomic_add_affected_colorops(struct drm_atomic_state *state,
> +drm_atomic_add_affected_colorops(struct drm_plane_state *plane_state,
>   				 struct drm_plane *plane)
>   {
>   	struct drm_colorop *colorop;
>   	struct drm_colorop_state *colorop_state;
>   
> -	WARN_ON(!drm_atomic_get_new_plane_state(state, plane));
> +	if (!plane_state || !plane_state->color_pipeline)
> +		return 0;
>   
>   	drm_dbg_atomic(plane->dev,
> -		       "Adding all current colorops for [PLANE:%d:%s] to %p\n",
> -		       plane->base.id, plane->name, state);
> +		       "Adding all current active colorops for [PLANE:%d:%s] to %p\n",
> +		       plane->base.id, plane->name, plane_state->state);
>   
> -	drm_for_each_colorop(colorop, plane->dev) {
> -		if (colorop->plane != plane)
> -			continue;
> -
> -		colorop_state = drm_atomic_get_colorop_state(state, colorop);
> +	for (colorop = plane_state->color_pipeline;
> +	     colorop;
> +	     colorop = colorop->next) {
> +		colorop_state = drm_atomic_get_colorop_state(plane_state->state, colorop);
>   		if (IS_ERR(colorop_state))
>   			return PTR_ERR(colorop_state);
>   	}
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index a768398a1884..c8dadbf5c319 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3752,12 +3752,9 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev,
>   			goto free;
>   		}
>   
> -		if (plane_state->color_pipeline) {
> -			err = drm_atomic_add_affected_colorops(state, plane);
> -			if (err)
> -				goto free;
> -		}
> -
> +		err = drm_atomic_add_affected_colorops(plane_state, plane);
> +		if (err)
> +			goto free;
>   	}
>   
>   	drm_connector_list_iter_begin(dev, &conn_iter);
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 8883290cd014..8916923f32b8 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -919,7 +919,7 @@ int __must_check
>   drm_atomic_add_affected_planes(struct drm_atomic_state *state,
>   			       struct drm_crtc *crtc);
>   int __must_check
> -drm_atomic_add_affected_colorops(struct drm_atomic_state *state,
> +drm_atomic_add_affected_colorops(struct drm_plane_state *state,

s/state/plane_state

Otherwise, LGTM. (needs a rebase though)

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

>   				 struct drm_plane *plane);
>   
>   int __must_check drm_atomic_check_only(struct drm_atomic_state *state);