[Mesa-dev] [PATCH 16/28] glsl: don't pack tessellation stages like we do other stages
Timothy Arceri
timothy.arceri at collabora.com
Wed Jan 6 16:28:30 PST 2016
On Wed, 2016-01-06 at 18:45 -0500, Ilia Mirkin wrote:
> On Wed, Jan 6, 2016 at 6:40 PM, Timothy Arceri
> <timothy.arceri at collabora.com> wrote:
> > On Wed, 2016-01-06 at 17:50 -0500, Ilia Mirkin wrote:
> > > On Tue, Dec 29, 2015 at 12:00 AM, Timothy Arceri
> > > <timothy.arceri at collabora.com> wrote:
> > > > Tessellation shaders treat varyings as shared memory and
> > > > invocations
> > > > can access each others varyings therefore we can't use the
> > > > existing
> > > > method to lower them.
> > >
> > > That's not strictly true... this is only true of tess control
> > > outputs
> > > (which can be written by the current invocation, but also read in
> > > by
> > > other invocations, effectively acting as a shared memory -- both
> > > true
> > > of per-invocation outputs as well as per-patch outputs). Does
> > > that
> > > information change this patch at all?
> >
> > I don't think so. The problem is that the current packing code
> > works
> > like this:
> >
> > - Change vars to be packed to temporaries, create new packed
> > varyings.
> > - Copy *all* values from the new packed input varying to the
> > temporaries at the start of main.
> > - Copy *all* values from the temporaries to the new packed output
> > vars
> > at the end of main (or before emit for GS).
> >
> > As well as the invocations stomping on each other this results in
> > 32
> > (GL_MAX_PATCH_VERTICES?) copies for each TCS input as it just
> > copies
> > the full array.
>
> Presumably it also does this for GS? Although it's a lot more common
> for a single GS invocation to consume
Right. I thought about changing GS to do it different also but until
the backend can clean this up better it would likely make things even
worse.
>
> >
> > The current packing just doesn't work well for tessellation, its
> > easier
> > to just disbale it for tessellation and do it all using a different
> > method rather than trying to mix and match.
>
> I thought it already *was* disabled... but I think you still have to
> have packing on TES outputs, because (a) your arguments against don't
> apply and (b) it might feed into transform feedback, which i have
> faint recollections must go through packing.
Yeah its a bit of a mess. Gallium tries to always disable packing
unless transform feedback is enabled. Are there any Gallium drivers
where its not enabled??
Then there is code that disables it for tessellation (except TES
outputs), as far as I understand it yes varyings for transform feedback
must go through packing. In which case I do need to allow these to be
lowered for TES outputs thanks for point it out, will change this.
>
> >
> >
> > >
> > > >
> > > > This adds a check for these stages as following patches will
> > > > allow explicit locations to be lowered even when the driver and
> > > > existing
> > > > tesselation checks ask for it to be disabled, we do this to
> > > > enable
> > > > support
> > > > for the component layout qualifier.
> > > > ---
> > > > src/glsl/lower_packed_varyings.cpp | 62 +++++++++++++++++++++-
> > > > ----
> > > > ------------
> > > > 1 file changed, 34 insertions(+), 28 deletions(-)
> > > >
> > > > diff --git a/src/glsl/lower_packed_varyings.cpp
> > > > b/src/glsl/lower_packed_varyings.cpp
> > > > index 2899846..e4e9a35 100644
> > > > --- a/src/glsl/lower_packed_varyings.cpp
> > > > +++ b/src/glsl/lower_packed_varyings.cpp
> > > > @@ -737,40 +737,46 @@ lower_packed_varyings(void *mem_ctx,
> > > > unsigned
> > > > locations_used,
> > > > ir_variable_mode mode, unsigned
> > > > gs_input_vertices,
> > > > gl_shader *shader, bool
> > > > disable_varying_packing)
> > > > {
> > > > - exec_list *instructions = shader->ir;
> > > > ir_function *main_func = shader->symbols
> > > > ->get_function("main");
> > > > exec_list void_parameters;
> > > > ir_function_signature *main_func_sig
> > > > = main_func->matching_signature(NULL, &void_parameters,
> > > > false);
> > > > - exec_list new_instructions, new_variables;
> > > > - lower_packed_varyings_visitor visitor(mem_ctx,
> > > > locations_used,
> > > > mode,
> > > > - gs_input_vertices,
> > > > - &new_instructions,
> > > > - &new_variables,
> > > > -
> > > > disable_varying_packing);
> > > > - visitor.run(shader);
> > > > - if (mode == ir_var_shader_out) {
> > > > - if (shader->Stage == MESA_SHADER_GEOMETRY) {
> > > > - /* For geometry shaders, outputs need to be lowered
> > > > before each call
> > > > - * to EmitVertex()
> > > > - */
> > > > - lower_packed_varyings_gs_splicer splicer(mem_ctx,
> > > > &new_instructions);
> > > > -
> > > > - /* Add all the variables in first. */
> > > > - main_func_sig->body.head
> > > > ->insert_before(&new_variables);
> > > >
> > > > - /* Now update all the EmitVertex instances */
> > > > - splicer.run(instructions);
> > > > + if (!(shader->Stage == MESA_SHADER_TESS_CTRL ||
> > > > + shader->Stage == MESA_SHADER_TESS_EVAL)) {
> > > > + exec_list *instructions = shader->ir;
> > > > + exec_list new_instructions, new_variables;
> > > > +
> > > > + lower_packed_varyings_visitor visitor(mem_ctx,
> > > > locations_used, mode,
> > > > + gs_input_vertices,
> > > > + &new_instructions,
> > > > + &new_variables,
> > > > +
> > > > disable_varying_packing);
> > > > + visitor.run(shader);
> > > > + if (mode == ir_var_shader_out) {
> > > > + if (shader->Stage == MESA_SHADER_GEOMETRY) {
> > > > + /* For geometry shaders, outputs need to be
> > > > lowered
> > > > before each
> > > > + * call to EmitVertex()
> > > > + */
> > > > + lower_packed_varyings_gs_splicer splicer(mem_ctx,
> > > > +
> > > > &new_instructions);
> > > > +
> > > > + /* Add all the variables in first. */
> > > > + main_func_sig->body.head
> > > > ->insert_before(&new_variables);
> > > > +
> > > > + /* Now update all the EmitVertex instances */
> > > > + splicer.run(instructions);
> > > > + } else {
> > > > + /* For other shader types, outputs need to be
> > > > lowered
> > > > at the end
> > > > + * of main()
> > > > + */
> > > > + main_func_sig->body.append_list(&new_variables);
> > > > + main_func_sig
> > > > ->body.append_list(&new_instructions);
> > > > + }
> > > > } else {
> > > > - /* For other shader types, outputs need to be lowered
> > > > at
> > > > the end of
> > > > - * main()
> > > > - */
> > > > - main_func_sig->body.append_list(&new_variables);
> > > > - main_func_sig->body.append_list(&new_instructions);
> > > > + /* Shader inputs need to be lowered at the beginning
> > > > of
> > > > main() */
> > > > + main_func_sig->body.head
> > > > ->insert_before(&new_instructions);
> > > > + main_func_sig->body.head
> > > > ->insert_before(&new_variables);
> > > > }
> > > > - } else {
> > > > - /* Shader inputs need to be lowered at the beginning of
> > > > main() */
> > > > - main_func_sig->body.head
> > > > ->insert_before(&new_instructions);
> > > > - main_func_sig->body.head->insert_before(&new_variables);
> > > > }
> > > > }
> > > > --
> > > > 2.4.3
> > > >
> > > > _______________________________________________
> > > > 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