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

[Kenneth Graunke]

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)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150603/24137b71/attachment-0001.sig>