Re: [Intel-gfx] [PATCH 2/2] drm/i915: fix up _wait_for macro
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 t...@linutronix.de
Reported-by: Jack Winter j...@alchemy.lu
Cc: Jack Winter j...@alchemy.lu
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
Reviewed-by: Ville Syrjälä ville.syrj...@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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: fix up _wait_for macro
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 t...@linutronix.de
Reported-by: Jack Winter j...@alchemy.lu
Cc: Jack Winter j...@alchemy.lu
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
Reviewed-by: Ville Syrjälä ville.syrj...@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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm/i915: fix up _wait_for macro
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 t...@linutronix.de
Reported-by: Jack Winter j...@alchemy.lu
Cc: Jack Winter j...@alchemy.lu
Signed-off-by: Daniel Vetter daniel.vet...@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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
