[Intel-gfx] [PATCH v3 5/6] drm/i915: Disable PSR2 while getting pipe CRC

[Dhinakaran Pandiyan]

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 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/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.

	

> 
> > 
> > >  	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