* [PATCH] drm/i915/psr: Display WA 0884 applied broadly for more HW tracking.
@ 2018-03-09 0:52 Rodrigo Vivi
2018-03-09 1:23 ` ✓ Fi.CI.BAT: success for " Patchwork
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Rodrigo Vivi @ 2018-03-09 0:52 UTC (permalink / raw)
To: intel-gfx; +Cc: Dhinakaran Pandiyan, Rodrigo Vivi
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.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 | 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)
+
/* 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.
+ */
+ 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915/psr: Display WA 0884 applied broadly for more HW tracking.
2018-03-09 0:52 [PATCH] drm/i915/psr: Display WA 0884 applied broadly for more HW tracking Rodrigo Vivi
@ 2018-03-09 1:23 ` Patchwork
2018-03-09 5:23 ` ✓ Fi.CI.IGT: " Patchwork
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2018-03-09 1:23 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/psr: Display WA 0884 applied broadly for more HW tracking.
URL : https://patchwork.freedesktop.org/series/39648/
State : success
== Summary ==
Series 39648v1 drm/i915/psr: Display WA 0884 applied broadly for more HW tracking.
https://patchwork.freedesktop.org/api/1.0/series/39648/revisions/1/mbox/
---- Known issues:
Test gem_mmap_gtt:
Subgroup basic-small-bo-tiledx:
pass -> FAIL (fi-gdg-551) fdo#102575
Test prime_vgem:
Subgroup basic-fence-flip:
pass -> FAIL (fi-ilk-650) fdo#104008
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:421s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:421s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:371s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:498s
fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:279s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:487s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:488s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:486s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:465s
fi-cfl-8700k total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:408s
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:585s
fi-cnl-y3 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:572s
fi-elk-e7500 total:288 pass:229 dwarn:0 dfail:0 fail:0 skip:59 time:417s
fi-gdg-551 total:288 pass:179 dwarn:0 dfail:0 fail:1 skip:108 time:291s
fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:517s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:396s
fi-ilk-650 total:288 pass:227 dwarn:0 dfail:0 fail:1 skip:60 time:411s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:463s
fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:424s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:471s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:464s
fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:505s
fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:582s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:438s
fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:518s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:531s
fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:502s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:483s
fi-skl-guc total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:421s
fi-snb-2520m total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:526s
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:396s
Blacklisted hosts:
fi-cnl-drrs total:288 pass:257 dwarn:3 dfail:0 fail:0 skip:19 time:508s
469c28df8d66d3cc0a4a2e4e12433a5c92102022 drm-tip: 2018y-03m-08d-22h-40m-12s UTC integration manifest
1888eb10d023 drm/i915/psr: Display WA 0884 applied broadly for more HW tracking.
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8281/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* ✓ Fi.CI.IGT: success for drm/i915/psr: Display WA 0884 applied broadly for more HW tracking.
2018-03-09 0:52 [PATCH] drm/i915/psr: Display WA 0884 applied broadly for more HW tracking Rodrigo Vivi
2018-03-09 1:23 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2018-03-09 5:23 ` Patchwork
2018-03-12 18:07 ` [PATCH] " Pandiyan, Dhinakaran
2018-03-12 18:29 ` Ville Syrjälä
3 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2018-03-09 5:23 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/psr: Display WA 0884 applied broadly for more HW tracking.
URL : https://patchwork.freedesktop.org/series/39648/
State : success
== Summary ==
---- Known issues:
Test gem_eio:
Subgroup in-flight-contexts:
pass -> INCOMPLETE (shard-apl) fdo#105341
Test kms_flip:
Subgroup 2x-plain-flip-fb-recreate:
pass -> FAIL (shard-hsw) fdo#100368
Subgroup flip-vs-expired-vblank-interruptible:
pass -> FAIL (shard-hsw) fdo#102887
Test kms_sysfs_edid_timing:
pass -> WARN (shard-apl) fdo#100047
fdo#105341 https://bugs.freedesktop.org/show_bug.cgi?id=105341
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
shard-apl total:3381 pass:1779 dwarn:1 dfail:0 fail:8 skip:1591 time:11773s
shard-hsw total:3467 pass:1771 dwarn:1 dfail:0 fail:3 skip:1691 time:11651s
shard-snb total:3467 pass:1363 dwarn:1 dfail:0 fail:2 skip:2101 time:6889s
Blacklisted hosts:
shard-kbl total:3308 pass:1858 dwarn:1 dfail:0 fail:9 skip:1438 time:8648s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8281/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915/psr: Display WA 0884 applied broadly for more HW tracking.
2018-03-09 0:52 [PATCH] drm/i915/psr: Display WA 0884 applied broadly for more HW tracking Rodrigo Vivi
2018-03-09 1:23 ` ✓ Fi.CI.BAT: success for " Patchwork
2018-03-09 5:23 ` ✓ Fi.CI.IGT: " Patchwork
@ 2018-03-12 18:07 ` Pandiyan, Dhinakaran
2018-03-12 18:45 ` Rodrigo Vivi
2018-03-12 18:29 ` Ville Syrjälä
3 siblings, 1 reply; 12+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-03-12 18:07 UTC (permalink / raw)
To: Vivi, Rodrigo; +Cc: intel-gfx
On Thu, 2018-03-08 at 16:52 -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.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 | 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)
> +
> /* 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)) {
I forgot if I asked you already about this, why not
psr.has_hw_tracking?
My interpretation of the flag is - any platform that can exit PSR by
- driver writing to some pipe/plane MMIO
- HW tracking frontbuffer modifications
- enabling vblanks
The patch works well on my SKL laptop with fbcon.
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> + 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.
> + */
> + 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))
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915/psr: Display WA 0884 applied broadly for more HW tracking.
2018-03-09 0:52 [PATCH] drm/i915/psr: Display WA 0884 applied broadly for more HW tracking Rodrigo Vivi
` (2 preceding siblings ...)
2018-03-12 18:07 ` [PATCH] " Pandiyan, Dhinakaran
@ 2018-03-12 18:29 ` Ville Syrjälä
2018-03-12 18:40 ` Pandiyan, Dhinakaran
3 siblings, 1 reply; 12+ messages in thread
From: Ville Syrjälä @ 2018-03-12 18:29 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx, Dhinakaran Pandiyan
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.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 | 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?
> + 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
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915/psr: Display WA 0884 applied broadly for more HW tracking.
2018-03-12 18:29 ` Ville Syrjälä
@ 2018-03-12 18:40 ` Pandiyan, Dhinakaran
2018-03-12 18:49 ` Ville Syrjälä
2018-03-12 18:50 ` Rodrigo Vivi
0 siblings, 2 replies; 12+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-03-12 18:40 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx, Vivi, Rodrigo
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.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 | 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.
>
> > + 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915/psr: Display WA 0884 applied broadly for more HW tracking.
2018-03-12 18:07 ` [PATCH] " Pandiyan, Dhinakaran
@ 2018-03-12 18:45 ` Rodrigo Vivi
2018-03-12 18:50 ` Pandiyan, Dhinakaran
0 siblings, 1 reply; 12+ messages in thread
From: Rodrigo Vivi @ 2018-03-12 18:45 UTC (permalink / raw)
To: Pandiyan, Dhinakaran; +Cc: intel-gfx
On Mon, Mar 12, 2018 at 11:07:55AM -0700, Pandiyan, Dhinakaran wrote:
>
>
>
> On Thu, 2018-03-08 at 16:52 -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.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 | 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)
> > +
> > /* 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)) {
>
> I forgot if I asked you already about this, why not
> psr.has_hw_tracking?
because we don't have that yet ;)
I wonder if we should do this change in a separated patch
>
> My interpretation of the flag is - any platform that can exit PSR by
> - driver writing to some pipe/plane MMIO
> - HW tracking frontbuffer modifications
> - enabling vblanks
>
> The patch works well on my SKL laptop with fbcon.
>
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Thanks. Pushed to dinq.
>
>
>
>
>
> > + 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.
> > + */
> > + 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))
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915/psr: Display WA 0884 applied broadly for more HW tracking.
2018-03-12 18:40 ` Pandiyan, Dhinakaran
@ 2018-03-12 18:49 ` Ville Syrjälä
2018-03-12 19:02 ` Pandiyan, Dhinakaran
2018-03-12 18:50 ` Rodrigo Vivi
1 sibling, 1 reply; 12+ messages in thread
From: Ville Syrjälä @ 2018-03-12 18:49 UTC (permalink / raw)
To: Pandiyan, Dhinakaran; +Cc: intel-gfx, Vivi, Rodrigo
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.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 | 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()?
>
>
> >
> > > + 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
> >
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915/psr: Display WA 0884 applied broadly for more HW tracking.
2018-03-12 18:45 ` Rodrigo Vivi
@ 2018-03-12 18:50 ` Pandiyan, Dhinakaran
0 siblings, 0 replies; 12+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-03-12 18:50 UTC (permalink / raw)
To: Vivi, Rodrigo; +Cc: intel-gfx
On Mon, 2018-03-12 at 11:45 -0700, Rodrigo Vivi wrote:
> On Mon, Mar 12, 2018 at 11:07:55AM -0700, Pandiyan, Dhinakaran wrote:
> >
> >
> >
> > On Thu, 2018-03-08 at 16:52 -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.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 | 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)
> > > +
> > > /* 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)) {
> >
> > I forgot if I asked you already about this, why not
> > psr.has_hw_tracking?
>
> because we don't have that yet ;)
>
> I wonder if we should do this change in a separated patch
>
> >
> > My interpretation of the flag is - any platform that can exit PSR by
> > - driver writing to some pipe/plane MMIO
> > - HW tracking frontbuffer modifications
> > - enabling vblanks
> >
> > The patch works well on my SKL laptop with fbcon.
> >
> > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>
> Thanks. Pushed to dinq.
>
Can the frontbuffer patches
(https://patchwork.freedesktop.org/series/39502/) be pushed too? We
should have PSR usable on SKL with the frontbuffer patches on top of
this one.
> >
> >
> >
> >
> >
> > > + 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.
> > > + */
> > > + 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))
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915/psr: Display WA 0884 applied broadly for more HW tracking.
2018-03-12 18:40 ` Pandiyan, Dhinakaran
2018-03-12 18:49 ` Ville Syrjälä
@ 2018-03-12 18:50 ` Rodrigo Vivi
2018-03-12 18:58 ` Ville Syrjälä
1 sibling, 1 reply; 12+ messages in thread
From: Rodrigo Vivi @ 2018-03-12 18:50 UTC (permalink / raw)
To: Pandiyan, Dhinakaran; +Cc: intel-gfx
On Mon, Mar 12, 2018 at 11:40:26AM -0700, 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.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 | 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.
Oh! sorry... my inbox refreshed after I pushed the button :(
I tried to keep near the cursor regs. Do you want me to move in a
follow-up patch? where it would be better?
> >
> > > +
> > > /* 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.
That was a design decision Paulo, and/or Chris, and/or Daniel made on
frontbuffer tracking engine, a while ago. By definition:
flush = invalidate + flush.
Otherwise in all fbdev code we would have to have a call together
of invalidate and flush.
We probably lack on documentation there...
>
>
> >
> > > + 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915/psr: Display WA 0884 applied broadly for more HW tracking.
2018-03-12 18:50 ` Rodrigo Vivi
@ 2018-03-12 18:58 ` Ville Syrjälä
0 siblings, 0 replies; 12+ messages in thread
From: Ville Syrjälä @ 2018-03-12 18:58 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx, Pandiyan, Dhinakaran
On Mon, Mar 12, 2018 at 11:50:09AM -0700, Rodrigo Vivi wrote:
> On Mon, Mar 12, 2018 at 11:40:26AM -0700, 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.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 | 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.
>
> Oh! sorry... my inbox refreshed after I pushed the button :(
> I tried to keep near the cursor regs. Do you want me to move in a
> follow-up patch? where it would be better?
After _CUR_FBC_CTL_A/CUR_FBC_CTL() seems like the correct place based on
the offset. Also could probably use a g4x+ comment.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915/psr: Display WA 0884 applied broadly for more HW tracking.
2018-03-12 18:49 ` Ville Syrjälä
@ 2018-03-12 19:02 ` Pandiyan, Dhinakaran
0 siblings, 0 replies; 12+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-03-12 19:02 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx, Vivi, Rodrigo
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.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 | 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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-03-12 19:16 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-09 0:52 [PATCH] drm/i915/psr: Display WA 0884 applied broadly for more HW tracking Rodrigo Vivi
2018-03-09 1:23 ` ✓ Fi.CI.BAT: success for " Patchwork
2018-03-09 5:23 ` ✓ Fi.CI.IGT: " Patchwork
2018-03-12 18:07 ` [PATCH] " Pandiyan, Dhinakaran
2018-03-12 18:45 ` Rodrigo Vivi
2018-03-12 18:50 ` Pandiyan, Dhinakaran
2018-03-12 18:29 ` Ville Syrjälä
2018-03-12 18:40 ` Pandiyan, Dhinakaran
2018-03-12 18:49 ` Ville Syrjälä
2018-03-12 19:02 ` Pandiyan, Dhinakaran
2018-03-12 18:50 ` Rodrigo Vivi
2018-03-12 18:58 ` Ville Syrjälä
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.