On 2012-08-28 09:00, Daniel Vetter wrote:
On Tue, Aug 28, 2012 at 5:51 PM, Ben Widawsky <[email protected]>
wrote:
On 2012-08-26 23:59, Jani Nikula wrote:
On Fri, 24 Aug 2012, Ben Widawsky <[email protected]> wrote:
A designer familiar with the hardware has stated that the
forcewake
timeout can theoretically be as high as a little over 1ms.
Therefore we
modify our code to use 2ms (appropriate fudge and because we don't
want
to round down).
Hopefully this can't prevent spurious timeouts.
Signed-off-by: Ben Widawsky <[email protected]>
---
drivers/gpu/drm/i915/intel_pm.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c
b/drivers/gpu/drm/i915/intel_pm.c
index f42c142..2a8468d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -31,7 +31,7 @@
#include "../../../platform/x86/intel_ips.h"
#include <linux/module.h>
-#define FORCEWAKE_ACK_TIMEOUT_US 500
+#define FORCEWAKE_ACK_TIMEOUT_MS 2
/* FBC, or Frame Buffer Compression, is a technique employed to
compress
the
* framebuffer contents in-memory, aiming at reducing the
required
bandwidth
@@ -3970,15 +3970,15 @@ static void
__gen6_gt_force_wake_get(struct
drm_i915_private *dev_priv)
else
forcewake_ack = FORCEWAKE_ACK;
- if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) &
1) ==
0,
- FORCEWAKE_ACK_TIMEOUT_US))
+ if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1)
== 0,
+ FORCEWAKE_ACK_TIMEOUT_MS))
Superficially this looks okay, but the implementation of
wait_for_atomic() not so. As a surprise, this change drops
cpu_relax()
from the busy loop, even thought the timeout is potentially much
longer.
The quick fix here would be to just use 2000 us with
wait_for_atomic_us(), but we should do something about
wait_for_atomic()
too. Luckily it's only ever used at one place.
BR,
Jani.
Hmm, dare I say, I think this is a bug in _wait_for. Without
spending too
much brain power on this, I believe the compiler can screw us over
here. No
matter the bug, cpu_relax is still probably desirable, unless there
is some
newer coolness here? I shall insert a patch before this to do the
cpu_relax
in _wait_for.
The original idea behind wiat_for_us was that we use udelay and
really
limit ourselves to that us timeout (since jiffies is too coarse). Now
that the timeout for forcewake is 2ms (gawk!) I think we can stop
bothering to pretend that this should timeout quickly and drop the
_us
variant (but still keep the cpu relax imo).
-Daniel
Unless I'm confused, you're acking what I was planning?
--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
