[Intel-gfx] [PATCH 2/3] drm/i915: Add hint to intel_wait_for_register_fw()
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Fri Apr 7 15:05:32 UTC 2017
On 07/04/2017 15:10, Michal Wajdeczko wrote:
> On Fri, Apr 07, 2017 at 02:58:17PM +0100, Tvrtko Ursulin wrote:
>>
>> On 07/04/2017 14:32, Michal Wajdeczko wrote:
>>> In some cases we may want to spend more time in atomic wait than
>>> hardcoded 2us. Let's add additional hint parameter to allow extending
>>> internal atomic timeout to 10us before switching into heavy wait.
>>>
>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>> ---
>>> drivers/gpu/drm/i915/i915_drv.h | 1 +
>>> drivers/gpu/drm/i915/intel_i2c.c | 2 +-
>>> drivers/gpu/drm/i915/intel_pm.c | 4 ++--
>>> drivers/gpu/drm/i915/intel_ringbuffer.c | 4 ++--
>>> drivers/gpu/drm/i915/intel_uncore.c | 12 +++++++++---
>>> 5 files changed, 15 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 41188d6..6f17517 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -3098,6 +3098,7 @@ int intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
>>> i915_reg_t reg,
>>> const u32 mask,
>>> const u32 value,
>>> + bool is_fast,
>>> const unsigned int timeout_ms);
>>>
>>> static inline bool intel_gvt_active(struct drm_i915_private *dev_priv)
>>> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
>>> index b6401e8..6753229 100644
>>> --- a/drivers/gpu/drm/i915/intel_i2c.c
>>> +++ b/drivers/gpu/drm/i915/intel_i2c.c
>>> @@ -299,7 +299,7 @@ gmbus_wait_idle(struct drm_i915_private *dev_priv)
>>>
>>> ret = intel_wait_for_register_fw(dev_priv,
>>> GMBUS2, GMBUS_ACTIVE, 0,
>>> - 10);
>>> + true, 10);
>>>
>>> I915_WRITE_FW(GMBUS4, 0);
>>> remove_wait_queue(&dev_priv->gmbus_wait_queue, &wait);
>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>> index 55e1e88..f7efaca 100644
>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>> @@ -8137,7 +8137,7 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
>>>
>>> if (intel_wait_for_register_fw(dev_priv,
>>> GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
>>> - 500)) {
>>> + true, 500)) {
>>> DRM_ERROR("timeout waiting for pcode read (%d) to finish\n", mbox);
>>> return -ETIMEDOUT;
>>> }
>>> @@ -8182,7 +8182,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
>>>
>>> if (intel_wait_for_register_fw(dev_priv,
>>> GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
>>> - 500)) {
>>> + true, 500)) {
>>> DRM_ERROR("timeout waiting for pcode write (%d) to finish\n", mbox);
>>> return -ETIMEDOUT;
>>> }
>>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>>> index c98acc2..be649cf 100644
>>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>>> @@ -540,7 +540,7 @@ static int init_ring_common(struct intel_engine_cs *engine)
>>> /* If the head is still not zero, the ring is dead */
>>> if (intel_wait_for_register_fw(dev_priv, RING_CTL(engine->mmio_base),
>>> RING_VALID, RING_VALID,
>>> - 50)) {
>>> + true, 50)) {
>>> DRM_ERROR("%s initialization failed "
>>> "ctl %08x (valid? %d) head %08x [%08x] tail %08x [%08x] start %08x [expected %08x]\n",
>>> engine->name,
>>> @@ -1733,7 +1733,7 @@ static void gen6_bsd_submit_request(struct drm_i915_gem_request *request)
>>> GEN6_BSD_SLEEP_PSMI_CONTROL,
>>> GEN6_BSD_SLEEP_INDICATOR,
>>> 0,
>>> - 50))
>>> + true, 50))
>>> DRM_ERROR("timed out waiting for the BSD ring to wake up\n");
>>>
>>> /* Now that the ring is fully powered up, update the tail */
>>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>>> index bcabf54..324a758 100644
>>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>>> @@ -1537,7 +1537,7 @@ static int gen6_hw_domain_reset(struct drm_i915_private *dev_priv,
>>> /* Spin waiting for the device to ack the reset requests */
>>> return intel_wait_for_register_fw(dev_priv,
>>> GEN6_GDRST, hw_domain_mask, 0,
>>> - 500);
>>> + true, 500);
>>> }
>>>
>>> /**
>>> @@ -1590,6 +1590,7 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
>>> * @reg: the register to read
>>> * @mask: mask to apply to register value
>>> * @value: expected value
>>> + * @is_fast: hint that it is expected to be a fast match
>>> * @timeout_ms: timeout in millisecond
>>> *
>>> * This routine waits until the target register @reg contains the expected
>>> @@ -1610,10 +1611,15 @@ int intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
>>> i915_reg_t reg,
>>> const u32 mask,
>>> const u32 value,
>>> + bool is_fast,
>>> const unsigned int timeout_ms)
>>> {
>>> #define done ((I915_READ_FW(reg) & mask) == value)
>>> - int ret = wait_for_us(done, 2);
>>> + int ret;
>>> + if (is_fast)
>>> + ret = wait_for_us(done, 2);
>>> + else
>>> + ret = wait_for_us(done, 10);
>>> if (ret)
>>> ret = wait_for(done, timeout_ms);
>>> return ret;
>>> @@ -1670,7 +1676,7 @@ static int gen8_request_engine_reset(struct intel_engine_cs *engine)
>>> RING_RESET_CTL(engine->mmio_base),
>>> RESET_CTL_READY_TO_RESET,
>>> RESET_CTL_READY_TO_RESET,
>>> - 700);
>>> + true, 700);
>>> if (ret)
>>> DRM_ERROR("%s: reset request timeout\n", engine->name);
>>>
>>>
>>
>> I would recommend a different solution here.
>>
>> Rename and change the prototype of intel_wait_for_register_fw to:
>>
>> int __intel_wait_for_register_fw(dev_priv, reg, mask, vlaue, fast_timeout_us, timeout_ms)
>> {
>> ..existing function body, just replace "2" with fast_timeout_us...
>> }
>>
>> int intel_wait_for_register_fw(dev_priv, reg, mask, vlaue, timeout_ms
>> {
>> return __intel_wait_for_register_fw(dev_priv, reg, mask, value, 2, timeout_ms);
>> }
>>
>> And use the __ version from the GuC code.
>>
>> There is no churn elsewhere in the code like this and it is also
>> more flexible for potential other new users.
>
> That was my initial approach, *but* this would require further changes,
> as wait_for_us() expects timeout parameter to be compile time constant.
>
> #define wait_for_us(COND, US) \
> ...
> BUILD_BUG_ON(!__builtin_constant_p(US));
>
> Possible alternate solutions were:
> a) use internal macro _wait_for_atomic() directly with explicit ATOMIC=0
> b) add another variant of wait_for_atomic_us() that will use ATOMIC=0
>
> but I'm not sure that we need to be that flexible in our wait function,
> as the only range for expected fast timeout is bound to 1..10us anyway.
>
> Hence my idea to just provide simple hint that covers two fast timeouts
> values 2us and 10us.
I forgot about that detail. :( I have no ideas at the moment other than
moving the double underscore version to a header file as static inline.
But that would cost some text size so I don't know.
Regards,
Tvrtko
More information about the Intel-gfx
mailing list