diff mbox

[v3,2/9] drm/i915: Add more control to wait_for routines

Message ID 20171205051513.8603-3-seanpaul@chromium.org (mailing list archive)
State New, archived
Headers show

Commit Message

Sean Paul Dec. 5, 2017, 5:15 a.m. UTC
This patch adds a little more control to a couple wait_for routines such
that we can avoid open-coding read/wait/timeout patterns which:
 - need the value of the register after the wait_for
 - run arbitrary operation for the read portion

This patch also chooses the correct sleep function (based on
timers-howto.txt) for the polling interval the caller specifies.

Changes in v2:
- Added to the series
Changes in v3:
- Rebased on drm-intel-next-queued and the new Wmin/max _wait_for
- Removed msleep option

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/i915/intel_drv.h    | 17 ++++++++++-------
 drivers/gpu/drm/i915/intel_uncore.c | 23 ++++++++++++++++-------
 drivers/gpu/drm/i915/intel_uncore.h | 14 +++++++++++++-
 3 files changed, 39 insertions(+), 15 deletions(-)

Comments

Daniel Vetter Dec. 5, 2017, 5:13 p.m. UTC | #1
On Tue, Dec 05, 2017 at 12:15:01AM -0500, Sean Paul wrote:
> This patch adds a little more control to a couple wait_for routines such
> that we can avoid open-coding read/wait/timeout patterns which:
>  - need the value of the register after the wait_for
>  - run arbitrary operation for the read portion
> 
> This patch also chooses the correct sleep function (based on
> timers-howto.txt) for the polling interval the caller specifies.
> 
> Changes in v2:
> - Added to the series
> Changes in v3:
> - Rebased on drm-intel-next-queued and the new Wmin/max _wait_for
> - Removed msleep option
> 
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>

Patches 1&2:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I can't find the dang docs for patch 3 ... needs a bit of digging or a
different victim.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_drv.h    | 17 ++++++++++-------
>  drivers/gpu/drm/i915/intel_uncore.c | 23 ++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_uncore.h | 14 +++++++++++++-
>  3 files changed, 39 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 64426d3e078e..852b3d161754 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -41,20 +41,21 @@
>  #include <drm/drm_atomic.h>
>  
>  /**
> - * _wait_for - magic (register) wait macro
> + * __wait_for - magic wait macro
>   *
> - * Does the right thing for modeset paths when run under kdgb or similar atomic
> - * contexts. Note that it's important that we check the condition again after
> - * having timed out, since the timeout could be due to preemption or similar and
> - * we've never had a chance to check the condition before the timeout.
> + * Macro to help avoid open coding check/wait/timeout patterns. Note that it's
> + * important that we check the condition again after having timed out, since the
> + * timeout could be due to preemption or similar and we've never had a chance to
> + * check the condition before the timeout.
>   */
> -#define _wait_for(COND, US, Wmin, Wmax) ({ \
> +#define __wait_for(OP, COND, US, Wmin, Wmax) ({ \
>  	unsigned long timeout__ = jiffies + usecs_to_jiffies(US) + 1;	\
>  	long wait__ = (Wmin); /* recommended min for usleep is 10 us */	\
>  	int ret__;							\
>  	might_sleep();							\
>  	for (;;) {							\
>  		bool expired__ = time_after(jiffies, timeout__);	\
> +		OP;							\
>  		if (COND) {						\
>  			ret__ = 0;					\
>  			break;						\
> @@ -70,7 +71,9 @@
>  	ret__;								\
>  })
>  
> -#define wait_for(COND, MS)	_wait_for((COND), (MS) * 1000, 10, 1000)
> +#define _wait_for(COND, US, Wmin, Wmax)	__wait_for(;, (COND), (US), (Wmin), \
> +						   (Wmax))
> +#define wait_for(COND, MS)		_wait_for((COND), (MS) * 1000, 10, 1000)
>  
>  /* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */
>  #if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT)
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index b4621271e7a2..9c7d07151f16 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1770,12 +1770,14 @@ int __intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
>  }
>  
>  /**
> - * intel_wait_for_register - wait until register matches expected state
> + * __intel_wait_for_register - wait until register matches expected state
>   * @dev_priv: the i915 device
>   * @reg: the register to read
>   * @mask: mask to apply to register value
>   * @value: expected value
> - * @timeout_ms: timeout in millisecond
> + * @fast_timeout_us: fast timeout in microsecond for atomic/tight wait
> + * @slow_timeout_ms: slow timeout in millisecond
> + * @out_value: optional placeholder to hold registry value
>   *
>   * This routine waits until the target register @reg contains the expected
>   * @value after applying the @mask, i.e. it waits until ::
> @@ -1786,15 +1788,18 @@ int __intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
>   *
>   * Returns 0 if the register matches the desired condition, or -ETIMEOUT.
>   */
> -int intel_wait_for_register(struct drm_i915_private *dev_priv,
> +int __intel_wait_for_register(struct drm_i915_private *dev_priv,
>  			    i915_reg_t reg,
>  			    u32 mask,
>  			    u32 value,
> -			    unsigned int timeout_ms)
> +			    unsigned int fast_timeout_us,
> +			    unsigned int slow_timeout_ms,
> +			    u32 *out_value)
>  {
>  	unsigned fw =
>  		intel_uncore_forcewake_for_reg(dev_priv, reg, FW_REG_READ);
>  	int ret;
> +	u32 reg_value;
>  
>  	might_sleep();
>  
> @@ -1803,14 +1808,18 @@ int intel_wait_for_register(struct drm_i915_private *dev_priv,
>  
>  	ret = __intel_wait_for_register_fw(dev_priv,
>  					   reg, mask, value,
> -					   2, 0, NULL);
> +					   fast_timeout_us, 0, &reg_value);
>  
>  	intel_uncore_forcewake_put__locked(dev_priv, fw);
>  	spin_unlock_irq(&dev_priv->uncore.lock);
>  
>  	if (ret)
> -		ret = wait_for((I915_READ_NOTRACE(reg) & mask) == value,
> -			       timeout_ms);
> +		ret = __wait_for(reg_value = I915_READ_NOTRACE(reg),
> +			         (reg_value & mask) == value,
> +			         slow_timeout_ms * 1000, 10, 1000);
> +
> +	if (out_value)
> +		*out_value = reg_value;
>  
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> index 9ce079b5dd0d..bed019ef000f 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.h
> +++ b/drivers/gpu/drm/i915/intel_uncore.h
> @@ -163,11 +163,23 @@ void intel_uncore_forcewake_put__locked(struct drm_i915_private *dev_priv,
>  void intel_uncore_forcewake_user_get(struct drm_i915_private *dev_priv);
>  void intel_uncore_forcewake_user_put(struct drm_i915_private *dev_priv);
>  
> +int __intel_wait_for_register(struct drm_i915_private *dev_priv,
> +			      i915_reg_t reg,
> +			      u32 mask,
> +			      u32 value,
> +			      unsigned int fast_timeout_us,
> +			      unsigned int slow_timeout_ms,
> +			      u32 *out_value);
> +static inline
>  int intel_wait_for_register(struct drm_i915_private *dev_priv,
>  			    i915_reg_t reg,
>  			    u32 mask,
>  			    u32 value,
> -			    unsigned int timeout_ms);
> +			    unsigned int timeout_ms)
> +{
> +	return __intel_wait_for_register(dev_priv, reg, mask, value, 2,
> +					 timeout_ms, NULL);
> +}
>  int __intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
>  				 i915_reg_t reg,
>  				 u32 mask,
> -- 
> 2.15.0.531.g2ccb3012c9-goog
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Chris Wilson Dec. 5, 2017, 11:09 p.m. UTC | #2
Quoting Sean Paul (2017-12-05 05:15:01)
> This patch adds a little more control to a couple wait_for routines such
> that we can avoid open-coding read/wait/timeout patterns which:
>  - need the value of the register after the wait_for
>  - run arbitrary operation for the read portion
> 
> This patch also chooses the correct sleep function (based on
> timers-howto.txt) for the polling interval the caller specifies.
> 
> Changes in v2:
> - Added to the series
> Changes in v3:
> - Rebased on drm-intel-next-queued and the new Wmin/max _wait_for
> - Removed msleep option
> 
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_drv.h    | 17 ++++++++++-------
>  drivers/gpu/drm/i915/intel_uncore.c | 23 ++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_uncore.h | 14 +++++++++++++-
>  3 files changed, 39 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 64426d3e078e..852b3d161754 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -41,20 +41,21 @@
>  #include <drm/drm_atomic.h>
>  
>  /**
> - * _wait_for - magic (register) wait macro
> + * __wait_for - magic wait macro
>   *
> - * Does the right thing for modeset paths when run under kdgb or similar atomic
> - * contexts. Note that it's important that we check the condition again after
> - * having timed out, since the timeout could be due to preemption or similar and
> - * we've never had a chance to check the condition before the timeout.
> + * Macro to help avoid open coding check/wait/timeout patterns. Note that it's
> + * important that we check the condition again after having timed out, since the
> + * timeout could be due to preemption or similar and we've never had a chance to
> + * check the condition before the timeout.
>   */
> -#define _wait_for(COND, US, Wmin, Wmax) ({ \
> +#define __wait_for(OP, COND, US, Wmin, Wmax) ({ \
>         unsigned long timeout__ = jiffies + usecs_to_jiffies(US) + 1;   \
>         long wait__ = (Wmin); /* recommended min for usleep is 10 us */ \
>         int ret__;                                                      \
>         might_sleep();                                                  \
>         for (;;) {                                                      \
>                 bool expired__ = time_after(jiffies, timeout__);        \
> +               OP;                                                     \
>                 if (COND) {                                             \
>                         ret__ = 0;                                      \
>                         break;                                          \
> @@ -70,7 +71,9 @@
>         ret__;                                                          \
>  })
>  
> -#define wait_for(COND, MS)     _wait_for((COND), (MS) * 1000, 10, 1000)
> +#define _wait_for(COND, US, Wmin, Wmax)        __wait_for(;, (COND), (US), (Wmin), \
> +                                                  (Wmax))

Hmm, doesn't an empty OP (__wait_for(, ...)) work?

> +int __intel_wait_for_register(struct drm_i915_private *dev_priv,
>                             i915_reg_t reg,
>                             u32 mask,
>                             u32 value,
> -                           unsigned int timeout_ms)
> +                           unsigned int fast_timeout_us,
> +                           unsigned int slow_timeout_ms,
> +                           u32 *out_value)
>  {
>         unsigned fw =
>                 intel_uncore_forcewake_for_reg(dev_priv, reg, FW_REG_READ);
>         int ret;
> +       u32 reg_value;

Before int ret; Try to avoid building a Christmas tree if possible.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 64426d3e078e..852b3d161754 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -41,20 +41,21 @@ 
 #include <drm/drm_atomic.h>
 
 /**
- * _wait_for - magic (register) wait macro
+ * __wait_for - magic wait macro
  *
- * Does the right thing for modeset paths when run under kdgb or similar atomic
- * contexts. Note that it's important that we check the condition again after
- * having timed out, since the timeout could be due to preemption or similar and
- * we've never had a chance to check the condition before the timeout.
+ * Macro to help avoid open coding check/wait/timeout patterns. Note that it's
+ * important that we check the condition again after having timed out, since the
+ * timeout could be due to preemption or similar and we've never had a chance to
+ * check the condition before the timeout.
  */
-#define _wait_for(COND, US, Wmin, Wmax) ({ \
+#define __wait_for(OP, COND, US, Wmin, Wmax) ({ \
 	unsigned long timeout__ = jiffies + usecs_to_jiffies(US) + 1;	\
 	long wait__ = (Wmin); /* recommended min for usleep is 10 us */	\
 	int ret__;							\
 	might_sleep();							\
 	for (;;) {							\
 		bool expired__ = time_after(jiffies, timeout__);	\
+		OP;							\
 		if (COND) {						\
 			ret__ = 0;					\
 			break;						\
@@ -70,7 +71,9 @@ 
 	ret__;								\
 })
 
-#define wait_for(COND, MS)	_wait_for((COND), (MS) * 1000, 10, 1000)
+#define _wait_for(COND, US, Wmin, Wmax)	__wait_for(;, (COND), (US), (Wmin), \
+						   (Wmax))
+#define wait_for(COND, MS)		_wait_for((COND), (MS) * 1000, 10, 1000)
 
 /* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */
 #if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index b4621271e7a2..9c7d07151f16 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1770,12 +1770,14 @@  int __intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
 }
 
 /**
- * intel_wait_for_register - wait until register matches expected state
+ * __intel_wait_for_register - wait until register matches expected state
  * @dev_priv: the i915 device
  * @reg: the register to read
  * @mask: mask to apply to register value
  * @value: expected value
- * @timeout_ms: timeout in millisecond
+ * @fast_timeout_us: fast timeout in microsecond for atomic/tight wait
+ * @slow_timeout_ms: slow timeout in millisecond
+ * @out_value: optional placeholder to hold registry value
  *
  * This routine waits until the target register @reg contains the expected
  * @value after applying the @mask, i.e. it waits until ::
@@ -1786,15 +1788,18 @@  int __intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
  *
  * Returns 0 if the register matches the desired condition, or -ETIMEOUT.
  */
-int intel_wait_for_register(struct drm_i915_private *dev_priv,
+int __intel_wait_for_register(struct drm_i915_private *dev_priv,
 			    i915_reg_t reg,
 			    u32 mask,
 			    u32 value,
-			    unsigned int timeout_ms)
+			    unsigned int fast_timeout_us,
+			    unsigned int slow_timeout_ms,
+			    u32 *out_value)
 {
 	unsigned fw =
 		intel_uncore_forcewake_for_reg(dev_priv, reg, FW_REG_READ);
 	int ret;
+	u32 reg_value;
 
 	might_sleep();
 
@@ -1803,14 +1808,18 @@  int intel_wait_for_register(struct drm_i915_private *dev_priv,
 
 	ret = __intel_wait_for_register_fw(dev_priv,
 					   reg, mask, value,
-					   2, 0, NULL);
+					   fast_timeout_us, 0, &reg_value);
 
 	intel_uncore_forcewake_put__locked(dev_priv, fw);
 	spin_unlock_irq(&dev_priv->uncore.lock);
 
 	if (ret)
-		ret = wait_for((I915_READ_NOTRACE(reg) & mask) == value,
-			       timeout_ms);
+		ret = __wait_for(reg_value = I915_READ_NOTRACE(reg),
+			         (reg_value & mask) == value,
+			         slow_timeout_ms * 1000, 10, 1000);
+
+	if (out_value)
+		*out_value = reg_value;
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
index 9ce079b5dd0d..bed019ef000f 100644
--- a/drivers/gpu/drm/i915/intel_uncore.h
+++ b/drivers/gpu/drm/i915/intel_uncore.h
@@ -163,11 +163,23 @@  void intel_uncore_forcewake_put__locked(struct drm_i915_private *dev_priv,
 void intel_uncore_forcewake_user_get(struct drm_i915_private *dev_priv);
 void intel_uncore_forcewake_user_put(struct drm_i915_private *dev_priv);
 
+int __intel_wait_for_register(struct drm_i915_private *dev_priv,
+			      i915_reg_t reg,
+			      u32 mask,
+			      u32 value,
+			      unsigned int fast_timeout_us,
+			      unsigned int slow_timeout_ms,
+			      u32 *out_value);
+static inline
 int intel_wait_for_register(struct drm_i915_private *dev_priv,
 			    i915_reg_t reg,
 			    u32 mask,
 			    u32 value,
-			    unsigned int timeout_ms);
+			    unsigned int timeout_ms)
+{
+	return __intel_wait_for_register(dev_priv, reg, mask, value, 2,
+					 timeout_ms, NULL);
+}
 int __intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
 				 i915_reg_t reg,
 				 u32 mask,