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.pohjolai...@intel.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96878
> > CC: Jason Ekstrand <ja...@jlekstrand.net>
> > CC: Kenneth Graunke <kenn...@whitecape.org>
> > CC: arek.r...@gmail.com
> > CC: awa...@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 <kenn...@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);
> >
> >
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
