[Intel-gfx] [PATCH 2/4] drm/i915: Disable PSR2 while getting pipe CRC

[Souza, Jose]

On Thu, 2019-02-14 at 11:00 -0800, Dhinakaran Pandiyan wrote:
> On Wed, 2019-02-13 at 18:02 -0800, José Roberto de Souza wrote:
> > As stated in CRC_CTL spec, after PSR entry state CRC will not be
> > calculated anymore what is not a problem as IGT tests do some
> > screen
> > change and then request the pipe CRC right after the change so PSR
> > will go to idle state and only will entry again after at least 6
> > idles frames.
> > 
> > But for PSR2 it is more problematic as any change to the screen
> > could
> > trigger a selective/partial update causing the CRC value not to be
> > calculated over the full frame.
> Okay, that reasoning runs counter to my understanding. My
> understanding
> is that the whole frame is fetched and processed at the pipe level
> but
> the DDI sends selective blocks of pixels. So, if the CRC's are
> calculated at the pipe level, the CRC should be for the full frame
> with
> PSR2 having no effect? Checking bspec, I see there are DDI CRCs as
> well, which should reflect the partial frame that PSR2 sends.
> 
> To get a better understanding, I'd like to know what the source for
> mismatching CRCs is?

Well the naming is misleading for newer gens, before BDW this was a
pipe register(8067) but now it is a DDI register(7536) but the register
offset was kept the same.

In my testing hardware generate 4 interruptions with wrong CRC values
then stops forever probably because vblank interruptions are off.

> 
> 
> > So here it disables PSR2 and keep it disabled while user is
> > requesting pipe CRC.
> > 
> > BSpec: 7536
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h       |  1 +
> >  drivers/gpu/drm/i915/intel_drv.h      |  1 +
> >  drivers/gpu/drm/i915/intel_pipe_crc.c | 10 ++++++++++
> >  drivers/gpu/drm/i915/intel_psr.c      | 23 +++++++++++++++++++++++
> >  4 files changed, 35 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 17fe942eaafa..609e9c5bd453 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -520,6 +520,7 @@ struct i915_psr {
> >  	bool sink_not_reliable;
> >  	bool irq_aux_error;
> >  	u16 su_x_granularity;
> > +	bool pipe_crc_enabled;
> >  };
> >  
> >  enum intel_pch {
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 3398b28c053b..40ce7a600585 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -2103,6 +2103,7 @@ void intel_psr_short_pulse(struct intel_dp
> > *intel_dp);
> >  int intel_psr_wait_for_idle(const struct intel_crtc_state
> > *new_crtc_state,
> >  			    u32 *out_value);
> >  bool intel_psr_enabled(struct intel_dp *intel_dp);
> > +void intel_psr_crc_prepare_or_finish(struct drm_i915_private
> > *dev_priv, enum pipe pipe, bool prepare);
> >  
> >  /* intel_quirks.c */
> >  void intel_init_quirks(struct drm_i915_private *dev_priv);
> > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > index a8554dc4f196..5d8772399f60 100644
> > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > @@ -583,6 +583,14 @@ int intel_crtc_verify_crc_source(struct
> > drm_crtc
> > *crtc, const char *source_name,
> >  	return -EINVAL;
> >  }
> >  
> > +static inline void intel_crtc_crc_prepare_or_finish(struct
> > drm_crtc
> > *crtc, bool prepare)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +
> > +	intel_psr_crc_prepare_or_finish(dev_priv, intel_crtc->pipe,
> > prepare);
> > +}
> > +
> >  int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char
> > *source_name)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > @@ -609,6 +617,8 @@ int intel_crtc_set_crc_source(struct drm_crtc
> > *crtc, const char *source_name)
> >  	if (ret != 0)
> >  		goto out;
> >  
> > +	intel_crtc_crc_prepare_or_finish(crtc, source !=
> > INTEL_PIPE_CRC_SOURCE_NONE);
> > +
> >  	pipe_crc->source = source;
> >  	I915_WRITE(PIPE_CRC_CTL(crtc->index), val);
> >  	POSTING_READ(PIPE_CRC_CTL(crtc->index));
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 08967836b48e..9c93138988aa 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -577,6 +577,9 @@ static bool intel_psr2_config_valid(struct
> > intel_dp *intel_dp,
> >  		return false;
> >  	}
> >  
> > +	if (dev_priv->psr.pipe_crc_enabled)
> > +		return false;
> > +
> >  	return true;
> >  }
> >  
> > @@ -1291,3 +1294,23 @@ bool intel_psr_enabled(struct intel_dp
> > *intel_dp)
> >  
> >  	return ret;
> >  }
> > +
> > +void intel_psr_crc_prepare_or_finish(struct drm_i915_private
> > *dev_priv, enum pipe pipe, bool prepare)
> > +{
> > +	bool fastset = false;
> > +
> > +	if (!CAN_PSR(dev_priv))
> > +		return;
> > +
> > +	mutex_lock(&dev_priv->psr.lock);
> > +
> > +	if (dev_priv->psr.pipe == pipe) {
> > +		dev_priv->psr.pipe_crc_enabled = prepare;
> > +		fastset = !prepare || dev_priv->psr.psr2_enabled;
> > +	}
> > +
> > +	mutex_unlock(&dev_priv->psr.lock);
> > +
> > +	if (fastset)
> > +		intel_psr_fastset_force(dev_priv);
> > +}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20190214/2896d06b/attachment-0001.sig>