diff mbox series

[v2,3/5] drm/rockchip: Use the helpers for PSR

Message ID 20190326204509.96515-3-sean@poorly.run (mailing list archive)
State New, archived
Headers show
Series [v2,1/5] drm: Add helpers to kick off self refresh mode in drivers | expand

Commit Message

Sean Paul March 26, 2019, 8:44 p.m. UTC
From: Sean Paul <seanpaul@chromium.org>

Instead of rolling our own implementation for tracking when PSR should
be [in]active, use the new self refresh helpers to do the heavy lifting.

Changes in v2:
- updated to reflect changes made in the helpers

Link to v1: https://patchwork.freedesktop.org/patch/msgid/20190228210939.83386-4-sean@poorly.run

Cc: Zain Wang <wzz@rock-chips.com>
Cc: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 .../drm/bridge/analogix/analogix_dp_core.c    | 213 ++++++++-----
 .../drm/bridge/analogix/analogix_dp_core.h    |   2 +-
 drivers/gpu/drm/rockchip/Makefile             |   3 +-
 .../gpu/drm/rockchip/analogix_dp-rockchip.c   |  70 ++---
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c    |  16 -
 drivers/gpu/drm/rockchip/rockchip_drm_psr.c   | 290 ------------------
 drivers/gpu/drm/rockchip/rockchip_drm_psr.h   |  30 --
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c   |  33 +-
 include/drm/bridge/analogix_dp.h              |   4 +-
 9 files changed, 185 insertions(+), 476 deletions(-)
 delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_psr.c
 delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_psr.h

Comments

Heiko Stübner March 29, 2019, 6:51 p.m. UTC | #1
Hi,

Am Dienstag, 26. März 2019, 21:44:56 CET schrieb Sean Paul:
> From: Sean Paul <seanpaul@chromium.org>
> 
> Instead of rolling our own implementation for tracking when PSR should
> be [in]active, use the new self refresh helpers to do the heavy lifting.

I only got patches 3-5 and had to pull 1+2 from patchwork, the following
applies to the whole series though.

While my Kevin display does still generally work with these 5 patches
applied functionality has regressed somehow.

Environment is a standard Debian with framebuffer console and sddm
login manager.

With this patchset applied, when sddm starts I only get the see the
login screen after I move the mouse, similarly the blinking cursor in
the password field also only blinks when I actively move the cursor
and stops when there is no mouse movement.

With all 5 patches reverted this goes back to normal.

So it looks like the PSR gets a bit too agressive with these?


Heiko

> Changes in v2:
> - updated to reflect changes made in the helpers
> 
> Link to v1: https://patchwork.freedesktop.org/patch/msgid/20190228210939.83386-4-sean@poorly.run
> 
> Cc: Zain Wang <wzz@rock-chips.com>
> Cc: Tomasz Figa <tfiga@chromium.org>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  .../drm/bridge/analogix/analogix_dp_core.c    | 213 ++++++++-----
>  .../drm/bridge/analogix/analogix_dp_core.h    |   2 +-
>  drivers/gpu/drm/rockchip/Makefile             |   3 +-
>  .../gpu/drm/rockchip/analogix_dp-rockchip.c   |  70 ++---
>  drivers/gpu/drm/rockchip/rockchip_drm_fb.c    |  16 -
>  drivers/gpu/drm/rockchip/rockchip_drm_psr.c   | 290 ------------------
>  drivers/gpu/drm/rockchip/rockchip_drm_psr.h   |  30 --
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c   |  33 +-
>  include/drm/bridge/analogix_dp.h              |   4 +-
>  9 files changed, 185 insertions(+), 476 deletions(-)
>  delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_psr.c
>  delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_psr.h
> 
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index af34554a5a02..c503fe3bc3b3 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -106,63 +106,13 @@ static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
>  	return 0;
>  }
>  
> -int analogix_dp_psr_enabled(struct analogix_dp_device *dp)
> +bool analogix_dp_self_refresh_changed(struct analogix_dp_device *dp)
>  {
> -
> -	return dp->psr_enable;
> +	return dp->connector.state->self_refresh_changed;
>  }
> -EXPORT_SYMBOL_GPL(analogix_dp_psr_enabled);
> +EXPORT_SYMBOL_GPL(analogix_dp_self_refresh_changed);
>  
> -int analogix_dp_enable_psr(struct analogix_dp_device *dp)
> -{
> -	struct edp_vsc_psr psr_vsc;
> -
> -	if (!dp->psr_enable)
> -		return 0;
> -
> -	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
> -	memset(&psr_vsc, 0, sizeof(psr_vsc));
> -	psr_vsc.sdp_header.HB0 = 0;
> -	psr_vsc.sdp_header.HB1 = 0x7;
> -	psr_vsc.sdp_header.HB2 = 0x2;
> -	psr_vsc.sdp_header.HB3 = 0x8;
> -
> -	psr_vsc.DB0 = 0;
> -	psr_vsc.DB1 = EDP_VSC_PSR_STATE_ACTIVE | EDP_VSC_PSR_CRC_VALUES_VALID;
> -
> -	return analogix_dp_send_psr_spd(dp, &psr_vsc, true);
> -}
> -EXPORT_SYMBOL_GPL(analogix_dp_enable_psr);
> -
> -int analogix_dp_disable_psr(struct analogix_dp_device *dp)
> -{
> -	struct edp_vsc_psr psr_vsc;
> -	int ret;
> -
> -	if (!dp->psr_enable)
> -		return 0;
> -
> -	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
> -	memset(&psr_vsc, 0, sizeof(psr_vsc));
> -	psr_vsc.sdp_header.HB0 = 0;
> -	psr_vsc.sdp_header.HB1 = 0x7;
> -	psr_vsc.sdp_header.HB2 = 0x2;
> -	psr_vsc.sdp_header.HB3 = 0x8;
> -
> -	psr_vsc.DB0 = 0;
> -	psr_vsc.DB1 = 0;
> -
> -	ret = drm_dp_dpcd_writeb(&dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
> -	if (ret != 1) {
> -		dev_err(dp->dev, "Failed to set DP Power0 %d\n", ret);
> -		return ret;
> -	}
> -
> -	return analogix_dp_send_psr_spd(dp, &psr_vsc, false);
> -}
> -EXPORT_SYMBOL_GPL(analogix_dp_disable_psr);
> -
> -static int analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
> +static bool analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
>  {
>  	unsigned char psr_version;
>  	int ret;
> @@ -170,14 +120,11 @@ static int analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
>  	ret = drm_dp_dpcd_readb(&dp->aux, DP_PSR_SUPPORT, &psr_version);
>  	if (ret != 1) {
>  		dev_err(dp->dev, "failed to get PSR version, disable it\n");
> -		return ret;
> +		return false;
>  	}
>  
>  	dev_dbg(dp->dev, "Panel PSR version : %x\n", psr_version);
> -
> -	dp->psr_enable = (psr_version & DP_PSR_IS_SUPPORTED) ? true : false;
> -
> -	return 0;
> +	return psr_version & DP_PSR_IS_SUPPORTED;
>  }
>  
>  static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)
> @@ -200,7 +147,7 @@ static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)
>  	}
>  
>  	/* Main-Link transmitter remains active during PSR active states */
> -	psr_en = DP_PSR_MAIN_LINK_ACTIVE | DP_PSR_CRC_VERIFICATION;
> +	psr_en = DP_PSR_CRC_VERIFICATION;
>  	ret = drm_dp_dpcd_writeb(&dp->aux, DP_PSR_EN_CFG, psr_en);
>  	if (ret != 1) {
>  		dev_err(dp->dev, "failed to set panel psr\n");
> @@ -208,8 +155,7 @@ static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)
>  	}
>  
>  	/* Enable psr function */
> -	psr_en = DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE |
> -		 DP_PSR_CRC_VERIFICATION;
> +	psr_en = DP_PSR_ENABLE | DP_PSR_CRC_VERIFICATION;
>  	ret = drm_dp_dpcd_writeb(&dp->aux, DP_PSR_EN_CFG, psr_en);
>  	if (ret != 1) {
>  		dev_err(dp->dev, "failed to set panel psr\n");
> @@ -218,10 +164,11 @@ static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)
>  
>  	analogix_dp_enable_psr_crc(dp);
>  
> +	dp->psr_supported = true;
> +
>  	return 0;
>  end:
>  	dev_err(dp->dev, "enable psr fail, force to disable psr\n");
> -	dp->psr_enable = false;
>  
>  	return ret;
>  }
> @@ -1036,25 +983,90 @@ static int analogix_dp_commit(struct analogix_dp_device *dp)
>  		}
>  	}
>  
> -	ret = analogix_dp_detect_sink_psr(dp);
> -	if (ret)
> -		return ret;
> -
>  	/* Check whether panel supports fast training */
>  	ret = analogix_dp_fast_link_train_detection(dp);
>  	if (ret)
> -		dp->psr_enable = false;
> +		return ret;
>  
> -	if (dp->psr_enable) {
> +	if (analogix_dp_detect_sink_psr(dp)) {
>  		ret = analogix_dp_enable_sink_psr(dp);
>  		if (ret)
>  			return ret;
>  	}
>  
> +	return ret;
> +}
> +
> +static int analogix_dp_enable_psr(struct analogix_dp_device *dp)
> +{
> +	struct edp_vsc_psr psr_vsc;
> +	int ret;
> +	u8 sink;
> +
> +	ret = drm_dp_dpcd_readb(&dp->aux, DP_PSR_STATUS, &sink);
> +	if (ret != 1)
> +		DRM_DEV_ERROR(dp->dev, "Failed to read psr status %d\n", ret);
> +	else if (sink == DP_PSR_SINK_ACTIVE_RFB)
> +		return 0;
> +
> +	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
> +	memset(&psr_vsc, 0, sizeof(psr_vsc));
> +	psr_vsc.sdp_header.HB0 = 0;
> +	psr_vsc.sdp_header.HB1 = 0x7;
> +	psr_vsc.sdp_header.HB2 = 0x2;
> +	psr_vsc.sdp_header.HB3 = 0x8;
> +	psr_vsc.DB0 = 0;
> +	psr_vsc.DB1 = EDP_VSC_PSR_STATE_ACTIVE | EDP_VSC_PSR_CRC_VALUES_VALID;
> +
> +	ret = analogix_dp_send_psr_spd(dp, &psr_vsc, true);
> +	if (!ret)
> +		analogix_dp_set_analog_power_down(dp, POWER_ALL, true);
>  
>  	return ret;
>  }
>  
> +static int analogix_dp_disable_psr(struct analogix_dp_device *dp)
> +{
> +	struct edp_vsc_psr psr_vsc;
> +	int ret;
> +	u8 sink;
> +
> +	analogix_dp_set_analog_power_down(dp, POWER_ALL, false);
> +
> +	ret = drm_dp_dpcd_writeb(&dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
> +	if (ret != 1) {
> +		DRM_DEV_ERROR(dp->dev, "Failed to set DP Power0 %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = drm_dp_dpcd_readb(&dp->aux, DP_PSR_STATUS, &sink);
> +	if (ret != 1) {
> +		DRM_DEV_ERROR(dp->dev, "Failed to read psr status %d\n", ret);
> +		return ret;
> +	} else if (sink == DP_PSR_SINK_INACTIVE) {
> +		DRM_DEV_ERROR(dp->dev, "sink inactive, skip disable psr");
> +		return 0;
> +	}
> +
> +	ret = analogix_dp_train_link(dp);
> +	if (ret) {
> +		DRM_DEV_ERROR(dp->dev, "Failed to train the link %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
> +	memset(&psr_vsc, 0, sizeof(psr_vsc));
> +	psr_vsc.sdp_header.HB0 = 0;
> +	psr_vsc.sdp_header.HB1 = 0x7;
> +	psr_vsc.sdp_header.HB2 = 0x2;
> +	psr_vsc.sdp_header.HB3 = 0x8;
> +
> +	psr_vsc.DB0 = 0;
> +	psr_vsc.DB1 = 0;
> +
> +	return analogix_dp_send_psr_spd(dp, &psr_vsc, true);
> +}
> +
>  /*
>   * This function is a bit of a catch-all for panel preparation, hopefully
>   * simplifying the logic of functions that need to prepare/unprepare the panel
> @@ -1145,9 +1157,23 @@ analogix_dp_best_encoder(struct drm_connector *connector)
>  	return dp->encoder;
>  }
>  
> +
> +int analogix_dp_atomic_check(struct drm_connector *connector,
> +			     struct drm_connector_state *state)
> +{
> +	struct analogix_dp_device *dp = to_dp(connector);
> +
> +	state->self_refresh_aware = true;
> +	if (state->self_refresh_active && !dp->psr_supported)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static const struct drm_connector_helper_funcs analogix_dp_connector_helper_funcs = {
>  	.get_modes = analogix_dp_get_modes,
>  	.best_encoder = analogix_dp_best_encoder,
> +	.atomic_check = analogix_dp_atomic_check,
>  };
>  
>  static enum drm_connector_status
> @@ -1244,6 +1270,10 @@ static void analogix_dp_bridge_pre_enable(struct drm_bridge *bridge)
>  	struct analogix_dp_device *dp = bridge->driver_private;
>  	int ret;
>  
> +	/* Don't touch the panel if we're coming back from PSR */
> +	if (dp->connector.state->self_refresh_changed)
> +		return;
> +
>  	ret = analogix_dp_prepare_panel(dp, true, true);
>  	if (ret)
>  		DRM_ERROR("failed to setup the panel ret = %d\n", ret);
> @@ -1309,6 +1339,14 @@ static void analogix_dp_bridge_enable(struct drm_bridge *bridge)
>  	struct analogix_dp_device *dp = bridge->driver_private;
>  	int timeout_loop = 0;
>  
> +	/* Not a full enable, just disable PSR and continue */
> +	if (dp->connector.state->self_refresh_changed) {
> +		int ret = analogix_dp_disable_psr(dp);
> +		if (ret)
> +			DRM_ERROR("Failed to disable psr %d\n", ret);
> +		return;
> +	}
> +
>  	if (dp->dpms_mode == DRM_MODE_DPMS_ON)
>  		return;
>  
> @@ -1356,11 +1394,37 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
>  	if (ret)
>  		DRM_ERROR("failed to setup the panel ret = %d\n", ret);
>  
> -	dp->psr_enable = false;
>  	dp->fast_train_enable = false;
> +	dp->psr_supported = false;
>  	dp->dpms_mode = DRM_MODE_DPMS_OFF;
>  }
>  
> +static void analogix_dp_bridge_atomic_disable(struct drm_bridge *bridge)
> +{
> +	struct analogix_dp_device *dp = bridge->driver_private;
> +	struct drm_connector_state *conn_state = dp->connector.state;
> +
> +	/* Don't do a full disable on PSR transitions */
> +	if (conn_state->crtc && conn_state->self_refresh_changed)
> +		return;
> +
> +	analogix_dp_bridge_disable(bridge);
> +}
> +
> +static void analogix_dp_bridge_post_disable(struct drm_bridge *bridge)
> +{
> +	struct analogix_dp_device *dp = bridge->driver_private;
> +	struct drm_connector_state *conn_state = dp->connector.state;
> +	int ret;
> +
> +	if (conn_state->self_refresh_active) {
> +		ret = analogix_dp_enable_psr(dp);
> +		if (ret)
> +			DRM_ERROR("Failed to enable psr (%d)\n", ret);
> +		return;
> +	}
> +}
> +
>  static void analogix_dp_bridge_mode_set(struct drm_bridge *bridge,
>  				const struct drm_display_mode *orig_mode,
>  				const struct drm_display_mode *mode)
> @@ -1440,16 +1504,11 @@ static void analogix_dp_bridge_mode_set(struct drm_bridge *bridge,
>  		video->interlaced = true;
>  }
>  
> -static void analogix_dp_bridge_nop(struct drm_bridge *bridge)
> -{
> -	/* do nothing */
> -}
> -
>  static const struct drm_bridge_funcs analogix_dp_bridge_funcs = {
>  	.pre_enable = analogix_dp_bridge_pre_enable,
>  	.enable = analogix_dp_bridge_enable,
> -	.disable = analogix_dp_bridge_disable,
> -	.post_disable = analogix_dp_bridge_nop,
> +	.disable = analogix_dp_bridge_atomic_disable,
> +	.post_disable = analogix_dp_bridge_post_disable,
>  	.mode_set = analogix_dp_bridge_mode_set,
>  	.attach = analogix_dp_bridge_attach,
>  };
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> index 769255dc6e99..d937fa3d9ee8 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> @@ -173,8 +173,8 @@ struct analogix_dp_device {
>  	int			dpms_mode;
>  	int			hpd_gpio;
>  	bool                    force_hpd;
> -	bool			psr_enable;
>  	bool			fast_train_enable;
> +	bool			psr_supported;
>  
>  	struct mutex		panel_lock;
>  	bool			panel_is_modeset;
> diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile
> index f6fc9d5dd0ad..a777726c23cb 100644
> --- a/drivers/gpu/drm/rockchip/Makefile
> +++ b/drivers/gpu/drm/rockchip/Makefile
> @@ -4,8 +4,7 @@
>  # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
>  
>  rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
> -		rockchip_drm_gem.o rockchip_drm_psr.o \
> -		rockchip_drm_vop.o rockchip_vop_reg.o
> +		rockchip_drm_gem.o rockchip_drm_vop.o rockchip_vop_reg.o
>  rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o
>  
>  rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> index bc4423624209..8f42c38848db 100644
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -32,7 +32,6 @@
>  #include <drm/bridge/analogix_dp.h>
>  
>  #include "rockchip_drm_drv.h"
> -#include "rockchip_drm_psr.h"
>  #include "rockchip_drm_vop.h"
>  
>  #define RK3288_GRF_SOC_CON6		0x25c
> @@ -77,29 +76,6 @@ struct rockchip_dp_device {
>  	struct analogix_dp_plat_data plat_data;
>  };
>  
> -static int analogix_dp_psr_set(struct drm_encoder *encoder, bool enabled)
> -{
> -	struct rockchip_dp_device *dp = to_dp(encoder);
> -	int ret;
> -
> -	if (!analogix_dp_psr_enabled(dp->adp))
> -		return 0;
> -
> -	DRM_DEV_DEBUG(dp->dev, "%s PSR...\n", enabled ? "Entry" : "Exit");
> -
> -	ret = rockchip_drm_wait_vact_end(dp->encoder.crtc,
> -					 PSR_WAIT_LINE_FLAG_TIMEOUT_MS);
> -	if (ret) {
> -		DRM_DEV_ERROR(dp->dev, "line flag interrupt did not arrive\n");
> -		return -ETIMEDOUT;
> -	}
> -
> -	if (enabled)
> -		return analogix_dp_enable_psr(dp->adp);
> -	else
> -		return analogix_dp_disable_psr(dp->adp);
> -}
> -
>  static int rockchip_dp_pre_init(struct rockchip_dp_device *dp)
>  {
>  	reset_control_assert(dp->rst);
> @@ -130,21 +106,9 @@ static int rockchip_dp_poweron_start(struct analogix_dp_plat_data *plat_data)
>  	return ret;
>  }
>  
> -static int rockchip_dp_poweron_end(struct analogix_dp_plat_data *plat_data)
> -{
> -	struct rockchip_dp_device *dp = to_dp(plat_data);
> -
> -	return rockchip_drm_psr_inhibit_put(&dp->encoder);
> -}
> -
>  static int rockchip_dp_powerdown(struct analogix_dp_plat_data *plat_data)
>  {
>  	struct rockchip_dp_device *dp = to_dp(plat_data);
> -	int ret;
> -
> -	ret = rockchip_drm_psr_inhibit_get(&dp->encoder);
> -	if (ret != 0)
> -		return ret;
>  
>  	clk_disable_unprepare(dp->pclk);
>  
> @@ -190,6 +154,9 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder)
>  	int ret;
>  	u32 val;
>  
> +	if (analogix_dp_self_refresh_changed(dp->adp))
> +		return;
> +
>  	ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
>  	if (ret < 0)
>  		return;
> @@ -214,9 +181,24 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder)
>  	clk_disable_unprepare(dp->grfclk);
>  }
>  
> -static void rockchip_dp_drm_encoder_nop(struct drm_encoder *encoder)
> +static void rockchip_dp_drm_encoder_disable(struct drm_encoder *encoder)
>  {
> -	/* do nothing */
> +	struct rockchip_dp_device *dp = to_dp(encoder);
> +	struct drm_crtc *crtc;
> +	int ret;
> +
> +	drm_for_each_crtc(crtc, dp->drm_dev) {
> +		if (!(crtc->state->encoder_mask & drm_encoder_mask(encoder)))
> +			continue;
> +
> +		if (!crtc->state->self_refresh_active)
> +			continue;
> +
> +		ret = rockchip_drm_wait_vact_end(crtc,
> +						 PSR_WAIT_LINE_FLAG_TIMEOUT_MS);
> +		if (ret)
> +			DRM_DEV_ERROR(dp->dev, "line flag irq timed out\n");
> +	}
>  }
>  
>  static int
> @@ -246,7 +228,7 @@ static struct drm_encoder_helper_funcs rockchip_dp_encoder_helper_funcs = {
>  	.mode_fixup = rockchip_dp_drm_encoder_mode_fixup,
>  	.mode_set = rockchip_dp_drm_encoder_mode_set,
>  	.enable = rockchip_dp_drm_encoder_enable,
> -	.disable = rockchip_dp_drm_encoder_nop,
> +	.disable = rockchip_dp_drm_encoder_disable,
>  	.atomic_check = rockchip_dp_drm_encoder_atomic_check,
>  };
>  
> @@ -338,23 +320,16 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
>  
>  	dp->plat_data.dev_type = dp->data->chip_type;
>  	dp->plat_data.power_on_start = rockchip_dp_poweron_start;
> -	dp->plat_data.power_on_end = rockchip_dp_poweron_end;
>  	dp->plat_data.power_off = rockchip_dp_powerdown;
>  	dp->plat_data.get_modes = rockchip_dp_get_modes;
>  
> -	ret = rockchip_drm_psr_register(&dp->encoder, analogix_dp_psr_set);
> -	if (ret < 0)
> -		goto err_cleanup_encoder;
> -
>  	dp->adp = analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
>  	if (IS_ERR(dp->adp)) {
>  		ret = PTR_ERR(dp->adp);
> -		goto err_unreg_psr;
> +		goto err_cleanup_encoder;
>  	}
>  
>  	return 0;
> -err_unreg_psr:
> -	rockchip_drm_psr_unregister(&dp->encoder);
>  err_cleanup_encoder:
>  	dp->encoder.funcs->destroy(&dp->encoder);
>  	return ret;
> @@ -366,7 +341,6 @@ static void rockchip_dp_unbind(struct device *dev, struct device *master,
>  	struct rockchip_dp_device *dp = dev_get_drvdata(dev);
>  
>  	analogix_dp_unbind(dp->adp);
> -	rockchip_drm_psr_unregister(&dp->encoder);
>  	dp->encoder.funcs->destroy(&dp->encoder);
>  
>  	dp->adp = ERR_PTR(-ENODEV);
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> index 97438bbbe389..7e121875e3c9 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -23,22 +23,10 @@
>  #include "rockchip_drm_drv.h"
>  #include "rockchip_drm_fb.h"
>  #include "rockchip_drm_gem.h"
> -#include "rockchip_drm_psr.h"
> -
> -static int rockchip_drm_fb_dirty(struct drm_framebuffer *fb,
> -				 struct drm_file *file,
> -				 unsigned int flags, unsigned int color,
> -				 struct drm_clip_rect *clips,
> -				 unsigned int num_clips)
> -{
> -	rockchip_drm_psr_flush_all(fb->dev);
> -	return 0;
> -}
>  
>  static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
>  	.destroy       = drm_gem_fb_destroy,
>  	.create_handle = drm_gem_fb_create_handle,
> -	.dirty	       = rockchip_drm_fb_dirty,
>  };
>  
>  static struct drm_framebuffer *
> @@ -132,8 +120,6 @@ rockchip_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
>  {
>  	struct drm_device *dev = old_state->dev;
>  
> -	rockchip_drm_psr_inhibit_get_state(old_state);
> -
>  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
>  
>  	drm_atomic_helper_commit_modeset_enables(dev, old_state);
> @@ -141,8 +127,6 @@ rockchip_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
>  	drm_atomic_helper_commit_planes(dev, old_state,
>  					DRM_PLANE_COMMIT_ACTIVE_ONLY);
>  
> -	rockchip_drm_psr_inhibit_put_state(old_state);
> -
>  	drm_atomic_helper_commit_hw_done(old_state);
>  
>  	drm_atomic_helper_wait_for_vblanks(dev, old_state);
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> deleted file mode 100644
> index a0c8bd235b67..000000000000
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> +++ /dev/null
> @@ -1,290 +0,0 @@
> -/*
> - * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
> - * Author: Yakir Yang <ykk@rock-chips.com>
> - *
> - * This software is licensed under the terms of the GNU General Public
> - * License version 2, as published by the Free Software Foundation, and
> - * may be copied, distributed, and modified under those terms.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - * GNU General Public License for more details.
> - */
> -
> -#include <drm/drmP.h>
> -#include <drm/drm_atomic.h>
> -#include <drm/drm_probe_helper.h>
> -
> -#include "rockchip_drm_drv.h"
> -#include "rockchip_drm_psr.h"
> -
> -#define PSR_FLUSH_TIMEOUT_MS	100
> -
> -struct psr_drv {
> -	struct list_head	list;
> -	struct drm_encoder	*encoder;
> -
> -	struct mutex		lock;
> -	int			inhibit_count;
> -	bool			enabled;
> -
> -	struct delayed_work	flush_work;
> -
> -	int (*set)(struct drm_encoder *encoder, bool enable);
> -};
> -
> -static struct psr_drv *find_psr_by_encoder(struct drm_encoder *encoder)
> -{
> -	struct rockchip_drm_private *drm_drv = encoder->dev->dev_private;
> -	struct psr_drv *psr;
> -
> -	mutex_lock(&drm_drv->psr_list_lock);
> -	list_for_each_entry(psr, &drm_drv->psr_list, list) {
> -		if (psr->encoder == encoder)
> -			goto out;
> -	}
> -	psr = ERR_PTR(-ENODEV);
> -
> -out:
> -	mutex_unlock(&drm_drv->psr_list_lock);
> -	return psr;
> -}
> -
> -static int psr_set_state_locked(struct psr_drv *psr, bool enable)
> -{
> -	int ret;
> -
> -	if (psr->inhibit_count > 0)
> -		return -EINVAL;
> -
> -	if (enable == psr->enabled)
> -		return 0;
> -
> -	ret = psr->set(psr->encoder, enable);
> -	if (ret)
> -		return ret;
> -
> -	psr->enabled = enable;
> -	return 0;
> -}
> -
> -static void psr_flush_handler(struct work_struct *work)
> -{
> -	struct psr_drv *psr = container_of(to_delayed_work(work),
> -					   struct psr_drv, flush_work);
> -
> -	mutex_lock(&psr->lock);
> -	psr_set_state_locked(psr, true);
> -	mutex_unlock(&psr->lock);
> -}
> -
> -/**
> - * rockchip_drm_psr_inhibit_put - release PSR inhibit on given encoder
> - * @encoder: encoder to obtain the PSR encoder
> - *
> - * Decrements PSR inhibit count on given encoder. Should be called only
> - * for a PSR inhibit count increment done before. If PSR inhibit counter
> - * reaches zero, PSR flush work is scheduled to make the hardware enter
> - * PSR mode in PSR_FLUSH_TIMEOUT_MS.
> - *
> - * Returns:
> - * Zero on success, negative errno on failure.
> - */
> -int rockchip_drm_psr_inhibit_put(struct drm_encoder *encoder)
> -{
> -	struct psr_drv *psr = find_psr_by_encoder(encoder);
> -
> -	if (IS_ERR(psr))
> -		return PTR_ERR(psr);
> -
> -	mutex_lock(&psr->lock);
> -	--psr->inhibit_count;
> -	WARN_ON(psr->inhibit_count < 0);
> -	if (!psr->inhibit_count)
> -		mod_delayed_work(system_wq, &psr->flush_work,
> -				 PSR_FLUSH_TIMEOUT_MS);
> -	mutex_unlock(&psr->lock);
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL(rockchip_drm_psr_inhibit_put);
> -
> -void rockchip_drm_psr_inhibit_get_state(struct drm_atomic_state *state)
> -{
> -	struct drm_crtc *crtc;
> -	struct drm_crtc_state *crtc_state;
> -	struct drm_encoder *encoder;
> -	u32 encoder_mask = 0;
> -	int i;
> -
> -	for_each_old_crtc_in_state(state, crtc, crtc_state, i) {
> -		encoder_mask |= crtc_state->encoder_mask;
> -		encoder_mask |= crtc->state->encoder_mask;
> -	}
> -
> -	drm_for_each_encoder_mask(encoder, state->dev, encoder_mask)
> -		rockchip_drm_psr_inhibit_get(encoder);
> -}
> -EXPORT_SYMBOL(rockchip_drm_psr_inhibit_get_state);
> -
> -void rockchip_drm_psr_inhibit_put_state(struct drm_atomic_state *state)
> -{
> -	struct drm_crtc *crtc;
> -	struct drm_crtc_state *crtc_state;
> -	struct drm_encoder *encoder;
> -	u32 encoder_mask = 0;
> -	int i;
> -
> -	for_each_old_crtc_in_state(state, crtc, crtc_state, i) {
> -		encoder_mask |= crtc_state->encoder_mask;
> -		encoder_mask |= crtc->state->encoder_mask;
> -	}
> -
> -	drm_for_each_encoder_mask(encoder, state->dev, encoder_mask)
> -		rockchip_drm_psr_inhibit_put(encoder);
> -}
> -EXPORT_SYMBOL(rockchip_drm_psr_inhibit_put_state);
> -
> -/**
> - * rockchip_drm_psr_inhibit_get - acquire PSR inhibit on given encoder
> - * @encoder: encoder to obtain the PSR encoder
> - *
> - * Increments PSR inhibit count on given encoder. This function guarantees
> - * that after it returns PSR is turned off on given encoder and no PSR-related
> - * hardware state change occurs at least until a matching call to
> - * rockchip_drm_psr_inhibit_put() is done.
> - *
> - * Returns:
> - * Zero on success, negative errno on failure.
> - */
> -int rockchip_drm_psr_inhibit_get(struct drm_encoder *encoder)
> -{
> -	struct psr_drv *psr = find_psr_by_encoder(encoder);
> -
> -	if (IS_ERR(psr))
> -		return PTR_ERR(psr);
> -
> -	mutex_lock(&psr->lock);
> -	psr_set_state_locked(psr, false);
> -	++psr->inhibit_count;
> -	mutex_unlock(&psr->lock);
> -	cancel_delayed_work_sync(&psr->flush_work);
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL(rockchip_drm_psr_inhibit_get);
> -
> -static void rockchip_drm_do_flush(struct psr_drv *psr)
> -{
> -	cancel_delayed_work_sync(&psr->flush_work);
> -
> -	mutex_lock(&psr->lock);
> -	if (!psr_set_state_locked(psr, false))
> -		mod_delayed_work(system_wq, &psr->flush_work,
> -				 PSR_FLUSH_TIMEOUT_MS);
> -	mutex_unlock(&psr->lock);
> -}
> -
> -/**
> - * rockchip_drm_psr_flush_all - force to flush all registered PSR encoders
> - * @dev: drm device
> - *
> - * Disable the PSR function for all registered encoders, and then enable the
> - * PSR function back after PSR_FLUSH_TIMEOUT. If encoder PSR state have been
> - * changed during flush time, then keep the state no change after flush
> - * timeout.
> - *
> - * Returns:
> - * Zero on success, negative errno on failure.
> - */
> -void rockchip_drm_psr_flush_all(struct drm_device *dev)
> -{
> -	struct rockchip_drm_private *drm_drv = dev->dev_private;
> -	struct psr_drv *psr;
> -
> -	mutex_lock(&drm_drv->psr_list_lock);
> -	list_for_each_entry(psr, &drm_drv->psr_list, list)
> -		rockchip_drm_do_flush(psr);
> -	mutex_unlock(&drm_drv->psr_list_lock);
> -}
> -EXPORT_SYMBOL(rockchip_drm_psr_flush_all);
> -
> -/**
> - * rockchip_drm_psr_register - register encoder to psr driver
> - * @encoder: encoder that obtain the PSR function
> - * @psr_set: call back to set PSR state
> - *
> - * The function returns with PSR inhibit counter initialized with one
> - * and the caller (typically encoder driver) needs to call
> - * rockchip_drm_psr_inhibit_put() when it becomes ready to accept PSR
> - * enable request.
> - *
> - * Returns:
> - * Zero on success, negative errno on failure.
> - */
> -int rockchip_drm_psr_register(struct drm_encoder *encoder,
> -			int (*psr_set)(struct drm_encoder *, bool enable))
> -{
> -	struct rockchip_drm_private *drm_drv;
> -	struct psr_drv *psr;
> -
> -	if (!encoder || !psr_set)
> -		return -EINVAL;
> -
> -	drm_drv = encoder->dev->dev_private;
> -
> -	psr = kzalloc(sizeof(struct psr_drv), GFP_KERNEL);
> -	if (!psr)
> -		return -ENOMEM;
> -
> -	INIT_DELAYED_WORK(&psr->flush_work, psr_flush_handler);
> -	mutex_init(&psr->lock);
> -
> -	psr->inhibit_count = 1;
> -	psr->enabled = false;
> -	psr->encoder = encoder;
> -	psr->set = psr_set;
> -
> -	mutex_lock(&drm_drv->psr_list_lock);
> -	list_add_tail(&psr->list, &drm_drv->psr_list);
> -	mutex_unlock(&drm_drv->psr_list_lock);
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL(rockchip_drm_psr_register);
> -
> -/**
> - * rockchip_drm_psr_unregister - unregister encoder to psr driver
> - * @encoder: encoder that obtain the PSR function
> - * @psr_set: call back to set PSR state
> - *
> - * It is expected that the PSR inhibit counter is 1 when this function is
> - * called, which corresponds to a state when related encoder has been
> - * disconnected from any CRTCs and its driver called
> - * rockchip_drm_psr_inhibit_get() to stop the PSR logic.
> - *
> - * Returns:
> - * Zero on success, negative errno on failure.
> - */
> -void rockchip_drm_psr_unregister(struct drm_encoder *encoder)
> -{
> -	struct rockchip_drm_private *drm_drv = encoder->dev->dev_private;
> -	struct psr_drv *psr, *n;
> -
> -	mutex_lock(&drm_drv->psr_list_lock);
> -	list_for_each_entry_safe(psr, n, &drm_drv->psr_list, list) {
> -		if (psr->encoder == encoder) {
> -			/*
> -			 * Any other value would mean that the encoder
> -			 * is still in use.
> -			 */
> -			WARN_ON(psr->inhibit_count != 1);
> -
> -			list_del(&psr->list);
> -			kfree(psr);
> -		}
> -	}
> -	mutex_unlock(&drm_drv->psr_list_lock);
> -}
> -EXPORT_SYMBOL(rockchip_drm_psr_unregister);
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.h b/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
> deleted file mode 100644
> index 25350ba3237b..000000000000
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
> +++ /dev/null
> @@ -1,30 +0,0 @@
> -/*
> - * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
> - * Author: Yakir Yang <ykk@rock-chips.com>
> - *
> - * This software is licensed under the terms of the GNU General Public
> - * License version 2, as published by the Free Software Foundation, and
> - * may be copied, distributed, and modified under those terms.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - * GNU General Public License for more details.
> - */
> -
> -#ifndef __ROCKCHIP_DRM_PSR___
> -#define __ROCKCHIP_DRM_PSR___
> -
> -void rockchip_drm_psr_flush_all(struct drm_device *dev);
> -
> -int rockchip_drm_psr_inhibit_put(struct drm_encoder *encoder);
> -int rockchip_drm_psr_inhibit_get(struct drm_encoder *encoder);
> -
> -void rockchip_drm_psr_inhibit_get_state(struct drm_atomic_state *state);
> -void rockchip_drm_psr_inhibit_put_state(struct drm_atomic_state *state);
> -
> -int rockchip_drm_psr_register(struct drm_encoder *encoder,
> -			int (*psr_set)(struct drm_encoder *, bool enable));
> -void rockchip_drm_psr_unregister(struct drm_encoder *encoder);
> -
> -#endif /* __ROCKCHIP_DRM_PSR__ */
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index c7d4c6073ea5..21fccda70356 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -21,6 +21,7 @@
>  #include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_probe_helper.h>
> +#include <drm/drm_self_refresh_helper.h>
>  #ifdef CONFIG_DRM_ANALOGIX_DP
>  #include <drm/bridge/analogix_dp.h>
>  #endif
> @@ -42,10 +43,11 @@
>  #include "rockchip_drm_drv.h"
>  #include "rockchip_drm_gem.h"
>  #include "rockchip_drm_fb.h"
> -#include "rockchip_drm_psr.h"
>  #include "rockchip_drm_vop.h"
>  #include "rockchip_rgb.h"
>  
> +#define VOP_SELF_REFRESH_ENTRY_DELAY_MS 100
> +
>  #define VOP_WIN_SET(vop, win, name, v) \
>  		vop_reg_set(vop, &win->phy->name, win->base, ~0, v, #name)
>  #define VOP_SCL_SET(vop, win, name, v) \
> @@ -541,7 +543,7 @@ static void vop_core_clks_disable(struct vop *vop)
>  	clk_disable(vop->hclk);
>  }
>  
> -static int vop_enable(struct drm_crtc *crtc)
> +static int vop_enable(struct drm_crtc *crtc, struct drm_crtc_state *old_state)
>  {
>  	struct vop *vop = to_vop(crtc);
>  	int ret, i;
> @@ -581,12 +583,18 @@ static int vop_enable(struct drm_crtc *crtc)
>  	 * We need to make sure that all windows are disabled before we
>  	 * enable the crtc. Otherwise we might try to scan from a destroyed
>  	 * buffer later.
> +	 *
> +	 * In the case of enable-after-PSR, we don't need to worry about this
> +	 * case since the buffer is guaranteed to be valid and disabling the
> +	 * window will result in screen glitches on PSR exit.
>  	 */
> -	for (i = 0; i < vop->data->win_size; i++) {
> -		struct vop_win *vop_win = &vop->win[i];
> -		const struct vop_win_data *win = vop_win->data;
> +	if (!old_state || !old_state->self_refresh_active) {
> +		for (i = 0; i < vop->data->win_size; i++) {
> +			struct vop_win *vop_win = &vop->win[i];
> +			const struct vop_win_data *win = vop_win->data;
>  
> -		VOP_WIN_SET(vop, win, enable, 0);
> +			VOP_WIN_SET(vop, win, enable, 0);
> +		}
>  	}
>  	spin_unlock(&vop->reg_lock);
>  
> @@ -937,12 +945,10 @@ static void vop_plane_atomic_async_update(struct drm_plane *plane,
>  	}
>  
>  	if (vop->is_enabled) {
> -		rockchip_drm_psr_inhibit_get_state(new_state->state);
>  		vop_plane_atomic_update(plane, plane->state);
>  		spin_lock(&vop->reg_lock);
>  		vop_cfg_done(vop);
>  		spin_unlock(&vop->reg_lock);
> -		rockchip_drm_psr_inhibit_put_state(new_state->state);
>  	}
>  
>  	plane->funcs->atomic_destroy_state(plane, plane_state);
> @@ -1035,7 +1041,7 @@ static void vop_crtc_atomic_enable(struct drm_crtc *crtc,
>  
>  	WARN_ON(vop->event);
>  
> -	ret = vop_enable(crtc);
> +	ret = vop_enable(crtc, old_state);
>  	if (ret) {
>  		mutex_unlock(&vop->vop_lock);
>  		DRM_DEV_ERROR(vop->dev, "Failed to enable vop (%d)\n", ret);
> @@ -1509,6 +1515,13 @@ static int vop_create_crtc(struct vop *vop)
>  	init_completion(&vop->line_flag_completion);
>  	crtc->port = port;
>  
> +	ret = drm_self_refresh_helper_register(crtc,
> +					       VOP_SELF_REFRESH_ENTRY_DELAY_MS);
> +	if (ret)
> +		DRM_DEV_DEBUG_KMS(vop->dev,
> +			"Failed to register %s with SR helpers %d, ignoring\n",
> +			crtc->name, ret);
> +
>  	return 0;
>  
>  err_cleanup_crtc:
> @@ -1526,6 +1539,8 @@ static void vop_destroy_crtc(struct vop *vop)
>  	struct drm_device *drm_dev = vop->drm_dev;
>  	struct drm_plane *plane, *tmp;
>  
> +	drm_self_refresh_helper_unregister(crtc);
> +
>  	of_node_put(crtc->port);
>  
>  	/*
> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
> index 475b706b49de..b6cc654143d7 100644
> --- a/include/drm/bridge/analogix_dp.h
> +++ b/include/drm/bridge/analogix_dp.h
> @@ -42,9 +42,7 @@ struct analogix_dp_plat_data {
>  			 struct drm_connector *);
>  };
>  
> -int analogix_dp_psr_enabled(struct analogix_dp_device *dp);
> -int analogix_dp_enable_psr(struct analogix_dp_device *dp);
> -int analogix_dp_disable_psr(struct analogix_dp_device *dp);
> +bool analogix_dp_self_refresh_changed(struct analogix_dp_device *dp);
>  
>  int analogix_dp_resume(struct analogix_dp_device *dp);
>  int analogix_dp_suspend(struct analogix_dp_device *dp);
>
Sean Paul March 29, 2019, 7 p.m. UTC | #2
On Fri, Mar 29, 2019 at 07:51:51PM +0100, Heiko Stübner wrote:
> Hi,
> 
> Am Dienstag, 26. März 2019, 21:44:56 CET schrieb Sean Paul:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > Instead of rolling our own implementation for tracking when PSR should
> > be [in]active, use the new self refresh helpers to do the heavy lifting.
> 
> I only got patches 3-5 and had to pull 1+2 from patchwork, the following
> applies to the whole series though.
> 
> While my Kevin display does still generally work with these 5 patches
> applied functionality has regressed somehow.
> 
> Environment is a standard Debian with framebuffer console and sddm
> login manager.

Ahh, this is probably b/c fb_dirty is not triggering PSR exit. I had it
working in v1, but removed it in v2 since Daniel suggested we require the
fb_dirty helpers. I neglected to add support for the fb_dirty helpers, so fbcon
updates won't cause the screen to refresh. I'll add the helpers in v3 and make
sure I test it out with fbcon.

Thanks for reporting this!

Sean

> 
> With this patchset applied, when sddm starts I only get the see the
> login screen after I move the mouse, similarly the blinking cursor in
> the password field also only blinks when I actively move the cursor
> and stops when there is no mouse movement.
> 
> With all 5 patches reverted this goes back to normal.
> 
> So it looks like the PSR gets a bit too agressive with these?
> 
> 
> Heiko
> 
> > Changes in v2:
> > - updated to reflect changes made in the helpers
> > 
> > Link to v1: https://patchwork.freedesktop.org/patch/msgid/20190228210939.83386-4-sean@poorly.run
> > 
> > Cc: Zain Wang <wzz@rock-chips.com>
> > Cc: Tomasz Figa <tfiga@chromium.org>
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> >  .../drm/bridge/analogix/analogix_dp_core.c    | 213 ++++++++-----
> >  .../drm/bridge/analogix/analogix_dp_core.h    |   2 +-
> >  drivers/gpu/drm/rockchip/Makefile             |   3 +-
> >  .../gpu/drm/rockchip/analogix_dp-rockchip.c   |  70 ++---
> >  drivers/gpu/drm/rockchip/rockchip_drm_fb.c    |  16 -
> >  drivers/gpu/drm/rockchip/rockchip_drm_psr.c   | 290 ------------------
> >  drivers/gpu/drm/rockchip/rockchip_drm_psr.h   |  30 --
> >  drivers/gpu/drm/rockchip/rockchip_drm_vop.c   |  33 +-
> >  include/drm/bridge/analogix_dp.h              |   4 +-
> >  9 files changed, 185 insertions(+), 476 deletions(-)
> >  delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> >  delete mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_psr.h
> > 
> > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > index af34554a5a02..c503fe3bc3b3 100644
> > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > @@ -106,63 +106,13 @@ static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
> >  	return 0;
> >  }
> >  
> > -int analogix_dp_psr_enabled(struct analogix_dp_device *dp)
> > +bool analogix_dp_self_refresh_changed(struct analogix_dp_device *dp)
> >  {
> > -
> > -	return dp->psr_enable;
> > +	return dp->connector.state->self_refresh_changed;
> >  }
> > -EXPORT_SYMBOL_GPL(analogix_dp_psr_enabled);
> > +EXPORT_SYMBOL_GPL(analogix_dp_self_refresh_changed);
> >  
> > -int analogix_dp_enable_psr(struct analogix_dp_device *dp)
> > -{
> > -	struct edp_vsc_psr psr_vsc;
> > -
> > -	if (!dp->psr_enable)
> > -		return 0;
> > -
> > -	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
> > -	memset(&psr_vsc, 0, sizeof(psr_vsc));
> > -	psr_vsc.sdp_header.HB0 = 0;
> > -	psr_vsc.sdp_header.HB1 = 0x7;
> > -	psr_vsc.sdp_header.HB2 = 0x2;
> > -	psr_vsc.sdp_header.HB3 = 0x8;
> > -
> > -	psr_vsc.DB0 = 0;
> > -	psr_vsc.DB1 = EDP_VSC_PSR_STATE_ACTIVE | EDP_VSC_PSR_CRC_VALUES_VALID;
> > -
> > -	return analogix_dp_send_psr_spd(dp, &psr_vsc, true);
> > -}
> > -EXPORT_SYMBOL_GPL(analogix_dp_enable_psr);
> > -
> > -int analogix_dp_disable_psr(struct analogix_dp_device *dp)
> > -{
> > -	struct edp_vsc_psr psr_vsc;
> > -	int ret;
> > -
> > -	if (!dp->psr_enable)
> > -		return 0;
> > -
> > -	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
> > -	memset(&psr_vsc, 0, sizeof(psr_vsc));
> > -	psr_vsc.sdp_header.HB0 = 0;
> > -	psr_vsc.sdp_header.HB1 = 0x7;
> > -	psr_vsc.sdp_header.HB2 = 0x2;
> > -	psr_vsc.sdp_header.HB3 = 0x8;
> > -
> > -	psr_vsc.DB0 = 0;
> > -	psr_vsc.DB1 = 0;
> > -
> > -	ret = drm_dp_dpcd_writeb(&dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
> > -	if (ret != 1) {
> > -		dev_err(dp->dev, "Failed to set DP Power0 %d\n", ret);
> > -		return ret;
> > -	}
> > -
> > -	return analogix_dp_send_psr_spd(dp, &psr_vsc, false);
> > -}
> > -EXPORT_SYMBOL_GPL(analogix_dp_disable_psr);
> > -
> > -static int analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
> > +static bool analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
> >  {
> >  	unsigned char psr_version;
> >  	int ret;
> > @@ -170,14 +120,11 @@ static int analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
> >  	ret = drm_dp_dpcd_readb(&dp->aux, DP_PSR_SUPPORT, &psr_version);
> >  	if (ret != 1) {
> >  		dev_err(dp->dev, "failed to get PSR version, disable it\n");
> > -		return ret;
> > +		return false;
> >  	}
> >  
> >  	dev_dbg(dp->dev, "Panel PSR version : %x\n", psr_version);
> > -
> > -	dp->psr_enable = (psr_version & DP_PSR_IS_SUPPORTED) ? true : false;
> > -
> > -	return 0;
> > +	return psr_version & DP_PSR_IS_SUPPORTED;
> >  }
> >  
> >  static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)
> > @@ -200,7 +147,7 @@ static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)
> >  	}
> >  
> >  	/* Main-Link transmitter remains active during PSR active states */
> > -	psr_en = DP_PSR_MAIN_LINK_ACTIVE | DP_PSR_CRC_VERIFICATION;
> > +	psr_en = DP_PSR_CRC_VERIFICATION;
> >  	ret = drm_dp_dpcd_writeb(&dp->aux, DP_PSR_EN_CFG, psr_en);
> >  	if (ret != 1) {
> >  		dev_err(dp->dev, "failed to set panel psr\n");
> > @@ -208,8 +155,7 @@ static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)
> >  	}
> >  
> >  	/* Enable psr function */
> > -	psr_en = DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE |
> > -		 DP_PSR_CRC_VERIFICATION;
> > +	psr_en = DP_PSR_ENABLE | DP_PSR_CRC_VERIFICATION;
> >  	ret = drm_dp_dpcd_writeb(&dp->aux, DP_PSR_EN_CFG, psr_en);
> >  	if (ret != 1) {
> >  		dev_err(dp->dev, "failed to set panel psr\n");
> > @@ -218,10 +164,11 @@ static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)
> >  
> >  	analogix_dp_enable_psr_crc(dp);
> >  
> > +	dp->psr_supported = true;
> > +
> >  	return 0;
> >  end:
> >  	dev_err(dp->dev, "enable psr fail, force to disable psr\n");
> > -	dp->psr_enable = false;
> >  
> >  	return ret;
> >  }
> > @@ -1036,25 +983,90 @@ static int analogix_dp_commit(struct analogix_dp_device *dp)
> >  		}
> >  	}
> >  
> > -	ret = analogix_dp_detect_sink_psr(dp);
> > -	if (ret)
> > -		return ret;
> > -
> >  	/* Check whether panel supports fast training */
> >  	ret = analogix_dp_fast_link_train_detection(dp);
> >  	if (ret)
> > -		dp->psr_enable = false;
> > +		return ret;
> >  
> > -	if (dp->psr_enable) {
> > +	if (analogix_dp_detect_sink_psr(dp)) {
> >  		ret = analogix_dp_enable_sink_psr(dp);
> >  		if (ret)
> >  			return ret;
> >  	}
> >  
> > +	return ret;
> > +}
> > +
> > +static int analogix_dp_enable_psr(struct analogix_dp_device *dp)
> > +{
> > +	struct edp_vsc_psr psr_vsc;
> > +	int ret;
> > +	u8 sink;
> > +
> > +	ret = drm_dp_dpcd_readb(&dp->aux, DP_PSR_STATUS, &sink);
> > +	if (ret != 1)
> > +		DRM_DEV_ERROR(dp->dev, "Failed to read psr status %d\n", ret);
> > +	else if (sink == DP_PSR_SINK_ACTIVE_RFB)
> > +		return 0;
> > +
> > +	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
> > +	memset(&psr_vsc, 0, sizeof(psr_vsc));
> > +	psr_vsc.sdp_header.HB0 = 0;
> > +	psr_vsc.sdp_header.HB1 = 0x7;
> > +	psr_vsc.sdp_header.HB2 = 0x2;
> > +	psr_vsc.sdp_header.HB3 = 0x8;
> > +	psr_vsc.DB0 = 0;
> > +	psr_vsc.DB1 = EDP_VSC_PSR_STATE_ACTIVE | EDP_VSC_PSR_CRC_VALUES_VALID;
> > +
> > +	ret = analogix_dp_send_psr_spd(dp, &psr_vsc, true);
> > +	if (!ret)
> > +		analogix_dp_set_analog_power_down(dp, POWER_ALL, true);
> >  
> >  	return ret;
> >  }
> >  
> > +static int analogix_dp_disable_psr(struct analogix_dp_device *dp)
> > +{
> > +	struct edp_vsc_psr psr_vsc;
> > +	int ret;
> > +	u8 sink;
> > +
> > +	analogix_dp_set_analog_power_down(dp, POWER_ALL, false);
> > +
> > +	ret = drm_dp_dpcd_writeb(&dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
> > +	if (ret != 1) {
> > +		DRM_DEV_ERROR(dp->dev, "Failed to set DP Power0 %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = drm_dp_dpcd_readb(&dp->aux, DP_PSR_STATUS, &sink);
> > +	if (ret != 1) {
> > +		DRM_DEV_ERROR(dp->dev, "Failed to read psr status %d\n", ret);
> > +		return ret;
> > +	} else if (sink == DP_PSR_SINK_INACTIVE) {
> > +		DRM_DEV_ERROR(dp->dev, "sink inactive, skip disable psr");
> > +		return 0;
> > +	}
> > +
> > +	ret = analogix_dp_train_link(dp);
> > +	if (ret) {
> > +		DRM_DEV_ERROR(dp->dev, "Failed to train the link %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
> > +	memset(&psr_vsc, 0, sizeof(psr_vsc));
> > +	psr_vsc.sdp_header.HB0 = 0;
> > +	psr_vsc.sdp_header.HB1 = 0x7;
> > +	psr_vsc.sdp_header.HB2 = 0x2;
> > +	psr_vsc.sdp_header.HB3 = 0x8;
> > +
> > +	psr_vsc.DB0 = 0;
> > +	psr_vsc.DB1 = 0;
> > +
> > +	return analogix_dp_send_psr_spd(dp, &psr_vsc, true);
> > +}
> > +
> >  /*
> >   * This function is a bit of a catch-all for panel preparation, hopefully
> >   * simplifying the logic of functions that need to prepare/unprepare the panel
> > @@ -1145,9 +1157,23 @@ analogix_dp_best_encoder(struct drm_connector *connector)
> >  	return dp->encoder;
> >  }
> >  
> > +
> > +int analogix_dp_atomic_check(struct drm_connector *connector,
> > +			     struct drm_connector_state *state)
> > +{
> > +	struct analogix_dp_device *dp = to_dp(connector);
> > +
> > +	state->self_refresh_aware = true;
> > +	if (state->self_refresh_active && !dp->psr_supported)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> >  static const struct drm_connector_helper_funcs analogix_dp_connector_helper_funcs = {
> >  	.get_modes = analogix_dp_get_modes,
> >  	.best_encoder = analogix_dp_best_encoder,
> > +	.atomic_check = analogix_dp_atomic_check,
> >  };
> >  
> >  static enum drm_connector_status
> > @@ -1244,6 +1270,10 @@ static void analogix_dp_bridge_pre_enable(struct drm_bridge *bridge)
> >  	struct analogix_dp_device *dp = bridge->driver_private;
> >  	int ret;
> >  
> > +	/* Don't touch the panel if we're coming back from PSR */
> > +	if (dp->connector.state->self_refresh_changed)
> > +		return;
> > +
> >  	ret = analogix_dp_prepare_panel(dp, true, true);
> >  	if (ret)
> >  		DRM_ERROR("failed to setup the panel ret = %d\n", ret);
> > @@ -1309,6 +1339,14 @@ static void analogix_dp_bridge_enable(struct drm_bridge *bridge)
> >  	struct analogix_dp_device *dp = bridge->driver_private;
> >  	int timeout_loop = 0;
> >  
> > +	/* Not a full enable, just disable PSR and continue */
> > +	if (dp->connector.state->self_refresh_changed) {
> > +		int ret = analogix_dp_disable_psr(dp);
> > +		if (ret)
> > +			DRM_ERROR("Failed to disable psr %d\n", ret);
> > +		return;
> > +	}
> > +
> >  	if (dp->dpms_mode == DRM_MODE_DPMS_ON)
> >  		return;
> >  
> > @@ -1356,11 +1394,37 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
> >  	if (ret)
> >  		DRM_ERROR("failed to setup the panel ret = %d\n", ret);
> >  
> > -	dp->psr_enable = false;
> >  	dp->fast_train_enable = false;
> > +	dp->psr_supported = false;
> >  	dp->dpms_mode = DRM_MODE_DPMS_OFF;
> >  }
> >  
> > +static void analogix_dp_bridge_atomic_disable(struct drm_bridge *bridge)
> > +{
> > +	struct analogix_dp_device *dp = bridge->driver_private;
> > +	struct drm_connector_state *conn_state = dp->connector.state;
> > +
> > +	/* Don't do a full disable on PSR transitions */
> > +	if (conn_state->crtc && conn_state->self_refresh_changed)
> > +		return;
> > +
> > +	analogix_dp_bridge_disable(bridge);
> > +}
> > +
> > +static void analogix_dp_bridge_post_disable(struct drm_bridge *bridge)
> > +{
> > +	struct analogix_dp_device *dp = bridge->driver_private;
> > +	struct drm_connector_state *conn_state = dp->connector.state;
> > +	int ret;
> > +
> > +	if (conn_state->self_refresh_active) {
> > +		ret = analogix_dp_enable_psr(dp);
> > +		if (ret)
> > +			DRM_ERROR("Failed to enable psr (%d)\n", ret);
> > +		return;
> > +	}
> > +}
> > +
> >  static void analogix_dp_bridge_mode_set(struct drm_bridge *bridge,
> >  				const struct drm_display_mode *orig_mode,
> >  				const struct drm_display_mode *mode)
> > @@ -1440,16 +1504,11 @@ static void analogix_dp_bridge_mode_set(struct drm_bridge *bridge,
> >  		video->interlaced = true;
> >  }
> >  
> > -static void analogix_dp_bridge_nop(struct drm_bridge *bridge)
> > -{
> > -	/* do nothing */
> > -}
> > -
> >  static const struct drm_bridge_funcs analogix_dp_bridge_funcs = {
> >  	.pre_enable = analogix_dp_bridge_pre_enable,
> >  	.enable = analogix_dp_bridge_enable,
> > -	.disable = analogix_dp_bridge_disable,
> > -	.post_disable = analogix_dp_bridge_nop,
> > +	.disable = analogix_dp_bridge_atomic_disable,
> > +	.post_disable = analogix_dp_bridge_post_disable,
> >  	.mode_set = analogix_dp_bridge_mode_set,
> >  	.attach = analogix_dp_bridge_attach,
> >  };
> > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> > index 769255dc6e99..d937fa3d9ee8 100644
> > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> > @@ -173,8 +173,8 @@ struct analogix_dp_device {
> >  	int			dpms_mode;
> >  	int			hpd_gpio;
> >  	bool                    force_hpd;
> > -	bool			psr_enable;
> >  	bool			fast_train_enable;
> > +	bool			psr_supported;
> >  
> >  	struct mutex		panel_lock;
> >  	bool			panel_is_modeset;
> > diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile
> > index f6fc9d5dd0ad..a777726c23cb 100644
> > --- a/drivers/gpu/drm/rockchip/Makefile
> > +++ b/drivers/gpu/drm/rockchip/Makefile
> > @@ -4,8 +4,7 @@
> >  # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
> >  
> >  rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
> > -		rockchip_drm_gem.o rockchip_drm_psr.o \
> > -		rockchip_drm_vop.o rockchip_vop_reg.o
> > +		rockchip_drm_gem.o rockchip_drm_vop.o rockchip_vop_reg.o
> >  rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o
> >  
> >  rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
> > diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> > index bc4423624209..8f42c38848db 100644
> > --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> > +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> > @@ -32,7 +32,6 @@
> >  #include <drm/bridge/analogix_dp.h>
> >  
> >  #include "rockchip_drm_drv.h"
> > -#include "rockchip_drm_psr.h"
> >  #include "rockchip_drm_vop.h"
> >  
> >  #define RK3288_GRF_SOC_CON6		0x25c
> > @@ -77,29 +76,6 @@ struct rockchip_dp_device {
> >  	struct analogix_dp_plat_data plat_data;
> >  };
> >  
> > -static int analogix_dp_psr_set(struct drm_encoder *encoder, bool enabled)
> > -{
> > -	struct rockchip_dp_device *dp = to_dp(encoder);
> > -	int ret;
> > -
> > -	if (!analogix_dp_psr_enabled(dp->adp))
> > -		return 0;
> > -
> > -	DRM_DEV_DEBUG(dp->dev, "%s PSR...\n", enabled ? "Entry" : "Exit");
> > -
> > -	ret = rockchip_drm_wait_vact_end(dp->encoder.crtc,
> > -					 PSR_WAIT_LINE_FLAG_TIMEOUT_MS);
> > -	if (ret) {
> > -		DRM_DEV_ERROR(dp->dev, "line flag interrupt did not arrive\n");
> > -		return -ETIMEDOUT;
> > -	}
> > -
> > -	if (enabled)
> > -		return analogix_dp_enable_psr(dp->adp);
> > -	else
> > -		return analogix_dp_disable_psr(dp->adp);
> > -}
> > -
> >  static int rockchip_dp_pre_init(struct rockchip_dp_device *dp)
> >  {
> >  	reset_control_assert(dp->rst);
> > @@ -130,21 +106,9 @@ static int rockchip_dp_poweron_start(struct analogix_dp_plat_data *plat_data)
> >  	return ret;
> >  }
> >  
> > -static int rockchip_dp_poweron_end(struct analogix_dp_plat_data *plat_data)
> > -{
> > -	struct rockchip_dp_device *dp = to_dp(plat_data);
> > -
> > -	return rockchip_drm_psr_inhibit_put(&dp->encoder);
> > -}
> > -
> >  static int rockchip_dp_powerdown(struct analogix_dp_plat_data *plat_data)
> >  {
> >  	struct rockchip_dp_device *dp = to_dp(plat_data);
> > -	int ret;
> > -
> > -	ret = rockchip_drm_psr_inhibit_get(&dp->encoder);
> > -	if (ret != 0)
> > -		return ret;
> >  
> >  	clk_disable_unprepare(dp->pclk);
> >  
> > @@ -190,6 +154,9 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder)
> >  	int ret;
> >  	u32 val;
> >  
> > +	if (analogix_dp_self_refresh_changed(dp->adp))
> > +		return;
> > +
> >  	ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
> >  	if (ret < 0)
> >  		return;
> > @@ -214,9 +181,24 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder)
> >  	clk_disable_unprepare(dp->grfclk);
> >  }
> >  
> > -static void rockchip_dp_drm_encoder_nop(struct drm_encoder *encoder)
> > +static void rockchip_dp_drm_encoder_disable(struct drm_encoder *encoder)
> >  {
> > -	/* do nothing */
> > +	struct rockchip_dp_device *dp = to_dp(encoder);
> > +	struct drm_crtc *crtc;
> > +	int ret;
> > +
> > +	drm_for_each_crtc(crtc, dp->drm_dev) {
> > +		if (!(crtc->state->encoder_mask & drm_encoder_mask(encoder)))
> > +			continue;
> > +
> > +		if (!crtc->state->self_refresh_active)
> > +			continue;
> > +
> > +		ret = rockchip_drm_wait_vact_end(crtc,
> > +						 PSR_WAIT_LINE_FLAG_TIMEOUT_MS);
> > +		if (ret)
> > +			DRM_DEV_ERROR(dp->dev, "line flag irq timed out\n");
> > +	}
> >  }
> >  
> >  static int
> > @@ -246,7 +228,7 @@ static struct drm_encoder_helper_funcs rockchip_dp_encoder_helper_funcs = {
> >  	.mode_fixup = rockchip_dp_drm_encoder_mode_fixup,
> >  	.mode_set = rockchip_dp_drm_encoder_mode_set,
> >  	.enable = rockchip_dp_drm_encoder_enable,
> > -	.disable = rockchip_dp_drm_encoder_nop,
> > +	.disable = rockchip_dp_drm_encoder_disable,
> >  	.atomic_check = rockchip_dp_drm_encoder_atomic_check,
> >  };
> >  
> > @@ -338,23 +320,16 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
> >  
> >  	dp->plat_data.dev_type = dp->data->chip_type;
> >  	dp->plat_data.power_on_start = rockchip_dp_poweron_start;
> > -	dp->plat_data.power_on_end = rockchip_dp_poweron_end;
> >  	dp->plat_data.power_off = rockchip_dp_powerdown;
> >  	dp->plat_data.get_modes = rockchip_dp_get_modes;
> >  
> > -	ret = rockchip_drm_psr_register(&dp->encoder, analogix_dp_psr_set);
> > -	if (ret < 0)
> > -		goto err_cleanup_encoder;
> > -
> >  	dp->adp = analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
> >  	if (IS_ERR(dp->adp)) {
> >  		ret = PTR_ERR(dp->adp);
> > -		goto err_unreg_psr;
> > +		goto err_cleanup_encoder;
> >  	}
> >  
> >  	return 0;
> > -err_unreg_psr:
> > -	rockchip_drm_psr_unregister(&dp->encoder);
> >  err_cleanup_encoder:
> >  	dp->encoder.funcs->destroy(&dp->encoder);
> >  	return ret;
> > @@ -366,7 +341,6 @@ static void rockchip_dp_unbind(struct device *dev, struct device *master,
> >  	struct rockchip_dp_device *dp = dev_get_drvdata(dev);
> >  
> >  	analogix_dp_unbind(dp->adp);
> > -	rockchip_drm_psr_unregister(&dp->encoder);
> >  	dp->encoder.funcs->destroy(&dp->encoder);
> >  
> >  	dp->adp = ERR_PTR(-ENODEV);
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > index 97438bbbe389..7e121875e3c9 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > @@ -23,22 +23,10 @@
> >  #include "rockchip_drm_drv.h"
> >  #include "rockchip_drm_fb.h"
> >  #include "rockchip_drm_gem.h"
> > -#include "rockchip_drm_psr.h"
> > -
> > -static int rockchip_drm_fb_dirty(struct drm_framebuffer *fb,
> > -				 struct drm_file *file,
> > -				 unsigned int flags, unsigned int color,
> > -				 struct drm_clip_rect *clips,
> > -				 unsigned int num_clips)
> > -{
> > -	rockchip_drm_psr_flush_all(fb->dev);
> > -	return 0;
> > -}
> >  
> >  static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
> >  	.destroy       = drm_gem_fb_destroy,
> >  	.create_handle = drm_gem_fb_create_handle,
> > -	.dirty	       = rockchip_drm_fb_dirty,
> >  };
> >  
> >  static struct drm_framebuffer *
> > @@ -132,8 +120,6 @@ rockchip_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
> >  {
> >  	struct drm_device *dev = old_state->dev;
> >  
> > -	rockchip_drm_psr_inhibit_get_state(old_state);
> > -
> >  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
> >  
> >  	drm_atomic_helper_commit_modeset_enables(dev, old_state);
> > @@ -141,8 +127,6 @@ rockchip_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
> >  	drm_atomic_helper_commit_planes(dev, old_state,
> >  					DRM_PLANE_COMMIT_ACTIVE_ONLY);
> >  
> > -	rockchip_drm_psr_inhibit_put_state(old_state);
> > -
> >  	drm_atomic_helper_commit_hw_done(old_state);
> >  
> >  	drm_atomic_helper_wait_for_vblanks(dev, old_state);
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> > deleted file mode 100644
> > index a0c8bd235b67..000000000000
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> > +++ /dev/null
> > @@ -1,290 +0,0 @@
> > -/*
> > - * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
> > - * Author: Yakir Yang <ykk@rock-chips.com>
> > - *
> > - * This software is licensed under the terms of the GNU General Public
> > - * License version 2, as published by the Free Software Foundation, and
> > - * may be copied, distributed, and modified under those terms.
> > - *
> > - * This program is distributed in the hope that it will be useful,
> > - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > - * GNU General Public License for more details.
> > - */
> > -
> > -#include <drm/drmP.h>
> > -#include <drm/drm_atomic.h>
> > -#include <drm/drm_probe_helper.h>
> > -
> > -#include "rockchip_drm_drv.h"
> > -#include "rockchip_drm_psr.h"
> > -
> > -#define PSR_FLUSH_TIMEOUT_MS	100
> > -
> > -struct psr_drv {
> > -	struct list_head	list;
> > -	struct drm_encoder	*encoder;
> > -
> > -	struct mutex		lock;
> > -	int			inhibit_count;
> > -	bool			enabled;
> > -
> > -	struct delayed_work	flush_work;
> > -
> > -	int (*set)(struct drm_encoder *encoder, bool enable);
> > -};
> > -
> > -static struct psr_drv *find_psr_by_encoder(struct drm_encoder *encoder)
> > -{
> > -	struct rockchip_drm_private *drm_drv = encoder->dev->dev_private;
> > -	struct psr_drv *psr;
> > -
> > -	mutex_lock(&drm_drv->psr_list_lock);
> > -	list_for_each_entry(psr, &drm_drv->psr_list, list) {
> > -		if (psr->encoder == encoder)
> > -			goto out;
> > -	}
> > -	psr = ERR_PTR(-ENODEV);
> > -
> > -out:
> > -	mutex_unlock(&drm_drv->psr_list_lock);
> > -	return psr;
> > -}
> > -
> > -static int psr_set_state_locked(struct psr_drv *psr, bool enable)
> > -{
> > -	int ret;
> > -
> > -	if (psr->inhibit_count > 0)
> > -		return -EINVAL;
> > -
> > -	if (enable == psr->enabled)
> > -		return 0;
> > -
> > -	ret = psr->set(psr->encoder, enable);
> > -	if (ret)
> > -		return ret;
> > -
> > -	psr->enabled = enable;
> > -	return 0;
> > -}
> > -
> > -static void psr_flush_handler(struct work_struct *work)
> > -{
> > -	struct psr_drv *psr = container_of(to_delayed_work(work),
> > -					   struct psr_drv, flush_work);
> > -
> > -	mutex_lock(&psr->lock);
> > -	psr_set_state_locked(psr, true);
> > -	mutex_unlock(&psr->lock);
> > -}
> > -
> > -/**
> > - * rockchip_drm_psr_inhibit_put - release PSR inhibit on given encoder
> > - * @encoder: encoder to obtain the PSR encoder
> > - *
> > - * Decrements PSR inhibit count on given encoder. Should be called only
> > - * for a PSR inhibit count increment done before. If PSR inhibit counter
> > - * reaches zero, PSR flush work is scheduled to make the hardware enter
> > - * PSR mode in PSR_FLUSH_TIMEOUT_MS.
> > - *
> > - * Returns:
> > - * Zero on success, negative errno on failure.
> > - */
> > -int rockchip_drm_psr_inhibit_put(struct drm_encoder *encoder)
> > -{
> > -	struct psr_drv *psr = find_psr_by_encoder(encoder);
> > -
> > -	if (IS_ERR(psr))
> > -		return PTR_ERR(psr);
> > -
> > -	mutex_lock(&psr->lock);
> > -	--psr->inhibit_count;
> > -	WARN_ON(psr->inhibit_count < 0);
> > -	if (!psr->inhibit_count)
> > -		mod_delayed_work(system_wq, &psr->flush_work,
> > -				 PSR_FLUSH_TIMEOUT_MS);
> > -	mutex_unlock(&psr->lock);
> > -
> > -	return 0;
> > -}
> > -EXPORT_SYMBOL(rockchip_drm_psr_inhibit_put);
> > -
> > -void rockchip_drm_psr_inhibit_get_state(struct drm_atomic_state *state)
> > -{
> > -	struct drm_crtc *crtc;
> > -	struct drm_crtc_state *crtc_state;
> > -	struct drm_encoder *encoder;
> > -	u32 encoder_mask = 0;
> > -	int i;
> > -
> > -	for_each_old_crtc_in_state(state, crtc, crtc_state, i) {
> > -		encoder_mask |= crtc_state->encoder_mask;
> > -		encoder_mask |= crtc->state->encoder_mask;
> > -	}
> > -
> > -	drm_for_each_encoder_mask(encoder, state->dev, encoder_mask)
> > -		rockchip_drm_psr_inhibit_get(encoder);
> > -}
> > -EXPORT_SYMBOL(rockchip_drm_psr_inhibit_get_state);
> > -
> > -void rockchip_drm_psr_inhibit_put_state(struct drm_atomic_state *state)
> > -{
> > -	struct drm_crtc *crtc;
> > -	struct drm_crtc_state *crtc_state;
> > -	struct drm_encoder *encoder;
> > -	u32 encoder_mask = 0;
> > -	int i;
> > -
> > -	for_each_old_crtc_in_state(state, crtc, crtc_state, i) {
> > -		encoder_mask |= crtc_state->encoder_mask;
> > -		encoder_mask |= crtc->state->encoder_mask;
> > -	}
> > -
> > -	drm_for_each_encoder_mask(encoder, state->dev, encoder_mask)
> > -		rockchip_drm_psr_inhibit_put(encoder);
> > -}
> > -EXPORT_SYMBOL(rockchip_drm_psr_inhibit_put_state);
> > -
> > -/**
> > - * rockchip_drm_psr_inhibit_get - acquire PSR inhibit on given encoder
> > - * @encoder: encoder to obtain the PSR encoder
> > - *
> > - * Increments PSR inhibit count on given encoder. This function guarantees
> > - * that after it returns PSR is turned off on given encoder and no PSR-related
> > - * hardware state change occurs at least until a matching call to
> > - * rockchip_drm_psr_inhibit_put() is done.
> > - *
> > - * Returns:
> > - * Zero on success, negative errno on failure.
> > - */
> > -int rockchip_drm_psr_inhibit_get(struct drm_encoder *encoder)
> > -{
> > -	struct psr_drv *psr = find_psr_by_encoder(encoder);
> > -
> > -	if (IS_ERR(psr))
> > -		return PTR_ERR(psr);
> > -
> > -	mutex_lock(&psr->lock);
> > -	psr_set_state_locked(psr, false);
> > -	++psr->inhibit_count;
> > -	mutex_unlock(&psr->lock);
> > -	cancel_delayed_work_sync(&psr->flush_work);
> > -
> > -	return 0;
> > -}
> > -EXPORT_SYMBOL(rockchip_drm_psr_inhibit_get);
> > -
> > -static void rockchip_drm_do_flush(struct psr_drv *psr)
> > -{
> > -	cancel_delayed_work_sync(&psr->flush_work);
> > -
> > -	mutex_lock(&psr->lock);
> > -	if (!psr_set_state_locked(psr, false))
> > -		mod_delayed_work(system_wq, &psr->flush_work,
> > -				 PSR_FLUSH_TIMEOUT_MS);
> > -	mutex_unlock(&psr->lock);
> > -}
> > -
> > -/**
> > - * rockchip_drm_psr_flush_all - force to flush all registered PSR encoders
> > - * @dev: drm device
> > - *
> > - * Disable the PSR function for all registered encoders, and then enable the
> > - * PSR function back after PSR_FLUSH_TIMEOUT. If encoder PSR state have been
> > - * changed during flush time, then keep the state no change after flush
> > - * timeout.
> > - *
> > - * Returns:
> > - * Zero on success, negative errno on failure.
> > - */
> > -void rockchip_drm_psr_flush_all(struct drm_device *dev)
> > -{
> > -	struct rockchip_drm_private *drm_drv = dev->dev_private;
> > -	struct psr_drv *psr;
> > -
> > -	mutex_lock(&drm_drv->psr_list_lock);
> > -	list_for_each_entry(psr, &drm_drv->psr_list, list)
> > -		rockchip_drm_do_flush(psr);
> > -	mutex_unlock(&drm_drv->psr_list_lock);
> > -}
> > -EXPORT_SYMBOL(rockchip_drm_psr_flush_all);
> > -
> > -/**
> > - * rockchip_drm_psr_register - register encoder to psr driver
> > - * @encoder: encoder that obtain the PSR function
> > - * @psr_set: call back to set PSR state
> > - *
> > - * The function returns with PSR inhibit counter initialized with one
> > - * and the caller (typically encoder driver) needs to call
> > - * rockchip_drm_psr_inhibit_put() when it becomes ready to accept PSR
> > - * enable request.
> > - *
> > - * Returns:
> > - * Zero on success, negative errno on failure.
> > - */
> > -int rockchip_drm_psr_register(struct drm_encoder *encoder,
> > -			int (*psr_set)(struct drm_encoder *, bool enable))
> > -{
> > -	struct rockchip_drm_private *drm_drv;
> > -	struct psr_drv *psr;
> > -
> > -	if (!encoder || !psr_set)
> > -		return -EINVAL;
> > -
> > -	drm_drv = encoder->dev->dev_private;
> > -
> > -	psr = kzalloc(sizeof(struct psr_drv), GFP_KERNEL);
> > -	if (!psr)
> > -		return -ENOMEM;
> > -
> > -	INIT_DELAYED_WORK(&psr->flush_work, psr_flush_handler);
> > -	mutex_init(&psr->lock);
> > -
> > -	psr->inhibit_count = 1;
> > -	psr->enabled = false;
> > -	psr->encoder = encoder;
> > -	psr->set = psr_set;
> > -
> > -	mutex_lock(&drm_drv->psr_list_lock);
> > -	list_add_tail(&psr->list, &drm_drv->psr_list);
> > -	mutex_unlock(&drm_drv->psr_list_lock);
> > -
> > -	return 0;
> > -}
> > -EXPORT_SYMBOL(rockchip_drm_psr_register);
> > -
> > -/**
> > - * rockchip_drm_psr_unregister - unregister encoder to psr driver
> > - * @encoder: encoder that obtain the PSR function
> > - * @psr_set: call back to set PSR state
> > - *
> > - * It is expected that the PSR inhibit counter is 1 when this function is
> > - * called, which corresponds to a state when related encoder has been
> > - * disconnected from any CRTCs and its driver called
> > - * rockchip_drm_psr_inhibit_get() to stop the PSR logic.
> > - *
> > - * Returns:
> > - * Zero on success, negative errno on failure.
> > - */
> > -void rockchip_drm_psr_unregister(struct drm_encoder *encoder)
> > -{
> > -	struct rockchip_drm_private *drm_drv = encoder->dev->dev_private;
> > -	struct psr_drv *psr, *n;
> > -
> > -	mutex_lock(&drm_drv->psr_list_lock);
> > -	list_for_each_entry_safe(psr, n, &drm_drv->psr_list, list) {
> > -		if (psr->encoder == encoder) {
> > -			/*
> > -			 * Any other value would mean that the encoder
> > -			 * is still in use.
> > -			 */
> > -			WARN_ON(psr->inhibit_count != 1);
> > -
> > -			list_del(&psr->list);
> > -			kfree(psr);
> > -		}
> > -	}
> > -	mutex_unlock(&drm_drv->psr_list_lock);
> > -}
> > -EXPORT_SYMBOL(rockchip_drm_psr_unregister);
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.h b/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
> > deleted file mode 100644
> > index 25350ba3237b..000000000000
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
> > +++ /dev/null
> > @@ -1,30 +0,0 @@
> > -/*
> > - * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
> > - * Author: Yakir Yang <ykk@rock-chips.com>
> > - *
> > - * This software is licensed under the terms of the GNU General Public
> > - * License version 2, as published by the Free Software Foundation, and
> > - * may be copied, distributed, and modified under those terms.
> > - *
> > - * This program is distributed in the hope that it will be useful,
> > - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > - * GNU General Public License for more details.
> > - */
> > -
> > -#ifndef __ROCKCHIP_DRM_PSR___
> > -#define __ROCKCHIP_DRM_PSR___
> > -
> > -void rockchip_drm_psr_flush_all(struct drm_device *dev);
> > -
> > -int rockchip_drm_psr_inhibit_put(struct drm_encoder *encoder);
> > -int rockchip_drm_psr_inhibit_get(struct drm_encoder *encoder);
> > -
> > -void rockchip_drm_psr_inhibit_get_state(struct drm_atomic_state *state);
> > -void rockchip_drm_psr_inhibit_put_state(struct drm_atomic_state *state);
> > -
> > -int rockchip_drm_psr_register(struct drm_encoder *encoder,
> > -			int (*psr_set)(struct drm_encoder *, bool enable));
> > -void rockchip_drm_psr_unregister(struct drm_encoder *encoder);
> > -
> > -#endif /* __ROCKCHIP_DRM_PSR__ */
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > index c7d4c6073ea5..21fccda70356 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > @@ -21,6 +21,7 @@
> >  #include <drm/drm_gem_framebuffer_helper.h>
> >  #include <drm/drm_plane_helper.h>
> >  #include <drm/drm_probe_helper.h>
> > +#include <drm/drm_self_refresh_helper.h>
> >  #ifdef CONFIG_DRM_ANALOGIX_DP
> >  #include <drm/bridge/analogix_dp.h>
> >  #endif
> > @@ -42,10 +43,11 @@
> >  #include "rockchip_drm_drv.h"
> >  #include "rockchip_drm_gem.h"
> >  #include "rockchip_drm_fb.h"
> > -#include "rockchip_drm_psr.h"
> >  #include "rockchip_drm_vop.h"
> >  #include "rockchip_rgb.h"
> >  
> > +#define VOP_SELF_REFRESH_ENTRY_DELAY_MS 100
> > +
> >  #define VOP_WIN_SET(vop, win, name, v) \
> >  		vop_reg_set(vop, &win->phy->name, win->base, ~0, v, #name)
> >  #define VOP_SCL_SET(vop, win, name, v) \
> > @@ -541,7 +543,7 @@ static void vop_core_clks_disable(struct vop *vop)
> >  	clk_disable(vop->hclk);
> >  }
> >  
> > -static int vop_enable(struct drm_crtc *crtc)
> > +static int vop_enable(struct drm_crtc *crtc, struct drm_crtc_state *old_state)
> >  {
> >  	struct vop *vop = to_vop(crtc);
> >  	int ret, i;
> > @@ -581,12 +583,18 @@ static int vop_enable(struct drm_crtc *crtc)
> >  	 * We need to make sure that all windows are disabled before we
> >  	 * enable the crtc. Otherwise we might try to scan from a destroyed
> >  	 * buffer later.
> > +	 *
> > +	 * In the case of enable-after-PSR, we don't need to worry about this
> > +	 * case since the buffer is guaranteed to be valid and disabling the
> > +	 * window will result in screen glitches on PSR exit.
> >  	 */
> > -	for (i = 0; i < vop->data->win_size; i++) {
> > -		struct vop_win *vop_win = &vop->win[i];
> > -		const struct vop_win_data *win = vop_win->data;
> > +	if (!old_state || !old_state->self_refresh_active) {
> > +		for (i = 0; i < vop->data->win_size; i++) {
> > +			struct vop_win *vop_win = &vop->win[i];
> > +			const struct vop_win_data *win = vop_win->data;
> >  
> > -		VOP_WIN_SET(vop, win, enable, 0);
> > +			VOP_WIN_SET(vop, win, enable, 0);
> > +		}
> >  	}
> >  	spin_unlock(&vop->reg_lock);
> >  
> > @@ -937,12 +945,10 @@ static void vop_plane_atomic_async_update(struct drm_plane *plane,
> >  	}
> >  
> >  	if (vop->is_enabled) {
> > -		rockchip_drm_psr_inhibit_get_state(new_state->state);
> >  		vop_plane_atomic_update(plane, plane->state);
> >  		spin_lock(&vop->reg_lock);
> >  		vop_cfg_done(vop);
> >  		spin_unlock(&vop->reg_lock);
> > -		rockchip_drm_psr_inhibit_put_state(new_state->state);
> >  	}
> >  
> >  	plane->funcs->atomic_destroy_state(plane, plane_state);
> > @@ -1035,7 +1041,7 @@ static void vop_crtc_atomic_enable(struct drm_crtc *crtc,
> >  
> >  	WARN_ON(vop->event);
> >  
> > -	ret = vop_enable(crtc);
> > +	ret = vop_enable(crtc, old_state);
> >  	if (ret) {
> >  		mutex_unlock(&vop->vop_lock);
> >  		DRM_DEV_ERROR(vop->dev, "Failed to enable vop (%d)\n", ret);
> > @@ -1509,6 +1515,13 @@ static int vop_create_crtc(struct vop *vop)
> >  	init_completion(&vop->line_flag_completion);
> >  	crtc->port = port;
> >  
> > +	ret = drm_self_refresh_helper_register(crtc,
> > +					       VOP_SELF_REFRESH_ENTRY_DELAY_MS);
> > +	if (ret)
> > +		DRM_DEV_DEBUG_KMS(vop->dev,
> > +			"Failed to register %s with SR helpers %d, ignoring\n",
> > +			crtc->name, ret);
> > +
> >  	return 0;
> >  
> >  err_cleanup_crtc:
> > @@ -1526,6 +1539,8 @@ static void vop_destroy_crtc(struct vop *vop)
> >  	struct drm_device *drm_dev = vop->drm_dev;
> >  	struct drm_plane *plane, *tmp;
> >  
> > +	drm_self_refresh_helper_unregister(crtc);
> > +
> >  	of_node_put(crtc->port);
> >  
> >  	/*
> > diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
> > index 475b706b49de..b6cc654143d7 100644
> > --- a/include/drm/bridge/analogix_dp.h
> > +++ b/include/drm/bridge/analogix_dp.h
> > @@ -42,9 +42,7 @@ struct analogix_dp_plat_data {
> >  			 struct drm_connector *);
> >  };
> >  
> > -int analogix_dp_psr_enabled(struct analogix_dp_device *dp);
> > -int analogix_dp_enable_psr(struct analogix_dp_device *dp);
> > -int analogix_dp_disable_psr(struct analogix_dp_device *dp);
> > +bool analogix_dp_self_refresh_changed(struct analogix_dp_device *dp);
> >  
> >  int analogix_dp_resume(struct analogix_dp_device *dp);
> >  int analogix_dp_suspend(struct analogix_dp_device *dp);
> > 
> 
> 
> 
>
Heiko Stübner March 29, 2019, 7:02 p.m. UTC | #3
Am Freitag, 29. März 2019, 20:00:10 CET schrieb Sean Paul:
> On Fri, Mar 29, 2019 at 07:51:51PM +0100, Heiko Stübner wrote:
> > Hi,
> > 
> > Am Dienstag, 26. März 2019, 21:44:56 CET schrieb Sean Paul:
> > > From: Sean Paul <seanpaul@chromium.org>
> > > 
> > > Instead of rolling our own implementation for tracking when PSR should
> > > be [in]active, use the new self refresh helpers to do the heavy lifting.
> > 
> > I only got patches 3-5 and had to pull 1+2 from patchwork, the following
> > applies to the whole series though.
> > 
> > While my Kevin display does still generally work with these 5 patches
> > applied functionality has regressed somehow.
> > 
> > Environment is a standard Debian with framebuffer console and sddm
> > login manager.
> 
> Ahh, this is probably b/c fb_dirty is not triggering PSR exit. I had it
> working in v1, but removed it in v2 since Daniel suggested we require the
> fb_dirty helpers. I neglected to add support for the fb_dirty helpers, so fbcon
> updates won't cause the screen to refresh. I'll add the helpers in v3 and make
> sure I test it out with fbcon.
> 
> Thanks for reporting this!

The cursor-not-blinking case above was X11+modesetting+sddm though, so
not directly fbcon-related probably ... 2nd issue?

Heiko
Sean Paul March 29, 2019, 7:12 p.m. UTC | #4
On Fri, Mar 29, 2019 at 08:02:17PM +0100, Heiko Stübner wrote:
> Am Freitag, 29. März 2019, 20:00:10 CET schrieb Sean Paul:
> > On Fri, Mar 29, 2019 at 07:51:51PM +0100, Heiko Stübner wrote:
> > > Hi,
> > > 
> > > Am Dienstag, 26. März 2019, 21:44:56 CET schrieb Sean Paul:
> > > > From: Sean Paul <seanpaul@chromium.org>
> > > > 
> > > > Instead of rolling our own implementation for tracking when PSR should
> > > > be [in]active, use the new self refresh helpers to do the heavy lifting.
> > > 
> > > I only got patches 3-5 and had to pull 1+2 from patchwork, the following
> > > applies to the whole series though.
> > > 
> > > While my Kevin display does still generally work with these 5 patches
> > > applied functionality has regressed somehow.
> > > 
> > > Environment is a standard Debian with framebuffer console and sddm
> > > login manager.
> > 
> > Ahh, this is probably b/c fb_dirty is not triggering PSR exit. I had it
> > working in v1, but removed it in v2 since Daniel suggested we require the
> > fb_dirty helpers. I neglected to add support for the fb_dirty helpers, so fbcon
> > updates won't cause the screen to refresh. I'll add the helpers in v3 and make
> > sure I test it out with fbcon.
> > 
> > Thanks for reporting this!
> 
> The cursor-not-blinking case above was X11+modesetting+sddm though, so
> not directly fbcon-related probably ... 2nd issue?

Yep, definitely could be. If you have the chance, could you collect some
DRM_DEBUG_KMS logs for the second issue to see what's going on? My guess is
there's a legacy path (update_plane?) that's somehow not triggering psr exit.

Sean

> 
> Heiko
> 
>
Daniel Vetter March 29, 2019, 7:24 p.m. UTC | #5
On Fri, Mar 29, 2019 at 8:12 PM Sean Paul <sean@poorly.run> wrote:
>
> On Fri, Mar 29, 2019 at 08:02:17PM +0100, Heiko Stübner wrote:
> > Am Freitag, 29. März 2019, 20:00:10 CET schrieb Sean Paul:
> > > On Fri, Mar 29, 2019 at 07:51:51PM +0100, Heiko Stübner wrote:
> > > > Hi,
> > > >
> > > > Am Dienstag, 26. März 2019, 21:44:56 CET schrieb Sean Paul:
> > > > > From: Sean Paul <seanpaul@chromium.org>
> > > > >
> > > > > Instead of rolling our own implementation for tracking when PSR should
> > > > > be [in]active, use the new self refresh helpers to do the heavy lifting.
> > > >
> > > > I only got patches 3-5 and had to pull 1+2 from patchwork, the following
> > > > applies to the whole series though.
> > > >
> > > > While my Kevin display does still generally work with these 5 patches
> > > > applied functionality has regressed somehow.
> > > >
> > > > Environment is a standard Debian with framebuffer console and sddm
> > > > login manager.
> > >
> > > Ahh, this is probably b/c fb_dirty is not triggering PSR exit. I had it
> > > working in v1, but removed it in v2 since Daniel suggested we require the
> > > fb_dirty helpers. I neglected to add support for the fb_dirty helpers, so fbcon
> > > updates won't cause the screen to refresh. I'll add the helpers in v3 and make
> > > sure I test it out with fbcon.
> > >
> > > Thanks for reporting this!
> >
> > The cursor-not-blinking case above was X11+modesetting+sddm though, so
> > not directly fbcon-related probably ... 2nd issue?
>
> Yep, definitely could be. If you have the chance, could you collect some
> DRM_DEBUG_KMS logs for the second issue to see what's going on? My guess is
> there's a legacy path (update_plane?) that's somehow not triggering psr exit.

fb_dirty helper is also for userspace kms. It's just that you get a
nice combo with the helpers that also takes care of fbcon (which is a
separate path if you don't bother with the dirty helpers but roll your
own for everything). And yeah the v2 design is a package, if you
handroll your own fb_dirty the magic won't work :-)
-Daniel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index af34554a5a02..c503fe3bc3b3 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -106,63 +106,13 @@  static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
 	return 0;
 }
 
-int analogix_dp_psr_enabled(struct analogix_dp_device *dp)
+bool analogix_dp_self_refresh_changed(struct analogix_dp_device *dp)
 {
-
-	return dp->psr_enable;
+	return dp->connector.state->self_refresh_changed;
 }
-EXPORT_SYMBOL_GPL(analogix_dp_psr_enabled);
+EXPORT_SYMBOL_GPL(analogix_dp_self_refresh_changed);
 
-int analogix_dp_enable_psr(struct analogix_dp_device *dp)
-{
-	struct edp_vsc_psr psr_vsc;
-
-	if (!dp->psr_enable)
-		return 0;
-
-	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
-	memset(&psr_vsc, 0, sizeof(psr_vsc));
-	psr_vsc.sdp_header.HB0 = 0;
-	psr_vsc.sdp_header.HB1 = 0x7;
-	psr_vsc.sdp_header.HB2 = 0x2;
-	psr_vsc.sdp_header.HB3 = 0x8;
-
-	psr_vsc.DB0 = 0;
-	psr_vsc.DB1 = EDP_VSC_PSR_STATE_ACTIVE | EDP_VSC_PSR_CRC_VALUES_VALID;
-
-	return analogix_dp_send_psr_spd(dp, &psr_vsc, true);
-}
-EXPORT_SYMBOL_GPL(analogix_dp_enable_psr);
-
-int analogix_dp_disable_psr(struct analogix_dp_device *dp)
-{
-	struct edp_vsc_psr psr_vsc;
-	int ret;
-
-	if (!dp->psr_enable)
-		return 0;
-
-	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
-	memset(&psr_vsc, 0, sizeof(psr_vsc));
-	psr_vsc.sdp_header.HB0 = 0;
-	psr_vsc.sdp_header.HB1 = 0x7;
-	psr_vsc.sdp_header.HB2 = 0x2;
-	psr_vsc.sdp_header.HB3 = 0x8;
-
-	psr_vsc.DB0 = 0;
-	psr_vsc.DB1 = 0;
-
-	ret = drm_dp_dpcd_writeb(&dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
-	if (ret != 1) {
-		dev_err(dp->dev, "Failed to set DP Power0 %d\n", ret);
-		return ret;
-	}
-
-	return analogix_dp_send_psr_spd(dp, &psr_vsc, false);
-}
-EXPORT_SYMBOL_GPL(analogix_dp_disable_psr);
-
-static int analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
+static bool analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
 {
 	unsigned char psr_version;
 	int ret;
@@ -170,14 +120,11 @@  static int analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
 	ret = drm_dp_dpcd_readb(&dp->aux, DP_PSR_SUPPORT, &psr_version);
 	if (ret != 1) {
 		dev_err(dp->dev, "failed to get PSR version, disable it\n");
-		return ret;
+		return false;
 	}
 
 	dev_dbg(dp->dev, "Panel PSR version : %x\n", psr_version);
-
-	dp->psr_enable = (psr_version & DP_PSR_IS_SUPPORTED) ? true : false;
-
-	return 0;
+	return psr_version & DP_PSR_IS_SUPPORTED;
 }
 
 static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)
@@ -200,7 +147,7 @@  static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)
 	}
 
 	/* Main-Link transmitter remains active during PSR active states */
-	psr_en = DP_PSR_MAIN_LINK_ACTIVE | DP_PSR_CRC_VERIFICATION;
+	psr_en = DP_PSR_CRC_VERIFICATION;
 	ret = drm_dp_dpcd_writeb(&dp->aux, DP_PSR_EN_CFG, psr_en);
 	if (ret != 1) {
 		dev_err(dp->dev, "failed to set panel psr\n");
@@ -208,8 +155,7 @@  static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)
 	}
 
 	/* Enable psr function */
-	psr_en = DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE |
-		 DP_PSR_CRC_VERIFICATION;
+	psr_en = DP_PSR_ENABLE | DP_PSR_CRC_VERIFICATION;
 	ret = drm_dp_dpcd_writeb(&dp->aux, DP_PSR_EN_CFG, psr_en);
 	if (ret != 1) {
 		dev_err(dp->dev, "failed to set panel psr\n");
@@ -218,10 +164,11 @@  static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)
 
 	analogix_dp_enable_psr_crc(dp);
 
+	dp->psr_supported = true;
+
 	return 0;
 end:
 	dev_err(dp->dev, "enable psr fail, force to disable psr\n");
-	dp->psr_enable = false;
 
 	return ret;
 }
@@ -1036,25 +983,90 @@  static int analogix_dp_commit(struct analogix_dp_device *dp)
 		}
 	}
 
-	ret = analogix_dp_detect_sink_psr(dp);
-	if (ret)
-		return ret;
-
 	/* Check whether panel supports fast training */
 	ret = analogix_dp_fast_link_train_detection(dp);
 	if (ret)
-		dp->psr_enable = false;
+		return ret;
 
-	if (dp->psr_enable) {
+	if (analogix_dp_detect_sink_psr(dp)) {
 		ret = analogix_dp_enable_sink_psr(dp);
 		if (ret)
 			return ret;
 	}
 
+	return ret;
+}
+
+static int analogix_dp_enable_psr(struct analogix_dp_device *dp)
+{
+	struct edp_vsc_psr psr_vsc;
+	int ret;
+	u8 sink;
+
+	ret = drm_dp_dpcd_readb(&dp->aux, DP_PSR_STATUS, &sink);
+	if (ret != 1)
+		DRM_DEV_ERROR(dp->dev, "Failed to read psr status %d\n", ret);
+	else if (sink == DP_PSR_SINK_ACTIVE_RFB)
+		return 0;
+
+	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
+	memset(&psr_vsc, 0, sizeof(psr_vsc));
+	psr_vsc.sdp_header.HB0 = 0;
+	psr_vsc.sdp_header.HB1 = 0x7;
+	psr_vsc.sdp_header.HB2 = 0x2;
+	psr_vsc.sdp_header.HB3 = 0x8;
+	psr_vsc.DB0 = 0;
+	psr_vsc.DB1 = EDP_VSC_PSR_STATE_ACTIVE | EDP_VSC_PSR_CRC_VALUES_VALID;
+
+	ret = analogix_dp_send_psr_spd(dp, &psr_vsc, true);
+	if (!ret)
+		analogix_dp_set_analog_power_down(dp, POWER_ALL, true);
 
 	return ret;
 }
 
+static int analogix_dp_disable_psr(struct analogix_dp_device *dp)
+{
+	struct edp_vsc_psr psr_vsc;
+	int ret;
+	u8 sink;
+
+	analogix_dp_set_analog_power_down(dp, POWER_ALL, false);
+
+	ret = drm_dp_dpcd_writeb(&dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
+	if (ret != 1) {
+		DRM_DEV_ERROR(dp->dev, "Failed to set DP Power0 %d\n", ret);
+		return ret;
+	}
+
+	ret = drm_dp_dpcd_readb(&dp->aux, DP_PSR_STATUS, &sink);
+	if (ret != 1) {
+		DRM_DEV_ERROR(dp->dev, "Failed to read psr status %d\n", ret);
+		return ret;
+	} else if (sink == DP_PSR_SINK_INACTIVE) {
+		DRM_DEV_ERROR(dp->dev, "sink inactive, skip disable psr");
+		return 0;
+	}
+
+	ret = analogix_dp_train_link(dp);
+	if (ret) {
+		DRM_DEV_ERROR(dp->dev, "Failed to train the link %d\n", ret);
+		return ret;
+	}
+
+	/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
+	memset(&psr_vsc, 0, sizeof(psr_vsc));
+	psr_vsc.sdp_header.HB0 = 0;
+	psr_vsc.sdp_header.HB1 = 0x7;
+	psr_vsc.sdp_header.HB2 = 0x2;
+	psr_vsc.sdp_header.HB3 = 0x8;
+
+	psr_vsc.DB0 = 0;
+	psr_vsc.DB1 = 0;
+
+	return analogix_dp_send_psr_spd(dp, &psr_vsc, true);
+}
+
 /*
  * This function is a bit of a catch-all for panel preparation, hopefully
  * simplifying the logic of functions that need to prepare/unprepare the panel
@@ -1145,9 +1157,23 @@  analogix_dp_best_encoder(struct drm_connector *connector)
 	return dp->encoder;
 }
 
+
+int analogix_dp_atomic_check(struct drm_connector *connector,
+			     struct drm_connector_state *state)
+{
+	struct analogix_dp_device *dp = to_dp(connector);
+
+	state->self_refresh_aware = true;
+	if (state->self_refresh_active && !dp->psr_supported)
+		return -EINVAL;
+
+	return 0;
+}
+
 static const struct drm_connector_helper_funcs analogix_dp_connector_helper_funcs = {
 	.get_modes = analogix_dp_get_modes,
 	.best_encoder = analogix_dp_best_encoder,
+	.atomic_check = analogix_dp_atomic_check,
 };
 
 static enum drm_connector_status
@@ -1244,6 +1270,10 @@  static void analogix_dp_bridge_pre_enable(struct drm_bridge *bridge)
 	struct analogix_dp_device *dp = bridge->driver_private;
 	int ret;
 
+	/* Don't touch the panel if we're coming back from PSR */
+	if (dp->connector.state->self_refresh_changed)
+		return;
+
 	ret = analogix_dp_prepare_panel(dp, true, true);
 	if (ret)
 		DRM_ERROR("failed to setup the panel ret = %d\n", ret);
@@ -1309,6 +1339,14 @@  static void analogix_dp_bridge_enable(struct drm_bridge *bridge)
 	struct analogix_dp_device *dp = bridge->driver_private;
 	int timeout_loop = 0;
 
+	/* Not a full enable, just disable PSR and continue */
+	if (dp->connector.state->self_refresh_changed) {
+		int ret = analogix_dp_disable_psr(dp);
+		if (ret)
+			DRM_ERROR("Failed to disable psr %d\n", ret);
+		return;
+	}
+
 	if (dp->dpms_mode == DRM_MODE_DPMS_ON)
 		return;
 
@@ -1356,11 +1394,37 @@  static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
 	if (ret)
 		DRM_ERROR("failed to setup the panel ret = %d\n", ret);
 
-	dp->psr_enable = false;
 	dp->fast_train_enable = false;
+	dp->psr_supported = false;
 	dp->dpms_mode = DRM_MODE_DPMS_OFF;
 }
 
+static void analogix_dp_bridge_atomic_disable(struct drm_bridge *bridge)
+{
+	struct analogix_dp_device *dp = bridge->driver_private;
+	struct drm_connector_state *conn_state = dp->connector.state;
+
+	/* Don't do a full disable on PSR transitions */
+	if (conn_state->crtc && conn_state->self_refresh_changed)
+		return;
+
+	analogix_dp_bridge_disable(bridge);
+}
+
+static void analogix_dp_bridge_post_disable(struct drm_bridge *bridge)
+{
+	struct analogix_dp_device *dp = bridge->driver_private;
+	struct drm_connector_state *conn_state = dp->connector.state;
+	int ret;
+
+	if (conn_state->self_refresh_active) {
+		ret = analogix_dp_enable_psr(dp);
+		if (ret)
+			DRM_ERROR("Failed to enable psr (%d)\n", ret);
+		return;
+	}
+}
+
 static void analogix_dp_bridge_mode_set(struct drm_bridge *bridge,
 				const struct drm_display_mode *orig_mode,
 				const struct drm_display_mode *mode)
@@ -1440,16 +1504,11 @@  static void analogix_dp_bridge_mode_set(struct drm_bridge *bridge,
 		video->interlaced = true;
 }
 
-static void analogix_dp_bridge_nop(struct drm_bridge *bridge)
-{
-	/* do nothing */
-}
-
 static const struct drm_bridge_funcs analogix_dp_bridge_funcs = {
 	.pre_enable = analogix_dp_bridge_pre_enable,
 	.enable = analogix_dp_bridge_enable,
-	.disable = analogix_dp_bridge_disable,
-	.post_disable = analogix_dp_bridge_nop,
+	.disable = analogix_dp_bridge_atomic_disable,
+	.post_disable = analogix_dp_bridge_post_disable,
 	.mode_set = analogix_dp_bridge_mode_set,
 	.attach = analogix_dp_bridge_attach,
 };
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
index 769255dc6e99..d937fa3d9ee8 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
@@ -173,8 +173,8 @@  struct analogix_dp_device {
 	int			dpms_mode;
 	int			hpd_gpio;
 	bool                    force_hpd;
-	bool			psr_enable;
 	bool			fast_train_enable;
+	bool			psr_supported;
 
 	struct mutex		panel_lock;
 	bool			panel_is_modeset;
diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile
index f6fc9d5dd0ad..a777726c23cb 100644
--- a/drivers/gpu/drm/rockchip/Makefile
+++ b/drivers/gpu/drm/rockchip/Makefile
@@ -4,8 +4,7 @@ 
 # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
 
 rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
-		rockchip_drm_gem.o rockchip_drm_psr.o \
-		rockchip_drm_vop.o rockchip_vop_reg.o
+		rockchip_drm_gem.o rockchip_drm_vop.o rockchip_vop_reg.o
 rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o
 
 rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index bc4423624209..8f42c38848db 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -32,7 +32,6 @@ 
 #include <drm/bridge/analogix_dp.h>
 
 #include "rockchip_drm_drv.h"
-#include "rockchip_drm_psr.h"
 #include "rockchip_drm_vop.h"
 
 #define RK3288_GRF_SOC_CON6		0x25c
@@ -77,29 +76,6 @@  struct rockchip_dp_device {
 	struct analogix_dp_plat_data plat_data;
 };
 
-static int analogix_dp_psr_set(struct drm_encoder *encoder, bool enabled)
-{
-	struct rockchip_dp_device *dp = to_dp(encoder);
-	int ret;
-
-	if (!analogix_dp_psr_enabled(dp->adp))
-		return 0;
-
-	DRM_DEV_DEBUG(dp->dev, "%s PSR...\n", enabled ? "Entry" : "Exit");
-
-	ret = rockchip_drm_wait_vact_end(dp->encoder.crtc,
-					 PSR_WAIT_LINE_FLAG_TIMEOUT_MS);
-	if (ret) {
-		DRM_DEV_ERROR(dp->dev, "line flag interrupt did not arrive\n");
-		return -ETIMEDOUT;
-	}
-
-	if (enabled)
-		return analogix_dp_enable_psr(dp->adp);
-	else
-		return analogix_dp_disable_psr(dp->adp);
-}
-
 static int rockchip_dp_pre_init(struct rockchip_dp_device *dp)
 {
 	reset_control_assert(dp->rst);
@@ -130,21 +106,9 @@  static int rockchip_dp_poweron_start(struct analogix_dp_plat_data *plat_data)
 	return ret;
 }
 
-static int rockchip_dp_poweron_end(struct analogix_dp_plat_data *plat_data)
-{
-	struct rockchip_dp_device *dp = to_dp(plat_data);
-
-	return rockchip_drm_psr_inhibit_put(&dp->encoder);
-}
-
 static int rockchip_dp_powerdown(struct analogix_dp_plat_data *plat_data)
 {
 	struct rockchip_dp_device *dp = to_dp(plat_data);
-	int ret;
-
-	ret = rockchip_drm_psr_inhibit_get(&dp->encoder);
-	if (ret != 0)
-		return ret;
 
 	clk_disable_unprepare(dp->pclk);
 
@@ -190,6 +154,9 @@  static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder)
 	int ret;
 	u32 val;
 
+	if (analogix_dp_self_refresh_changed(dp->adp))
+		return;
+
 	ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
 	if (ret < 0)
 		return;
@@ -214,9 +181,24 @@  static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder)
 	clk_disable_unprepare(dp->grfclk);
 }
 
-static void rockchip_dp_drm_encoder_nop(struct drm_encoder *encoder)
+static void rockchip_dp_drm_encoder_disable(struct drm_encoder *encoder)
 {
-	/* do nothing */
+	struct rockchip_dp_device *dp = to_dp(encoder);
+	struct drm_crtc *crtc;
+	int ret;
+
+	drm_for_each_crtc(crtc, dp->drm_dev) {
+		if (!(crtc->state->encoder_mask & drm_encoder_mask(encoder)))
+			continue;
+
+		if (!crtc->state->self_refresh_active)
+			continue;
+
+		ret = rockchip_drm_wait_vact_end(crtc,
+						 PSR_WAIT_LINE_FLAG_TIMEOUT_MS);
+		if (ret)
+			DRM_DEV_ERROR(dp->dev, "line flag irq timed out\n");
+	}
 }
 
 static int
@@ -246,7 +228,7 @@  static struct drm_encoder_helper_funcs rockchip_dp_encoder_helper_funcs = {
 	.mode_fixup = rockchip_dp_drm_encoder_mode_fixup,
 	.mode_set = rockchip_dp_drm_encoder_mode_set,
 	.enable = rockchip_dp_drm_encoder_enable,
-	.disable = rockchip_dp_drm_encoder_nop,
+	.disable = rockchip_dp_drm_encoder_disable,
 	.atomic_check = rockchip_dp_drm_encoder_atomic_check,
 };
 
@@ -338,23 +320,16 @@  static int rockchip_dp_bind(struct device *dev, struct device *master,
 
 	dp->plat_data.dev_type = dp->data->chip_type;
 	dp->plat_data.power_on_start = rockchip_dp_poweron_start;
-	dp->plat_data.power_on_end = rockchip_dp_poweron_end;
 	dp->plat_data.power_off = rockchip_dp_powerdown;
 	dp->plat_data.get_modes = rockchip_dp_get_modes;
 
-	ret = rockchip_drm_psr_register(&dp->encoder, analogix_dp_psr_set);
-	if (ret < 0)
-		goto err_cleanup_encoder;
-
 	dp->adp = analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
 	if (IS_ERR(dp->adp)) {
 		ret = PTR_ERR(dp->adp);
-		goto err_unreg_psr;
+		goto err_cleanup_encoder;
 	}
 
 	return 0;
-err_unreg_psr:
-	rockchip_drm_psr_unregister(&dp->encoder);
 err_cleanup_encoder:
 	dp->encoder.funcs->destroy(&dp->encoder);
 	return ret;
@@ -366,7 +341,6 @@  static void rockchip_dp_unbind(struct device *dev, struct device *master,
 	struct rockchip_dp_device *dp = dev_get_drvdata(dev);
 
 	analogix_dp_unbind(dp->adp);
-	rockchip_drm_psr_unregister(&dp->encoder);
 	dp->encoder.funcs->destroy(&dp->encoder);
 
 	dp->adp = ERR_PTR(-ENODEV);
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index 97438bbbe389..7e121875e3c9 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -23,22 +23,10 @@ 
 #include "rockchip_drm_drv.h"
 #include "rockchip_drm_fb.h"
 #include "rockchip_drm_gem.h"
-#include "rockchip_drm_psr.h"
-
-static int rockchip_drm_fb_dirty(struct drm_framebuffer *fb,
-				 struct drm_file *file,
-				 unsigned int flags, unsigned int color,
-				 struct drm_clip_rect *clips,
-				 unsigned int num_clips)
-{
-	rockchip_drm_psr_flush_all(fb->dev);
-	return 0;
-}
 
 static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
 	.destroy       = drm_gem_fb_destroy,
 	.create_handle = drm_gem_fb_create_handle,
-	.dirty	       = rockchip_drm_fb_dirty,
 };
 
 static struct drm_framebuffer *
@@ -132,8 +120,6 @@  rockchip_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
 {
 	struct drm_device *dev = old_state->dev;
 
-	rockchip_drm_psr_inhibit_get_state(old_state);
-
 	drm_atomic_helper_commit_modeset_disables(dev, old_state);
 
 	drm_atomic_helper_commit_modeset_enables(dev, old_state);
@@ -141,8 +127,6 @@  rockchip_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
 	drm_atomic_helper_commit_planes(dev, old_state,
 					DRM_PLANE_COMMIT_ACTIVE_ONLY);
 
-	rockchip_drm_psr_inhibit_put_state(old_state);
-
 	drm_atomic_helper_commit_hw_done(old_state);
 
 	drm_atomic_helper_wait_for_vblanks(dev, old_state);
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
deleted file mode 100644
index a0c8bd235b67..000000000000
--- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
+++ /dev/null
@@ -1,290 +0,0 @@ 
-/*
- * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
- * Author: Yakir Yang <ykk@rock-chips.com>
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- */
-
-#include <drm/drmP.h>
-#include <drm/drm_atomic.h>
-#include <drm/drm_probe_helper.h>
-
-#include "rockchip_drm_drv.h"
-#include "rockchip_drm_psr.h"
-
-#define PSR_FLUSH_TIMEOUT_MS	100
-
-struct psr_drv {
-	struct list_head	list;
-	struct drm_encoder	*encoder;
-
-	struct mutex		lock;
-	int			inhibit_count;
-	bool			enabled;
-
-	struct delayed_work	flush_work;
-
-	int (*set)(struct drm_encoder *encoder, bool enable);
-};
-
-static struct psr_drv *find_psr_by_encoder(struct drm_encoder *encoder)
-{
-	struct rockchip_drm_private *drm_drv = encoder->dev->dev_private;
-	struct psr_drv *psr;
-
-	mutex_lock(&drm_drv->psr_list_lock);
-	list_for_each_entry(psr, &drm_drv->psr_list, list) {
-		if (psr->encoder == encoder)
-			goto out;
-	}
-	psr = ERR_PTR(-ENODEV);
-
-out:
-	mutex_unlock(&drm_drv->psr_list_lock);
-	return psr;
-}
-
-static int psr_set_state_locked(struct psr_drv *psr, bool enable)
-{
-	int ret;
-
-	if (psr->inhibit_count > 0)
-		return -EINVAL;
-
-	if (enable == psr->enabled)
-		return 0;
-
-	ret = psr->set(psr->encoder, enable);
-	if (ret)
-		return ret;
-
-	psr->enabled = enable;
-	return 0;
-}
-
-static void psr_flush_handler(struct work_struct *work)
-{
-	struct psr_drv *psr = container_of(to_delayed_work(work),
-					   struct psr_drv, flush_work);
-
-	mutex_lock(&psr->lock);
-	psr_set_state_locked(psr, true);
-	mutex_unlock(&psr->lock);
-}
-
-/**
- * rockchip_drm_psr_inhibit_put - release PSR inhibit on given encoder
- * @encoder: encoder to obtain the PSR encoder
- *
- * Decrements PSR inhibit count on given encoder. Should be called only
- * for a PSR inhibit count increment done before. If PSR inhibit counter
- * reaches zero, PSR flush work is scheduled to make the hardware enter
- * PSR mode in PSR_FLUSH_TIMEOUT_MS.
- *
- * Returns:
- * Zero on success, negative errno on failure.
- */
-int rockchip_drm_psr_inhibit_put(struct drm_encoder *encoder)
-{
-	struct psr_drv *psr = find_psr_by_encoder(encoder);
-
-	if (IS_ERR(psr))
-		return PTR_ERR(psr);
-
-	mutex_lock(&psr->lock);
-	--psr->inhibit_count;
-	WARN_ON(psr->inhibit_count < 0);
-	if (!psr->inhibit_count)
-		mod_delayed_work(system_wq, &psr->flush_work,
-				 PSR_FLUSH_TIMEOUT_MS);
-	mutex_unlock(&psr->lock);
-
-	return 0;
-}
-EXPORT_SYMBOL(rockchip_drm_psr_inhibit_put);
-
-void rockchip_drm_psr_inhibit_get_state(struct drm_atomic_state *state)
-{
-	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
-	struct drm_encoder *encoder;
-	u32 encoder_mask = 0;
-	int i;
-
-	for_each_old_crtc_in_state(state, crtc, crtc_state, i) {
-		encoder_mask |= crtc_state->encoder_mask;
-		encoder_mask |= crtc->state->encoder_mask;
-	}
-
-	drm_for_each_encoder_mask(encoder, state->dev, encoder_mask)
-		rockchip_drm_psr_inhibit_get(encoder);
-}
-EXPORT_SYMBOL(rockchip_drm_psr_inhibit_get_state);
-
-void rockchip_drm_psr_inhibit_put_state(struct drm_atomic_state *state)
-{
-	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
-	struct drm_encoder *encoder;
-	u32 encoder_mask = 0;
-	int i;
-
-	for_each_old_crtc_in_state(state, crtc, crtc_state, i) {
-		encoder_mask |= crtc_state->encoder_mask;
-		encoder_mask |= crtc->state->encoder_mask;
-	}
-
-	drm_for_each_encoder_mask(encoder, state->dev, encoder_mask)
-		rockchip_drm_psr_inhibit_put(encoder);
-}
-EXPORT_SYMBOL(rockchip_drm_psr_inhibit_put_state);
-
-/**
- * rockchip_drm_psr_inhibit_get - acquire PSR inhibit on given encoder
- * @encoder: encoder to obtain the PSR encoder
- *
- * Increments PSR inhibit count on given encoder. This function guarantees
- * that after it returns PSR is turned off on given encoder and no PSR-related
- * hardware state change occurs at least until a matching call to
- * rockchip_drm_psr_inhibit_put() is done.
- *
- * Returns:
- * Zero on success, negative errno on failure.
- */
-int rockchip_drm_psr_inhibit_get(struct drm_encoder *encoder)
-{
-	struct psr_drv *psr = find_psr_by_encoder(encoder);
-
-	if (IS_ERR(psr))
-		return PTR_ERR(psr);
-
-	mutex_lock(&psr->lock);
-	psr_set_state_locked(psr, false);
-	++psr->inhibit_count;
-	mutex_unlock(&psr->lock);
-	cancel_delayed_work_sync(&psr->flush_work);
-
-	return 0;
-}
-EXPORT_SYMBOL(rockchip_drm_psr_inhibit_get);
-
-static void rockchip_drm_do_flush(struct psr_drv *psr)
-{
-	cancel_delayed_work_sync(&psr->flush_work);
-
-	mutex_lock(&psr->lock);
-	if (!psr_set_state_locked(psr, false))
-		mod_delayed_work(system_wq, &psr->flush_work,
-				 PSR_FLUSH_TIMEOUT_MS);
-	mutex_unlock(&psr->lock);
-}
-
-/**
- * rockchip_drm_psr_flush_all - force to flush all registered PSR encoders
- * @dev: drm device
- *
- * Disable the PSR function for all registered encoders, and then enable the
- * PSR function back after PSR_FLUSH_TIMEOUT. If encoder PSR state have been
- * changed during flush time, then keep the state no change after flush
- * timeout.
- *
- * Returns:
- * Zero on success, negative errno on failure.
- */
-void rockchip_drm_psr_flush_all(struct drm_device *dev)
-{
-	struct rockchip_drm_private *drm_drv = dev->dev_private;
-	struct psr_drv *psr;
-
-	mutex_lock(&drm_drv->psr_list_lock);
-	list_for_each_entry(psr, &drm_drv->psr_list, list)
-		rockchip_drm_do_flush(psr);
-	mutex_unlock(&drm_drv->psr_list_lock);
-}
-EXPORT_SYMBOL(rockchip_drm_psr_flush_all);
-
-/**
- * rockchip_drm_psr_register - register encoder to psr driver
- * @encoder: encoder that obtain the PSR function
- * @psr_set: call back to set PSR state
- *
- * The function returns with PSR inhibit counter initialized with one
- * and the caller (typically encoder driver) needs to call
- * rockchip_drm_psr_inhibit_put() when it becomes ready to accept PSR
- * enable request.
- *
- * Returns:
- * Zero on success, negative errno on failure.
- */
-int rockchip_drm_psr_register(struct drm_encoder *encoder,
-			int (*psr_set)(struct drm_encoder *, bool enable))
-{
-	struct rockchip_drm_private *drm_drv;
-	struct psr_drv *psr;
-
-	if (!encoder || !psr_set)
-		return -EINVAL;
-
-	drm_drv = encoder->dev->dev_private;
-
-	psr = kzalloc(sizeof(struct psr_drv), GFP_KERNEL);
-	if (!psr)
-		return -ENOMEM;
-
-	INIT_DELAYED_WORK(&psr->flush_work, psr_flush_handler);
-	mutex_init(&psr->lock);
-
-	psr->inhibit_count = 1;
-	psr->enabled = false;
-	psr->encoder = encoder;
-	psr->set = psr_set;
-
-	mutex_lock(&drm_drv->psr_list_lock);
-	list_add_tail(&psr->list, &drm_drv->psr_list);
-	mutex_unlock(&drm_drv->psr_list_lock);
-
-	return 0;
-}
-EXPORT_SYMBOL(rockchip_drm_psr_register);
-
-/**
- * rockchip_drm_psr_unregister - unregister encoder to psr driver
- * @encoder: encoder that obtain the PSR function
- * @psr_set: call back to set PSR state
- *
- * It is expected that the PSR inhibit counter is 1 when this function is
- * called, which corresponds to a state when related encoder has been
- * disconnected from any CRTCs and its driver called
- * rockchip_drm_psr_inhibit_get() to stop the PSR logic.
- *
- * Returns:
- * Zero on success, negative errno on failure.
- */
-void rockchip_drm_psr_unregister(struct drm_encoder *encoder)
-{
-	struct rockchip_drm_private *drm_drv = encoder->dev->dev_private;
-	struct psr_drv *psr, *n;
-
-	mutex_lock(&drm_drv->psr_list_lock);
-	list_for_each_entry_safe(psr, n, &drm_drv->psr_list, list) {
-		if (psr->encoder == encoder) {
-			/*
-			 * Any other value would mean that the encoder
-			 * is still in use.
-			 */
-			WARN_ON(psr->inhibit_count != 1);
-
-			list_del(&psr->list);
-			kfree(psr);
-		}
-	}
-	mutex_unlock(&drm_drv->psr_list_lock);
-}
-EXPORT_SYMBOL(rockchip_drm_psr_unregister);
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.h b/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
deleted file mode 100644
index 25350ba3237b..000000000000
--- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
+++ /dev/null
@@ -1,30 +0,0 @@ 
-/*
- * Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
- * Author: Yakir Yang <ykk@rock-chips.com>
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- */
-
-#ifndef __ROCKCHIP_DRM_PSR___
-#define __ROCKCHIP_DRM_PSR___
-
-void rockchip_drm_psr_flush_all(struct drm_device *dev);
-
-int rockchip_drm_psr_inhibit_put(struct drm_encoder *encoder);
-int rockchip_drm_psr_inhibit_get(struct drm_encoder *encoder);
-
-void rockchip_drm_psr_inhibit_get_state(struct drm_atomic_state *state);
-void rockchip_drm_psr_inhibit_put_state(struct drm_atomic_state *state);
-
-int rockchip_drm_psr_register(struct drm_encoder *encoder,
-			int (*psr_set)(struct drm_encoder *, bool enable));
-void rockchip_drm_psr_unregister(struct drm_encoder *encoder);
-
-#endif /* __ROCKCHIP_DRM_PSR__ */
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index c7d4c6073ea5..21fccda70356 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -21,6 +21,7 @@ 
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_probe_helper.h>
+#include <drm/drm_self_refresh_helper.h>
 #ifdef CONFIG_DRM_ANALOGIX_DP
 #include <drm/bridge/analogix_dp.h>
 #endif
@@ -42,10 +43,11 @@ 
 #include "rockchip_drm_drv.h"
 #include "rockchip_drm_gem.h"
 #include "rockchip_drm_fb.h"
-#include "rockchip_drm_psr.h"
 #include "rockchip_drm_vop.h"
 #include "rockchip_rgb.h"
 
+#define VOP_SELF_REFRESH_ENTRY_DELAY_MS 100
+
 #define VOP_WIN_SET(vop, win, name, v) \
 		vop_reg_set(vop, &win->phy->name, win->base, ~0, v, #name)
 #define VOP_SCL_SET(vop, win, name, v) \
@@ -541,7 +543,7 @@  static void vop_core_clks_disable(struct vop *vop)
 	clk_disable(vop->hclk);
 }
 
-static int vop_enable(struct drm_crtc *crtc)
+static int vop_enable(struct drm_crtc *crtc, struct drm_crtc_state *old_state)
 {
 	struct vop *vop = to_vop(crtc);
 	int ret, i;
@@ -581,12 +583,18 @@  static int vop_enable(struct drm_crtc *crtc)
 	 * We need to make sure that all windows are disabled before we
 	 * enable the crtc. Otherwise we might try to scan from a destroyed
 	 * buffer later.
+	 *
+	 * In the case of enable-after-PSR, we don't need to worry about this
+	 * case since the buffer is guaranteed to be valid and disabling the
+	 * window will result in screen glitches on PSR exit.
 	 */
-	for (i = 0; i < vop->data->win_size; i++) {
-		struct vop_win *vop_win = &vop->win[i];
-		const struct vop_win_data *win = vop_win->data;
+	if (!old_state || !old_state->self_refresh_active) {
+		for (i = 0; i < vop->data->win_size; i++) {
+			struct vop_win *vop_win = &vop->win[i];
+			const struct vop_win_data *win = vop_win->data;
 
-		VOP_WIN_SET(vop, win, enable, 0);
+			VOP_WIN_SET(vop, win, enable, 0);
+		}
 	}
 	spin_unlock(&vop->reg_lock);
 
@@ -937,12 +945,10 @@  static void vop_plane_atomic_async_update(struct drm_plane *plane,
 	}
 
 	if (vop->is_enabled) {
-		rockchip_drm_psr_inhibit_get_state(new_state->state);
 		vop_plane_atomic_update(plane, plane->state);
 		spin_lock(&vop->reg_lock);
 		vop_cfg_done(vop);
 		spin_unlock(&vop->reg_lock);
-		rockchip_drm_psr_inhibit_put_state(new_state->state);
 	}
 
 	plane->funcs->atomic_destroy_state(plane, plane_state);
@@ -1035,7 +1041,7 @@  static void vop_crtc_atomic_enable(struct drm_crtc *crtc,
 
 	WARN_ON(vop->event);
 
-	ret = vop_enable(crtc);
+	ret = vop_enable(crtc, old_state);
 	if (ret) {
 		mutex_unlock(&vop->vop_lock);
 		DRM_DEV_ERROR(vop->dev, "Failed to enable vop (%d)\n", ret);
@@ -1509,6 +1515,13 @@  static int vop_create_crtc(struct vop *vop)
 	init_completion(&vop->line_flag_completion);
 	crtc->port = port;
 
+	ret = drm_self_refresh_helper_register(crtc,
+					       VOP_SELF_REFRESH_ENTRY_DELAY_MS);
+	if (ret)
+		DRM_DEV_DEBUG_KMS(vop->dev,
+			"Failed to register %s with SR helpers %d, ignoring\n",
+			crtc->name, ret);
+
 	return 0;
 
 err_cleanup_crtc:
@@ -1526,6 +1539,8 @@  static void vop_destroy_crtc(struct vop *vop)
 	struct drm_device *drm_dev = vop->drm_dev;
 	struct drm_plane *plane, *tmp;
 
+	drm_self_refresh_helper_unregister(crtc);
+
 	of_node_put(crtc->port);
 
 	/*
diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
index 475b706b49de..b6cc654143d7 100644
--- a/include/drm/bridge/analogix_dp.h
+++ b/include/drm/bridge/analogix_dp.h
@@ -42,9 +42,7 @@  struct analogix_dp_plat_data {
 			 struct drm_connector *);
 };
 
-int analogix_dp_psr_enabled(struct analogix_dp_device *dp);
-int analogix_dp_enable_psr(struct analogix_dp_device *dp);
-int analogix_dp_disable_psr(struct analogix_dp_device *dp);
+bool analogix_dp_self_refresh_changed(struct analogix_dp_device *dp);
 
 int analogix_dp_resume(struct analogix_dp_device *dp);
 int analogix_dp_suspend(struct analogix_dp_device *dp);