[Intel-gfx] [PATCH v3] drm/i915: Use atomic waits for short non-atomic ones

[Chris Wilson]

On Tue, Jun 28, 2016 at 05:20:43PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin at 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 reimplement the _wait_for_atomic macro to
> be safe with regards to preemption and interrupts.
> 
> v2: Reimplement _wait_for_atomic to be irq and preemption safe.
>     (Chris Wilson and Imre Deak)
> 
> v3: Fixed in_atomic check due rebase error.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Imre Deak <imre.deak at intel.com>
> Cc: Mika Kuoppala <mika.kuoppala at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h | 56 ++++++++++++++++++++++++++--------------
>  1 file changed, 37 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3156d8df7921..265f76157876 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -69,39 +69,57 @@
>  })
>  
>  #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) ({ \
> -	unsigned long end__; \
> -	int ret__ = 0; \
> -	_WAIT_FOR_ATOMIC_CHECK; \
> +#define _wait_for_atomic(COND, US, ATOMIC) \
> +({ \
> +	int cpu, ret, timeout = (US) * 1000; \
> +	u64 base; \
> +	_WAIT_FOR_ATOMIC_CHECK(ATOMIC); \
>  	BUILD_BUG_ON((US) > 50000); \
> -	end__ = (local_clock() >> 10) + (US) + 1; \
> -	while (!(COND)) { \
> -		if (time_after((unsigned long)(local_clock() >> 10), end__)) { \
> -			/* Unlike the regular wait_for(), this atomic variant \
> -			 * cannot be preempted (and we'll just ignore the issue\
> -			 * of irq interruptions) and so we know that no time \
> -			 * has passed since the last check of COND and can \
> -			 * immediately report the timeout. \
> -			 */ \
> -			ret__ = -ETIMEDOUT; \
> +	preempt_disable(); \
> +	cpu = smp_processor_id(); \
> +	base = local_clock(); \
> +	for (;;) { \
> +		u64 now = local_clock(); \
> +		preempt_enable(); \
> +		if (COND) { \
> +			ret = 0; \
> +			break; \
> +		} \
> +		if (now - base >= timeout) { \
> +			ret = -ETIMEDOUT; \
>  			break; \
>  		} \
>  		cpu_relax(); \
> +		preempt_disable(); \
> +		if (unlikely(cpu != smp_processor_id())) { \
> +			timeout -= now - base; \
> +			cpu = smp_processor_id(); \
> +			base = local_clock(); \
> +		} \
>  	} \
> +	ret; \
> +})
> +
> +#define wait_for_us(COND, US) \
> +({ \
> +	int ret__; \

       /* usleep_range() is not recommended for waits shorten than 10us. */ \

> +	if ((US) > 10) \

Do we want __builtin_constant_p(US) to be sure this folds away?

Can you resend this as a new series, I'm 100% sure if CI picked it up?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre