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

Anuj Phogat anuj.phogat at gmail.com
Fri Jan 22 12:08:51 PST 2016 

On Thu, Jan 21, 2016 at 3:49 PM, Timothy Arceri
<timothy.arceri at collabora.com> wrote:
> On Thu, 2016-01-21 at 15:28 -0800, Anuj Phogat wrote:
>> On Thu, Jan 21, 2016 at 1:47 PM, Timothy Arceri
>> <timothy.arceri at collabora.com> wrote:
>> > On Thu, 2016-01-21 at 09:23 -0800, Anuj Phogat wrote:
>> > > 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.
>> >
>> > I don't see any such restriction in the spec:
>> >
>> > "* PROGRAM_INPUT corresponds to the set of active input variables
>> >    used by the first shader stage of <program>.  If <program>
>> > includes
>> >    multiple shader stages, input variables from any shader stage
>> >    other than the first will not be enumerated.
>> >
>> >  * PROGRAM_OUTPUT corresponds to the set of active output variables
>> >    (section 2.14.11) used by the last shader stage of <program>.
>> >     If <program> includes multiple shader stages, output variables
>> >    from any shader stage other than the last will not be
>> > enumerated."
>> >
>> I can see the vertex inputs and fragment outputs added to the
>> resource
>> list to satisfy above quoted queries. I still don't see why varyings
>> should
>> be added for above queries.
>
> Well it says the first and last shader stage not the vertex and
> fragment stages. What if there is no fragment shader? My understanding
> is that in that case we would add the varyings of the last stage.
>
I was making a false assumption that fragment shader must be present
in the program for non-sso. Thanks for explaining.

Patch is:
Reviewed-by: Anuj Phogat <anuj.phogat at gmail.com>
>
>>
>> > ...
>> >
>> >  For the UNIFORM, PROGRAM_INPUT, PROGRAM_OUTPUT, and
>> >  TRANSFORM_FEEDBACK_VARYING interfaces, the active resource list
>> > will
>> >  include all active variables for the interface, including any
>> > active
>> >  built-in variables."
>> >
>> My understanding isn't changed by above quote from the spec.  Maybe
>> Ian
>> will have some comments. Adding him to CC.
>>
>> > >
>> > >
>> > > > 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
>> > > _______________________________________________
>> > > 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

More information about the mesa-dev mailing list