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

Timothy Arceri timothy.arceri at collabora.com
Thu Jan 21 15:49:30 PST 2016 

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. 


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