Re: [Intel-gfx] [PATCH 3/5] drm/i915/psr: Display WA 0884 applied broadly for more HW tracking.
On Sat, Feb 24, 2018 at 12:24:35AM +, 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 0x 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.
>
> > 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
> > Signed-off-by: Rodrigo Vivi
> > ---
> > 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?
comment added..
>
>
> > + }
> > + }
> >
> > 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.
I cannot do that. If invalidation came from invalidate the flush
needs to be able to activate it back.
Also it is protected by
if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
So it won't be called in vain...
>
>
> ___
> 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
Re: [Intel-gfx] [PATCH 3/5] drm/i915/psr: Display WA 0884 applied broadly for more HW tracking.
On Mon, 2018-02-26 at 15:08 -0800, Rodrigo Vivi wrote:
> 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 0x 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...
>
We have to figure out HW frontbuffer tracking for PSR2 and I have been
mostly refraining from thinking about PSR2 problems until now. If this
helps PSR1, let's go with it.
> 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
> > > Signed-off-by: Rodrigo Vivi
> > > ---
> > > 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 _DSPACNTR0x70180
> > > #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
Re: [Intel-gfx] [PATCH 3/5] drm/i915/psr: Display WA 0884 applied broadly for more HW tracking.
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 0x 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
> > Signed-off-by: Rodrigo Vivi
> > ---
> > 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
Re: [Intel-gfx] [PATCH 3/5] drm/i915/psr: Display WA 0884 applied broadly for more HW tracking.
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 0x 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.
> 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
> Signed-off-by: Rodrigo Vivi
> ---
> 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 _DSPACNTR0x70180
> #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?
> + }
> + }
>
> 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.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
