diff mbox series

[1/2] drm/i915/psr: Remove wait_for_idle() for PSR2

Message ID 20180824230844.12428-1-dhinakaran.pandiyan@intel.com (mailing list archive)
State New, archived
Headers show
Series [1/2] drm/i915/psr: Remove wait_for_idle() for PSR2 | expand

Commit Message

Dhinakaran Pandiyan Aug. 24, 2018, 11:08 p.m. UTC
CI runs show PSR2 does not go to IDLE with selective update enabled on
all PSR exit triggers. Specifically, logs indicate the hardware enters
"SLEEP Selective Update" and not "IDLE Reset state', like the kernel
expects, when vblank interrupts are enabled. This check was added for PSR1
but incorrectly extended to PSR2, remove the check as it breaks tests
and prints out misleading error messages.

v2: Split out non-code changes (Rodrigo)

Cc: Tarun Vyas <tarun.vyas@intel.com>
Cc: José Roberto de Souza <jose.souza@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Fixes: c43dbcbbcc8c ("drm/i915/psr: Lockless version of psr_wait_for_idle")
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

Comments

Rodrigo Vivi Aug. 24, 2018, 11:18 p.m. UTC | #1
On Fri, Aug 24, 2018 at 04:08:43PM -0700, Dhinakaran Pandiyan wrote:
> CI runs show PSR2 does not go to IDLE with selective update enabled on
> all PSR exit triggers. Specifically, logs indicate the hardware enters
> "SLEEP Selective Update" and not "IDLE Reset state', like the kernel
> expects, when vblank interrupts are enabled. This check was added for PSR1
> but incorrectly extended to PSR2, remove the check as it breaks tests
> and prints out misleading error messages.
> 
> v2: Split out non-code changes (Rodrigo)
> 
> Cc: Tarun Vyas <tarun.vyas@intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Fixes: c43dbcbbcc8c ("drm/i915/psr: Lockless version of psr_wait_for_idle")
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_psr.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index da583a45e942..2cb931f3019b 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -771,8 +771,6 @@ int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state,
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	i915_reg_t reg;
> -	u32 mask;
>  
>  	if (!dev_priv->psr.enabled || !new_crtc_state->has_psr)
>  		return 0;
> @@ -787,13 +785,10 @@ int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state,
>  	 * not needed and will induce latencies in the atomic
>  	 * update path.
>  	 */
> -	if (dev_priv->psr.psr2_enabled) {
> -		reg = EDP_PSR2_STATUS;
> -		mask = EDP_PSR2_STATUS_STATE_MASK;
> -	} else {
> -		reg = EDP_PSR_STATUS;
> -		mask = EDP_PSR_STATUS_STATE_MASK;
> -	}
> +
> +	/* FIXME: Update this for PSR2 if we need to wait for idle */
> +	if (READ_ONCE(dev_priv->psr.psr2_enabled))
> +		return 0;
>  
>  	/*
>  	 * Max time for PSR to idle = Inverse of the refresh rate +
> @@ -801,7 +796,8 @@ int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state,
>  	 * handshake. 50 msec is defesive enough to cover everything.
>  	 */
>  
> -	return __intel_wait_for_register(dev_priv, reg, mask,
> +	return __intel_wait_for_register(dev_priv, EDP_PSR_STATUS,
> +					 EDP_PSR_STATUS_STATE_MASK,
>  					 EDP_PSR_STATUS_STATE_IDLE, 2, 50,
>  					 out_value);
>  }
> -- 
> 2.17.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index da583a45e942..2cb931f3019b 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -771,8 +771,6 @@  int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state,
 {
 	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-	i915_reg_t reg;
-	u32 mask;
 
 	if (!dev_priv->psr.enabled || !new_crtc_state->has_psr)
 		return 0;
@@ -787,13 +785,10 @@  int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state,
 	 * not needed and will induce latencies in the atomic
 	 * update path.
 	 */
-	if (dev_priv->psr.psr2_enabled) {
-		reg = EDP_PSR2_STATUS;
-		mask = EDP_PSR2_STATUS_STATE_MASK;
-	} else {
-		reg = EDP_PSR_STATUS;
-		mask = EDP_PSR_STATUS_STATE_MASK;
-	}
+
+	/* FIXME: Update this for PSR2 if we need to wait for idle */
+	if (READ_ONCE(dev_priv->psr.psr2_enabled))
+		return 0;
 
 	/*
 	 * Max time for PSR to idle = Inverse of the refresh rate +
@@ -801,7 +796,8 @@  int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state,
 	 * handshake. 50 msec is defesive enough to cover everything.
 	 */
 
-	return __intel_wait_for_register(dev_priv, reg, mask,
+	return __intel_wait_for_register(dev_priv, EDP_PSR_STATUS,
+					 EDP_PSR_STATUS_STATE_MASK,
 					 EDP_PSR_STATUS_STATE_IDLE, 2, 50,
 					 out_value);
 }