On Sat, Jan 6, 2018 at 5:12 PM, Kenneth Graunke <kenn...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
