[Intel-gfx] [PATCH v5 1/6] drm/i915/gen8: Add infrastructure to initialize WA batch buffers

[Chris Wilson]

I'm pretty happy with the code, I was just confused by the series
changing the setup halfway through

On Thu, Jun 18, 2015 at 02:07:30PM +0100, Arun Siluvery wrote:
> +static int gen8_init_indirectctx_bb(struct intel_engine_cs *ring,
> +				    uint32_t **wa_ctx_batch,
> +				    uint32_t offset,
> +				    uint32_t *num_dwords)
> +{
> +	uint32_t index;
> +	uint32_t *batch = *wa_ctx_batch;
> +
> +	index = offset;
> +
> +	/* FIXME: fill one cacheline with NOOPs.
> +	 * Replace these instructions with WA
> +	 */
> +	while (index < (offset + 16))
> +		wa_ctx_emit(batch, MI_NOOP);

If this was

/* Replace me with WA */
wa_ctx_emit(batch, MI_NOOP)

/* Pad to end of cacheline */
while (index % 16)
	wa_ctx_emit(batch, MI_NOOP);

You then don't need to alter the code when yo add the real w/a. Note
that using (unsigned long)batch as you do later for cacheline
calculation is wrong, as that is a local physical CPU address (not the
virtual address used by the cache in the GPU) and was page aligned
anyway.

Similary,

> +static int gen8_init_perctx_bb(struct intel_engine_cs *ring,
> +			       uint32_t **wa_ctx_batch,
> +			       uint32_t offset,
> +			       uint32_t *num_dwords)
> +{
> +	uint32_t index;
> +	uint32_t *batch = *wa_ctx_batch;
> +
> +	index = offset;
> +

If this just did
		wa_ctx_emit(batch, MI_BATCH_BUFFER_END);
rather than insert a cacheline of noops, again you wouldn't need to
touch this infrastructure as you added the w/a.

As it stands, I was a little worried halfway through when the cache
alignment suddenly disappeared - but this patch implied to me that it
was necessary.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre