On Mon, 2018-03-12 at 20:49 +0200, Ville Syrjälä wrote:
> On Mon, Mar 12, 2018 at 06:40:26PM +0000, Pandiyan, Dhinakaran wrote:
> >
> >
> >
> > On Mon, 2018-03-12 at 20:29 +0200, Ville Syrjälä wrote:
> > > On Thu, Mar 08, 2018 at 04:52:18PM -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 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.
> > > >
> > > > v2: - Rebase on drm-tip
> > > > - (DK) Add a comment to explain that WA
> > > > tells about writing 0 to CUR_SURFLIVE_A but we write to
> > > > CUR_SURFLIVE(pipe).
> > > > v3: Wa doesn't work on PSR2.
> > > >
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com>
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.v...@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/i915_reg.h | 3 +++
> > > > drivers/gpu/drm/i915/intel_psr.c | 19 +++++++++++++++++--
> > > > 2 files changed, 20 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > index e6a8c0ee7df1..abdc513a9edd 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -6032,6 +6032,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)
> > >
> > > There should be a better place for this.
> > >
> > > > +
> > > > /* 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 30932527e663..b0286722a72f 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -1027,8 +1027,23 @@ 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 (dev_priv->psr.psr2_support ||
> > > > + 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.
> > > > + * Workaround tells us to write 0 to
> > > > CUR_SURLIVE_A,
> > > > + * but it makes more sense write to the current
> > > > active
> > > > + * pipe.
> > > > + */
> > >
> > > Might want to note that SURFLIVE is read only so the write should not
> > > not have any other side effects, or at least I hope that is the case.
> > >
> > > I don't really understand why we're doing PSR exit in the frontbuffer
> > > flush though. Shouldn't we have already exited on invalidate?
> >
> > fbdev code doesn't call invalidate at all and cannot call because the
> > buffer modification functions are all in atomic contexts.
>
> Oh that old chestnut again. No one thinking of adding some
> schedule_work()?
Not yet, should be doable though. Retaining exit() in flush() is simpler
and lets us move forward with necessary PSR fixes IMO.
>
> >
> >
> > >
> > > > + I915_WRITE(CUR_SURLIVE(pipe), 0);
> > > > + }
> > > > + }
> > > >
> > > > if (!dev_priv->psr.active &&
> > > > !dev_priv->psr.busy_frontbuffer_bits)
> > > > if (!work_busy(&dev_priv->psr.work.work))
> > > > --
> > > > 2.13.6
> > > >
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > >
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
