[Mesa-dev] [PATCH] glsl: xfb_stride applies to buffers, not block members

[Timothy Arceri]

On 10/07/17 17:58, Juan A. Suarez Romero wrote:
> On Mon, 2017-07-10 at 14:30 +0800, Timothy Arceri wrote:
>>
>> On 8 July 2017 12:19:48 am GMT+08:00, "Juan A. Suarez Romero" <jasuarez at igalia.com> wrote:
>>> When we have an interface block like:
>>>
>>> layout (xfb_buffer = 0, xfb_offset = 0) out Block {
>>>                              vec4 var1;
>>>     layout (xfb_stride = 48) vec4 var2;
>>>                              vec4 var3;
>>> };
>>>
>>> According to ARB_enhanced_layouts spec:
>>>
>>>    "The *xfb_stride* qualifier specifies how many bytes are consumed by
>>>     each captured vertex.  It applies to the transform feedback buffer
>>>     for that declaration, whether it is inherited or explicitly
>>>     declared. It can be applied to variables, blocks, block members, or
>>>     just the qualifier out. [ ...] While *xfb_stride* can be declared
>>>     multiple times for the same buffer, it is a compile-time or
>>>     link-time error to have different values specified for the stride
>>>     for the same buffer."
>>>
>>> This means xfb_stride actually applies to the buffer, and not to the
>>> individual components.
>>
>> Right. As I understand it, it applies to the buffer but can be set via a qualifier on a block member as the spec quote says above.
>>
>>>
>>> In the above example, it means that var2 consumes 16 bytes, and var3 is
>>> at offset 32.
>>>
>>> This has been confirmed also by John Kessenich, the main contact for
>>> the
>>> ARB_enhanced_layouts specs,
>>
>> What exactly did he confirm? The example you give above or just that the stride applies to the buffer.
>>
> 
> Both:
> 
> * Strides applies to buffer, not to block member. Hence, var2 requires
> 16 bytes, not 48.
> 
> * So this means that offsets for block members are, respectively, 0, 16
> and 32.
> 
>> Are you saying that the qualifier should be ignored besides throwing link/compile errors for block members?
>>
>> My assumption was it should just apply to the whole block if it was set for any member.
>>
> 
> Correct. That is what I'm saying. The qualifier applies to the full
> block.
> 
> 
> The main issue this patch fixes is that right now, Mesa is returning
> var3 offset as 64 = 16 + 48, being 16 the var2 offset and 48 the
> stride. In other words, it is applying the stride in the block member.
> 
> The right value should be 32 (16 + 16, var2 offset and var2 size).

Apologies for the delay I've been on holiday. In this case I wonder if 
something like the following actually sets the stride correctly.

layout (xfb_buffer = 0, xfb_offset = 0) out Block {
                               vec4 var1;
      layout (xfb_stride = 64) vec4 var2;
                               vec4 var3;
} arr[2];

anyway this patch looks correct, thanks.

Reviewed-by: Timothy Arceri <tarceri at itsqueeze.com>

> 
> 	J.A.
> 
> 
> 
> 
>> I can't look at the CTS until next week.
>>
>> and also because this commit fixes:
>>>
>>> GL45.enhanced_layouts.xfb_block_member_stride
>>>
>>> This commit is in practice a revert of 598790e8564 (glsl: apply
>>> xfb_stride to implicit offsets for ifc block members).
>>> ---
>>> src/compiler/glsl/ast_to_hir.cpp | 5 ++---
>>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/compiler/glsl/ast_to_hir.cpp
>>> b/src/compiler/glsl/ast_to_hir.cpp
>>> index c338ad7..3968657 100644
>>> --- a/src/compiler/glsl/ast_to_hir.cpp
>>> +++ b/src/compiler/glsl/ast_to_hir.cpp
>>> @@ -7372,14 +7372,13 @@
>>> ast_process_struct_or_iface_block_members(exec_list *instructions,
>>>                                           qual->offset, &xfb_offset)) {
>>>                 fields[i].offset = xfb_offset;
>>>                 block_xfb_offset = fields[i].offset +
>>> -                  MAX2(xfb_stride, (int) (4 *
>>> field_type->component_slots()));
>>> +                  4 * field_type->component_slots();
>>>              }
>>>           } else {
>>>              if (layout && layout->flags.q.explicit_xfb_offset) {
>>>                 unsigned align = field_type->is_64bit() ? 8 : 4;
>>>                 fields[i].offset = glsl_align(block_xfb_offset, align);
>>> -               block_xfb_offset +=
>>> -                  MAX2(xfb_stride, (int) (4 *
>>> field_type->component_slots()));
>>> +               block_xfb_offset += 4 * field_type->component_slots();
>>>              }
>>>           }
>>>
>>> -- 
>>> 2.9.4
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>>