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


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

Reply via email to