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

[Kenneth Graunke]

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)

> +{
> +   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);
>  
> 

-------------- 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: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160713/63ba0da8/attachment-0001.sig>