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>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > drivers/gpu/drm/i915/intel_i2c.c | 2 +-
> > drivers/gpu/drm/i915/intel_pm.c | 4 ++--
> > drivers/gpu/drm/i915/intel_ringbuffer.c | 4 ++--
> > drivers/gpu/drm/i915/intel_uncore.c | 12 +++++++++---
> > 5 files changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 41188d6..6f17517 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3098,6 +3098,7 @@ int intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
> > i915_reg_t reg,
> > const u32 mask,
> > const u32 value,
> > + bool is_fast,
> > const unsigned int timeout_ms);
> >
> > static inline bool intel_gvt_active(struct drm_i915_private *dev_priv)
> > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> > index b6401e8..6753229 100644
> > --- a/drivers/gpu/drm/i915/intel_i2c.c
> > +++ b/drivers/gpu/drm/i915/intel_i2c.c
> > @@ -299,7 +299,7 @@ gmbus_wait_idle(struct drm_i915_private *dev_priv)
> >
> > ret = intel_wait_for_register_fw(dev_priv,
> > GMBUS2, GMBUS_ACTIVE, 0,
> > - 10);
> > + true, 10);
> >
> > I915_WRITE_FW(GMBUS4, 0);
> > remove_wait_queue(&dev_priv->gmbus_wait_queue, &wait);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 55e1e88..f7efaca 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -8137,7 +8137,7 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
> >
> > if (intel_wait_for_register_fw(dev_priv,
> > GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
> > - 500)) {
> > + true, 500)) {
> > DRM_ERROR("timeout waiting for pcode read (%d) to finish\n", mbox);
> > return -ETIMEDOUT;
> > }
> > @@ -8182,7 +8182,7 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
> >
> > if (intel_wait_for_register_fw(dev_priv,
> > GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
> > - 500)) {
> > + true, 500)) {
> > DRM_ERROR("timeout waiting for pcode write (%d) to finish\n", mbox);
> > return -ETIMEDOUT;
> > }
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index c98acc2..be649cf 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -540,7 +540,7 @@ static int init_ring_common(struct intel_engine_cs *engine)
> > /* If the head is still not zero, the ring is dead */
> > if (intel_wait_for_register_fw(dev_priv, RING_CTL(engine->mmio_base),
> > RING_VALID, RING_VALID,
> > - 50)) {
> > + true, 50)) {
> > DRM_ERROR("%s initialization failed "
> > "ctl %08x (valid? %d) head %08x [%08x] tail %08x [%08x] start %08x [expected %08x]\n",
> > engine->name,
> > @@ -1733,7 +1733,7 @@ static void gen6_bsd_submit_request(struct drm_i915_gem_request *request)
> > GEN6_BSD_SLEEP_PSMI_CONTROL,
> > GEN6_BSD_SLEEP_INDICATOR,
> > 0,
> > - 50))
> > + true, 50))
> > DRM_ERROR("timed out waiting for the BSD ring to wake up\n");
> >
> > /* Now that the ring is fully powered up, update the tail */
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index bcabf54..324a758 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -1537,7 +1537,7 @@ static int gen6_hw_domain_reset(struct drm_i915_private *dev_priv,
> > /* Spin waiting for the device to ack the reset requests */
> > return intel_wait_for_register_fw(dev_priv,
> > GEN6_GDRST, hw_domain_mask, 0,
> > - 500);
> > + true, 500);
> > }
> >
> > /**
> > @@ -1590,6 +1590,7 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
> > * @reg: the register to read
> > * @mask: mask to apply to register value
> > * @value: expected value
> > + * @is_fast: hint that it is expected to be a fast match
> > * @timeout_ms: timeout in millisecond
> > *
> > * This routine waits until the target register @reg contains the expected
> > @@ -1610,10 +1611,15 @@ int intel_wait_for_register_fw(struct drm_i915_private *dev_priv,
> > i915_reg_t reg,
> > const u32 mask,
> > const u32 value,
> > + bool is_fast,
> > const unsigned int timeout_ms)
> > {
> > #define done ((I915_READ_FW(reg) & mask) == value)
> > - int ret = wait_for_us(done, 2);
> > + int ret;
> > + if (is_fast)
> > + ret = wait_for_us(done, 2);
> > + else
> > + ret = wait_for_us(done, 10);
> > if (ret)
> > ret = wait_for(done, timeout_ms);
> > return ret;
> > @@ -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.
#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
but I'm not sure that we need to be that flexible in our wait function,
as the only range for expected fast timeout is bound to 1..10us anyway.
Hence my idea to just provide simple hint that covers two fast timeouts
values 2us and 10us.
-Michal
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
