* [PATCH v2 0/2] drm/atomic: track colorop changes of a given plane
@ 2026-03-23 13:15 Melissa Wen
2026-03-23 13:15 ` [PATCH v2 1/2] drm/atomic: track individual colorop updates Melissa Wen
2026-03-23 13:15 ` [PATCH v2 2/2] drm/amd/display: use plane color_mgmt_changed to track colorop changes Melissa Wen
0 siblings, 2 replies; 7+ messages in thread
From: Melissa Wen @ 2026-03-23 13:15 UTC (permalink / raw)
To: airlied, alexander.deucher, christian.koenig, harry.wentland,
maarten.lankhorst, mripard, simona, siqueira, sunpeng.li,
tzimmermann
Cc: Alex Hung, Chaitanya Kumar Borah, Simon Ser, Uma Shankar,
Xaver Hugl, amd-gfx, kernel-dev, Rob Clark, Dmitry Baryshkov,
Abhinav Kumar, Jessica Zhang, Sean Paul, Marijn Suijten,
linux-arm-msm, dri-devel, freedreno
Similar to what is done for tracking CRTC color mgmt property changes
with CRTC `color_mgmt_changed` flag, track colorop updates of a given
plane color pipeline by setting plane `color_mgmt_changed` flag. Also
true if setting a different color pipeline to a given plane. That way,
the driver can react accordingly and update their color blocks.
This small series fix shaper/3D LUT updates when changing night mode
settings on gamescope with a custom branch that supports
`COLOR_PIPELINE`[1].
This series doesn't cover 1D/3D LUT interpolation, since it's documented
as read-only properties.
v1: https://lore.kernel.org/dri-devel/20260318162348.299807-1-mwen@igalia.com/
Changes:
- include linux types for function's bool return type (kernel bot on MSM
driver)
- add Harry's r-b tags
Let me know your thoughts!
[1] https://github.com/ValveSoftware/gamescope/pull/2113
Melissa Wen
Melissa Wen (2):
drm/atomic: track individual colorop updates
drm/amd/display: use plane color_mgmt_changed to track colorop changes
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 ++-
drivers/gpu/drm/drm_atomic_uapi.c | 53 +++++++++++++++----
include/drm/drm_atomic_uapi.h | 4 +-
3 files changed, 50 insertions(+), 13 deletions(-)
--
2.53.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] drm/atomic: track individual colorop updates
2026-03-23 13:15 [PATCH v2 0/2] drm/atomic: track colorop changes of a given plane Melissa Wen
@ 2026-03-23 13:15 ` Melissa Wen
2026-03-25 9:08 ` Borah, Chaitanya Kumar
2026-03-23 13:15 ` [PATCH v2 2/2] drm/amd/display: use plane color_mgmt_changed to track colorop changes Melissa Wen
1 sibling, 1 reply; 7+ messages in thread
From: Melissa Wen @ 2026-03-23 13:15 UTC (permalink / raw)
To: airlied, alexander.deucher, christian.koenig, harry.wentland,
maarten.lankhorst, mripard, simona, siqueira, sunpeng.li,
tzimmermann
Cc: Alex Hung, Chaitanya Kumar Borah, Simon Ser, Uma Shankar,
Xaver Hugl, amd-gfx, kernel-dev, Rob Clark, Dmitry Baryshkov,
Abhinav Kumar, Jessica Zhang, Sean Paul, Marijn Suijten,
linux-arm-msm, dri-devel, freedreno
As we do for CRTC color mgmt properties, use color_mgmt_changed flag to
track any value changes in the color pipeline of a given plane, so that
drivers can update color blocks as soon as plane color pipeline or
individual colorop values change.
Reviewed-by: Harry Wentland <harry.wentland@amd.com> #v1
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
v2: add linux types to provide bool for MSM driver (kernel bot)
---
drivers/gpu/drm/drm_atomic_uapi.c | 53 ++++++++++++++++++++++++-------
include/drm/drm_atomic_uapi.h | 4 ++-
2 files changed, 45 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 87de41fb4459..713fa9e81732 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -265,13 +265,19 @@ EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
*
* Helper function to select the color pipeline on a plane by setting
* it to the first drm_colorop element of the pipeline.
+ *
+ * Return: true if plane color pipeline value changed, false otherwise.
*/
-void
+bool
drm_atomic_set_colorop_for_plane(struct drm_plane_state *plane_state,
struct drm_colorop *colorop)
{
struct drm_plane *plane = plane_state->plane;
+ /* Color pipeline didn't change */
+ if (plane_state->color_pipeline == colorop)
+ return false;
+
if (colorop)
drm_dbg_atomic(plane->dev,
"Set [COLOROP:%d] for [PLANE:%d:%s] state %p\n",
@@ -283,6 +289,8 @@ drm_atomic_set_colorop_for_plane(struct drm_plane_state *plane_state,
plane->base.id, plane->name, plane_state);
plane_state->color_pipeline = colorop;
+
+ return true;
}
EXPORT_SYMBOL(drm_atomic_set_colorop_for_plane);
@@ -600,7 +608,7 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
if (val && !colorop)
return -EACCES;
- drm_atomic_set_colorop_for_plane(state, colorop);
+ state->color_mgmt_changed |= drm_atomic_set_colorop_for_plane(state, colorop);
} else if (property == config->prop_fb_damage_clips) {
ret = drm_property_replace_blob_from_id(dev,
&state->fb_damage_clips,
@@ -709,11 +717,11 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
static int drm_atomic_color_set_data_property(struct drm_colorop *colorop,
struct drm_colorop_state *state,
struct drm_property *property,
- uint64_t val)
+ uint64_t val,
+ bool *replaced)
{
ssize_t elem_size = -1;
ssize_t size = -1;
- bool replaced = false;
switch (colorop->type) {
case DRM_COLOROP_1D_LUT:
@@ -735,28 +743,39 @@ static int drm_atomic_color_set_data_property(struct drm_colorop *colorop,
&state->data,
val,
-1, size, elem_size,
- &replaced);
+ replaced);
}
static int drm_atomic_colorop_set_property(struct drm_colorop *colorop,
struct drm_colorop_state *state,
struct drm_file *file_priv,
struct drm_property *property,
- uint64_t val)
+ uint64_t val,
+ bool *replaced)
{
if (property == colorop->bypass_property) {
- state->bypass = val;
+ if (state->bypass != val) {
+ state->bypass = val;
+ *replaced = true;
+ }
} else if (property == colorop->lut1d_interpolation_property) {
colorop->lut1d_interpolation = val;
} else if (property == colorop->curve_1d_type_property) {
- state->curve_1d_type = val;
+ if (state->curve_1d_type != val) {
+ state->curve_1d_type = val;
+ *replaced = true;
+ }
} else if (property == colorop->multiplier_property) {
- state->multiplier = val;
+ if (state->multiplier != val) {
+ state->multiplier = val;
+ *replaced = true;
+ }
} else if (property == colorop->lut3d_interpolation_property) {
colorop->lut3d_interpolation = val;
} else if (property == colorop->data_property) {
return drm_atomic_color_set_data_property(colorop, state,
- property, val);
+ property, val,
+ replaced);
} else {
drm_dbg_atomic(colorop->dev,
"[COLOROP:%d:%d] unknown property [PROP:%d:%s]\n",
@@ -1273,6 +1292,8 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
case DRM_MODE_OBJECT_COLOROP: {
struct drm_colorop *colorop = obj_to_colorop(obj);
struct drm_colorop_state *colorop_state;
+ struct drm_plane_state *plane_state;
+ bool replaced = false;
colorop_state = drm_atomic_get_colorop_state(state, colorop);
if (IS_ERR(colorop_state)) {
@@ -1281,7 +1302,17 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
}
ret = drm_atomic_colorop_set_property(colorop, colorop_state,
- file_priv, prop, prop_value);
+ file_priv, prop, prop_value,
+ &replaced);
+ if (ret || !replaced)
+ break;
+
+ plane_state = drm_atomic_get_plane_state(state, colorop->plane);
+ if (IS_ERR(plane_state)) {
+ ret = PTR_ERR(plane_state);
+ break;
+ }
+ plane_state->color_mgmt_changed = true;
break;
}
default:
diff --git a/include/drm/drm_atomic_uapi.h b/include/drm/drm_atomic_uapi.h
index 436315523326..4e7e78f711e2 100644
--- a/include/drm/drm_atomic_uapi.h
+++ b/include/drm/drm_atomic_uapi.h
@@ -29,6 +29,8 @@
#ifndef DRM_ATOMIC_UAPI_H_
#define DRM_ATOMIC_UAPI_H_
+#include <linux/types.h>
+
struct drm_crtc_state;
struct drm_display_mode;
struct drm_property_blob;
@@ -50,7 +52,7 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
struct drm_crtc *crtc);
void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
struct drm_framebuffer *fb);
-void drm_atomic_set_colorop_for_plane(struct drm_plane_state *plane_state,
+bool drm_atomic_set_colorop_for_plane(struct drm_plane_state *plane_state,
struct drm_colorop *colorop);
int __must_check
drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
--
2.53.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] drm/amd/display: use plane color_mgmt_changed to track colorop changes
2026-03-23 13:15 [PATCH v2 0/2] drm/atomic: track colorop changes of a given plane Melissa Wen
2026-03-23 13:15 ` [PATCH v2 1/2] drm/atomic: track individual colorop updates Melissa Wen
@ 2026-03-23 13:15 ` Melissa Wen
1 sibling, 0 replies; 7+ messages in thread
From: Melissa Wen @ 2026-03-23 13:15 UTC (permalink / raw)
To: airlied, alexander.deucher, christian.koenig, harry.wentland,
maarten.lankhorst, mripard, simona, siqueira, sunpeng.li,
tzimmermann
Cc: Alex Hung, Chaitanya Kumar Borah, Simon Ser, Uma Shankar,
Xaver Hugl, amd-gfx, kernel-dev, Rob Clark, Dmitry Baryshkov,
Abhinav Kumar, Jessica Zhang, Sean Paul, Marijn Suijten,
linux-arm-msm, dri-devel, freedreno
Ensure the driver tracks changes in any colorop property of a plane
color pipeline by using the same mechanism of CRTC color management and
update plane color blocks when any colorop property changes. It fixes an
issue observed on gamescope settings for night mode which is done via
shaper/3D-LUT updates.
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
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 dfe95c9b8746..dc3f284d0834 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -9955,7 +9955,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
continue;
bundle->surface_updates[planes_count].surface = dc_plane;
- if (new_pcrtc_state->color_mgmt_changed) {
+ if (new_pcrtc_state->color_mgmt_changed || new_plane_state->color_mgmt_changed) {
bundle->surface_updates[planes_count].gamma = &dc_plane->gamma_correction;
bundle->surface_updates[planes_count].in_transfer_func = &dc_plane->in_transfer_func;
bundle->surface_updates[planes_count].gamut_remap_matrix = &dc_plane->gamut_remap_matrix;
@@ -11695,6 +11695,10 @@ static bool should_reset_plane(struct drm_atomic_state *state,
if (new_crtc_state->color_mgmt_changed)
return true;
+ /* Plane color pipeline or its colorop changes. */
+ if (new_plane_state->color_mgmt_changed)
+ return true;
+
/*
* On zpos change, planes need to be reordered by removing and re-adding
* them one by one to the dc state, in order of descending zpos.
--
2.53.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] drm/atomic: track individual colorop updates
2026-03-23 13:15 ` [PATCH v2 1/2] drm/atomic: track individual colorop updates Melissa Wen
@ 2026-03-25 9:08 ` Borah, Chaitanya Kumar
2026-03-26 2:13 ` Melissa Wen
0 siblings, 1 reply; 7+ messages in thread
From: Borah, Chaitanya Kumar @ 2026-03-25 9:08 UTC (permalink / raw)
To: Melissa Wen, airlied, alexander.deucher, christian.koenig,
harry.wentland, maarten.lankhorst, mripard, simona, siqueira,
sunpeng.li, tzimmermann
Cc: Alex Hung, Simon Ser, Uma Shankar, Xaver Hugl, amd-gfx,
kernel-dev, Rob Clark, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Sean Paul, Marijn Suijten, linux-arm-msm,
dri-devel, freedreno
Hi Melissa,
On 3/23/2026 6:45 PM, Melissa Wen wrote:
> As we do for CRTC color mgmt properties, use color_mgmt_changed flag to
> track any value changes in the color pipeline of a given plane, so that
> drivers can update color blocks as soon as plane color pipeline or
> individual colorop values change.
>
> Reviewed-by: Harry Wentland <harry.wentland@amd.com> #v1
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
>
> v2: add linux types to provide bool for MSM driver (kernel bot)
> ---
> drivers/gpu/drm/drm_atomic_uapi.c | 53 ++++++++++++++++++++++++-------
> include/drm/drm_atomic_uapi.h | 4 ++-
> 2 files changed, 45 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 87de41fb4459..713fa9e81732 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -265,13 +265,19 @@ EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
> *
> * Helper function to select the color pipeline on a plane by setting
> * it to the first drm_colorop element of the pipeline.
> + *
> + * Return: true if plane color pipeline value changed, false otherwise.
> */
> -void
> +bool
> drm_atomic_set_colorop_for_plane(struct drm_plane_state *plane_state,
> struct drm_colorop *colorop)
> {
> struct drm_plane *plane = plane_state->plane;
>
> + /* Color pipeline didn't change */
> + if (plane_state->color_pipeline == colorop)
> + return false;
> +
> if (colorop)
> drm_dbg_atomic(plane->dev,
> "Set [COLOROP:%d] for [PLANE:%d:%s] state %p\n",
> @@ -283,6 +289,8 @@ drm_atomic_set_colorop_for_plane(struct drm_plane_state *plane_state,
> plane->base.id, plane->name, plane_state);
>
> plane_state->color_pipeline = colorop;
> +
> + return true;
> }
> EXPORT_SYMBOL(drm_atomic_set_colorop_for_plane);
>
> @@ -600,7 +608,7 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
> if (val && !colorop)
> return -EACCES;
>
> - drm_atomic_set_colorop_for_plane(state, colorop);
> + state->color_mgmt_changed |= drm_atomic_set_colorop_for_plane(state, colorop);
> } else if (property == config->prop_fb_damage_clips) {
> ret = drm_property_replace_blob_from_id(dev,
> &state->fb_damage_clips,
> @@ -709,11 +717,11 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> static int drm_atomic_color_set_data_property(struct drm_colorop *colorop,
> struct drm_colorop_state *state,
> struct drm_property *property,
> - uint64_t val)
> + uint64_t val,
> + bool *replaced)
> {
> ssize_t elem_size = -1;
> ssize_t size = -1;
> - bool replaced = false;
>
> switch (colorop->type) {
> case DRM_COLOROP_1D_LUT:
> @@ -735,28 +743,39 @@ static int drm_atomic_color_set_data_property(struct drm_colorop *colorop,
> &state->data,
> val,
> -1, size, elem_size,
> - &replaced);
> + replaced);
> }
>
> static int drm_atomic_colorop_set_property(struct drm_colorop *colorop,
> struct drm_colorop_state *state,
> struct drm_file *file_priv,
> struct drm_property *property,
> - uint64_t val)
> + uint64_t val,
> + bool *replaced)
> {
> if (property == colorop->bypass_property) {
> - state->bypass = val;
> + if (state->bypass != val) {
> + state->bypass = val;
> + *replaced = true;
> + }
> } else if (property == colorop->lut1d_interpolation_property) {
> colorop->lut1d_interpolation = val;
> } else if (property == colorop->curve_1d_type_property) {
> - state->curve_1d_type = val;
> + if (state->curve_1d_type != val) {
> + state->curve_1d_type = val;
> + *replaced = true;
> + }
> } else if (property == colorop->multiplier_property) {
> - state->multiplier = val;
> + if (state->multiplier != val) {
> + state->multiplier = val;
> + *replaced = true;
> + }
> } else if (property == colorop->lut3d_interpolation_property) {
> colorop->lut3d_interpolation = val;
I think it would be prudent to add this logic for both the 1dlut and
3dlut interpolation properties. Even though they have just one value
exposed right now, that might change in future.
> } else if (property == colorop->data_property) {
> return drm_atomic_color_set_data_property(colorop, state,
> - property, val);
> + property, val,
> + replaced);
> } else {
> drm_dbg_atomic(colorop->dev,
> "[COLOROP:%d:%d] unknown property [PROP:%d:%s]\n",
> @@ -1273,6 +1292,8 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
> case DRM_MODE_OBJECT_COLOROP: {
> struct drm_colorop *colorop = obj_to_colorop(obj);
> struct drm_colorop_state *colorop_state;
> + struct drm_plane_state *plane_state;
> + bool replaced = false;
>
> colorop_state = drm_atomic_get_colorop_state(state, colorop);
> if (IS_ERR(colorop_state)) {
> @@ -1281,7 +1302,17 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
> }
>
> ret = drm_atomic_colorop_set_property(colorop, colorop_state,
> - file_priv, prop, prop_value);
> + file_priv, prop, prop_value,
> + &replaced);
> + if (ret || !replaced)
> + break;
> +
> + plane_state = drm_atomic_get_plane_state(state, colorop->plane);
> + if (IS_ERR(plane_state)) {
> + ret = PTR_ERR(plane_state);
> + break;
> + }
> + plane_state->color_mgmt_changed = true;
I am not sure if it was the intention of the uapi design but as I
understand there are no guardrails for setting a colorop in an
"inactive" pipeline.
So, color_mgmt_changed is set to true even if a colorop from a color
pipeline that is not currently selected(or set to Bypass) by the
user-space is changed.
I guess, the driver needs to be intelligent enough to ignore those
colorop but should we reject it at drm core?
==
Chaitanya
> break;
> }
> default:
> diff --git a/include/drm/drm_atomic_uapi.h b/include/drm/drm_atomic_uapi.h
> index 436315523326..4e7e78f711e2 100644
> --- a/include/drm/drm_atomic_uapi.h
> +++ b/include/drm/drm_atomic_uapi.h
> @@ -29,6 +29,8 @@
> #ifndef DRM_ATOMIC_UAPI_H_
> #define DRM_ATOMIC_UAPI_H_
>
> +#include <linux/types.h>
> +
> struct drm_crtc_state;
> struct drm_display_mode;
> struct drm_property_blob;
> @@ -50,7 +52,7 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
> struct drm_crtc *crtc);
> void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
> struct drm_framebuffer *fb);
> -void drm_atomic_set_colorop_for_plane(struct drm_plane_state *plane_state,
> +bool drm_atomic_set_colorop_for_plane(struct drm_plane_state *plane_state,
> struct drm_colorop *colorop);
> int __must_check
> drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] drm/atomic: track individual colorop updates
2026-03-25 9:08 ` Borah, Chaitanya Kumar
@ 2026-03-26 2:13 ` Melissa Wen
2026-03-26 3:08 ` Alex Hung
0 siblings, 1 reply; 7+ messages in thread
From: Melissa Wen @ 2026-03-26 2:13 UTC (permalink / raw)
To: Borah, Chaitanya Kumar, airlied, alexander.deucher,
christian.koenig, harry.wentland, maarten.lankhorst, mripard,
simona, siqueira, sunpeng.li, tzimmermann
Cc: Alex Hung, Simon Ser, Uma Shankar, Xaver Hugl, amd-gfx,
kernel-dev, Rob Clark, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Sean Paul, Marijn Suijten, linux-arm-msm,
dri-devel, freedreno
On 25/03/2026 06:08, Borah, Chaitanya Kumar wrote:
> Hi Melissa,
>
> On 3/23/2026 6:45 PM, Melissa Wen wrote:
>> As we do for CRTC color mgmt properties, use color_mgmt_changed flag to
>> track any value changes in the color pipeline of a given plane, so that
>> drivers can update color blocks as soon as plane color pipeline or
>> individual colorop values change.
>>
>> Reviewed-by: Harry Wentland <harry.wentland@amd.com> #v1
>> Signed-off-by: Melissa Wen <mwen@igalia.com>
>> ---
>>
>> v2: add linux types to provide bool for MSM driver (kernel bot)
>> ---
>> drivers/gpu/drm/drm_atomic_uapi.c | 53 ++++++++++++++++++++++++-------
>> include/drm/drm_atomic_uapi.h | 4 ++-
>> 2 files changed, 45 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
>> b/drivers/gpu/drm/drm_atomic_uapi.c
>> index 87de41fb4459..713fa9e81732 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -265,13 +265,19 @@ EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
>> *
>> * Helper function to select the color pipeline on a plane by setting
>> * it to the first drm_colorop element of the pipeline.
>> + *
>> + * Return: true if plane color pipeline value changed, false otherwise.
>> */
>> -void
>> +bool
>> drm_atomic_set_colorop_for_plane(struct drm_plane_state *plane_state,
>> struct drm_colorop *colorop)
>> {
>> struct drm_plane *plane = plane_state->plane;
>> + /* Color pipeline didn't change */
>> + if (plane_state->color_pipeline == colorop)
>> + return false;
>> +
>> if (colorop)
>> drm_dbg_atomic(plane->dev,
>> "Set [COLOROP:%d] for [PLANE:%d:%s] state %p\n",
>> @@ -283,6 +289,8 @@ drm_atomic_set_colorop_for_plane(struct
>> drm_plane_state *plane_state,
>> plane->base.id, plane->name, plane_state);
>> plane_state->color_pipeline = colorop;
>> +
>> + return true;
>> }
>> EXPORT_SYMBOL(drm_atomic_set_colorop_for_plane);
>> @@ -600,7 +608,7 @@ static int drm_atomic_plane_set_property(struct
>> drm_plane *plane,
>> if (val && !colorop)
>> return -EACCES;
>> - drm_atomic_set_colorop_for_plane(state, colorop);
>> + state->color_mgmt_changed |=
>> drm_atomic_set_colorop_for_plane(state, colorop);
>> } else if (property == config->prop_fb_damage_clips) {
>> ret = drm_property_replace_blob_from_id(dev,
>> &state->fb_damage_clips,
>> @@ -709,11 +717,11 @@ drm_atomic_plane_get_property(struct drm_plane
>> *plane,
>> static int drm_atomic_color_set_data_property(struct drm_colorop
>> *colorop,
>> struct drm_colorop_state *state,
>> struct drm_property *property,
>> - uint64_t val)
>> + uint64_t val,
>> + bool *replaced)
>> {
>> ssize_t elem_size = -1;
>> ssize_t size = -1;
>> - bool replaced = false;
>> switch (colorop->type) {
>> case DRM_COLOROP_1D_LUT:
>> @@ -735,28 +743,39 @@ static int
>> drm_atomic_color_set_data_property(struct drm_colorop *colorop,
>> &state->data,
>> val,
>> -1, size, elem_size,
>> - &replaced);
>> + replaced);
>> }
>> static int drm_atomic_colorop_set_property(struct drm_colorop
>> *colorop,
>> struct drm_colorop_state *state,
>> struct drm_file *file_priv,
>> struct drm_property *property,
>> - uint64_t val)
>> + uint64_t val,
>> + bool *replaced)
>> {
>> if (property == colorop->bypass_property) {
>> - state->bypass = val;
>> + if (state->bypass != val) {
>> + state->bypass = val;
>> + *replaced = true;
>> + }
>> } else if (property == colorop->lut1d_interpolation_property) {
>> colorop->lut1d_interpolation = val;
>> } else if (property == colorop->curve_1d_type_property) {
>> - state->curve_1d_type = val;
>> + if (state->curve_1d_type != val) {
>> + state->curve_1d_type = val;
>> + *replaced = true;
>> + }
>> } else if (property == colorop->multiplier_property) {
>> - state->multiplier = val;
>> + if (state->multiplier != val) {
>> + state->multiplier = val;
>> + *replaced = true;
>> + }
>> } else if (property == colorop->lut3d_interpolation_property) {
>> colorop->lut3d_interpolation = val;
>
> I think it would be prudent to add this logic for both the 1dlut and
> 3dlut interpolation properties. Even though they have just one value
> exposed right now, that might change in future.
I didn't include interpolations in the color_mgmt_changed logic because
there is a comment in `include/drm/drm_colorop.h` saying that they are
read-only.
But thinking better about it, and I think we should not allow
`drm_atomic_colorop_set_property()` calls to change values of these
properties if they are read-only.
I didn't track the discussions about what are the plans for these
properties, how the userspace knows they are read-only properties and
shouldn't set any value?
>
>> } else if (property == colorop->data_property) {
>> return drm_atomic_color_set_data_property(colorop, state,
>> - property, val);
>> + property, val,
>> + replaced);
>> } else {
>> drm_dbg_atomic(colorop->dev,
>> "[COLOROP:%d:%d] unknown property [PROP:%d:%s]\n",
>> @@ -1273,6 +1292,8 @@ int drm_atomic_set_property(struct
>> drm_atomic_state *state,
>> case DRM_MODE_OBJECT_COLOROP: {
>> struct drm_colorop *colorop = obj_to_colorop(obj);
>> struct drm_colorop_state *colorop_state;
>> + struct drm_plane_state *plane_state;
>> + bool replaced = false;
>> colorop_state = drm_atomic_get_colorop_state(state,
>> colorop);
>> if (IS_ERR(colorop_state)) {
>> @@ -1281,7 +1302,17 @@ int drm_atomic_set_property(struct
>> drm_atomic_state *state,
>> }
>> ret = drm_atomic_colorop_set_property(colorop,
>> colorop_state,
>> - file_priv, prop, prop_value);
>> + file_priv, prop, prop_value,
>> + &replaced);
>> + if (ret || !replaced)
>> + break;
>> +
>> + plane_state = drm_atomic_get_plane_state(state,
>> colorop->plane);
>> + if (IS_ERR(plane_state)) {
>> + ret = PTR_ERR(plane_state);
>> + break;
>> + }
>> + plane_state->color_mgmt_changed = true;
>
> I am not sure if it was the intention of the uapi design but as I
> understand there are no guardrails for setting a colorop in an
> "inactive" pipeline.
>
> So, color_mgmt_changed is set to true even if a colorop from a color
> pipeline that is not currently selected(or set to Bypass) by the
> user-space is changed.
> I guess, the driver needs to be intelligent enough to ignore those
> colorop but should we reject it at drm core?
>
Thanks for pointing it out, makes sense!
I agree that drm core should reject changes in inactive pipelines.
Melissa
> ==
> Chaitanya
>
>> break;
>> }
>> default:
>> diff --git a/include/drm/drm_atomic_uapi.h
>> b/include/drm/drm_atomic_uapi.h
>> index 436315523326..4e7e78f711e2 100644
>> --- a/include/drm/drm_atomic_uapi.h
>> +++ b/include/drm/drm_atomic_uapi.h
>> @@ -29,6 +29,8 @@
>> #ifndef DRM_ATOMIC_UAPI_H_
>> #define DRM_ATOMIC_UAPI_H_
>> +#include <linux/types.h>
>> +
>> struct drm_crtc_state;
>> struct drm_display_mode;
>> struct drm_property_blob;
>> @@ -50,7 +52,7 @@ drm_atomic_set_crtc_for_plane(struct
>> drm_plane_state *plane_state,
>> struct drm_crtc *crtc);
>> void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
>> struct drm_framebuffer *fb);
>> -void drm_atomic_set_colorop_for_plane(struct drm_plane_state
>> *plane_state,
>> +bool drm_atomic_set_colorop_for_plane(struct drm_plane_state
>> *plane_state,
>> struct drm_colorop *colorop);
>> int __must_check
>> drm_atomic_set_crtc_for_connector(struct drm_connector_state
>> *conn_state,
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] drm/atomic: track individual colorop updates
2026-03-26 2:13 ` Melissa Wen
@ 2026-03-26 3:08 ` Alex Hung
2026-03-26 5:51 ` Borah, Chaitanya Kumar
0 siblings, 1 reply; 7+ messages in thread
From: Alex Hung @ 2026-03-26 3:08 UTC (permalink / raw)
To: Melissa Wen, Borah, Chaitanya Kumar, airlied, alexander.deucher,
christian.koenig, harry.wentland, maarten.lankhorst, mripard,
simona, siqueira, sunpeng.li, tzimmermann
Cc: Simon Ser, Uma Shankar, Xaver Hugl, amd-gfx, kernel-dev,
Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
Sean Paul, Marijn Suijten, linux-arm-msm, dri-devel, freedreno
On 3/25/26 20:13, Melissa Wen wrote:
>
>
> On 25/03/2026 06:08, Borah, Chaitanya Kumar wrote:
>> Hi Melissa,
>>
>> On 3/23/2026 6:45 PM, Melissa Wen wrote:
>>> As we do for CRTC color mgmt properties, use color_mgmt_changed flag to
>>> track any value changes in the color pipeline of a given plane, so that
>>> drivers can update color blocks as soon as plane color pipeline or
>>> individual colorop values change.
>>>
>>> Reviewed-by: Harry Wentland <harry.wentland@amd.com> #v1
>>> Signed-off-by: Melissa Wen <mwen@igalia.com>
>>> ---
>>>
>>> v2: add linux types to provide bool for MSM driver (kernel bot)
>>> ---
>>> drivers/gpu/drm/drm_atomic_uapi.c | 53 ++++++++++++++++++++++++-------
>>> include/drm/drm_atomic_uapi.h | 4 ++-
>>> 2 files changed, 45 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/
>>> drm_atomic_uapi.c
>>> index 87de41fb4459..713fa9e81732 100644
>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>>> @@ -265,13 +265,19 @@ EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
>>> *
>>> * Helper function to select the color pipeline on a plane by setting
>>> * it to the first drm_colorop element of the pipeline.
>>> + *
>>> + * Return: true if plane color pipeline value changed, false otherwise.
>>> */
>>> -void
>>> +bool
>>> drm_atomic_set_colorop_for_plane(struct drm_plane_state *plane_state,
>>> struct drm_colorop *colorop)
>>> {
>>> struct drm_plane *plane = plane_state->plane;
>>> + /* Color pipeline didn't change */
>>> + if (plane_state->color_pipeline == colorop)
>>> + return false;
>>> +
>>> if (colorop)
>>> drm_dbg_atomic(plane->dev,
>>> "Set [COLOROP:%d] for [PLANE:%d:%s] state %p\n",
>>> @@ -283,6 +289,8 @@ drm_atomic_set_colorop_for_plane(struct
>>> drm_plane_state *plane_state,
>>> plane->base.id, plane->name, plane_state);
>>> plane_state->color_pipeline = colorop;
>>> +
>>> + return true;
>>> }
>>> EXPORT_SYMBOL(drm_atomic_set_colorop_for_plane);
>>> @@ -600,7 +608,7 @@ static int drm_atomic_plane_set_property(struct
>>> drm_plane *plane,
>>> if (val && !colorop)
>>> return -EACCES;
>>> - drm_atomic_set_colorop_for_plane(state, colorop);
>>> + state->color_mgmt_changed |=
>>> drm_atomic_set_colorop_for_plane(state, colorop);
>>> } else if (property == config->prop_fb_damage_clips) {
>>> ret = drm_property_replace_blob_from_id(dev,
>>> &state->fb_damage_clips,
>>> @@ -709,11 +717,11 @@ drm_atomic_plane_get_property(struct drm_plane
>>> *plane,
>>> static int drm_atomic_color_set_data_property(struct drm_colorop
>>> *colorop,
>>> struct drm_colorop_state *state,
>>> struct drm_property *property,
>>> - uint64_t val)
>>> + uint64_t val,
>>> + bool *replaced)
>>> {
>>> ssize_t elem_size = -1;
>>> ssize_t size = -1;
>>> - bool replaced = false;
>>> switch (colorop->type) {
>>> case DRM_COLOROP_1D_LUT:
>>> @@ -735,28 +743,39 @@ static int
>>> drm_atomic_color_set_data_property(struct drm_colorop *colorop,
>>> &state->data,
>>> val,
>>> -1, size, elem_size,
>>> - &replaced);
>>> + replaced);
>>> }
>>> static int drm_atomic_colorop_set_property(struct drm_colorop
>>> *colorop,
>>> struct drm_colorop_state *state,
>>> struct drm_file *file_priv,
>>> struct drm_property *property,
>>> - uint64_t val)
>>> + uint64_t val,
>>> + bool *replaced)
>>> {
>>> if (property == colorop->bypass_property) {
>>> - state->bypass = val;
>>> + if (state->bypass != val) {
>>> + state->bypass = val;
>>> + *replaced = true;
>>> + }
>>> } else if (property == colorop->lut1d_interpolation_property) {
>>> colorop->lut1d_interpolation = val;
>>> } else if (property == colorop->curve_1d_type_property) {
>>> - state->curve_1d_type = val;
>>> + if (state->curve_1d_type != val) {
>>> + state->curve_1d_type = val;
>>> + *replaced = true;
>>> + }
>>> } else if (property == colorop->multiplier_property) {
>>> - state->multiplier = val;
>>> + if (state->multiplier != val) {
>>> + state->multiplier = val;
>>> + *replaced = true;
>>> + }
>>> } else if (property == colorop->lut3d_interpolation_property) {
>>> colorop->lut3d_interpolation = val;
>>
>> I think it would be prudent to add this logic for both the 1dlut and
>> 3dlut interpolation properties. Even though they have just one value
>> exposed right now, that might change in future.
>
> I didn't include interpolations in the color_mgmt_changed logic because
> there is a comment in `include/drm/drm_colorop.h` saying that they are
> read-only.
> But thinking better about it, and I think we should not allow
> `drm_atomic_colorop_set_property()` calls to change values of these
> properties if they are read-only.
> I didn't track the discussions about what are the plans for these
> properties, how the userspace knows they are read-only properties and
> shouldn't set any value?
It has been a while but I don't remember that userspace needs to set
this value, so this can be a mistake. Device driver just need to give a
supported interpolation that best describes the hardware.
We can remove setting them in drm_atomic_colorop_set_property if
everybody agrees.
>
>>
>>> } else if (property == colorop->data_property) {
>>> return drm_atomic_color_set_data_property(colorop, state,
>>> - property, val);
>>> + property, val,
>>> + replaced);
>>> } else {
>>> drm_dbg_atomic(colorop->dev,
>>> "[COLOROP:%d:%d] unknown property [PROP:%d:%s]\n",
>>> @@ -1273,6 +1292,8 @@ int drm_atomic_set_property(struct
>>> drm_atomic_state *state,
>>> case DRM_MODE_OBJECT_COLOROP: {
>>> struct drm_colorop *colorop = obj_to_colorop(obj);
>>> struct drm_colorop_state *colorop_state;
>>> + struct drm_plane_state *plane_state;
>>> + bool replaced = false;
>>> colorop_state = drm_atomic_get_colorop_state(state,
>>> colorop);
>>> if (IS_ERR(colorop_state)) {
>>> @@ -1281,7 +1302,17 @@ int drm_atomic_set_property(struct
>>> drm_atomic_state *state,
>>> }
>>> ret = drm_atomic_colorop_set_property(colorop,
>>> colorop_state,
>>> - file_priv, prop, prop_value);
>>> + file_priv, prop, prop_value,
>>> + &replaced);
>>> + if (ret || !replaced)
>>> + break;
>>> +
>>> + plane_state = drm_atomic_get_plane_state(state, colorop-
>>> >plane);
>>> + if (IS_ERR(plane_state)) {
>>> + ret = PTR_ERR(plane_state);
>>> + break;
>>> + }
>>> + plane_state->color_mgmt_changed = true;
>>
>> I am not sure if it was the intention of the uapi design but as I
>> understand there are no guardrails for setting a colorop in an
>> "inactive" pipeline.
>>
>> So, color_mgmt_changed is set to true even if a colorop from a color
>> pipeline that is not currently selected(or set to Bypass) by the user-
>> space is changed.
>> I guess, the driver needs to be intelligent enough to ignore those
>> colorop but should we reject it at drm core?
>>
>
> Thanks for pointing it out, makes sense!
> I agree that drm core should reject changes in inactive pipelines.
>
> Melissa
>
>
>> ==
>> Chaitanya
>>
>>> break;
>>> }
>>> default:
>>> diff --git a/include/drm/drm_atomic_uapi.h b/include/drm/
>>> drm_atomic_uapi.h
>>> index 436315523326..4e7e78f711e2 100644
>>> --- a/include/drm/drm_atomic_uapi.h
>>> +++ b/include/drm/drm_atomic_uapi.h
>>> @@ -29,6 +29,8 @@
>>> #ifndef DRM_ATOMIC_UAPI_H_
>>> #define DRM_ATOMIC_UAPI_H_
>>> +#include <linux/types.h>
>>> +
>>> struct drm_crtc_state;
>>> struct drm_display_mode;
>>> struct drm_property_blob;
>>> @@ -50,7 +52,7 @@ drm_atomic_set_crtc_for_plane(struct
>>> drm_plane_state *plane_state,
>>> struct drm_crtc *crtc);
>>> void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
>>> struct drm_framebuffer *fb);
>>> -void drm_atomic_set_colorop_for_plane(struct drm_plane_state
>>> *plane_state,
>>> +bool drm_atomic_set_colorop_for_plane(struct drm_plane_state
>>> *plane_state,
>>> struct drm_colorop *colorop);
>>> int __must_check
>>> drm_atomic_set_crtc_for_connector(struct drm_connector_state
>>> *conn_state,
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] drm/atomic: track individual colorop updates
2026-03-26 3:08 ` Alex Hung
@ 2026-03-26 5:51 ` Borah, Chaitanya Kumar
0 siblings, 0 replies; 7+ messages in thread
From: Borah, Chaitanya Kumar @ 2026-03-26 5:51 UTC (permalink / raw)
To: Alex Hung, Melissa Wen, airlied, alexander.deucher,
christian.koenig, harry.wentland, maarten.lankhorst, mripard,
simona, siqueira, sunpeng.li, tzimmermann
Cc: Simon Ser, Uma Shankar, Xaver Hugl, amd-gfx, kernel-dev,
Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
Sean Paul, Marijn Suijten, linux-arm-msm, dri-devel, freedreno
On 3/26/2026 8:38 AM, Alex Hung wrote:
>
>
> On 3/25/26 20:13, Melissa Wen wrote:
>>
>>
>> On 25/03/2026 06:08, Borah, Chaitanya Kumar wrote:
>>> Hi Melissa,
>>>
>>> On 3/23/2026 6:45 PM, Melissa Wen wrote:
>>>> As we do for CRTC color mgmt properties, use color_mgmt_changed flag to
>>>> track any value changes in the color pipeline of a given plane, so that
>>>> drivers can update color blocks as soon as plane color pipeline or
>>>> individual colorop values change.
>>>>
>>>> Reviewed-by: Harry Wentland <harry.wentland@amd.com> #v1
>>>> Signed-off-by: Melissa Wen <mwen@igalia.com>
>>>> ---
>>>>
>>>> v2: add linux types to provide bool for MSM driver (kernel bot)
>>>> ---
>>>> drivers/gpu/drm/drm_atomic_uapi.c | 53 +++++++++++++++++++++++
>>>> +-------
>>>> include/drm/drm_atomic_uapi.h | 4 ++-
>>>> 2 files changed, 45 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/
>>>> drm_atomic_uapi.c
>>>> index 87de41fb4459..713fa9e81732 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>>>> @@ -265,13 +265,19 @@ EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
>>>> *
>>>> * Helper function to select the color pipeline on a plane by setting
>>>> * it to the first drm_colorop element of the pipeline.
>>>> + *
>>>> + * Return: true if plane color pipeline value changed, false
>>>> otherwise.
>>>> */
>>>> -void
>>>> +bool
>>>> drm_atomic_set_colorop_for_plane(struct drm_plane_state *plane_state,
>>>> struct drm_colorop *colorop)
>>>> {
>>>> struct drm_plane *plane = plane_state->plane;
>>>> + /* Color pipeline didn't change */
>>>> + if (plane_state->color_pipeline == colorop)
>>>> + return false;
>>>> +
>>>> if (colorop)
>>>> drm_dbg_atomic(plane->dev,
>>>> "Set [COLOROP:%d] for [PLANE:%d:%s] state %p\n",
>>>> @@ -283,6 +289,8 @@ drm_atomic_set_colorop_for_plane(struct
>>>> drm_plane_state *plane_state,
>>>> plane->base.id, plane->name, plane_state);
>>>> plane_state->color_pipeline = colorop;
>>>> +
>>>> + return true;
>>>> }
>>>> EXPORT_SYMBOL(drm_atomic_set_colorop_for_plane);
>>>> @@ -600,7 +608,7 @@ static int
>>>> drm_atomic_plane_set_property(struct drm_plane *plane,
>>>> if (val && !colorop)
>>>> return -EACCES;
>>>> - drm_atomic_set_colorop_for_plane(state, colorop);
>>>> + state->color_mgmt_changed |=
>>>> drm_atomic_set_colorop_for_plane(state, colorop);
>>>> } else if (property == config->prop_fb_damage_clips) {
>>>> ret = drm_property_replace_blob_from_id(dev,
>>>> &state->fb_damage_clips,
>>>> @@ -709,11 +717,11 @@ drm_atomic_plane_get_property(struct drm_plane
>>>> *plane,
>>>> static int drm_atomic_color_set_data_property(struct drm_colorop
>>>> *colorop,
>>>> struct drm_colorop_state *state,
>>>> struct drm_property *property,
>>>> - uint64_t val)
>>>> + uint64_t val,
>>>> + bool *replaced)
>>>> {
>>>> ssize_t elem_size = -1;
>>>> ssize_t size = -1;
>>>> - bool replaced = false;
>>>> switch (colorop->type) {
>>>> case DRM_COLOROP_1D_LUT:
>>>> @@ -735,28 +743,39 @@ static int
>>>> drm_atomic_color_set_data_property(struct drm_colorop *colorop,
>>>> &state->data,
>>>> val,
>>>> -1, size, elem_size,
>>>> - &replaced);
>>>> + replaced);
>>>> }
>>>> static int drm_atomic_colorop_set_property(struct drm_colorop
>>>> *colorop,
>>>> struct drm_colorop_state *state,
>>>> struct drm_file *file_priv,
>>>> struct drm_property *property,
>>>> - uint64_t val)
>>>> + uint64_t val,
>>>> + bool *replaced)
>>>> {
>>>> if (property == colorop->bypass_property) {
>>>> - state->bypass = val;
>>>> + if (state->bypass != val) {
>>>> + state->bypass = val;
>>>> + *replaced = true;
>>>> + }
>>>> } else if (property == colorop->lut1d_interpolation_property) {
>>>> colorop->lut1d_interpolation = val;
>>>> } else if (property == colorop->curve_1d_type_property) {
>>>> - state->curve_1d_type = val;
>>>> + if (state->curve_1d_type != val) {
>>>> + state->curve_1d_type = val;
>>>> + *replaced = true;
>>>> + }
>>>> } else if (property == colorop->multiplier_property) {
>>>> - state->multiplier = val;
>>>> + if (state->multiplier != val) {
>>>> + state->multiplier = val;
>>>> + *replaced = true;
>>>> + }
>>>> } else if (property == colorop->lut3d_interpolation_property) {
>>>> colorop->lut3d_interpolation = val;
>>>
>>> I think it would be prudent to add this logic for both the 1dlut and
>>> 3dlut interpolation properties. Even though they have just one value
>>> exposed right now, that might change in future.
>>
>> I didn't include interpolations in the color_mgmt_changed logic
>> because there is a comment in `include/drm/drm_colorop.h` saying that
>> they are read-only.
>> But thinking better about it, and I think we should not allow
>> `drm_atomic_colorop_set_property()` calls to change values of these
>> properties if they are read-only.
>> I didn't track the discussions about what are the plans for these
>> properties, how the userspace knows they are read-only properties and
>> shouldn't set any value?
>
> It has been a while but I don't remember that userspace needs to set
> this value, so this can be a mistake. Device driver just need to give a
> supported interpolation that best describes the hardware.
>
> We can remove setting them in drm_atomic_colorop_set_property if
> everybody agrees.
>
In that case, they need to be marked DRM_MODE_PROP_IMMUTABLE at creation.
==
Chaitanya
>>
>>>
>>>> } else if (property == colorop->data_property) {
>>>> return drm_atomic_color_set_data_property(colorop, state,
>>>> - property, val);
>>>> + property, val,
>>>> + replaced);
>>>> } else {
>>>> drm_dbg_atomic(colorop->dev,
>>>> "[COLOROP:%d:%d] unknown property [PROP:%d:%s]\n",
>>>> @@ -1273,6 +1292,8 @@ int drm_atomic_set_property(struct
>>>> drm_atomic_state *state,
>>>> case DRM_MODE_OBJECT_COLOROP: {
>>>> struct drm_colorop *colorop = obj_to_colorop(obj);
>>>> struct drm_colorop_state *colorop_state;
>>>> + struct drm_plane_state *plane_state;
>>>> + bool replaced = false;
>>>> colorop_state = drm_atomic_get_colorop_state(state,
>>>> colorop);
>>>> if (IS_ERR(colorop_state)) {
>>>> @@ -1281,7 +1302,17 @@ int drm_atomic_set_property(struct
>>>> drm_atomic_state *state,
>>>> }
>>>> ret = drm_atomic_colorop_set_property(colorop,
>>>> colorop_state,
>>>> - file_priv, prop, prop_value);
>>>> + file_priv, prop, prop_value,
>>>> + &replaced);
>>>> + if (ret || !replaced)
>>>> + break;
>>>> +
>>>> + plane_state = drm_atomic_get_plane_state(state, colorop-
>>>> >plane);
>>>> + if (IS_ERR(plane_state)) {
>>>> + ret = PTR_ERR(plane_state);
>>>> + break;
>>>> + }
>>>> + plane_state->color_mgmt_changed = true;
>>>
>>> I am not sure if it was the intention of the uapi design but as I
>>> understand there are no guardrails for setting a colorop in an
>>> "inactive" pipeline.
>>>
>>> So, color_mgmt_changed is set to true even if a colorop from a color
>>> pipeline that is not currently selected(or set to Bypass) by the
>>> user- space is changed.
>>> I guess, the driver needs to be intelligent enough to ignore those
>>> colorop but should we reject it at drm core?
>>>
>>
>> Thanks for pointing it out, makes sense!
>> I agree that drm core should reject changes in inactive pipelines.
>>
>> Melissa
>>
>>
>>> ==
>>> Chaitanya
>>>
>>>> break;
>>>> }
>>>> default:
>>>> diff --git a/include/drm/drm_atomic_uapi.h b/include/drm/
>>>> drm_atomic_uapi.h
>>>> index 436315523326..4e7e78f711e2 100644
>>>> --- a/include/drm/drm_atomic_uapi.h
>>>> +++ b/include/drm/drm_atomic_uapi.h
>>>> @@ -29,6 +29,8 @@
>>>> #ifndef DRM_ATOMIC_UAPI_H_
>>>> #define DRM_ATOMIC_UAPI_H_
>>>> +#include <linux/types.h>
>>>> +
>>>> struct drm_crtc_state;
>>>> struct drm_display_mode;
>>>> struct drm_property_blob;
>>>> @@ -50,7 +52,7 @@ drm_atomic_set_crtc_for_plane(struct
>>>> drm_plane_state *plane_state,
>>>> struct drm_crtc *crtc);
>>>> void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
>>>> struct drm_framebuffer *fb);
>>>> -void drm_atomic_set_colorop_for_plane(struct drm_plane_state
>>>> *plane_state,
>>>> +bool drm_atomic_set_colorop_for_plane(struct drm_plane_state
>>>> *plane_state,
>>>> struct drm_colorop *colorop);
>>>> int __must_check
>>>> drm_atomic_set_crtc_for_connector(struct drm_connector_state
>>>> *conn_state,
>>>
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2026-03-26 5:51 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-23 13:15 [PATCH v2 0/2] drm/atomic: track colorop changes of a given plane Melissa Wen
2026-03-23 13:15 ` [PATCH v2 1/2] drm/atomic: track individual colorop updates Melissa Wen
2026-03-25 9:08 ` Borah, Chaitanya Kumar
2026-03-26 2:13 ` Melissa Wen
2026-03-26 3:08 ` Alex Hung
2026-03-26 5:51 ` Borah, Chaitanya Kumar
2026-03-23 13:15 ` [PATCH v2 2/2] drm/amd/display: use plane color_mgmt_changed to track colorop changes Melissa Wen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox