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.so...@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.v...@intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandi...@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
