All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] drm/bridge: analogix_dp: Make PSR-exit block less
@ 2021-10-29 23:50 Brian Norris
  2021-11-03 19:31   ` Sean Paul
  0 siblings, 1 reply; 3+ messages in thread
From: Brian Norris @ 2021-10-29 23:50 UTC (permalink / raw)
  To: Andrzej Hajda, Neil Armstrong, Robert Foss
  Cc: Jonas Karlman, dri-devel, Sean Paul, Jernej Skrabec,
	Laurent Pinchart, Brian Norris, stable, Zain Wang, Tomasz Figa,
	Heiko Stuebner, Sean Paul

Prior to commit 6c836d965bad ("drm/rockchip: Use the helpers for PSR"),
"PSR exit" used non-blocking analogix_dp_send_psr_spd(). The refactor
started using the blocking variant, for a variety of reasons -- quoting
Sean Paul's potentially-faulty memory:

"""
 - To avoid racing a subsequent PSR entry (if exit takes a long time)
 - To avoid racing disable/modeset
 - We're not displaying new content while exiting PSR anyways, so there
   is minimal utility in allowing frames to be submitted
 - We're lying to userspace telling them frames are on the screen when
   we're just dropping them on the floor
"""

However, I'm finding that this blocking transition is causing upwards of
60+ ms of unneeded latency on PSR-exit, to the point that initial cursor
movements when leaving PSR are unbearably jumpy.

It turns out that we need to meet in the middle somewhere: Sean is right
that we were "lying to userspace" with a non-blocking PSR-exit, but the
new blocking behavior is also waiting too long:

According to the eDP specification, the sink device must support PSR
entry transitions from both state 4 (ACTIVE_RESYNC) and state 0
(INACTIVE). It also states that in ACTIVE_RESYNC, "the Sink device must
display the incoming active frames from the Source device with no
visible glitches and/or artifacts."

Thus, for our purposes, we only need to wait for ACTIVE_RESYNC before
moving on; we are ready to display video, and subsequent PSR-entry is
safe.

Tested on a Samsung Chromebook Plus (i.e., Rockchip RK3399 Gru Kevin),
where this saves about 60ms of latency, for PSR-exit that used to
take about 80ms.

Fixes: 6c836d965bad ("drm/rockchip: Use the helpers for PSR")
Cc: <stable@vger.kernel.org>
Cc: Zain Wang <wzz@rock-chips.com>
Cc: Tomasz Figa <tfiga@chromium.org>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
CC list is partially constructed from the commit message of the Fixed
commit

Changes in v2:
 - retitled subject (previous: "drm/bridge: analogix_dp: Make
   PSR-disable non-blocking")
 - instead of completely non-blocking, make this "less"-blocking
 - more background (thanks Sean!)
 - more specification details

 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
index cab6c8b92efd..f8e119e84ae2 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
@@ -998,11 +998,21 @@ int analogix_dp_send_psr_spd(struct analogix_dp_device *dp,
 	if (!blocking)
 		return 0;
 
+	/*
+	 * db[1]==0: entering PSR, wait for fully active remote frame buffer.
+	 * db[1]!=0: exiting PSR, wait for either
+	 *  (a) ACTIVE_RESYNC - the sink "must display the
+	 *      incoming active frames from the Source device with no visible
+	 *      glitches and/or artifacts", even though timings may still be
+	 *      re-synchronizing; or
+	 *  (b) INACTIVE - the transition is fully complete.
+	 */
 	ret = readx_poll_timeout(analogix_dp_get_psr_status, dp, psr_status,
 		psr_status >= 0 &&
 		((vsc->db[1] && psr_status == DP_PSR_SINK_ACTIVE_RFB) ||
-		(!vsc->db[1] && psr_status == DP_PSR_SINK_INACTIVE)), 1500,
-		DP_TIMEOUT_PSR_LOOP_MS * 1000);
+		(!vsc->db[1] && (psr_status == DP_PSR_SINK_ACTIVE_RESYNC ||
+				 psr_status == DP_PSR_SINK_INACTIVE))),
+		1500, DP_TIMEOUT_PSR_LOOP_MS * 1000);
 	if (ret) {
 		dev_warn(dp->dev, "Failed to apply PSR %d\n", ret);
 		return ret;
-- 
2.33.1.1089.g2158813163f-goog


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH v2] drm/bridge: analogix_dp: Make PSR-exit block less
  2021-10-29 23:50 [PATCH v2] drm/bridge: analogix_dp: Make PSR-exit block less Brian Norris
@ 2021-11-03 19:31   ` Sean Paul
  0 siblings, 0 replies; 3+ messages in thread
From: Sean Paul @ 2021-11-03 19:31 UTC (permalink / raw)
  To: Brian Norris
  Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Jonas Karlman,
	dri-devel, Sean Paul, Jernej Skrabec, Laurent Pinchart, stable,
	Zain Wang, Tomasz Figa, Heiko Stuebner, Sean Paul

On Fri, Oct 29, 2021 at 04:50:55PM -0700, Brian Norris wrote:
> Prior to commit 6c836d965bad ("drm/rockchip: Use the helpers for PSR"),
> "PSR exit" used non-blocking analogix_dp_send_psr_spd(). The refactor
> started using the blocking variant, for a variety of reasons -- quoting
> Sean Paul's potentially-faulty memory:
> 
> """
>  - To avoid racing a subsequent PSR entry (if exit takes a long time)
>  - To avoid racing disable/modeset
>  - We're not displaying new content while exiting PSR anyways, so there
>    is minimal utility in allowing frames to be submitted
>  - We're lying to userspace telling them frames are on the screen when
>    we're just dropping them on the floor
> """
> 
> However, I'm finding that this blocking transition is causing upwards of
> 60+ ms of unneeded latency on PSR-exit, to the point that initial cursor
> movements when leaving PSR are unbearably jumpy.
> 
> It turns out that we need to meet in the middle somewhere: Sean is right
> that we were "lying to userspace" with a non-blocking PSR-exit, but the
> new blocking behavior is also waiting too long:
> 
> According to the eDP specification, the sink device must support PSR
> entry transitions from both state 4 (ACTIVE_RESYNC) and state 0
> (INACTIVE). It also states that in ACTIVE_RESYNC, "the Sink device must
> display the incoming active frames from the Source device with no
> visible glitches and/or artifacts."
> 
> Thus, for our purposes, we only need to wait for ACTIVE_RESYNC before
> moving on; we are ready to display video, and subsequent PSR-entry is
> safe.
> 
> Tested on a Samsung Chromebook Plus (i.e., Rockchip RK3399 Gru Kevin),
> where this saves about 60ms of latency, for PSR-exit that used to
> take about 80ms.
> 
> Fixes: 6c836d965bad ("drm/rockchip: Use the helpers for PSR")
> Cc: <stable@vger.kernel.org>
> Cc: Zain Wang <wzz@rock-chips.com>
> Cc: Tomasz Figa <tfiga@chromium.org>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: Sean Paul <seanpaul@chromium.org>

Thank you for revising this!

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> CC list is partially constructed from the commit message of the Fixed
> commit
> 
> Changes in v2:
>  - retitled subject (previous: "drm/bridge: analogix_dp: Make
>    PSR-disable non-blocking")
>  - instead of completely non-blocking, make this "less"-blocking
>  - more background (thanks Sean!)
>  - more specification details
> 
>  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> index cab6c8b92efd..f8e119e84ae2 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> @@ -998,11 +998,21 @@ int analogix_dp_send_psr_spd(struct analogix_dp_device *dp,
>  	if (!blocking)
>  		return 0;
>  
> +	/*
> +	 * db[1]==0: entering PSR, wait for fully active remote frame buffer.
> +	 * db[1]!=0: exiting PSR, wait for either
> +	 *  (a) ACTIVE_RESYNC - the sink "must display the
> +	 *      incoming active frames from the Source device with no visible
> +	 *      glitches and/or artifacts", even though timings may still be
> +	 *      re-synchronizing; or
> +	 *  (b) INACTIVE - the transition is fully complete.
> +	 */
>  	ret = readx_poll_timeout(analogix_dp_get_psr_status, dp, psr_status,
>  		psr_status >= 0 &&
>  		((vsc->db[1] && psr_status == DP_PSR_SINK_ACTIVE_RFB) ||
> -		(!vsc->db[1] && psr_status == DP_PSR_SINK_INACTIVE)), 1500,
> -		DP_TIMEOUT_PSR_LOOP_MS * 1000);
> +		(!vsc->db[1] && (psr_status == DP_PSR_SINK_ACTIVE_RESYNC ||
> +				 psr_status == DP_PSR_SINK_INACTIVE))),
> +		1500, DP_TIMEOUT_PSR_LOOP_MS * 1000);
>  	if (ret) {
>  		dev_warn(dp->dev, "Failed to apply PSR %d\n", ret);
>  		return ret;
> -- 
> 2.33.1.1089.g2158813163f-goog
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH v2] drm/bridge: analogix_dp: Make PSR-exit block less
@ 2021-11-03 19:31   ` Sean Paul
  0 siblings, 0 replies; 3+ messages in thread
From: Sean Paul @ 2021-11-03 19:31 UTC (permalink / raw)
  To: Brian Norris
  Cc: Jernej Skrabec, Jonas Karlman, Neil Armstrong, dri-devel,
	Tomasz Figa, Zain Wang, Sean Paul, Robert Foss, Andrzej Hajda,
	stable, Sean Paul, Laurent Pinchart

On Fri, Oct 29, 2021 at 04:50:55PM -0700, Brian Norris wrote:
> Prior to commit 6c836d965bad ("drm/rockchip: Use the helpers for PSR"),
> "PSR exit" used non-blocking analogix_dp_send_psr_spd(). The refactor
> started using the blocking variant, for a variety of reasons -- quoting
> Sean Paul's potentially-faulty memory:
> 
> """
>  - To avoid racing a subsequent PSR entry (if exit takes a long time)
>  - To avoid racing disable/modeset
>  - We're not displaying new content while exiting PSR anyways, so there
>    is minimal utility in allowing frames to be submitted
>  - We're lying to userspace telling them frames are on the screen when
>    we're just dropping them on the floor
> """
> 
> However, I'm finding that this blocking transition is causing upwards of
> 60+ ms of unneeded latency on PSR-exit, to the point that initial cursor
> movements when leaving PSR are unbearably jumpy.
> 
> It turns out that we need to meet in the middle somewhere: Sean is right
> that we were "lying to userspace" with a non-blocking PSR-exit, but the
> new blocking behavior is also waiting too long:
> 
> According to the eDP specification, the sink device must support PSR
> entry transitions from both state 4 (ACTIVE_RESYNC) and state 0
> (INACTIVE). It also states that in ACTIVE_RESYNC, "the Sink device must
> display the incoming active frames from the Source device with no
> visible glitches and/or artifacts."
> 
> Thus, for our purposes, we only need to wait for ACTIVE_RESYNC before
> moving on; we are ready to display video, and subsequent PSR-entry is
> safe.
> 
> Tested on a Samsung Chromebook Plus (i.e., Rockchip RK3399 Gru Kevin),
> where this saves about 60ms of latency, for PSR-exit that used to
> take about 80ms.
> 
> Fixes: 6c836d965bad ("drm/rockchip: Use the helpers for PSR")
> Cc: <stable@vger.kernel.org>
> Cc: Zain Wang <wzz@rock-chips.com>
> Cc: Tomasz Figa <tfiga@chromium.org>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: Sean Paul <seanpaul@chromium.org>

Thank you for revising this!

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> CC list is partially constructed from the commit message of the Fixed
> commit
> 
> Changes in v2:
>  - retitled subject (previous: "drm/bridge: analogix_dp: Make
>    PSR-disable non-blocking")
>  - instead of completely non-blocking, make this "less"-blocking
>  - more background (thanks Sean!)
>  - more specification details
> 
>  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> index cab6c8b92efd..f8e119e84ae2 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> @@ -998,11 +998,21 @@ int analogix_dp_send_psr_spd(struct analogix_dp_device *dp,
>  	if (!blocking)
>  		return 0;
>  
> +	/*
> +	 * db[1]==0: entering PSR, wait for fully active remote frame buffer.
> +	 * db[1]!=0: exiting PSR, wait for either
> +	 *  (a) ACTIVE_RESYNC - the sink "must display the
> +	 *      incoming active frames from the Source device with no visible
> +	 *      glitches and/or artifacts", even though timings may still be
> +	 *      re-synchronizing; or
> +	 *  (b) INACTIVE - the transition is fully complete.
> +	 */
>  	ret = readx_poll_timeout(analogix_dp_get_psr_status, dp, psr_status,
>  		psr_status >= 0 &&
>  		((vsc->db[1] && psr_status == DP_PSR_SINK_ACTIVE_RFB) ||
> -		(!vsc->db[1] && psr_status == DP_PSR_SINK_INACTIVE)), 1500,
> -		DP_TIMEOUT_PSR_LOOP_MS * 1000);
> +		(!vsc->db[1] && (psr_status == DP_PSR_SINK_ACTIVE_RESYNC ||
> +				 psr_status == DP_PSR_SINK_INACTIVE))),
> +		1500, DP_TIMEOUT_PSR_LOOP_MS * 1000);
>  	if (ret) {
>  		dev_warn(dp->dev, "Failed to apply PSR %d\n", ret);
>  		return ret;
> -- 
> 2.33.1.1089.g2158813163f-goog
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-11-03 19:31 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-29 23:50 [PATCH v2] drm/bridge: analogix_dp: Make PSR-exit block less Brian Norris
2021-11-03 19:31 ` Sean Paul
2021-11-03 19:31   ` Sean Paul

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.