* [PATCH 1/3] drm/i915: Fix type of timeout_ms parameter in intel_wait_for_register_fw()
@ 2017-04-07 13:32 Michal Wajdeczko
2017-04-07 13:32 ` [PATCH 2/3] drm/i915: Add hint to intel_wait_for_register_fw() Michal Wajdeczko
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Michal Wajdeczko @ 2017-04-07 13:32 UTC (permalink / raw)
To: intel-gfx
There is no need to specify timeout as unsigned long since this parameter
will be consumed by usecs_to_jiffies() which expects unsigned int only.
Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 2 +-
drivers/gpu/drm/i915/intel_uncore.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c9b0949..41188d6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3098,7 +3098,7 @@ int intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
i915_reg_t reg,
const u32 mask,
const u32 value,
- const unsigned long timeout_ms);
+ const unsigned int timeout_ms);
static inline bool intel_gvt_active(struct drm_i915_private *dev_priv)
{
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 6d1ea26..bcabf54 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1610,7 +1610,7 @@ int intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
i915_reg_t reg,
const u32 mask,
const u32 value,
- const unsigned long timeout_ms)
+ const unsigned int timeout_ms)
{
#define done ((I915_READ_FW(reg) & mask) == value)
int ret = wait_for_us(done, 2);
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] drm/i915: Add hint to intel_wait_for_register_fw()
2017-04-07 13:32 [PATCH 1/3] drm/i915: Fix type of timeout_ms parameter in intel_wait_for_register_fw() Michal Wajdeczko
@ 2017-04-07 13:32 ` Michal Wajdeczko
2017-04-07 13:58 ` Tvrtko Ursulin
2017-04-07 14:18 ` Chris Wilson
2017-04-07 13:32 ` [PATCH 3/3] drm/i915/guc: Use intel_wait_for_register_fw() while sending over MMIO Michal Wajdeczko
` (2 subsequent siblings)
3 siblings, 2 replies; 12+ messages in thread
From: Michal Wajdeczko @ 2017-04-07 13:32 UTC (permalink / raw)
To: intel-gfx
In some cases we may want to spend more time in atomic wait than
hardcoded 2us. Let's add additional hint parameter to allow extending
internal atomic timeout to 10us before switching into heavy wait.
Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/intel_i2c.c | 2 +-
drivers/gpu/drm/i915/intel_pm.c | 4 ++--
drivers/gpu/drm/i915/intel_ringbuffer.c | 4 ++--
drivers/gpu/drm/i915/intel_uncore.c | 12 +++++++++---
5 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 41188d6..6f17517 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3098,6 +3098,7 @@ int intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
i915_reg_t reg,
const u32 mask,
const u32 value,
+ bool is_fast,
const unsigned int timeout_ms);
static inline bool intel_gvt_active(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index b6401e8..6753229 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -299,7 +299,7 @@ gmbus_wait_idle(struct drm_i915_private *dev_priv)
ret = intel_wait_for_register_fw(dev_priv,
GMBUS2, GMBUS_ACTIVE, 0,
- 10);
+ true, 10);
I915_WRITE_FW(GMBUS4, 0);
remove_wait_queue(&dev_priv->gmbus_wait_queue, &wait);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 55e1e88..f7efaca 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -8137,7 +8137,7 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
if (intel_wait_for_register_fw(dev_priv,
GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
- 500)) {
+ true, 500)) {
DRM_ERROR("timeout waiting for pcode read (%d) to finish\n", mbox);
return -ETIMEDOUT;
}
@@ -8182,7 +8182,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
if (intel_wait_for_register_fw(dev_priv,
GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
- 500)) {
+ true, 500)) {
DRM_ERROR("timeout waiting for pcode write (%d) to finish\n", mbox);
return -ETIMEDOUT;
}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index c98acc2..be649cf 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -540,7 +540,7 @@ static int init_ring_common(struct intel_engine_cs *engine)
/* If the head is still not zero, the ring is dead */
if (intel_wait_for_register_fw(dev_priv, RING_CTL(engine->mmio_base),
RING_VALID, RING_VALID,
- 50)) {
+ true, 50)) {
DRM_ERROR("%s initialization failed "
"ctl %08x (valid? %d) head %08x [%08x] tail %08x [%08x] start %08x [expected %08x]\n",
engine->name,
@@ -1733,7 +1733,7 @@ static void gen6_bsd_submit_request(struct drm_i915_gem_request *request)
GEN6_BSD_SLEEP_PSMI_CONTROL,
GEN6_BSD_SLEEP_INDICATOR,
0,
- 50))
+ true, 50))
DRM_ERROR("timed out waiting for the BSD ring to wake up\n");
/* Now that the ring is fully powered up, update the tail */
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index bcabf54..324a758 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1537,7 +1537,7 @@ static int gen6_hw_domain_reset(struct drm_i915_private *dev_priv,
/* Spin waiting for the device to ack the reset requests */
return intel_wait_for_register_fw(dev_priv,
GEN6_GDRST, hw_domain_mask, 0,
- 500);
+ true, 500);
}
/**
@@ -1590,6 +1590,7 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
* @reg: the register to read
* @mask: mask to apply to register value
* @value: expected value
+ * @is_fast: hint that it is expected to be a fast match
* @timeout_ms: timeout in millisecond
*
* This routine waits until the target register @reg contains the expected
@@ -1610,10 +1611,15 @@ int intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
i915_reg_t reg,
const u32 mask,
const u32 value,
+ bool is_fast,
const unsigned int timeout_ms)
{
#define done ((I915_READ_FW(reg) & mask) == value)
- int ret = wait_for_us(done, 2);
+ int ret;
+ if (is_fast)
+ ret = wait_for_us(done, 2);
+ else
+ ret = wait_for_us(done, 10);
if (ret)
ret = wait_for(done, timeout_ms);
return ret;
@@ -1670,7 +1676,7 @@ static int gen8_request_engine_reset(struct intel_engine_cs *engine)
RING_RESET_CTL(engine->mmio_base),
RESET_CTL_READY_TO_RESET,
RESET_CTL_READY_TO_RESET,
- 700);
+ true, 700);
if (ret)
DRM_ERROR("%s: reset request timeout\n", engine->name);
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/3] drm/i915/guc: Use intel_wait_for_register_fw() while sending over MMIO
2017-04-07 13:32 [PATCH 1/3] drm/i915: Fix type of timeout_ms parameter in intel_wait_for_register_fw() Michal Wajdeczko
2017-04-07 13:32 ` [PATCH 2/3] drm/i915: Add hint to intel_wait_for_register_fw() Michal Wajdeczko
@ 2017-04-07 13:32 ` Michal Wajdeczko
2017-04-07 14:27 ` Chris Wilson
2017-04-07 13:51 ` ✗ Fi.CI.BAT: warning for series starting with [1/3] drm/i915: Fix type of timeout_ms parameter in intel_wait_for_register_fw() Patchwork
2017-04-07 13:52 ` [PATCH 1/3] " Tvrtko Ursulin
3 siblings, 1 reply; 12+ messages in thread
From: Michal Wajdeczko @ 2017-04-07 13:32 UTC (permalink / raw)
To: intel-gfx
Waiting for the response status in scratch register can be done
using our generic function that waits for the expected register
state. Since completion of the GuC commands can take longer than
2us mark that wait as bit slower to extend atomic wait to 10us.
Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/intel_uc.c | 27 ++++++++-------------------
1 file changed, 8 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index c117424..432b540 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -360,19 +360,6 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
}
/*
- * Read GuC command/status register (SOFT_SCRATCH_0)
- * Return true if it contains a response rather than a command
- */
-static bool guc_recv(struct intel_guc *guc, u32 *status)
-{
- struct drm_i915_private *dev_priv = guc_to_i915(guc);
-
- u32 val = I915_READ(SOFT_SCRATCH(0));
- *status = val;
- return INTEL_GUC_RECV_IS_RESPONSE(val);
-}
-
-/*
* This function implements the MMIO based host to GuC interface.
*/
int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
@@ -399,13 +386,15 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
I915_WRITE(GUC_SEND_INTERRUPT, GUC_SEND_TRIGGER);
/*
- * Fast commands should complete in less than 10us, so sample quickly
- * up to that length of time, then switch to a slower sleep-wait loop.
- * No inte_guc_send command should ever take longer than 10ms.
+ * No GuC command should ever take longer than 10ms.
+ * Fast commands should still complete in 10us.
*/
- ret = wait_for_us(guc_recv(guc, &status), 10);
- if (ret)
- ret = wait_for(guc_recv(guc, &status), 10);
+ ret = intel_wait_for_register_fw(dev_priv,
+ SOFT_SCRATCH(0),
+ INTEL_GUC_RECV_MASK,
+ INTEL_GUC_RECV_MASK,
+ false, 10);
+ status = I915_READ(SOFT_SCRATCH(0));
if (status != INTEL_GUC_STATUS_SUCCESS) {
/*
* Either the GuC explicitly returned an error (which
--
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 12+ messages in thread
* ✗ Fi.CI.BAT: warning for series starting with [1/3] drm/i915: Fix type of timeout_ms parameter in intel_wait_for_register_fw()
2017-04-07 13:32 [PATCH 1/3] drm/i915: Fix type of timeout_ms parameter in intel_wait_for_register_fw() Michal Wajdeczko
2017-04-07 13:32 ` [PATCH 2/3] drm/i915: Add hint to intel_wait_for_register_fw() Michal Wajdeczko
2017-04-07 13:32 ` [PATCH 3/3] drm/i915/guc: Use intel_wait_for_register_fw() while sending over MMIO Michal Wajdeczko
@ 2017-04-07 13:51 ` Patchwork
2017-04-07 13:52 ` [PATCH 1/3] " Tvrtko Ursulin
3 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2017-04-07 13:51 UTC (permalink / raw)
To: Michal Wajdeczko; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/3] drm/i915: Fix type of timeout_ms parameter in intel_wait_for_register_fw()
URL : https://patchwork.freedesktop.org/series/22669/
State : warning
== Summary ==
Series 22669v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/22669/revisions/1/mbox/
Test gem_exec_suspend:
Subgroup basic-s4-devices:
dmesg-warn -> PASS (fi-kbl-7560u) fdo#100125
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-c:
pass -> DMESG-WARN (fi-bsw-n3050)
fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125
fi-bdw-gvtdvm total:278 pass:255 dwarn:8 dfail:0 fail:1 skip:14 time:426s
fi-bsw-n3050 total:278 pass:240 dwarn:1 dfail:0 fail:1 skip:36 time:569s
fi-bxt-j4205 total:278 pass:258 dwarn:0 dfail:0 fail:1 skip:19 time:509s
fi-bxt-t5700 total:278 pass:257 dwarn:0 dfail:0 fail:1 skip:20 time:539s
fi-byt-j1900 total:278 pass:253 dwarn:0 dfail:0 fail:1 skip:24 time:488s
fi-byt-n2820 total:278 pass:249 dwarn:0 dfail:0 fail:1 skip:28 time:478s
fi-hsw-4770 total:278 pass:261 dwarn:0 dfail:0 fail:1 skip:16 time:412s
fi-hsw-4770r total:278 pass:261 dwarn:0 dfail:0 fail:1 skip:16 time:410s
fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:1 skip:49 time:419s
fi-ivb-3520m total:278 pass:259 dwarn:0 dfail:0 fail:1 skip:18 time:492s
fi-ivb-3770 total:278 pass:259 dwarn:0 dfail:0 fail:1 skip:18 time:466s
fi-kbl-7500u total:278 pass:259 dwarn:0 dfail:0 fail:1 skip:18 time:453s
fi-kbl-7560u total:278 pass:267 dwarn:0 dfail:0 fail:1 skip:10 time:563s
fi-skl-6260u total:278 pass:267 dwarn:0 dfail:0 fail:1 skip:10 time:452s
fi-skl-6700hq total:278 pass:260 dwarn:0 dfail:0 fail:1 skip:17 time:569s
fi-skl-6700k total:278 pass:255 dwarn:4 dfail:0 fail:1 skip:18 time:468s
fi-skl-6770hq total:278 pass:267 dwarn:0 dfail:0 fail:1 skip:10 time:494s
fi-skl-gvtdvm total:278 pass:264 dwarn:0 dfail:0 fail:1 skip:13 time:432s
fi-snb-2520m total:278 pass:249 dwarn:0 dfail:0 fail:1 skip:28 time:530s
fi-snb-2600 total:278 pass:248 dwarn:0 dfail:0 fail:1 skip:29 time:398s
c5d424760e9a5d40f7024591d5e69f074711e7ff drm-tip: 2017y-04m-07d-12h-40m-43s UTC integration manifest
f5a3028 drm/i915/guc: Use intel_wait_for_register_fw() while sending over MMIO
c13a8fe drm/i915: Add hint to intel_wait_for_register_fw()
019362b drm/i915: Fix type of timeout_ms parameter in intel_wait_for_register_fw()
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4440/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] drm/i915: Fix type of timeout_ms parameter in intel_wait_for_register_fw()
2017-04-07 13:32 [PATCH 1/3] drm/i915: Fix type of timeout_ms parameter in intel_wait_for_register_fw() Michal Wajdeczko
` (2 preceding siblings ...)
2017-04-07 13:51 ` ✗ Fi.CI.BAT: warning for series starting with [1/3] drm/i915: Fix type of timeout_ms parameter in intel_wait_for_register_fw() Patchwork
@ 2017-04-07 13:52 ` Tvrtko Ursulin
2017-04-07 15:00 ` Chris Wilson
3 siblings, 1 reply; 12+ messages in thread
From: Tvrtko Ursulin @ 2017-04-07 13:52 UTC (permalink / raw)
To: Michal Wajdeczko, intel-gfx
On 07/04/2017 14:32, Michal Wajdeczko wrote:
> There is no need to specify timeout as unsigned long since this parameter
> will be consumed by usecs_to_jiffies() which expects unsigned int only.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 2 +-
> drivers/gpu/drm/i915/intel_uncore.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c9b0949..41188d6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3098,7 +3098,7 @@ int intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
> i915_reg_t reg,
> const u32 mask,
> const u32 value,
> - const unsigned long timeout_ms);
> + const unsigned int timeout_ms);
>
> static inline bool intel_gvt_active(struct drm_i915_private *dev_priv)
> {
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 6d1ea26..bcabf54 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1610,7 +1610,7 @@ int intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
> i915_reg_t reg,
> const u32 mask,
> const u32 value,
> - const unsigned long timeout_ms)
> + const unsigned int timeout_ms)
> {
> #define done ((I915_READ_FW(reg) & mask) == value)
> int ret = wait_for_us(done, 2);
>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] drm/i915: Add hint to intel_wait_for_register_fw()
2017-04-07 13:32 ` [PATCH 2/3] drm/i915: Add hint to intel_wait_for_register_fw() Michal Wajdeczko
@ 2017-04-07 13:58 ` Tvrtko Ursulin
2017-04-07 14:10 ` Michal Wajdeczko
2017-04-07 14:18 ` Chris Wilson
1 sibling, 1 reply; 12+ messages in thread
From: Tvrtko Ursulin @ 2017-04-07 13:58 UTC (permalink / raw)
To: Michal Wajdeczko, intel-gfx
On 07/04/2017 14:32, Michal Wajdeczko wrote:
> In some cases we may want to spend more time in atomic wait than
> hardcoded 2us. Let's add additional hint parameter to allow extending
> internal atomic timeout to 10us before switching into heavy wait.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/intel_i2c.c | 2 +-
> drivers/gpu/drm/i915/intel_pm.c | 4 ++--
> drivers/gpu/drm/i915/intel_ringbuffer.c | 4 ++--
> drivers/gpu/drm/i915/intel_uncore.c | 12 +++++++++---
> 5 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 41188d6..6f17517 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3098,6 +3098,7 @@ int intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
> i915_reg_t reg,
> const u32 mask,
> const u32 value,
> + bool is_fast,
> const unsigned int timeout_ms);
>
> static inline bool intel_gvt_active(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index b6401e8..6753229 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -299,7 +299,7 @@ gmbus_wait_idle(struct drm_i915_private *dev_priv)
>
> ret = intel_wait_for_register_fw(dev_priv,
> GMBUS2, GMBUS_ACTIVE, 0,
> - 10);
> + true, 10);
>
> I915_WRITE_FW(GMBUS4, 0);
> remove_wait_queue(&dev_priv->gmbus_wait_queue, &wait);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 55e1e88..f7efaca 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -8137,7 +8137,7 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
>
> if (intel_wait_for_register_fw(dev_priv,
> GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
> - 500)) {
> + true, 500)) {
> DRM_ERROR("timeout waiting for pcode read (%d) to finish\n", mbox);
> return -ETIMEDOUT;
> }
> @@ -8182,7 +8182,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
>
> if (intel_wait_for_register_fw(dev_priv,
> GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
> - 500)) {
> + true, 500)) {
> DRM_ERROR("timeout waiting for pcode write (%d) to finish\n", mbox);
> return -ETIMEDOUT;
> }
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index c98acc2..be649cf 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -540,7 +540,7 @@ static int init_ring_common(struct intel_engine_cs *engine)
> /* If the head is still not zero, the ring is dead */
> if (intel_wait_for_register_fw(dev_priv, RING_CTL(engine->mmio_base),
> RING_VALID, RING_VALID,
> - 50)) {
> + true, 50)) {
> DRM_ERROR("%s initialization failed "
> "ctl %08x (valid? %d) head %08x [%08x] tail %08x [%08x] start %08x [expected %08x]\n",
> engine->name,
> @@ -1733,7 +1733,7 @@ static void gen6_bsd_submit_request(struct drm_i915_gem_request *request)
> GEN6_BSD_SLEEP_PSMI_CONTROL,
> GEN6_BSD_SLEEP_INDICATOR,
> 0,
> - 50))
> + true, 50))
> DRM_ERROR("timed out waiting for the BSD ring to wake up\n");
>
> /* Now that the ring is fully powered up, update the tail */
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index bcabf54..324a758 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1537,7 +1537,7 @@ static int gen6_hw_domain_reset(struct drm_i915_private *dev_priv,
> /* Spin waiting for the device to ack the reset requests */
> return intel_wait_for_register_fw(dev_priv,
> GEN6_GDRST, hw_domain_mask, 0,
> - 500);
> + true, 500);
> }
>
> /**
> @@ -1590,6 +1590,7 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
> * @reg: the register to read
> * @mask: mask to apply to register value
> * @value: expected value
> + * @is_fast: hint that it is expected to be a fast match
> * @timeout_ms: timeout in millisecond
> *
> * This routine waits until the target register @reg contains the expected
> @@ -1610,10 +1611,15 @@ int intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
> i915_reg_t reg,
> const u32 mask,
> const u32 value,
> + bool is_fast,
> const unsigned int timeout_ms)
> {
> #define done ((I915_READ_FW(reg) & mask) == value)
> - int ret = wait_for_us(done, 2);
> + int ret;
> + if (is_fast)
> + ret = wait_for_us(done, 2);
> + else
> + ret = wait_for_us(done, 10);
> if (ret)
> ret = wait_for(done, timeout_ms);
> return ret;
> @@ -1670,7 +1676,7 @@ static int gen8_request_engine_reset(struct intel_engine_cs *engine)
> RING_RESET_CTL(engine->mmio_base),
> RESET_CTL_READY_TO_RESET,
> RESET_CTL_READY_TO_RESET,
> - 700);
> + true, 700);
> if (ret)
> DRM_ERROR("%s: reset request timeout\n", engine->name);
>
>
I would recommend a different solution here.
Rename and change the prototype of intel_wait_for_register_fw to:
int __intel_wait_for_register_fw(dev_priv, reg, mask, vlaue, fast_timeout_us, timeout_ms)
{
..existing function body, just replace "2" with fast_timeout_us...
}
int intel_wait_for_register_fw(dev_priv, reg, mask, vlaue, timeout_ms
{
return __intel_wait_for_register_fw(dev_priv, reg, mask, value, 2, timeout_ms);
}
And use the __ version from the GuC code.
There is no churn elsewhere in the code like this and it is also
more flexible for potential other new users.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] drm/i915: Add hint to intel_wait_for_register_fw()
2017-04-07 13:58 ` Tvrtko Ursulin
@ 2017-04-07 14:10 ` Michal Wajdeczko
2017-04-07 14:23 ` Chris Wilson
2017-04-07 15:05 ` Tvrtko Ursulin
0 siblings, 2 replies; 12+ messages in thread
From: Michal Wajdeczko @ 2017-04-07 14:10 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
On Fri, Apr 07, 2017 at 02:58:17PM +0100, Tvrtko Ursulin wrote:
>
> On 07/04/2017 14:32, Michal Wajdeczko wrote:
> > In some cases we may want to spend more time in atomic wait than
> > hardcoded 2us. Let's add additional hint parameter to allow extending
> > internal atomic timeout to 10us before switching into heavy wait.
> >
> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > drivers/gpu/drm/i915/intel_i2c.c | 2 +-
> > drivers/gpu/drm/i915/intel_pm.c | 4 ++--
> > drivers/gpu/drm/i915/intel_ringbuffer.c | 4 ++--
> > drivers/gpu/drm/i915/intel_uncore.c | 12 +++++++++---
> > 5 files changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 41188d6..6f17517 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3098,6 +3098,7 @@ int intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
> > i915_reg_t reg,
> > const u32 mask,
> > const u32 value,
> > + bool is_fast,
> > const unsigned int timeout_ms);
> >
> > static inline bool intel_gvt_active(struct drm_i915_private *dev_priv)
> > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> > index b6401e8..6753229 100644
> > --- a/drivers/gpu/drm/i915/intel_i2c.c
> > +++ b/drivers/gpu/drm/i915/intel_i2c.c
> > @@ -299,7 +299,7 @@ gmbus_wait_idle(struct drm_i915_private *dev_priv)
> >
> > ret = intel_wait_for_register_fw(dev_priv,
> > GMBUS2, GMBUS_ACTIVE, 0,
> > - 10);
> > + true, 10);
> >
> > I915_WRITE_FW(GMBUS4, 0);
> > remove_wait_queue(&dev_priv->gmbus_wait_queue, &wait);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 55e1e88..f7efaca 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -8137,7 +8137,7 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
> >
> > if (intel_wait_for_register_fw(dev_priv,
> > GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
> > - 500)) {
> > + true, 500)) {
> > DRM_ERROR("timeout waiting for pcode read (%d) to finish\n", mbox);
> > return -ETIMEDOUT;
> > }
> > @@ -8182,7 +8182,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
> >
> > if (intel_wait_for_register_fw(dev_priv,
> > GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
> > - 500)) {
> > + true, 500)) {
> > DRM_ERROR("timeout waiting for pcode write (%d) to finish\n", mbox);
> > return -ETIMEDOUT;
> > }
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index c98acc2..be649cf 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -540,7 +540,7 @@ static int init_ring_common(struct intel_engine_cs *engine)
> > /* If the head is still not zero, the ring is dead */
> > if (intel_wait_for_register_fw(dev_priv, RING_CTL(engine->mmio_base),
> > RING_VALID, RING_VALID,
> > - 50)) {
> > + true, 50)) {
> > DRM_ERROR("%s initialization failed "
> > "ctl %08x (valid? %d) head %08x [%08x] tail %08x [%08x] start %08x [expected %08x]\n",
> > engine->name,
> > @@ -1733,7 +1733,7 @@ static void gen6_bsd_submit_request(struct drm_i915_gem_request *request)
> > GEN6_BSD_SLEEP_PSMI_CONTROL,
> > GEN6_BSD_SLEEP_INDICATOR,
> > 0,
> > - 50))
> > + true, 50))
> > DRM_ERROR("timed out waiting for the BSD ring to wake up\n");
> >
> > /* Now that the ring is fully powered up, update the tail */
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index bcabf54..324a758 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -1537,7 +1537,7 @@ static int gen6_hw_domain_reset(struct drm_i915_private *dev_priv,
> > /* Spin waiting for the device to ack the reset requests */
> > return intel_wait_for_register_fw(dev_priv,
> > GEN6_GDRST, hw_domain_mask, 0,
> > - 500);
> > + true, 500);
> > }
> >
> > /**
> > @@ -1590,6 +1590,7 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
> > * @reg: the register to read
> > * @mask: mask to apply to register value
> > * @value: expected value
> > + * @is_fast: hint that it is expected to be a fast match
> > * @timeout_ms: timeout in millisecond
> > *
> > * This routine waits until the target register @reg contains the expected
> > @@ -1610,10 +1611,15 @@ int intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
> > i915_reg_t reg,
> > const u32 mask,
> > const u32 value,
> > + bool is_fast,
> > const unsigned int timeout_ms)
> > {
> > #define done ((I915_READ_FW(reg) & mask) == value)
> > - int ret = wait_for_us(done, 2);
> > + int ret;
> > + if (is_fast)
> > + ret = wait_for_us(done, 2);
> > + else
> > + ret = wait_for_us(done, 10);
> > if (ret)
> > ret = wait_for(done, timeout_ms);
> > return ret;
> > @@ -1670,7 +1676,7 @@ static int gen8_request_engine_reset(struct intel_engine_cs *engine)
> > RING_RESET_CTL(engine->mmio_base),
> > RESET_CTL_READY_TO_RESET,
> > RESET_CTL_READY_TO_RESET,
> > - 700);
> > + true, 700);
> > if (ret)
> > DRM_ERROR("%s: reset request timeout\n", engine->name);
> >
> >
>
> I would recommend a different solution here.
>
> Rename and change the prototype of intel_wait_for_register_fw to:
>
> int __intel_wait_for_register_fw(dev_priv, reg, mask, vlaue, fast_timeout_us, timeout_ms)
> {
> ..existing function body, just replace "2" with fast_timeout_us...
> }
>
> int intel_wait_for_register_fw(dev_priv, reg, mask, vlaue, timeout_ms
> {
> return __intel_wait_for_register_fw(dev_priv, reg, mask, value, 2, timeout_ms);
> }
>
> And use the __ version from the GuC code.
>
> There is no churn elsewhere in the code like this and it is also
> more flexible for potential other new users.
That was my initial approach, *but* this would require further changes,
as wait_for_us() expects timeout parameter to be compile time constant.
#define wait_for_us(COND, US) \
...
BUILD_BUG_ON(!__builtin_constant_p(US));
Possible alternate solutions were:
a) use internal macro _wait_for_atomic() directly with explicit ATOMIC=0
b) add another variant of wait_for_atomic_us() that will use ATOMIC=0
but I'm not sure that we need to be that flexible in our wait function,
as the only range for expected fast timeout is bound to 1..10us anyway.
Hence my idea to just provide simple hint that covers two fast timeouts
values 2us and 10us.
-Michal
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] drm/i915: Add hint to intel_wait_for_register_fw()
2017-04-07 13:32 ` [PATCH 2/3] drm/i915: Add hint to intel_wait_for_register_fw() Michal Wajdeczko
2017-04-07 13:58 ` Tvrtko Ursulin
@ 2017-04-07 14:18 ` Chris Wilson
1 sibling, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2017-04-07 14:18 UTC (permalink / raw)
To: Michal Wajdeczko; +Cc: intel-gfx
On Fri, Apr 07, 2017 at 01:32:11PM +0000, Michal Wajdeczko wrote:
> In some cases we may want to spend more time in atomic wait than
> hardcoded 2us. Let's add additional hint parameter to allow extending
> internal atomic timeout to 10us before switching into heavy wait.
Examples? I know the 2us is arbitrary, it's picked as a small enough
value that hopefully will escape notice by the RT crew and friends, and
based on the experience that hw usually gets is right after the second
or third read.
I would rather pass in the fast_timeout_us, slow_timeout_ms. If we are
being sensible about reducing the policy at the lowest level, lets do it
a bit more completely and push it as far up the callchain as we chain.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] drm/i915: Add hint to intel_wait_for_register_fw()
2017-04-07 14:10 ` Michal Wajdeczko
@ 2017-04-07 14:23 ` Chris Wilson
2017-04-07 15:05 ` Tvrtko Ursulin
1 sibling, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2017-04-07 14:23 UTC (permalink / raw)
To: Michal Wajdeczko; +Cc: intel-gfx
On Fri, Apr 07, 2017 at 04:10:29PM +0200, Michal Wajdeczko wrote:
> On Fri, Apr 07, 2017 at 02:58:17PM +0100, Tvrtko Ursulin wrote:
> >
> > On 07/04/2017 14:32, Michal Wajdeczko wrote:
> > > In some cases we may want to spend more time in atomic wait than
> > > hardcoded 2us. Let's add additional hint parameter to allow extending
> > > internal atomic timeout to 10us before switching into heavy wait.
> > >
> > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > > @@ -1670,7 +1676,7 @@ static int gen8_request_engine_reset(struct intel_engine_cs *engine)
> > > RING_RESET_CTL(engine->mmio_base),
> > > RESET_CTL_READY_TO_RESET,
> > > RESET_CTL_READY_TO_RESET,
> > > - 700);
> > > + true, 700);
> > > if (ret)
> > > DRM_ERROR("%s: reset request timeout\n", engine->name);
> > >
> > >
> >
> > I would recommend a different solution here.
> >
> > Rename and change the prototype of intel_wait_for_register_fw to:
> >
> > int __intel_wait_for_register_fw(dev_priv, reg, mask, vlaue, fast_timeout_us, timeout_ms)
> > {
> > ..existing function body, just replace "2" with fast_timeout_us...
> > }
> >
> > int intel_wait_for_register_fw(dev_priv, reg, mask, vlaue, timeout_ms
> > {
> > return __intel_wait_for_register_fw(dev_priv, reg, mask, value, 2, timeout_ms);
> > }
> >
> > And use the __ version from the GuC code.
> >
> > There is no churn elsewhere in the code like this and it is also
> > more flexible for potential other new users.
>
> That was my initial approach, *but* this would require further changes,
> as wait_for_us() expects timeout parameter to be compile time constant.
Hmm.
> #define wait_for_us(COND, US) \
> ...
> BUILD_BUG_ON(!__builtin_constant_p(US));
>
> Possible alternate solutions were:
> a) use internal macro _wait_for_atomic() directly with explicit ATOMIC=0
> b) add another variant of wait_for_atomic_us() that will use ATOMIC=0
I'd vote to reducing the number of wait_for_us we have in the driver,
ideally limiting it to intel_uncore.c as the only user. Given how slow
mmio reads are, there's no good excuse against having an extra function
call. Is that achievable?
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] drm/i915/guc: Use intel_wait_for_register_fw() while sending over MMIO
2017-04-07 13:32 ` [PATCH 3/3] drm/i915/guc: Use intel_wait_for_register_fw() while sending over MMIO Michal Wajdeczko
@ 2017-04-07 14:27 ` Chris Wilson
0 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2017-04-07 14:27 UTC (permalink / raw)
To: Michal Wajdeczko; +Cc: intel-gfx
On Fri, Apr 07, 2017 at 01:32:12PM +0000, Michal Wajdeczko wrote:
> Waiting for the response status in scratch register can be done
> using our generic function that waits for the expected register
> state. Since completion of the GuC commands can take longer than
> 2us mark that wait as bit slower to extend atomic wait to 10us.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/intel_uc.c | 27 ++++++++-------------------
> 1 file changed, 8 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index c117424..432b540 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -360,19 +360,6 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
> }
>
> /*
> - * Read GuC command/status register (SOFT_SCRATCH_0)
> - * Return true if it contains a response rather than a command
> - */
> -static bool guc_recv(struct intel_guc *guc, u32 *status)
> -{
> - struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -
> - u32 val = I915_READ(SOFT_SCRATCH(0));
> - *status = val;
> - return INTEL_GUC_RECV_IS_RESPONSE(val);
> -}
> -
> -/*
> * This function implements the MMIO based host to GuC interface.
> */
> int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
> @@ -399,13 +386,15 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len)
> I915_WRITE(GUC_SEND_INTERRUPT, GUC_SEND_TRIGGER);
>
> /*
> - * Fast commands should complete in less than 10us, so sample quickly
> - * up to that length of time, then switch to a slower sleep-wait loop.
> - * No inte_guc_send command should ever take longer than 10ms.
> + * No GuC command should ever take longer than 10ms.
> + * Fast commands should still complete in 10us.
> */
> - ret = wait_for_us(guc_recv(guc, &status), 10);
> - if (ret)
> - ret = wait_for(guc_recv(guc, &status), 10);
> + ret = intel_wait_for_register_fw(dev_priv,
> + SOFT_SCRATCH(0),
> + INTEL_GUC_RECV_MASK,
> + INTEL_GUC_RECV_MASK,
> + false, 10);
> + status = I915_READ(SOFT_SCRATCH(0));
Reporting the final register value is common -- I think most of the
remaining wait_for_us() holdouts are because they wanted to interpret
the value after the loop. I wonder if adding an out param for the value
to wait_for_register would cover the remaining missing users?
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] drm/i915: Fix type of timeout_ms parameter in intel_wait_for_register_fw()
2017-04-07 13:52 ` [PATCH 1/3] " Tvrtko Ursulin
@ 2017-04-07 15:00 ` Chris Wilson
0 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2017-04-07 15:00 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
On Fri, Apr 07, 2017 at 02:52:25PM +0100, Tvrtko Ursulin wrote:
>
> On 07/04/2017 14:32, Michal Wajdeczko wrote:
> >There is no need to specify timeout as unsigned long since this parameter
> >will be consumed by usecs_to_jiffies() which expects unsigned int only.
> >
> >Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >---
> > drivers/gpu/drm/i915/i915_drv.h | 2 +-
> > drivers/gpu/drm/i915/intel_uncore.c | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >index c9b0949..41188d6 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >@@ -3098,7 +3098,7 @@ int intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
> > i915_reg_t reg,
> > const u32 mask,
> > const u32 value,
> >- const unsigned long timeout_ms);
> >+ const unsigned int timeout_ms);
> >
> > static inline bool intel_gvt_active(struct drm_i915_private *dev_priv)
> > {
> >diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> >index 6d1ea26..bcabf54 100644
> >--- a/drivers/gpu/drm/i915/intel_uncore.c
> >+++ b/drivers/gpu/drm/i915/intel_uncore.c
> >@@ -1610,7 +1610,7 @@ int intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
> > i915_reg_t reg,
> > const u32 mask,
> > const u32 value,
> >- const unsigned long timeout_ms)
> >+ const unsigned int timeout_ms)
> > {
> > #define done ((I915_READ_FW(reg) & mask) == value)
> > int ret = wait_for_us(done, 2);
> >
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Pushed this patch an excuse to kick CI. Thanks!
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] drm/i915: Add hint to intel_wait_for_register_fw()
2017-04-07 14:10 ` Michal Wajdeczko
2017-04-07 14:23 ` Chris Wilson
@ 2017-04-07 15:05 ` Tvrtko Ursulin
1 sibling, 0 replies; 12+ messages in thread
From: Tvrtko Ursulin @ 2017-04-07 15:05 UTC (permalink / raw)
To: Michal Wajdeczko; +Cc: intel-gfx
On 07/04/2017 15:10, Michal Wajdeczko wrote:
> On Fri, Apr 07, 2017 at 02:58:17PM +0100, Tvrtko Ursulin wrote:
>>
>> On 07/04/2017 14:32, Michal Wajdeczko wrote:
>>> In some cases we may want to spend more time in atomic wait than
>>> hardcoded 2us. Let's add additional hint parameter to allow extending
>>> internal atomic timeout to 10us before switching into heavy wait.
>>>
>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> ---
>>> drivers/gpu/drm/i915/i915_drv.h | 1 +
>>> drivers/gpu/drm/i915/intel_i2c.c | 2 +-
>>> drivers/gpu/drm/i915/intel_pm.c | 4 ++--
>>> drivers/gpu/drm/i915/intel_ringbuffer.c | 4 ++--
>>> drivers/gpu/drm/i915/intel_uncore.c | 12 +++++++++---
>>> 5 files changed, 15 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 41188d6..6f17517 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -3098,6 +3098,7 @@ int intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
>>> i915_reg_t reg,
>>> const u32 mask,
>>> const u32 value,
>>> + bool is_fast,
>>> const unsigned int timeout_ms);
>>>
>>> static inline bool intel_gvt_active(struct drm_i915_private *dev_priv)
>>> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
>>> index b6401e8..6753229 100644
>>> --- a/drivers/gpu/drm/i915/intel_i2c.c
>>> +++ b/drivers/gpu/drm/i915/intel_i2c.c
>>> @@ -299,7 +299,7 @@ gmbus_wait_idle(struct drm_i915_private *dev_priv)
>>>
>>> ret = intel_wait_for_register_fw(dev_priv,
>>> GMBUS2, GMBUS_ACTIVE, 0,
>>> - 10);
>>> + true, 10);
>>>
>>> I915_WRITE_FW(GMBUS4, 0);
>>> remove_wait_queue(&dev_priv->gmbus_wait_queue, &wait);
>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>> index 55e1e88..f7efaca 100644
>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>> @@ -8137,7 +8137,7 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
>>>
>>> if (intel_wait_for_register_fw(dev_priv,
>>> GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
>>> - 500)) {
>>> + true, 500)) {
>>> DRM_ERROR("timeout waiting for pcode read (%d) to finish\n", mbox);
>>> return -ETIMEDOUT;
>>> }
>>> @@ -8182,7 +8182,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
>>>
>>> if (intel_wait_for_register_fw(dev_priv,
>>> GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
>>> - 500)) {
>>> + true, 500)) {
>>> DRM_ERROR("timeout waiting for pcode write (%d) to finish\n", mbox);
>>> return -ETIMEDOUT;
>>> }
>>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>>> index c98acc2..be649cf 100644
>>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>>> @@ -540,7 +540,7 @@ static int init_ring_common(struct intel_engine_cs *engine)
>>> /* If the head is still not zero, the ring is dead */
>>> if (intel_wait_for_register_fw(dev_priv, RING_CTL(engine->mmio_base),
>>> RING_VALID, RING_VALID,
>>> - 50)) {
>>> + true, 50)) {
>>> DRM_ERROR("%s initialization failed "
>>> "ctl %08x (valid? %d) head %08x [%08x] tail %08x [%08x] start %08x [expected %08x]\n",
>>> engine->name,
>>> @@ -1733,7 +1733,7 @@ static void gen6_bsd_submit_request(struct drm_i915_gem_request *request)
>>> GEN6_BSD_SLEEP_PSMI_CONTROL,
>>> GEN6_BSD_SLEEP_INDICATOR,
>>> 0,
>>> - 50))
>>> + true, 50))
>>> DRM_ERROR("timed out waiting for the BSD ring to wake up\n");
>>>
>>> /* Now that the ring is fully powered up, update the tail */
>>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>>> index bcabf54..324a758 100644
>>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>>> @@ -1537,7 +1537,7 @@ static int gen6_hw_domain_reset(struct drm_i915_private *dev_priv,
>>> /* Spin waiting for the device to ack the reset requests */
>>> return intel_wait_for_register_fw(dev_priv,
>>> GEN6_GDRST, hw_domain_mask, 0,
>>> - 500);
>>> + true, 500);
>>> }
>>>
>>> /**
>>> @@ -1590,6 +1590,7 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
>>> * @reg: the register to read
>>> * @mask: mask to apply to register value
>>> * @value: expected value
>>> + * @is_fast: hint that it is expected to be a fast match
>>> * @timeout_ms: timeout in millisecond
>>> *
>>> * This routine waits until the target register @reg contains the expected
>>> @@ -1610,10 +1611,15 @@ int intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
>>> i915_reg_t reg,
>>> const u32 mask,
>>> const u32 value,
>>> + bool is_fast,
>>> const unsigned int timeout_ms)
>>> {
>>> #define done ((I915_READ_FW(reg) & mask) == value)
>>> - int ret = wait_for_us(done, 2);
>>> + int ret;
>>> + if (is_fast)
>>> + ret = wait_for_us(done, 2);
>>> + else
>>> + ret = wait_for_us(done, 10);
>>> if (ret)
>>> ret = wait_for(done, timeout_ms);
>>> return ret;
>>> @@ -1670,7 +1676,7 @@ static int gen8_request_engine_reset(struct intel_engine_cs *engine)
>>> RING_RESET_CTL(engine->mmio_base),
>>> RESET_CTL_READY_TO_RESET,
>>> RESET_CTL_READY_TO_RESET,
>>> - 700);
>>> + true, 700);
>>> if (ret)
>>> DRM_ERROR("%s: reset request timeout\n", engine->name);
>>>
>>>
>>
>> I would recommend a different solution here.
>>
>> Rename and change the prototype of intel_wait_for_register_fw to:
>>
>> int __intel_wait_for_register_fw(dev_priv, reg, mask, vlaue, fast_timeout_us, timeout_ms)
>> {
>> ..existing function body, just replace "2" with fast_timeout_us...
>> }
>>
>> int intel_wait_for_register_fw(dev_priv, reg, mask, vlaue, timeout_ms
>> {
>> return __intel_wait_for_register_fw(dev_priv, reg, mask, value, 2, timeout_ms);
>> }
>>
>> And use the __ version from the GuC code.
>>
>> There is no churn elsewhere in the code like this and it is also
>> more flexible for potential other new users.
>
> That was my initial approach, *but* this would require further changes,
> as wait_for_us() expects timeout parameter to be compile time constant.
>
> #define wait_for_us(COND, US) \
> ...
> BUILD_BUG_ON(!__builtin_constant_p(US));
>
> Possible alternate solutions were:
> a) use internal macro _wait_for_atomic() directly with explicit ATOMIC=0
> b) add another variant of wait_for_atomic_us() that will use ATOMIC=0
>
> but I'm not sure that we need to be that flexible in our wait function,
> as the only range for expected fast timeout is bound to 1..10us anyway.
>
> Hence my idea to just provide simple hint that covers two fast timeouts
> values 2us and 10us.
I forgot about that detail. :( I have no ideas at the moment other than
moving the double underscore version to a header file as static inline.
But that would cost some text size so I don't know.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-04-07 15:05 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-07 13:32 [PATCH 1/3] drm/i915: Fix type of timeout_ms parameter in intel_wait_for_register_fw() Michal Wajdeczko
2017-04-07 13:32 ` [PATCH 2/3] drm/i915: Add hint to intel_wait_for_register_fw() Michal Wajdeczko
2017-04-07 13:58 ` Tvrtko Ursulin
2017-04-07 14:10 ` Michal Wajdeczko
2017-04-07 14:23 ` Chris Wilson
2017-04-07 15:05 ` Tvrtko Ursulin
2017-04-07 14:18 ` Chris Wilson
2017-04-07 13:32 ` [PATCH 3/3] drm/i915/guc: Use intel_wait_for_register_fw() while sending over MMIO Michal Wajdeczko
2017-04-07 14:27 ` Chris Wilson
2017-04-07 13:51 ` ✗ Fi.CI.BAT: warning for series starting with [1/3] drm/i915: Fix type of timeout_ms parameter in intel_wait_for_register_fw() Patchwork
2017-04-07 13:52 ` [PATCH 1/3] " Tvrtko Ursulin
2017-04-07 15:00 ` Chris Wilson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.