Re: [Intel-gfx] [PATCH v2 2/8] drm/i915: Add more control to wait_for routines

[Sean Paul <seanpaul@chromium.org>]

On Fri, Dec 1, 2017 at 12:44 PM, Chris Wilson <chris@chris-wilson.co.uk> 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, &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