[PATCH] drm/i915: Use ktime on wait_for

[PATCH] drm/i915: Use ktime on wait_for

[Mika Kuoppala]

We use jiffies to determine when wait expires. However
Imre did find out that jiffies can and will do a >1
increments on certain situations [1]. When this happens
in a wait_for loop, we return timeout errorneously
much earlier than what the real wallclock would say.

We can't afford our waits to timeout prematurely.
Discard jiffies and change to ktime to detect timeouts.

Reported-by: Imre Deak <imre.deak@intel.com>
References: https://lkml.org/lkml/2018/4/18/798 [1]
Cc: Imre Deak <imre.deak@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8b20824e806e..ac7565220aa3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -49,12 +49,12 @@
  * check the condition before the timeout.
  */
 #define __wait_for(OP, COND, US, Wmin, Wmax) ({ \
-	unsigned long timeout__ = jiffies + usecs_to_jiffies(US) + 1;	\
+	const ktime_t end__ = ktime_add_ns(ktime_get_raw(), 1000ll * (US)); \
 	long wait__ = (Wmin); /* recommended min for usleep is 10 us */	\
 	int ret__;							\
 	might_sleep();							\
 	for (;;) {							\
-		bool expired__ = time_after(jiffies, timeout__);	\
+		const bool expired__ = ktime_after(ktime_get_raw(), end__); \
 		OP;							\
 		if (COND) {						\
 			ret__ = 0;					\
-- 
2.14.1

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

Re: [PATCH] drm/i915: Use ktime on wait_for

[Sagar Arun Kamble]

On 4/20/2018 3:24 PM, Mika Kuoppala wrote:
> We use jiffies to determine when wait expires. However
> Imre did find out that jiffies can and will do a >1
> increments on certain situations [1]. When this happens
> in a wait_for loop, we return timeout errorneously
> much earlier than what the real wallclock would say.
>
> We can't afford our waits to timeout prematurely.
> Discard jiffies and change to ktime to detect timeouts.
>
> Reported-by: Imre Deak <imre.deak@intel.com>
> References: https://lkml.org/lkml/2018/4/18/798 [1]
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_drv.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 8b20824e806e..ac7565220aa3 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -49,12 +49,12 @@
>    * check the condition before the timeout.
>    */
>   #define __wait_for(OP, COND, US, Wmin, Wmax) ({ \
> -	unsigned long timeout__ = jiffies + usecs_to_jiffies(US) + 1;	\
> +	const ktime_t end__ = ktime_add_ns(ktime_get_raw(), 1000ll * (US)); \
Is ktime_get_raw() monotonic? Thomas suggested ktime_get()
>   	long wait__ = (Wmin); /* recommended min for usleep is 10 us */	\
>   	int ret__;							\
>   	might_sleep();							\
>   	for (;;) {							\
> -		bool expired__ = time_after(jiffies, timeout__);	\
> +		const bool expired__ = ktime_after(ktime_get_raw(), end__); \
>   		OP;							\
>   		if (COND) {						\
>   			ret__ = 0;					\

-- 
Thanks,
Sagar

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

Re: [PATCH] drm/i915: Use ktime on wait_for

[Chris Wilson]

Quoting Mika Kuoppala (2018-04-20 10:54:26)
> We use jiffies to determine when wait expires. However
> Imre did find out that jiffies can and will do a >1
> increments on certain situations [1]. When this happens
> in a wait_for loop, we return timeout errorneously
> much earlier than what the real wallclock would say.
> 
> We can't afford our waits to timeout prematurely.
> Discard jiffies and change to ktime to detect timeouts.
> 
> Reported-by: Imre Deak <imre.deak@intel.com>
> References: https://lkml.org/lkml/2018/4/18/798 [1]
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

The atomic variant is already jiffie-less (since it has to work in
irq-off contexts). Maybe a bit tricky to suggest that the callers know
if jiffie incremens are accurate or not.

What is not clear from the link is whether our wait_for() is running
across suspend, or whether it is just jiffie recalibration some time
during resume that breaks.

>  drivers/gpu/drm/i915/intel_drv.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 8b20824e806e..ac7565220aa3 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -49,12 +49,12 @@
>   * check the condition before the timeout.
>   */
>  #define __wait_for(OP, COND, US, Wmin, Wmax) ({ \
> -       unsigned long timeout__ = jiffies + usecs_to_jiffies(US) + 1;   \
> +       const ktime_t end__ = ktime_add_ns(ktime_get_raw(), 1000ll * (US)); \
>         long wait__ = (Wmin); /* recommended min for usleep is 10 us */ \
>         int ret__;                                                      \
>         might_sleep();                                                  \
>         for (;;) {                                                      \
> -               bool expired__ = time_after(jiffies, timeout__);        \
> +               const bool expired__ = ktime_after(ktime_get_raw(), end__); \
>                 OP;                                                     \
>                 if (COND) {                                             \
>                         ret__ = 0;                                      \

Nevertheless, the patch is ok and I don't have too much objection to
adding another tsc (at best, hpet at worst!) read around every mmio+sleep,
plus expanding the code for the function calls. Out of curiosity what is
the size delta? How many wait_for() do we have left that we need to
convert to a function call rather than macro expansion?

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Cc stable?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use ktime on wait_for

[Chris Wilson]

Quoting Sagar Arun Kamble (2018-04-20 11:23:50)
> 
> 
> On 4/20/2018 3:24 PM, Mika Kuoppala wrote:
> > We use jiffies to determine when wait expires. However
> > Imre did find out that jiffies can and will do a >1
> > increments on certain situations [1]. When this happens
> > in a wait_for loop, we return timeout errorneously
> > much earlier than what the real wallclock would say.
> >
> > We can't afford our waits to timeout prematurely.
> > Discard jiffies and change to ktime to detect timeouts.
> >
> > Reported-by: Imre Deak <imre.deak@intel.com>
> > References: https://lkml.org/lkml/2018/4/18/798 [1]
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_drv.h | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 8b20824e806e..ac7565220aa3 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -49,12 +49,12 @@
> >    * check the condition before the timeout.
> >    */
> >   #define __wait_for(OP, COND, US, Wmin, Wmax) ({ \
> > -     unsigned long timeout__ = jiffies + usecs_to_jiffies(US) + 1;   \
> > +     const ktime_t end__ = ktime_add_ns(ktime_get_raw(), 1000ll * (US)); \
> Is ktime_get_raw() monotonic? Thomas suggested ktime_get()

It proclaims to be monotonic, without the clock drift calibration. For
the milliseconds we should be sleeping at most, I hope that is
immaterial.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Use ktime on wait_for

[Patchwork]

== Series Details ==

Series: drm/i915: Use ktime on wait_for
URL   : https://patchwork.freedesktop.org/series/42023/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4072 -> Patchwork_8762 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/42023/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_8762 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_mmap_gtt@basic-small-bo-tiledx:
      fi-gdg-551:         PASS -> FAIL (fdo#102575)

    
  fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575


== Participating hosts (33 -> 31) ==

  Missing    (2): fi-ilk-m540 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4072 -> Patchwork_8762

  CI_DRM_4072: b35e59e5c6a9cae11d5183d2bf9c5c99ceedbc7c @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4442: 8168bb65d5e64d4df4e5d847d448bab2d2825d73 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8762: 62a2b14393160309a7e197bd6b3131a56edd01ab @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4442: e60d247eb359f044caf0c09904da14e39d7adca1 @ git://anongit.freedesktop.org/piglit


== Linux commits ==

62a2b1439316 drm/i915: Use ktime on wait_for

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8762/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use ktime on wait_for

[Sagar Arun Kamble]

On 4/20/2018 4:09 PM, Chris Wilson wrote:
> Quoting Sagar Arun Kamble (2018-04-20 11:23:50)
>>
>> On 4/20/2018 3:24 PM, Mika Kuoppala wrote:
>>> We use jiffies to determine when wait expires. However
>>> Imre did find out that jiffies can and will do a >1
>>> increments on certain situations [1]. When this happens
>>> in a wait_for loop, we return timeout errorneously
>>> much earlier than what the real wallclock would say.
>>>
>>> We can't afford our waits to timeout prematurely.
>>> Discard jiffies and change to ktime to detect timeouts.
>>>
>>> Reported-by: Imre Deak <imre.deak@intel.com>
>>> References: https://lkml.org/lkml/2018/4/18/798 [1]
>>> Cc: Imre Deak <imre.deak@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/intel_drv.h | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>> index 8b20824e806e..ac7565220aa3 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -49,12 +49,12 @@
>>>     * check the condition before the timeout.
>>>     */
>>>    #define __wait_for(OP, COND, US, Wmin, Wmax) ({ \
>>> -     unsigned long timeout__ = jiffies + usecs_to_jiffies(US) + 1;   \
>>> +     const ktime_t end__ = ktime_add_ns(ktime_get_raw(), 1000ll * (US)); \
>> Is ktime_get_raw() monotonic? Thomas suggested ktime_get()
> It proclaims to be monotonic, without the clock drift calibration. For
> the milliseconds we should be sleeping at most, I hope that is
> immaterial.
Yes. I remembered from Imre's comment[1] that raw clock can jump and 
will not be calibrated. If jumps are are not > jiffies we should be good 
get_raw then.

[1] 
https://lists.freedesktop.org/archives/dri-devel/2012-October/028878.html
> -Chris

-- 
Thanks,
Sagar

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

✓ Fi.CI.IGT: success for drm/i915: Use ktime on wait_for

[Patchwork]

== Series Details ==

Series: drm/i915: Use ktime on wait_for
URL   : https://patchwork.freedesktop.org/series/42023/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4072_full -> Patchwork_8762_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_8762_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8762_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/42023/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_8762_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_schedule@deep-blt:
      shard-kbl:          PASS -> SKIP

    igt@gem_exec_schedule@deep-bsd1:
      shard-kbl:          SKIP -> PASS +1

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-c-frame-sequence:
      shard-glk:          PASS -> SKIP +88

    igt@kms_universal_plane@universal-plane-pipe-b-sanity:
      shard-glk:          SKIP -> PASS +102

    
== Known issues ==

  Here are the changes found in Patchwork_8762_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_ppgtt@blt-vs-render-ctxn:
      shard-kbl:          PASS -> INCOMPLETE (fdo#106023, fdo#103665)

    
    ==== Possible fixes ====

    igt@kms_flip@dpms-vs-vblank-race-interruptible:
      shard-glk:          FAIL (fdo#103060) -> PASS

    igt@kms_flip@plain-flip-fb-recreate:
      shard-hsw:          FAIL (fdo#100368) -> PASS
      shard-glk:          FAIL (fdo#100368) -> SKIP

    igt@perf@blocking:
      shard-hsw:          FAIL (fdo#102252) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023


== Participating hosts (6 -> 5) ==

  Missing    (1): shard-glkb 


== Build changes ==

    * Linux: CI_DRM_4072 -> Patchwork_8762

  CI_DRM_4072: b35e59e5c6a9cae11d5183d2bf9c5c99ceedbc7c @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4442: 8168bb65d5e64d4df4e5d847d448bab2d2825d73 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8762: 62a2b14393160309a7e197bd6b3131a56edd01ab @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4442: e60d247eb359f044caf0c09904da14e39d7adca1 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8762/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use ktime on wait_for

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Sagar Arun Kamble (2018-04-20 11:23:50)
>> 
>> 
>> On 4/20/2018 3:24 PM, Mika Kuoppala wrote:
>> > We use jiffies to determine when wait expires. However
>> > Imre did find out that jiffies can and will do a >1
>> > increments on certain situations [1]. When this happens
>> > in a wait_for loop, we return timeout errorneously
>> > much earlier than what the real wallclock would say.
>> >
>> > We can't afford our waits to timeout prematurely.
>> > Discard jiffies and change to ktime to detect timeouts.
>> >
>> > Reported-by: Imre Deak <imre.deak@intel.com>
>> > References: https://lkml.org/lkml/2018/4/18/798 [1]
>> > Cc: Imre Deak <imre.deak@intel.com>
>> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> > ---
>> >   drivers/gpu/drm/i915/intel_drv.h | 4 ++--
>> >   1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> > index 8b20824e806e..ac7565220aa3 100644
>> > --- a/drivers/gpu/drm/i915/intel_drv.h
>> > +++ b/drivers/gpu/drm/i915/intel_drv.h
>> > @@ -49,12 +49,12 @@
>> >    * check the condition before the timeout.
>> >    */
>> >   #define __wait_for(OP, COND, US, Wmin, Wmax) ({ \
>> > -     unsigned long timeout__ = jiffies + usecs_to_jiffies(US) + 1;   \
>> > +     const ktime_t end__ = ktime_add_ns(ktime_get_raw(), 1000ll * (US)); \
>> Is ktime_get_raw() monotonic? Thomas suggested ktime_get()
>
> It proclaims to be monotonic, without the clock drift calibration. For
> the milliseconds we should be sleeping at most, I hope that is
> immaterial.

Agreed. And even if we would be affected by drift, we would want
to extend the end a little instead of accepting a drift calibration
while we are waiting.

Here is how the code size is affected:

add/remove: 0/0 grow/shrink: 5/29 up/down: 77/-1099 (-1022)
Function                                     old     new   delta
__intel_wait_for_register_fw                 462     496     +34
swsci                                        576     596     +20
skl_pcode_request                            488     507     +19
intel_hdcp_auth                             3806    3809      +3
__intel_wait_for_register                    436     437      +1
hsw_power_well_disable                       548     539      -9
i915_gem_idle_work_handler                   608     593     -15
guc_fw_xfer                                  875     854     -21
bxt_ddi_pll_disable                          324     303     -21
chv_set_cdclk                                357     334     -23
vlv_wm_get_hw_state                         2474    2450     -24
gmbus_wait                                   564     538     -26
cnl_cdclk_pll_enable                         286     259     -27
cnl_cdclk_pll_disable                        247     220     -27
hsw_enable_pc8                              1699    1671     -28
bdw_set_cdclk                               1006     977     -29
wait_for_pipe_scanline_moving                388     358     -30
intel_enable_dsi_pll                        1060    1028     -32
i915_gem_wait_for_idle                       348     314     -34
chv_set_memory_dvfs                          258     223     -35
vlv_set_power_well                           321     282     -39
intel_hdmi_hdcp_check_link                   485     446     -39
chv_set_pipe_power_well.constprop            359     320     -39
g33_do_reset                                 216     175     -41
vlv_wait_for_pw_status                       210     168     -42
intel_engines_park                           348     306     -42
vlv_set_cdclk                                711     662     -49
lspcon_wait_mode                             265     211     -54
lpt_init_pch_refclk                         1542    1488     -54
g4x_do_reset                                 595     541     -54
bxt_ddi_pll_enable                          2407    2353     -54
ironlake_crtc_enable                        3279    3211     -68
i915_do_reset                                443     375     -68
intel_guc_send_ct                           1691    1616     -75
Total: Before=1232834, After=1231812, chg -0.08%

-Mika
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use ktime on wait_for

[Imre Deak]

On Fri, Apr 20, 2018 at 11:27:55AM +0100, Chris Wilson wrote:
> Quoting Mika Kuoppala (2018-04-20 10:54:26)
> > We use jiffies to determine when wait expires. However
> > Imre did find out that jiffies can and will do a >1
> > increments on certain situations [1]. When this happens
> > in a wait_for loop, we return timeout errorneously
> > much earlier than what the real wallclock would say.
> > 
> > We can't afford our waits to timeout prematurely.
> > Discard jiffies and change to ktime to detect timeouts.
> > 
> > Reported-by: Imre Deak <imre.deak@intel.com>
> > References: https://lkml.org/lkml/2018/4/18/798 [1]
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> 
> The atomic variant is already jiffie-less (since it has to work in
> irq-off contexts). Maybe a bit tricky to suggest that the callers know
> if jiffie incremens are accurate or not.
> 
> What is not clear from the link is whether our wait_for() is running
> across suspend, or whether it is just jiffie recalibration some time
> during resume that breaks.

The wait_for starts on the resume path, so the jump shouldn't be related
to any of the timekeeping adjustments across suspend/resume (happening
already during syscore resume). It looks like a delayed LAPIC timer
interrupt on that GLK system, trying to get more details on that with
irqsoff ftracing.

> 
> >  drivers/gpu/drm/i915/intel_drv.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 8b20824e806e..ac7565220aa3 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -49,12 +49,12 @@
> >   * check the condition before the timeout.
> >   */
> >  #define __wait_for(OP, COND, US, Wmin, Wmax) ({ \
> > -       unsigned long timeout__ = jiffies + usecs_to_jiffies(US) + 1;   \
> > +       const ktime_t end__ = ktime_add_ns(ktime_get_raw(), 1000ll * (US)); \
> >         long wait__ = (Wmin); /* recommended min for usleep is 10 us */ \
> >         int ret__;                                                      \
> >         might_sleep();                                                  \
> >         for (;;) {                                                      \
> > -               bool expired__ = time_after(jiffies, timeout__);        \
> > +               const bool expired__ = ktime_after(ktime_get_raw(), end__); \
> >                 OP;                                                     \
> >                 if (COND) {                                             \
> >                         ret__ = 0;                                      \
> 
> Nevertheless, the patch is ok and I don't have too much objection to
> adding another tsc (at best, hpet at worst!) read around every mmio+sleep,
> plus expanding the code for the function calls. Out of curiosity what is
> the size delta? How many wait_for() do we have left that we need to
> convert to a function call rather than macro expansion?
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Cc stable?
> -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Use ktime on wait_for

[Mika Kuoppala]

Imre Deak <imre.deak@intel.com> writes:

> On Fri, Apr 20, 2018 at 11:27:55AM +0100, Chris Wilson wrote:
>> Quoting Mika Kuoppala (2018-04-20 10:54:26)
>> > We use jiffies to determine when wait expires. However
>> > Imre did find out that jiffies can and will do a >1
>> > increments on certain situations [1]. When this happens
>> > in a wait_for loop, we return timeout errorneously
>> > much earlier than what the real wallclock would say.
>> > 
>> > We can't afford our waits to timeout prematurely.
>> > Discard jiffies and change to ktime to detect timeouts.
>> > 
>> > Reported-by: Imre Deak <imre.deak@intel.com>
>> > References: https://lkml.org/lkml/2018/4/18/798 [1]
>> > Cc: Imre Deak <imre.deak@intel.com>
>> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> 
>> The atomic variant is already jiffie-less (since it has to work in
>> irq-off contexts). Maybe a bit tricky to suggest that the callers know
>> if jiffie incremens are accurate or not.
>> 
>> What is not clear from the link is whether our wait_for() is running
>> across suspend, or whether it is just jiffie recalibration some time
>> during resume that breaks.
>
> The wait_for starts on the resume path, so the jump shouldn't be related
> to any of the timekeeping adjustments across suspend/resume (happening
> already during syscore resume). It looks like a delayed LAPIC timer
> interrupt on that GLK system, trying to get more details on that with
> irqsoff ftracing.

Both patches pushed. Thanks for the report and reviews.

-Mika

>
>> 
>> >  drivers/gpu/drm/i915/intel_drv.h | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> > index 8b20824e806e..ac7565220aa3 100644
>> > --- a/drivers/gpu/drm/i915/intel_drv.h
>> > +++ b/drivers/gpu/drm/i915/intel_drv.h
>> > @@ -49,12 +49,12 @@
>> >   * check the condition before the timeout.
>> >   */
>> >  #define __wait_for(OP, COND, US, Wmin, Wmax) ({ \
>> > -       unsigned long timeout__ = jiffies + usecs_to_jiffies(US) + 1;   \
>> > +       const ktime_t end__ = ktime_add_ns(ktime_get_raw(), 1000ll * (US)); \
>> >         long wait__ = (Wmin); /* recommended min for usleep is 10 us */ \
>> >         int ret__;                                                      \
>> >         might_sleep();                                                  \
>> >         for (;;) {                                                      \
>> > -               bool expired__ = time_after(jiffies, timeout__);        \
>> > +               const bool expired__ = ktime_after(ktime_get_raw(), end__); \
>> >                 OP;                                                     \
>> >                 if (COND) {                                             \
>> >                         ret__ = 0;                                      \
>> 
>> Nevertheless, the patch is ok and I don't have too much objection to
>> adding another tsc (at best, hpet at worst!) read around every mmio+sleep,
>> plus expanding the code for the function calls. Out of curiosity what is
>> the size delta? How many wait_for() do we have left that we need to
>> convert to a function call rather than macro expansion?
>> 
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>> 
>> Cc stable?
>> -Chris
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx