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

From: Imre Deak <imre.deak@intel.com>
To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
	Intel-gfx@lists.freedesktop.org
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Subject: Re: [PATCH 2/2] drm/i915: Use atomic waits for short non-atomic ones
Date: Tue, 28 Jun 2016 15:19:04 +0300	[thread overview]
Message-ID: <1467116344.20290.40.camel@intel.com> (raw)
In-Reply-To: <1467114710-29989-2-git-send-email-tvrtko.ursulin@linux.intel.com>

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
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.

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

>  	end__ = (local_clock() >> 10) + (US) + 1; \
>  	while (!(COND)) { \
>  		if (time_after((unsigned long)(local_clock() >> 10), end__)) { \
> @@ -97,11 +98,23 @@
>  		} \
>  		cpu_relax(); \
>  	} \
> +	if (!(ATOMIC)) \
> +		preempt_enable(); \
>  	ret__; \
>  })
>  
> -#define wait_for_atomic(COND, MS)	_wait_for_atomic((COND), (MS) * 1000)
> -#define wait_for_atomic_us(COND, US)	_wait_for_atomic((COND), (US))
> +#define wait_for_us(COND, US) \
> +({ \
> +	int ret__; \
> +	if ((US) > 10) \
> +		ret__ = _wait_for((COND), (US), 10); \
> +	else \
> +		ret__ = _wait_for_atomic((COND), (US), 0); \
> +	ret__; \
> +})
> +
> +#define wait_for_atomic(COND, MS)	_wait_for_atomic((COND), (MS) * 1000, 1)
> +#define wait_for_atomic_us(COND, US)	_wait_for_atomic((COND), (US), 1)
>  
>  #define KHz(x) (1000 * (x))
>  #define MHz(x) KHz(1000 * (x))
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx