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

[Chris Wilson]

On Mon, Apr 10, 2017 at 05:20:32PM +0200, Michal Wajdeczko wrote:
> On Mon, Apr 10, 2017 at 04:02:05PM +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.
> 
> So we don't enforce this any more ?

I considered adding a GEM_BUG_ON / GEM_WARN_ON, but really just not
listening to them and hitting the sleep was my answer to sillyness.

Atomic spinning for 20ms is not acceptable. :|

> >   * 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);
> >  
> > -	if (fast_timeout_us > 10)
> > -		ret = _wait_for(done, fast_timeout_us, 10);
> > -	else
> > +	ret = -ETIMEDOUT;
> > +	if (fast_timeout_us && fast_timeout_us < 20000)
> >  		ret = _wait_for_atomic(done, fast_timeout_us, 0);
> >  	if (ret)
> >  		ret = wait_for(done, slow_timeout_ms);
> 
> What if someone passes fast=20001us and slow=0ms ?
> Maybe like this:
> 
> 	might_sleep_if(fast_timeout_us > 20000 || slow_timeout_ms);
> 
> 	if (fast_timeout_us && fast_timeout_us < 20000)
> 		ret = _wait_for_atomic(done, fast_timeout_us, 0);
> 	else
> 		slow_timeout_ms += fast_timeout_us/1000;

No. Just shoot it down in review.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre