[PATCH 1/5] drm/i915: Improve PSR activation timing

[PATCH 1/5] drm/i915: Improve PSR activation timing

[Rodrigo Vivi]

From: Andy Lutomirski <luto@kernel.org>

The current PSR code has a two call sites that each schedule delayed
work to activate PSR.  As far as I can tell, each call site intends
to keep PSR inactive for the given amount of time and then allow it
to be activated.

The call sites are:

 - intel_psr_enable(), which explicitly states in a comment that
   it's trying to keep PSR off a short time after the dispay is
   initialized as a workaround.

 - intel_psr_flush().  There isn't an explcit explanation, but the
   intent is presumably to keep PSR off until the display has been
   idle for 100ms.

The current code doesn't actually accomplish either of these goals.
Rather than keeping PSR inactive for the given amount of time, it
will schedule PSR for activation after the given time, with the
earliest target time in such a request winning.

In other words, if intel_psr_enable() is immediately followed by
intel_psr_flush(), then PSR will be activated after 100ms even if
intel_psr_enable() wanted a longer delay.  And, if the screen is
being constantly updated so that intel_psr_flush() is called once
per frame at 60Hz, PSR will still be activated once every 100ms.

Rewrite the code so that it does what was intended.  This adds
a new function intel_psr_schedule(), which will enable PSR after
the requested time but no sooner.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
Tested-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

---
 drivers/gpu/drm/i915/i915_debugfs.c |  8 +++--
 drivers/gpu/drm/i915/i915_drv.h     |  3 +-
 drivers/gpu/drm/i915/intel_psr.c    | 66 ++++++++++++++++++++++++++++++++-----
 3 files changed, 66 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 960302668649..da80ee16a3cf 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2521,8 +2521,12 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 	seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active));
 	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
 		   dev_priv->psr.busy_frontbuffer_bits);
-	seq_printf(m, "Re-enable work scheduled: %s\n",
-		   yesno(work_busy(&dev_priv->psr.work.work)));
+
+	if (timer_pending(&dev_priv->psr.activate_timer))
+		seq_printf(m, "Activate scheduled: yes, in %dms\n",
+			   jiffies_to_msecs(dev_priv->psr.activate_timer.expires - jiffies));
+	else
+		seq_printf(m, "Activate scheduled: no\n");
 
 	if (HAS_DDI(dev_priv)) {
 		if (dev_priv->psr.psr2_support)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c06d4126c447..2afa5c05a79b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -762,7 +762,8 @@ struct i915_psr {
 	bool sink_support;
 	struct intel_dp *enabled;
 	bool active;
-	struct delayed_work work;
+	struct timer_list activate_timer;
+	struct work_struct activate_work;
 	unsigned busy_frontbuffer_bits;
 	bool psr2_support;
 	bool aux_frame_sync;
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 2ef374f936b9..826b480841ac 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -450,6 +450,28 @@ static void intel_psr_activate(struct intel_dp *intel_dp)
 	dev_priv->psr.active = true;
 }
 
+static void intel_psr_schedule(struct drm_i915_private *i915,
+			       unsigned long min_wait_ms)
+{
+	unsigned long next;
+
+	lockdep_assert_held(&i915->psr.lock);
+
+	/*
+	 * We update next enable and call mod_timer() because it's
+	 * possible that intel_psr_wrk() has already been called and is
+	 * waiting for psr.lock. If that's the case, we don't want it
+	 * to immediately enable PSR.
+	 *
+	 * We also need to make sure that PSR is never activated earlier
+	 * than requested to avoid breaking intel_psr_enable()'s workaround
+	 * for pre-gen9 hardware.
+	 */
+	next = jiffies + msecs_to_jiffies(min_wait_ms);
+	if (time_after(next, i915->psr.activate_timer.expires))
+		mod_timer(&i915->psr.activate_timer, next);
+}
+
 static void hsw_psr_enable_source(struct intel_dp *intel_dp,
 				  const struct intel_crtc_state *crtc_state)
 {
@@ -534,8 +556,8 @@ void intel_psr_enable(struct intel_dp *intel_dp,
 		 *     - On HSW/BDW we get a recoverable frozen screen until
 		 *       next exit-activate sequence.
 		 */
-		schedule_delayed_work(&dev_priv->psr.work,
-				      msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5));
+		intel_psr_schedule(dev_priv,
+				   intel_dp->panel_power_cycle_delay * 5);
 	}
 
 unlock:
@@ -653,13 +675,14 @@ void intel_psr_disable(struct intel_dp *intel_dp,
 	dev_priv->psr.enabled = NULL;
 	mutex_unlock(&dev_priv->psr.lock);
 
-	cancel_delayed_work_sync(&dev_priv->psr.work);
+	del_timer_sync(&dev_priv->psr.activate_timer);
+	cancel_work_sync(&dev_priv->psr.activate_work);
 }
 
 static void intel_psr_work(struct work_struct *work)
 {
 	struct drm_i915_private *dev_priv =
-		container_of(work, typeof(*dev_priv), psr.work.work);
+		container_of(work, typeof(*dev_priv), psr.activate_work);
 	struct intel_dp *intel_dp = dev_priv->psr.enabled;
 	struct drm_crtc *crtc = dp_to_dig_port(intel_dp)->base.base.crtc;
 	enum pipe pipe = to_intel_crtc(crtc)->pipe;
@@ -705,6 +728,17 @@ static void intel_psr_work(struct work_struct *work)
 	if (!intel_dp)
 		goto unlock;
 
+	if (time_before(jiffies, dev_priv->psr.activate_timer.expires)) {
+		/*
+		 * We raced: intel_psr_schedule() tried to delay us, but
+		 * we were already in intel_psr_timer_fn() or already in
+		 * the workqueue. We can safely return -- the
+		 * intel_psr_schedule() call that put the activate_timer
+		 * into the future will also have called mod_timer().
+		 */
+		goto unlock;
+	}
+
 	/*
 	 * The delayed work can race with an invalidate hence we need to
 	 * recheck. Since psr_flush first clears this and then reschedules we
@@ -718,6 +752,20 @@ static void intel_psr_work(struct work_struct *work)
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
+static void intel_psr_timer_fn(struct timer_list *timer)
+{
+	struct drm_i915_private *i915 =
+		from_timer(i915, timer, psr.activate_timer);
+
+	/*
+	 * We need a non-atomic context to activate PSR.  Using
+	 * delayed_work wouldn't be an improvement -- delayed_work is
+	 * just the same timer that schedules work when it fires, but
+	 * there's no equivalent of mod_timer() for delayed_work.
+	 */
+	schedule_work(&i915->psr.activate_work);
+}
+
 static void intel_psr_exit(struct drm_i915_private *dev_priv)
 {
 	struct intel_dp *intel_dp = dev_priv->psr.enabled;
@@ -898,9 +946,8 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
 		intel_psr_exit(dev_priv);
 
 	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
-		if (!work_busy(&dev_priv->psr.work.work))
-			schedule_delayed_work(&dev_priv->psr.work,
-					      msecs_to_jiffies(100));
+		intel_psr_schedule(dev_priv, 100);
+
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
@@ -947,7 +994,8 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
 		dev_priv->psr.link_standby = false;
 	}
 
-	INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
+	timer_setup(&dev_priv->psr.activate_timer, intel_psr_timer_fn, 0);
+	INIT_WORK(&dev_priv->psr.activate_work, intel_psr_work);
 	mutex_init(&dev_priv->psr.lock);
 
 	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
@@ -963,4 +1011,6 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
 		dev_priv->psr.activate = hsw_psr_activate;
 		dev_priv->psr.setup_vsc = hsw_psr_setup_vsc;
 	}
+
+	dev_priv->psr.activate_timer.expires = jiffies - 1;
 }
-- 
2.13.6

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

[PATCH 2/5] drm/i915/psr: Kill scheduled work for Core platforms.

[Rodrigo Vivi]

It is a fact that scheduled work is now improved.

But it is also a fact that on core platforms that shouldn't
be needed. We only need to actually wait on VLV/CHV.

The immediate enabling is actually not an issue for the
HW perspective for core platforms that have HW tracking.
HW will wait few identical idle frames before transitioning
to actual psr active anyways.

Note that this patch also remove the delayed activation
on HSW and BDW introduced by commit 'd0ac896a477d
("drm/i915: Delay first PSR activation.")'. This was
introduced to fix a blank screen on VLV/CHV and also
masked some frozen screens on other core platforms.
Probably the same that we are now properly hunting and fixing.

Furthermore, if we stop using delayed activation on core
platforms we will be able, on following up patches,
to use available workarounds to make HW tracking properly
exit PSR instead of the big nuke of disabling psr and
re-enable on exit and activate respectively.
At least on few reliable cases.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++-------
 drivers/gpu/drm/i915/intel_psr.c    | 27 +++++++++++++++++++--------
 2 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index da80ee16a3cf..541290c307c7 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2522,18 +2522,18 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
 		   dev_priv->psr.busy_frontbuffer_bits);
 
-	if (timer_pending(&dev_priv->psr.activate_timer))
-		seq_printf(m, "Activate scheduled: yes, in %dms\n",
-			   jiffies_to_msecs(dev_priv->psr.activate_timer.expires - jiffies));
-	else
-		seq_printf(m, "Activate scheduled: no\n");
-
-	if (HAS_DDI(dev_priv)) {
+	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {
 		if (dev_priv->psr.psr2_support)
 			enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
 		else
 			enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
 	} else {
+		if (timer_pending(&dev_priv->psr.activate_timer))
+			seq_printf(m, "Activate scheduled: yes, in %dms\n",
+				   jiffies_to_msecs(dev_priv->psr.activate_timer.expires - jiffies));
+		else
+			seq_printf(m, "Activate scheduled: no\n");
+
 		for_each_pipe(dev_priv, pipe) {
 			enum transcoder cpu_transcoder =
 				intel_pipe_to_cpu_transcoder(dev_priv, pipe);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 826b480841ac..13409c6301e8 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -455,6 +455,8 @@ static void intel_psr_schedule(struct drm_i915_private *i915,
 {
 	unsigned long next;
 
+	WARN_ON(!IS_VALLEYVIEW(i915) && !IS_CHERRYVIEW(i915));
+
 	lockdep_assert_held(&i915->psr.lock);
 
 	/*
@@ -543,7 +545,7 @@ void intel_psr_enable(struct intel_dp *intel_dp,
 	dev_priv->psr.enable_source(intel_dp, crtc_state);
 	dev_priv->psr.enabled = intel_dp;
 
-	if (INTEL_GEN(dev_priv) >= 9) {
+	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {
 		intel_psr_activate(intel_dp);
 	} else {
 		/*
@@ -553,8 +555,6 @@ void intel_psr_enable(struct intel_dp *intel_dp,
 		 * However on some platforms we face issues when first
 		 * activation follows a modeset so quickly.
 		 *     - On VLV/CHV we get bank screen on first activation
-		 *     - On HSW/BDW we get a recoverable frozen screen until
-		 *       next exit-activate sequence.
 		 */
 		intel_psr_schedule(dev_priv,
 				   intel_dp->panel_power_cycle_delay * 5);
@@ -687,6 +687,8 @@ static void intel_psr_work(struct work_struct *work)
 	struct drm_crtc *crtc = dp_to_dig_port(intel_dp)->base.base.crtc;
 	enum pipe pipe = to_intel_crtc(crtc)->pipe;
 
+	WARN_ON(!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv));
+
 	/* We have to make sure PSR is ready for re-enable
 	 * otherwise it keeps disabled until next full enable/disable cycle.
 	 * PSR might take some time to get fully disabled
@@ -757,6 +759,8 @@ static void intel_psr_timer_fn(struct timer_list *timer)
 	struct drm_i915_private *i915 =
 		from_timer(i915, timer, psr.activate_timer);
 
+	WARN_ON(!IS_VALLEYVIEW(i915) && !IS_CHERRYVIEW(i915));
+
 	/*
 	 * We need a non-atomic context to activate PSR.  Using
 	 * delayed_work wouldn't be an improvement -- delayed_work is
@@ -945,9 +949,12 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
 	if (frontbuffer_bits)
 		intel_psr_exit(dev_priv);
 
-	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
-		intel_psr_schedule(dev_priv, 100);
-
+	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits) {
+		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+			intel_psr_schedule(dev_priv, 100);
+		else
+			intel_psr_activate(dev_priv->psr.enabled);
+	}
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
@@ -994,8 +1001,12 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
 		dev_priv->psr.link_standby = false;
 	}
 
-	timer_setup(&dev_priv->psr.activate_timer, intel_psr_timer_fn, 0);
-	INIT_WORK(&dev_priv->psr.activate_work, intel_psr_work);
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
+		timer_setup(&dev_priv->psr.activate_timer,
+			    intel_psr_timer_fn, 0);
+		INIT_WORK(&dev_priv->psr.activate_work, intel_psr_work);
+	}
+
 	mutex_init(&dev_priv->psr.lock);
 
 	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
-- 
2.13.6

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

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

[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.

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)
+
 /* 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);
+		}
+	}
 
 	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits) {
 		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
-- 
2.13.6

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

[PATCH 4/5] drm/i915/psr: Display WA #1110

[Rodrigo Vivi]

Missing flips when FBC is enabled with PSR
link off/PSR2 deep sleep scenarios.

WA: When FBC is enabled with PSR/PSR2,
set bit 30 of MMIO register 0x420CC to 1b.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  | 1 +
 drivers/gpu/drm/i915/intel_psr.c | 8 +++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index ac09d17cd835..0f423cd52983 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7153,6 +7153,7 @@ enum {
 #define CHICKEN_TRANS_A         0x420c0
 #define CHICKEN_TRANS_B         0x420c4
 #define CHICKEN_TRANS(trans) _MMIO_TRANS(trans, CHICKEN_TRANS_A, CHICKEN_TRANS_B)
+#define  DDI_MASK_INTERRUPTS_PSR	(1<<30)
 #define  DDI_TRAINING_OVERRIDE_ENABLE	(1<<19)
 #define  DDI_TRAINING_OVERRIDE_VALUE	(1<<18)
 #define  DDIE_TRAINING_OVERRIDE_ENABLE	(1<<17) /* CHICKEN_TRANS_A only */
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 49554036ffb8..43c702b70519 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -481,7 +481,7 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
 	struct drm_device *dev = dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
-	u32 chicken;
+	u32 chicken = I915_READ(CHICKEN_TRANS(cpu_transcoder));
 
 	if (dev_priv->psr.psr2_support) {
 		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
@@ -508,6 +508,12 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
 			   EDP_PSR_DEBUG_MASK_HPD |
 			   EDP_PSR_DEBUG_MASK_LPSP);
 	}
+
+	/* Display WA #1110: skl,kbl,cfl,bxt */
+	if (IS_GEN9_BC(dev_priv) || IS_BROXTON(dev_priv)) {
+		chicken |= DDI_MASK_INTERRUPTS_PSR;
+		I915_WRITE(CHICKEN_TRANS(cpu_transcoder), chicken);
+	}
 }
 
 /**
-- 
2.13.6

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

[PATCH 5/5] drm/i915/psr: Display WA #1130: bxt, glk

[Rodrigo Vivi]

Host/Render modifications do not trigger PSR exit
or Wireless quick capture exit correctly.

WA: Set MMIO register 0x4653C bit 31 = 1b.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 1 +
 drivers/gpu/drm/i915/intel_pm.c | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0f423cd52983..8a4cd8b4bd7c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3924,6 +3924,7 @@ enum {
 #define   PWM1_GATING_DIS		(1 << 13)
 
 #define GEN9_CLKGATE_DIS_4		_MMIO(0x4653C)
+#define   BXT_DCIPH_GATING_DIS		(1 << 31)
 #define   BXT_GMBUS_GATING_DIS		(1 << 14)
 
 #define _CLKGATE_DIS_PSL_A		0x46520
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 7e15b261821d..a0a6b4b7c47b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -114,6 +114,10 @@ static void bxt_init_clock_gating(struct drm_i915_private *dev_priv)
 	 */
 	I915_WRITE(GEN9_CLKGATE_DIS_0, I915_READ(GEN9_CLKGATE_DIS_0) |
 		   PWM1_GATING_DIS | PWM2_GATING_DIS);
+
+	/* Display WA #1130:bxt */
+	I915_WRITE(GEN9_CLKGATE_DIS_4, I915_READ(GEN9_CLKGATE_DIS_4) |
+		   BXT_DCIPH_GATING_DIS);
 }
 
 static void glk_init_clock_gating(struct drm_i915_private *dev_priv)
@@ -137,6 +141,9 @@ static void glk_init_clock_gating(struct drm_i915_private *dev_priv)
 		I915_WRITE(CHICKEN_MISC_2, val);
 	}
 
+	/* Display WA #1130:glk */
+	I915_WRITE(GEN9_CLKGATE_DIS_4, I915_READ(GEN9_CLKGATE_DIS_4) |
+		   BXT_DCIPH_GATING_DIS);
 }
 
 static void i915_pineview_get_mem_freq(struct drm_i915_private *dev_priv)
-- 
2.13.6

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

✗ Fi.CI.BAT: failure for series starting with [1/5] drm/i915: Improve PSR activation timing

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/i915: Improve PSR activation timing
URL   : https://patchwork.freedesktop.org/series/38199/
State : failure

== Summary ==

Series 38199v1 series starting with [1/5] drm/i915: Improve PSR activation timing
https://patchwork.freedesktop.org/api/1.0/series/38199/revisions/1/mbox/

Test gem_ctx_switch:
        Subgroup basic-default-heavy:
                pass       -> INCOMPLETE (fi-cnl-y3)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                pass       -> INCOMPLETE (fi-bxt-dsi)
Test kms_psr_sink_crc:
        Subgroup psr_basic:
                pass       -> DMESG-WARN (fi-skl-6600u)
                pass       -> DMESG-WARN (fi-skl-6700hq) fdo#101144
                pass       -> DMESG-WARN (fi-kbl-7560u)
                pass       -> DMESG-WARN (fi-kbl-r)
                pass       -> DMESG-WARN (fi-glk-1)
                pass       -> DMESG-WARN (fi-cfl-s2)

fdo#101144 https://bugs.freedesktop.org/show_bug.cgi?id=101144

fi-bdw-5557u     total:288  pass:265  dwarn:0   dfail:0   fail:2   skip:21  time:438s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:430s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:376s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:494s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:290s
fi-bxt-dsi       total:246  pass:219  dwarn:0   dfail:0   fail:0   skip:26 
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:489s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:473s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:462s
fi-cfl-s2        total:288  pass:261  dwarn:1   dfail:0   fail:0   skip:26  time:574s
fi-cnl-y3        total:22   pass:21   dwarn:0   dfail:0   fail:0   skip:0  
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:424s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:285s
fi-glk-1         total:288  pass:259  dwarn:1   dfail:0   fail:0   skip:28  time:511s
fi-hsw-4770      total:288  pass:259  dwarn:0   dfail:0   fail:2   skip:27  time:413s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:413s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:465s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:416s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:458s
fi-kbl-7560u     total:288  pass:268  dwarn:1   dfail:0   fail:0   skip:19  time:499s
fi-kbl-r         total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:499s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:591s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:430s
fi-skl-6600u     total:288  pass:260  dwarn:1   dfail:0   fail:0   skip:27  time:508s
fi-skl-6700hq    total:288  pass:261  dwarn:1   dfail:0   fail:0   skip:26  time:526s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:495s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:485s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:421s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:432s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:527s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:398s
Blacklisted hosts:
fi-glk-dsi       total:288  pass:181  dwarn:1   dfail:4   fail:0   skip:102 time:769s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:451s

55dfdd18a566b5c632cbb34086b2a03410e810fb drm-tip: 2018y-02m-13d-18h-37m-11s UTC integration manifest
fa066f8ebd80 drm/i915/psr: Display WA #1130: bxt, glk
9fea4988f6f6 drm/i915/psr: Display WA #1110
68bcf8babd26 drm/i915/psr: Display WA 0884 applied broadly for more HW tracking.
51fd4ce038af drm/i915/psr: Kill scheduled work for Core platforms.
4175f54e54b7 drm/i915: Improve PSR activation timing

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8001/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: Improve PSR activation timing

[Pandiyan, Dhinakaran]

On Tue, 2018-02-13 at 15:26 -0800, Rodrigo Vivi wrote:
> From: Andy Lutomirski <luto@kernel.org>
> 
> The current PSR code has a two call sites that each schedule delayed
> work to activate PSR.  As far as I can tell, each call site intends
> to keep PSR inactive for the given amount of time and then allow it
> to be activated.
> 
> The call sites are:
> 
>  - intel_psr_enable(), which explicitly states in a comment that
>    it's trying to keep PSR off a short time after the dispay is
>    initialized as a workaround.
> 
>  - intel_psr_flush().  There isn't an explcit explanation, but the
>    intent is presumably to keep PSR off until the display has been
>    idle for 100ms.
> 
> The current code doesn't actually accomplish either of these goals.
> Rather than keeping PSR inactive for the given amount of time, it
> will schedule PSR for activation after the given time, with the
> earliest target time in such a request winning.
> 
> In other words, if intel_psr_enable() is immediately followed by
> intel_psr_flush(), then PSR will be activated after 100ms even if
> intel_psr_enable() wanted a longer delay.  And, if the screen is
> being constantly updated so that intel_psr_flush() is called once
> per frame at 60Hz, PSR will still be activated once every 100ms.
> 
> Rewrite the code so that it does what was intended.  This adds
> a new function intel_psr_schedule(), which will enable PSR after
> the requested time but no sooner.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> Tested-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  8 +++--
>  drivers/gpu/drm/i915/i915_drv.h     |  3 +-
>  drivers/gpu/drm/i915/intel_psr.c    | 66 ++++++++++++++++++++++++++++++++-----
>  3 files changed, 66 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 960302668649..da80ee16a3cf 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2521,8 +2521,12 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>  	seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active));
>  	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
>  		   dev_priv->psr.busy_frontbuffer_bits);
> -	seq_printf(m, "Re-enable work scheduled: %s\n",
> -		   yesno(work_busy(&dev_priv->psr.work.work)));
> +
> +	if (timer_pending(&dev_priv->psr.activate_timer))
> +		seq_printf(m, "Activate scheduled: yes, in %dms\n",
> +			   jiffies_to_msecs(dev_priv->psr.activate_timer.expires - jiffies));
> +	else
> +		seq_printf(m, "Activate scheduled: no\n");
>  
>  	if (HAS_DDI(dev_priv)) {
>  		if (dev_priv->psr.psr2_support)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c06d4126c447..2afa5c05a79b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -762,7 +762,8 @@ struct i915_psr {
>  	bool sink_support;
>  	struct intel_dp *enabled;
>  	bool active;
> -	struct delayed_work work;
> +	struct timer_list activate_timer;
> +	struct work_struct activate_work;
>  	unsigned busy_frontbuffer_bits;
>  	bool psr2_support;
>  	bool aux_frame_sync;
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 2ef374f936b9..826b480841ac 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -450,6 +450,28 @@ static void intel_psr_activate(struct intel_dp *intel_dp)
>  	dev_priv->psr.active = true;
>  }
>  
> +static void intel_psr_schedule(struct drm_i915_private *i915,
> +			       unsigned long min_wait_ms)
> +{
> +	unsigned long next;
> +
> +	lockdep_assert_held(&i915->psr.lock);
> +
> +	/*
> +	 * We update next enable and call mod_timer() because it's
> +	 * possible that intel_psr_wrk() has already been called and is
> +	 * waiting for psr.lock. If that's the case, we don't want it
> +	 * to immediately enable PSR.
> +	 *
> +	 * We also need to make sure that PSR is never activated earlier
> +	 * than requested to avoid breaking intel_psr_enable()'s workaround
> +	 * for pre-gen9 hardware.
> +	 */
> +	next = jiffies + msecs_to_jiffies(min_wait_ms);
> +	if (time_after(next, i915->psr.activate_timer.expires))

.expires is an internal member, does not seem like a good idea to read
it outside of the exported interfaces.


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

Re: [PATCH 1/5] drm/i915: Improve PSR activation timing

[Rodrigo Vivi]

"Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> writes:

> On Tue, 2018-02-13 at 15:26 -0800, Rodrigo Vivi wrote:
>> From: Andy Lutomirski <luto@kernel.org>
>> 
>> The current PSR code has a two call sites that each schedule delayed
>> work to activate PSR.  As far as I can tell, each call site intends
>> to keep PSR inactive for the given amount of time and then allow it
>> to be activated.
>> 
>> The call sites are:
>> 
>>  - intel_psr_enable(), which explicitly states in a comment that
>>    it's trying to keep PSR off a short time after the dispay is
>>    initialized as a workaround.
>> 
>>  - intel_psr_flush().  There isn't an explcit explanation, but the
>>    intent is presumably to keep PSR off until the display has been
>>    idle for 100ms.
>> 
>> The current code doesn't actually accomplish either of these goals.
>> Rather than keeping PSR inactive for the given amount of time, it
>> will schedule PSR for activation after the given time, with the
>> earliest target time in such a request winning.
>> 
>> In other words, if intel_psr_enable() is immediately followed by
>> intel_psr_flush(), then PSR will be activated after 100ms even if
>> intel_psr_enable() wanted a longer delay.  And, if the screen is
>> being constantly updated so that intel_psr_flush() is called once
>> per frame at 60Hz, PSR will still be activated once every 100ms.
>> 
>> Rewrite the code so that it does what was intended.  This adds
>> a new function intel_psr_schedule(), which will enable PSR after
>> the requested time but no sooner.
>> 
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> Tested-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> 
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c |  8 +++--
>>  drivers/gpu/drm/i915/i915_drv.h     |  3 +-
>>  drivers/gpu/drm/i915/intel_psr.c    | 66 ++++++++++++++++++++++++++++++++-----
>>  3 files changed, 66 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 960302668649..da80ee16a3cf 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -2521,8 +2521,12 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>>  	seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active));
>>  	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
>>  		   dev_priv->psr.busy_frontbuffer_bits);
>> -	seq_printf(m, "Re-enable work scheduled: %s\n",
>> -		   yesno(work_busy(&dev_priv->psr.work.work)));
>> +
>> +	if (timer_pending(&dev_priv->psr.activate_timer))
>> +		seq_printf(m, "Activate scheduled: yes, in %dms\n",
>> +			   jiffies_to_msecs(dev_priv->psr.activate_timer.expires - jiffies));
>> +	else
>> +		seq_printf(m, "Activate scheduled: no\n");
>>  
>>  	if (HAS_DDI(dev_priv)) {
>>  		if (dev_priv->psr.psr2_support)
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index c06d4126c447..2afa5c05a79b 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -762,7 +762,8 @@ struct i915_psr {
>>  	bool sink_support;
>>  	struct intel_dp *enabled;
>>  	bool active;
>> -	struct delayed_work work;
>> +	struct timer_list activate_timer;
>> +	struct work_struct activate_work;
>>  	unsigned busy_frontbuffer_bits;
>>  	bool psr2_support;
>>  	bool aux_frame_sync;
>> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
>> index 2ef374f936b9..826b480841ac 100644
>> --- a/drivers/gpu/drm/i915/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/intel_psr.c
>> @@ -450,6 +450,28 @@ static void intel_psr_activate(struct intel_dp *intel_dp)
>>  	dev_priv->psr.active = true;
>>  }
>>  
>> +static void intel_psr_schedule(struct drm_i915_private *i915,
>> +			       unsigned long min_wait_ms)
>> +{
>> +	unsigned long next;
>> +
>> +	lockdep_assert_held(&i915->psr.lock);
>> +
>> +	/*
>> +	 * We update next enable and call mod_timer() because it's
>> +	 * possible that intel_psr_wrk() has already been called and is
>> +	 * waiting for psr.lock. If that's the case, we don't want it
>> +	 * to immediately enable PSR.
>> +	 *
>> +	 * We also need to make sure that PSR is never activated earlier
>> +	 * than requested to avoid breaking intel_psr_enable()'s workaround
>> +	 * for pre-gen9 hardware.
>> +	 */
>> +	next = jiffies + msecs_to_jiffies(min_wait_ms);
>> +	if (time_after(next, i915->psr.activate_timer.expires))
>
> .expires is an internal member, does not seem like a good idea to read
> it outside of the exported interfaces.

Chris I believe this question is for you.

I just ignored for a while because I thought it was for Andy,
but now I saw that you modified the original patch on exactly this point.

Btw I believe this is a modification that should had been clear
highlighted when you sent and with your Signed-off-by.

So, could you please explain and give the sign-off here?
Or should we rebase original Andy's version without this change?

Thanks,
Rodrigo.

>
>
> -DK
> _______________________________________________
> 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: [PATCH 2/5] drm/i915/psr: Kill scheduled work for Core platforms.

[Pandiyan, Dhinakaran]

On Tue, 2018-02-13 at 15:26 -0800, Rodrigo Vivi wrote:
> It is a fact that scheduled work is now improved.
> 
> But it is also a fact that on core platforms that shouldn't
> be needed. We only need to actually wait on VLV/CHV.
> 
> The immediate enabling is actually not an issue for the
> HW perspective for core platforms that have HW tracking.
> HW will wait few identical idle frames before transitioning
> to actual psr active anyways.
> 
> Note that this patch also remove the delayed activation
> on HSW and BDW introduced by commit 'd0ac896a477d
> ("drm/i915: Delay first PSR activation.")'. This was
> introduced to fix a blank screen on VLV/CHV and also
> masked some frozen screens on other core platforms.
> Probably the same that we are now properly hunting and fixing.
> 
> Furthermore, if we stop using delayed activation on core
> platforms we will be able, on following up patches,
> to use available workarounds to make HW tracking properly
> exit PSR instead of the big nuke of disabling psr and
> re-enable on exit and activate respectively.
> At least on few reliable cases.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++-------
>  drivers/gpu/drm/i915/intel_psr.c    | 27 +++++++++++++++++++--------
>  2 files changed, 26 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index da80ee16a3cf..541290c307c7 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2522,18 +2522,18 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>  	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
>  		   dev_priv->psr.busy_frontbuffer_bits);
>  
> -	if (timer_pending(&dev_priv->psr.activate_timer))
> -		seq_printf(m, "Activate scheduled: yes, in %dms\n",
> -			   jiffies_to_msecs(dev_priv->psr.activate_timer.expires - jiffies));
> -	else
> -		seq_printf(m, "Activate scheduled: no\n");
> -
> -	if (HAS_DDI(dev_priv)) {
> +	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {

I don't get this change, it is better to retain HAS_DDI().


>  		if (dev_priv->psr.psr2_support)
>  			enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
>  		else
>  			enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
>  	} else {
> +		if (timer_pending(&dev_priv->psr.activate_timer))
> +			seq_printf(m, "Activate scheduled: yes, in %dms\n",
> +				   jiffies_to_msecs(dev_priv->psr.activate_timer.expires - jiffies));
> +		else
> +			seq_printf(m, "Activate scheduled: no\n");
> +
>  		for_each_pipe(dev_priv, pipe) {
>  			enum transcoder cpu_transcoder =
>  				intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 826b480841ac..13409c6301e8 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -455,6 +455,8 @@ static void intel_psr_schedule(struct drm_i915_private *i915,
>  {
>  	unsigned long next;
>  
> +	WARN_ON(!IS_VALLEYVIEW(i915) && !IS_CHERRYVIEW(i915));
> +
How about using only !(IS_VLV() || IS_CHV) in this file.

I think this is a reasonable check to have, please add a return too.
	WARN_ON(!(IS_VLV() || IS_CHV())
		return;	

>  	lockdep_assert_held(&i915->psr.lock);
>  
>  	/*
> @@ -543,7 +545,7 @@ void intel_psr_enable(struct intel_dp *intel_dp,
>  	dev_priv->psr.enable_source(intel_dp, crtc_state);
>  	dev_priv->psr.enabled = intel_dp;
>  
> -	if (INTEL_GEN(dev_priv) >= 9) {
> +	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {

How about inverting this? 

if (IS_VLV() || IS_CHV())
	intel_psr_schedule()
else 
	intel_psr_activate()

is easier to follow IMO.


What is the reason to not use HAS_DDI() ?

>  		intel_psr_activate(intel_dp);
>  	} else {
>  		/*
> @@ -553,8 +555,6 @@ void intel_psr_enable(struct intel_dp *intel_dp,
>  		 * However on some platforms we face issues when first
>  		 * activation follows a modeset so quickly.
>  		 *     - On VLV/CHV we get bank screen on first activation
> -		 *     - On HSW/BDW we get a recoverable frozen screen until
> -		 *       next exit-activate sequence.
>  		 */
>  		intel_psr_schedule(dev_priv,
>  				   intel_dp->panel_power_cycle_delay * 5);
> @@ -687,6 +687,8 @@ static void intel_psr_work(struct work_struct *work)
>  	struct drm_crtc *crtc = dp_to_dig_port(intel_dp)->base.base.crtc;
>  	enum pipe pipe = to_intel_crtc(crtc)->pipe;
>  
> +	WARN_ON(!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv));
> +

This is not needed, we don't even setup the work function for VLV/CHV.
Since the functions are all contained in this one file, I don't see much
risk of somehow ending up here.

>  	/* We have to make sure PSR is ready for re-enable
>  	 * otherwise it keeps disabled until next full enable/disable cycle.
>  	 * PSR might take some time to get fully disabled
> @@ -757,6 +759,8 @@ static void intel_psr_timer_fn(struct timer_list *timer)
>  	struct drm_i915_private *i915 =
>  		from_timer(i915, timer, psr.activate_timer);
>  
> +	WARN_ON(!IS_VALLEYVIEW(i915) && !IS_CHERRYVIEW(i915));
> +

This is not needed too.

>  	/*
>  	 * We need a non-atomic context to activate PSR.  Using
>  	 * delayed_work wouldn't be an improvement -- delayed_work is
> @@ -945,9 +949,12 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
>  	if (frontbuffer_bits)
>  		intel_psr_exit(dev_priv);
>  
> -	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
> -		intel_psr_schedule(dev_priv, 100);
> -
> +	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits) {
> +		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> +			intel_psr_schedule(dev_priv, 100);
> +		else
> +			intel_psr_activate(dev_priv->psr.enabled);
> +	}
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
>  
> @@ -994,8 +1001,12 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
>  		dev_priv->psr.link_standby = false;
>  	}
>  
> -	timer_setup(&dev_priv->psr.activate_timer, intel_psr_timer_fn, 0);
> -	INIT_WORK(&dev_priv->psr.activate_work, intel_psr_work);
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> +		timer_setup(&dev_priv->psr.activate_timer,
> +			    intel_psr_timer_fn, 0);
> +		INIT_WORK(&dev_priv->psr.activate_work, intel_psr_work);
> +	}
> +
>  	mutex_init(&dev_priv->psr.lock);
>  
>  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: Improve PSR activation timing

[Andy Lutomirski]

On Tue, Feb 13, 2018 at 11:26 PM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> From: Andy Lutomirski <luto@kernel.org>
>
> +
> +       dev_priv->psr.activate_timer.expires = jiffies - 1;

That can't possibly be okay.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Pandiyan, Dhinakaran]

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.

> 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?


> +		}
> +	}
>  
>  	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

Re: [PATCH 4/5] drm/i915/psr: Display WA #1110

[Pandiyan, Dhinakaran]

On Tue, 2018-02-13 at 15:26 -0800, Rodrigo Vivi wrote:
> Missing flips when FBC is enabled with PSR
> link off/PSR2 deep sleep scenarios.
> 

I am wary of this. Is there a test to compare flip misses before/after
this workaround? We also have to confirm disabling FBC has the same
effect of not having this workaround.


+Ville
+Chris

any idea on how to quantitatively test if this workaround improves
anything?





> WA: When FBC is enabled with PSR/PSR2,
> set bit 30 of MMIO register 0x420CC to 1b.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  | 1 +
>  drivers/gpu/drm/i915/intel_psr.c | 8 +++++++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index ac09d17cd835..0f423cd52983 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7153,6 +7153,7 @@ enum {
>  #define CHICKEN_TRANS_A         0x420c0
>  #define CHICKEN_TRANS_B         0x420c4
>  #define CHICKEN_TRANS(trans) _MMIO_TRANS(trans, CHICKEN_TRANS_A, CHICKEN_TRANS_B)
> +#define  DDI_MASK_INTERRUPTS_PSR	(1<<30)
>  #define  DDI_TRAINING_OVERRIDE_ENABLE	(1<<19)
>  #define  DDI_TRAINING_OVERRIDE_VALUE	(1<<18)
>  #define  DDIE_TRAINING_OVERRIDE_ENABLE	(1<<17) /* CHICKEN_TRANS_A only */
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 49554036ffb8..43c702b70519 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -481,7 +481,7 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
>  	struct drm_device *dev = dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> -	u32 chicken;
> +	u32 chicken = I915_READ(CHICKEN_TRANS(cpu_transcoder));
>  
>  	if (dev_priv->psr.psr2_support) {
>  		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> @@ -508,6 +508,12 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp,
>  			   EDP_PSR_DEBUG_MASK_HPD |
>  			   EDP_PSR_DEBUG_MASK_LPSP);
>  	}
> +
> +	/* Display WA #1110: skl,kbl,cfl,bxt */
> +	if (IS_GEN9_BC(dev_priv) || IS_BROXTON(dev_priv)) {
> +		chicken |= DDI_MASK_INTERRUPTS_PSR;
> +		I915_WRITE(CHICKEN_TRANS(cpu_transcoder), chicken);
> +	}
>  }
>  
>  /**
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915/psr: Display WA #1130: bxt, glk

[Pandiyan, Dhinakaran]

On Tue, 2018-02-13 at 15:26 -0800, Rodrigo Vivi wrote:
> Host/Render modifications do not trigger PSR exit
> or Wireless quick capture exit correctly.
> 

I don't get this workaround either. The wording indicates frontbuffer
modifications are expected to trigger PSR exit in HW. But we rely on the
driver's frontbuffer tracking to do that for us.



> WA: Set MMIO register 0x4653C bit 31 = 1b.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 1 +
>  drivers/gpu/drm/i915/intel_pm.c | 7 +++++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0f423cd52983..8a4cd8b4bd7c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3924,6 +3924,7 @@ enum {
>  #define   PWM1_GATING_DIS		(1 << 13)
>  
>  #define GEN9_CLKGATE_DIS_4		_MMIO(0x4653C)
> +#define   BXT_DCIPH_GATING_DIS		(1 << 31)
>  #define   BXT_GMBUS_GATING_DIS		(1 << 14)
>  
>  #define _CLKGATE_DIS_PSL_A		0x46520
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 7e15b261821d..a0a6b4b7c47b 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -114,6 +114,10 @@ static void bxt_init_clock_gating(struct drm_i915_private *dev_priv)
>  	 */
>  	I915_WRITE(GEN9_CLKGATE_DIS_0, I915_READ(GEN9_CLKGATE_DIS_0) |
>  		   PWM1_GATING_DIS | PWM2_GATING_DIS);
> +
> +	/* Display WA #1130:bxt */
> +	I915_WRITE(GEN9_CLKGATE_DIS_4, I915_READ(GEN9_CLKGATE_DIS_4) |
> +		   BXT_DCIPH_GATING_DIS);
>  }
>  
>  static void glk_init_clock_gating(struct drm_i915_private *dev_priv)
> @@ -137,6 +141,9 @@ static void glk_init_clock_gating(struct drm_i915_private *dev_priv)
>  		I915_WRITE(CHICKEN_MISC_2, val);
>  	}
>  
> +	/* Display WA #1130:glk */
> +	I915_WRITE(GEN9_CLKGATE_DIS_4, I915_READ(GEN9_CLKGATE_DIS_4) |
> +		   BXT_DCIPH_GATING_DIS);
>  }
>  
>  static void i915_pineview_get_mem_freq(struct drm_i915_private *dev_priv)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915/psr: Display WA #1110

[Pandiyan, Dhinakaran]

On Fri, 2018-02-23 at 16:59 -0800, Dhinakaran Pandiyan wrote:
> On Tue, 2018-02-13 at 15:26 -0800, Rodrigo Vivi wrote:
> > Missing flips when FBC is enabled with PSR
> > link off/PSR2 deep sleep scenarios.
> > 
> 
> I am wary of this. Is there a test to compare flip misses before/after
> this workaround? We also have to confirm disabling FBC has the same
> effect of not having this workaround.
           *having
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915/psr: Display WA #1130: bxt, glk

[Rodrigo Vivi]

On Fri, Feb 23, 2018 at 04:40:38PM -0800, Pandiyan, Dhinakaran wrote:
> On Tue, 2018-02-13 at 15:26 -0800, Rodrigo Vivi wrote:
> > Host/Render modifications do not trigger PSR exit
> > or Wireless quick capture exit correctly.
> > 
> 
> I don't get this workaround either. The wording indicates frontbuffer
> modifications are expected to trigger PSR exit in HW. But we rely on the
> driver's frontbuffer tracking to do that for us.

With us moving more towards more HW tracking I believe it is good
to have hw tracking Wa related in place just in case.

> 
> 
> 
> > WA: Set MMIO register 0x4653C bit 31 = 1b.
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h | 1 +
> >  drivers/gpu/drm/i915/intel_pm.c | 7 +++++++
> >  2 files changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 0f423cd52983..8a4cd8b4bd7c 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3924,6 +3924,7 @@ enum {
> >  #define   PWM1_GATING_DIS		(1 << 13)
> >  
> >  #define GEN9_CLKGATE_DIS_4		_MMIO(0x4653C)
> > +#define   BXT_DCIPH_GATING_DIS		(1 << 31)
> >  #define   BXT_GMBUS_GATING_DIS		(1 << 14)
> >  
> >  #define _CLKGATE_DIS_PSL_A		0x46520
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 7e15b261821d..a0a6b4b7c47b 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -114,6 +114,10 @@ static void bxt_init_clock_gating(struct drm_i915_private *dev_priv)
> >  	 */
> >  	I915_WRITE(GEN9_CLKGATE_DIS_0, I915_READ(GEN9_CLKGATE_DIS_0) |
> >  		   PWM1_GATING_DIS | PWM2_GATING_DIS);
> > +
> > +	/* Display WA #1130:bxt */
> > +	I915_WRITE(GEN9_CLKGATE_DIS_4, I915_READ(GEN9_CLKGATE_DIS_4) |
> > +		   BXT_DCIPH_GATING_DIS);
> >  }
> >  
> >  static void glk_init_clock_gating(struct drm_i915_private *dev_priv)
> > @@ -137,6 +141,9 @@ static void glk_init_clock_gating(struct drm_i915_private *dev_priv)
> >  		I915_WRITE(CHICKEN_MISC_2, val);
> >  	}
> >  
> > +	/* Display WA #1130:glk */
> > +	I915_WRITE(GEN9_CLKGATE_DIS_4, I915_READ(GEN9_CLKGATE_DIS_4) |
> > +		   BXT_DCIPH_GATING_DIS);
> >  }
> >  
> >  static void i915_pineview_get_mem_freq(struct drm_i915_private *dev_priv)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Rodrigo Vivi]

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

Re: [PATCH 2/5] drm/i915/psr: Kill scheduled work for Core platforms.

[Rodrigo Vivi]

On Fri, Feb 23, 2018 at 03:46:09PM -0800, Pandiyan, Dhinakaran wrote:
> On Tue, 2018-02-13 at 15:26 -0800, Rodrigo Vivi wrote:
> > It is a fact that scheduled work is now improved.
> > 
> > But it is also a fact that on core platforms that shouldn't
> > be needed. We only need to actually wait on VLV/CHV.
> > 
> > The immediate enabling is actually not an issue for the
> > HW perspective for core platforms that have HW tracking.
> > HW will wait few identical idle frames before transitioning
> > to actual psr active anyways.
> > 
> > Note that this patch also remove the delayed activation
> > on HSW and BDW introduced by commit 'd0ac896a477d
> > ("drm/i915: Delay first PSR activation.")'. This was
> > introduced to fix a blank screen on VLV/CHV and also
> > masked some frozen screens on other core platforms.
> > Probably the same that we are now properly hunting and fixing.
> > 
> > Furthermore, if we stop using delayed activation on core
> > platforms we will be able, on following up patches,
> > to use available workarounds to make HW tracking properly
> > exit PSR instead of the big nuke of disabling psr and
> > re-enable on exit and activate respectively.
> > At least on few reliable cases.
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++-------
> >  drivers/gpu/drm/i915/intel_psr.c    | 27 +++++++++++++++++++--------
> >  2 files changed, 26 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index da80ee16a3cf..541290c307c7 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2522,18 +2522,18 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
> >  	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
> >  		   dev_priv->psr.busy_frontbuffer_bits);
> >  
> > -	if (timer_pending(&dev_priv->psr.activate_timer))
> > -		seq_printf(m, "Activate scheduled: yes, in %dms\n",
> > -			   jiffies_to_msecs(dev_priv->psr.activate_timer.expires - jiffies));
> > -	else
> > -		seq_printf(m, "Activate scheduled: no\n");
> > -
> > -	if (HAS_DDI(dev_priv)) {
> > +	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {
> 
> I don't get this change, it is better to retain HAS_DDI().
> 
> 
> >  		if (dev_priv->psr.psr2_support)
> >  			enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
> >  		else
> >  			enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
> >  	} else {
> > +		if (timer_pending(&dev_priv->psr.activate_timer))
> > +			seq_printf(m, "Activate scheduled: yes, in %dms\n",
> > +				   jiffies_to_msecs(dev_priv->psr.activate_timer.expires - jiffies));
> > +		else
> > +			seq_printf(m, "Activate scheduled: no\n");
> > +
> >  		for_each_pipe(dev_priv, pipe) {
> >  			enum transcoder cpu_transcoder =
> >  				intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 826b480841ac..13409c6301e8 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -455,6 +455,8 @@ static void intel_psr_schedule(struct drm_i915_private *i915,
> >  {
> >  	unsigned long next;
> >  
> > +	WARN_ON(!IS_VALLEYVIEW(i915) && !IS_CHERRYVIEW(i915));
> > +
> How about using only !(IS_VLV() || IS_CHV) in this file.
> 
> I think this is a reasonable check to have, please add a return too.
> 	WARN_ON(!(IS_VLV() || IS_CHV())
> 		return;	
> 
> >  	lockdep_assert_held(&i915->psr.lock);
> >  
> >  	/*
> > @@ -543,7 +545,7 @@ void intel_psr_enable(struct intel_dp *intel_dp,
> >  	dev_priv->psr.enable_source(intel_dp, crtc_state);
> >  	dev_priv->psr.enabled = intel_dp;
> >  
> > -	if (INTEL_GEN(dev_priv) >= 9) {
> > +	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {
> 
> How about inverting this? 
> 
> if (IS_VLV() || IS_CHV())
> 	intel_psr_schedule()
> else 
> 	intel_psr_activate()
> 
> is easier to follow IMO.
> 
>
> What is the reason to not use HAS_DDI() ?

I believe HAS_DDI is not meaningful here. It is just a coincidence.

maybe we could simplify everything with has_hw_tracking.... but also
a coincidence in other cases...

maybe create something meaninfull like VLV_PSR... :/

no strong feelings actually...

> 
> >  		intel_psr_activate(intel_dp);
> >  	} else {
> >  		/*
> > @@ -553,8 +555,6 @@ void intel_psr_enable(struct intel_dp *intel_dp,
> >  		 * However on some platforms we face issues when first
> >  		 * activation follows a modeset so quickly.
> >  		 *     - On VLV/CHV we get bank screen on first activation
> > -		 *     - On HSW/BDW we get a recoverable frozen screen until
> > -		 *       next exit-activate sequence.
> >  		 */
> >  		intel_psr_schedule(dev_priv,
> >  				   intel_dp->panel_power_cycle_delay * 5);
> > @@ -687,6 +687,8 @@ static void intel_psr_work(struct work_struct *work)
> >  	struct drm_crtc *crtc = dp_to_dig_port(intel_dp)->base.base.crtc;
> >  	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> >  
> > +	WARN_ON(!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv));
> > +
> 
> This is not needed, we don't even setup the work function for VLV/CHV.
> Since the functions are all contained in this one file, I don't see much
> risk of somehow ending up here.
> 
> >  	/* We have to make sure PSR is ready for re-enable
> >  	 * otherwise it keeps disabled until next full enable/disable cycle.
> >  	 * PSR might take some time to get fully disabled
> > @@ -757,6 +759,8 @@ static void intel_psr_timer_fn(struct timer_list *timer)
> >  	struct drm_i915_private *i915 =
> >  		from_timer(i915, timer, psr.activate_timer);
> >  
> > +	WARN_ON(!IS_VALLEYVIEW(i915) && !IS_CHERRYVIEW(i915));
> > +
> 
> This is not needed too.
> 
> >  	/*
> >  	 * We need a non-atomic context to activate PSR.  Using
> >  	 * delayed_work wouldn't be an improvement -- delayed_work is
> > @@ -945,9 +949,12 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
> >  	if (frontbuffer_bits)
> >  		intel_psr_exit(dev_priv);
> >  
> > -	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
> > -		intel_psr_schedule(dev_priv, 100);
> > -
> > +	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits) {
> > +		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > +			intel_psr_schedule(dev_priv, 100);
> > +		else
> > +			intel_psr_activate(dev_priv->psr.enabled);
> > +	}
> >  	mutex_unlock(&dev_priv->psr.lock);
> >  }
> >  
> > @@ -994,8 +1001,12 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
> >  		dev_priv->psr.link_standby = false;
> >  	}
> >  
> > -	timer_setup(&dev_priv->psr.activate_timer, intel_psr_timer_fn, 0);
> > -	INIT_WORK(&dev_priv->psr.activate_work, intel_psr_work);
> > +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > +		timer_setup(&dev_priv->psr.activate_timer,
> > +			    intel_psr_timer_fn, 0);
> > +		INIT_WORK(&dev_priv->psr.activate_work, intel_psr_work);
> > +	}
> > +
> >  	mutex_init(&dev_priv->psr.lock);
> >  
> >  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Pandiyan, Dhinakaran]

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 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...
> 

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 <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

Re: [PATCH 2/5] drm/i915/psr: Kill scheduled work for Core platforms.

[Pandiyan, Dhinakaran]

On Mon, 2018-02-26 at 15:12 -0800, Rodrigo Vivi wrote:
> On Fri, Feb 23, 2018 at 03:46:09PM -0800, Pandiyan, Dhinakaran wrote:
> > On Tue, 2018-02-13 at 15:26 -0800, Rodrigo Vivi wrote:
> > > It is a fact that scheduled work is now improved.
> > > 
> > > But it is also a fact that on core platforms that shouldn't
> > > be needed. We only need to actually wait on VLV/CHV.
> > > 
> > > The immediate enabling is actually not an issue for the
> > > HW perspective for core platforms that have HW tracking.
> > > HW will wait few identical idle frames before transitioning
> > > to actual psr active anyways.
> > > 
> > > Note that this patch also remove the delayed activation
> > > on HSW and BDW introduced by commit 'd0ac896a477d
> > > ("drm/i915: Delay first PSR activation.")'. This was
> > > introduced to fix a blank screen on VLV/CHV and also
> > > masked some frozen screens on other core platforms.
> > > Probably the same that we are now properly hunting and fixing.
> > > 
> > > Furthermore, if we stop using delayed activation on core
> > > platforms we will be able, on following up patches,
> > > to use available workarounds to make HW tracking properly
> > > exit PSR instead of the big nuke of disabling psr and
> > > re-enable on exit and activate respectively.
> > > At least on few reliable cases.
> > > 
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++-------
> > >  drivers/gpu/drm/i915/intel_psr.c    | 27 +++++++++++++++++++--------
> > >  2 files changed, 26 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index da80ee16a3cf..541290c307c7 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -2522,18 +2522,18 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
> > >  	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
> > >  		   dev_priv->psr.busy_frontbuffer_bits);
> > >  
> > > -	if (timer_pending(&dev_priv->psr.activate_timer))
> > > -		seq_printf(m, "Activate scheduled: yes, in %dms\n",
> > > -			   jiffies_to_msecs(dev_priv->psr.activate_timer.expires - jiffies));
> > > -	else
> > > -		seq_printf(m, "Activate scheduled: no\n");
> > > -
> > > -	if (HAS_DDI(dev_priv)) {
> > > +	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {
> > 
> > I don't get this change, it is better to retain HAS_DDI().
> > 
> > 
> > >  		if (dev_priv->psr.psr2_support)
> > >  			enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
> > >  		else
> > >  			enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
> > >  	} else {
> > > +		if (timer_pending(&dev_priv->psr.activate_timer))
> > > +			seq_printf(m, "Activate scheduled: yes, in %dms\n",
> > > +				   jiffies_to_msecs(dev_priv->psr.activate_timer.expires - jiffies));
> > > +		else
> > > +			seq_printf(m, "Activate scheduled: no\n");
> > > +
> > >  		for_each_pipe(dev_priv, pipe) {
> > >  			enum transcoder cpu_transcoder =
> > >  				intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > index 826b480841ac..13409c6301e8 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -455,6 +455,8 @@ static void intel_psr_schedule(struct drm_i915_private *i915,
> > >  {
> > >  	unsigned long next;
> > >  
> > > +	WARN_ON(!IS_VALLEYVIEW(i915) && !IS_CHERRYVIEW(i915));
> > > +
> > How about using only !(IS_VLV() || IS_CHV) in this file.
> > 
> > I think this is a reasonable check to have, please add a return too.
> > 	WARN_ON(!(IS_VLV() || IS_CHV())
> > 		return;	
> > 
> > >  	lockdep_assert_held(&i915->psr.lock);
> > >  
> > >  	/*
> > > @@ -543,7 +545,7 @@ void intel_psr_enable(struct intel_dp *intel_dp,
> > >  	dev_priv->psr.enable_source(intel_dp, crtc_state);
> > >  	dev_priv->psr.enabled = intel_dp;
> > >  
> > > -	if (INTEL_GEN(dev_priv) >= 9) {
> > > +	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {
> > 
> > How about inverting this? 
> > 
> > if (IS_VLV() || IS_CHV())
> > 	intel_psr_schedule()
> > else 
> > 	intel_psr_activate()
> > 
> > is easier to follow IMO.
> > 
> >
> > What is the reason to not use HAS_DDI() ?
> 
> I believe HAS_DDI is not meaningful here. It is just a coincidence.
> 
> maybe we could simplify everything with has_hw_tracking.... but also
> a coincidence in other cases...
> 
> maybe create something meaninfull like VLV_PSR... :/
> 
> no strong feelings actually...
> 

Thanks for the clarification, IS_VLV() || IS_CHV() is good enough in
that case. Since you also have a comment explaining that a blank screen
is seen if we activate PSR immediately on VLV/CHV, let's go ahead with
the inverted 'if'.



> > 
> > >  		intel_psr_activate(intel_dp);
> > >  	} else {
> > >  		/*
> > > @@ -553,8 +555,6 @@ void intel_psr_enable(struct intel_dp *intel_dp,
> > >  		 * However on some platforms we face issues when first
> > >  		 * activation follows a modeset so quickly.
> > >  		 *     - On VLV/CHV we get bank screen on first activation
> > > -		 *     - On HSW/BDW we get a recoverable frozen screen until
> > > -		 *       next exit-activate sequence.
> > >  		 */
> > >  		intel_psr_schedule(dev_priv,
> > >  				   intel_dp->panel_power_cycle_delay * 5);
> > > @@ -687,6 +687,8 @@ static void intel_psr_work(struct work_struct *work)
> > >  	struct drm_crtc *crtc = dp_to_dig_port(intel_dp)->base.base.crtc;
> > >  	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> > >  
> > > +	WARN_ON(!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv));
> > > +
> > 
> > This is not needed, we don't even setup the work function for VLV/CHV.
> > Since the functions are all contained in this one file, I don't see much
> > risk of somehow ending up here.
> > 
> > >  	/* We have to make sure PSR is ready for re-enable
> > >  	 * otherwise it keeps disabled until next full enable/disable cycle.
> > >  	 * PSR might take some time to get fully disabled
> > > @@ -757,6 +759,8 @@ static void intel_psr_timer_fn(struct timer_list *timer)
> > >  	struct drm_i915_private *i915 =
> > >  		from_timer(i915, timer, psr.activate_timer);
> > >  
> > > +	WARN_ON(!IS_VALLEYVIEW(i915) && !IS_CHERRYVIEW(i915));
> > > +
> > 
> > This is not needed too.
> > 
> > >  	/*
> > >  	 * We need a non-atomic context to activate PSR.  Using
> > >  	 * delayed_work wouldn't be an improvement -- delayed_work is
> > > @@ -945,9 +949,12 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
> > >  	if (frontbuffer_bits)
> > >  		intel_psr_exit(dev_priv);
> > >  
> > > -	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
> > > -		intel_psr_schedule(dev_priv, 100);
> > > -
> > > +	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits) {
> > > +		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > +			intel_psr_schedule(dev_priv, 100);
> > > +		else
> > > +			intel_psr_activate(dev_priv->psr.enabled);
> > > +	}
> > >  	mutex_unlock(&dev_priv->psr.lock);
> > >  }
> > >  
> > > @@ -994,8 +1001,12 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
> > >  		dev_priv->psr.link_standby = false;
> > >  	}
> > >  
> > > -	timer_setup(&dev_priv->psr.activate_timer, intel_psr_timer_fn, 0);
> > > -	INIT_WORK(&dev_priv->psr.activate_work, intel_psr_work);
> > > +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > > +		timer_setup(&dev_priv->psr.activate_timer,
> > > +			    intel_psr_timer_fn, 0);
> > > +		INIT_WORK(&dev_priv->psr.activate_work, intel_psr_work);
> > > +	}
> > > +
> > >  	mutex_init(&dev_priv->psr.lock);
> > >  
> > >  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Rodrigo Vivi]

On Sat, Feb 24, 2018 at 12:24:35AM +0000, 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.
> 
> > 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?

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: [PATCH 1/5] drm/i915: Improve PSR activation timing

[Chris Wilson]

Quoting Andy Lutomirski (2018-02-24 00:07:23)
> On Tue, Feb 13, 2018 at 11:26 PM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > From: Andy Lutomirski <luto@kernel.org>
> >
> > +
> > +       dev_priv->psr.activate_timer.expires = jiffies - 1;
> 
> That can't possibly be okay.

As an initialisation value, set to the previous jiffie? You can set it
to the current jiffie, but then you have the issue of not noticing the
update to the current jiffie.

So how is this any more incorrect?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: Improve PSR activation timing

[Andy Lutomirski]

On Wed, Feb 28, 2018 at 12:26 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting Andy Lutomirski (2018-02-24 00:07:23)
>> On Tue, Feb 13, 2018 at 11:26 PM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
>> > From: Andy Lutomirski <luto@kernel.org>
>> >
>> > +
>> > +       dev_priv->psr.activate_timer.expires = jiffies - 1;
>>
>> That can't possibly be okay.
>
> As an initialisation value, set to the previous jiffie? You can set it
> to the current jiffie, but then you have the issue of not noticing the
> update to the current jiffie.
>
> So how is this any more incorrect?

I don't think you can just write to fields in struct timer_list like that.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx