* [PATCH 1/2] drm/i915: fold wait_for_atomic_us into wait_for_atomic
@ 2013-03-27 23:03 Daniel Vetter
2013-03-27 23:03 ` [PATCH 2/2] drm/i915: fix up _wait_for macro Daniel Vetter
2013-03-28 10:32 ` [PATCH 1/2] drm/i915: fold wait_for_atomic_us into wait_for_atomic Ville Syrjälä
0 siblings, 2 replies; 7+ messages in thread
From: Daniel Vetter @ 2013-03-27 23:03 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter, Jack Winter
Since
commit bcf9dcc1e6269fac674e41f25d007ff75f76e840
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date: Sun Jul 15 09:42:38 2012 +0100
drm/i915: Workaround hang with BSD and forcewake on SandyBridge
and
commit 0cc2764cc4a4bd73df55f8893c871778cf7ddd0f
Author: Ben Widawsky <ben@bwidawsk.net>
Date: Sat Sep 1 22:59:48 2012 -0700
drm/i915: use cpu_relax() in wait_for_atomic
these two macros are essentially the same, so unify them. We keep the
_us version since it's a nice documentation for smaller timeouts.
Cc: Jack Winter <jbh@alchemy.lu>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/intel_drv.h | 15 ++-------------
1 file changed, 2 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 54bc2ea..c8c1979 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -50,21 +50,10 @@
ret__; \
})
-#define wait_for_atomic_us(COND, US) ({ \
- unsigned long timeout__ = jiffies + usecs_to_jiffies(US); \
- int ret__ = 0; \
- while (!(COND)) { \
- if (time_after(jiffies, timeout__)) { \
- ret__ = -ETIMEDOUT; \
- break; \
- } \
- cpu_relax(); \
- } \
- ret__; \
-})
-
#define wait_for(COND, MS) _wait_for(COND, MS, 1)
#define wait_for_atomic(COND, MS) _wait_for(COND, MS, 0)
+#define wait_for_atomic_us(COND, US) _wait_for((COND), \
+ usecs_to_jiffies((US)), 0)
#define KHz(x) (1000*x)
#define MHz(x) KHz(1000*x)
--
1.7.11.7
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] drm/i915: fix up _wait_for macro
2013-03-27 23:03 [PATCH 1/2] drm/i915: fold wait_for_atomic_us into wait_for_atomic Daniel Vetter
@ 2013-03-27 23:03 ` Daniel Vetter
2013-03-28 11:00 ` Ville Syrjälä
2013-03-28 10:32 ` [PATCH 1/2] drm/i915: fold wait_for_atomic_us into wait_for_atomic Ville Syrjälä
1 sibling, 1 reply; 7+ messages in thread
From: Daniel Vetter @ 2013-03-27 23:03 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter, Thomas Gleixner, Jack Winter
As Thomas Gleixner spotted, it's rather horrible racy:
- We can miss almost a full tick, so need to compensate by 1 jiffy.
- We need to re-check the condition when having timed-out, since a
the last check could have been before the timeout expired. E.g. when
we've been preempted or a long irq happened.
Cc: Thomas Gleixner <tglx@linutronix.de>
Reported-by: Jack Winter <jbh@alchemy.lu>
Cc: Jack Winter <jbh@alchemy.lu>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/intel_drv.h | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c8c1979..9dcae4e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -33,12 +33,21 @@
#include <drm/drm_fb_helper.h>
#include <drm/drm_dp_helper.h>
+/**
+ * _wait_for - magic (register) wait macro
+ *
+ * Does the right thing for modeset paths when run under kdgb or similar atomic
+ * contexts. Note that it's important that we check the condition again after
+ * having timed out, since the timeout could be due to preemption or similar and
+ * we've never had a chance to check the condition before the timeout.
+ */
#define _wait_for(COND, MS, W) ({ \
- unsigned long timeout__ = jiffies + msecs_to_jiffies(MS); \
+ unsigned long timeout__ = jiffies + msecs_to_jiffies(MS) + 1; \
int ret__ = 0; \
while (!(COND)) { \
if (time_after(jiffies, timeout__)) { \
- ret__ = -ETIMEDOUT; \
+ if (!(COND)) \
+ ret__ = -ETIMEDOUT; \
break; \
} \
if (W && drm_can_sleep()) { \
--
1.7.11.7
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] drm/i915: fold wait_for_atomic_us into wait_for_atomic
2013-03-28 10:32 ` [PATCH 1/2] drm/i915: fold wait_for_atomic_us into wait_for_atomic Ville Syrjälä
@ 2013-03-28 10:31 ` Daniel Vetter
2013-03-28 10:53 ` Ville Syrjälä
0 siblings, 1 reply; 7+ messages in thread
From: Daniel Vetter @ 2013-03-28 10:31 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter, Jack Winter
Since
commit bcf9dcc1e6269fac674e41f25d007ff75f76e840
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date: Sun Jul 15 09:42:38 2012 +0100
drm/i915: Workaround hang with BSD and forcewake on SandyBridge
and
commit 0cc2764cc4a4bd73df55f8893c871778cf7ddd0f
Author: Ben Widawsky <ben@bwidawsk.net>
Date: Sat Sep 1 22:59:48 2012 -0700
drm/i915: use cpu_relax() in wait_for_atomic
these two macros are essentially the same, so unify them. We keep the
_us version since it's a nice documentation for smaller timeouts.
v2: Fixup time unit conversion, _wait_for takes ms (Ville).
Cc: Jack Winter <jbh@alchemy.lu>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/intel_drv.h | 15 ++-------------
1 file changed, 2 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 54bc2ea..8e76f52f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -50,21 +50,10 @@
ret__; \
})
-#define wait_for_atomic_us(COND, US) ({ \
- unsigned long timeout__ = jiffies + usecs_to_jiffies(US); \
- int ret__ = 0; \
- while (!(COND)) { \
- if (time_after(jiffies, timeout__)) { \
- ret__ = -ETIMEDOUT; \
- break; \
- } \
- cpu_relax(); \
- } \
- ret__; \
-})
-
#define wait_for(COND, MS) _wait_for(COND, MS, 1)
#define wait_for_atomic(COND, MS) _wait_for(COND, MS, 0)
+#define wait_for_atomic_us(COND, US) _wait_for((COND), \
+ DIV_ROUND_UP((US), 1000), 0)
#define KHz(x) (1000*x)
#define MHz(x) KHz(1000*x)
--
1.7.11.7
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] drm/i915: fold wait_for_atomic_us into wait_for_atomic
2013-03-27 23:03 [PATCH 1/2] drm/i915: fold wait_for_atomic_us into wait_for_atomic Daniel Vetter
2013-03-27 23:03 ` [PATCH 2/2] drm/i915: fix up _wait_for macro Daniel Vetter
@ 2013-03-28 10:32 ` Ville Syrjälä
2013-03-28 10:31 ` [PATCH] " Daniel Vetter
1 sibling, 1 reply; 7+ messages in thread
From: Ville Syrjälä @ 2013-03-28 10:32 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development, Jack Winter
On Thu, Mar 28, 2013 at 12:03:24AM +0100, Daniel Vetter wrote:
> Since
>
> commit bcf9dcc1e6269fac674e41f25d007ff75f76e840
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date: Sun Jul 15 09:42:38 2012 +0100
>
> drm/i915: Workaround hang with BSD and forcewake on SandyBridge
>
> and
>
> commit 0cc2764cc4a4bd73df55f8893c871778cf7ddd0f
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date: Sat Sep 1 22:59:48 2012 -0700
>
> drm/i915: use cpu_relax() in wait_for_atomic
>
> these two macros are essentially the same, so unify them. We keep the
> _us version since it's a nice documentation for smaller timeouts.
>
> Cc: Jack Winter <jbh@alchemy.lu>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/i915/intel_drv.h | 15 ++-------------
> 1 file changed, 2 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 54bc2ea..c8c1979 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -50,21 +50,10 @@
> ret__; \
> })
>
> -#define wait_for_atomic_us(COND, US) ({ \
> - unsigned long timeout__ = jiffies + usecs_to_jiffies(US); \
> - int ret__ = 0; \
> - while (!(COND)) { \
> - if (time_after(jiffies, timeout__)) { \
> - ret__ = -ETIMEDOUT; \
> - break; \
> - } \
> - cpu_relax(); \
> - } \
> - ret__; \
> -})
> -
> #define wait_for(COND, MS) _wait_for(COND, MS, 1)
> #define wait_for_atomic(COND, MS) _wait_for(COND, MS, 0)
> +#define wait_for_atomic_us(COND, US) _wait_for((COND), \
> + usecs_to_jiffies((US)), 0)
usecs_to_jiffies() is wrong here. DIV_ROUND_UP((US), 1000) maybe,
or change _wait_for() to take the timeout in jiffies.
>
> #define KHz(x) (1000*x)
> #define MHz(x) KHz(1000*x)
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: fold wait_for_atomic_us into wait_for_atomic
2013-03-28 10:31 ` [PATCH] " Daniel Vetter
@ 2013-03-28 10:53 ` Ville Syrjälä
0 siblings, 0 replies; 7+ messages in thread
From: Ville Syrjälä @ 2013-03-28 10:53 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development, Jack Winter
On Thu, Mar 28, 2013 at 11:31:04AM +0100, Daniel Vetter wrote:
> Since
>
> commit bcf9dcc1e6269fac674e41f25d007ff75f76e840
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date: Sun Jul 15 09:42:38 2012 +0100
>
> drm/i915: Workaround hang with BSD and forcewake on SandyBridge
>
> and
>
> commit 0cc2764cc4a4bd73df55f8893c871778cf7ddd0f
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date: Sat Sep 1 22:59:48 2012 -0700
>
> drm/i915: use cpu_relax() in wait_for_atomic
>
> these two macros are essentially the same, so unify them. We keep the
> _us version since it's a nice documentation for smaller timeouts.
>
> v2: Fixup time unit conversion, _wait_for takes ms (Ville).
>
> Cc: Jack Winter <jbh@alchemy.lu>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_drv.h | 15 ++-------------
> 1 file changed, 2 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 54bc2ea..8e76f52f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -50,21 +50,10 @@
> ret__; \
> })
>
> -#define wait_for_atomic_us(COND, US) ({ \
> - unsigned long timeout__ = jiffies + usecs_to_jiffies(US); \
> - int ret__ = 0; \
> - while (!(COND)) { \
> - if (time_after(jiffies, timeout__)) { \
> - ret__ = -ETIMEDOUT; \
> - break; \
> - } \
> - cpu_relax(); \
> - } \
> - ret__; \
> -})
> -
> #define wait_for(COND, MS) _wait_for(COND, MS, 1)
> #define wait_for_atomic(COND, MS) _wait_for(COND, MS, 0)
> +#define wait_for_atomic_us(COND, US) _wait_for((COND), \
> + DIV_ROUND_UP((US), 1000), 0)
>
> #define KHz(x) (1000*x)
> #define MHz(x) KHz(1000*x)
> --
> 1.7.11.7
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm/i915: fix up _wait_for macro
2013-03-27 23:03 ` [PATCH 2/2] drm/i915: fix up _wait_for macro Daniel Vetter
@ 2013-03-28 11:00 ` Ville Syrjälä
2013-03-28 15:41 ` Daniel Vetter
0 siblings, 1 reply; 7+ messages in thread
From: Ville Syrjälä @ 2013-03-28 11:00 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development, Thomas Gleixner, Jack Winter
On Thu, Mar 28, 2013 at 12:03:25AM +0100, Daniel Vetter wrote:
> As Thomas Gleixner spotted, it's rather horrible racy:
> - We can miss almost a full tick, so need to compensate by 1 jiffy.
I have a feeling this is a rather common pattern, so I wonder
if [mu]secs_to_jiffies() should do the +1 already for everyone.
Or maybe there should be some other macros for specifying
timeouts that would do the +1.
> - We need to re-check the condition when having timed-out, since a
> the last check could have been before the timeout expired. E.g. when
> we've been preempted or a long irq happened.
>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Reported-by: Jack Winter <jbh@alchemy.lu>
> Cc: Jack Winter <jbh@alchemy.lu>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_drv.h | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index c8c1979..9dcae4e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -33,12 +33,21 @@
> #include <drm/drm_fb_helper.h>
> #include <drm/drm_dp_helper.h>
>
> +/**
> + * _wait_for - magic (register) wait macro
> + *
> + * Does the right thing for modeset paths when run under kdgb or similar atomic
> + * contexts. Note that it's important that we check the condition again after
> + * having timed out, since the timeout could be due to preemption or similar and
> + * we've never had a chance to check the condition before the timeout.
> + */
> #define _wait_for(COND, MS, W) ({ \
> - unsigned long timeout__ = jiffies + msecs_to_jiffies(MS); \
> + unsigned long timeout__ = jiffies + msecs_to_jiffies(MS) + 1; \
> int ret__ = 0; \
> while (!(COND)) { \
> if (time_after(jiffies, timeout__)) { \
> - ret__ = -ETIMEDOUT; \
> + if (!(COND)) \
> + ret__ = -ETIMEDOUT; \
> break; \
> } \
> if (W && drm_can_sleep()) { \
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drm/i915: fix up _wait_for macro
2013-03-28 11:00 ` Ville Syrjälä
@ 2013-03-28 15:41 ` Daniel Vetter
0 siblings, 0 replies; 7+ messages in thread
From: Daniel Vetter @ 2013-03-28 15:41 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Daniel Vetter, Intel Graphics Development, Thomas Gleixner,
Jack Winter
On Thu, Mar 28, 2013 at 01:00:56PM +0200, Ville Syrjälä wrote:
> On Thu, Mar 28, 2013 at 12:03:25AM +0100, Daniel Vetter wrote:
> > As Thomas Gleixner spotted, it's rather horrible racy:
> > - We can miss almost a full tick, so need to compensate by 1 jiffy.
>
> I have a feeling this is a rather common pattern, so I wonder
> if [mu]secs_to_jiffies() should do the +1 already for everyone.
> Or maybe there should be some other macros for specifying
> timeouts that would do the +1.
>
> > - We need to re-check the condition when having timed-out, since a
> > the last check could have been before the timeout expired. E.g. when
> > we've been preempted or a long irq happened.
> >
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Reported-by: Jack Winter <jbh@alchemy.lu>
> > Cc: Jack Winter <jbh@alchemy.lu>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Merged both patches, thanks for the review.
-Daniel
>
> > ---
> > drivers/gpu/drm/i915/intel_drv.h | 13 +++++++++++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index c8c1979..9dcae4e 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -33,12 +33,21 @@
> > #include <drm/drm_fb_helper.h>
> > #include <drm/drm_dp_helper.h>
> >
> > +/**
> > + * _wait_for - magic (register) wait macro
> > + *
> > + * Does the right thing for modeset paths when run under kdgb or similar atomic
> > + * contexts. Note that it's important that we check the condition again after
> > + * having timed out, since the timeout could be due to preemption or similar and
> > + * we've never had a chance to check the condition before the timeout.
> > + */
> > #define _wait_for(COND, MS, W) ({ \
> > - unsigned long timeout__ = jiffies + msecs_to_jiffies(MS); \
> > + unsigned long timeout__ = jiffies + msecs_to_jiffies(MS) + 1; \
> > int ret__ = 0; \
> > while (!(COND)) { \
> > if (time_after(jiffies, timeout__)) { \
> > - ret__ = -ETIMEDOUT; \
> > + if (!(COND)) \
> > + ret__ = -ETIMEDOUT; \
> > break; \
> > } \
> > if (W && drm_can_sleep()) { \
> > --
> > 1.7.11.7
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-03-28 15:38 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-03-27 23:03 [PATCH 1/2] drm/i915: fold wait_for_atomic_us into wait_for_atomic Daniel Vetter
2013-03-27 23:03 ` [PATCH 2/2] drm/i915: fix up _wait_for macro Daniel Vetter
2013-03-28 11:00 ` Ville Syrjälä
2013-03-28 15:41 ` Daniel Vetter
2013-03-28 10:32 ` [PATCH 1/2] drm/i915: fold wait_for_atomic_us into wait_for_atomic Ville Syrjälä
2013-03-28 10:31 ` [PATCH] " Daniel Vetter
2013-03-28 10:53 ` Ville Syrjälä
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox