On Tue, 26 May 2026, Alex Hung <[email protected]> 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 <[email protected]>
>> ---
>> 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
