[Mesa-dev] [PATCH v2 1/2] mesa: Add a _mesa_active_fragment_shader_has_side_effects helper

[Iago Toral]

On Thu, 2015-12-17 at 16:29 +0200, Francisco Jerez wrote:
> Iago Toral Quiroga <itoral at igalia.com> writes:
> 
> > Some drivers can disable the FS unit if there is nothing in the shader code
> > that writes to an output (i.e. color, depth, etc). Right now, mesa has
> > a function to check for atomic buffers and the i965 driver also checks for
> > images. Refactor this logic into a generic function that we can use for
> > any source of side effects in a fragment shader. Sugested by Jason.
> > ---
> >  src/mesa/drivers/dri/i965/gen7_wm_state.c |  6 +-----
> >  src/mesa/drivers/dri/i965/gen8_ps_state.c |  3 +--
> >  src/mesa/main/mtypes.h                    | 15 ++++++++++++---
> >  3 files changed, 14 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/gen7_wm_state.c b/src/mesa/drivers/dri/i965/gen7_wm_state.c
> > index 06d5e65..a6d1028 100644
> > --- a/src/mesa/drivers/dri/i965/gen7_wm_state.c
> > +++ b/src/mesa/drivers/dri/i965/gen7_wm_state.c
> > @@ -77,13 +77,9 @@ upload_wm_state(struct brw_context *brw)
> >        dw1 |= GEN7_WM_KILL_ENABLE;
> >     }
> >  
> > -   if (_mesa_active_fragment_shader_has_atomic_ops(&brw->ctx)) {
> > -      dw1 |= GEN7_WM_DISPATCH_ENABLE;
> > -   }
> > -
> >     /* _NEW_BUFFERS | _NEW_COLOR */
> >     if (brw_color_buffer_write_enabled(brw) || writes_depth ||
> > -       prog_data->base.nr_image_params ||
> > +       _mesa_active_fragment_shader_has_side_effects(&brw->ctx) ||
> >         dw1 & GEN7_WM_KILL_ENABLE) {
> >        dw1 |= GEN7_WM_DISPATCH_ENABLE;
> >     }
> 
> Hey, it looks like SSBOs are still missing a couple of things that could
> make their side effects rather non-deterministic on i965 hardware: On
> HSW you should probably set the UAV_ONLY WM state bit when there are no
> colour or depth buffer writes as is done for images below in this same
> function, and on all hardware you 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.

Sure, I'll add this and send a new version. Thanks Curro!

BTW, I see that we are doing these two things only for images at the
moment, I guess we should we do it for atomic buffers as well, right?

> > diff --git a/src/mesa/drivers/dri/i965/gen8_ps_state.c b/src/mesa/drivers/dri/i965/gen8_ps_state.c
> > index 945f710..3cc8c68 100644
> > --- a/src/mesa/drivers/dri/i965/gen8_ps_state.c
> > +++ b/src/mesa/drivers/dri/i965/gen8_ps_state.c
> > @@ -90,8 +90,7 @@ gen8_upload_ps_extra(struct brw_context *brw,
> >      *
> >      * BRW_NEW_FS_PROG_DATA | BRW_NEW_FRAGMENT_PROGRAM | _NEW_BUFFERS | _NEW_COLOR
> >      */
> > -   if ((_mesa_active_fragment_shader_has_atomic_ops(&brw->ctx) ||
> > -        prog_data->base.nr_image_params) &&
> > +   if (_mesa_active_fragment_shader_has_side_effects(&brw->ctx) &&
> >         !brw_color_buffer_write_enabled(brw))
> >        dw1 |= GEN8_PSX_SHADER_HAS_UAV;
> >  
> > diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> > index 191a9ea..834ba59 100644
> > --- a/src/mesa/main/mtypes.h
> > +++ b/src/mesa/main/mtypes.h
> > @@ -4538,11 +4538,20 @@ enum _debug
> >     DEBUG_INCOMPLETE_FBO         = (1 << 3)
> >  };
> >  
> > +/**
> > + * Checks if the active fragment shader program can have side effects due
> > + * to use of things like atomic buffers or images
> > + */
> >  static inline bool
> > -_mesa_active_fragment_shader_has_atomic_ops(const struct gl_context *ctx)
> > +_mesa_active_fragment_shader_has_side_effects(const struct gl_context *ctx)
> >  {
> > -   return ctx->Shader._CurrentFragmentProgram != NULL &&
> > -      ctx->Shader._CurrentFragmentProgram->_LinkedShaders[MESA_SHADER_FRAGMENT]->NumAtomicBuffers > 0;
> > +   const struct gl_shader *sh;
> > +
> > +   if (!ctx->Shader._CurrentFragmentProgram)
> > +      return false;
> > +
> > +   sh = ctx->Shader._CurrentFragmentProgram->_LinkedShaders[MESA_SHADER_FRAGMENT];
> > +   return sh->NumAtomicBuffers > 0 || sh->NumImages > 0;
> >  }
> >  
> >  #ifdef __cplusplus
> > -- 
> > 1.9.1
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev