[Intel-gfx] [PATCH v4 8/9] drm/i915/psr: Set idle frames to maximum while getting pipe CRC

[Dhinakaran Pandiyan]

On Mon, 2019-03-04 at 10:40 -0800, Souza, Jose wrote:
> On Mon, 2019-03-04 at 10:31 -0800, Dhinakaran Pandiyan wrote:
> > On Fri, 2019-03-01 at 17:34 -0800, José Roberto de Souza wrote:
> > > Increase the idle frames to activate PSR1 to avoid CRC timeouts,
> > > as
> > > soon as pipe CRC is enabled it will avoid PSR1 to activate but if
> > > PSR1 is activate before that, hardware goes to lower power states
> > > that inhibits CRC calculations causing CRC timeout errors in IGT
> > > tests.

Okay, so you are solving the case where PSR is already active when we
want CRCs. How does modifying the idle frame count help if PSR is
already active? 

The commit also says PSR does not become active if CRCs were already
enabled (idle count should not matter here). In that case, just
disabling and re-enabling should be good, isn't it? Or perhaps, just
doing a psr_flush() would do the same job?

> > > 
> > > 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_psr.c | 17 +++++++++++++++--
> > >  2 files changed, 16 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 453af7438e67..e336f758e481 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -521,6 +521,7 @@ struct i915_psr {
> > >  	bool sink_not_reliable;
> > >  	bool irq_aux_error;
> > >  	u16 su_x_granularity;
> > > +	bool crc_enabled;
> > >  };
> > >  
> > >  enum intel_pch {
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index d3e3996551c6..b237d96db277 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -452,6 +452,16 @@ static void hsw_activate_psr1(struct
> > > intel_dp
> > > *intel_dp)
> > >  	 * frames, we'll go with 9 frames for now
> > >  	 */
> > >  	idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency
> > > + 1);
> > > +
> > > +	/*
> > > +	 * Increase the idle frames to active PSR1 to avoid CRC
> > > timeouts, as
> > > +	 * soon as pipe CRC is enabled it will avoid PSR1 to activate
> > > but if
> > > +	 * PSR1 is activate before that, hardware goes to lower power
> > > states
> > > +	 * that inhibits CRC calculations.
> > > +	 */
> > > +	if (dev_priv->psr.crc_enabled)
> > > +		idle_frames = 0xf;
> > 
> > My understanding was only the enable bit can be changed when PSR is
> > enabled. Does this work?
> 
> That is true, the additional changes in intel_psr_update() will
> disable
> and then enable PSR with this new idle_frames.
> 
> > 
> > > +
> > >  	val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;