Hi,
I've been investigating several eDP issues on a Rockchip RK3399 system and have two proposed bugfixes. RK3399 has two CRTCs, either of which can be used for eDP output. For both fixes, we have bugs due to the relationship between the generalized "self refresh helpers" and the analogix-dp bridge driver which controls much of the PSR mechanics. These bugs are most visible when switching between CRTCs.
I'm not a DRM expert, but I've been poking at a lot of Rockchip display drivers recently. I'd love some skeptical eyes, so feel free to ask questions if I haven't explained issues well, or the proposals look fishy.
Regards, Brian
Brian Norris (2): drm/bridge: analogix_dp: Support PSR-exit to disable transition drm/atomic: Force bridge self-refresh-exit on CRTC switch
.../drm/bridge/analogix/analogix_dp_core.c | 42 +++++++++++++++++-- drivers/gpu/drm/drm_atomic_helper.c | 16 +++++-- 2 files changed, 51 insertions(+), 7 deletions(-)
Most eDP panel functions only work correctly when the panel is not in self-refresh. In particular, analogix_dp_bridge_disable() tends to hit AUX channel errors if the panel is in self-refresh.
Given the above, it appears that so far, this driver assumes that we are never in self-refresh when it comes time to fully disable the bridge. Prior to commit 846c7dfc1193 ("drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2."), this tended to be true, because we would automatically disable the pipe when framebuffers were removed, and so we'd typically disable the bridge shortly after the last display activity.
However, that is not guaranteed: an idle (self-refresh) display pipe may be disabled, e.g., when switching CRTCs. We need to exit PSR first.
Stable notes: this is definitely a bugfix, and the bug has likely existed in some form for quite a while. It may predate the "PSR helpers" refactor, but the code looked very different before that, and it's probably not worth rewriting the fix.
Cc: stable@vger.kernel.org Fixes: 6c836d965bad ("drm/rockchip: Use the helpers for PSR") Signed-off-by: Brian Norris briannorris@chromium.org ---
.../drm/bridge/analogix/analogix_dp_core.c | 42 +++++++++++++++++-- 1 file changed, 38 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index b7d2e4449cfa..6ee0f62a7161 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c @@ -1268,6 +1268,25 @@ static int analogix_dp_bridge_attach(struct drm_bridge *bridge, return 0; }
+static +struct drm_crtc *analogix_dp_get_old_crtc(struct analogix_dp_device *dp, + struct drm_atomic_state *state) +{ + struct drm_encoder *encoder = dp->encoder; + struct drm_connector *connector; + struct drm_connector_state *conn_state; + + connector = drm_atomic_get_old_connector_for_encoder(state, encoder); + if (!connector) + return NULL; + + conn_state = drm_atomic_get_old_connector_state(state, connector); + if (!conn_state) + return NULL; + + return conn_state->crtc; +} + static struct drm_crtc *analogix_dp_get_new_crtc(struct analogix_dp_device *dp, struct drm_atomic_state *state) @@ -1448,14 +1467,16 @@ analogix_dp_bridge_atomic_disable(struct drm_bridge *bridge, { struct drm_atomic_state *old_state = old_bridge_state->base.state; struct analogix_dp_device *dp = bridge->driver_private; - struct drm_crtc *crtc; + struct drm_crtc *old_crtc, *new_crtc; + struct drm_crtc_state *old_crtc_state = NULL; struct drm_crtc_state *new_crtc_state = NULL; + int ret;
- crtc = analogix_dp_get_new_crtc(dp, old_state); - if (!crtc) + new_crtc = analogix_dp_get_new_crtc(dp, old_state); + if (!new_crtc) goto out;
- new_crtc_state = drm_atomic_get_new_crtc_state(old_state, crtc); + new_crtc_state = drm_atomic_get_new_crtc_state(old_state, new_crtc); if (!new_crtc_state) goto out;
@@ -1464,6 +1485,19 @@ analogix_dp_bridge_atomic_disable(struct drm_bridge *bridge, return;
out: + old_crtc = analogix_dp_get_old_crtc(dp, old_state); + if (old_crtc) { + old_crtc_state = drm_atomic_get_old_crtc_state(old_state, + old_crtc); + + /* When moving from PSR to fully disabled, exit PSR first. */ + if (old_crtc_state && old_crtc_state->self_refresh_active) { + ret = analogix_dp_disable_psr(dp); + if (ret) + DRM_ERROR("Failed to disable psr (%d)\n", ret); + } + } + analogix_dp_bridge_disable(bridge); }
It's possible to change which CRTC is in use for a given connector/encoder/bridge while we're in self-refresh without fully disabling the connector/encoder/bridge along the way. This can confuse the bridge encoder/bridge, because (a) it needs to track the SR state (trying to perform "active" operations while the panel is still in SR can be Bad(TM)); and (b) it tracks the SR state via the CRTC state (and after the switch, the previous SR state is lost).
Thus, we need to either somehow carry the self-refresh state over to the new CRTC, or else force an encoder/bridge self-refresh transition during such a switch.
I choose the latter, so we disable the encoder (and exit PSR) before attaching it to the new CRTC (where we can continue to assume a clean (non-self-refresh) state).
This fixes PSR issues seen on Rockchip RK3399 systems with drivers/gpu/drm/bridge/analogix/analogix_dp_core.c.
Cc: stable@vger.kernel.org Fixes: 1452c25b0e60 ("drm: Add helpers to kick off self refresh mode in drivers") Signed-off-by: Brian Norris briannorris@chromium.org ---
drivers/gpu/drm/drm_atomic_helper.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 9603193d2fa1..74161d007894 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1011,9 +1011,19 @@ crtc_needs_disable(struct drm_crtc_state *old_state, return drm_atomic_crtc_effectively_active(old_state);
/* - * We need to run through the crtc_funcs->disable() function if the CRTC - * is currently on, if it's transitioning to self refresh mode, or if - * it's in self refresh mode and needs to be fully disabled. + * We need to disable bridge(s) and CRTC if we're transitioning out of + * self-refresh and changing CRTCs at the same time, because the + * bridge tracks self-refresh status via CRTC state. + */ + if (old_state->self_refresh_active && new_state->enable && + old_state->crtc != new_state->crtc) + return true; + + /* + * We also need to run through the crtc_funcs->disable() function if + * the CRTC is currently on, if it's transitioning to self refresh + * mode, or if it's in self refresh mode and needs to be fully + * disabled. */ return old_state->active || (old_state->self_refresh_active && !new_state->active) ||
Hi Brian,
On Tue, 2022-02-15 at 15:54 -0800, Brian Norris wrote:
It's possible to change which CRTC is in use for a given connector/encoder/bridge while we're in self-refresh without fully disabling the connector/encoder/bridge along the way. This can confuse the bridge encoder/bridge, because (a) it needs to track the SR state (trying to perform "active" operations while the panel is still in SR can be Bad(TM)); and (b) it tracks the SR state via the CRTC state (and after the switch, the previous SR state is lost).
Thus, we need to either somehow carry the self-refresh state over to the new CRTC, or else force an encoder/bridge self-refresh transition during such a switch.
I choose the latter, so we disable the encoder (and exit PSR) before attaching it to the new CRTC (where we can continue to assume a clean (non-self-refresh) state).
This fixes PSR issues seen on Rockchip RK3399 systems with drivers/gpu/drm/bridge/analogix/analogix_dp_core.c.
Cc: stable@vger.kernel.org Fixes: 1452c25b0e60 ("drm: Add helpers to kick off self refresh mode in drivers") Signed-off-by: Brian Norris briannorris@chromium.org
drivers/gpu/drm/drm_atomic_helper.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 9603193d2fa1..74161d007894 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1011,9 +1011,19 @@ crtc_needs_disable(struct drm_crtc_state *old_state, return drm_atomic_crtc_effectively_active(old_state);
/*
* We need to run through the crtc_funcs->disable() function if the CRTC* is currently on, if it's transitioning to self refresh mode, or if* it's in self refresh mode and needs to be fully disabled.
* We need to disable bridge(s) and CRTC if we're transitioning out of* self-refresh and changing CRTCs at the same time, because the* bridge tracks self-refresh status via CRTC state.*/- if (old_state->self_refresh_active && new_state->enable &&
old_state->crtc != new_state->crtc)return true;
I think 'new_state->enable' should be changed to 'new_state->active', because 'active' is the one to enable/disable the CRTC while 'enable' reflects whether a mode blob is set to CRTC state. The overall logic added above is ok to me. Let's see if others have any comments.
Regards, Liu Ying
- /*
* We also need to run through the crtc_funcs->disable() function if* the CRTC is currently on, if it's transitioning to self refresh* mode, or if it's in self refresh mode and needs to be fully* disabled.
Hi Liu,
On Mon, Feb 28, 2022 at 1:02 AM Liu Ying victor.liu@oss.nxp.com wrote:
On Tue, 2022-02-15 at 15:54 -0800, Brian Norris wrote:
--- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1011,9 +1011,19 @@ crtc_needs_disable(struct drm_crtc_state *old_state, return drm_atomic_crtc_effectively_active(old_state);
/*
* We need to run through the crtc_funcs->disable() function if the CRTC* is currently on, if it's transitioning to self refresh mode, or if* it's in self refresh mode and needs to be fully disabled.
* We need to disable bridge(s) and CRTC if we're transitioning out of* self-refresh and changing CRTCs at the same time, because the* bridge tracks self-refresh status via CRTC state.*/if (old_state->self_refresh_active && new_state->enable &&old_state->crtc != new_state->crtc)return true;I think 'new_state->enable' should be changed to 'new_state->active', because 'active' is the one to enable/disable the CRTC while 'enable' reflects whether a mode blob is set to CRTC state. The overall logic added above is ok to me. Let's see if others have any comments.
Thanks for the review, and good catch. This actually shows that most of my development was before commit 69e630016ef4 ("drm/atomic: Check new_crtc_state->active to determine if CRTC needs disable in self refresh mode"). In fact, the "state->enable" condition was included here mostly as a complement to the "!state->enable" condition that was present previously, and I didn't adapt it properly upon rebase.
In practice, this portion of the condition is not needed at all; we really want to exit PSR on CRTC-switch regardless of the new-CRTC state. So rather than change "enable" to "active", I plan to remove it entirely.
I'll give it some local tests and send v2 eventually.
Thanks, Brian
dri-devel@lists.freedesktop.org
-
Brian Norris -
Liu Ying