Re: [PATCH v4 1/6] drm/i915/gen8: Add infrastructure to initialize WA batch buffers

Siluvery, Arun
2015-06-17T19:48:20Z

On 17/06/2015 19:48, Siluvery, Arun wrote:
> On 16/06/2015 21:25, Chris Wilson wrote:
>> On Tue, Jun 16, 2015 at 08:25:20PM +0100, Arun Siluvery wrote:
>>> +static int gen8_init_indirectctx_bb(struct intel_engine_cs *ring,
>>> + uint32_t offset,
>>> + uint32_t *num_dwords)
>>> +{
>>> + uint32_t index;
>>> + struct page *page;
>>> + uint32_t *cmd;
>>> +
>>> + page = i915_gem_object_get_page(ring->wa_ctx.obj, 0);
>>> + cmd = kmap_atomic(page);
>>> +
>>> + index = offset;
>>> +
>>> + /* FIXME: fill one cacheline with NOOPs.
>>> + * Replace these instructions with WA
>>> + */
>>> + while (index < (offset + 16))
>>> + cmd[index++] = MI_NOOP;
>>> +
>>> + /*
>>> + * MI_BATCH_BUFFER_END is not required in Indirect ctx BB because
>>> + * execution depends on the length specified in terms of cache lines
>>> + * in the register CTX_RCS_INDIRECT_CTX
>>> + */
>>> +
>>> + kunmap_atomic(cmd);
>>> +
>>> + if (index > (PAGE_SIZE / sizeof(uint32_t)))
>>> + return -EINVAL;
>>
>> Check before you GPF!
>>
>> You just overran the buffer and corrupted memory, if you didn't succeed
>> in trapping a segfault.
>>
>> To be generic, align to the cacheline then check you have enough room
>> for your own data.
>> -Chris
>>
> Hi Chris,
>
> The placement of condition is not correct. I don't completely follow
> your suggestion, could you please elaborate; here we don't know upfront
> how much more data to be written.
> I have made below changes to check after writing every command and
> return error as soon as we reach the end.
>
> #define wa_ctx_emit(batch, cmd) { \
> if (WARN_ON(index >= (PAGE_SIZE / sizeof(uint32_t)))) { \
> kunmap_atomic(batch); \
> return -ENOSPC; \
> } \
> batch[index++] = (cmd); \
> }
> is this acceptable?
> I think this is the only one issue, all other comments are addressed.
>
one other improvement is possible - mapping/unmapping page can be kept
in common path, will update the patch accordingly.

regards
Arun

> regards
> Arun
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>