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

[sashiko-bot@kernel.org]

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