Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753142AbdLERNT (ORCPT ); Tue, 5 Dec 2017 12:13:19 -0500 Received: from mail-wm0-f68.google.com ([74.125.82.68]:42714 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752593AbdLERNO (ORCPT ); Tue, 5 Dec 2017 12:13:14 -0500 X-Google-Smtp-Source: AGs4zMadqQ0mUIjAXV1zrbXIF7FuOqomETymIGeoAlfOat9MyioArSkf7W15IIkcbVS7o7KHrS4+Uw== Date: Tue, 5 Dec 2017 18:13:09 +0100 From: Daniel Vetter To: Sean Paul Cc: dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, seanpaul@google.com, David Airlie , Joonas Lahtinen , linux-kernel@vger.kernel.org, Rodrigo Vivi , daniel.vetter@intel.com Subject: Re: [PATCH v3 2/9] drm/i915: Add more control to wait_for routines Message-ID: <20171205171309.iqbwao7nvivjvknd@phenom.ffwll.local> Mail-Followup-To: Sean Paul , dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, seanpaul@google.com, David Airlie , Joonas Lahtinen , linux-kernel@vger.kernel.org, Rodrigo Vivi , daniel.vetter@intel.com References: <20171205051513.8603-1-seanpaul@chromium.org> <20171205051513.8603-3-seanpaul@chromium.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171205051513.8603-3-seanpaul@chromium.org> X-Operating-System: Linux phenom 4.13.0-1-amd64 User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6804 Lines: 180 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 > Signed-off-by: Sean Paul Patches 1&2: Reviewed-by: Daniel Vetter 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 > > /** > - * _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, ®_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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch