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

[Jason Ekstrand]

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:

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 --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180106/e8d88e1a/attachment-0001.html>