[Mesa-dev] [PATCH v03 18/38] i965: Port Gen6+ DEPTH_STENCIL state to genxml.

[Kenneth Graunke]

On Wednesday, May 3, 2017 9:59:03 PM PDT Pohjolainen, Topi wrote:
> On Mon, May 01, 2017 at 06:43:06PM -0700, Rafael Antognolli wrote:
> > +#if GEN_GEN == 6
> > +   brw_batch_emit(brw, GENX(3DSTATE_CC_STATE_POINTERS), ptr) {
> > +      ptr.PointertoDEPTH_STENCIL_STATE = ds_offset;
> > +      ptr.DEPTH_STENCIL_STATEChange = true;
> > +   }
> > +#elif GEN_GEN == 7
> > +   brw_batch_emit(brw, GENX(3DSTATE_DEPTH_STENCIL_STATE_POINTERS), ptr) {
> > +      ptr.PointertoDEPTH_STENCIL_STATE = ds_offset;
> 
> Don't we need here also:
> 
>          ptr.DEPTH_STENCIL_STATEChange = true;

No - in gen7.xml bit 32 is marked "mbo" or "Must Be One".  The genxml
packing magic will automatically set that for us.  It's a bit
surprising, but there's actually no named field, so you can't set it
explicitly.

> > +   }
> > +#endif
> > +}
> > +
> > +static const struct brw_tracked_state genX(depth_stencil_state) = {
> > +   .dirty = {
> > +      .mesa = _NEW_BUFFERS |
> > +              _NEW_DEPTH |
> > +              _NEW_STENCIL,
> > +      .brw  = BRW_NEW_BLORP |
> > +              (GEN_GEN >= 8 ? BRW_NEW_CONTEXT
> 
> Shouldn't this be >= 6 or am I missing something?

Gen6+ uses DEPTH_STENCIL_STATE which has to be re-emitted on every batch
buffer (BRW_NEW_BATCH).  On Gen8+ it's 3DSTATE_WM_DEPTH_STENCIL (an
actual packet) which doesn't need to be re-emitted every batch.  We use
BRW_NEW_CONTEXT.

BRW_NEW_CONTEXT is unnecessary if you listen to BRW_NEW_BATCH.  It's
signalled on context creation (but we signal everything, so...not sure
what the point is)...and on new batch buffers when there isn't a hw_ctx
(but we already signal BRW_NEW_BATCH).  Honestly, I've thought about
just canning the bit entirely...

> 
> > +                            : BRW_NEW_BATCH |
> > +                              BRW_NEW_STATE_BASE_ADDRESS),
> > +   },
> > +   .emit = genX(upload_depth_stencil_state),
> > +};
> > +
> > +/* ---------------------------------------------------------------------- */
> > +
> > +#endif
> > +
> >  void
> >  genX(init_atoms)(struct brw_context *brw)
> >  {
> > @@ -250,7 +349,7 @@ genX(init_atoms)(struct brw_context *brw)
> 
> Isn't this leaving gen6 with:
> 
>          &gen6_depth_stencil_state,       /* must do before cc unit */

Yeah, but &genX(depth_stencil_state) is &gen6_depth_stencil_state so
it's OK.  We should probably change it nonetheless...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170503/670a64fd/attachment.sig>