Re: [PATCH v4 05/11] drm/amd/display: only check colorops of an active color pipeline

[Harry Wentland <harry.wentland@amd.com>]

On 2026-08-11 12:45, Melissa Wen wrote:
> dm_plane_color_pipeline_active() iterates every colorop state in the
> atomic commit, so colorops of a pipeline that userspace deactivated via
> plane COLOR_PIPELINE are still taken into account, even though their
> BYPASS property is irrelevant once the pipeline is off. Walk the color
> pipeline of the plane state under evaluation instead, falling back to
> the committed colorop state when a colorop isn't in the atomic commit.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Fixes: d3a549f4df78 ("drm/amd/display: Use overlay cursor when color pipeline is active")
> Acked-by: Harry Wentland <harry.wentland@amd.com>
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> 

Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

> ---
> 
> v4:
> - drop the ternary and add just a warn_on since both current callers
>    iterate planes already in the atomic state. (John H.)
> - explain the reason to use commited colorop (John H.)
> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 33 ++++++++++++++-----
>   1 file changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index d0e612371c8f..384541b9ac9c 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6551,9 +6551,9 @@ static int add_affected_mst_dsc_crtcs(struct drm_atomic_commit *state, struct dr
>    * @use_old: if true, inspect the old colorop states; otherwise the new ones
>    *
>    * A color pipeline may be selected (color_pipeline != NULL) but still is
> - * inactive if every colorop in the chain is bypassed.  Only return
> - * true when at least one colorop has bypass == false, meaning the cursor
> - * would be subjected to the transformation in native mode.
> + * inactive if every colorop in the chain is bypassed. Only return true when at
> + * least one colorop has bypass == false, meaning the cursor would be subjected
> + * to the transformation in native mode.
>    *
>    * Return: true if the pipeline modifies pixels, false otherwise.
>    */
> @@ -6561,18 +6561,33 @@ static bool dm_plane_color_pipeline_active(struct drm_atomic_commit *state,
>   					   struct drm_plane *plane,
>   					   bool use_old)
>   {
> +	struct drm_plane_state *plane_state = use_old ?
> +					      drm_atomic_get_old_plane_state(state, plane) :
> +					      drm_atomic_get_new_plane_state(state, plane);
>   	struct drm_colorop *colorop;
> -	struct drm_colorop_state *old_colorop_state, *new_colorop_state;
> -	int i;
> +	struct drm_colorop_state *cstate;
>   
> -	for_each_oldnew_colorop_in_state(state, colorop, old_colorop_state, new_colorop_state, i) {
> -		struct drm_colorop_state *cstate = use_old ? old_colorop_state : new_colorop_state;
> +	if (drm_WARN_ON(plane->dev, !plane_state))
> +		return false;
>   
> -		if (cstate->colorop->plane != plane)
> -			continue;
> +	/*
> +	 * A commit may change only some colorops of a pipeline, and only those
> +	 * have old and new states here. Telling whether the pipeline modifies
> +	 * pixels requires every colorop of the selected pipeline, so fall back
> +	 * to the committed state of the untouched ones; it's both their old
> +	 * and new state.
> +	 */
> +	drm_for_each_colorop_in_pipeline(colorop, plane_state->color_pipeline) {
> +		cstate = use_old ?
> +			 drm_atomic_get_old_colorop_state(state, colorop) :
> +			 drm_atomic_get_new_colorop_state(state, colorop);
> +
> +		if (!cstate)
> +			cstate = colorop->state;
>   		if (!cstate->bypass)
>   			return true;
>   	}
> +
>   	return false;
>   }
>