[PATCH] drm/amd/display: Fix pageflip event race condition for DCN. (v2) - dri-devel - freedesktop.org experimental mailing list

5 Mar 2020

Commit '16f17eda8bad ("drm/amd/display: Send vblank and user
events at vsartup for DCN")' introduces a new way of pageflip
completion handling for DCN, and some trouble.
The current implementation introduces a race condition, which
can cause pageflip completion events to be sent out one vblank
too early, thereby confusing userspace and causing flicker:
prepare_flip_isr():
1. Pageflip programming takes the ddev->event_lock.
2. Sets acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED
3. Releases ddev->event_lock.
--> Deadline for surface address regs double-buffering passes on
    target pipe.
4. dc_commit_updates_for_stream() MMIO programs the new pageflip
   into hw, but too late for current vblank.
=> pflip_status == AMDGPU_FLIP_SUBMITTED, but flip won't complete
   in current vblank due to missing the double-buffering deadline
   by a tiny bit.
5. VSTARTUP trigger point in vblank is reached, VSTARTUP irq fires,
   dm_dcn_crtc_high_irq() gets called.
6. Detects pflip_status == AMDGPU_FLIP_SUBMITTED and assumes the
   pageflip has been completed/will complete in this vblank and
   sends out pageflip completion event to userspace and resets
   pflip_status = AMDGPU_FLIP_NONE.
=> Flip completion event sent out one vblank too early.
This behaviour has been observed during my testing with measurement
hardware a couple of time.
The commit message says that the extra flip event code was added to
dm_dcn_crtc_high_irq() to prevent missing to send out pageflip events
in case the pflip irq doesn't fire, because the "DCH HUBP" component
is clock gated and doesn't fire pflip irqs in that state. Also that
this clock gating may happen if no planes are active. According to
Nicholas, the clock gating can also happen if psr is active, and the
gating is controlled independently by the hardware, so difficult to
detect if and when the completion code in above commit is needed.
This patch tries the following solution: It only executes the extra pflip
completion code in dm_dcn_crtc_high_irq() iff the hardware reports
that there aren't any surface updated pending in the double-buffered
surface scanout address registers. Otherwise it leaves pflip completion
to the pflip irq handler, for a more race-free experience.
This would only guard against the order of events mentioned above.
If Step 5 (VSTARTUP trigger) happens before step 4 then this won't help
at all, because 1-3 + 5 might happen even without the hw being programmed
at all, ie. no surface update pending because none yet programmed into hw.
Therefore this patch also changes locking in amdgpu_dm_commit_planes(),
so that prepare_flip_isr() and dc_commit_updates_for_stream() are done
under event_lock protection within the same critical section.
v2: Take Nicholas comments into account, try a different solution.
Lightly tested on Polaris (locking) and Raven (the whole DCN stuff).
Seems to work without causing obvious new trouble.
Fixes: 16f17eda8bad ("drm/amd/display: Send vblank and user events at vsartup for DCN")
Signed-off-by: Mario Kleiner mario.kleiner.de@gmail.com
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 80 ++++++++++++++++---
 1 file changed, 67 insertions(+), 13 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 d7df1a85e72f..aa4e941b276f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -287,6 +287,28 @@ static inline bool amdgpu_dm_vrr_active(struct dm_crtc_state *dm_state)
           dm_state->freesync_config.state == VRR_STATE_ACTIVE_FIXED;
 }
+/**
+ * dm_crtc_is_flip_pending() - Is a pageflip pending on this crtc?
+ *
+ * Returns true if any plane on the crtc has a flip pending, false otherwise.
+ */
+static bool dm_crtc_is_flip_pending(struct dm_crtc_state *acrtc_state)
+{
+	struct dc_stream_status *status = dc_stream_get_status(acrtc_state->stream);
+	const struct dc_plane_status *plane_status;
+	int i;
+	bool pending = false;
+
+	for (i = 0; i < status->plane_count; i++) {
+		plane_status = dc_plane_get_status(status->plane_states[i]);
+		pending |= plane_status->is_flip_pending;
+		DRM_DEBUG_DRIVER("plane:%d, flip_pending=%d\n",
+				 i, plane_status->is_flip_pending);
+	}
+
+	return pending;
+}
+
 /**
  * dm_pflip_high_irq() - Handle pageflip interrupt
  * @interrupt_params: ignored
@@ -435,6 +457,11 @@ static void dm_vupdate_high_irq(void *interrupt_params)
    			spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
    		}
    	}
+
+		if (acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED) {
+			DRM_DEBUG_DRIVER("%s:crtc:%d, flip_pending=%d\n", __func__,
+					    acrtc->crtc_id, dm_crtc_is_flip_pending(acrtc_state));
+		}
    }
 }
@@ -489,6 +516,11 @@ static void dm_crtc_high_irq(void *interrupt_params)
    			&acrtc_state->vrr_params.adjust);
    		spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
    	}
+
+		if (acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED) {
+			DRM_DEBUG_DRIVER("%s:crtc:%d, flip_pending=%d\n", __func__,
+					 acrtc->crtc_id, dm_crtc_is_flip_pending(acrtc_state));
+		}
    }
 }
@@ -543,13 +575,29 @@ static void dm_dcn_crtc_high_irq(void *interrupt_params)
    		&acrtc_state->vrr_params.adjust);
    }
-	if (acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED) {
+	/*
+	 * If there aren't any active_planes, or PSR is active, then DCH HUBP
+	 * may be clock-gated. In that state, pageflip completion interrupts
+	 * won't fire and pageflip completion events won't get delivered.
+	 *
+	 * Prevent this by sending pending pageflip events from here if a flip
+	 * has been submitted, but is no longer pending in hw, ie. has already
+	 * completed.
+	 *
+	 * If the flip is still pending in hw, then use dm_pflip_high_irq()
+	 * instead, handling completion as usual by pflip irq.
+	 */
+	if (acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED &&
+	    !dm_crtc_is_flip_pending(acrtc_state)) {
    	if (acrtc->event) {
    		drm_crtc_send_vblank_event(&acrtc->base, acrtc->event);
    		acrtc->event = NULL;
    		drm_crtc_vblank_put(&acrtc->base);
    	}
    	acrtc->pflip_status = AMDGPU_FLIP_NONE;
+
+		DRM_DEBUG_DRIVER("crtc:%d, pflip_stat:AMDGPU_FLIP_NONE\n",
+				 acrtc->crtc_id);
    }
spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
@@ -6325,7 +6373,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
    		to_dm_crtc_state(drm_atomic_get_old_crtc_state(state, pcrtc));
    int planes_count = 0, vpos, hpos;
    long r;
-	unsigned long flags;
+	unsigned long flags = 0;
    struct amdgpu_bo *abo;
    uint64_t tiling_flags;
    uint32_t target_vblank, last_flip_vblank;
@@ -6513,17 +6561,6 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
    		usleep_range(1000, 1100);
    	}
-		if (acrtc_attach->base.state->event) {
-			drm_crtc_vblank_get(pcrtc);
-
-			spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
-
-			WARN_ON(acrtc_attach->pflip_status != AMDGPU_FLIP_NONE);
-			prepare_flip_isr(acrtc_attach);
-
-			spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags);
-		}
-
    	if (acrtc_state->stream) {
    		if (acrtc_state->freesync_vrr_info_changed)
    			bundle->stream_update.vrr_infopacket =
@@ -6575,6 +6612,15 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
    			acrtc_state->stream->link->psr_allow_active)
    		amdgpu_dm_psr_disable(acrtc_state->stream);
+		if (pflip_present && acrtc_attach->base.state->event) {
+			drm_crtc_vblank_get(pcrtc);
+
+			spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
+
+			WARN_ON(acrtc_attach->pflip_status != AMDGPU_FLIP_NONE);
+			prepare_flip_isr(acrtc_attach);
+		}
+
    	dc_commit_updates_for_stream(dm->dc,
    					     bundle->surface_updates,
    					     planes_count,
@@ -6582,6 +6628,14 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
    					     &bundle->stream_update,
    					     dc_state);
+		/*
+		 * Must event_lock protect prepare_flip_isr() above and
+		 * dc_commit_updates_for_stream within same critical section,
+		 * or pageflip completion will suffer bad races on DCN.
+		 */
+		if (pflip_present && acrtc_attach->pflip_status == AMDGPU_FLIP_SUBMITTED)
+			spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags);
+
    	if ((acrtc_state->update_type > UPDATE_TYPE_FAST) &&
    					acrtc_state->stream->psr_version &&
    					!acrtc_state->stream->link->psr_feature_enabled)
-- 
2.20.1


    