Re: [PATCH 3/5] drm/i915/psr: Display WA 0884 applied broadly for more HW tracking.

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Feb 23, 2018 at 04:24:35PM -0800, Pandiyan, Dhinakaran wrote:
> On Tue, 2018-02-13 at 15:26 -0800, Rodrigo Vivi wrote:
> > WA 0884:bxt:all,cnl:*:A - "When FBC is enabled with eDP PSR,
> > the CPU host modify writes may not get updated on the Display
> > as expected.
> > WA: Write 0x00000000 to CUR_SURFLIVE_A with every CPU
> > host modify write to trigger PSR exit."
> > 
> > We can also find on spec other cases where they describe
> > bogus writes to cursor registers to force PSR exit with
> > HW tracking. And it was confirmed by HW engineers that
> > this Wa can be safely applied for any frontbuffer activity.
> > 
> 
> So the idea is to do a dummy MMIO write to trigger PSR exit.

yeap. But not good for PSR2 though :(
We would need something else...

any idea?

So I will hold the v2 for now...

> 
> > So let's use this more and more here instead of forcibly
> > disable and re-enable PSR everytime that we have a simple
> > reliable flush case.
> > 
> > Other commits improve the fbcon/fbdev use a lot, but this
> > approach is the only when where we can get a fully reliable
> > console with no slowness or missed frames and PSR still
> > enabled and active.
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h  |  3 +++
> >  drivers/gpu/drm/i915/intel_psr.c | 15 +++++++++++++--
> >  2 files changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index f6afa5e5e7c1..ac09d17cd835 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6007,6 +6007,9 @@ enum {
> >  #define IVB_CURSOR_B_OFFSET 0x71080
> >  #define IVB_CURSOR_C_OFFSET 0x72080
> >  
> > +#define _CUR_SURLIVE		0x700AC
> > +#define CUR_SURLIVE(pipe)	_CURSOR2(pipe, _CUR_SURLIVE)
> 
> Register address is correct.
> This is a *status* register that provides current surface base address.
> We aren't reading this register anywhere, so writing to it should be
> fine.
> 
> > +
> >  /* Display A control */
> >  #define _DSPACNTR				0x70180
> >  #define   DISPLAY_PLANE_ENABLE			(1<<31)
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 13409c6301e8..49554036ffb8 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -946,8 +946,19 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
> >  	dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
> >  
> >  	/* By definition flush = invalidate + flush */
> > -	if (frontbuffer_bits)
> > -		intel_psr_exit(dev_priv);
> > +	if (frontbuffer_bits) {
> > +		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > +			intel_psr_exit(dev_priv);
> > +		} else {
> > +			/*
> > +			 * Display WA #0884: all
> > +			 * This documented WA for bxt can be safely applied
> > +			 * broadly so we can force HW tracking to exit PSR
> > +			 * instead of disabling and re-enabling.
> > +			 */
> > +			I915_WRITE(CUR_SURLIVE(pipe), 0);
> 
> The workaround asks 0 to be written to CUR_SURFLIVE_A. But I think
> writing to the active pipe register makes sense.Can you add that to the
> comment since the patch deviates from the workaround?

yeap, worth a comment...

> 
> 
> > +		}
> > +	}
> >  
> >  	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits) {
> >  		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> 
> 
> There is a psr_activate that follows, you should remove that too. HW
> should be able to activate PSR by itself.

agreed

> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx