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
Changes in v2:
- Drop "->enable" condition in crtc_needs_disable(); this could possibly
be "->active" to reflect the intended hardware state, but it also is a
little over-specific. We want to make a transition through "disabled"
any time we're exiting PSR at the same time as a CRTC switch.
(Thanks Liu Ying)
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(-)
--
2.35.1.574.g5d30c73bfb-goog
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: <[email protected]>
Fixes: 6c836d965bad ("drm/rockchip: Use the helpers for PSR")
Signed-off-by: Brian Norris <[email protected]>
---
(no changes since v1)
.../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);
}
--
2.35.1.574.g5d30c73bfb-goog
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.
Change in v2:
- Drop "->enable" condition; this could possibly be "->active" to
reflect the intended hardware state, but it also is a little
over-specific. We want to make a transition through "disabled" any
time we're exiting PSR at the same time as a CRTC switch.
(Thanks Liu Ying)
Cc: Liu Ying <[email protected]>
Cc: <[email protected]>
Fixes: 1452c25b0e60 ("drm: Add helpers to kick off self refresh mode in drivers")
Signed-off-by: Brian Norris <[email protected]>
---
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..987e4b212e9f 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 &&
+ 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) ||
--
2.35.1.574.g5d30c73bfb-goog
On Mon, 2022-02-28 at 12:25 -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.
>
> Change in v2:
>
> - Drop "->enable" condition; this could possibly be "->active" to
> reflect the intended hardware state, but it also is a little
> over-specific. We want to make a transition through "disabled" any
> time we're exiting PSR at the same time as a CRTC switch.
Cool. I don't see any particular issue regarding the drop.
Liu Ying
> (Thanks Liu Ying)
>
> Cc: Liu Ying <[email protected]>
> Cc: <[email protected]>
> Fixes: 1452c25b0e60 ("drm: Add helpers to kick off self refresh mode in drivers")
> Signed-off-by: Brian Norris <[email protected]>
> ---
> 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..987e4b212e9f 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 &&
> + 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) ||
On Mon, Feb 28, 2022 at 12:25 PM Brian Norris <[email protected]> wrote:
>
> 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
Ping for review? Sean, perhaps? (You already reviewed this on the
Chromium tracker.)
Brian
On Thu, Mar 10, 2022 at 3:50 PM Brian Norris <[email protected]> wrote:
> On Mon, Feb 28, 2022 at 12:25 PM Brian Norris <[email protected]> wrote:
> Ping for review? Sean, perhaps? (You already reviewed this on the
> Chromium tracker.)
Ping
On Mon, May 23, 2022 at 5:51 PM Brian Norris <[email protected]> wrote:
>
> On Thu, Mar 10, 2022 at 3:50 PM Brian Norris <[email protected]> wrote:
> > On Mon, Feb 28, 2022 at 12:25 PM Brian Norris <[email protected]> wrote:
>
> > Ping for review? Sean, perhaps? (You already reviewed this on the
> > Chromium tracker.)
>
> Ping
Apologies for the delay. Please in future ping on irc/chat if you're
waiting for review from me, my inbox is often neglected.
The set still looks good to me,
Reviewed-by: Sean Paul <[email protected]>
Sean
Hi,
On Fri, Jun 3, 2022 at 8:11 AM Sean Paul <[email protected]> wrote:
>
> On Mon, May 23, 2022 at 5:51 PM Brian Norris <[email protected]> wrote:
> >
> > On Thu, Mar 10, 2022 at 3:50 PM Brian Norris <[email protected]> wrote:
> > > On Mon, Feb 28, 2022 at 12:25 PM Brian Norris <[email protected]> wrote:
> >
> > > Ping for review? Sean, perhaps? (You already reviewed this on the
> > > Chromium tracker.)
> >
> > Ping
>
> Apologies for the delay. Please in future ping on irc/chat if you're
> waiting for review from me, my inbox is often neglected.
>
> The set still looks good to me,
>
> Reviewed-by: Sean Paul <[email protected]>
Unless someone yells, I'll plan to apply both patches to
drm-misc-fixes early next week, possibly Monday. Seems like if someone
was going to object to these they've had plenty of time up until now.
-Doug
Hi,
On Fri, Jun 3, 2022 at 8:17 AM Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Fri, Jun 3, 2022 at 8:11 AM Sean Paul <[email protected]> wrote:
> >
> > On Mon, May 23, 2022 at 5:51 PM Brian Norris <[email protected]> wrote:
> > >
> > > On Thu, Mar 10, 2022 at 3:50 PM Brian Norris <[email protected]> wrote:
> > > > On Mon, Feb 28, 2022 at 12:25 PM Brian Norris <[email protected]> wrote:
> > >
> > > > Ping for review? Sean, perhaps? (You already reviewed this on the
> > > > Chromium tracker.)
> > >
> > > Ping
> >
> > Apologies for the delay. Please in future ping on irc/chat if you're
> > waiting for review from me, my inbox is often neglected.
> >
> > The set still looks good to me,
> >
> > Reviewed-by: Sean Paul <[email protected]>
>
> Unless someone yells, I'll plan to apply both patches to
> drm-misc-fixes early next week, possibly Monday. Seems like if someone
> was going to object to these they've had plenty of time up until now.
As promised, I pushed these to drm-misc-fixes:
e54a4424925a drm/atomic: Force bridge self-refresh-exit on CRTC switch
ca871659ec16 drm/bridge: analogix_dp: Support PSR-exit to disable transition
-Doug
On Mon, Jun 6, 2022 at 1:30 PM Doug Anderson <[email protected]> wrote:
> On Fri, Jun 3, 2022 at 8:17 AM Doug Anderson <[email protected]> wrote:
> > On Fri, Jun 3, 2022 at 8:11 AM Sean Paul <[email protected]> wrote:
> > > Apologies for the delay. Please in future ping on irc/chat if you're
> > > waiting for review from me, my inbox is often neglected.
OK, I'll try to keep that in mind. I can't help myself with the
semi-relevant XKCD though ;)
https://xkcd.com/1254/
> > > The set still looks good to me,
> > >
> > > Reviewed-by: Sean Paul <[email protected]>
Thanks!
> > Unless someone yells, I'll plan to apply both patches to
> > drm-misc-fixes early next week, possibly Monday. Seems like if someone
> > was going to object to these they've had plenty of time up until now.
>
> As promised, I pushed these to drm-misc-fixes:
>
> e54a4424925a drm/atomic: Force bridge self-refresh-exit on CRTC switch
> ca871659ec16 drm/bridge: analogix_dp: Support PSR-exit to disable transition
And thanks, Doug.
Brian