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

2017-07-18 Thread 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" 
 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 



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, _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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev




___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2017-07-16 Thread Nicolai Hähnle 

On 07.07.2017 18:19, Juan A. Suarez Romero 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.

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


Thanks for taking care of this.

Reviewed-by: Nicolai Hähnle 



---
  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, _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();
  }
   }
  




--
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2017-07-10 Thread 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" 
>  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, _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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2017-07-10 Thread Timothy Arceri 


On 8 July 2017 12:19:48 am GMT+08:00, "Juan A. Suarez Romero" 
 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. 

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. 

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, _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@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/mesa-dev

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2017-07-07 Thread Juan A. Suarez Romero 
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.

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, 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, _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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev