[Intel-gfx] [PATCH] drm/i915: Fix PSR2 selective update corruption after PSR1 setup

[Souza, Jose]

On Tue, 2019-03-12 at 14:46 -0700, Pandiyan, Dhinakaran wrote:
> On Tue, 2019-03-12 at 14:28 -0700, Souza, Jose wrote:
> > On Tue, 2019-03-12 at 14:14 -0700, Dhinakaran Pandiyan wrote:
> > > On Tue, 2019-03-12 at 13:53 -0700, Rodrigo Vivi wrote:
> > > > On Tue, Mar 12, 2019 at 01:42:17PM -0700, José Roberto de Souza
> > > > wrote:
> > > > > For some reason if the PSR1 EDP_PSR_TP1_TP3_SEL register is
> > > > > kept
> > > > > set
> > > > > while PSR2 is enabled, it causes some selective updates to
> > > > > fail
> > > > > after
> > > > > got back from DC6 for the first time.
> > > 
> > > That's suspicious, why does a PSR1 control register have any
> > > effect
> > > on
> > > PSR2. Was PSR1 enabled before PSR2? 
> > 
> > Yes it only happens when switching from PSR1 -> PSR2, I caught this
> > issue now because I had a external connector attached so display
> > was
> > never going to DC6.
> 
> I am a bit confused now :) Do you see the issue if PSR1 was never
> enabled before enabling PSR2?

I don't see, just being cautions about how this issue could be trigged.

> 
> > > 
> > > 
> > > > > So lets clear this register before enabled PSR2, as it could
> > > > > be
> > > > > set
> > > > > by a previous i915 module, firmware/BIOS or by a previous
> > > > > mode
> > > > > that
> > > > > is not compatible with PSR2.
> > > > 
> > > > Does it happen when you don't have DMC loaded?
> > > > 
> > > > Because It looks like a DMC bug for me...
> > > > 
> > > > If by removing DMC we don't see the issue, could we please file
> > > > this bug to DMC while adding a FIXME with DMC bugged version on
> > > > it?
> > > > 
> > > > Aa: Pavana
> > > > 
> > > > If it doesn't happen with DMC loaded than maybe a HSD would for
> > > > hw
> > > > team would be good anyway.
> > > > 
> > > > Cc: Art.
> > > > 
> > > > Thanks,
> > > > Rodrigo.
> > > > 
> > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_psr.c | 12 ++++++++++--
> > > > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > > index 7bab6a009e0d..ae62f8124558 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > @@ -494,12 +494,20 @@ static void hsw_activate_psr2(struct
> > > > > intel_dp
> > > > > *intel_dp)
> > > > >  {
> > > > >  	struct drm_i915_private *dev_priv =
> > > > > dp_to_i915(intel_dp);
> > > > >  	u32 val;
> > > > > +	int idle_frames;
> > > > > +
> > > > > +	/*
> > > > > +	 * Keeping this PSR1 register set while PSR2 is enabled
> > > > > causes
> > > > > some
> > > > > +	 * PSR2 selective updates to fail, corrupting screen.
> > > > > +	 */
> > > > > +	val = I915_READ(EDP_PSR_CTL);
> > > > > +	if (val & EDP_PSR_TP1_TP3_SEL)
> > > > > +		I915_WRITE(EDP_PSR_CTL, val &
> > > > > ~EDP_PSR_TP1_TP3_SEL);
> > > 
> > > Since PSR1 should be disabled at this point, a rmw is not
> > > necessary.
> > > Does this work? 
> > > I915_WRITE(EDP_PSR_CTL, 0);
> > > 
> > > We could do the same thing in psr_exit() as well.
> > 
> > Write 0 also works and I'm doing RMW because I always try to save
> > any
> > writes.
> Why so? Do we want to retain the old value in PSR_CTL when PSR2 is
> enabled? 

We don't need the old PSR_CTL value but on psr_exit() we can't set to 0
for sure as BSpec says to not modify the other registers when the
enable bit is set, it probably uses the TP SEL and TP training values
to do the PSR exit.
Not a strong argument but in here I would say is better to unset just
the one causing the issue so we can better track the issue in DMC side.

> 
> > No need to do it in psr_exit(), the important is do it here because
> > other i915 module or BIOS could leave it set.
> Right, that was an orthogonal suggestion to avoid the RMW that we do
> to
> disable PSR1 and PSR2.
> 
> psr1 and psr2 activate functions recompute the values fully, so there
> is no benefit in flipping just the enable bit in psr_exit().
> > > > >  
> > > > >  	/* Let's use 6 as the minimum to cover all known cases
> > > > > including the
> > > > >  	 * off-by-one issue that HW has in some cases.
> > > > >  	 */
> > > > > -	int idle_frames = max(6, dev_priv-
> > > > > > vbt.psr.idle_frames);
> > > > > -
> > > > > +	idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
> > > > >  	idle_frames = max(idle_frames, dev_priv-
> > > > > > psr.sink_sync_latency
> > > > > + 1);
> > > > >  	val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT;
> > > > >  
> > > > > -- 
> > > > > 2.21.0
> > > > > 
-------------- 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/20190312/5c8e95ad/attachment.sig>