[Mesa-dev] [PATCH] i965: Correct constant buffer MOCS

[Ben Widawsky]

On Wed, Jun 03, 2015 at 12:20:09AM -0700, Kenneth Graunke wrote:
> On Tuesday, June 02, 2015 11:41:56 PM Ben Widawsky wrote:
> > On Tue, 02 Jun 2015 20:32:13 -0700
> > Kenneth Graunke <kenneth at whitecape.org> wrote:
> > 
> > > On Tuesday, June 02, 2015 08:07:50 PM Ben Widawsky wrote:
> > > > I'm very confused here. It seems pretty clear that since the
> > > > command has been introduced with support for MOCS, MOCS lives at
> > > > bit 8 of dword 0 for all constant buffers. The error has existed
> > > > since forever AFAICT.
> > > > 
> > > > No piglit regressions or fixes:
> > > > http://otc-mesa-ci.jf.intel.com/view/dev/job/bwidawsk/143/
> > > > 
> > > > I suspect the low bits are discarded by the hardware when using the
> > > > buffer offset (we were setting 1). I suppose it could potentially
> > > > impact perf, though I'd imagine the kernel is already doing the
> > > > right thing by default anyway. This would explain why the patch
> > > > doesn't really do anything behaviorally.
> > > > 
> > > > NOTE: gen8+ should have no change at all since MOCS was always 0
> > > > anyway.
> > > > 
> > > > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > > > ---
> > > >  src/mesa/drivers/dri/i965/gen7_vs_state.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/src/mesa/drivers/dri/i965/gen7_vs_state.c
> > > > b/src/mesa/drivers/dri/i965/gen7_vs_state.c index 278b3ec..f60dcb1
> > > > 100644 --- a/src/mesa/drivers/dri/i965/gen7_vs_state.c
> > > > +++ b/src/mesa/drivers/dri/i965/gen7_vs_state.c
> > > > @@ -42,13 +42,13 @@ gen7_upload_constant_state(struct brw_context
> > > > *brw, 
> > > >     int dwords = brw->gen >= 8 ? 11 : 7;
> > > >     BEGIN_BATCH(dwords);
> > > > -   OUT_BATCH(opcode << 16 | (dwords - 2));
> > > > +   OUT_BATCH(opcode << 16 | (mocs << 8) | (dwords - 2));
> > > >     OUT_BATCH(active ? stage_state->push_const_size : 0);
> > > >     OUT_BATCH(0);
> > > >     /* Pointer to the constant buffer.  Covered by the set of state
> > > > flags
> > > >      * from gen6_prepare_wm_contants
> > > >      */
> > > > -   OUT_BATCH(active ? (stage_state->push_const_offset | mocs) : 0);
> > > > +   OUT_BATCH(active ? stage_state->push_const_offset : 0);
> > > >     OUT_BATCH(0);
> > > >     OUT_BATCH(0);
> > > >     OUT_BATCH(0);
> > > 
> > > Now I'm confused.
> > > 
> > > On BDW, DW0 bits 14:8 are MOCS, but "must always be programmed to
> > > zero." On pre-BDW, DW0 bits 14:8 are clearly marked as "Reserved MBZ".
> > > 
> > > The documentation for 3DSTATE_CONSTANT(Body) lists DWord 2 (of the
> > > body, aka DWord 3 of the 3DSTATE_CONSTANT_VS packet as a whole) bits
> > > 4:0 as "Constant Buffer Object Control State", for all pre-DevBDW
> > > platforms, which is MOCS for all four buffers.
> > > 
> > > So the code seems correct as it was, based on the versions of
> > > documentation I've read.  You must be looking at something different
> > > than I am...
> > > 
> > 
> > It did seem really strange to me. Everything I looked at for internal
> > docs seems to suggest DW0 was always MOCS (I am not looking at it now,
> > this is just from memory). Do me a favor if you haven't already... go
> > look at internal docs.
> 
> I actually was looking at the internal docs - specifically,
> 3DSTATE_CONSTANT_VS and 3DSTATE_CONSTANT(Body)[All].
> 
> > If you don't have time I can check again
> > tomorrow, but clearly my eyes must not be trustworthy. I've had been
> > looking at 3DSTATE_CONSTANT_PS. Earlier when I went to verify and
> > try to figure out how this could be wrong, I [foolishly] checked SNB
> > (see below). Anyway, here is everything I found from the PRMs which
> > suggest you're mostly right (though we agree the code is wrong for
> > BDW?).
> 
> The code is fine for BDW.  We OR 'mocs' into the wrong place, but we set
> it to zero so it doesn't actually have any effect.  Looks a bit funny
> but works, and since MOCS is required to be zero...hard to really care...
> 
> > PRE-SNB - Don't care
> > SNB DW0 (I didn't think SNB had MOCS...):
> > https://01.org/sites/default/files/documentation/snb_ihd_os_vol2_part1.pdf#page=286
> > IVB (MBZ)
> > https://01.org/sites/default/files/documentation/ivb_ihd_os_vol2_part1.pdf#page=291
> > HSW (MBZ)
> > https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-hsw-commandrefrence-instructions_0.pdf#page=771
> > BYT (MBZ)
> > https://01.org/sites/default/files/documentation/intel_os_gfx_prm_vol2_-_cmd_ref_instructions.pdf#page=35
> > BDW (DW0 as you mentioned)


Okay, I see what I was looking at which led me to get confused and you are right
(I went to our legacy docs and it's pretty clear that you are right). For
posterity, the above is correct and can be said concisely as:
MOCS is per constant buffer on GEN7. It's in DW0 for everything else.

Thanks for dealing with me and my misinterpretation of the docs.