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

[Pohjolainen, Topi]

On Wed, May 03, 2017 at 11:28:31PM -0700, Kenneth Graunke wrote:
> 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.

Ok. Thanks for explaining.

> 
> > > +   }
> > > +#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...

Right. This is fine then.

> 
> > 
> > > +                            : 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...

Ok.