[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