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

[Andy Lutomirski]

On Wed, Feb 28, 2018 at 12:22 AM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> Quoting Rodrigo Vivi (2018-02-28 00:14:08)
>> From: Andy Lutomirski <luto at 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.pandiyan at intel.com>
>> Signed-off-by: Andy Lutomirski <luto at kernel.org>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at 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) *
>
> 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.