Re: [PATCH] drm/i915/psr: Check for SET_POWER_CAPABLE bit at PSR init time.

[Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>]

On Fri, 2018-05-11 at 15:24 +0300, Ville Syrjälä wrote:
> On Wed, May 09, 2018 at 02:54:01PM -0700, Dhinakaran Pandiyan wrote:
> > 
> > By moving the check from psr_compute_config() to psr_init_dpcd(),
> > we get
> > to set the dev_priv->psr.sink_support flag only when the panel is
> > capable of changing power state. An additional benefit is that the
> > check
> > will be performed only at init time instead of every atomic_check.
> > 
> > This should change the psr_basic IGT failures on HSW to skips.
> > 
> > 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_dp.c  |  8 ++++++--
> >  drivers/gpu/drm/i915/intel_psr.c | 12 +++++-------
> >  2 files changed, 11 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index dde92e4af5d3..f1747dff9ca3 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3762,8 +3762,6 @@ intel_edp_init_dpcd(struct intel_dp
> > *intel_dp)
> >  		dev_priv->no_aux_handshake = intel_dp-
> > >dpcd[DP_MAX_DOWNSPREAD] &
> >  			DP_NO_AUX_HANDSHAKE_LINK_TRAINING;
> >  
> > -	intel_psr_init_dpcd(intel_dp);
> > -
> >  	/*
> >  	 * Read the eDP display control registers.
> >  	 *
> > @@ -3779,6 +3777,12 @@ intel_edp_init_dpcd(struct intel_dp
> > *intel_dp)
> >  		DRM_DEBUG_KMS("eDP DPCD: %*ph\n", (int)
> > sizeof(intel_dp->edp_dpcd),
> >  			      intel_dp->edp_dpcd);
> >  
> > +	/*
> > +	 * This has to be called after initializing intel_dp-
> > >edp_dpcd, PSR
> > +	 * checks for the SET_POWER_CAPABLE bit.
> > +	 */
> > +	intel_psr_init_dpcd(intel_dp);
> > +
> >  	/* Read the eDP 1.4+ supported link rates. */
> >  	if (intel_dp->edp_dpcd[0] >= DP_EDP_14) {
> >  		__le16 sink_rates[DP_MAX_SUPPORTED_RATES];
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index db27f2faa1de..9e6534d57479 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -251,8 +251,11 @@ void intel_psr_init_dpcd(struct intel_dp
> > *intel_dp)
> >  			 sizeof(intel_dp->psr_dpcd));
> >  
> >  	if (intel_dp->psr_dpcd[0]) {
> > -		dev_priv->psr.sink_support = true;
> > -		DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> > +		DRM_DEBUG_KMS("Detected EDP PSR panel\n");
> > +		if (intel_dp->edp_dpcd[1] & DP_EDP_SET_POWER_CAP)
> > +			dev_priv->psr.sink_support = true;
> > +		else
> > +			DRM_DEBUG_KMS("Panel lacks power state
> > control, PSR cannot be enabled\n");
> >  	}
> This function looks a bit crazy. Why are we going on to parse other
> things after psr_dpcd[0] was already deemed to be zero?
> 
> Wouldn't it make much more sense to invert the first psr_dpcd[0]
> check
> and bail out entirely?
> 
Yeah, this is wasteful. We did have the dpcd reads bounded by PSR
capability checks until recently. I'll fix it up, thanks!

-DK

> > 
> >  
> >  	if (INTEL_GEN(dev_priv) >= 9 &&
> > @@ -640,11 +643,6 @@ void intel_psr_compute_config(struct intel_dp
> > *intel_dp,
> >  		return;
> >  	}
> >  
> > -	if (!(intel_dp->edp_dpcd[1] & DP_EDP_SET_POWER_CAP)) {
> > -		DRM_DEBUG_KMS("PSR condition failed: panel lacks
> > power state control\n");
> > -		return;
> > -	}
> > -
> >  	crtc_state->has_psr = true;
> >  	crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp,
> > crtc_state);
> >  	DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ?
> > "2" : "");
> > -- 
> > 2.14.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