* Re: [PATCH] drm/drm_atomic: duplicate colorop states if plane color pipeline in use
2026-03-16 20:52 [PATCH] drm/drm_atomic: duplicate colorop states if plane color pipeline in use Melissa Wen
@ 2026-03-17 20:43 ` Harry Wentland
2026-03-17 21:25 ` Alex Hung
2026-03-18 9:21 ` Borah, Chaitanya Kumar
2 siblings, 0 replies; 5+ messages in thread
From: Harry Wentland @ 2026-03-17 20:43 UTC (permalink / raw)
To: Melissa Wen, maarten.lankhorst, mripard, tzimmermann, airlied,
simona, contact, alex.hung, sebastian.wick, daniels
Cc: Chaitanya Kumar Borah, Uma Shankar, amd-gfx, dri-devel
On 2026-03-16 16:52, Melissa Wen wrote:
> For suspend/resume to work correctly, do for colorop state the same we
> do for plane/crtc/connector states: duplicate the state of colorops in a
> color pipeline if it's in use by a given plane when suspending and
> restore cached colorop states when resuming.
>
> Fixes: 2afc3184f3b3 ("drm/plane: Add COLOR PIPELINE property")
> Signed-off-by: Melissa Wen <mwen@igalia.com>
Of course. Thanks for fixing this.
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
> ---
>
> Hi,
>
> I've been working on making gamescope use KMS plane color API, instead
> of AMD driver-specific color properties [1] and I found this issue
> during Steam Deck suspend/resume process.
>
Great to hear.
Harry
> Initially I thought I should also set plane color_mgmt_changed to make
> AMD driver reprogram the color state, but looks like it's not needed
> (still testing). Therefore, I think the change here is enough to fix it.
>
> It applies on top of drm-misc-next and is inspired by commit
> 6955d6bca053 ("drm/atomic: Add affected colorops with affected planes").
>
> [1] https://github.com/ValveSoftware/gamescope/pull/2113
>
> drivers/gpu/drm/drm_atomic_helper.c | 12 ++++++++++++
> include/drm/drm_atomic.h | 3 ++-
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 26953ed6b53e..683a0e207f71 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3751,6 +3751,13 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev,
> err = PTR_ERR(plane_state);
> goto free;
> }
> +
> + if (plane_state->color_pipeline) {
> + err = drm_atomic_add_affected_colorops(state, plane);
> + if (err)
> + goto free;
> + }
> +
> }
>
> drm_connector_list_iter_begin(dev, &conn_iter);
> @@ -3856,6 +3863,8 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state,
> int i, ret;
> struct drm_plane *plane;
> struct drm_plane_state *new_plane_state;
> + struct drm_colorop *colorop;
> + struct drm_colorop_state *new_colorop_state;
> struct drm_connector *connector;
> struct drm_connector_state *new_conn_state;
> struct drm_crtc *crtc;
> @@ -3866,6 +3875,9 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state,
> for_each_new_plane_in_state(state, plane, new_plane_state, i)
> state->planes[i].old_state = plane->state;
>
> + for_each_new_colorop_in_state(state, colorop, new_colorop_state, i)
> + state->colorops[i].old_state = colorop->state;
> +
> for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> state->crtcs[i].old_state = crtc->state;
>
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 0b1b32bcd2bd..96fd32a3e92c 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -1102,7 +1102,8 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
> for_each_if ((__state)->colorops[__i].ptr && \
> ((colorop) = (__state)->colorops[__i].ptr, \
> (void)(colorop) /* Only to avoid unused-but-set-variable warning */, \
> - (new_colorop_state) = (__state)->colorops[__i].new_state, 1))
> + (new_colorop_state) = (__state)->colorops[__i].new_state,\
> + (void)(new_colorop_state) /* Only to avoid unused-but-set-variable warning */, 1))
>
> /**
> * for_each_oldnew_plane_in_state - iterate over all planes in an atomic update
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/drm_atomic: duplicate colorop states if plane color pipeline in use
2026-03-16 20:52 [PATCH] drm/drm_atomic: duplicate colorop states if plane color pipeline in use Melissa Wen
2026-03-17 20:43 ` Harry Wentland
@ 2026-03-17 21:25 ` Alex Hung
2026-03-18 9:21 ` Borah, Chaitanya Kumar
2 siblings, 0 replies; 5+ messages in thread
From: Alex Hung @ 2026-03-17 21:25 UTC (permalink / raw)
To: Melissa Wen, maarten.lankhorst, mripard, tzimmermann, airlied,
simona, contact, harry.wentland, sebastian.wick, daniels
Cc: Chaitanya Kumar Borah, Uma Shankar, amd-gfx, dri-devel
Reviewed-by: Alex Hung <alex.hung@amd.com>
On 3/16/26 14:52, Melissa Wen wrote:
> For suspend/resume to work correctly, do for colorop state the same we
> do for plane/crtc/connector states: duplicate the state of colorops in a
> color pipeline if it's in use by a given plane when suspending and
> restore cached colorop states when resuming.
>
> Fixes: 2afc3184f3b3 ("drm/plane: Add COLOR PIPELINE property")
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
>
> Hi,
>
> I've been working on making gamescope use KMS plane color API, instead
> of AMD driver-specific color properties [1] and I found this issue
> during Steam Deck suspend/resume process.
>
> Initially I thought I should also set plane color_mgmt_changed to make
> AMD driver reprogram the color state, but looks like it's not needed
> (still testing). Therefore, I think the change here is enough to fix it.
>
> It applies on top of drm-misc-next and is inspired by commit
> 6955d6bca053 ("drm/atomic: Add affected colorops with affected planes").
>
> [1] https://github.com/ValveSoftware/gamescope/pull/2113
>
> drivers/gpu/drm/drm_atomic_helper.c | 12 ++++++++++++
> include/drm/drm_atomic.h | 3 ++-
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 26953ed6b53e..683a0e207f71 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3751,6 +3751,13 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev,
> err = PTR_ERR(plane_state);
> goto free;
> }
> +
> + if (plane_state->color_pipeline) {
> + err = drm_atomic_add_affected_colorops(state, plane);
> + if (err)
> + goto free;
> + }
> +
> }
>
> drm_connector_list_iter_begin(dev, &conn_iter);
> @@ -3856,6 +3863,8 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state,
> int i, ret;
> struct drm_plane *plane;
> struct drm_plane_state *new_plane_state;
> + struct drm_colorop *colorop;
> + struct drm_colorop_state *new_colorop_state;
> struct drm_connector *connector;
> struct drm_connector_state *new_conn_state;
> struct drm_crtc *crtc;
> @@ -3866,6 +3875,9 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state,
> for_each_new_plane_in_state(state, plane, new_plane_state, i)
> state->planes[i].old_state = plane->state;
>
> + for_each_new_colorop_in_state(state, colorop, new_colorop_state, i)
> + state->colorops[i].old_state = colorop->state;
> +
> for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> state->crtcs[i].old_state = crtc->state;
>
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 0b1b32bcd2bd..96fd32a3e92c 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -1102,7 +1102,8 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
> for_each_if ((__state)->colorops[__i].ptr && \
> ((colorop) = (__state)->colorops[__i].ptr, \
> (void)(colorop) /* Only to avoid unused-but-set-variable warning */, \
> - (new_colorop_state) = (__state)->colorops[__i].new_state, 1))
> + (new_colorop_state) = (__state)->colorops[__i].new_state,\
> + (void)(new_colorop_state) /* Only to avoid unused-but-set-variable warning */, 1))
>
> /**
> * for_each_oldnew_plane_in_state - iterate over all planes in an atomic update
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/drm_atomic: duplicate colorop states if plane color pipeline in use
2026-03-16 20:52 [PATCH] drm/drm_atomic: duplicate colorop states if plane color pipeline in use Melissa Wen
2026-03-17 20:43 ` Harry Wentland
2026-03-17 21:25 ` Alex Hung
@ 2026-03-18 9:21 ` Borah, Chaitanya Kumar
2026-03-18 11:19 ` Melissa Wen
2 siblings, 1 reply; 5+ messages in thread
From: Borah, Chaitanya Kumar @ 2026-03-18 9:21 UTC (permalink / raw)
To: Melissa Wen, maarten.lankhorst, mripard, tzimmermann, airlied,
simona, contact, harry.wentland, alex.hung, sebastian.wick,
daniels
Cc: Uma Shankar, amd-gfx, dri-devel
On 3/17/2026 2:22 AM, Melissa Wen wrote:
> For suspend/resume to work correctly, do for colorop state the same we
> do for plane/crtc/connector states: duplicate the state of colorops in a
> color pipeline if it's in use by a given plane when suspending and
> restore cached colorop states when resuming.
>
> Fixes: 2afc3184f3b3 ("drm/plane: Add COLOR PIPELINE property")
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
>
> Hi,
>
> I've been working on making gamescope use KMS plane color API, instead
> of AMD driver-specific color properties [1] and I found this issue
> during Steam Deck suspend/resume process.
>
> Initially I thought I should also set plane color_mgmt_changed to make
> AMD driver reprogram the color state, but looks like it's not needed
> (still testing). Therefore, I think the change here is enough to fix it.
>
> It applies on top of drm-misc-next and is inspired by commit
> 6955d6bca053 ("drm/atomic: Add affected colorops with affected planes").
>
> [1] https://github.com/ValveSoftware/gamescope/pull/2113
>
> drivers/gpu/drm/drm_atomic_helper.c | 12 ++++++++++++
> include/drm/drm_atomic.h | 3 ++-
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 26953ed6b53e..683a0e207f71 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3751,6 +3751,13 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev,
> err = PTR_ERR(plane_state);
> goto free;
> }
> +
> + if (plane_state->color_pipeline) {
> + err = drm_atomic_add_affected_colorops(state, plane);
> + if (err)
> + goto free;
> + }
> +
> }
>
> drm_connector_list_iter_begin(dev, &conn_iter);
> @@ -3856,6 +3863,8 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state,
> int i, ret;
> struct drm_plane *plane;
> struct drm_plane_state *new_plane_state;
> + struct drm_colorop *colorop;
> + struct drm_colorop_state *new_colorop_state;
> struct drm_connector *connector;
> struct drm_connector_state *new_conn_state;
> struct drm_crtc *crtc;
> @@ -3866,6 +3875,9 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state,
> for_each_new_plane_in_state(state, plane, new_plane_state, i)
> state->planes[i].old_state = plane->state;
>
> + for_each_new_colorop_in_state(state, colorop, new_colorop_state, i)
> + state->colorops[i].old_state = colorop->state;
> +
Nit: Just to maintain the object hierarchy this could be moved above planes.
> for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> state->crtcs[i].old_state = crtc->state;
>
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 0b1b32bcd2bd..96fd32a3e92c 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -1102,7 +1102,8 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
> for_each_if ((__state)->colorops[__i].ptr && \
> ((colorop) = (__state)->colorops[__i].ptr, \
> (void)(colorop) /* Only to avoid unused-but-set-variable warning */, \
> - (new_colorop_state) = (__state)->colorops[__i].new_state, 1))
> + (new_colorop_state) = (__state)->colorops[__i].new_state,\
> + (void)(new_colorop_state) /* Only to avoid unused-but-set-variable warning */, 1))
>
Perhaps this should move to a separate patch.
Otherwise, the changes LGTM.
Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> /**
> * for_each_oldnew_plane_in_state - iterate over all planes in an atomic update
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/drm_atomic: duplicate colorop states if plane color pipeline in use
2026-03-18 9:21 ` Borah, Chaitanya Kumar
@ 2026-03-18 11:19 ` Melissa Wen
0 siblings, 0 replies; 5+ messages in thread
From: Melissa Wen @ 2026-03-18 11:19 UTC (permalink / raw)
To: Borah, Chaitanya Kumar, maarten.lankhorst, mripard, tzimmermann,
airlied, simona, contact, harry.wentland, alex.hung,
sebastian.wick, daniels
Cc: Uma Shankar, amd-gfx, dri-devel
On 18/03/2026 06:21, Borah, Chaitanya Kumar wrote:
>
>
> On 3/17/2026 2:22 AM, Melissa Wen wrote:
>> For suspend/resume to work correctly, do for colorop state the same we
>> do for plane/crtc/connector states: duplicate the state of colorops in a
>> color pipeline if it's in use by a given plane when suspending and
>> restore cached colorop states when resuming.
>>
>> Fixes: 2afc3184f3b3 ("drm/plane: Add COLOR PIPELINE property")
>> Signed-off-by: Melissa Wen <mwen@igalia.com>
>> ---
>>
>> Hi,
>>
>> I've been working on making gamescope use KMS plane color API, instead
>> of AMD driver-specific color properties [1] and I found this issue
>> during Steam Deck suspend/resume process.
>>
>> Initially I thought I should also set plane color_mgmt_changed to make
>> AMD driver reprogram the color state, but looks like it's not needed
>> (still testing). Therefore, I think the change here is enough to fix it.
>>
>> It applies on top of drm-misc-next and is inspired by commit
>> 6955d6bca053 ("drm/atomic: Add affected colorops with affected planes").
>>
>> [1] https://github.com/ValveSoftware/gamescope/pull/2113
>>
>> drivers/gpu/drm/drm_atomic_helper.c | 12 ++++++++++++
>> include/drm/drm_atomic.h | 3 ++-
>> 2 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
>> b/drivers/gpu/drm/drm_atomic_helper.c
>> index 26953ed6b53e..683a0e207f71 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -3751,6 +3751,13 @@ drm_atomic_helper_duplicate_state(struct
>> drm_device *dev,
>> err = PTR_ERR(plane_state);
>> goto free;
>> }
>> +
>> + if (plane_state->color_pipeline) {
>> + err = drm_atomic_add_affected_colorops(state, plane);
>> + if (err)
>> + goto free;
>> + }
>> +
>> }
>> drm_connector_list_iter_begin(dev, &conn_iter);
>> @@ -3856,6 +3863,8 @@ int
>> drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state
>> *state,
>> int i, ret;
>> struct drm_plane *plane;
>> struct drm_plane_state *new_plane_state;
>> + struct drm_colorop *colorop;
>> + struct drm_colorop_state *new_colorop_state;
>> struct drm_connector *connector;
>> struct drm_connector_state *new_conn_state;
>> struct drm_crtc *crtc;
>> @@ -3866,6 +3875,9 @@ int
>> drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state
>> *state,
>> for_each_new_plane_in_state(state, plane, new_plane_state, i)
>> state->planes[i].old_state = plane->state;
>> + for_each_new_colorop_in_state(state, colorop,
>> new_colorop_state, i)
>> + state->colorops[i].old_state = colorop->state;
>> +
>
> Nit: Just to maintain the object hierarchy this could be moved above
> planes.
Ack
>
>> for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
>> state->crtcs[i].old_state = crtc->state;
>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>> index 0b1b32bcd2bd..96fd32a3e92c 100644
>> --- a/include/drm/drm_atomic.h
>> +++ b/include/drm/drm_atomic.h
>> @@ -1102,7 +1102,8 @@ void drm_state_dump(struct drm_device *dev,
>> struct drm_printer *p);
>> for_each_if ((__state)->colorops[__i].ptr && \
>> ((colorop) = (__state)->colorops[__i].ptr, \
>> (void)(colorop) /* Only to avoid
>> unused-but-set-variable warning */, \
>> - (new_colorop_state) =
>> (__state)->colorops[__i].new_state, 1))
>> + (new_colorop_state) =
>> (__state)->colorops[__i].new_state,\
>> + (void)(new_colorop_state) /* Only to avoid
>> unused-but-set-variable warning */, 1))
>>
>
> Perhaps this should move to a separate patch.
As the unused-variable warning didn't appear without the
drm_atomic_helper.c changes made here, I think keeping them together is
the best for context.
I mean, the problem doesn't exist before.
Thanks for reviewing,
Melissa
>
> Otherwise, the changes LGTM.
>
> Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
>
>
>> /**
>> * for_each_oldnew_plane_in_state - iterate over all planes in an
>> atomic update
>
^ permalink raw reply [flat|nested] 5+ messages in thread