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@xxxxxxxxxxxxxxx>
> Fixes: 1452c25b0e60 ("drm: Add helpers to kick off self refresh mode in drivers")
> Signed-off-by: Brian Norris <briannorris@xxxxxxxxxxxx>
> ---
>
> 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.
> */
> return old_state->active ||
> (old_state->self_refresh_active && !new_state->active) ||
