[Mesa-dev] [PATCH 23/28] glsl: add pack varying to resource list for vertex input / fragment output

[Timothy Arceri]

On Wed, 2016-01-20 at 15:38 -0800, Anuj Phogat wrote:
> On Wed, Jan 20, 2016 at 1:39 PM, Timothy Arceri
> <timothy.arceri at collabora.com> wrote:
> > On Wed, 2016-01-20 at 10:06 -0800, Anuj Phogat wrote:
> > > On Mon, Dec 28, 2015 at 9:00 PM, Timothy Arceri
> > > <timothy.arceri at collabora.com> wrote:
> > > > This is needed now that we pack these type of varyings when
> > > > they
> > > > have a
> > > > component layout qualifier.
> > > > ---
> > > >  src/glsl/linker.cpp | 15 ++++++++-------
> > > >  1 file changed, 8 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> > > > index 44dd7f0..52a326a 100644
> > > > --- a/src/glsl/linker.cpp
> > > > +++ b/src/glsl/linker.cpp
> > > > @@ -3763,13 +3763,14 @@ build_program_resource_list(struct
> > > > gl_shader_program *shProg)
> > > >     if (input_stage == MESA_SHADER_STAGES && output_stage == 0)
> > > >        return;
> > > > 
> > > > -   /* Program interface needs to expose varyings in case of
> > > > SSO.
> > > > */
> > > > -   if (shProg->SeparateShader) {
> > > > -      if (!add_packed_varyings(shProg, input_stage,
> > > > GL_PROGRAM_INPUT))
> > > > -         return;
> > > > -      if (!add_packed_varyings(shProg, output_stage,
> > > > GL_PROGRAM_OUTPUT))
> > > > -         return;
> > > > -   }
> > > > +   /* Program interface needs to expose varyings in case of
> > > > SSO,
> > > > or in case of
> > > > +    * vertex inputs/fragement outputs that are packed unsing
> > > > the
> > > > component
> > > > +    * layout qualifier.
> > > > +    */
> > > > +   if (!add_packed_varyings(shProg, input_stage,
> > > > GL_PROGRAM_INPUT))
> > > > +      return;
> > > > +   if (!add_packed_varyings(shProg, output_stage,
> > > > GL_PROGRAM_OUTPUT))
> > > > +      return;
> > > > 
> > > >     if (!add_fragdata_arrays(shProg))
> > > >        return;
> > > > --
> > > > 2.4.3
> > > > 
> > > > _______________________________________________
> > > > mesa-dev mailing list
> > > > mesa-dev at lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > 
> > > I will give you my r-b on this if you can help me understand the
> > > concept of
> > > exposing varyings and how it's utilized during linking.
> > 
> > Sure. The inputs and outputs to a program need to be added to the
> > resource list so that they can be queried by
> > ARB_program_interface_query via PROGRAM_INPUT/PROGRAM_OUTPUT.
> Right. Aren't the input/output variables added by 
>  add_interface_variables() ?
> add_packed_varyings() is used just for SSO with below explanation
> from
> commit : 4e7fd66:
> "Varyings can be considered inputs or outputs of a program only when
>  SSO is in use. With multi-stage programs, inputs contain only inputs
>   for first stage and outputs contains outputs of the final shader
> stage."
> 
> Can you explain why do we need to add packed varyings to the resource
> list even with non-SSO?

That commit message looks incorrect to me. I don't see anything in the
ARB_program_interface_query spec saying that only SSO programs can be
queried. In fact without this change one I believe one of the piglit
tests for that spec breaks since we pack all explicit locations
regardless of having a component qualifier in order to deal with cases
were the default is used (I guess this could be improved with some sort
of table used to only pack when there are two varyings at the same
location but thats another issue).
This change likely fixes some other use cases that don't have tests
currently.

I believe the reason for that commit message is that without the SSO
check the code in that commit broke some non-SSO piglit tests but this
was because it was adding both the inputs and outputs of the first and
last stage which I fixed in 0508d9504aa.


> 
> > 
> > I can add that to the commit message if you think its useful.
> > 
> > 
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev