[PATCH 4/4] drm/amd/display: Lock the CRTC when setting CRC source

[Nicholas Kazlauskas <nicholas.kazlauskas-5C7GfCeVMHo@public.gmane.org>]

[Why]
We need to ensure that we're holding the lock on the CRTC when setting
the CRC source since we're modifying the CRTC state directly.

We also need to wait for any outstanding non-blocking commits to finish
so they aren't reading state that's potentially being modified -
non-blocking commits don't hold the CRTC lock while doing commit tail
work.

[How]
Lock the CRTC using its mutex. While holding the lock check if there's
any commit active on the CRTC - if there is, it's non-blocking and
we should wait until it's finished by waiting for hw_done to be
signaled since that's the last point where we touch CRTC state.

Cc: David Francis <David.Francis@amd.com>
Cc: Bhawanpreet Lakha <Bhawanpreet.Lakha@amd.com>
Cc: Leo Li <sunpeng.li@amd.com>
Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c | 63 +++++++++++++++----
 1 file changed, 51 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
index e9d5b1fad625..fa86b9e706af 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
@@ -135,13 +135,13 @@ int amdgpu_dm_crtc_configure_crc_source(struct drm_crtc *crtc,
 
 int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
 {
-	struct dm_crtc_state *crtc_state = to_dm_crtc_state(crtc->state);
+	enum amdgpu_dm_pipe_crc_source source = dm_parse_crc_source(src_name);
+	struct drm_crtc_commit *commit;
+	struct dm_crtc_state *crtc_state;
 	struct drm_dp_aux *aux = NULL;
 	bool enable = false;
 	bool enabled = false;
-	int ret;
-
-	enum amdgpu_dm_pipe_crc_source source = dm_parse_crc_source(src_name);
+	int ret = 0;
 
 	if (source < 0) {
 		DRM_DEBUG_DRIVER("Unknown CRC source %s for CRTC%d\n",
@@ -149,7 +149,33 @@ int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
 		return -EINVAL;
 	}
 
+	ret = drm_modeset_lock(&crtc->mutex, NULL);
+	if (ret)
+		return ret;
+
+	spin_lock(&crtc->commit_lock);
+	commit = list_first_entry_or_null(&crtc->commit_list,
+					  struct drm_crtc_commit, commit_entry);
+	if (commit)
+		drm_crtc_commit_get(commit);
+	spin_unlock(&crtc->commit_lock);
+
+	if (commit) {
+		/*
+		 * Need to wait for all outstanding programming to complete
+		 * in commit tail since it can modify CRC related fields and
+		 * hardware state. Since we're holding the CRTC lock we're
+		 * guaranteed that no other commit work can be queued off
+		 * before we modify the state below.
+		 */
+		ret = wait_for_completion_interruptible_timeout(
+			&commit->hw_done, 10 * HZ);
+		if (ret)
+			goto cleanup;
+	}
+
 	enable = amdgpu_dm_is_valid_crc_source(source);
+ 	crtc_state = to_dm_crtc_state(crtc->state);
 
 	/*
 	 * USER REQ SRC | CURRENT SRC | BEHAVIOR
@@ -183,19 +209,23 @@ int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
 
 		if (!aconn) {
 			DRM_DEBUG_DRIVER("No amd connector matching CRTC-%d\n", crtc->index);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto cleanup;
 		}
 
 		aux = &aconn->dm_dp_aux.aux;
 
 		if (!aux) {
 			DRM_DEBUG_DRIVER("No dp aux for amd connector\n");
-			return -EINVAL;
+			ret = -EINVAL;
+			goto cleanup;
 		}
 	}
 
-	if (amdgpu_dm_crtc_configure_crc_source(crtc, crtc_state, source))
-		return -EINVAL;
+	if (amdgpu_dm_crtc_configure_crc_source(crtc, crtc_state, source)) {
+		ret = -EINVAL;
+		goto cleanup;
+	}
 
 	/*
 	 * Reading the CRC requires the vblank interrupt handler to be
@@ -205,12 +235,13 @@ int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
 	if (!enabled && enable) {
 		ret = drm_crtc_vblank_get(crtc);
 		if (ret)
-			return ret;
+			goto cleanup;
 
 		if (dm_is_crc_source_dprx(source)) {
 			if (drm_dp_start_crc(aux, crtc)) {
 				DRM_DEBUG_DRIVER("dp start crc failed\n");
-				return -EINVAL;
+				ret = -EINVAL;
+				goto cleanup;
 			}
 		}
 	} else if (enabled && !enable) {
@@ -218,7 +249,8 @@ int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
 		if (dm_is_crc_source_dprx(source)) {
 			if (drm_dp_stop_crc(aux)) {
 				DRM_DEBUG_DRIVER("dp stop crc failed\n");
-				return -EINVAL;
+				ret = -EINVAL;
+				goto cleanup;
 			}
 		}
 	}
@@ -227,7 +259,14 @@ int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
 
 	/* Reset crc_skipped on dm state */
 	crtc_state->crc_skip_count = 0;
-	return 0;
+
+cleanup:
+	if (commit)
+		drm_crtc_commit_put(commit);
+
+	drm_modeset_unlock(&crtc->mutex);
+
+	return ret;
 }
 
 /**
-- 
2.17.1

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