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

[Juan A. Suarez Romero]

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).

	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
> 
>