[Mesa-dev] [PATCH 4/4] i965/es3.1: Implement glMemoryBarrierByRegion

[Lofstedt, Marta]

> -----Original Message-----
> From: mesa-dev [mailto:mesa-dev-bounces at lists.freedesktop.org] On
> Behalf Of Matt Turner
> Sent: Friday, July 31, 2015 7:48 PM
> To: Ilia Mirkin
> Cc: mesa-dev at lists.freedesktop.org
> Subject: Re: [Mesa-dev] [PATCH 4/4] i965/es3.1: Implement
> glMemoryBarrierByRegion
> 
> On Fri, Jul 31, 2015 at 10:25 AM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> > On Fri, Jul 31, 2015 at 8:15 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/mesa/drivers/dri/i965/brw_program.c | 34
> >> +++++++++++++++++++++++++++++++++
> >>  1 file changed, 34 insertions(+)
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/brw_program.c
> >> b/src/mesa/drivers/dri/i965/brw_program.c
> >> index 85e271d..332d84e 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_program.c
> >> +++ b/src/mesa/drivers/dri/i965/brw_program.c
> >> @@ -226,6 +226,39 @@ brw_memory_barrier(struct gl_context *ctx,
> GLbitfield barriers)
> >>     brw_emit_pipe_control_flush(brw, bits);  }
> >>
> >> +static void
> >> +brw_memory_barrier_by_region(struct gl_context *ctx, GLbitfield
> >> +barriers) {
> >> +   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;
> >> +   /*
> >> +    * According to OpenGL ES 3.1 spec. April 29, 2015, 7.11.2:
> >> +    * "When barriers are ALL_BARRIERS_BIT, shader memory access
> >> +    * will be synchronized realtive to all theese barrier bits,
> >> +    * but not to other barrier bits specific to MemoryBarrier."
> >> +    * I.e if bariiers is the special value GL_ALL_BARRIER_BITS,
> >> +    * then all barriers allowed by glMemoryBarrierByRegion
> >> +    * should be activated.
> >> +   */
> >> +   if (barriers == GL_ALL_BARRIER_BITS)
> >> +      return brw_memory_barrier(ctx, all_allowed_bits);
> >> +
> >> +   /*
> >> +    * If barriers contain a value that is not allowed
> >> +    * for glMemoryBarrierByRegion an GL_INVALID_VALUE
> >> +    * should be generated.
> >> +   */
> >> +   if ((all_allowed_bits | barriers) ^ all_allowed_bits)
> >> +       _mesa_error(ctx, GL_INVALID_VALUE,
> >> +            "glMemoryBarrierByRegion(unsupported barrier bit");
> >
> > It's fairly unusual to do _mesa_error() in the driver. It's done for
> > some texture stuff, but in general such checking is left to the common
> > implementation.
> >
> > Is the list of allowed bits a per-driver thing? If so, perhaps there
> > should be a ctx->Const.MemoryBarriers bitfield?
> 
> I don't think so. It seems to be that exact list, according to the spec.
> 
> > And then you wouldn't even need this separate callback (at least for
> > now) and just use functions->MemoryBarrier().
> 
> Oh, that's a good point.

Thanks for the comments Ilia and Matt, both here and in: 
http://patchwork.freedesktop.org/patch/51879/

About, the coding style comments, I will of course just follow these.
Also, I agree to move the tests for the allowed bits to main/shaderimage.c 

However, I don't agree that the driver interface should be removed.
In my opinion, it is a driver specific decision if glMemoryBarrierByRegion should be
implemented directly on top of glMemoryBarrier or if you want to do some
optimizations. Why else would there be a specific glMemoryBarrierByRegion
function added to the standard?

> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
----------------------------------------------------------------------
Intel Sweden AB
Registered Office: Knarrarnasgatan 15, 164 40 Kista, Stockholm, Sweden
Registration Number: 556189-6027

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.