Skip to contentSkip to conversation

drm/i915: _wait_for might be called when irq is off

RFC In review [email protected] 2 participants 2 messages Apr 2, 2015

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

RFC 1/1 drm/i915: _wait_for might be called when irq is off

From: Deepak S <[email protected]>

Sometimes, i915 might call _wait_for when irq is disabled. If the cpu is the main cpu to process jiffies, jiffies wouldn't be increased as this cpu disables irq. Then, time_after(jiffies, timeout__) becomes meaningless. If gunit doesn't work now, kernel wouldn't exit as the timeout doesn't work.

The patch fixes it by using sched_clock instead of jiffies.

---
 drivers/gpu/drm/i915/intel_drv.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/intel_drv.h +4 -2
index 6036e3b..2c6ebce 100644
@@ -49,10 +49,12 @@
  * 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) + 1;	\
+	u64 timeout_ = sched_clock() + MS * ((u64) NSEC_PER_MSEC);	\
+	u64 clock;							\
 	int ret__ = 0;							\
 	while (!(COND)) {						\
-		if (time_after(jiffies, timeout__)) {			\
+		clock = sched_clock();					\
+		if (clock >= timeout_) {				\
 			if (!(COND))					\
 				ret__ = -ETIMEDOUT;			\
 			break;						\
Signature · 6 lines
1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
#1 lore raw
Quoted 10 lines · On Thu, Apr 02, 2015 at 07:21:40PM +0530, [email protected] wrote:

From: Deepak S <[email protected]>

Sometimes, i915 might call _wait_for when irq is disabled. If the cpu is the main cpu to process jiffies, jiffies wouldn't be increased as this cpu disables irq. Then, time_after(jiffies, timeout__) becomes meaningless. If gunit doesn't work now, kernel wouldn't exit as the timeout doesn't work.

The patch fixes it by using sched_clock instead of jiffies.

sched_clock() requires irq disabled, or at least so the header claims, at the very least it would require preemption disabled - definitely not for our general waits of many ms. Also local_clock() would seem to be the right choice in these tight loops.

I think you want a specialised macro (if any) that is very aware of the constraints it is running under. I would hope we never have to busy spin with interrupts disabled. And so I want such code that does to blatantly obvious and scrutinised carefully.
-Chris

Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx