[Mesa-dev] [PATCH 2/2] i965/tcs: support gl_PatchVerticesIn as a system value from SPIR-V

[Kenneth Graunke]

On Saturday, January 6, 2018 9:07:44 PM PST Jason Ekstrand wrote:
> On Sat, Jan 6, 2018 at 5:12 PM, Kenneth Graunke <kenneth at whitecape.org>
> wrote:
> 
> > On Wednesday, November 15, 2017 11:53:08 PM PST Iago Toral Quiroga wrote:
> > > We currently handle this by lowering it to a uniform for gen8+ but
> > > the SPIR-V path generates this as a system value, so handle that
> > > case as well.
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_tcs.c | 9 ++++++++-
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/brw_tcs.c
> > b/src/mesa/drivers/dri/i965/brw_tcs.c
> > > index 4424efea4f0..b07b11f485d 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_tcs.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_tcs.c
> > > @@ -296,7 +296,14 @@ brw_tcs_populate_key(struct brw_context *brw,
> > >        per_patch_slots |= prog->info.patch_outputs_written;
> > >     }
> > >
> > > -   if (devinfo->gen < 8 || !tcp)
> > > +   /* For GLSL, gen8+ lowers gl_PatchVerticesIn to a uniform, however
> > > +    * the SPIR-V path always lowers it to a system value.
> > > +    */
> > > +   bool reads_patch_vertices_as_system_value =
> > > +      tcp && (tcp->program.nir->info.system_values_read &
> > > +              BITFIELD64_BIT(SYSTEM_VALUE_VERTICES_IN));
> > > +
> > > +   if (devinfo->gen < 8 || !tcp || reads_patch_vertices_as_
> > system_value)
> > >        key->input_vertices = brw->ctx.TessCtrlProgram.patch_vertices;
> > >     key->outputs_written = per_vertex_slots;
> > >     key->patch_outputs_written = per_patch_slots;
> > >
> >
> > I guess this is okay, and it's better than nothing.  I'd really rather
> > see it converted to a uniform, like it is in the normal GLSL paths.  If
> > you're going to add recompiles based on the key like this, it might be
> > nice to at least update the brw_tcs_precompile function to guess, so we
> > at least attempt to avoid a recompile.
> >
> 
> Ugh... I'm happy to give a stronger "I don't like this".  In Vulkan, this
> is part of the pipeline state so we just pass it in through the shader
> key.  With GL, ugh... Personally, I think I'd be ok with just making it
> state based all the time but we already have the infrastructure to pass it
> through as a uniform so we may as well.  I think the better thing to do
> would be to add a quick little pass that moves VERTICES_IN to a uniform and
> call that on gen8+ brw_link.cpp.  Then we can delete
> LowerTESPatchVerticesIn as i965 is the only user.  The "pass" would be
> really easy:

LowerTCSPatchVerticesIn rather.  I like this plan.

> 
> void
> brw_nir_lower_tcs_vertices_in_to_uniform(nir_shader *nir, const struct
> gl_program *prog, brw_tcs_prog_data *prog_data)
> {
>    int uniform = -1;
>    nir_foreach_var_safe(var, &nir->system_values) {
>       if (var->data.location != SYSTEM_VALUE_VERTICES_IN)
>          continue;
> 
>       if (uniform < -1) {
>          gl_state_index tokens[5] = {
>             STATE_INTERNAL,
>             STATE_TESS_PATCH_VERTICES_IN,
>          };
>          int index = _mesa_add_state_reference(prog->Parameters, tokens);
> 
>          uniform = prog_data->nr_params;
>          uint32_t *param =
> brw_stage_prog_data_add_params(&prog_data->base->base, 1);
>          *param = BRW_PARAM_PARAMETER(index, SWIZZLE_XXXX);
>       }
> 
>       var->mode = nir_var_uniform;
>       var->data.location = uniform;
>       exec_node_remove(&var->node);
>       exec_list_push_tail(&nir->uniforms, &var->node);
>    }
> }
> 
> I may not have gotten my state referencing quite right there, but I think
> it's close.  I'd probably put the pas in brw_nir_uniforms.cpp if I was
> writing it.
> 
> --Jason
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180106/44c531aa/attachment.sig>