Re: [PATCH v3] drm/i915: Use exponential backoff for wait_for()

[Sagar Arun Kamble <sagar.a.kamble@intel.com>]

On 11/22/2017 2:29 AM, Chris Wilson wrote:
> Instead of sleeping for a fixed 1ms (roughly, depending on timer slack),
> start with a small sleep and exponentially increase the sleep on each
> cycle.
>
> A good example of a beneficiary is the guc mmio communication channel.
As Tvrtko said, for the current GuC communication (guc_send_mmio) we 
will need to update fast timeout of
__intel_wait_for_register to 20us. Improvement this patch proposes 
through wait_for will
certainly be seen once we switch over to GuC CT. May be specifying "GuC 
CT channel" here is apt.
> Typically we expect (and so spin) for 10us for a quick response, but this
> doesn't cover everything and so sometimes we fallback to the millisecond+
> sleep. This incurs a significant delay in time-critical operations like
> preemption (igt/gem_exec_latency), which can be improved significantly by
> using a small sleep after the spin fails.
>
> We've made this suggestion many times, but had little experimental data
> to support adding the complexity.
>
> v2: Bump the minimum usleep to 10us on advice of
> Documentation/timers/timers-howto.txt (Tvrko)
> v3: Specify min, max range for usleep intervals -- some code may
> crucially depend upon and so want to specify the sleep pattern.
>
> References: 1758b90e38f5 ("drm/i915: Use a hybrid scheme for fast register waits")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: John Harrison <John.C.Harrison@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_drv.h | 11 +++++++----
>   drivers/gpu/drm/i915/intel_pm.c  |  2 +-
>   2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 635a96fcd788..c00441a3d649 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -48,8 +48,9 @@
>    * having timed out, since the timeout could be due to preemption or similar and
>    * we've never had a chance to check the condition before the timeout.
>    */
> -#define _wait_for(COND, US, W) ({ \
> +#define _wait_for(COND, US, Wmin, Wmax) ({ \
>   	unsigned long timeout__ = jiffies + usecs_to_jiffies(US) + 1;	\
> +	long wait__ = (Wmin); /* recommended min for usleep is 10 us */	\
>   	int ret__;							\
>   	might_sleep();							\
>   	for (;;) {							\
> @@ -62,12 +63,14 @@
>   			ret__ = -ETIMEDOUT;				\
>   			break;						\
>   		}							\
> -		usleep_range((W), (W) * 2);				\
> +		usleep_range(wait__, wait__ * 2);			\
> +		if (wait__ < (Wmax))					\
> +			wait__ <<= 1;					\
I think we need to keep track of total time we have waited else we might 
wait for longer than necessary.
For e.g. for wait_for_us(COND, 900) this approach might actually lead to 
sleep of 1270us.
>   	}								\
>   	ret__;								\
>   })
>   
> -#define wait_for(COND, MS)	  	_wait_for((COND), (MS) * 1000, 1000)
> +#define wait_for(COND, MS)	_wait_for((COND), (MS) * 1000, 10, 1000)
>   
>   /* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */
>   #if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT)
> @@ -116,7 +119,7 @@
>   	int ret__; \
>   	BUILD_BUG_ON(!__builtin_constant_p(US)); \
>   	if ((US) > 10) \
> -		ret__ = _wait_for((COND), (US), 10); \
> +		ret__ = _wait_for((COND), (US), 10, 10); \
>   	else \
>   		ret__ = _wait_for_atomic((COND), (US), 0); \
>   	ret__; \
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index e445ec174831..f07f14ae198d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -9294,7 +9294,7 @@ int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request,
>   		ret = 0;
>   		goto out;
>   	}
> -	ret = _wait_for(COND, timeout_base_ms * 1000, 10);
> +	ret = _wait_for(COND, timeout_base_ms * 1000, 10, 10);
>   	if (!ret)
>   		goto out;
>   

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