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

[Rodrigo Vivi]

"Pandiyan, Dhinakaran" <dhinakaran.pandiyan at intel.com> writes:

> On Tue, 2018-02-13 at 15:26 -0800, Rodrigo Vivi wrote:
>> 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.
>> 
>> Signed-off-by: Andy Lutomirski <luto at kernel.org>
>> Tested-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> Acked-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> 
>> ---
>>  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.

Chris I believe this question is for you.

I just ignored for a while because I thought it was for Andy,
but now I saw that you modified the original patch on exactly this point.

Btw I believe this is a modification that should had been clear
highlighted when you sent and with your Signed-off-by.

So, could you please explain and give the sign-off here?
Or should we rebase original Andy's version without this change?

Thanks,
Rodrigo.

>
>
> -DK
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx