[Intel-gfx] [PATCH v2 4/5] drm/i915/psr: Check if sink PSR capability changed

[Souza, Jose]

On Wed, 2019-11-27 at 17:21 -0800, Matt Roper wrote:
> On Mon, Nov 25, 2019 at 04:53:59PM -0800, José Roberto de Souza
> wrote:
> > eDP specification states that sink can have its PSR capability
> > changed, I have never found any panel doing that but lets add that
> > for completeness.
> > For now it is not reading back the PSR capabilities and if possible
> > re-enabling PSR, this will be added if a panel is found using this
> > feature.
> 
> I'm not super familiar with PSR details, but is it required for us to
> disable PSR in this case?  From a quick skim of the spec it sounds
> like
> the sink is required to keep operating with the same capabilities
> until
> the source clears the CAPS_CHANGE bit (which we never do in the patch
> below).  What happens if we just ignore the sink's notification and
> never ack it by writing a 1 to clear the bit?

Yeah, it is not clearing DP_PSR_CAPS_CHANGE will fix that, thanks.

If we just ignore, sink is supposed to keep the current capabilities
but if wants to change it is because it has some internal problem that
is causing or will cause image corruption.


> 
> I guess disabling is still probably the safest thing to do if we're
> not
> sure and don't have any real panels to test it out with.  Should we
> still clean by clearing the bit even though we disabled PSR?
> 
> Otherwise,
> 
> Reviewed-by: Matt Roper <matthew.d.roper at intel.com>
> 
> 
> > Cc: Gwan-gyeong Mun <gwan-gyeong.mun at intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_psr.c | 21
> > +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index a757b6445f21..ce76e1c6a0b9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -1437,6 +1437,26 @@ static void psr_alpm_check(struct intel_dp
> > *intel_dp)
> >  	}
> >  }
> >  
> > +static void psr_capability_changed_check(struct intel_dp
> > *intel_dp)
> > +{
> > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > +	struct i915_psr *psr = &dev_priv->psr;
> > +	u8 val;
> > +	int r;
> > +
> > +	r = drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_ESI, &val);
> > +	if (r != 1) {
> > +		DRM_ERROR("Error reading DP_PSR_ESI\n");
> > +		return;
> > +	}
> > +
> > +	if (val & DP_PSR_CAPS_CHANGE) {
> > +		intel_psr_disable_locked(intel_dp);
> > +		psr->sink_not_reliable = true;
> > +		DRM_DEBUG_KMS("Sink PSR capability changed, disabling
> > PSR\n");
> > +	}
> > +}
> > +
> >  void intel_psr_short_pulse(struct intel_dp *intel_dp)
> >  {
> >  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > @@ -1480,6 +1500,7 @@ void intel_psr_short_pulse(struct intel_dp
> > *intel_dp)
> >  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_ERROR_STATUS,
> > error_status);
> >  
> >  	psr_alpm_check(intel_dp);
> > +	psr_capability_changed_check(intel_dp);
> >  
> >  exit:
> >  	mutex_unlock(&psr->lock);
> > -- 
> > 2.24.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx