[Intel-gfx] [PATCH 2/3] drm/i915: Add hint to intel_wait_for_register_fw()

[Chris Wilson]

On Fri, Apr 07, 2017 at 04:10:29PM +0200, Michal Wajdeczko wrote:
> On Fri, Apr 07, 2017 at 02:58:17PM +0100, Tvrtko Ursulin wrote:
> > 
> > On 07/04/2017 14:32, Michal Wajdeczko wrote:
> > > In some cases we may want to spend more time in atomic wait than
> > > hardcoded 2us. Let's add additional hint parameter to allow extending
> > > internal atomic timeout to 10us before switching into heavy wait.
> > > 
> > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> > > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > > ---
> > > @@ -1670,7 +1676,7 @@ static int gen8_request_engine_reset(struct intel_engine_cs *engine)
> > >  					 RING_RESET_CTL(engine->mmio_base),
> > >  					 RESET_CTL_READY_TO_RESET,
> > >  					 RESET_CTL_READY_TO_RESET,
> > > -					 700);
> > > +					 true, 700);
> > >  	if (ret)
> > >  		DRM_ERROR("%s: reset request timeout\n", engine->name);
> > >  
> > > 
> > 
> > I would recommend a different solution here.
> > 
> > Rename and change the prototype of intel_wait_for_register_fw to:
> > 
> > int __intel_wait_for_register_fw(dev_priv, reg, mask, vlaue, fast_timeout_us, timeout_ms)
> > {
> > 	..existing function body, just replace "2" with fast_timeout_us... 
> > } 
> > 
> > int intel_wait_for_register_fw(dev_priv, reg, mask, vlaue, timeout_ms
> > {
> > 	return __intel_wait_for_register_fw(dev_priv, reg, mask, value, 2, timeout_ms);
> > }
> > 
> > And use the __ version from the GuC code.
> > 
> > There is no churn elsewhere in the code like this and it is also
> > more flexible for potential other new users.
> 
> That was my initial approach, *but* this would require further changes,
> as wait_for_us() expects timeout parameter to be compile time constant.

Hmm.

> 	#define wait_for_us(COND, US) \
> 		...
> 		BUILD_BUG_ON(!__builtin_constant_p(US));
> 
> Possible alternate solutions were:
> a) use internal macro _wait_for_atomic() directly with explicit ATOMIC=0
> b) add another variant of wait_for_atomic_us() that will use ATOMIC=0

I'd vote to reducing the number of wait_for_us we have in the driver,
ideally limiting it to intel_uncore.c as the only user. Given how slow
mmio reads are, there's no good excuse against having an extra function
call. Is that achievable?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre