linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Chris Wilson <chris@chris-wilson.co.uk>
To: Sean Paul <seanpaul@chromium.org>,
	dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org
Cc: "David Airlie" <airlied@linux.ie>,
	linux-kernel@vger.kernel.org,
	"Rodrigo Vivi" <rodrigo.vivi@intel.com>,
	daniel.vetter@intel.com
Subject: Re: [Intel-gfx] [PATCH v2 2/8] drm/i915: Add more control to wait_for routines
Date: Fri, 01 Dec 2017 17:44:02 +0000	[thread overview]
Message-ID: <151215024280.1324.2765501954695029499@mail.alporthouse.com> (raw)
In-Reply-To: <20171201172032.47357-3-seanpaul@chromium.org>

Quoting Sean Paul (2017-12-01 17:20:24)
>  /**
> - * _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
> + * Macro to help avoid open coding check/wait/timeout patterns, will do the
> + * right think wrt to choosing msleep vs usleep_range based on how long the wait
> + * interval is. 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, W) ({ \
> +#define __wait_for(OP, COND, US, W) ({ \
>         unsigned long timeout__ = jiffies + usecs_to_jiffies(US) + 1;   \
>         int ret__;                                                      \
>         might_sleep();                                                  \
>         for (;;) {                                                      \
>                 bool expired__ = time_after(jiffies, timeout__);        \
> +               OP;                                                     \
>                 if (COND) {                                             \
>                         ret__ = 0;                                      \
>                         break;                                          \
> @@ -62,11 +64,16 @@
>                         ret__ = -ETIMEDOUT;                             \
>                         break;                                          \
>                 }                                                       \
> -               usleep_range((W), (W) * 2);                             \
> +               if (W > (20 * 1000))                                    \
> +                       msleep(W / 1000);                               \
> +               else                                                    \
> +                       usleep_range((W), (W) * 2);                     \

The current wait_for() is a little more complicated nowadays (variable
W).

Are ms intervals going to be that common? Using a state-machine springs
to mind, but you could argue that msleep() is just a yield. Using msleep
though is going to leave D processes visible and a bump in load :|

>         }                                                               \
>         ret__;                                                          \
>  })
>  
> +#define _wait_for(COND, US, W)         __wait_for(;,(COND), US, W)
> +
>  #define wait_for(COND, MS)             _wait_for((COND), (MS) * 1000, 1000)
>  
>  /* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index b4621271e7a2..c851b0c0602d 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, 1000);
> +
> +       if (out_value)
> +               *out_value = reg_value;

Looks good.
-Chris

  reply	other threads:[~2017-12-01 17:44 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-01 17:20 [PATCH v2 0/8] drm/i915: Implement HDCP Sean Paul
2017-12-01 17:20 ` [PATCH v2 1/8] drm: Fix link-status kerneldoc line lengths Sean Paul
2017-12-01 17:20 ` [PATCH v2 2/8] drm/i915: Add more control to wait_for routines Sean Paul
2017-12-01 17:44   ` Chris Wilson [this message]
2017-12-01 17:48     ` [Intel-gfx] " Sean Paul
     [not found]       ` <151215091507.4852.2176019139113860843@mail.alporthouse.com>
     [not found]         ` <151215105956.4852.3393236663014165115@mail.alporthouse.com>
2017-12-01 18:00           ` Sean Paul
2017-12-01 17:20 ` [PATCH v2 3/8] drm: Add Content Protection property Sean Paul
2017-12-01 17:20 ` [PATCH v2 4/8] drm: Add some HDCP related #defines Sean Paul
2017-12-01 17:20 ` [PATCH v2 5/8] drm/i915: Add HDCP framework + base implementation Sean Paul
2017-12-01 17:20 ` [PATCH v2 6/8] drm/i915: Add function to output Aksv over GMBUS Sean Paul
2017-12-01 19:06   ` Ville Syrjälä
2017-12-01 19:17     ` Sean Paul
2017-12-01 20:03       ` Ville Syrjälä
2017-12-01 17:20 ` [PATCH v2 7/8] drm/i915: Implement HDCP for HDMI Sean Paul
2017-12-01 17:20 ` [PATCH v2 8/8] drm/i915: Implement HDCP for DisplayPort Sean Paul
2017-12-01 18:47 ` [PATCH v2 0/8] drm/i915: Implement HDCP Hans Verkuil
2017-12-01 18:58   ` Sean Paul

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=151215024280.1324.2765501954695029499@mail.alporthouse.com \
    --to=chris@chris-wilson.co.uk \
    --cc=airlied@linux.ie \
    --cc=daniel.vetter@intel.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rodrigo.vivi@intel.com \
    --cc=seanpaul@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).