[Mesa-dev] [PATCH] glsl: do not reset prog->TransformFeedback.BufferStride

[Juan A. Suarez Romero]

On Thu, 2017-06-22 at 09:27 +1000, Timothy Arceri wrote:
> 
> 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 <jasuarez at 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;
>      }
> 

What I meant is that prog->TransformFeedback.BufferStride was not
reset, but liked_shader->info.TransformFeedback.BufferStride.

And prog->TransformFeedback.BufferStride changed if the shader had an
explicit stride.


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

Thanks for the explanation. I think I better understand the situation.

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

Sure.

	J.A.

> > I'm sending a new version.
> > 
> > 
> > > > -
> > > >       for (unsigned i = 0; i < num_shaders; i++) {
> > > >          struct gl_shader *shader = shader_list[i];
> > > >    
> > > > 
> > > 
> > > 
> 
>