[Mesa-dev] [PATCH v3 2/3] i965: Ensure FS execution in presence of atomic buffers

[Samuel Iglesias Gonsálvez]

On Mon, 2015-12-21 at 19:24 +0200, Francisco Jerez wrote:
> Iago Toral Quiroga <itoral at igalia.com> writes:
> 
> > On Haswell we need to set the UAV_ONLY WM state bit when there are
> > no colour
> > or depth buffer writes and on all hardware we should set the early
> > depth/stencil control field to PSEXEC unless early fragment tests
> > are enabled
> > to make sure that the fragment shader is executed regardless of
> > whether
> > per-fragment tests pass or not as the spec requires.
> > 
> > So far we have been doing this for images only, but we should apply
> > the same
> > treatment to all side effectful scenarios. Suggested by Curro.
> 
> I think it would be useful to clarify here that this is not strictly
> required for compliance with the original ARB_shader_atomic_counters
> extension, it's only necessary to get the execution semantics
> specified
> in GL4.2+ right.
> 
> > ---
> >  src/mesa/drivers/dri/i965/gen7_wm_state.c | 9 +++++----
> >  src/mesa/drivers/dri/i965/gen8_ps_state.c | 2 +-
> >  2 files changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/gen7_wm_state.c
> > b/src/mesa/drivers/dri/i965/gen7_wm_state.c
> > index a6d1028..74a2cf3 100644
> > --- a/src/mesa/drivers/dri/i965/gen7_wm_state.c
> > +++ b/src/mesa/drivers/dri/i965/gen7_wm_state.c
> > @@ -78,9 +78,10 @@ upload_wm_state(struct brw_context *brw)
> >     }
> >  
> >     /* _NEW_BUFFERS | _NEW_COLOR */
> > +   bool active_fs_has_side_effects =
> 
> If you mark this const and change the commit message as suggested
> above
> the series is:
> 
> Reviewed-by: Francisco Jerez <currojerez at riseup.net>
> 

OK. Thanks a lot!

Sam

> > +      _mesa_active_fragment_shader_has_side_effects(&brw->ctx);
> >     if (brw_color_buffer_write_enabled(brw) || writes_depth ||
> > -       _mesa_active_fragment_shader_has_side_effects(&brw->ctx) ||
> > -       dw1 & GEN7_WM_KILL_ENABLE) {
> > +       active_fs_has_side_effects || dw1 & GEN7_WM_KILL_ENABLE) {
> >        dw1 |= GEN7_WM_DISPATCH_ENABLE;
> >     }
> >     if (multisampled_fbo) {
> > @@ -106,7 +107,7 @@ upload_wm_state(struct brw_context *brw)
> >     /* BRW_NEW_FS_PROG_DATA */
> >     if (prog_data->early_fragment_tests)
> >        dw1 |= GEN7_WM_EARLY_DS_CONTROL_PREPS;
> > -   else if (prog_data->base.nr_image_params)
> > +   else if (active_fs_has_side_effects)
> >        dw1 |= GEN7_WM_EARLY_DS_CONTROL_PSEXEC;
> >  
> >     /* The "UAV access enable" bits are unnecessary on HSW because
> > they only
> > @@ -119,7 +120,7 @@ upload_wm_state(struct brw_context *brw)
> >      */
> >     if (brw->is_haswell &&
> >         !(brw_color_buffer_write_enabled(brw) || writes_depth) &&
> > -       prog_data->base.nr_image_params)
> > +       active_fs_has_side_effects)
> >        dw2 |= HSW_WM_UAV_ONLY;
> >  
> >     BEGIN_BATCH(3);
> > diff --git a/src/mesa/drivers/dri/i965/gen8_ps_state.c
> > b/src/mesa/drivers/dri/i965/gen8_ps_state.c
> > index 3cc8c68..74cdcef 100644
> > --- a/src/mesa/drivers/dri/i965/gen8_ps_state.c
> > +++ b/src/mesa/drivers/dri/i965/gen8_ps_state.c
> > @@ -156,7 +156,7 @@ upload_wm_state(struct brw_context *brw)
> >     /* BRW_NEW_FS_PROG_DATA */
> >     if (brw->wm.prog_data->early_fragment_tests)
> >        dw1 |= GEN7_WM_EARLY_DS_CONTROL_PREPS;
> > -   else if (brw->wm.prog_data->base.nr_image_params)
> > +   else if (_mesa_active_fragment_shader_has_side_effects(&brw-
> > >ctx))
> >        dw1 |= GEN7_WM_EARLY_DS_CONTROL_PSEXEC;
> >  
> >     BEGIN_BATCH(2);
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev