[Intel-gfx] [PATCH 1/3] drm/i915/execlists: Force write serialisation into context image vs execution

[Chris Wilson]

Quoting Mika Kuoppala (2018-11-08 12:00:39)
> Chris Wilson <chris at chris-wilson.co.uk> 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 at chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > ---
> >  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 at linux.intel.com>

Aye. Sure about r-b? I'm quite happy to take an a-b since we're just
postulating to gather evidence.
-Chris