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

[Souza, Jose]

On Mon, 2019-03-04 at 11:00 -0800, Dhinakaran Pandiyan wrote:
> 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?

Exactly this sporadicts timeout should be caused by this:

- IGT test modify frontbuffer or do a flip
- IGT execution is preempted and it takes more than X idle frames
- PSR is activated
- IGT test start CRC capture
- CRC timeout

So increasing the idle frames should reduce the scenario above.

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

Good suggestion, psr_flush() should do the same job I will test and
change to that.

> 
> > > > 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;
-------------- 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/20190304/d2f64c95/attachment.sig>