On Tue, 2018-02-27 at 16:14 -0800, Rodrigo Vivi wrote:
> From: Andy Lutomirski <l...@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.
> 
> v3: (by Rodrigo): Rebased on top of recent drm-tip without any
>     modification from the original.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com>
> Signed-off-by: Andy Lutomirski <l...@kernel.org>
> Signed-off-by: Rodrigo Vivi <rodrigo.v...@intel.com>
> ---
>  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 33fbf3965309..1ac942d1742e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2572,8 +2572,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 7bbec5546d12..6e6cf2ce3749 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -764,7 +764,9 @@ 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 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 05770790a4e9..c10d5225dc7c 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -570,6 +570,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)
>  {
> @@ -656,8 +680,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:
> @@ -777,13 +800,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);


lockdep does not seem to be happy.
        
[  555.236359] Setting dangerous option enable_psr - tainting kernel
[  557.368892] INFO: trying to register non-static key.
[  557.368899] the code is fine but needs lockdep annotation.
[  557.368902] turning off the locking correctness validator.
[  557.368907] CPU: 3 PID: 4404 Comm: kms_psr_sink_cr Tainted: G     U
4.16.0-rc3-CI-Patchwork_8182+ #1
[  557.368910] Hardware name: TOSHIBA SATELLITE
P50-C/06F4                            , BIOS 1.40 03/29/2016
[  557.368913] Call Trace:
[  557.368922]  dump_stack+0x5f/0x86
[  557.368931]  register_lock_class+0x57e/0x590
[  557.368936]  ? lock_acquire+0xaf/0x200
[  557.368941]  __lock_acquire+0xa4/0x1b60
[  557.368948]  ? try_to_del_timer_sync+0x60/0x60
[  557.368955]  ? drm_dp_dpcd_access+0xdb/0xf0
[  557.368960]  ? lock_acquire+0xaf/0x200
[  557.368966]  lock_acquire+0xaf/0x200
[  557.368971]  ? try_to_del_timer_sync+0x60/0x60
[  557.368977]  del_timer_sync+0x37/0xb0
[  557.368982]  ? try_to_del_timer_sync+0x60/0x60
[  557.369052]  intel_psr_disable+0xc5/0xe0 [i915]
[  557.369120]  intel_disable_ddi+0x75/0xf0 [i915]


Full log here:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8182/fi-skl-6600u/dmesg0.log



>  }
>  
>  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;
> @@ -793,6 +817,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,
> @@ -829,6 +854,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
> @@ -842,6 +879,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;
> @@ -1022,9 +1073,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);
>  }
>  
> @@ -1071,7 +1121,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)) {
> @@ -1087,4 +1138,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;
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to