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
