[Intel-gfx] [PATCH] drm/i915: Fallback to reserve forcewake if primary ack missing

[Chris Wilson]

Quoting Mika Kuoppala (2017-10-30 12:27:07)
> There is a possibility on gen9 hardware to miss the forcewake ack
> message. The recommended workaround is to use another free
> bit and toggle it until original bit is successfully acknowledged.
> 
> Some future gen9 revs might or might not fix the underlying issue but
> the fallback to reserve bit dance can be considered as harmless:
> without the ack timeout we never reach the reserve bit forcewake.
> Thus as of now we adopt a blanket approach for all gen9 and leave
> the bypassing the reserve bit approach for future patches if
> corresponding hw revisions do appear.
> 
> Commit 83e3337204b2 ("drm/i915: Increase maximum polling time to 50ms
> for forcewake request/clear ack") did increase the forcewake timeout.
> If the issue was a delayed ack, future work could include finding
> a suitable timeout value both for primary ack and reserve toggle
> to reduce the worst case latency.
> 
> v2: use bit 15, naming, comment (Chris), only wait fallback ack
> v3: fix return on fallback, backoff after fallback write (Chris)
> 
> References: HSDES #1604254524
> References: https://bugs.freedesktop.org/show_bug.cgi?id=102051
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> ---
> +static int
> +fw_domain_wait_ack_with_fallback(const struct drm_i915_private *i915,
> +                                const struct intel_uncore_forcewake_domain *d,
> +                                const enum ack_type type)
> +{
> +       const u32 ack_bit = FORCEWAKE_KERNEL;
> +       const u32 value = type == ACK_SET ? ack_bit : 0;
> +       unsigned int pass = 0;
> +       bool ack_detected;
> +
> +       /*
> +        * There is a possibility of driver's wake request colliding
> +        * with hardware's own wake requests and that can cause
> +        * hardware to not deliver the driver's ack message.
> +        *
> +        * Use a fallback bit toggle to kick the gpu state machine
> +        * in hopes that the original ack will be delivered along with
> +        * the fallback ack.

s/in hopes/in the hope/

> +        *
> +        * This workaround is described in HSDES #1604254524
> +        */
> +
> +       do {
> +               wait_ack_clear(i915, d, FORCEWAKE_KERNEL_FALLBACK);
> +
> +               __raw_i915_write32(i915, d->reg_set,
> +                                  _MASKED_BIT_ENABLE(FORCEWAKE_KERNEL_FALLBACK));
> +               /* Give gt some time to relax before the polling frenzy */
> +               udelay(10 * pass);
> +               wait_ack_set(i915, d, FORCEWAKE_KERNEL_FALLBACK);

I would have started from pass=1 (i.e. udelay(10)) as we already have a
0-delay for the primary wait_ack before we hit the fallback.

> +
> +               ack_detected = (__raw_i915_read32(i915, d->reg_ack) & ack_bit) == value;
> +
> +               __raw_i915_write32(i915, d->reg_set,
> +                                  _MASKED_BIT_DISABLE(FORCEWAKE_KERNEL_FALLBACK));
> +               pass++;
> +       } while (!ack_detected && pass < 10);

	unsigned int pass = 1;
	do {
		...
	} while (!ack_detected && pass++ < 10);

> +
> +       DRM_DEBUG_DRIVER("%s had to use fallback to %s ack, 0x%x (passes %u)\n",
> +                        intel_uncore_forcewake_domain_to_str(d->id),
> +                        type == ACK_SET ? "set" : "clear",
> +                        __raw_i915_read32(i915, d->reg_ack),
> +                        pass);
> +
> +       return ack_detected ? 0 : -ETIMEDOUT;
> +}

I was going to say a-b, but given the state machine we've deduced that
explains why this w/a has any chance of succeeding, I feel a

Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>

is justified. (If it's wrong, I'm equally culpable ;)
-Chris