[PATCH] drm/i915: Improve PSR activation timing

[PATCH] drm/i915: Improve PSR activation timing

[Andy Lutomirski]

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>
---
 drivers/gpu/drm/i915/i915_debugfs.c |  9 +++--
 drivers/gpu/drm/i915/i915_drv.h     |  4 ++-
 drivers/gpu/drm/i915/intel_psr.c    | 69 ++++++++++++++++++++++++++++++++-----
 3 files changed, 71 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index c65e381b85f3..b67db93f905d 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2663,8 +2663,13 @@ 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 %ldms\n",
+			   (long)(dev_priv->psr.earliest_activate - jiffies) *
+			   1000 / HZ);
+	else
+		seq_printf(m, "Re-enable 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 46eb729b367d..c0fb7d65cda6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1192,7 +1192,9 @@ struct i915_psr {
 	bool source_ok;
 	struct intel_dp *enabled;
 	bool active;
-	struct delayed_work work;
+	struct timer_list activate_timer;
+	struct work_struct activate_work;
+	unsigned long earliest_activate;
 	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 55ea5eb3b7df..333d90d4e5af 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -461,6 +461,30 @@ static void intel_psr_activate(struct intel_dp *intel_dp)
 	dev_priv->psr.active = true;
 }
 
+static void intel_psr_schedule(struct drm_i915_private *dev_priv,
+			       unsigned long min_wait_ms)
+{
+	unsigned long next;
+
+	lockdep_assert_held(&dev_priv->psr.lock);
+
+	/*
+	 * We update next_enable *and* call mod_timer() because it's
+	 * possible that intel_psr_work() 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, dev_priv->psr.earliest_activate)) {
+		dev_priv->psr.earliest_activate = next;
+		mod_timer(&dev_priv->psr.activate_timer, next);
+	}
+}
+
 static void hsw_psr_enable_source(struct intel_dp *intel_dp,
 				  const struct intel_crtc_state *crtc_state)
 {
@@ -544,8 +568,7 @@ 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:
@@ -660,13 +683,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);
+	cancel_work_sync(&dev_priv->psr.activate_work);
+	del_timer_sync(&dev_priv->psr.activate_timer);
 }
 
 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;
@@ -676,6 +700,7 @@ static void intel_psr_work(struct work_struct *work)
 	 * PSR might take some time to get fully disabled
 	 * and be ready for re-enable.
 	 */
+
 	if (HAS_DDI(dev_priv)) {
 		if (dev_priv->psr.psr2_support) {
 			if (intel_wait_for_register(dev_priv,
@@ -712,6 +737,18 @@ static void intel_psr_work(struct work_struct *work)
 	if (!intel_dp)
 		goto unlock;
 
+
+	if (!time_after_eq(jiffies, dev_priv->psr.earliest_activate)) {
+		/*
+		 * 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 earliest_activeate
+		 * in the future will 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
@@ -725,6 +762,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 *dev_priv =
+		container_of(timer, typeof(*dev_priv), psr.activate_timer);
+
+	/*
+	 * We need a non-atomic context to activate PSR.  Using
+	 * delayed_work wouldn't be an improvement -- delayed_work is
+	 * just a timer that schedules work when it fires, but there's
+	 * no equivalent of mod_timer() for delayed_work.
+	 */
+	schedule_work(&dev_priv->psr.activate_work);
+}
+
 static void intel_psr_exit(struct drm_i915_private *dev_priv)
 {
 	struct intel_dp *intel_dp = dev_priv->psr.enabled;
@@ -905,9 +956,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);
 }
 
@@ -951,7 +1001,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)) {
@@ -967,4 +1018,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.earliest_activate = jiffies;
 }
-- 
2.14.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✗ Fi.CI.BAT: failure for drm/i915: Improve PSR activation timing

[Patchwork]

== Series Details ==

Series: drm/i915: Improve PSR activation timing
URL   : https://patchwork.freedesktop.org/series/37693/
State : failure

== Summary ==

Applying: drm/i915: Improve PSR activation timing
error: sha1 information is lacking or useless (drivers/gpu/drm/i915/i915_drv.h).
error: could not build fake ancestor
Patch failed at 0001 drm/i915: Improve PSR activation timing
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

[PATCH] drm/i915: Improve PSR activation timing

[Chris Wilson]

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

Rebase into drm-tip.
-Chris

---
 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 2bdce9fea671..e0de912a5ec9 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2528,8 +2528,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 7db3557b945c..26b9fb0eda40 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 e9feffdea899..087c0726aa8a 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.16.1

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

✗ Fi.CI.BAT: warning for drm/i915: Improve PSR activation timing (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Improve PSR activation timing (rev2)
URL   : https://patchwork.freedesktop.org/series/37693/
State : warning

== Summary ==

Series 37693v2 drm/i915: Improve PSR activation timing
https://patchwork.freedesktop.org/api/1.0/series/37693/revisions/2/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                incomplete -> PASS       (fi-snb-2520m) fdo#103713
Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                pass       -> FAIL       (fi-gdg-551) fdo#102575
Test drv_module_reload:
        Subgroup basic-reload-inject:
                pass       -> DMESG-WARN (fi-bdw-5557u)

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

fi-bdw-5557u     total:288  pass:266  dwarn:1   dfail:0   fail:0   skip:21  time:418s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:427s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:375s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:487s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:286s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:482s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:483s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:466s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:456s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:562s
fi-cnl-y3        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:581s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:415s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:285s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:509s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:388s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:410s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:466s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:422s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:458s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:500s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:452s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:504s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:589s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:433s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:510s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:528s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:486s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:475s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:414s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:426s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:516s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:400s
Blacklisted hosts:
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:469s

ede4b1ac40b2c32511532ddb6849b11c0183020e drm-tip: 2018y-02m-08d-07h-43m-06s UTC integration manifest
ea396041101b drm/i915: Improve PSR activation timing

== Logs ==

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

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

[Rodrigo Vivi]

Hi Andy,

thanks for getting involved with PSR and sorry for not replying sooner.

I first saw this patch on that bugzilla entry but only now I stop to
really think why I have written the code that way.

So some clarity below.

On Mon, Feb 05, 2018 at 10:07:09PM +0000, Andy Lutomirski wrote:
> 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.

First of all I really want to kill this call here and remove the
FIXME. It was an ugly hack that I added to solve a corner case
that was leaving me with blank screens when activating so sooner.

>
>  - 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 reason for 100 is kind of ugly-nonsense-empirical value
I concluded from VLV/CHV experience.
On platforms with HW tracking HW waits few identical frames
until really activating PSR. VLV/CHV activation is immediate.
But HW is also different and there it seemed that hw needed a
few more time before starting the transitions.
Furthermore I didn't want to add that so quickly because I didn't
want to take the risk of killing battery with software tracking
when doing transitions so quickly using software tracking.

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

Putting that way I was asking myself how that hack had ever fixed
my issue. Because the way you explained here seems obvious that it
wouldn't ever fix my bug or any other.

So I applied your patch and it made even more sense (without considering
the fact I want to kill the first call anyways).

So I came back, removed your patch and tried to understand how did
it ever worked.

So, the thing is that intel_psr_flush will never be really executed
if intel_psr_enable wasn't executed. That is guaranteed by:

mutex_lock(&dev_priv->psr.lock);
	if (!dev_priv->psr.enabled) {

So, intel_psr_enable will be for sure the first one to schedule the
work delayed to the ugly higher delay.

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

During this time you are right, many calls of intel_psr_exit
coming from flush functions can be called... But none of
them will schedule the work with 100 delay.

they will skip on
if (!work_busy(&dev_priv->psr.work.work))

So, the higher delayed *hack* will be respected and PSR won't get
activated before that.

On the other hand you might ask what if,
for some strange reason,
(intel_dp->panel_power_cycle_delay * 5) is lesser than 100.
Well, on this case this would be ok, because it happens only
once and only on gen > 9 where hw tracking will wait the minimal
number of frames before the actual transition to PSR.

In either cases I believe we are safe.

Thanks,
Rodrigo.

>
> 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>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  9 +++--
>  drivers/gpu/drm/i915/i915_drv.h     |  4 ++-
>  drivers/gpu/drm/i915/intel_psr.c    | 69 ++++++++++++++++++++++++++++++++-----
>  3 files changed, 71 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index c65e381b85f3..b67db93f905d 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2663,8 +2663,13 @@ 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 %ldms\n",
> +			   (long)(dev_priv->psr.earliest_activate - jiffies) *
> +			   1000 / HZ);
> +	else
> +		seq_printf(m, "Re-enable 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 46eb729b367d..c0fb7d65cda6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1192,7 +1192,9 @@ struct i915_psr {
>  	bool source_ok;
>  	struct intel_dp *enabled;
>  	bool active;
> -	struct delayed_work work;
> +	struct timer_list activate_timer;
> +	struct work_struct activate_work;
> +	unsigned long earliest_activate;
>  	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 55ea5eb3b7df..333d90d4e5af 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -461,6 +461,30 @@ static void intel_psr_activate(struct intel_dp *intel_dp)
>  	dev_priv->psr.active = true;
>  }
>
> +static void intel_psr_schedule(struct drm_i915_private *dev_priv,
> +			       unsigned long min_wait_ms)
> +{
> +	unsigned long next;
> +
> +	lockdep_assert_held(&dev_priv->psr.lock);
> +
> +	/*
> +	 * We update next_enable *and* call mod_timer() because it's
> +	 * possible that intel_psr_work() 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, dev_priv->psr.earliest_activate)) {
> +		dev_priv->psr.earliest_activate = next;
> +		mod_timer(&dev_priv->psr.activate_timer, next);
> +	}
> +}
> +
>  static void hsw_psr_enable_source(struct intel_dp *intel_dp,
>  				  const struct intel_crtc_state *crtc_state)
>  {
> @@ -544,8 +568,7 @@ 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:
> @@ -660,13 +683,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);
> +	cancel_work_sync(&dev_priv->psr.activate_work);
> +	del_timer_sync(&dev_priv->psr.activate_timer);
>  }
>
>  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;
> @@ -676,6 +700,7 @@ static void intel_psr_work(struct work_struct *work)
>  	 * PSR might take some time to get fully disabled
>  	 * and be ready for re-enable.
>  	 */
> +
>  	if (HAS_DDI(dev_priv)) {
>  		if (dev_priv->psr.psr2_support) {
>  			if (intel_wait_for_register(dev_priv,
> @@ -712,6 +737,18 @@ static void intel_psr_work(struct work_struct *work)
>  	if (!intel_dp)
>  		goto unlock;
>
> +
> +	if (!time_after_eq(jiffies, dev_priv->psr.earliest_activate)) {
> +		/*
> +		 * 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 earliest_activeate
> +		 * in the future will 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
> @@ -725,6 +762,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 *dev_priv =
> +		container_of(timer, typeof(*dev_priv), psr.activate_timer);
> +
> +	/*
> +	 * We need a non-atomic context to activate PSR.  Using
> +	 * delayed_work wouldn't be an improvement -- delayed_work is
> +	 * just a timer that schedules work when it fires, but there's
> +	 * no equivalent of mod_timer() for delayed_work.
> +	 */
> +	schedule_work(&dev_priv->psr.activate_work);
> +}
> +
>  static void intel_psr_exit(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_dp *intel_dp = dev_priv->psr.enabled;
> @@ -905,9 +956,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);
>  }
>
> @@ -951,7 +1001,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)) {
> @@ -967,4 +1018,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.earliest_activate = jiffies;
>  }
> --
> 2.14.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Improve PSR activation timing

[Pandiyan, Dhinakaran]

On Thu, 2018-02-08 at 14:48 -0800, Rodrigo Vivi wrote:
> Hi Andy,
> 
> thanks for getting involved with PSR and sorry for not replying sooner.
> 
> I first saw this patch on that bugzilla entry but only now I stop to
> really think why I have written the code that way.
> 
> So some clarity below.
> 
> On Mon, Feb 05, 2018 at 10:07:09PM +0000, Andy Lutomirski wrote:
> > 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.
> 
> First of all I really want to kill this call here and remove the
> FIXME. It was an ugly hack that I added to solve a corner case
> that was leaving me with blank screens when activating so sooner.
> 
> >
> >  - 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 reason for 100 is kind of ugly-nonsense-empirical value
> I concluded from VLV/CHV experience.
> On platforms with HW tracking HW waits few identical frames
> until really activating PSR. VLV/CHV activation is immediate.
> But HW is also different and there it seemed that hw needed a
> few more time before starting the transitions.
> Furthermore I didn't want to add that so quickly because I didn't
> want to take the risk of killing battery with software tracking
> when doing transitions so quickly using software tracking.
> 
> >
> > 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.
> 
> Putting that way I was asking myself how that hack had ever fixed
> my issue. Because the way you explained here seems obvious that it
> wouldn't ever fix my bug or any other.
> 
> So I applied your patch and it made even more sense (without considering
> the fact I want to kill the first call anyways).
> 
> So I came back, removed your patch and tried to understand how did
> it ever worked.
> 
> So, the thing is that intel_psr_flush will never be really executed
> if intel_psr_enable wasn't executed. That is guaranteed by:
> 
> mutex_lock(&dev_priv->psr.lock);
> 	if (!dev_priv->psr.enabled) {
> 
> So, intel_psr_enable will be for sure the first one to schedule the
> work delayed to the ugly higher delay.
> 
> >
> > 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.
> 
> During this time you are right, many calls of intel_psr_exit
> coming from flush functions can be called... But none of
> them will schedule the work with 100 delay.
> 
> they will skip on
> if (!work_busy(&dev_priv->psr.work.work))

Wouldn't work_busy() return false until the work is actually queued
which is 100ms after calling schedule_delayed_work()?

For e.g, flushes at 0, 16, 32...96 will have work_busy() returning false
until 100ms.

The first psr_work will end up getting scheduled at 100ms, which I
believe is not what we want. 


However, I think 

	if (dev_priv->psr.busy_frontbuffer_bits)
		goto unlock;

	intel_psr_activate(intel_dp);

in psr_work might prevent activate being called at 100ms if an
invalidate happened to be called before that.




> 
> So, the higher delayed *hack* will be respected and PSR won't get
> activated before that.
> 
> On the other hand you might ask what if,
> for some strange reason,
> (intel_dp->panel_power_cycle_delay * 5) is lesser than 100.
> Well, on this case this would be ok, because it happens only
> once and only on gen > 9 where hw tracking will wait the minimal
> number of frames before the actual transition to PSR.
> 
> In either cases I believe we are safe.
> 
> Thanks,
> Rodrigo.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Andy Lutomirski]

> On Feb 8, 2018, at 4:39 PM, Pandiyan, Dhinakaran <dhinakaran.pandiyan@intel.com> wrote:
> 
> 
>> On Thu, 2018-02-08 at 14:48 -0800, Rodrigo Vivi wrote:
>> Hi Andy,
>> 
>> thanks for getting involved with PSR and sorry for not replying sooner.
>> 
>> I first saw this patch on that bugzilla entry but only now I stop to
>> really think why I have written the code that way.
>> 
>> So some clarity below.
>> 
>>> On Mon, Feb 05, 2018 at 10:07:09PM +0000, Andy Lutomirski wrote:
>>> 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.
>> 
>> First of all I really want to kill this call here and remove the
>> FIXME. It was an ugly hack that I added to solve a corner case
>> that was leaving me with blank screens when activating so sooner.
>> 
>>> 
>>> - 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 reason for 100 is kind of ugly-nonsense-empirical value
>> I concluded from VLV/CHV experience.
>> On platforms with HW tracking HW waits few identical frames
>> until really activating PSR. VLV/CHV activation is immediate.
>> But HW is also different and there it seemed that hw needed a
>> few more time before starting the transitions.
>> Furthermore I didn't want to add that so quickly because I didn't
>> want to take the risk of killing battery with software tracking
>> when doing transitions so quickly using software tracking.
>> 
>>> 
>>> 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.
>> 
>> Putting that way I was asking myself how that hack had ever fixed
>> my issue. Because the way you explained here seems obvious that it
>> wouldn't ever fix my bug or any other.
>> 
>> So I applied your patch and it made even more sense (without considering
>> the fact I want to kill the first call anyways).
>> 
>> So I came back, removed your patch and tried to understand how did
>> it ever worked.
>> 
>> So, the thing is that intel_psr_flush will never be really executed
>> if intel_psr_enable wasn't executed. That is guaranteed by:
>> 
>> mutex_lock(&dev_priv->psr.lock);
>>    if (!dev_priv->psr.enabled) {
>> 
>> So, intel_psr_enable will be for sure the first one to schedule the
>> work delayed to the ugly higher delay.
>> 
>>> 
>>> 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.
>> 
>> During this time you are right, many calls of intel_psr_exit
>> coming from flush functions can be called... But none of
>> them will schedule the work with 100 delay.
>> 
>> they will skip on
>> if (!work_busy(&dev_priv->psr.work.work))

As below, the first call will.  Then, 100ms later, the work will fire.  Then the next flush will schedule it again, etc.

> 
> Wouldn't work_busy() return false until the work is actually queued
> which is 100ms after calling schedule_delayed_work()?
> 
> For e.g, flushes at 0, 16, 32...96 will have work_busy() returning false
> until 100ms.
> 
> The first psr_work will end up getting scheduled at 100ms, which I
> believe is not what we want. 

Indeed.  I stuck some printks in and this seems to be what happens.

> 
> 
> However, I think 
> 
>    if (dev_priv->psr.busy_frontbuffer_bits)
>        goto unlock;
> 
>    intel_psr_activate(intel_dp);
> 
> in psr_work might prevent activate being called at 100ms if an
> invalidate happened to be called before that.
> 

On my system, invalidate is never called.  Even if it were called, that check would only help if we got lucky and the work fired after invalidate and before the subsequent flush.

> 
> 
> 
>> 
>> So, the higher delayed *hack* will be respected and PSR won't get
>> activated before that.
>> 
>> On the other hand you might ask what if,
>> for some strange reason,
>> (intel_dp->panel_power_cycle_delay * 5) is lesser than 100.
>> Well, on this case this would be ok, because it happens only
>> once and only on gen > 9 where hw tracking will wait the minimal
>> number of frames before the actual transition to PSR.
>> 
>> In either cases I believe we are safe.
>> 
>> Thanks,
>> Rodrigo.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Pandiyan, Dhinakaran]

On Thu, 2018-02-08 at 17:01 -0800, Andy Lutomirski wrote:
> 
> 
> > On Feb 8, 2018, at 4:39 PM, Pandiyan, Dhinakaran <dhinakaran.pandiyan@intel.com> wrote:
> > 
> > 
> >> On Thu, 2018-02-08 at 14:48 -0800, Rodrigo Vivi wrote:
> >> Hi Andy,
> >> 
> >> thanks for getting involved with PSR and sorry for not replying sooner.
> >> 
> >> I first saw this patch on that bugzilla entry but only now I stop to
> >> really think why I have written the code that way.
> >> 
> >> So some clarity below.
> >> 
> >>> On Mon, Feb 05, 2018 at 10:07:09PM +0000, Andy Lutomirski wrote:
> >>> 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.
> >> 
> >> First of all I really want to kill this call here and remove the
> >> FIXME. It was an ugly hack that I added to solve a corner case
> >> that was leaving me with blank screens when activating so sooner.
> >> 
> >>> 
> >>> - 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 reason for 100 is kind of ugly-nonsense-empirical value
> >> I concluded from VLV/CHV experience.
> >> On platforms with HW tracking HW waits few identical frames
> >> until really activating PSR. VLV/CHV activation is immediate.
> >> But HW is also different and there it seemed that hw needed a
> >> few more time before starting the transitions.
> >> Furthermore I didn't want to add that so quickly because I didn't
> >> want to take the risk of killing battery with software tracking
> >> when doing transitions so quickly using software tracking.
> >> 
> >>> 
> >>> 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.
> >> 
> >> Putting that way I was asking myself how that hack had ever fixed
> >> my issue. Because the way you explained here seems obvious that it
> >> wouldn't ever fix my bug or any other.
> >> 
> >> So I applied your patch and it made even more sense (without considering
> >> the fact I want to kill the first call anyways).
> >> 
> >> So I came back, removed your patch and tried to understand how did
> >> it ever worked.
> >> 
> >> So, the thing is that intel_psr_flush will never be really executed
> >> if intel_psr_enable wasn't executed. That is guaranteed by:
> >> 
> >> mutex_lock(&dev_priv->psr.lock);
> >>    if (!dev_priv->psr.enabled) {
> >> 
> >> So, intel_psr_enable will be for sure the first one to schedule the
> >> work delayed to the ugly higher delay.
> >> 
> >>> 
> >>> 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.
> >> 
> >> During this time you are right, many calls of intel_psr_exit
> >> coming from flush functions can be called... But none of
> >> them will schedule the work with 100 delay.
> >> 
> >> they will skip on
> >> if (!work_busy(&dev_priv->psr.work.work))
> 
> As below, the first call will.  Then, 100ms later, the work will fire.  Then the next flush will schedule it again, etc.
> 
> > 
> > Wouldn't work_busy() return false until the work is actually queued
> > which is 100ms after calling schedule_delayed_work()?
> > 
> > For e.g, flushes at 0, 16, 32...96 will have work_busy() returning false
> > until 100ms.
> > 
> > The first psr_work will end up getting scheduled at 100ms, which I
> > believe is not what we want. 
> 
> Indeed.  I stuck some printks in and this seems to be what happens.
> 
> > 
> > 
> > However, I think 
> > 
> >    if (dev_priv->psr.busy_frontbuffer_bits)
> >        goto unlock;
> > 
> >    intel_psr_activate(intel_dp);
> > 
> > in psr_work might prevent activate being called at 100ms if an
> > invalidate happened to be called before that.
> > 
> 
> On my system, invalidate is never called.
I noticed the same in console mode with fbdev, there are no invalidates.

>   Even if it were called, that check would only help if we got lucky and the work fired after invalidate and before the subsequent flush.
> 
Agreed. This dependency on invalidate sure looks wrong. 

> > 
> > 
> > 
> >> 
> >> So, the higher delayed *hack* will be respected and PSR won't get
> >> activated before that.
> >> 
> >> On the other hand you might ask what if,
> >> for some strange reason,
> >> (intel_dp->panel_power_cycle_delay * 5) is lesser than 100.
> >> Well, on this case this would be ok, because it happens only
> >> once and only on gen > 9 where hw tracking will wait the minimal
> >> number of frames before the actual transition to PSR.
> >> 
> >> In either cases I believe we are safe.
> >> 
> >> Thanks,
> >> Rodrigo.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Rodrigo Vivi]

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

> On Thu, 2018-02-08 at 14:48 -0800, Rodrigo Vivi wrote:
>> Hi Andy,
>> 
>> thanks for getting involved with PSR and sorry for not replying sooner.
>> 
>> I first saw this patch on that bugzilla entry but only now I stop to
>> really think why I have written the code that way.
>> 
>> So some clarity below.
>> 
>> On Mon, Feb 05, 2018 at 10:07:09PM +0000, Andy Lutomirski wrote:
>> > 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.
>> 
>> First of all I really want to kill this call here and remove the
>> FIXME. It was an ugly hack that I added to solve a corner case
>> that was leaving me with blank screens when activating so sooner.
>> 
>> >
>> >  - 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 reason for 100 is kind of ugly-nonsense-empirical value
>> I concluded from VLV/CHV experience.
>> On platforms with HW tracking HW waits few identical frames
>> until really activating PSR. VLV/CHV activation is immediate.
>> But HW is also different and there it seemed that hw needed a
>> few more time before starting the transitions.
>> Furthermore I didn't want to add that so quickly because I didn't
>> want to take the risk of killing battery with software tracking
>> when doing transitions so quickly using software tracking.
>> 
>> >
>> > 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.
>> 
>> Putting that way I was asking myself how that hack had ever fixed
>> my issue. Because the way you explained here seems obvious that it
>> wouldn't ever fix my bug or any other.
>> 
>> So I applied your patch and it made even more sense (without considering
>> the fact I want to kill the first call anyways).
>> 
>> So I came back, removed your patch and tried to understand how did
>> it ever worked.
>> 
>> So, the thing is that intel_psr_flush will never be really executed
>> if intel_psr_enable wasn't executed. That is guaranteed by:
>> 
>> mutex_lock(&dev_priv->psr.lock);
>> 	if (!dev_priv->psr.enabled) {
>> 
>> So, intel_psr_enable will be for sure the first one to schedule the
>> work delayed to the ugly higher delay.
>> 
>> >
>> > 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.
>> 
>> During this time you are right, many calls of intel_psr_exit
>> coming from flush functions can be called... But none of
>> them will schedule the work with 100 delay.
>> 
>> they will skip on
>> if (!work_busy(&dev_priv->psr.work.work))
>
> Wouldn't work_busy() return false until the work is actually queued
> which is 100ms after calling schedule_delayed_work()?

That's not my understanding of work_busy man.

"work_busy - test whether a work is currently pending or running"

I consider it as pending one...

But yeap... it was a long time ago that I did this so I'm not sure...

>
> For e.g, flushes at 0, 16, 32...96 will have work_busy() returning false
> until 100ms.
>
> The first psr_work will end up getting scheduled at 100ms, which I
> believe is not what we want. 
>
>
> However, I think 
>
> 	if (dev_priv->psr.busy_frontbuffer_bits)
> 		goto unlock;
>
> 	intel_psr_activate(intel_dp);
>
> in psr_work might prevent activate being called at 100ms if an
> invalidate happened to be called before that.
>
>
>
>
>> 
>> So, the higher delayed *hack* will be respected and PSR won't get
>> activated before that.
>> 
>> On the other hand you might ask what if,
>> for some strange reason,
>> (intel_dp->panel_power_cycle_delay * 5) is lesser than 100.
>> Well, on this case this would be ok, because it happens only
>> once and only on gen > 9 where hw tracking will wait the minimal
>> number of frames before the actual transition to PSR.
>> 
>> In either cases I believe we are safe.
>> 
>> Thanks,
>> Rodrigo.
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx