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

[Pandiyan, Dhinakaran]

On Tue, 2018-02-27 at 16:14 -0800, Rodrigo Vivi wrote:
> From: 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.
> 
> v3: (by Rodrigo): Rebased on top of recent drm-tip without any
> modification from the original.
> 
> Cc: Dhinakaran Pandiyan 
> Signed-off-by: Andy Lutomirski 
> Signed-off-by: Rodrigo Vivi 
> ---
>  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(_priv->psr.work.work)));
> +
> + if (timer_pending(_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(_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(_priv->psr.activate_timer, next);
> + }
> +}
> +
>  static void hsw_psr_enable_source(struct intel_dp *intel_dp,
> const struct intel_crtc_state *crtc_state)
>  {
> @@ 

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

[Andy Lutomirski]

On Wed, Feb 28, 2018 at 12:22 AM, Chris Wilson  wrote:
> Quoting Rodrigo Vivi (2018-02-28 00:14:08)
>> From: 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.
>>
>> v3: (by Rodrigo): Rebased on top of recent drm-tip without any
>> modification from the original.
>>
>> Cc: Dhinakaran Pandiyan 
>> Signed-off-by: Andy Lutomirski 
>> Signed-off-by: Rodrigo Vivi 
>> ---
>>  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(_priv->psr.work.work)));
>> +
>> +   if (timer_pending(_priv->psr.activate_timer))
>> +   seq_printf(m, "Activate scheduled: yes, in %ldms\n",
>> +  (long)(dev_priv->psr.earliest_activate - jiffies) 
>> *
>
> msecs_from_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;
>
> Incorporated into struct timer_list, so this is redundant.

This way gives a clean way to say "don't do the work before
such-and-such time".  I don't think we can do it with mod_timer()
since the timer might already have started firing, and we can't
del_timer_sync() because there would be a lock inversion.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

Quoting Rodrigo Vivi (2018-02-28 00:14:08)
> From: 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.
> 
> v3: (by Rodrigo): Rebased on top of recent drm-tip without any
> modification from the original.
> 
> Cc: Dhinakaran Pandiyan 
> Signed-off-by: Andy Lutomirski 
> Signed-off-by: Rodrigo Vivi 
> ---
>  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(_priv->psr.work.work)));
> +
> +   if (timer_pending(_priv->psr.activate_timer))
> +   seq_printf(m, "Activate scheduled: yes, in %ldms\n",
> +  (long)(dev_priv->psr.earliest_activate - jiffies) *

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

Incorporated into struct timer_list, so this is redundant.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Rodrigo Vivi]

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

v3: (by Rodrigo): Rebased on top of recent drm-tip without any
modification from the original.

Cc: Dhinakaran Pandiyan 
Signed-off-by: Andy Lutomirski 
Signed-off-by: Rodrigo Vivi 
---
 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(_priv->psr.work.work)));
+
+   if (timer_pending(_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(_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(_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.