Re: [PATCH 1/3] drm/atomic: only add states of active or transient active colorops

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 26 May 2026, Alex Hung <alex.hung@amd.com> wrote:
> On 5/26/26 08:17, Melissa Wen wrote:
>> Only consider affected colorop states those that are part of an active
>> color pipeline or a pipeline that is about to be activated or
>> deactivated in the same atomic commit, i.e., colorop is in the chain of
>> old/new plane color pipeline property. To cover color_pipeline
>> deactivation, remove the condition for plane_state->color_pipeline.
>> 
>> Signed-off-by: Melissa Wen <mwen@igalia.com>
>> ---
>>   drivers/gpu/drm/drm_atomic.c | 67 +++++++++++++++++++++++++++++++-----
>>   1 file changed, 58 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 170de30c28ae..4fb3a23e862a 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -812,6 +812,59 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state,
>>   	return 0;
>>   }
>>   
>> +/*
>> + * This function walks old and new plane state color pipelines and adds all
>> + * colorops in use by @plane to the atomic configuration @state. This is useful
>> + * when an atomic commit needs to check all currently enabled or about to be
>> + * enabled colorop on @plane, e.g. when changing the mode. This also avoids
>> + * including colorop states that are not part of the atomic state.
>> + *
>> + * Returns:
>> + * 0 on success or can fail with -EDEADLK or -ENOMEM. When the error is EDEADLK
>> + * then the w/w mutex code has detected a deadlock and the entire atomic
>> + * sequence must be restarted. All other errors are fatal.
>> + */
>> +static int
>> +drm_atomic_add_pipeline_colorops(struct drm_atomic_commit *state,
>> +				 struct drm_plane *plane)
>> +{
>> +	struct drm_colorop *colorop;
>> +	struct drm_colorop_state *colorop_state;
>> +	struct drm_plane_state *new_plane_state, *old_plane_state;
>> +
>> +	new_plane_state = drm_atomic_get_new_plane_state(state, plane);
>> +	old_plane_state = drm_atomic_get_old_plane_state(state, plane);
>> +
>> +	if (WARN_ON(!new_plane_state || !old_plane_state))
>> +		return -EINVAL;
>> +
>> +	drm_dbg_atomic(plane->dev,
>> +		       "Adding old+new pipeline colorops for [PLANE:%d:%s]\n",
>> +		       plane->base.id, plane->name);
>> +
>> +	for (colorop = new_plane_state->color_pipeline;
>> +	     colorop;
>> +	     colorop = colorop->next) {
>
> This for-loop is used 5 times in this patchset. How about a macro in 
> drm_colorop.h?
>
> #define drm_for_each_colorop_in_pipeline(colorop, pipeline) \
>      for ((colorop) = (pipeline); (colorop); (colorop) = (colorop)->next)

Is there a reason struct drm_colorop reinvents lists and doesn't have
struct list_head node?

BR,
Jani.

>
>> +		colorop_state = drm_atomic_get_colorop_state(state, colorop);
>> +		if (IS_ERR(colorop_state))
>> +			return PTR_ERR(colorop_state);
>> +	}
>> +
>> +	/* Same color pipeline as new; no point walking old. */
>> +	if (new_plane_state->color_pipeline == old_plane_state->color_pipeline)
>> +		return 0;
>> +
>> +	for (colorop = old_plane_state->color_pipeline;
>> +	     colorop;
>> +	     colorop = colorop->next) {
>> +		colorop_state = drm_atomic_get_colorop_state(state, colorop);
>> +		if (IS_ERR(colorop_state))
>> +			return PTR_ERR(colorop_state);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static void drm_atomic_colorop_print_state(struct drm_printer *p,
>>   					   const struct drm_colorop_state *state)
>>   {
>> @@ -1591,11 +1644,9 @@ drm_atomic_add_affected_planes(struct drm_atomic_commit *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_pipeline_colorops(state, plane);
>> +		if (ret)
>> +			return ret;
>>   	}
>>   	return 0;
>>   }
>> @@ -1607,10 +1658,8 @@ EXPORT_SYMBOL(drm_atomic_add_affected_planes);
>>    * @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 @plane to the atomic configuration @state. It's 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
>

-- 
Jani Nikula, Intel