Re: [PATCH v4 6/6] drm/i915/gen8: Add WaRsRestoreWithPerCtxtBb workaround

Chris Wilson
2015-06-16T20:43:51Z

On Tue, Jun 16, 2015 at 08:25:25PM +0100, Arun Siluvery wrote:
> v3: Length defined in current definitions of LRM, LRR instructions is not correct.
> Correct and use existing definitions and also move them out of command parser
> placeholder to appropriate place.

Not that it wasn't correct. Common form has to be use 0 for instructions
whose length vary between platforms and then to fill in (X - 2) as
required. (At least that is what we do in userspace.)

> + /* WaRsRestoreWithPerCtxtBb:bdw,chv */
> + cmd[index++] = MI_LOAD_REGISTER_IMM(1);
> + cmd[index++] = INSTPM;
> + cmd[index++] = _MASKED_BIT_DISABLE(INSTPM_FORCE_ORDERING);
> +
> + flags = MI_ATOMIC_MEMORY_TYPE_GGTT |
> + MI_ATOMIC_INLINE_DATA |
> + MI_ATOMIC_CS_STALL |
> + MI_ATOMIC_RETURN_DATA_CTL |
> + MI_ATOMIC_MOVE;
> +
> + cmd[index++] = MI_ATOMIC(5) | flags;

This is very inconsistent style, and indeed flags doesn't make the code
any more readable than simply specifying each in the cmd.

> + cmd[index++] = scratch_addr;
> + cmd[index++] = 0;
> + cmd[index++] = _MASKED_BIT_ENABLE(INSTPM_FORCE_ORDERING);
> + cmd[index++] = _MASKED_BIT_ENABLE(INSTPM_FORCE_ORDERING);
> +
> + /*
> + * BSpec says MI_LOAD_REGISTER_MEM, MI_LOAD_REGISTER_REG and
> + * MI_BATCH_BUFFER_END instructions in this sequence need to be
> + * in the same cacheline. To satisfy this case even if more WA are
> + * added in future, pad current cacheline and start remaining sequence
> + * in new cacheline.
> + */
> + while (((unsigned long) (cmd + index) % CACHELINE_BYTES) != 0)
> + cmd[index++] = MI_NOOP;
> +
> + cmd[index++] = MI_LOAD_REGISTER_MEM_GEN8 |
> + MI_LRM_USE_GLOBAL_GTT |
> + MI_LRM_ASYNC_MODE_ENABLE;

Use brackets to force alignment when it's ugly like this.
-Chris

--
Chris Wilson, Intel Open Source Technology Centre