[PATCH 0/2] drm/amd/display: Keep FreeSync flip timing across plane resets

[PATCH 0/2] drm/amd/display: Keep FreeSync flip timing across plane resets

[Davey Hughes via B4 Relay]

On a 48-165 Hz FreeSync panel, fullscreen clients under KWin with HDR
enabled and color accuracy set to "Prefer efficiency" run at about 51
fps instead of 60, with flips locked to the 13.45 ms BTR mid-point
grid. That grid drifts upward at the rate expected if FreeSync takes
the 32-bit microsecond timestamp itself as the last render time (3.46
us/s measured, 3.45 predicted).

KWin replaces the CRTC GAMMA_LUT blob on every commit in that mode, so
every flip resets the plane and the new DC plane starts with zeroed
flip timing. Patch 1 carries the timing over from the old DC plane, as
DM already does for flip_addr
(amdgpu_dm_plane_drm_plane_duplicate_state()) and for the BTR state
itself (dm_irq_params.vrr_params). Patch 2 adds KUnit tests, kept
separate so patch 1 backports to stable trees without the KUnit
infrastructure.

Tested with KUnit (UML, all AMD display suites pass; the new timing
tests fail without the fix) and on 7.2.11 with a Navi 31, an AW3423DWF
over DP, KWin 6.8.1 and Mesa 26.2.5: 60 fps content now flips at a
steady 16.65 ms instead of on the 13.45 ms grid, 30 fps content still
gets BTR (steady 33.2 ms), and debugfs shows color_mgmt_changed=1 on
the CRTC in every sampled commit. Not tested: DCE, overlay planes,
other compositors.

Open question: the copy happens in atomic check, so a blocking commit
that resets the plane while a flip is still in flight can copy timing
that is one flip old (at most one BTR frame). Copying in
amdgpu_dm_commit_planes() before the preflip would avoid that; I can
send it as v2 if you prefer.

I am not sure of the current AI disclosure policy, but I will freely
mention that I used Claude Opus 5.5 to help debug and write this patch +
tests. I have tried to do my best to match existing style/procedures.

---
Davey Hughes (2):
      drm/amd/display: Keep FreeSync flip timing across plane resets
      drm/amd/display: Test FreeSync flip timing across plane resets

 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  |  17 ++
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   3 +
 .../amd/display/amdgpu_dm/tests/amdgpu_dm_test.c   | 190 +++++++++++++++++++++
 3 files changed, 210 insertions(+)
---
base-commit: 77f548b127c6de34ce271fa09ba270cb1ef9f2da
change-id: 20261027-freesync-plane-reset-time-5db657eebfde

Best regards,
--  
Davey Hughes <davidralphhughes@gmail.com>

[PATCH 1/2] drm/amd/display: Keep FreeSync flip timing across plane resets

[Davey Hughes via B4 Relay]

From: Davey Hughes <davidralphhughes@gmail.com>

dm_update_plane_state() replaces the DC plane with a newly allocated one
whenever the plane is reset. The new plane's flip timing is zero, and
mod_freesync_handle_preflip() runs before DC stores the new plane's
first flip timestamp, so FreeSync takes the 32-bit microsecond timestamp
itself as the last render time: the uptime modulo about 71.6 minutes.

On a panel with below-the-range compensation (BTR), that engages BTR and
pins the frame time at the BTR mid-point (13.447 ms on a 48-165 Hz
panel). A panel without BTR (max refresh below twice the min) goes
through the fixed refresh path instead and locks to its max refresh
after six frames.

It takes a reset on every flip to keep the panel there, and color
management changes reset the plane. DRM sets color_mgmt_changed whenever
a color property is given a different blob, even one with the same
contents, and KWin with HDR enabled and its color accuracy setting at
"Prefer efficiency" sets a new GAMMA_LUT blob on every commit. Plane
resets for CRTC color changes, added by
commit 7316c4ad2996 ("drm/amd/display: Reset planes for color management
changes"), and later for plane color properties and colorops, made this
reachable on every flip.

Copy the flip timing from the old DC plane into the new one when the
plane stays on the same CRTC. This covers both the BTR and the fixed
refresh path. A plane that is newly enabled or moved to another CRTC
still starts without history.

Fixes: 180db303ff46 ("drm/amd/display: Add below the range support for FreeSync")
Cc: stable@vger.kernel.org
Signed-off-by: Davey Hughes <davidralphhughes@gmail.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++++
 1 file changed, 5 insertions(+)

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 bdabb4bf14da..a69e28685857 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6076,6 +6076,11 @@ dm_update_plane_state(struct dc *dc,
 			goto out;
 		}
 
+		/* FreeSync times each frame from the plane's previous flip. */
+		if (dm_old_plane_state->dc_state &&
+		    old_plane_crtc == new_plane_crtc)
+			dc_new_plane_state->time = dm_old_plane_state->dc_state->time;
+
 		drm_dbg_atomic(new_plane_crtc->dev, "Enabling DRM plane: %d on DRM crtc %d\n",
 				 plane->base.id, new_plane_crtc->base.id);
 

-- 
2.56.0

[PATCH 2/2] drm/amd/display: Test FreeSync flip timing across plane resets

[Davey Hughes via B4 Relay]

From: Davey Hughes <davidralphhughes@gmail.com>

KUnit can't reach the plane enable path of dm_update_plane_state(), so
the flip timing copy there is untested.

Move the copy into dm_plane_inherit_flip_time() and expose it to KUnit.
Test that a plane reset on the same CRTC keeps its flip timing, and that
a newly enabled plane or one moved to another CRTC starts over.

Also flip a reset plane one 60 Hz frame after its last flip on a
48-165 Hz panel with BTR. With the inherited timing BTR stays off;
without it FreeSync engages BTR.

Signed-off-by: Davey Hughes <davidralphhughes@gmail.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  |  20 ++-
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   3 +
 .../amd/display/amdgpu_dm/tests/amdgpu_dm_test.c   | 190 +++++++++++++++++++++
 3 files changed, 209 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 a69e28685857..d1c1b01584ae 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5966,6 +5966,20 @@ STATIC_IFN_KUNIT bool should_reset_plane(struct drm_atomic_commit *state,
 }
 EXPORT_IF_KUNIT(should_reset_plane);
 
+/* FreeSync times each frame from the plane's previous flip. */
+STATIC_IFN_KUNIT void
+dm_plane_inherit_flip_time(struct dc_plane_state *dc_plane_state,
+			   struct drm_plane_state *old_plane_state,
+			   struct drm_plane_state *new_plane_state)
+{
+	struct dc_plane_state *old_dc_plane_state =
+		to_dm_plane_state(old_plane_state)->dc_state;
+
+	if (old_dc_plane_state && old_plane_state->crtc == new_plane_state->crtc)
+		dc_plane_state->time = old_dc_plane_state->time;
+}
+EXPORT_IF_KUNIT(dm_plane_inherit_flip_time);
+
 STATIC_IFN_KUNIT int
 dm_update_plane_state(struct dc *dc,
 		      struct drm_atomic_commit *state,
@@ -6076,10 +6090,8 @@ dm_update_plane_state(struct dc *dc,
 			goto out;
 		}
 
-		/* FreeSync times each frame from the plane's previous flip. */
-		if (dm_old_plane_state->dc_state &&
-		    old_plane_crtc == new_plane_crtc)
-			dc_new_plane_state->time = dm_old_plane_state->dc_state->time;
+		dm_plane_inherit_flip_time(dc_new_plane_state, old_plane_state,
+					   new_plane_state);
 
 		drm_dbg_atomic(new_plane_crtc->dev, "Enabling DRM plane: %d on DRM crtc %d\n",
 				 plane->base.id, new_plane_crtc->base.id);
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 594d275918d5..18dce8244af1 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -1266,6 +1266,9 @@ int dm_update_plane_state(struct dc *dc,
 			  bool enable,
 			  bool *lock_and_validation_needed,
 			  bool *is_top_most_overlay);
+void dm_plane_inherit_flip_time(struct dc_plane_state *dc_plane_state,
+				struct drm_plane_state *old_plane_state,
+				struct drm_plane_state *new_plane_state);
 int amdgpu_dm_atomic_check(struct drm_device *dev,
 			   struct drm_atomic_commit *state);
 void amdgpu_dm_mod_power_update_streams(struct drm_atomic_commit *state,
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_test.c
index a497cce332bb..4bdb9831f508 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_test.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_test.c
@@ -34,6 +34,7 @@
 #include "atom.h"
 #include "amdgpu_dm.h"
 #include "amdgpu_dm_audio.h"
+#include "amdgpu_dm_freesync.h"
 #include "amdgpu_dm_hdcp.h"
 #include "amdgpu_dm_mst_types.h"
 #include "amdgpu_dm_kunit_test_helpers.h"
@@ -3271,6 +3272,190 @@ static void dm_test_update_plane_state_detached(struct kunit *test)
 	KUNIT_EXPECT_NULL(test, new_plane_state->dc_state);
 }
 
+/* Tests for dm_plane_inherit_flip_time() */
+
+struct dm_test_flip_time_ctx {
+	struct dm_plane_state *old_plane_state;
+	struct dm_plane_state *new_plane_state;
+	struct dc_plane_state *dc_plane_state;
+};
+
+/*
+ * A plane reset on one CRTC: the old DC plane has FreeSync flip history and
+ * the new one is freshly created.
+ */
+static struct dm_test_flip_time_ctx *
+dm_test_flip_time_ctx_alloc(struct kunit *test)
+{
+	struct dm_test_flip_time_ctx *ctx;
+	struct dc_plane_state *old_dc;
+	struct drm_crtc *crtc;
+
+	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, ctx);
+	ctx->old_plane_state = kunit_kzalloc(test, sizeof(*ctx->old_plane_state),
+					     GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, ctx->old_plane_state);
+	ctx->new_plane_state = kunit_kzalloc(test, sizeof(*ctx->new_plane_state),
+					     GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, ctx->new_plane_state);
+	ctx->dc_plane_state = kunit_kzalloc(test, sizeof(*ctx->dc_plane_state),
+					    GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, ctx->dc_plane_state);
+	old_dc = kunit_kzalloc(test, sizeof(*old_dc), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, old_dc);
+	crtc = kunit_kzalloc(test, sizeof(*crtc), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, crtc);
+
+	old_dc->time.time_elapsed_in_us[0] = 16667;
+	old_dc->time.index = 1;
+	old_dc->time.prev_update_time_in_us = 1234567;
+	ctx->old_plane_state->dc_state = old_dc;
+	ctx->old_plane_state->base.crtc = crtc;
+	ctx->new_plane_state->base.crtc = crtc;
+
+	return ctx;
+}
+
+static void dm_test_flip_time_inherit(struct dm_test_flip_time_ctx *ctx)
+{
+	dm_plane_inherit_flip_time(ctx->dc_plane_state,
+				   &ctx->old_plane_state->base,
+				   &ctx->new_plane_state->base);
+}
+
+/**
+ * dm_test_flip_time_same_crtc - Test a reset plane keeps its flip history
+ * @test: The KUnit test context
+ */
+static void dm_test_flip_time_same_crtc(struct kunit *test)
+{
+	struct dm_test_flip_time_ctx *ctx = dm_test_flip_time_ctx_alloc(test);
+
+	dm_test_flip_time_inherit(ctx);
+
+	KUNIT_EXPECT_MEMEQ(test, &ctx->dc_plane_state->time,
+			   &ctx->old_plane_state->dc_state->time,
+			   sizeof(ctx->dc_plane_state->time));
+}
+
+/**
+ * dm_test_flip_time_crtc_changed - Test a plane moved between CRTCs starts over
+ * @test: The KUnit test context
+ */
+static void dm_test_flip_time_crtc_changed(struct kunit *test)
+{
+	struct dm_test_flip_time_ctx *ctx = dm_test_flip_time_ctx_alloc(test);
+	struct dc_plane_flip_time *time = &ctx->dc_plane_state->time;
+	struct drm_crtc *other_crtc;
+
+	other_crtc = kunit_kzalloc(test, sizeof(*other_crtc), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, other_crtc);
+	ctx->new_plane_state->base.crtc = other_crtc;
+
+	dm_test_flip_time_inherit(ctx);
+
+	KUNIT_EXPECT_EQ(test, time->prev_update_time_in_us, 0);
+	KUNIT_EXPECT_EQ(test, time->index, 0);
+}
+
+/**
+ * dm_test_flip_time_no_old_dc_state - Test a newly enabled plane starts over
+ * @test: The KUnit test context
+ */
+static void dm_test_flip_time_no_old_dc_state(struct kunit *test)
+{
+	struct dm_test_flip_time_ctx *ctx = dm_test_flip_time_ctx_alloc(test);
+	struct dc_plane_flip_time *time = &ctx->dc_plane_state->time;
+
+	ctx->old_plane_state->dc_state = NULL;
+
+	dm_test_flip_time_inherit(ctx);
+
+	KUNIT_EXPECT_EQ(test, time->prev_update_time_in_us, 0);
+	KUNIT_EXPECT_EQ(test, time->index, 0);
+}
+
+/*
+ * Flip the reset plane one 60 Hz frame after the old plane's last flip on a
+ * 48-165 Hz panel with below-the-range compensation, and report whether
+ * FreeSync engaged BTR.
+ */
+static bool dm_test_flip_time_btr_active(struct kunit *test,
+					 struct dm_test_flip_time_ctx *ctx)
+{
+	struct amdgpu_device *adev = dm_kunit_alloc_adev(test);
+	struct dc_stream_state *stream = dm_kunit_alloc_stream(test, NULL);
+	struct dm_crtc_state *crtc_state;
+	struct mod_vrr_params *vrr;
+	struct amdgpu_crtc *acrtc;
+	u32 flip_time_in_us;
+
+	acrtc = kunit_kzalloc(test, sizeof(*acrtc), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, acrtc);
+	crtc_state = kunit_kzalloc(test, sizeof(*crtc_state), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, crtc_state);
+	adev->dm.adev = adev;
+	/* Only pre-AI flips dereference the module, so a stub will do. */
+	adev->family = AMDGPU_FAMILY_NV;
+	adev->dm.freesync_module = kunit_kzalloc(test, sizeof(struct mod_freesync),
+						 GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, adev->dm.freesync_module);
+
+	/* 165 Hz nominal */
+	stream->ctx = dm_kunit_alloc_dc_with_ctx(test)->ctx;
+	stream->timing.pix_clk_100hz = 4083750;
+	stream->timing.h_total = 2200;
+	stream->timing.v_total = 1125;
+
+	crtc_state->base.crtc = &acrtc->base;
+	crtc_state->base.vrr_enabled = true;
+	crtc_state->stream = stream;
+	crtc_state->vrr_supported = true;
+	crtc_state->freesync_config.min_refresh_in_uhz = 48000000;
+	crtc_state->freesync_config.max_refresh_in_uhz = 165000000;
+	crtc_state->freesync_config.btr = true;
+
+	amdgpu_dm_update_stream_irq_parameters(&adev->dm, crtc_state);
+
+	vrr = &acrtc->dm_irq_params.vrr_params;
+	KUNIT_ASSERT_EQ(test, (int)vrr->state, (int)VRR_STATE_ACTIVE_VARIABLE);
+	KUNIT_ASSERT_TRUE(test, vrr->btr.btr_enabled);
+	KUNIT_ASSERT_FALSE(test, vrr->btr.btr_active);
+
+	flip_time_in_us =
+		ctx->old_plane_state->dc_state->time.prev_update_time_in_us + 16667;
+	amdgpu_dm_update_freesync_state_on_stream(&adev->dm, crtc_state, stream,
+						  ctx->dc_plane_state,
+						  flip_time_in_us);
+
+	return vrr->btr.btr_active;
+}
+
+/**
+ * dm_test_flip_time_btr_inherited - Test a reset plane's next flip stays out of BTR
+ * @test: The KUnit test context
+ */
+static void dm_test_flip_time_btr_inherited(struct kunit *test)
+{
+	struct dm_test_flip_time_ctx *ctx = dm_test_flip_time_ctx_alloc(test);
+
+	dm_test_flip_time_inherit(ctx);
+
+	KUNIT_EXPECT_FALSE(test, dm_test_flip_time_btr_active(test, ctx));
+}
+
+/**
+ * dm_test_flip_time_btr_no_history - Test a plane without flip history enters BTR
+ * @test: The KUnit test context
+ */
+static void dm_test_flip_time_btr_no_history(struct kunit *test)
+{
+	struct dm_test_flip_time_ctx *ctx = dm_test_flip_time_ctx_alloc(test);
+
+	KUNIT_EXPECT_TRUE(test, dm_test_flip_time_btr_active(test, ctx));
+}
+
 /**
  * dm_test_mod_power_update_streams_empty - Test an empty commit updates no streams
  * @test: The KUnit test context
@@ -5361,6 +5546,11 @@ static struct kunit_case amdgpu_dm_tests[] = {
 	KUNIT_CASE(dm_test_aquire_global_lock_waits_commit),
 	KUNIT_CASE(dm_test_update_crtc_state_unchanged),
 	KUNIT_CASE(dm_test_update_plane_state_detached),
+	KUNIT_CASE(dm_test_flip_time_same_crtc),
+	KUNIT_CASE(dm_test_flip_time_crtc_changed),
+	KUNIT_CASE(dm_test_flip_time_no_old_dc_state),
+	KUNIT_CASE(dm_test_flip_time_btr_inherited),
+	KUNIT_CASE(dm_test_flip_time_btr_no_history),
 	KUNIT_CASE(dm_test_mod_power_update_streams_empty),
 	KUNIT_CASE(dm_test_mod_power_update_streams_no_modeset),
 	KUNIT_CASE(dm_test_mod_power_update_streams_enable),

-- 
2.56.0

Re: [PATCH 1/2] drm/amd/display: Keep FreeSync flip timing across plane resets

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/amd/display: race and stale flip time copy in dm_update_plane_state()
--

commit ab38a8475d9a38b0920f07d4b8cd8a1171060e9b
Author: Davey Hughes <davidralphhughes@gmail.com>

drm/amd/display: Keep FreeSync flip timing across plane resets

This commit preserves the DC plane flip timing across plane resets when the
plane remains on the same CRTC, preventing FreeSync below-the-range (BTR) or
fixed-refresh paths from seeing a zeroed flip history on color management
updates.

> 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 bdabb4bf14dab..a69e28685857f 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6076,6 +6076,11 @@ dm_update_plane_state(struct dc *dc,
>  			goto out;
>  		}
>  
> +		/* FreeSync times each frame from the plane's previous flip. */
> +		if (dm_old_plane_state->dc_state &&
> +		    old_plane_crtc == new_plane_crtc)
> +			dc_new_plane_state->time = dm_old_plane_state->dc_state->time;

[Severity: Medium]
Can copying dm_old_plane_state->dc_state->time here in dm_update_plane_state()
race with an in-flight non-blocking commit or copy stale flip timestamps?

When userspace submits a non-blocking commit (Commit 1),
drm_atomic_helper_swap_state() installs Commit 1's plane state into
plane->state and queues commit_work before dropping drm_modeset_locks.

Commit 1's worker then runs asynchronously without holding drm_modeset_locks:

  amdgpu_dm_atomic_commit_tail()
    -> amdgpu_dm_commit_planes()
      -> drm_atomic_helper_wait_for_fences()
      -> mutex_lock(&dm->dc_lock)
      -> update_planes_and_stream_adapter()
        -> dc_update_planes_and_stream()
          -> update_planes_and_stream_state()
            -> copy_surface_update_to_plane()

In drivers/gpu/drm/amd/display/dc/core/dc.c:copy_surface_update_to_plane(),
DC mutates surface->time under dm->dc_lock:

	surface->time.time_elapsed_in_us[surface->time.index] =
		(unsigned int)(srf_update->flip_addr->flip_timestamp_in_us -
			surface->time.prev_update_time_in_us);
	surface->time.prev_update_time_in_us =
		(unsigned int)srf_update->flip_addr->flip_timestamp_in_us;
	surface->time.index++;
	if (surface->time.index >= DC_PLANE_UPDATE_TIMES_MAX)
		surface->time.index = 0;

If a second commit (Commit 2) that resets the plane enters
amdgpu_dm_atomic_check() while Commit 1's commit_work is still waiting on
fences or executing copy_surface_update_to_plane(), amdgpu_dm_atomic_check()
calls dm_update_plane_state() before calling do_aquire_global_lock():

  amdgpu_dm_atomic_check() {
      ...
      for_each_oldnew_plane_in_descending_zpos(...) {
          ret = dm_update_plane_state(dc, state, plane,
                                      old_plane_state,
                                      new_plane_state,
                                      true,
                                      &needs_dc_state_realloc,
                                      &is_top_most_overlay);
          ...
      }
      ...
      if (needs_dc_state_realloc) {
          ...
          ret = do_aquire_global_lock(dev, state);
      }
  }

Because dm_update_plane_state() reads dm_old_plane_state->dc_state->time
without holding dm->dc_lock and before do_aquire_global_lock() waits for
Commit 1's hw_done and flip_done:

1. If Commit 1 is currently in copy_surface_update_to_plane(), the unlocked
   struct assignment races with the writes to surface->time.
2. If Commit 1 is still waiting on fences before acquiring dm->dc_lock,
   dm_update_plane_state() snapshots the pre-Commit-1 flip time (which is all
   zeroes if Commit 1 also reset or enabled the plane, or Commit 0's timestamp
   otherwise). Then do_aquire_global_lock() blocks until Commit 1 finishes
   updating dm_old_plane_state->dc_state->time and signals flip_done, allowing
   even a non-blocking Commit 2 to pass stall_checks() in
   drm_atomic_helper_setup_commit() and commit with the stale dc_new_plane_state->time.

Would it be safer to copy or update the flip timing in commit_tail (for
example inside amdgpu_dm_commit_planes() under dm->dc_lock after the previous
commit has completed), or after synchronizing with the previous commit?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-freesync-plane-reset-time-v1-0-bb43b501faf4@gmail.com?part=1

Re: [PATCH 1/2] drm/amd/display: Keep FreeSync flip timing across plane resets

[David Hughes]

> Can copying dm_old_plane_state->dc_state->time here in dm_update_plane_state()
> race with an in-flight non-blocking commit or copy stale flip timestamps?

Yes. The copy in atomic check can read the old DC plane's flip timing
while the previous commit is still waiting on fences or writing it in
copy_surface_update_to_plane(). FreeSync only reads
prev_update_time_in_us, so a stale copy usually costs one frame timed
across two flips, and at worst falls back to the behaviour without this
patch. It is still a data race, though.

v2 moves the copy to amdgpu_dm_commit_planes(), before the preflip.
commit_tail() has already waited for the previous commit there
(drm_atomic_helper_wait_for_dependencies()), so the old plane has been
programmed and nothing else writes it, and the copy doesn't need
dm->dc_lock. I'm building and testing it right now.


On Tue, Oct 27, 2026 at 6:52 AM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] drm/amd/display: race and stale flip time copy in dm_update_plane_state()
> --
>
> commit ab38a8475d9a38b0920f07d4b8cd8a1171060e9b
> Author: Davey Hughes <davidralphhughes@gmail.com>
>
> drm/amd/display: Keep FreeSync flip timing across plane resets
>
> This commit preserves the DC plane flip timing across plane resets when the
> plane remains on the same CRTC, preventing FreeSync below-the-range (BTR) or
> fixed-refresh paths from seeing a zeroed flip history on color management
> updates.
>
> > 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 bdabb4bf14dab..a69e28685857f 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -6076,6 +6076,11 @@ dm_update_plane_state(struct dc *dc,
> >                       goto out;
> >               }
> >
> > +             /* FreeSync times each frame from the plane's previous flip. */
> > +             if (dm_old_plane_state->dc_state &&
> > +                 old_plane_crtc == new_plane_crtc)
> > +                     dc_new_plane_state->time = dm_old_plane_state->dc_state->time;
>
> [Severity: Medium]
> Can copying dm_old_plane_state->dc_state->time here in dm_update_plane_state()
> race with an in-flight non-blocking commit or copy stale flip timestamps?
>
> When userspace submits a non-blocking commit (Commit 1),
> drm_atomic_helper_swap_state() installs Commit 1's plane state into
> plane->state and queues commit_work before dropping drm_modeset_locks.
>
> Commit 1's worker then runs asynchronously without holding drm_modeset_locks:
>
>   amdgpu_dm_atomic_commit_tail()
>     -> amdgpu_dm_commit_planes()
>       -> drm_atomic_helper_wait_for_fences()
>       -> mutex_lock(&dm->dc_lock)
>       -> update_planes_and_stream_adapter()
>         -> dc_update_planes_and_stream()
>           -> update_planes_and_stream_state()
>             -> copy_surface_update_to_plane()
>
> In drivers/gpu/drm/amd/display/dc/core/dc.c:copy_surface_update_to_plane(),
> DC mutates surface->time under dm->dc_lock:
>
>         surface->time.time_elapsed_in_us[surface->time.index] =
>                 (unsigned int)(srf_update->flip_addr->flip_timestamp_in_us -
>                         surface->time.prev_update_time_in_us);
>         surface->time.prev_update_time_in_us =
>                 (unsigned int)srf_update->flip_addr->flip_timestamp_in_us;
>         surface->time.index++;
>         if (surface->time.index >= DC_PLANE_UPDATE_TIMES_MAX)
>                 surface->time.index = 0;
>
> If a second commit (Commit 2) that resets the plane enters
> amdgpu_dm_atomic_check() while Commit 1's commit_work is still waiting on
> fences or executing copy_surface_update_to_plane(), amdgpu_dm_atomic_check()
> calls dm_update_plane_state() before calling do_aquire_global_lock():
>
>   amdgpu_dm_atomic_check() {
>       ...
>       for_each_oldnew_plane_in_descending_zpos(...) {
>           ret = dm_update_plane_state(dc, state, plane,
>                                       old_plane_state,
>                                       new_plane_state,
>                                       true,
>                                       &needs_dc_state_realloc,
>                                       &is_top_most_overlay);
>           ...
>       }
>       ...
>       if (needs_dc_state_realloc) {
>           ...
>           ret = do_aquire_global_lock(dev, state);
>       }
>   }
>
> Because dm_update_plane_state() reads dm_old_plane_state->dc_state->time
> without holding dm->dc_lock and before do_aquire_global_lock() waits for
> Commit 1's hw_done and flip_done:
>
> 1. If Commit 1 is currently in copy_surface_update_to_plane(), the unlocked
>    struct assignment races with the writes to surface->time.
> 2. If Commit 1 is still waiting on fences before acquiring dm->dc_lock,
>    dm_update_plane_state() snapshots the pre-Commit-1 flip time (which is all
>    zeroes if Commit 1 also reset or enabled the plane, or Commit 0's timestamp
>    otherwise). Then do_aquire_global_lock() blocks until Commit 1 finishes
>    updating dm_old_plane_state->dc_state->time and signals flip_done, allowing
>    even a non-blocking Commit 2 to pass stall_checks() in
>    drm_atomic_helper_setup_commit() and commit with the stale dc_new_plane_state->time.
>
> Would it be safer to copy or update the flip timing in commit_tail (for
> example inside amdgpu_dm_commit_planes() under dm->dc_lock after the previous
> commit has completed), or after synchronizing with the previous commit?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261027-freesync-plane-reset-time-v1-0-bb43b501faf4@gmail.com?part=1