* [PATCH v9 1/4] drm/colorop: Remove read-only comments from interpolation fields
2026-06-09 10:20 [PATCH v9 0/4] drm/atomic: track individual colorop updates Melissa Wen
@ 2026-06-09 10:20 ` Melissa Wen
2026-06-09 10:20 ` [PATCH v9 2/4] drm/colorop: make lut(1/3)d_interpolation props correctly behave as mutable Melissa Wen
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Melissa Wen @ 2026-06-09 10:20 UTC (permalink / raw)
To: airlied, alexander.deucher, alex.hung, christian.koenig, contact,
daniels, harry.wentland, louis.chauvet, maarten.lankhorst,
mripard, mwen, sebastian.wick, simona, siqueira, sunpeng.li,
tzimmermann
Cc: Uma Shankar, Chaitanya Kumar Borah, Xaver Hugl, Pekka Paalanen,
Matthew Schwartz, amd-gfx, kernel-dev, Rob Clark,
Dmitry Baryshkov, Sean Paul, Marijn Suijten, linux-arm-msm,
freedreno, intel-xe, intel-gfx, dri-devel
From: Alex Hung <alex.hung@amd.com>
The lut1d_interpolation and lut3d_interpolation fields and their
associated properties were marked as read-only, but userspace
can set them via drm_atomic_colorop_set_property().
Fixes: 7fa3ee8c0a79 ("drm/colorop: Define LUT_1D interpolation")
Fixes: db971856bbe0 ("drm/colorop: Add 3D LUT support to color pipeline")
Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
Signed-off-by: Alex Hung <alex.hung@amd.com>
Fixes: 9ba25915efba ("drm/amd/display: Add support for sRGB EOTF in DEGAM block")
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
include/drm/drm_colorop.h | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
index bd082854ca74..61cc8206b4c4 100644
--- a/include/drm/drm_colorop.h
+++ b/include/drm/drm_colorop.h
@@ -309,7 +309,6 @@ struct drm_colorop {
/**
* @lut1d_interpolation:
*
- * Read-only
* Interpolation for DRM_COLOROP_1D_LUT
*/
enum drm_colorop_lut1d_interpolation_type lut1d_interpolation;
@@ -317,7 +316,6 @@ struct drm_colorop {
/**
* @lut3d_interpolation:
*
- * Read-only
* Interpolation for DRM_COLOROP_3D_LUT
*/
enum drm_colorop_lut3d_interpolation_type lut3d_interpolation;
@@ -325,7 +323,7 @@ struct drm_colorop {
/**
* @lut1d_interpolation_property:
*
- * Read-only property for DRM_COLOROP_1D_LUT interpolation
+ * Property for DRM_COLOROP_1D_LUT interpolation
*/
struct drm_property *lut1d_interpolation_property;
@@ -353,7 +351,7 @@ struct drm_colorop {
/**
* @lut3d_interpolation_property:
*
- * Read-only property for DRM_COLOROP_3D_LUT interpolation
+ * Property for DRM_COLOROP_3D_LUT interpolation
*/
struct drm_property *lut3d_interpolation_property;
--
2.53.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v9 2/4] drm/colorop: make lut(1/3)d_interpolation props correctly behave as mutable
2026-06-09 10:20 [PATCH v9 0/4] drm/atomic: track individual colorop updates Melissa Wen
2026-06-09 10:20 ` [PATCH v9 1/4] drm/colorop: Remove read-only comments from interpolation fields Melissa Wen
@ 2026-06-09 10:20 ` Melissa Wen
2026-06-09 11:17 ` sashiko-bot
2026-06-09 10:20 ` [PATCH v9 3/4] drm/atomic: track individual colorop updates Melissa Wen
2026-06-09 10:20 ` [PATCH v9 4/4] drm/amd/display: use plane color_mgmt_changed to track colorop changes Melissa Wen
3 siblings, 1 reply; 8+ messages in thread
From: Melissa Wen @ 2026-06-09 10:20 UTC (permalink / raw)
To: airlied, alexander.deucher, alex.hung, christian.koenig, contact,
daniels, harry.wentland, louis.chauvet, maarten.lankhorst,
mripard, mwen, sebastian.wick, simona, siqueira, sunpeng.li,
tzimmermann
Cc: Uma Shankar, Chaitanya Kumar Borah, Xaver Hugl, Pekka Paalanen,
Matthew Schwartz, amd-gfx, kernel-dev, Rob Clark,
Dmitry Baryshkov, Sean Paul, Marijn Suijten, linux-arm-msm,
freedreno, intel-xe, intel-gfx, dri-devel
As interpolation props are actually mutable props, any changes should be
handled by drm_colorop_state. Move their enum and make it correctly
behaves as mutable.
Fixes: 7fa3ee8c0a79 ("drm/colorop: Define LUT_1D interpolation")
Fixes: db971856bbe0 ("drm/colorop: Add 3D LUT support to color pipeline")
Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
Reviewed-by: Alex Hung <alex.hung@amd.com>
Fixes: 9ba25915efba ("drm/amd/display: Add support for sRGB EOTF in DEGAM block")
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
drivers/gpu/drm/drm_atomic.c | 4 ++--
drivers/gpu/drm/drm_atomic_uapi.c | 8 ++++----
drivers/gpu/drm/drm_colorop.c | 16 ++++++++++++++--
include/drm/drm_colorop.h | 28 ++++++++++++++--------------
4 files changed, 34 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 41c57063f3b4..0eb52d1d5af2 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -830,7 +830,7 @@ static void drm_atomic_colorop_print_state(struct drm_printer *p,
case DRM_COLOROP_1D_LUT:
drm_printf(p, "\tsize=%d\n", colorop->size);
drm_printf(p, "\tinterpolation=%s\n",
- drm_get_colorop_lut1d_interpolation_name(colorop->lut1d_interpolation));
+ drm_get_colorop_lut1d_interpolation_name(state->lut1d_interpolation));
drm_printf(p, "\tdata blob id=%d\n", state->data ? state->data->base.id : 0);
break;
case DRM_COLOROP_CTM_3X4:
@@ -842,7 +842,7 @@ static void drm_atomic_colorop_print_state(struct drm_printer *p,
case DRM_COLOROP_3D_LUT:
drm_printf(p, "\tsize=%d\n", colorop->size);
drm_printf(p, "\tinterpolation=%s\n",
- drm_get_colorop_lut3d_interpolation_name(colorop->lut3d_interpolation));
+ drm_get_colorop_lut3d_interpolation_name(state->lut3d_interpolation));
drm_printf(p, "\tdata blob id=%d\n", state->data ? state->data->base.id : 0);
break;
default:
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 5bd5bf6661df..b81dbba4d8c3 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -751,13 +751,13 @@ static int drm_atomic_colorop_set_property(struct drm_colorop *colorop,
if (property == colorop->bypass_property) {
state->bypass = val;
} else if (property == colorop->lut1d_interpolation_property) {
- colorop->lut1d_interpolation = val;
+ state->lut1d_interpolation = val;
} else if (property == colorop->curve_1d_type_property) {
state->curve_1d_type = val;
} else if (property == colorop->multiplier_property) {
state->multiplier = val;
} else if (property == colorop->lut3d_interpolation_property) {
- colorop->lut3d_interpolation = val;
+ state->lut3d_interpolation = val;
} else if (property == colorop->data_property) {
return drm_atomic_color_set_data_property(colorop, state,
property, val);
@@ -782,7 +782,7 @@ drm_atomic_colorop_get_property(struct drm_colorop *colorop,
else if (property == colorop->bypass_property)
*val = state->bypass;
else if (property == colorop->lut1d_interpolation_property)
- *val = colorop->lut1d_interpolation;
+ *val = state->lut1d_interpolation;
else if (property == colorop->curve_1d_type_property)
*val = state->curve_1d_type;
else if (property == colorop->multiplier_property)
@@ -790,7 +790,7 @@ drm_atomic_colorop_get_property(struct drm_colorop *colorop,
else if (property == colorop->size_property)
*val = colorop->size;
else if (property == colorop->lut3d_interpolation_property)
- *val = colorop->lut3d_interpolation;
+ *val = state->lut3d_interpolation;
else if (property == colorop->data_property)
*val = (state->data) ? state->data->base.id : 0;
else
diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
index 566816e3c6f0..509678e5371f 100644
--- a/drivers/gpu/drm/drm_colorop.c
+++ b/drivers/gpu/drm/drm_colorop.c
@@ -342,7 +342,6 @@ int drm_plane_colorop_curve_1d_lut_init(struct drm_device *dev, struct drm_color
colorop->lut1d_interpolation_property = prop;
drm_object_attach_property(&colorop->base, prop, interpolation);
- colorop->lut1d_interpolation = interpolation;
/* data */
ret = drm_colorop_create_data_prop(dev, colorop);
@@ -442,7 +441,6 @@ int drm_plane_colorop_3dlut_init(struct drm_device *dev, struct drm_colorop *col
colorop->lut3d_interpolation_property = prop;
drm_object_attach_property(&colorop->base, prop, interpolation);
- colorop->lut3d_interpolation = interpolation;
/* data */
ret = drm_colorop_create_data_prop(dev, colorop);
@@ -521,6 +519,20 @@ static void __drm_colorop_state_reset(struct drm_colorop_state *colorop_state,
&val);
colorop_state->curve_1d_type = val;
}
+
+ if (colorop->lut1d_interpolation_property) {
+ if (!drm_object_property_get_default_value(&colorop->base,
+ colorop->lut1d_interpolation_property,
+ &val))
+ colorop_state->lut1d_interpolation = val;
+ }
+
+ if (colorop->lut3d_interpolation_property) {
+ if (!drm_object_property_get_default_value(&colorop->base,
+ colorop->lut3d_interpolation_property,
+ &val))
+ colorop_state->lut3d_interpolation = val;
+ }
}
/**
diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
index 61cc8206b4c4..d5b45339333f 100644
--- a/include/drm/drm_colorop.h
+++ b/include/drm/drm_colorop.h
@@ -183,6 +183,20 @@ struct drm_colorop_state {
*/
struct drm_property_blob *data;
+ /**
+ * @lut1d_interpolation:
+ *
+ * Interpolation for DRM_COLOROP_1D_LUT
+ */
+ enum drm_colorop_lut1d_interpolation_type lut1d_interpolation;
+
+ /**
+ * @lut3d_interpolation:
+ *
+ * Interpolation for DRM_COLOROP_3D_LUT
+ */
+ enum drm_colorop_lut3d_interpolation_type lut3d_interpolation;
+
/** @state: backpointer to global drm_atomic_state */
struct drm_atomic_state *state;
};
@@ -306,20 +320,6 @@ struct drm_colorop {
*/
uint32_t size;
- /**
- * @lut1d_interpolation:
- *
- * Interpolation for DRM_COLOROP_1D_LUT
- */
- enum drm_colorop_lut1d_interpolation_type lut1d_interpolation;
-
- /**
- * @lut3d_interpolation:
- *
- * Interpolation for DRM_COLOROP_3D_LUT
- */
- enum drm_colorop_lut3d_interpolation_type lut3d_interpolation;
-
/**
* @lut1d_interpolation_property:
*
--
2.53.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v9 2/4] drm/colorop: make lut(1/3)d_interpolation props correctly behave as mutable
2026-06-09 10:20 ` [PATCH v9 2/4] drm/colorop: make lut(1/3)d_interpolation props correctly behave as mutable Melissa Wen
@ 2026-06-09 11:17 ` sashiko-bot
0 siblings, 0 replies; 8+ messages in thread
From: sashiko-bot @ 2026-06-09 11:17 UTC (permalink / raw)
To: Melissa Wen; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] The function `drm_colorop_atomic_destroy_state` frees the `drm_colorop_state` struct without releasing the reference to the `state->data` blob, leading to a memory leak of `drm_property_blob` objects.
- [High] `__drm_atomic_helper_colorop_duplicate_state` unconditionally resets `bypass = true`, destroying user state.
--
commit af8aea1831e57bd16ec95c73773d4e52e3faa54a
Author: Melissa Wen <mwen@igalia.com>
Subject: drm/colorop: make lut(1/3)d_interpolation props correctly behave as mutable
This commit moves the interpolation properties into drm_colorop_state so
they can correctly behave as mutable properties. It updates the getters,
setters, and state printing to use the state objects instead of the base
colorop.
> diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
> index 44eb823585d2e..5ab77cf47f0a9 100644
> --- a/drivers/gpu/drm/drm_colorop.c
> +++ b/drivers/gpu/drm/drm_colorop.c
[ ... ]
> @@ -488,6 +486,20 @@ static void __drm_colorop_state_reset(struct drm_colorop_state *colorop_state,
> &val);
> colorop_state->curve_1d_type = val;
> }
> +
> + if (colorop->lut1d_interpolation_property) {
> + if (!drm_object_property_get_default_value(&colorop->base,
> + colorop->lut1d_interpolation_property,
> + &val))
> + colorop_state->lut1d_interpolation = val;
> + }
> +
> + if (colorop->lut3d_interpolation_property) {
> + if (!drm_object_property_get_default_value(&colorop->base,
> + colorop->lut3d_interpolation_property,
> + &val))
> + colorop_state->lut3d_interpolation = val;
> + }
> }
[Severity: High]
This isn't a bug introduced by this patch, but does
drm_colorop_atomic_destroy_state() leak the state->data blob reference?
During atomic state duplication, a reference is taken on the data blob:
static void __drm_atomic_helper_colorop_duplicate_state(...)
{
...
if (state->data)
drm_property_blob_get(state->data);
...
}
However, the destruction function frees the state without dropping this
reference:
void drm_colorop_atomic_destroy_state(...)
{
kfree(state);
}
Could this leak system memory on every atomic commit that includes the
colorop? Is it possible this also happens in drm_colorop_reset() when it
calls kfree() on colorop->state?
[Severity: High]
This is also a pre-existing issue, but does
__drm_atomic_helper_colorop_duplicate_state() corrupt the user state by
forcing bypass to true?
static void __drm_atomic_helper_colorop_duplicate_state(...)
{
memcpy(state, colorop->state, sizeof(*state));
...
state->bypass = true;
}
If userspace configures a color pipeline with bypass = false, will a
subsequent unrelated atomic commit (like moving a cursor or doing a pageflip)
that duplicates the colorop state inadvertently reset bypass back to true?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260609110420.1298352-1-mwen@igalia.com?part=2
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v9 3/4] drm/atomic: track individual colorop updates
2026-06-09 10:20 [PATCH v9 0/4] drm/atomic: track individual colorop updates Melissa Wen
2026-06-09 10:20 ` [PATCH v9 1/4] drm/colorop: Remove read-only comments from interpolation fields Melissa Wen
2026-06-09 10:20 ` [PATCH v9 2/4] drm/colorop: make lut(1/3)d_interpolation props correctly behave as mutable Melissa Wen
@ 2026-06-09 10:20 ` Melissa Wen
2026-06-09 11:21 ` sashiko-bot
2026-06-09 10:20 ` [PATCH v9 4/4] drm/amd/display: use plane color_mgmt_changed to track colorop changes Melissa Wen
3 siblings, 1 reply; 8+ messages in thread
From: Melissa Wen @ 2026-06-09 10:20 UTC (permalink / raw)
To: airlied, alexander.deucher, alex.hung, christian.koenig, contact,
daniels, harry.wentland, louis.chauvet, maarten.lankhorst,
mripard, mwen, sebastian.wick, simona, siqueira, sunpeng.li,
tzimmermann
Cc: Uma Shankar, Chaitanya Kumar Borah, Xaver Hugl, Pekka Paalanen,
Matthew Schwartz, amd-gfx, kernel-dev, Rob Clark,
Dmitry Baryshkov, Sean Paul, Marijn Suijten, linux-arm-msm,
freedreno, intel-xe, intel-gfx, dri-devel
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. Since we're here, only announce and
track changes to plane COLOR_PIPELINE prop if its value is actually
changing.
Fixes: 8c5ea1745f4c ("drm/colorop: Add BYPASS property")
Fixes: 7fa3ee8c0a79 ("drm/colorop: Define LUT_1D interpolation")
Fixes: 41651f9d42eb ("drm/colorop: Add 1D Curve subtype")
Fixes: 3410108037d5 ("drm/colorop: Add multiplier type")
Fixes: db971856bbe0 ("drm/colorop: Add 3D LUT support to color pipeline")
Fixes: e5719e7f1900 ("drm/colorop: Add 3x4 CTM type")
Fixes: 99a4e4f08abe ("drm/colorop: Add 1D Curve Custom LUT type")
Fixes: 2afc3184f3b3 ("drm/plane: Add COLOR PIPELINE property")
Reviewed-by: Harry Wentland <harry.wentland@amd.com> #v1
Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
Reviewed-by: Alex Hung <alex.hung@amd.com>
Fixes: 9ba25915efba ("drm/amd/display: Add support for sRGB EOTF in DEGAM block")
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
drivers/gpu/drm/drm_atomic_uapi.c | 64 ++++++++++++++++++++++++-------
include/drm/drm_atomic_uapi.h | 4 +-
2 files changed, 54 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index b81dbba4d8c3..5eaf0e8a494b 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);
@@ -604,7 +612,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,
@@ -713,11 +721,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:
@@ -739,28 +747,45 @@ 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) {
- state->lut1d_interpolation = val;
+ if (state->lut1d_interpolation != val) {
+ state->lut1d_interpolation = val;
+ *replaced = true;
+ }
} 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) {
- state->lut3d_interpolation = val;
+ if (state->lut3d_interpolation != val) {
+ state->lut3d_interpolation = val;
+ *replaced = true;
+ }
} 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",
@@ -1275,8 +1300,10 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
break;
}
case DRM_MODE_OBJECT_COLOROP: {
+ struct drm_plane_state *plane_state;
struct drm_colorop *colorop = obj_to_colorop(obj);
struct drm_colorop_state *colorop_state;
+ bool replaced = false;
colorop_state = drm_atomic_get_colorop_state(state, colorop);
if (IS_ERR(colorop_state)) {
@@ -1285,7 +1312,18 @@ 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 |= replaced;
+
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] 8+ messages in thread* Re: [PATCH v9 3/4] drm/atomic: track individual colorop updates
2026-06-09 10:20 ` [PATCH v9 3/4] drm/atomic: track individual colorop updates Melissa Wen
@ 2026-06-09 11:21 ` sashiko-bot
0 siblings, 0 replies; 8+ messages in thread
From: sashiko-bot @ 2026-06-09 11:21 UTC (permalink / raw)
To: Melissa Wen; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Missing `async_flip` validation for `DRM_MODE_OBJECT_COLOROP` allows colorop properties to be modified during async page flips, resulting in silently discarded updates.
- [High] Leaked `drm_property_blob` references in colorop state destruction.
--
commit e431074495b7fd11806af7e6be2bb0ec2ea72838
Author: Melissa Wen <mwen@igalia.com>
drm/atomic: track individual colorop updates
As we do for CRTC color mgmt properties, this commit uses the
color_mgmt_changed flag to track value changes in the color pipeline
of a given plane. This allows drivers to update color blocks as soon
as plane color pipeline or individual colorop values change.
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index cd0fb5dc7d3ec..f799863c350c7 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
[ ... ]
> @@ -1271,8 +1296,10 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
> break;
> }
> case DRM_MODE_OBJECT_COLOROP: {
> + struct drm_plane_state *plane_state;
> struct drm_colorop *colorop = obj_to_colorop(obj);
[Severity: High]
This isn't a bug introduced by this patch, but does this block lack
validation for async_flip?
Unlike the blocks for planes, CRTCs, and connectors, there is no check for
async_flip here. If userspace submits an atomic commit with
DRM_MODE_PAGE_FLIP_ASYNC that modifies colorop properties, the ioctl
succeeds and modifies colorop_state inside the transaction.
However, drm_atomic_helper_async_commit() completely ignores colorops and
does not swap state->colorops. Could this lead to the updated state being
silently discarded and never applied to hardware, causing state divergence
between userspace expectations and kernel state?
> struct drm_colorop_state *colorop_state;
> + bool replaced = false;
>
> colorop_state = drm_atomic_get_colorop_state(state, colorop);
> if (IS_ERR(colorop_state)) {
[Severity: High]
This is a pre-existing issue, but is there a leaked drm_property_blob
reference in colorop state destruction?
Looking at drm_colorop_atomic_destroy_state() in
drivers/gpu/drm/drm_colorop.c:
void drm_colorop_atomic_destroy_state(struct drm_colorop *colorop,
struct drm_colorop_state *state)
{
kfree(state);
}
When an atomic state is committed or aborted, the atomic framework frees the
associated state objects by calling their respective destroy functions. This
function simply calls kfree(state) without dropping the reference to the data
blob (state->data) via drm_property_blob_put().
Because drm_atomic_helper_colorop_duplicate_state() explicitly increments the
blob's reference count, and property setting acquires new blob references,
could failing to release the blob reference during state teardown lead to an
unbounded memory leak for every atomic update touching the color pipeline?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260609110420.1298352-1-mwen@igalia.com?part=3
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v9 4/4] drm/amd/display: use plane color_mgmt_changed to track colorop changes
2026-06-09 10:20 [PATCH v9 0/4] drm/atomic: track individual colorop updates Melissa Wen
` (2 preceding siblings ...)
2026-06-09 10:20 ` [PATCH v9 3/4] drm/atomic: track individual colorop updates Melissa Wen
@ 2026-06-09 10:20 ` Melissa Wen
2026-06-09 11:17 ` sashiko-bot
3 siblings, 1 reply; 8+ messages in thread
From: Melissa Wen @ 2026-06-09 10:20 UTC (permalink / raw)
To: airlied, alexander.deucher, alex.hung, christian.koenig, contact,
daniels, harry.wentland, louis.chauvet, maarten.lankhorst,
mripard, mwen, sebastian.wick, simona, siqueira, sunpeng.li,
tzimmermann
Cc: Uma Shankar, Chaitanya Kumar Borah, Xaver Hugl, Pekka Paalanen,
Matthew Schwartz, amd-gfx, kernel-dev, Rob Clark,
Dmitry Baryshkov, Sean Paul, Marijn Suijten, linux-arm-msm,
freedreno, intel-xe, intel-gfx, dri-devel
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.
Fixes: 9ba25915efba ("drm/amd/display: Add support for sRGB EOTF in DEGAM block")
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
Reviewed-by: Alex Hung <alex.hung@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 e96a12ff2d31..d3237f61246c 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -10067,7 +10067,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;
@@ -11808,6 +11808,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] 8+ messages in thread* Re: [PATCH v9 4/4] drm/amd/display: use plane color_mgmt_changed to track colorop changes
2026-06-09 10:20 ` [PATCH v9 4/4] drm/amd/display: use plane color_mgmt_changed to track colorop changes Melissa Wen
@ 2026-06-09 11:17 ` sashiko-bot
0 siblings, 0 replies; 8+ messages in thread
From: sashiko-bot @ 2026-06-09 11:17 UTC (permalink / raw)
To: Melissa Wen; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The logic in `should_reset_plane` fails to trigger a reset for all planes on a CRTC when another plane's color management changes via the new DRM Color Pipeline API, leading to plane z-order corruption.
--
commit 52aefc9e9f5291e7e9724eb87eff29b790339396
Author: Melissa Wen <mwen@igalia.com>
drm/amd/display: use plane color_mgmt_changed to track colorop changes
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.
> 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 8f80420e9f0b8..56ba9bf40ed9f 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
[ ... ]
> @@ -12208,6 +12208,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
[Severity: High]
Does this code need to check color_mgmt_changed for other planes on the
CRTC as well?
Looking further down in should_reset_plane(), there is a loop over
other planes that checks legacy AMD color properties, but it misses checking
new_other_state->color_mgmt_changed:
for_each_oldnew_plane_in_state(state, other, old_other_state, new_other_state, i) {
...
/* HDR/Transfer Function changes. */
if (dm_old_other_state->degamma_tf != dm_new_other_state->degamma_tf ||
...
If Plane A has color_mgmt_changed == true, should_reset_plane() correctly
returns true and Plane A gets reset.
However, when should_reset_plane() is evaluated for Plane B on the same CRTC,
it iterates over the other planes (including Plane A). Since the loop does
not check Plane A's color_mgmt_changed state, it could return false for Plane B.
Could this cause only some planes on the CRTC to be removed and re-added,
resulting in a scrambled hardware z-order or pipeline acquisition errors?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260609110420.1298352-1-mwen@igalia.com?part=4
^ permalink raw reply [flat|nested] 8+ messages in thread