[Mesa-dev] [PATCH] i965/blorp/gen7+: Bring back push constant setup

[Pohjolainen, Topi]

On Wed, Jul 13, 2016 at 01:08:45AM -0700, Kenneth Graunke wrote:
> On Wednesday, July 13, 2016 10:10:51 AM PDT Topi Pohjolainen wrote:
> > This is partial revert of commit cc2d0e64.
> > 
> > It looks that even though blorp disables a stage the corresponding
> > 3DSTATE_CONSTANT_XS packet is needed to be programmed. Hardware
> > seems to try to fetch the constants even for disabled stages.
> > Therefore care needs to be taken that the constant buffer is
> > set up properly. Blorp will continue to trash it into non-existing
> > such as before.
> > It is possible that this could be omitted on SKL where the
> > constant buffer is considered when the corresponding binding table
> > settings are changed. Bspec:
> > 
> >   "The 3DSTATE_CONSTANT_* command is not committed to the shader
> >    unit until the corresponding (same shader)
> >    3DSTATE_BINDING_TABLE_POINTER_* command is parsed."
> > 
> > However, as CONSTANT_XS packet itself does not seem to stall on its
> > own, it is safer to emit the packets for SKL also.
> > 
> > Possible alternative to blorp trashing could have been to setup
> > defaults in the beginning of each batch buffer. However, hardware
> > doesn't seem to tolerate these packets being programmed multiple
> > times per primitive. Bspec for IVB:
> > 
> >   "It is invalid to execute this command more than once between
> >    3D_PRIMITIVE commands."
> > 
> > Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96878
> > CC: Jason Ekstrand <jason at jlekstrand.net>
> > CC: Kenneth Graunke <kenneth at whitecape.org>
> > CC: arek.rusi at gmail.com
> > CC: awatry at gmail.com
> > ---
> >  src/mesa/drivers/dri/i965/gen7_blorp.c | 24 ++++++++++++++++++++++++
> >  src/mesa/drivers/dri/i965/gen8_blorp.c | 28 ++++++++++++++++++++++++++++
> >  2 files changed, 52 insertions(+)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/gen7_blorp.c b/src/mesa/drivers/dri/i965/gen7_blorp.c
> > index 0afd76b..dc62a93 100644
> > --- a/src/mesa/drivers/dri/i965/gen7_blorp.c
> > +++ b/src/mesa/drivers/dri/i965/gen7_blorp.c
> > @@ -228,6 +228,24 @@ gen7_blorp_emit_surface_state(struct brw_context *brw,
> >     return wm_surf_offset;
> >  }
> >  
> > +/* Hardware seems to try to fetch the constants even though the corresponding
> > + * stage gets disables. Therefore make sure the settings for the constant
> > + * buffer are valid.
> > + */
> > +static void
> > +gen7_blorp_emit_disable_constant_state(struct brw_context *brw,
> > +                                       unsigned opcode)
> 
> As is, this is:
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
> 
> A couple of small suggestions:
> 
> - Why not use this for disabling PS constants too?  Otherwise, you've
>   basically got the same function twice...for no real benefit.
> - "emit disable" sounds a little odd to me, perhaps just call it
>   "gen7_blorp_disable_constant_state" (either way is fine)

I already have a follow-up patch for it :)

> 
> > +{
> > +   BEGIN_BATCH(7);
> > +   OUT_BATCH(opcode << 16 | (7 - 2));
> > +   OUT_BATCH(0);
> > +   OUT_BATCH(0);
> > +   OUT_BATCH(0);
> > +   OUT_BATCH(0);
> > +   OUT_BATCH(0);
> > +   OUT_BATCH(0);
> > +   ADVANCE_BATCH();
> > +}
> >  
> >  /* 3DSTATE_VS
> >   *
> > @@ -760,6 +778,12 @@ gen7_blorp_exec(struct brw_context *brw,
> >        gen7_blorp_emit_blend_state_pointer(brw, cc_blend_state_offset);
> >        gen7_blorp_emit_cc_state_pointer(brw, cc_state_offset);
> >     }
> > +
> > +   gen7_blorp_emit_disable_constant_state(brw, _3DSTATE_CONSTANT_VS);
> > +   gen7_blorp_emit_disable_constant_state(brw, _3DSTATE_CONSTANT_HS);
> > +   gen7_blorp_emit_disable_constant_state(brw, _3DSTATE_CONSTANT_DS);
> > +   gen7_blorp_emit_disable_constant_state(brw, _3DSTATE_CONSTANT_GS);
> > +
> >     depthstencil_offset = gen6_blorp_emit_depth_stencil_state(brw, params);
> >     gen7_blorp_emit_depth_stencil_state_pointers(brw, depthstencil_offset);
> >     if (brw->use_resource_streamer)
> > diff --git a/src/mesa/drivers/dri/i965/gen8_blorp.c b/src/mesa/drivers/dri/i965/gen8_blorp.c
> > index 7ca24a8..bc113ec 100644
> > --- a/src/mesa/drivers/dri/i965/gen8_blorp.c
> > +++ b/src/mesa/drivers/dri/i965/gen8_blorp.c
> > @@ -156,6 +156,29 @@ gen8_blorp_emit_blend_state(struct brw_context *brw,
> >     return blend_state_offset;
> >  }
> >  
> > +/* Hardware seems to try to fetch the constants even though the corresponding
> > + * stage gets disables. Therefore make sure the settings for the constant
> > + * buffer are valid.
> > + */
> > +static void
> > +gen8_blorp_emit_disable_constant_state(struct brw_context *brw,
> > +                                       unsigned opcode)
> > +{
> > +   BEGIN_BATCH(11);
> > +   OUT_BATCH(opcode << 16 | (11 - 2));
> > +   OUT_BATCH(0);
> > +   OUT_BATCH(0);
> > +   OUT_BATCH(0);
> > +   OUT_BATCH(0);
> > +   OUT_BATCH(0);
> > +   OUT_BATCH(0);
> > +   OUT_BATCH(0);
> > +   OUT_BATCH(0);
> > +   OUT_BATCH(0);
> > +   OUT_BATCH(0);
> > +   ADVANCE_BATCH();
> > +}
> > +
> >  /* 3DSTATE_VS
> >   *
> >   * Disable vertex shader.
> > @@ -657,6 +680,11 @@ gen8_blorp_exec(struct brw_context *brw, const struct brw_blorp_params *params)
> >     const uint32_t cc_state_offset = gen6_blorp_emit_cc_state(brw);
> >     gen7_blorp_emit_cc_state_pointer(brw, cc_state_offset);
> >  
> > +   gen8_blorp_emit_disable_constant_state(brw, _3DSTATE_CONSTANT_VS);
> > +   gen8_blorp_emit_disable_constant_state(brw, _3DSTATE_CONSTANT_HS);
> > +   gen8_blorp_emit_disable_constant_state(brw, _3DSTATE_CONSTANT_DS);
> > +   gen8_blorp_emit_disable_constant_state(brw, _3DSTATE_CONSTANT_GS);
> > +
> >     gen8_blorp_emit_disable_constant_ps(brw);
> >     wm_bind_bo_offset = gen8_blorp_emit_surface_states(brw, params);
> >  
> > 
>