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

[Ben Widawsky]

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. 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?).

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)