Re: [Intel-gfx] [PATCH 2/3] drm/i915: Change forcewake timeout to 2ms
On 2012-08-26 23:59, Jani Nikula wrote: On Fri, 24 Aug 2012, Ben Widawsky b...@bwidawsk.net 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 b...@bwidawsk.net --- 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. Nice catch. Ben DRM_ERROR(Force wake wait timed out\n); I915_WRITE_NOTRACE(FORCEWAKE, 1); POSTING_READ(FORCEWAKE); - if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) 1), - FORCEWAKE_ACK_TIMEOUT_US)) + if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) 1), + FORCEWAKE_ACK_TIMEOUT_MS)) DRM_ERROR(Force wake wait timed out\n); __gen6_gt_wait_for_thread_c0(dev_priv); @@ -3993,15 +3993,15 @@ static void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv) else forcewake_ack = FORCEWAKE_MT_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)) DRM_ERROR(Force wake wait timed out\n); I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_ENABLE(1)); POSTING_READ(FORCEWAKE_MT); - if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) 1), - FORCEWAKE_ACK_TIMEOUT_US)) + if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) 1), + FORCEWAKE_ACK_TIMEOUT_MS)) DRM_ERROR(Force wake wait timed out\n); __gen6_gt_wait_for_thread_c0(dev_priv); @@ -4088,8 +4088,8 @@ static void vlv_force_wake_get(struct drm_i915_private *dev_priv) I915_WRITE_NOTRACE(FORCEWAKE_VLV, 0x); POSTING_READ(FORCEWAKE_VLV); - if (wait_for_atomic_us((I915_READ_NOTRACE(FORCEWAKE_ACK_VLV) 1), - FORCEWAKE_ACK_TIMEOUT_US)) + if (wait_for_atomic((I915_READ_NOTRACE(FORCEWAKE_ACK_VLV) 1), + FORCEWAKE_ACK_TIMEOUT_MS)) DRM_ERROR(Force wake wait timed out\n); __gen6_gt_wait_for_thread_c0(dev_priv); -- 1.7.12 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] drm/i915: Change forcewake timeout to 2ms
On Tue, Aug 28, 2012 at 5:51 PM, Ben Widawsky b...@bwidawsk.net wrote: On 2012-08-26 23:59, Jani Nikula wrote: On Fri, 24 Aug 2012, Ben Widawsky b...@bwidawsk.net 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 b...@bwidawsk.net --- 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 -- Daniel Vetter daniel.vet...@ffwll.ch - +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
Re: [Intel-gfx] [PATCH 2/3] drm/i915: Change forcewake timeout to 2ms
On 2012-08-28 09:00, Daniel Vetter wrote: On Tue, Aug 28, 2012 at 5:51 PM, Ben Widawsky b...@bwidawsk.net wrote: On 2012-08-26 23:59, Jani Nikula wrote: On Fri, 24 Aug 2012, Ben Widawsky b...@bwidawsk.net 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 b...@bwidawsk.net --- 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 Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] drm/i915: Change forcewake timeout to 2ms
On Tue, Aug 28, 2012 at 6:07 PM, Ben Widawsky b...@bwidawsk.net wrote: On 2012-08-28 09:00, Daniel Vetter wrote: On Tue, Aug 28, 2012 at 5:51 PM, Ben Widawsky b...@bwidawsk.net wrote: On 2012-08-26 23:59, Jani Nikula wrote: On Fri, 24 Aug 2012, Ben Widawsky b...@bwidawsk.net 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 b...@bwidawsk.net --- 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? If what you're planing is to fix up wait_for_atomic to look like wait_for_us and the ditch the _us variant, yep, acked. -Daniel -- Daniel Vetter daniel.vet...@ffwll.ch - +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
Re: [Intel-gfx] [PATCH 2/3] drm/i915: Change forcewake timeout to 2ms
On Fri, 24 Aug 2012, Ben Widawsky b...@bwidawsk.net 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 b...@bwidawsk.net --- 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. DRM_ERROR(Force wake wait timed out\n); I915_WRITE_NOTRACE(FORCEWAKE, 1); POSTING_READ(FORCEWAKE); - if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) 1), -FORCEWAKE_ACK_TIMEOUT_US)) + if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) 1), + FORCEWAKE_ACK_TIMEOUT_MS)) DRM_ERROR(Force wake wait timed out\n); __gen6_gt_wait_for_thread_c0(dev_priv); @@ -3993,15 +3993,15 @@ static void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv) else forcewake_ack = FORCEWAKE_MT_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)) DRM_ERROR(Force wake wait timed out\n); I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_ENABLE(1)); POSTING_READ(FORCEWAKE_MT); - if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) 1), -FORCEWAKE_ACK_TIMEOUT_US)) + if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) 1), + FORCEWAKE_ACK_TIMEOUT_MS)) DRM_ERROR(Force wake wait timed out\n); __gen6_gt_wait_for_thread_c0(dev_priv); @@ -4088,8 +4088,8 @@ static void vlv_force_wake_get(struct drm_i915_private *dev_priv) I915_WRITE_NOTRACE(FORCEWAKE_VLV, 0x); POSTING_READ(FORCEWAKE_VLV); - if (wait_for_atomic_us((I915_READ_NOTRACE(FORCEWAKE_ACK_VLV) 1), -FORCEWAKE_ACK_TIMEOUT_US)) + if (wait_for_atomic((I915_READ_NOTRACE(FORCEWAKE_ACK_VLV) 1), + FORCEWAKE_ACK_TIMEOUT_MS)) DRM_ERROR(Force wake wait timed out\n); __gen6_gt_wait_for_thread_c0(dev_priv); -- 1.7.12 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/3] drm/i915: Change forcewake timeout to 2ms
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 b...@bwidawsk.net --- 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)) DRM_ERROR(Force wake wait timed out\n); I915_WRITE_NOTRACE(FORCEWAKE, 1); POSTING_READ(FORCEWAKE); - if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) 1), - FORCEWAKE_ACK_TIMEOUT_US)) + if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) 1), + FORCEWAKE_ACK_TIMEOUT_MS)) DRM_ERROR(Force wake wait timed out\n); __gen6_gt_wait_for_thread_c0(dev_priv); @@ -3993,15 +3993,15 @@ static void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv) else forcewake_ack = FORCEWAKE_MT_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)) DRM_ERROR(Force wake wait timed out\n); I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_ENABLE(1)); POSTING_READ(FORCEWAKE_MT); - if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) 1), - FORCEWAKE_ACK_TIMEOUT_US)) + if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) 1), + FORCEWAKE_ACK_TIMEOUT_MS)) DRM_ERROR(Force wake wait timed out\n); __gen6_gt_wait_for_thread_c0(dev_priv); @@ -4088,8 +4088,8 @@ static void vlv_force_wake_get(struct drm_i915_private *dev_priv) I915_WRITE_NOTRACE(FORCEWAKE_VLV, 0x); POSTING_READ(FORCEWAKE_VLV); - if (wait_for_atomic_us((I915_READ_NOTRACE(FORCEWAKE_ACK_VLV) 1), - FORCEWAKE_ACK_TIMEOUT_US)) + if (wait_for_atomic((I915_READ_NOTRACE(FORCEWAKE_ACK_VLV) 1), + FORCEWAKE_ACK_TIMEOUT_MS)) DRM_ERROR(Force wake wait timed out\n); __gen6_gt_wait_for_thread_c0(dev_priv); -- 1.7.12 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
