[Intel-gfx] [PATCH] drm/i915/psr: Don't avoid PSR when PSR2

[Pandiyan, Dhinakaran]

On Wed, 2018-02-14 at 08:03 -0800, Rodrigo Vivi wrote:
> We can still use PSR1 when PSR2 conditions are not met.
> 
> So, let's split the check in a way that we make sure has_psr
> gets set independently of PSR2 criteria.
> 
> v2: Duh! Handle proper return to avoid breaking PSR2.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 55 ++++++++++++++++++++--------------------
>  1 file changed, 28 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 200c085ba644..56e7050f9042 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -341,6 +341,33 @@ static void hsw_psr_activate(struct intel_dp *intel_dp)
>  		hsw_activate_psr1(intel_dp);
>  }
>  
> +static bool intel_psr2_conditions(struct intel_dp *intel_dp,
> +				  struct intel_crtc_state *crtc_state)

nit: static bool psr2_config_check()/psr2_config_valid()
> +{
> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> +	const struct drm_display_mode *adjusted_mode =
> +		&crtc_state->base.adjusted_mode;
> +
> +	/* PSR2 is restricted to work with panel resolutions up to 3640x2304 */
> +	if (adjusted_mode->crtc_hdisplay > 3640 ||
> +	    adjusted_mode->crtc_vdisplay > 2304) {
> +		DRM_DEBUG_KMS("PSR2 disabled, panel resolution too big\n");
> +		return false;
> +	}
> +
> +	/*
> +	 * FIXME:enable psr2 only for y-cordinate psr2 panels
> +	 * After gtc implementation , remove this restriction.
> +	 */
> +	if (!dev_priv->psr.y_cord_support) {
> +		DRM_DEBUG_KMS("PSR2 disabled, panel does not support Y coordinate\n");
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>  void intel_psr_compute_config(struct intel_dp *intel_dp,
>  			      struct intel_crtc_state *crtc_state)
>  {
> @@ -403,34 +430,8 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
>  		return;
>  	}
>  
> -	/*
> -	 * FIXME psr2_support is messed up. It's both computed
> -	 * dynamically during PSR enable, and extracted from sink
> -	 * caps during eDP detection.
> -	 */

1) edp_init_dpcd sets dev_priv->psr.psr2_support if sink supports psr2
2) compute_config sets crtc_state.has_psr2 if configuration can support
psr2
3) psr_enable copies crtc_state.has_psr2 to dev_priv->psr.psr2_support.
4) psr_activate, enable_source, enable_sink etc check
dev_priv->psr.psr2_support.

In summary, dev_priv->psr.psr2_support means different things at
different points of time. This is still messed up IMHO, we should retain
the comment at the least.


> -	if (!dev_priv->psr.psr2_support) {
> -		crtc_state->has_psr = true;
> -		return;
> -	}
psr.psr2_support is set if sink supports psr2, what is the reason to
remove this?

> -
> -	/* PSR2 is restricted to work with panel resolutions up to 3640x2304 */
> -	if (adjusted_mode->crtc_hdisplay > 3640 ||
> -	    adjusted_mode->crtc_vdisplay > 2304) {
> -		DRM_DEBUG_KMS("PSR2 disabled, panel resolution too big\n");
> -		return;
> -	}
> -
> -	/*
> -	 * FIXME:enable psr2 only for y-cordinate psr2 panels
> -	 * After gtc implementation , remove this restriction.
> -	 */
> -	if (!dev_priv->psr.y_cord_support) {
> -		DRM_DEBUG_KMS("PSR2 disabled, panel does not support Y coordinate\n");
> -		return;
> -	}
> -
>  	crtc_state->has_psr = true;
> -	crtc_state->has_psr2 = true;
> +	crtc_state->has_psr2 = intel_psr2_conditions(intel_dp, crtc_state);

We should debug log whether psr1 or psr2 is being enabled, especially
now that psr1 can be enabled on a psr2 panel.


>  }
>  
>  static void intel_psr_activate(struct intel_dp *intel_dp)