On Mon, Aug 10, 2015 at 4:29 AM, Lofstedt, Marta <marta.lofst...@intel.com> wrote: >> -----Original Message----- >> From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On >> Behalf Of Ilia Mirkin >> Sent: Friday, August 7, 2015 9:56 PM >> To: Matt Turner >> Cc: mesa-dev@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 <matts...@gmail.com> wrote: >> > On Tue, Aug 4, 2015 at 1:22 AM, Marta Lofstedt >> > <marta.lofst...@linux.intel.com> wrote: >> >> From: Marta Lofstedt <marta.lofst...@intel.com> >> >> >> >> Signed-off-by: Marta Lofstedt <marta.lofst...@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 <matts...@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]. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
