Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes: > Quoting Mika Kuoppala (2018-11-08 12:00:39) >> Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes: >> >> > Ensure that the writes into the context image are completed prior to the >> > register mmio to trigger execution. Although previously we were assured >> > by the SDM that all writes are flushed before an uncached memory >> > transaction (our mmio write to submit the context to HW for execution), >> > we have empirical evidence to believe that this is not actually the >> > case. >> > >> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108656 >> >> I would have marked this also as References. > > I'm confident in my local results indicating some success here, albeit > in not exactly the same quick death, but still out-of-bounds execution. > >> > References: https://bugs.freedesktop.org/show_bug.cgi?id=108315 >> > References: https://bugs.freedesktop.org/show_bug.cgi?id=106887 >> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> >> > Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx> >> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> >> > --- >> > drivers/gpu/drm/i915/intel_lrc.c | 14 +++++++++++++- >> > 1 file changed, 13 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c >> > index 22b57b8926fc..f7892ddb3f13 100644 >> > --- a/drivers/gpu/drm/i915/intel_lrc.c >> > +++ b/drivers/gpu/drm/i915/intel_lrc.c >> > @@ -380,7 +380,8 @@ static u64 execlists_update_context(struct i915_request *rq) >> > >> > reg_state[CTX_RING_TAIL+1] = intel_ring_set_tail(rq->ring, rq->tail); >> > >> > - /* True 32b PPGTT with dynamic page allocation: update PDP >> > + /* >> > + * True 32b PPGTT with dynamic page allocation: update PDP >> > * registers and point the unallocated PDPs to scratch page. >> > * PML4 is allocated during ppgtt init, so this is not needed >> > * in 48-bit mode. >> > @@ -388,6 +389,17 @@ static u64 execlists_update_context(struct i915_request *rq) >> > if (!i915_vm_is_48bit(&ppgtt->vm)) >> > execlists_update_context_pdps(ppgtt, reg_state); >> > >> > + /* >> > + * Make sure the context image is complete before we submit it to HW. >> > + * >> > + * Ostensibly, writes (including the WCB) should be flushed prior to >> > + * an uncached write such as our mmio register access, the empirical >> > + * evidence (esp. on Braswell) suggests that the WC write into memory >> > + * may not be visible to the HW prior to the completion of the UC >> > + * register write and that we may begin execution from the context >> > + * before its image is complete leading to invalid PD chasing. >> > + */ >> > + wmb(); >> >> Let's put it into use and gather more evidence. >> >> Reviewed-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx> > > Aye. Sure about r-b? I'm quite happy to take an a-b since we're just > postulating to gather evidence. Agreed that a-b is more accurate. r-b would indicate I know what the heck is going on there under the hood. -Mika _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
