On Saturday, January 6, 2018 9:07:44 PM PST Jason Ekstrand wrote: > 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:
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
>
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
