[PATCH v2 0/2] drm/amd/display: don't overwrite regamma LUT with empty data

[PATCH v2 0/2] drm/amd/display: don't overwrite regamma LUT with empty data

[Melissa Wen]

Hi,

Xaver first reported flickering when changing GAMMA_LUT often in KDE
with an HDR-enabled display:
https://gitlab.freedesktop.org/drm/amd/-/issues/4444

This issue is reproducible on Fedora 42 from [1], with integrated and/or
external monitors. I was able to reproduce it on DCN3.01,  but other
users also reported similar issue with DCN3 and DCN4 families. Besides
that, when setting Night Light mode and changing a Display setting, such
as Overscan, colors on the screen look like gamma LUT was lost (and it
was).

After further investigation, the root cause of all these problems
appears to be the way DM updates the DC stream's color state at
`atomic_check`, allowing test-only commit settings to override
non-blocking commit settings before it is committed in `commit_tail`.
Xaver explained that KWin always performs a test commit without the LUT,
then another with the LUT, and finally a non-test-only commit with the
same LUT value still set. This sequence makes the issue more likely to
occur, as the commit_tail of a non-blocking commit can delay until a
subsequent test-only commit without the LUT "removes" the gamma LUT set
by the non-blocking commit at `atomic_check` from the DC stream's state.

I first tried to address this issue by only programming output transfer
func if and only if out_tf flag is set (v1 [2]), but this approach
doesn't solve all problems, it only addressed the gamma LUT loss in the
case of Night Light mode + Display settings change. Therefore, this
version targets the same problem but with a completly different
approach.

This series resolves all reported issues by moving DC stream color
changes from `atomic_check` to `atomic_setup_commit`, preventing
test-only commit CRTC color properties from being programmed into
`commit_tail`. I see this change in line with the kernel-doc on
amdgpu_dm_atomic_check(), which says: "It is important not to modify the
existing DC state.  Otherwise, atomic_check may unexpectedly commit
hardware changes."

Adding here a shortened trace that exemplifies this bad sequence with
some custom printk to track color settings in `drm_atomic_state`:

[  +0.000046] DRM: non block commit call: begin
[  +0.000003] amdgpu 0000:04:00.0: [drm:drm_atomic_check_only [drm]] checking 00000000183d1556 <-- non-blocking commit
[  +0.000053] amdgpu: AMD DM: atomic_check: CHECK color mgmt: color change: 1, AMD regamma Tf update: 0, needs modeset: 0 for drm_atomic_state: 00000000183d1556
[  +0.000005] amdgpu: program atomic regamma? 1 for drm_atomic_state: 00000000183d1556
[  +0.000003] amdgpu: AMD DM: set_atomic_regamma: PROGRAM regamma
[  +0.000699] amdgpu 0000:04:00.0: [drm:drm_atomic_nonblocking_commit [drm]] committing 00000000183d1556 nonblocking
[  +0.000076] DRM: non block commit call: end
[  +0.000002] amdgpu 0000:04:00.0: [drm:drm_atomic_get_connector_state [drm]] Added [CONNECTOR:93:eDP-1] 00000000c74c75cc state to 00000000bce01e59 <-- test-only commit
[[  +0.000046] amdgpu 0000:04:00.0: [drm:drm_atomic_get_crtc_state [drm]] Added [CRTC:79:crtc-0] 000000004fe38ea2 state to 00000000bce01e59
[  +0.000048] amdgpu 0000:04:00.0: [drm:drm_atomic_get_plane_state [drm]] Added [PLANE:76:plane-6] 00000000a3ba0680 state to 00000000bce01e59
[  -0.000252] amdgpu 0000:04:00.0: [drm:amdgpu_dm_atomic_commit_tail [amdgpu]] amdgpu_crtc id:0 crtc_state_flags: enable:1, active:1, planes_changed:1, mode_changed:0,active_changed:0,connectors_changed:0
[  +0.000045] amdgpu 0000:04:00.0: [drm:drm_atomic_get_plane_state [drm]] Added [PLANE:58:plane-3] 0000000007ae0fe5 state to 00000000bce01e59
[  +0.000073] DRM: color mgmt changed for DEGAMMA? 0
[  +0.000054] DRM: color mgmt changed for CTM? 0
[  +0.000314] DRM: color mgmt changed for REGAMMA? 1
[  +0.000004] DRM: TEST ONLY call: begin
[  +0.000003] amdgpu 0000:04:00.0: [drm:drm_atomic_check_only [drm]] checking 00000000bce01e59 <-- test-only commit
[  +0.000055] amdgpu: AMD DM: atomic check: CHECK color mgmt: color change: 1, AMD regamma Tf update: 0, needs modeset: 0 for drm_atomic_state: 00000000bce01e59
[  +0.000005] amdgpu: program atomic regamma? 0 for drm_atomic_state 00000000bce01e59
[  +0.000003] amdgpu: AMD DM: set atomic regamma: REMOVE regamma
[  +0.000247] amdgpu 0000:04:00.0: [drm:amdgpu_dm_atomic_commit_tail [amdgpu]] [HDCP_DM] pipe_ctx dispname=ANX7530 U
[  +0.000441] amdgpu 0000:04:00.0: [drm:amdgpu_dm_atomic_commit_tail [amdgpu]] crtc:0, pflip_stat:AMDGPU_FLIP_SUBMITTED
[  +0.000731] amdgpu: AMD DM: commit planes: color mgmt changed, out tf type: 2 for drm_atomic_commit 00000000183d1556 <-- non-blocking commit taking out TF = BYPASS
[  +0.000570] DC: set_output_transfer_func? enable: 0, out_tf:1
[  +0.000005] DC: set_output_transfer_func: begin
[  +0.000004] DC: set_output_transfer_func: configure output gamma: 2 <-- BYPASS
[  +0.000004] DC: set_output_gamma: begin
[  +0.000002] DC: set_output_gamma: DISABLE OGAM
[  +0.000107] amdgpu 0000:04:00.0: [drm:drm_atomic_state_default_clear [drm]] Clearing atomic state 00000000183d1556
[  +0.000053] DRM: TEST ONLY call: end
[  +0.000008] amdgpu 0000:04:00.0: [drm:drm_atomic_state_default_clear [drm]] Clearing atomic state 00000000bce01e59
[  +0.000012] amdgpu 0000:04:00.0: [drm:__drm_atomic_state_free [drm]] Freeing atomic state 00000000bce01e59
[  +0.000085] amdgpu 0000:04:00.0: [drm:__drm_atomic_state_free [drm]] Freeing atomic state 00000000183d1556

I hope this trace can illustrate the situation.
 
[1] https://bodhi.fedoraproject.org/updates/FEDORA-2025-b58c14c454

[2] https://lore.kernel.org/amd-gfx/20250822211552.1472375-1-mwen@igalia.com/

PS: maybe we should do a follow-up refactor in the
amdgpu_dm_atomic_check() and move other DC state changes from
`atomic_check` to `atomic_setup_commit`.

Best Regards,

Melissa

Melissa Wen (2):
  drm/amd/display: update color on atomic commit time
  drm/amd/display: change dc stream color settings only in atomic commit

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  38 ++++++-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   1 +
 .../amd/display/amdgpu_dm/amdgpu_dm_color.c   | 105 +++++++++++++++++-
 3 files changed, 139 insertions(+), 5 deletions(-)

-- 
2.47.2

[PATCH v2 1/2] drm/amd/display: update color on atomic commit time

[Melissa Wen]

Use `atomic_commit_setup` to change the DC stream state. It's a
preparation to remove from `atomic_check` changes in CRTC color
components of DC stream state and prevent DC to commit TEST_ONLY
changes.

Link: https://gitlab.freedesktop.org/drm/amd/-/issues/4444
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 36 ++++++++++++++++++-
 1 file changed, 35 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 242f98564261..9bd82e04fe5c 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -233,6 +233,7 @@ static int amdgpu_dm_encoder_init(struct drm_device *dev,
 
 static int amdgpu_dm_connector_get_modes(struct drm_connector *connector);
 
+static int amdgpu_dm_atomic_setup_commit(struct drm_atomic_state *state);
 static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state);
 
 static int amdgpu_dm_atomic_check(struct drm_device *dev,
@@ -3651,7 +3652,7 @@ static const struct drm_mode_config_funcs amdgpu_dm_mode_funcs = {
 
 static struct drm_mode_config_helper_funcs amdgpu_dm_mode_config_helperfuncs = {
 	.atomic_commit_tail = amdgpu_dm_atomic_commit_tail,
-	.atomic_commit_setup = drm_dp_mst_atomic_setup_commit,
+	.atomic_commit_setup = amdgpu_dm_atomic_setup_commit,
 };
 
 static void update_connector_ext_caps(struct amdgpu_dm_connector *aconnector)
@@ -10279,6 +10280,39 @@ static void amdgpu_dm_update_hdcp(struct drm_atomic_state *state)
 	}
 }
 
+static int amdgpu_dm_atomic_setup_commit(struct drm_atomic_state *state)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+	struct dm_crtc_state *dm_old_crtc_state, *dm_new_crtc_state;
+	int i, ret;
+
+	ret = drm_dp_mst_atomic_setup_commit(state);
+	if (ret)
+		return ret;
+
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
+		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
+		/*
+		 * Color management settings. We also update color properties
+		 * when a modeset is needed, to ensure it gets reprogrammed.
+		 */
+		if (dm_new_crtc_state->base.active && dm_new_crtc_state->stream &&
+		    (dm_new_crtc_state->base.color_mgmt_changed ||
+		     dm_old_crtc_state->regamma_tf != dm_new_crtc_state->regamma_tf ||
+		     drm_atomic_crtc_needs_modeset(new_crtc_state))) {
+			ret = amdgpu_dm_update_crtc_color_mgmt(dm_new_crtc_state);
+			if (ret) {
+				pr_info("BUG when updating crtc color\n");
+				return ret;
+			}
+		}
+	}
+
+	return 0;
+}
+
 /**
  * amdgpu_dm_atomic_commit_tail() - AMDgpu DM's commit tail implementation.
  * @state: The atomic state to commit
-- 
2.47.2

[PATCH v2 2/2] drm/amd/display: change dc stream color settings only in atomic commit

[Melissa Wen]

Don't update DC stream color components during atomic check. The driver
will continue validating the new CRTC color state but will not change DC
stream color components. The DC stream color state will only be
programmed at commit time in the `atomic_setup_commit` stage.

It fixes gamma LUT loss reported by KDE users when changing brightness
quickly or changing Display settings (such as overscan) with nightlight
on and HDR. As KWin can do a test commit with color settings different
from those that should be applied in a non-test-only commit, if the
driver changes DC stream color state in atomic check, this state can be
eventually HW programmed in commit tail, instead of the respective state
set by the non-blocking commit.

Link: https://gitlab.freedesktop.org/drm/amd/-/issues/4444
Reported-by: Xaver Hugl <xaver.hugl@gmail.com>
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |   2 +-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   1 +
 .../amd/display/amdgpu_dm/amdgpu_dm_color.c   | 105 +++++++++++++++++-
 3 files changed, 104 insertions(+), 4 deletions(-)

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 9bd82e04fe5c..ba40346eaf95 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -11125,7 +11125,7 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
 	if (dm_new_crtc_state->base.color_mgmt_changed ||
 	    dm_old_crtc_state->regamma_tf != dm_new_crtc_state->regamma_tf ||
 	    drm_atomic_crtc_needs_modeset(new_crtc_state)) {
-		ret = amdgpu_dm_update_crtc_color_mgmt(dm_new_crtc_state);
+		ret = amdgpu_dm_check_crtc_color_mgmt(dm_new_crtc_state);
 		if (ret)
 			goto fail;
 	}
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index ce74125c713e..1cc3d83e377a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -1041,6 +1041,7 @@ void amdgpu_dm_init_color_mod(void);
 int amdgpu_dm_create_color_properties(struct amdgpu_device *adev);
 int amdgpu_dm_verify_lut_sizes(const struct drm_crtc_state *crtc_state);
 int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc);
+int amdgpu_dm_check_crtc_color_mgmt(struct dm_crtc_state *crtc);
 int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc,
 				      struct drm_plane_state *plane_state,
 				      struct dc_plane_state *dc_plane_state);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
index c7387af725d6..a7cfcdba1fc9 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
@@ -566,12 +566,11 @@ static int __set_output_tf(struct dc_transfer_func *func,
 	return res ? 0 : -ENOMEM;
 }
 
-static int amdgpu_dm_set_atomic_regamma(struct dc_stream_state *stream,
+static int amdgpu_dm_set_atomic_regamma(struct dc_transfer_func *out_tf,
 					const struct drm_color_lut *regamma_lut,
 					uint32_t regamma_size, bool has_rom,
 					enum dc_transfer_func_predefined tf)
 {
-	struct dc_transfer_func *out_tf = &stream->out_transfer_func;
 	int ret = 0;
 
 	if (regamma_size || tf != TRANSFER_FUNCTION_LINEAR) {
@@ -969,7 +968,7 @@ int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc)
 			return r;
 	} else {
 		regamma_size = has_regamma ? regamma_size : 0;
-		r = amdgpu_dm_set_atomic_regamma(stream, regamma_lut,
+		r = amdgpu_dm_set_atomic_regamma(&stream->out_transfer_func, regamma_lut,
 						 regamma_size, has_rom, tf);
 		if (r)
 			return r;
@@ -1008,6 +1007,106 @@ int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc)
 	return 0;
 }
 
+/**
+ * amdgpu_dm_check_crtc_color_mgmt: Check if DRM color props are programmable by DC.
+ * @crtc: amdgpu_dm crtc state
+ *
+ * This function just verifies CRTC LUT sizes, if there is enough space for
+ * output transfer function and if its parameters can be calculated by AMD
+ * color module. It also adjusts some settings for programming CRTC degamma at
+ * plane stage, using plane DGM block.
+ *
+ * The RGM block is typically more fully featured and accurate across
+ * all ASICs - DCE can't support a custom non-linear CRTC DGM.
+ *
+ * For supporting both plane level color management and CRTC level color
+ * management at once we have to either restrict the usage of some CRTC
+ * properties or blend adjustments together.
+ *
+ * Returns:
+ * 0 on success. Error code if validation fails.
+ */
+
+int amdgpu_dm_check_crtc_color_mgmt(struct dm_crtc_state *crtc)
+{
+	struct amdgpu_device *adev = drm_to_adev(crtc->base.state->dev);
+	bool has_rom = adev->asic_type <= CHIP_RAVEN;
+	const struct drm_color_lut *degamma_lut, *regamma_lut;
+	uint32_t degamma_size, regamma_size;
+	bool has_regamma, has_degamma;
+	struct dc_transfer_func *out_tf;
+	enum dc_transfer_func_predefined tf = TRANSFER_FUNCTION_LINEAR;
+	bool is_legacy;
+	int r;
+
+	tf = amdgpu_tf_to_dc_tf(crtc->regamma_tf);
+
+	r = amdgpu_dm_verify_lut_sizes(&crtc->base);
+	if (r)
+		return r;
+
+	degamma_lut = __extract_blob_lut(crtc->base.degamma_lut, &degamma_size);
+	regamma_lut = __extract_blob_lut(crtc->base.gamma_lut, &regamma_size);
+
+	has_degamma =
+		degamma_lut && !__is_lut_linear(degamma_lut, degamma_size);
+
+	has_regamma =
+		regamma_lut && !__is_lut_linear(regamma_lut, regamma_size);
+
+	is_legacy = regamma_size == MAX_COLOR_LEGACY_LUT_ENTRIES;
+
+	/* Reset all adjustments. */
+	crtc->cm_has_degamma = false;
+	crtc->cm_is_degamma_srgb = false;
+
+	out_tf = kzalloc(sizeof(*out_tf), GFP_KERNEL);
+	if (!out_tf)
+		return -ENOMEM;
+
+	/* Setup regamma and degamma. */
+	if (is_legacy) {
+		/*
+		 * Legacy regamma forces us to use the sRGB RGM as a base.
+		 * This also means we can't use linear DGM since DGM needs
+		 * to use sRGB as a base as well, resulting in incorrect CRTC
+		 * DGM and CRTC CTM.
+		 *
+		 * TODO: Just map this to the standard regamma interface
+		 * instead since this isn't really right. One of the cases
+		 * where this setup currently fails is trying to do an
+		 * inverse color ramp in legacy userspace.
+		 */
+		crtc->cm_is_degamma_srgb = true;
+		out_tf->type = TF_TYPE_DISTRIBUTED_POINTS;
+		out_tf->tf = TRANSFER_FUNCTION_SRGB;
+		/*
+		 * Note: although we pass has_rom as parameter here, we never
+		 * actually use ROM because the color module only takes the ROM
+		 * path if transfer_func->type == PREDEFINED.
+		 *
+		 * See more in mod_color_calculate_regamma_params()
+		 */
+		r = __set_legacy_tf(out_tf, regamma_lut,
+				    regamma_size, has_rom);
+	} else {
+		regamma_size = has_regamma ? regamma_size : 0;
+		r = amdgpu_dm_set_atomic_regamma(out_tf, regamma_lut,
+						 regamma_size, has_rom, tf);
+	}
+
+	/*
+	 * CRTC DGM goes into DGM LUT. It would be nice to place it
+	 * into the RGM since it's a more featured block but we'd
+	 * have to place the CTM in the OCSC in that case.
+	 */
+	crtc->cm_has_degamma = has_degamma;
+	dc_transfer_func_release(out_tf);
+
+	return r;
+}
+
+
 static int
 map_crtc_degamma_to_dc_plane(struct dm_crtc_state *crtc,
 			     struct dc_plane_state *dc_plane_state,
-- 
2.47.2