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 <l...@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-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
