Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751918AbdLARsm (ORCPT ); Fri, 1 Dec 2017 12:48:42 -0500 Received: from mail-it0-f54.google.com ([209.85.214.54]:38196 "EHLO mail-it0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751157AbdLARsk (ORCPT ); Fri, 1 Dec 2017 12:48:40 -0500 X-Google-Smtp-Source: AGs4zMZMnPfZ141+G3Wd4Dd07DSY8wE0MfowiInQiECQS+y8FCacs+JSMfJ/iiEE+OkLBJpVLnsoaw== MIME-Version: 1.0 In-Reply-To: <151215024280.1324.2765501954695029499@mail.alporthouse.com> References: <20171201172032.47357-1-seanpaul@chromium.org> <20171201172032.47357-3-seanpaul@chromium.org> <151215024280.1324.2765501954695029499@mail.alporthouse.com> From: Sean Paul Date: Fri, 1 Dec 2017 12:48:17 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [Intel-gfx] [PATCH v2 2/8] drm/i915: Add more control to wait_for routines To: Chris Wilson Cc: dri-devel , Intel Graphics Development , David Airlie , Linux Kernel Mailing List , Rodrigo Vivi , Daniel Vetter Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5929 Lines: 124 On Fri, Dec 1, 2017 at 12:44 PM, Chris Wilson wrote: > 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). > Hmm, am I based off the wrong tree? I'm using drm-intel-next. > 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 :| > Probably uncommon, but at the very least, I need one. I wouldn't feel comfortable handling such a large wait using usleep_range. Sean >> } \ >> 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, ®_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