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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Aug 09, 2018 at 05:41:35PM -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. This check was added for PSR1 but incorrectly extended to PSR2,
> remove this check for PSR2 as there is a plan to test only PSR1 on PSR2
> panels.
> 
> Also add bspec reference to the comment about idle timeout.
> 
> Cc: Tarun Vyas <tarun.vyas@intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 39 ++++++++++++--------------------
>  1 file changed, 14 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 5686ddaa6a72..09be9bfee2be 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -722,37 +722,26 @@ 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 (!new_crtc_state->has_psr)
> -		return 0;
>  
>  	/*
> -	 * The sole user right now is intel_pipe_update_start(),
> -	 * which won't race with psr_enable/disable, which is
> -	 * where psr2_enabled is written to. So, we don't need
> -	 * to acquire the psr.lock. More importantly, we want the
> -	 * latency inside intel_pipe_update_start() to be as low
> -	 * as possible, so no need to acquire psr.lock when it is
> -	 * not needed and will induce latencies in the atomic
> -	 * update path.
> +	 * The sole user right now is intel_pipe_update_start(), which won't
> +	 * race with psr_enable/disable where psr2_enabled is written to. So, we
> +	 * don't need to acquire the psr.lock. More importantly, we want the
> +	 * latency inside intel_pipe_update_start() to be as low as possible, so
> +	 * no need to acquire psr.lock when it is not needed and will induce
> +	 * latencies in the atomic update path.
>  	 */

I think we shouldn't change this format here to keep patch cleaner...
if there is any change here I couldn't see because it is changing all
lines and if there is no change I think it is better not to touch because
it removes the focus of the real changes.

> -	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;
> -	}
> +	if (!new_crtc_state->has_psr || READ_ONCE(dev_priv->psr.psr2_enabled))

I now see that we are removing psr2 of the picture, but I don't see how we are
improving psr2 situation here.
what am I missing?

> +		return 0;
>  
>  	/*
> -	 * Max time for PSR to idle = Inverse of the refresh rate +
> -	 * 6 ms of exit training time + 1.5 ms of aux channel
> -	 * handshake. 50 msec is defesive enough to cover everything.
> +	 * From Bspec Panel Self Refresh (BDW+):

This is another case, if we didn't change the format only this line ^
would be in the patch and it would be cleaner and easier to review the
changes.

but my biggest concern with this patch is how do we check now wait_psr2 idle

> +	 * Max. time for PSR to idle = inverse of the refresh rate + 6 ms of
> +	 * exit training time + 1.5 ms of aux channel handshake. 50 ms is
> +	 * defensive 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx