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

[Francisco Jerez]

Iago Toral <itoral at igalia.com> writes:

> 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?
>
Yes and no.  As far as I'm aware the execution semantics of the original
ARB_shader_atomic_counters extension were rather sloppy (or let's say
opportunistic), I don't think it would be necessarily illegal for an
implementation to omit the side effects of atomic counter operations
where a fragment shader invocation can be optimized out e.g. due to
early depth testing, and it may in fact have some performance benefit.

GL4.x and ARB_shader_image_load_store are more explicit about when
shader invocations can be expected to have side effects, and therefore
we'll have to start treating them the same way as images at some point
in the future.  It could also be argued that we should already force
late fragment tests for atomic counters just because we expose the
ARB_shader_image_load_store extension even if the program doesn't
actually use it.  Either way it should be okay to treat atomic counters
the same way because at worst it will make the implementation slightly
more strict than the spec requires.

>> > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151218/7fc08689/attachment.sig>