Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 8BIT
To: Sean Paul <seanpaul@chromium.org>, dri-devel@lists.freedesktop.org,
        intel-gfx@lists.freedesktop.org
From: Chris Wilson <chris@chris-wilson.co.uk>
In-Reply-To: <20171205051513.8603-3-seanpaul@chromium.org>
Cc: seanpaul@google.com, "David Airlie" <airlied@linux.ie>,
        linux-kernel@vger.kernel.org, "Rodrigo Vivi" <rodrigo.vivi@intel.com>,
        daniel.vetter@intel.com
References: <20171205051513.8603-1-seanpaul@chromium.org>
 <20171205051513.8603-3-seanpaul@chromium.org>
Message-ID: <151251534148.3497.565078431241355506@mail.alporthouse.com>
User-Agent: alot/0.3.6
Subject: Re: [Intel-gfx] [PATCH v3 2/9] drm/i915: Add more control to wait_for
 routines
Date: Tue, 05 Dec 2017 23:09:01 +0000
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3861
Lines: 83

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