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

From: Daniel Vetter
Date: Tue Dec 05 2017 - 12:13:23 EST

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@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx>

Patches 1&2:

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

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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch