On Thu, 2019-02-28 at 17:57 -0800, Dhinakaran Pandiyan wrote:
> On Thu, 2019-02-28 at 15:07 -0800, Souza, Jose wrote:
> > On Thu, 2019-02-28 at 18:58 +0200, Ville Syrjälä wrote:
> > > On Wed, Feb 27, 2019 at 05:32:58PM -0800, José Roberto de Souza
> > > wrote:
> > > > When PSR2 is active aka after the number of frames programmed
> > > > in
> > > > PSR2_CTL 'Frames Before SU Entry' hardware stops to generate
> > > > CRC
> > > > interruptions causing IGT tests to fail due timeout.
> > > > 
> > > > Oddly that don't happen when PSR1 active, so here it switches
> > > > from
> > > > PSR2 to PSR1 while user is requesting pipe CRC.
> > > > 
> > > > Force setting mode_changed as true is necessary to atomic
> > > > checks
> > > > functions compute new PSR state, that is why it was added to
> > > > intel_crtc_crc_prepare().
> > > > 
> > > > v3: Reusing intel_crtc_crc_prepare() and crc_enabled
> > > > 
> > > > v2: Changed commit description to describe that PSR2 inhibit
> > > > CRC
> > > > calculations.
> > > > 
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_pipe_crc.c | 1 +
> > > >  drivers/gpu/drm/i915/intel_psr.c      | 3 +++
> > > >  2 files changed, 4 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > index f6d0b2aaffe2..e7ac24c33650 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > > > @@ -308,6 +308,7 @@ intel_crtc_crc_prepare(struct
> > > > drm_i915_private
> > > > *dev_priv, struct drm_crtc *crtc,
> > > >  		goto put_state;
> > > >  	}
> > > >  
> > > > +	pipe_config->base.mode_changed = pipe_config-
> > > > >crc_enabled !=
> > > > enable;
> > > 
> > > Do we really want to set that unconditionally?
> > 
> > Without it atomic helpers will return ealier as there is no state
> > changes, I was wondering if the IPS was being applied in the bellow
> > connectors_changed is not set.
> > Anyways to triggers the code paths to disable PSR2 it is needed,
> > and
> > with fastboot enable by default in gen9+ it will do a fastset so
> > not
> > much drawbacks.
> What about pre gen9 platforms that do not have PSR? Running through
> state checks and acquiring several locks on platforms that will never
> need it is wasteful.
> 
> Why not make it conditional upon?
> 	if (HAS_PSR()) or even CAN_PSR() for that matter.

Okay, done.

> 
> 	
> 
> > > >  	pipe_config->crc_enabled = enable;
> > > >  
> > > >  	if (IS_HASWELL(dev_priv) && intel_crtc->pipe == PIPE_A)
> > > > {
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 6175b1d2e0c8..f7730b8b2ec0 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -572,6 +572,9 @@ static bool intel_psr2_config_valid(struct
> > > > intel_dp *intel_dp,
> > > >  		return false;
> > > >  	}
> > > >  
> > > > +	if (crtc_state->crc_enabled)
> > > > +		return false;
> > > > +
> > > >  	return true;
> > > >  }
> > > >  
> > > > -- 
> > > > 2.21.0