* [PATCH 2/4] drm/amd/display: Use connector list for finding DPRX CRC aux
[not found] ` <20190820175932.7884-1-nicholas.kazlauskas-5C7GfCeVMHo@public.gmane.org>
@ 2019-08-20 17:59 ` Nicholas Kazlauskas
2019-08-20 17:59 ` [PATCH 3/4] drm/amd/display: Split out DC programming for CRC capture Nicholas Kazlauskas
2019-08-20 17:59 ` [PATCH 4/4] drm/amd/display: Lock the CRTC when setting CRC source Nicholas Kazlauskas
2 siblings, 0 replies; 5+ messages in thread
From: Nicholas Kazlauskas @ 2019-08-20 17:59 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Cc: Leo Li, David Francis, Bhawanpreet Lakha, Nicholas Kazlauskas
[Why]
This change is a refactor in preparation for adding locking and removing
the requirement for a stream state on the CRTC for enabling CRC capture
to fix igt@kms_plane_multiple@* warnings.
[How]
We can get the aux by finding the matching connector for the CRTC
with the assumption that we're not doing cloning.
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 | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 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 f675626ef56b..c57ff8821fe2 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
@@ -101,7 +101,6 @@ int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
struct amdgpu_device *adev = crtc->dev->dev_private;
struct dm_crtc_state *crtc_state = to_dm_crtc_state(crtc->state);
struct dc_stream_state *stream_state = crtc_state->stream;
- struct amdgpu_dm_connector *aconn;
struct drm_dp_aux *aux = NULL;
bool enable = false;
bool enabled = false;
@@ -137,9 +136,21 @@ int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
* DPRX DITHER | XXXX | Enable DPRX CRC, need 'aux', set dither
*/
if (dm_is_crc_source_dprx(source) ||
- (source == AMDGPU_DM_PIPE_CRC_SOURCE_NONE &&
- dm_is_crc_source_dprx(crtc_state->crc_src))) {
- aconn = stream_state->link->priv;
+ (source == AMDGPU_DM_PIPE_CRC_SOURCE_NONE &&
+ dm_is_crc_source_dprx(crtc_state->crc_src))) {
+ struct amdgpu_dm_connector *aconn = NULL;
+ struct drm_connector *connector;
+ struct drm_connector_list_iter conn_iter;
+
+ drm_connector_list_iter_begin(crtc->dev, &conn_iter);
+ drm_for_each_connector_iter (connector, &conn_iter) {
+ if (!connector->state || connector->state->crtc != crtc)
+ continue;
+
+ aconn = to_amdgpu_dm_connector(connector);
+ break;
+ }
+ drm_connector_list_iter_end(&conn_iter);
if (!aconn) {
DRM_DEBUG_DRIVER("No amd connector matching CRTC-%d\n", crtc->index);
--
2.17.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 3/4] drm/amd/display: Split out DC programming for CRC capture
[not found] ` <20190820175932.7884-1-nicholas.kazlauskas-5C7GfCeVMHo@public.gmane.org>
2019-08-20 17:59 ` [PATCH 2/4] drm/amd/display: Use connector list for finding DPRX CRC aux Nicholas Kazlauskas
@ 2019-08-20 17:59 ` Nicholas Kazlauskas
2019-08-20 17:59 ` [PATCH 4/4] drm/amd/display: Lock the CRTC when setting CRC source Nicholas Kazlauskas
2 siblings, 0 replies; 5+ messages in thread
From: Nicholas Kazlauskas @ 2019-08-20 17:59 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Cc: Leo Li, David Francis, Bhawanpreet Lakha, Nicholas Kazlauskas
[Why]
Calling amdgpu_dm_crtc_set_crc_source in amdgpu_dm directly has the
consequence of adding additional vblank references or starting DPRX
CRC capture more than once without calling stop first.
Vblank references for CRC capture should be managed entirely by opening
and closing the CRC file from userspace.
Stream state also shouldn't be required on the CRC so we can close the
file after the CRTC has been disabled.
[How]
Do DC programming required for configuring CRC capture separately from
setting the source. Whenever we re-enable or reset a CRC this
programming should be reapplied.
CRC vblank reference handling in amdgpu_dm can be entirely dropped after
this.
Stream state also no longer needs to be required since we can just defer
the programming to when the stream is actually enabled.
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>
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 25 ++------
.../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c | 62 ++++++++++++-------
.../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h | 6 ++
3 files changed, 49 insertions(+), 44 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 911fe78b47c1..7cf8dbccce95 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5967,11 +5967,9 @@ static void amdgpu_dm_enable_crtc_interrupts(struct drm_device *dev,
/* The stream has changed so CRC capture needs to re-enabled. */
source = dm_new_crtc_state->crc_src;
if (amdgpu_dm_is_valid_crc_source(source)) {
- dm_new_crtc_state->crc_src = AMDGPU_DM_PIPE_CRC_SOURCE_NONE;
- if (source == AMDGPU_DM_PIPE_CRC_SOURCE_CRTC)
- amdgpu_dm_crtc_set_crc_source(crtc, "crtc");
- else if (source == AMDGPU_DM_PIPE_CRC_SOURCE_DPRX)
- amdgpu_dm_crtc_set_crc_source(crtc, "dprx");
+ amdgpu_dm_crtc_configure_crc_source(
+ crtc, dm_new_crtc_state,
+ dm_new_crtc_state->crc_src);
}
#endif
}
@@ -6022,23 +6020,8 @@ static int amdgpu_dm_atomic_commit(struct drm_device *dev,
if (dm_old_crtc_state->interrupts_enabled &&
(!dm_new_crtc_state->interrupts_enabled ||
- drm_atomic_crtc_needs_modeset(new_crtc_state))) {
- /*
- * Drop the extra vblank reference added by CRC
- * capture if applicable.
- */
- if (amdgpu_dm_is_valid_crc_source(dm_new_crtc_state->crc_src))
- drm_crtc_vblank_put(crtc);
-
- /*
- * Only keep CRC capture enabled if there's
- * still a stream for the CRTC.
- */
- if (!dm_new_crtc_state->stream)
- dm_new_crtc_state->crc_src = AMDGPU_DM_PIPE_CRC_SOURCE_NONE;
-
+ drm_atomic_crtc_needs_modeset(new_crtc_state)))
manage_dm_interrupts(adev, acrtc, false);
- }
}
/*
* Add check here for SoC's that support hardware cursor plane, to
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 c57ff8821fe2..e9d5b1fad625 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
@@ -96,11 +96,46 @@ amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc, const char *src_name,
return 0;
}
-int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
+int amdgpu_dm_crtc_configure_crc_source(struct drm_crtc *crtc,
+ struct dm_crtc_state *dm_crtc_state,
+ enum amdgpu_dm_pipe_crc_source source)
{
struct amdgpu_device *adev = crtc->dev->dev_private;
+ struct dc_stream_state *stream_state = dm_crtc_state->stream;
+ bool enable = amdgpu_dm_is_valid_crc_source(source);
+ int ret = 0;
+
+ /* Configuration will be deferred to stream enable. */
+ if (!stream_state)
+ return 0;
+
+ mutex_lock(&adev->dm.dc_lock);
+
+ /* Enable CRTC CRC generation if necessary. */
+ if (dm_is_crc_source_crtc(source)) {
+ if (!dc_stream_configure_crc(stream_state->ctx->dc,
+ stream_state, enable, enable)) {
+ ret = -EINVAL;
+ goto unlock;
+ }
+ }
+
+ /* Configure dithering */
+ if (!dm_need_crc_dither(source))
+ dc_stream_set_dither_option(stream_state, DITHER_OPTION_TRUN8);
+ else
+ dc_stream_set_dither_option(stream_state,
+ DITHER_OPTION_DEFAULT);
+
+unlock:
+ mutex_unlock(&adev->dm.dc_lock);
+
+ return ret;
+}
+
+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);
- struct dc_stream_state *stream_state = crtc_state->stream;
struct drm_dp_aux *aux = NULL;
bool enable = false;
bool enabled = false;
@@ -114,14 +149,8 @@ int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
return -EINVAL;
}
- if (!stream_state) {
- DRM_ERROR("No stream state for CRTC%d\n", crtc->index);
- return -EINVAL;
- }
-
enable = amdgpu_dm_is_valid_crc_source(source);
- mutex_lock(&adev->dm.dc_lock);
/*
* USER REQ SRC | CURRENT SRC | BEHAVIOR
* -----------------------------
@@ -154,7 +183,6 @@ 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);
- mutex_unlock(&adev->dm.dc_lock);
return -EINVAL;
}
@@ -162,24 +190,12 @@ int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
if (!aux) {
DRM_DEBUG_DRIVER("No dp aux for amd connector\n");
- mutex_unlock(&adev->dm.dc_lock);
- return -EINVAL;
- }
- } else if (dm_is_crc_source_crtc(source)) {
- if (!dc_stream_configure_crc(stream_state->ctx->dc, stream_state,
- enable, enable)) {
- mutex_unlock(&adev->dm.dc_lock);
return -EINVAL;
}
}
- /* configure dithering */
- if (!dm_need_crc_dither(source))
- dc_stream_set_dither_option(stream_state, DITHER_OPTION_TRUN8);
- else if (!dm_need_crc_dither(crtc_state->crc_src))
- dc_stream_set_dither_option(stream_state, DITHER_OPTION_DEFAULT);
-
- mutex_unlock(&adev->dm.dc_lock);
+ if (amdgpu_dm_crtc_configure_crc_source(crtc, crtc_state, source))
+ return -EINVAL;
/*
* Reading the CRC requires the vblank interrupt handler to be
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h
index 14de7301c28d..f7d731797d3f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.h
@@ -26,6 +26,9 @@
#ifndef AMD_DAL_DEV_AMDGPU_DM_AMDGPU_DM_CRC_H_
#define AMD_DAL_DEV_AMDGPU_DM_AMDGPU_DM_CRC_H_
+struct drm_crtc;
+struct dm_crtc_state;
+
enum amdgpu_dm_pipe_crc_source {
AMDGPU_DM_PIPE_CRC_SOURCE_NONE = 0,
AMDGPU_DM_PIPE_CRC_SOURCE_CRTC,
@@ -44,6 +47,9 @@ static inline bool amdgpu_dm_is_valid_crc_source(enum amdgpu_dm_pipe_crc_source
/* amdgpu_dm_crc.c */
#ifdef CONFIG_DEBUG_FS
+int amdgpu_dm_crtc_configure_crc_source(struct drm_crtc *crtc,
+ struct dm_crtc_state *dm_crtc_state,
+ enum amdgpu_dm_pipe_crc_source source);
int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name);
int amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc,
const char *src_name,
--
2.17.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 4/4] drm/amd/display: Lock the CRTC when setting CRC source
[not found] ` <20190820175932.7884-1-nicholas.kazlauskas-5C7GfCeVMHo@public.gmane.org>
2019-08-20 17:59 ` [PATCH 2/4] drm/amd/display: Use connector list for finding DPRX CRC aux Nicholas Kazlauskas
2019-08-20 17:59 ` [PATCH 3/4] drm/amd/display: Split out DC programming for CRC capture Nicholas Kazlauskas
@ 2019-08-20 17:59 ` Nicholas Kazlauskas
[not found] ` <20190820175932.7884-4-nicholas.kazlauskas-5C7GfCeVMHo@public.gmane.org>
2 siblings, 1 reply; 5+ messages in thread
From: Nicholas Kazlauskas @ 2019-08-20 17:59 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Cc: Leo Li, David Francis, Bhawanpreet Lakha, Nicholas Kazlauskas
[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
^ permalink raw reply related [flat|nested] 5+ messages in thread