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

[Anuj Phogat]

On Wed, Jan 20, 2016 at 4:28 PM, Timothy Arceri
<timothy.arceri at collabora.com> wrote:
> 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.
Commit message just says varyings are also considered program inputs/
outputs in case of SSO. So, add them to to the resource list in addition to
the vertex inputs and fragment outputs.

I meant to ask why do we need to always add "varyings" to the resource list
to support querying vertex inputs and fragment outputs via PROGRAM_INPUT/
PROGRAM_OUTPUT? "varyings" are not supposed to be the part of these
queries. Correct me if I've misunderstood it.


> 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