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@xxxxxxxxx>
> > > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > > Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> > > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > > ---
> > > @@ -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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
