Re: [PATCH 2/2] drm/i915: Use atomic waits for short non-atomic ones

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 28/06/16 13:19, Imre Deak wrote:
> On ti, 2016-06-28 at 12:51 +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> usleep_range is not recommended for waits shorten than 10us.
>>
>> Make the wait_for_us use the atomic variant for such waits.
>>
>> To do so we need to disable the !in_atomic warning for such uses
>> and also disable preemption since the macro is written in a way
>> to only be safe to be used in atomic context (local_clock() and
>> no second COND check after the timeout).
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_drv.h | 29 +++++++++++++++++++++--------
>>   1 file changed, 21 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 3156d8df7921..e21bf6e6f119 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -69,20 +69,21 @@
>>   })
>>
>>   #define wait_for(COND, MS)	  	_wait_for((COND), (MS) * 1000, 1000)
>> -#define wait_for_us(COND, US)	  	_wait_for((COND), (US), 1)
>>
>>   /* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */
>>   #if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT)
>> -# define _WAIT_FOR_ATOMIC_CHECK WARN_ON_ONCE(!in_atomic())
>> +# define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) WARN_ON_ONCE((ATOMIC) && !in_atomic())
>>   #else
>> -# define _WAIT_FOR_ATOMIC_CHECK do { } while (0)
>> +# define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) do { } while (0)
>>   #endif
>>
>> -#define _wait_for_atomic(COND, US) ({ \
>> +#define _wait_for_atomic(COND, US, ATOMIC) ({ \
>>   	unsigned long end__; \
>>   	int ret__ = 0; \
>> -	_WAIT_FOR_ATOMIC_CHECK; \
>> -	BUILD_BUG_ON((US) > 50000); \
>> +	_WAIT_FOR_ATOMIC_CHECK(ATOMIC); \
>> +	BUILD_BUG_ON((ATOMIC) && (US) > 50000); \
>> +	if (!(ATOMIC)) \
>> +		preempt_disable(); \
>
> Disabling preemption for this purpose (scheduling a timeout) could be
> frowned upon, although for 10us may be not an issue. Another

Possibly, but I don't see how to otherwise do it.

And about the number itself - I chose 10us just because usleep_range is 
not recommended for <10us due setup overhead.

> possibility would be to use cpu_clock() instead which would have some
> overhead in case of scheduling away from the initial CPU, but we'd only
> incur it for the non-atomic <10us case, so would be negligible imo.
> You'd also have to re-check the condition with that solution.

How would you implement it with cpu_clock? What would you do when 
re-scheduled?

> Also could you explain how can we ignore hard IRQs as hinted by the
> comment in _wait_for_atomic()?

Hm, in retrospect it does not look safe. Upside that after your fixes 
from today it will be, since all remaining callers are with interrupts 
disabled. And downside that the patch from this thread is not safe then 
and would need the condition put back in. Possibly only in the !ATOMIC 
case but that might be too fragile for the future.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx