Re: [Intel-gfx] [PATCH 1/5] drm/i915: Improve PSR activation timing
On Wed, Feb 28, 2018 at 12:26 AM, Chris Wilson wrote: > Quoting Andy Lutomirski (2018-02-24 00:07:23) >> On Tue, Feb 13, 2018 at 11:26 PM, Rodrigo Vivi >> wrote: >> > From: Andy Lutomirski >> > >> > + >> > + dev_priv->psr.activate_timer.expires = jiffies - 1; >> >> That can't possibly be okay. > > As an initialisation value, set to the previous jiffie? You can set it > to the current jiffie, but then you have the issue of not noticing the > update to the current jiffie. > > So how is this any more incorrect? I don't think you can just write to fields in struct timer_list like that. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/5] drm/i915: Improve PSR activation timing
Quoting Andy Lutomirski (2018-02-24 00:07:23) > On Tue, Feb 13, 2018 at 11:26 PM, Rodrigo Vivi wrote: > > From: Andy Lutomirski > > > > + > > + dev_priv->psr.activate_timer.expires = jiffies - 1; > > That can't possibly be okay. As an initialisation value, set to the previous jiffie? You can set it to the current jiffie, but then you have the issue of not noticing the update to the current jiffie. So how is this any more incorrect? -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/5] drm/i915: Improve PSR activation timing
On Tue, Feb 13, 2018 at 11:26 PM, Rodrigo Vivi wrote: > From: Andy Lutomirski > > + > + dev_priv->psr.activate_timer.expires = jiffies - 1; That can't possibly be okay. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/5] drm/i915: Improve PSR activation timing
"Pandiyan, Dhinakaran" writes:
> On Tue, 2018-02-13 at 15:26 -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.
>>
>> Signed-off-by: Andy Lutomirski
>> Tested-by: Rodrigo Vivi
>> Acked-by: Rodrigo Vivi
>> Signed-off-by: Rodrigo Vivi
>>
>> ---
>> 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-o
Re: [Intel-gfx] [PATCH 1/5] drm/i915: Improve PSR activation timing
On Tue, 2018-02-13 at 15:26 -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.
>
> Signed-off-by: Andy Lutomirski
> Tested-by: Rodrigo Vivi
> Acked-by: Rodrigo Vivi
> Signed-off-by: Rodrigo Vivi
>
> ---
> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
