[PATCH 1/3] drm/i915: Fix type of timeout_ms parameter in intel_wait_for_register_fw()

[PATCH 1/3] drm/i915: Fix type of timeout_ms parameter in intel_wait_for_register_fw()

[Michal Wajdeczko]

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

[PATCH 2/3] drm/i915: Add hint to intel_wait_for_register_fw()

[Michal Wajdeczko]

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

[PATCH 3/3] drm/i915/guc: Use intel_wait_for_register_fw() while sending over MMIO

[Michal Wajdeczko]

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

✗ 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]

== 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

Re: [PATCH 1/3] drm/i915: Fix type of timeout_ms parameter in intel_wait_for_register_fw()

[Tvrtko Ursulin]

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

Re: [PATCH 2/3] drm/i915: Add hint to intel_wait_for_register_fw()

[Tvrtko Ursulin]

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

Re: [PATCH 2/3] drm/i915: Add hint to intel_wait_for_register_fw()

[Michal Wajdeczko]

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

Re: [PATCH 2/3] drm/i915: Add hint to intel_wait_for_register_fw()

[Chris Wilson]

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

Re: [PATCH 2/3] drm/i915: Add hint to intel_wait_for_register_fw()

[Chris Wilson]

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

Re: [PATCH 3/3] drm/i915/guc: Use intel_wait_for_register_fw() while sending over MMIO

[Chris Wilson]

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

Re: [PATCH 1/3] drm/i915: Fix type of timeout_ms parameter in intel_wait_for_register_fw()

[Chris Wilson]

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

Re: [PATCH 2/3] drm/i915: Add hint to intel_wait_for_register_fw()

[Tvrtko Ursulin]

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