[Intel-gfx] [PATCH 1/5] drm/i915: Stop second guessing the caller for intel_uncore_wait_for_register()

[Tvrtko Ursulin]

On 11/04/2017 12:21, Chris Wilson wrote:
> On Tue, Apr 11, 2017 at 11:13:36AM +0100, Chris Wilson wrote:
>> Allow the caller to use the fast_timeout_us to specify how long to wait
>> within the atomic section, rather than transparently switching to a
>> sleeping loop for larger values. This is required as some callsites may
>> need a long wait and are in an atomic section.
>>
>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_uncore.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index eb38392a2435..53c8457869f6 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -1601,7 +1601,7 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
>>   *
>>   * Otherwise, the wait will timeout after @slow_timeout_ms milliseconds.
>>   * For atomic context @slow_timeout_ms must be zero and @fast_timeout_us
>> - * must be not larger than 10 microseconds.
>> + * must be not larger than 20,0000 microseconds.
>>   *
>>   * Note that this routine assumes the caller holds forcewake asserted, it is
>>   * not suitable for very long waits. See intel_wait_for_register() if you
>> @@ -1623,16 +1623,17 @@ int __intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
>>  	int ret;
>>
>>  	/* Catch any overuse of this function */
>> -	might_sleep_if(fast_timeout_us > 10 || slow_timeout_ms);
>> +	might_sleep_if(slow_timeout_ms);
>
> For whatever reason, replies are not in my mbox, so Tvrtko,Michal wrote:
>
>> I agree with Michal here. Kerneldoc even says "must not be larger than
>> 20ms" so it would be better and completely fine in my opinion to:
>>
>> 	if (GEM_WARN_ON(fast_timeout_us > 20000))
>> 		return -EINVAL;
>
> Not EINVAL, ENODEV if we must. So GEM_BUG_ON(fast_timeout_us > 20000) as
> documentation?

Yeah thats fine.

> We need to keep the fast_timeout_us < X check for gcc.
>
>> Hm but it would break the bisectability of the series and break the
>> sandybridge pcode.
>
> No it doesn't, nobody is passing in such a large *fast_timeout_us*. What
> have I missed?

Sounds like I got confused, sorry.

>> So patch 4/5 looks broken since it changes the timeout from 500ms to
>> 500us. I don't see how to fix that without splitting the _fw and atomic
>> concepts.
>
> That's not being broken, thats part of the fix. 500ms timeout inside an
> atomic section is insane. 500ms timeout outside of that is unwise and
> deserves an error of its own. I am quite happy to add that error and
> fight that battle later (as BAT is happy, EXT might not be, and users
> never).

I see. Ok, then, with the GEM_BUG_ON:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko