On 22/06/17 02:41, Juan A. Suarez Romero wrote:
On Wed, 2017-06-21 at 20:24 +1000, Timothy Arceri wrote:
On 21/06/17 18:13, Juan A. Suarez Romero wrote:
link_xfb_stride_layout_qualifiers() can be called multiple times, and
each time we call prog->TransformFeedback.BufferStride is reset to 0.

Thus it is loosing the values set in previous call.

Do not perform such reset.

Fixes:
KHR-GL45.enhanced_layouts.xfb_stride_of_empty_list
KHR-GL45.enhanced_layouts.xfb_stride_of_empty_list_and_api

Signed-off-by: Juan A. Suarez Romero <jasua...@igalia.com>
---
   src/compiler/glsl/linker.cpp | 4 ----
   1 file changed, 4 deletions(-)

diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
index adfa3b7b1d..1fe0ccc496 100644
--- a/src/compiler/glsl/linker.cpp
+++ b/src/compiler/glsl/linker.cpp
@@ -1623,10 +1623,6 @@ link_xfb_stride_layout_qualifiers(struct gl_context *ctx,
                                     struct gl_shader **shader_list,
                                     unsigned num_shaders)
   {
-   for (unsigned i = 0; i < MAX_FEEDBACK_BUFFERS; i++) {
-      prog->TransformFeedback.BufferStride[i] = 0;
-   }

I think this probably needs to be moved rather than just deleted. I
think the idea is to reset things to 0 in case we are re-linking an
existing program with different shaders attached.


Checking how this was done prior the refactoring in 4d65f68 (mesa/glsl:
move TransformFeedbackBufferStride to gl_shader), actually the reset
should be done if the new shaders provide an explicit xfb_stride.
Otherwise we should kept the old value.


No, that's not how it worked. The related change in 4d65f68 was:

    for (unsigned i = 0; i < MAX_FEEDBACK_BUFFERS; i++) {
-      linked_shader->info.TransformFeedback.BufferStride[i] = 0;
+      prog->TransformFeedback.BufferStride[i] = 0;
    }

There was no check previously we just reset everything.

The problem is that previously each stage had its own copy of a BufferStride array. There was no need for this as xfb only applies to the last stage in the pipeline before the fragment shader.

4d65f68 changed it so that we share a single BufferStride array across the program instead, this allowed simplifications elsewhere and reduced the amount of data needed for shader cache. I assume the issue is that if we have a fragment shader attached then the array would get incorrectly reset.

Please see my reply to v2 for a suggested fix. Please let me know if that doesn't fix it.


I'm sending a new version.


-
      for (unsigned i = 0; i < num_shaders; i++) {
         struct gl_shader *shader = shader_list[i];


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

Reply via email to