[virglrenderer-devel] [PATCH 2/4] vrend_shader: generate bindings with layout qualifiers

[Erik Faye-Lund]

On 07. aug. 2018 11:31, Dave Airlie wrote:
>
>
> On Tue., 7 Aug. 2018, 18:44 Erik Faye-Lund, 
> <erik.faye-lund at collabora.com <mailto:erik.faye-lund at collabora.com>> 
> wrote:
>
>     On 07. aug. 2018 02:51, Dave Airlie wrote:
>     > On 7 August 2018 at 04:38, Erik Faye-Lund
>     <erik.faye-lund at collabora.com
>     <mailto:erik.faye-lund at collabora.com>> wrote:
>     >> OpenGL ES 3.1 doesn't support using glUniform1i() for binding
>     >> images, so let's use layout qualifiers instead. This should in
>     >> theory be slightly more performant as well, as we do less API
>     >> calls.
>     > I like this idea, I think we can get rid of a bunch of stuff with
>     > explicit bindings,
>     >
>     > However I think explicit layout bindings on GL require a shader
>     extension,
>     >
>     > At least on desktop GL a lot of tests fail after I apply this.
>     >
>     > Dave.
>
>     Hmm, that's odd. I don't think there's any extensions that should be
>     required by this, but I'll take a look.
>
>     Thanks for pointing this out.
>
>
> Our baseline glsl version is 130 btw
>
>
Right.

I think I got a relevant error here:

vrend_compile_shader: context error reported 8 "Xorg" Illegal shader 0
shader failed to compile
0:9(1): error: the "binding" qualifier only applies to uniform blocks, 
storage blocks, opaque variables, or arrays thereof

GLSL:
#version 330
#extension GL_ARB_compute_shader : require
#extension GL_ARB_shader_storage_buffer_object : require
#extension GL_ARB_shader_bit_encoding : require
#extension GL_ARB_shader_image_load_store : require
layout (local_size_x = 2, local_size_y = 4, local_size_z = 1) in;
vec4 temp0[3];
uint ssbo_addr_temp;
layout(binding=0, r32ui) uniform uimage2D csimg0;
layout (binding = 6, std430) buffer csssbo6 { uint csssbocontents6[]; };
void main(void)
{
temp0[0].xy = vec2(uintBitsToFloat((uvec4(gl_WorkGroupID.x, 
gl_WorkGroupID.y, gl_WorkGroupID.z, gl_WorkGroupID.z) * 
(uvec4(2U,4U,1U,1U)) + uvec4(gl_LocalInvocationID.x, 
gl_LocalInvocationID.y, gl_LocalInvocationID.z, 
gl_LocalInvocationID.z)).xy));
temp0[1].x = float(uintBitsToFloat((uvec4(uvec4(gl_NumWorkGroups.x, 
gl_NumWorkGroups.x, gl_NumWorkGroups.x, gl_NumWorkGroups.x)) * 
uvec4((uvec4(2U,2U,2U,2U))))).x);
temp0[2].x = uintBitsToFloat(imageLoad(csimg0, ivec2(floatBitsToInt( 
temp0[0].xyxx ))).x);
temp0[0].x = float(uintBitsToFloat((floatBitsToUint(temp0[0].yyyy) * 
floatBitsToUint(temp0[1].xxxx) + floatBitsToUint(temp0[0].xxxx)).x));
temp0[0].x = 
float(uintBitsToFloat((uvec4(floatBitsToUint(temp0[0].xxxx)) * 
uvec4((uvec4(16U,16U,16U,16U))))).x);
csssbocontents6[uint(floatBitsToUint( temp0[0].xxxx ))>>2] = 
floatBitsToUint( temp0[2].xxxx ).x;
}

Checking the mesa source that's from:

---8<---
     } else if ((state->is_version(420, 310) ||
                 state->ARB_shading_language_420pack_enable) &&
                base_type->is_image()) {
        assert(ctx->Const.MaxImageUnits <= MAX_IMAGE_UNITS);
        if (max_index >= ctx->Const.MaxImageUnits) {
           _mesa_glsl_error(loc, state, "Image binding %d exceeds the "
                            "maximum number of image units (%d)", max_index,
                            ctx->Const.MaxImageUnits);
           return;
        }

     } else {
        _mesa_glsl_error(loc, state,
                         "the \"binding\" qualifier only applies to 
uniform "
                         "blocks, storage blocks, opaque variables, or 
arrays "
                         "thereof");
        return;
     }
---8<---

... Yeah, seems we need GLSL 420 (or 
ARB_shading_language_420pack_enable) to use this construct.

So, question is... Should we
a) make GLSL 420 a hard requirement for images, or
b) keep the old approach if on GL 4.1 (or ealier) + 
GL_ARB_shader_image_load_store?

I guess a) is the "nicer" thing to do, but it makes the support burden a 
bit worse.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/virglrenderer-devel/attachments/20180807/a1a8c904/attachment.html>