[Intel-gfx] [PATCH v2 2/8] drm/i915: Add more control to wait_for routines
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 Suggested-by: Chris Wilson Signed-off-by: Sean Paul --- 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, 41 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 69aab324aaa1..191c80fc4314 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -41,19 +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 + * 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); \ } \ 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, r
Re: [Intel-gfx] [PATCH v2 2/8] drm/i915: Add more control to wait_for routines
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,
Re: [Intel-gfx] [PATCH v2 2/8] drm/i915: Add more control to wait_for routines
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; >> >>
Re: [Intel-gfx] [PATCH v2 2/8] drm/i915: Add more control to wait_for routines
Quoting Sean Paul (2017-12-01 17:48:17) > 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. drm-intel-next is what was sent as a PR; drm-intel-next-queued is always current. To be sure CI, doesn't complain about merge conflicts, base on drm-tip. > > 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. We can use the constants to remove the predicate for when it never will be needed, so it's not an issue -- either wait will produce a long uninterruptible sleep. It's just if that we were to frequently hit that long sleep, there would be some push back against using wait_for() on that path as no one likes an "idle" system with load 1. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 2/8] drm/i915: Add more control to wait_for routines
Quoting Chris Wilson (2017-12-01 17:55:15) > Quoting Sean Paul (2017-12-01 17:48:17) > > On Fri, Dec 1, 2017 at 12:44 PM, Chris Wilson > > wrote: > > > 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. > > drm-intel-next is what was sent as a PR; drm-intel-next-queued is always > current. To be sure CI, doesn't complain about merge conflicts, base on > drm-tip. Speaking of CI, do you have any instructions on how we might set up a test system? Just needs a compatible monitor and some test code? Could chamelium or something like that be used as a validator? -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 2/8] drm/i915: Add more control to wait_for routines
On Fri, Dec 1, 2017 at 12:57 PM, Chris Wilson wrote: > Quoting Chris Wilson (2017-12-01 17:55:15) >> Quoting Sean Paul (2017-12-01 17:48:17) >> > On Fri, Dec 1, 2017 at 12:44 PM, Chris Wilson >> > wrote: >> > > 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. >> >> drm-intel-next is what was sent as a PR; drm-intel-next-queued is always >> current. To be sure CI, doesn't complain about merge conflicts, base on >> drm-tip. > A, i forgot about -queued. ok, will rebase. > Speaking of CI, do you have any instructions on how we might set up a > test system? I'm working on an igt test for the property now. > Just needs a compatible monitor and some test code? Yep. For testing, I exposed the property via sysfs and fiddle with it that way. > Could chamelium or something like that be used as a validator? You would have to implement the receiver side of HDCP on chamelium in order for this to work. So, probably not. Sean > -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx