[Mesa-dev] [PATCH v2] gles/es3.1: Implement glMemoryBarrierByRegion

[Lofstedt, Marta]

> -----Original Message-----
> From: mesa-dev [mailto:mesa-dev-bounces at lists.freedesktop.org] On
> Behalf Of Ilia Mirkin
> Sent: Friday, August 7, 2015 9:56 PM
> To: Matt Turner
> Cc: mesa-dev at lists.freedesktop.org
> Subject: Re: [Mesa-dev] [PATCH v2] gles/es3.1: Implement
> glMemoryBarrierByRegion
> 
> On Fri, Aug 7, 2015 at 2:18 PM, Matt Turner <mattst88 at gmail.com> wrote:
> > On Tue, Aug 4, 2015 at 1:22 AM, Marta Lofstedt
> > <marta.lofstedt at linux.intel.com> wrote:
> >> From: Marta Lofstedt <marta.lofstedt at intel.com>
> >>
> >> Signed-off-by: Marta Lofstedt <marta.lofstedt at intel.com>
> >> ---
> >>  src/mapi/glapi/gen/gl_API.xml           |  4 ++++
> >>  src/mesa/main/shaderimage.c             | 40
> +++++++++++++++++++++++++++++++++
> >>  src/mesa/main/shaderimage.h             |  3 +++
> >>  src/mesa/main/tests/dispatch_sanity.cpp |  3 +--
> >>  4 files changed, 48 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/mapi/glapi/gen/gl_API.xml
> >> b/src/mapi/glapi/gen/gl_API.xml index 658efa4..3db4349 100644
> >> --- a/src/mapi/glapi/gen/gl_API.xml
> >> +++ b/src/mapi/glapi/gen/gl_API.xml
> >> @@ -2966,6 +2966,10 @@
> >>          <param name="height" type="GLsizei"/>
> >>          <glx rop="191"/>
> >>      </function>
> >> +
> >> +    <function name="MemoryBarrierByRegion" es2="3.1">
> >> +        <param name="barriers" type="GLbitfield"/>
> >> +    </function>
> >>  </category>
> >>
> >>  <category name="1.1">
> >> diff --git a/src/mesa/main/shaderimage.c
> >> b/src/mesa/main/shaderimage.c index a348cdb..7337f22 100644
> >> --- a/src/mesa/main/shaderimage.c
> >> +++ b/src/mesa/main/shaderimage.c
> >> @@ -653,3 +653,43 @@ _mesa_MemoryBarrier(GLbitfield barriers)
> >>     if (ctx->Driver.MemoryBarrier)
> >>        ctx->Driver.MemoryBarrier(ctx, barriers);  }
> >> +
> >> +void GLAPIENTRY
> >> +_mesa_MemoryBarrierByRegion(GLbitfield barriers) {
> >> +   GET_CURRENT_CONTEXT(ctx);
> >> +
> >> +   GLbitfield all_allowed_bits = GL_ATOMIC_COUNTER_BARRIER_BIT |
> >> +                                 GL_FRAMEBUFFER_BARRIER_BIT |
> >> +                                 GL_SHADER_IMAGE_ACCESS_BARRIER_BIT |
> >> +                                 GL_SHADER_STORAGE_BARRIER_BIT |
> >> +                                 GL_TEXTURE_FETCH_BARRIER_BIT |
> >> +                                 GL_UNIFORM_BARRIER_BIT;
> >> +
> >> +   if (ctx->Driver.MemoryBarrier) {
> >> +      /* From section 7.11.2 of the OpenGL ES 3.1 specification:
> >> +       *
> >> +       *    "When barriers is ALL_BARRIER_BITS, shader memory accesses
> will be
> >> +       *     synchronized relative to all these barrier bits, but not to other
> >> +       *     barrier bits specific to MemoryBarrier."
> >> +       *
> >> +       * That is, if barriers is the special value GL_ALL_BARRIER_BITS, then
> all
> >> +       * barriers allowed by glMemoryBarrierByRegion should be
> activated."
> >> +       */
> >> +      if (barriers == GL_ALL_BARRIER_BITS)
> >> +         return ctx->Driver.MemoryBarrier(ctx, all_allowed_bits);
> >> +
> >> +      /* From section 7.11.2 of the OpenGL ES 3.1 specification:
> >> +       *
> >> +       *    "An INVALID_VALUE error is generated if barriers is not the
> special
> >> +       *     value ALL_BARRIER_BITS, and has any bits set other than those
> >> +       *     described above."
> >> +       */
> >> +      if ((barriers & ~all_allowed_bits) != 0) {
> >> +         _mesa_error(ctx, GL_INVALID_VALUE,
> >> +                     "glMemoryBarrierByRegion(unsupported barrier bit");
> >> +      }
> >> +
> >> +      ctx->Driver.MemoryBarrier(ctx, barriers);
> >> +   }
> >
> > Would probably be nice to put an unreachable("not implemented") as an
> > else case for future implementors.
> >
> > Reviewed-by: Matt Turner <mattst88 at gmail.com>
> 
> I wonder if this shouldn't just be
> 
> if (!ctx->Driver.MemoryBarrier)
>   INVALID_OPERATION
> 
> But this is largely hypothetical... I'm not too worried about it.
> 
Hi Ilia,

Since the patch isn't merged I assume you want me to change something, but I am not sure what I should change. 

I see no consensus in the existing code on what to do at:
If (!ctx->Driver.NNN)

Ilia, are you suggesting that setting an INVALID_OPERATION should be the "new" standard way of handling this?

> >
> > Thanks!
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev