On Tue, 2018-02-13 at 15:26 -0800, Rodrigo Vivi wrote:
> From: Andy Lutomirski <luto@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Tested-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Acked-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
