> 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 <[email protected]> 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 <[email protected]> > > 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/[email protected]?part=1
