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

[Ilia Mirkin]

On Mon, Aug 10, 2015 at 4:29 AM, Lofstedt, Marta
<marta.lofstedt at intel.com> wrote:
>> -----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?

Merely providing an alternative mechanism to write the same logic that
would not cause an extra indent level. and would produce an error when
called on a driver without MemoryBarrier implemented. But I don't feel
strongly about that. I assumed someone would actually be checking this
against various conformance tests and check it in [didn't even realize
that you didn't have push privileges, tbh].