[PATCH] drm/amd/display: Work around race beetween hw_done and wait_for_flip

[S, Shirish]

This workaround is not fixing the issue.

Regards,
Shirish S

-----Original Message-----
From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of Harry Wentland
Sent: Friday, September 28, 2018 6:46 PM
To: amd-gfx at lists.freedesktop.org; S, Shirish <Shirish.S at amd.com>; Li, Sun peng (Leo) <Sunpeng.Li at amd.com>
Cc: Wentland, Harry <Harry.Wentland at amd.com>
Subject: [PATCH] drm/amd/display: Work around race beetween hw_done and wait_for_flip

[Why]
There is a race that between drm_crtc_commit_hw_done and drm_atomic_helper_wait_for_flip where the possibilty exist for the
crtc->commit to be cleared after the null check in wait_for_flip_done
but before the call to wait_for_completion_timeout on commit->flip_done.

[How]
Take a reference to all commits in the state before drm_crtc_commit_hw_done is called and release those after drm_atomic_helper_wait_for_flip has finished.

Signed-off-by: Harry Wentland <harry.wentland at amd.com>
---

Would something like this work? I get the strong sense that this happens because Intel and IMX use the helpers in the other order, hence the dependency between hw_done and wait_for_flip was missed.

I'd rather make it obvious that there's (a) no reason to reorder these two calls on AMD HW (other than this unexpected dependency) and (b) this is something we'll probably want to fix in DRM.

Sorry it took me a while to understand what was happening here. Been busy at XDC until Jordan and Leo reminded me to take another look.

Harry

 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 15 +++++++++++++++
 1 file changed, 15 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 0f10d920a785..ed9a7d680b63 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4626,12 +4626,27 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 	}
 	spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
 
+	/*
+	 * WORKAROUND: Take a ref for all crtc_state commits to avoid
+	 * a race where the commit gets freed before commit_hw_done had
+	 * a chance to look for commit->flip_done
+	 */
+	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
+		drm_crtc_commit_get(new_crtc_state->commit);
+
 	/* Signal HW programming completion */
 	drm_atomic_helper_commit_hw_done(state);
 
 	if (wait_for_vblank)
 		drm_atomic_helper_wait_for_flip_done(dev, state);
 
+	/*
+	 * WORKAROUND: put the commit refs from above (see comment on
+	 * the drm_crtc_commit_get call above)
+	 */
+	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
+		drm_crtc_commit_put(new_crtc_state->commit);
+
 	drm_atomic_helper_cleanup_planes(dev, state);
 
 	/*
--
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx at lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx