[Intel-gfx] [PATCH v2] drm/i915/cnl: WaPipeControlBefore3DStateSamplePattern

[Rafael Antognolli]

On Fri, Jan 26, 2018 at 11:17:01PM +0000, Chris Wilson wrote:
> Quoting Rafael Antognolli (2018-01-26 23:05:24)
> > This workaround should prevent a bug that can be hit on a context
> > restore. To avoid the issue, we must emit a PIPE_CONTROL with CS stall
> > (0x7a000004 0x00100000 0x00000000 0x00000000) followed by 12DW's of
> > NOOP(0x0) in the indirect context batch buffer, to ensure the engine is
> > idle prior to programming 3DSTATE_SAMPLE_PATTERN.
> > 
> > References: HSD#1939868
> > 
> >  v2:
> >    - More descriptive changelog and comments.
> >    - Fix math when counting dwords.
> > 
> > Signed-off-by: Rafael Antognolli <rafael.antognolli at intel.com>
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_lrc.c | 34 +++++++++++++++++++++++++++++++++-
> >  1 file changed, 33 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index 51e61b04a555..46c130d106d7 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -1366,6 +1366,36 @@ static u32 *gen9_init_indirectctx_bb(struct intel_engine_cs *engine, u32 *batch)
> >         return batch;
> >  }
> >  
> > +static u32 *gen10_init_indirectctx_bb(struct intel_engine_cs *engine, u32 *batch)
> > +{
> > +       int i;
> > +
> > +       /*
> > +        * WaPipeControlBefore3DStateSamplePattern: cnl
> > +        *
> > +        * Ensure the engine is idle prior to programming a
> > +        * 3DSTATE_SAMPLE_PATTERN during a context restore.
> > +        */
> > +       batch = gen8_emit_pipe_control(batch,
> > +                                      PIPE_CONTROL_CS_STALL,
> > +                                      0);
> > +       /*
> > +        * WaPipeControlBefore3DStateSamplePattern says we need 4 dwords for
> > +        * the PIPE_CONTROL followed by 12 dwords of 0x0, so 16 dwords in
> > +        * total. Since gen8_emit_pipe_control() already advances the batch by
> > +        * 6 dwords, we advance the other 10 here.
> > +        */
> > +       for (i = 0; i < 10; i++)
> > +               *batch++ = MI_NOOP;
> 
> If the spec says emit 12 nops, emit 12 nops. Otherwise if we add another
> w/a here it may break this magic.

So, in the bspec, there's the following text:

"The address above needs to be a GGTT address and contain a pipe control
with CS stall(0x7a000004 0x00100000 0x00000000 0x00000000)followed by
12DW’s of NOOP(0x00000000)"

while on an internal ticket pointed by it, it is:

"The address above needs to be a GGTT address.. It would contain the
following value: 7A000004 00100000 followed by 14 DW’s of zero."

That's why I think it is really 16 DW's in total. And
gen8_emit_pipe_control() already emits 6 of them. How are we supposed to
emit 12 after that?

I don't know if they expect the pipe control to be cacheline aligned,
but I can ask and try to find out.

> I am not going to ask what the magic nops do, it just gives a 72 cycle
> delay in the CS parser. My guess is that they are trying to isolate a
> cacheline, in which case they are expecting the pipecontrol to be
> cacheline aligned. (I would get that clarified and add an assert to
> document such a requirement.) Or they just mean that the indirect ctx is
> cacheline aligned and the nops are just the normal padding and nothing
> to do with the w/a.
> 
> s/10/12/ here and tweak the comment to remove the assumed cacheline
> tail, but please see if someone can clarify if the nops are indeed part
> of the w/a or just the normal filler.
> With s/10/12/,
> Acked-by: Chris Wilson <chris at chris-wilson.co.uk>
> -Chris