<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sat, Jan 6, 2018 at 5:12 PM, Kenneth Graunke <span dir="ltr"><<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5">On Wednesday, November 15, 2017 11:53:08 PM PST Iago Toral Quiroga wrote:<br>
> We currently handle this by lowering it to a uniform for gen8+ but<br>
> the SPIR-V path generates this as a system value, so handle that<br>
> case as well.<br>
> ---<br>
> src/mesa/drivers/dri/i965/brw_<wbr>tcs.c | 9 ++++++++-<br>
> 1 file changed, 8 insertions(+), 1 deletion(-)<br>
><br>
> diff --git a/src/mesa/drivers/dri/i965/<wbr>brw_tcs.c b/src/mesa/drivers/dri/i965/<wbr>brw_tcs.c<br>
> index 4424efea4f0..b07b11f485d 100644<br>
> --- a/src/mesa/drivers/dri/i965/<wbr>brw_tcs.c<br>
> +++ b/src/mesa/drivers/dri/i965/<wbr>brw_tcs.c<br>
> @@ -296,7 +296,14 @@ brw_tcs_populate_key(struct brw_context *brw,<br>
> per_patch_slots |= prog->info.patch_outputs_<wbr>written;<br>
> }<br>
><br>
> - if (devinfo->gen < 8 || !tcp)<br>
> + /* For GLSL, gen8+ lowers gl_PatchVerticesIn to a uniform, however<br>
> + * the SPIR-V path always lowers it to a system value.<br>
> + */<br>
> + bool reads_patch_vertices_as_<wbr>system_value =<br>
> + tcp && (tcp->program.nir->info.<wbr>system_values_read &<br>
> + BITFIELD64_BIT(SYSTEM_VALUE_<wbr>VERTICES_IN));<br>
> +<br>
> + if (devinfo->gen < 8 || !tcp || reads_patch_vertices_as_<wbr>system_value)<br>
> key->input_vertices = brw->ctx.TessCtrlProgram.<wbr>patch_vertices;<br>
> key->outputs_written = per_vertex_slots;<br>
> key->patch_outputs_written = per_patch_slots;<br>
><br>
<br>
</div></div>I guess this is okay, and it's better than nothing. I'd really rather<br>
see it converted to a uniform, like it is in the normal GLSL paths. If<br>
you're going to add recompiles based on the key like this, it might be<br>
nice to at least update the brw_tcs_precompile function to guess, so we<br>
at least attempt to avoid a recompile.<br></blockquote></div></div><div class="gmail_extra"><br></div><div class="gmail_extra">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:</div><div class="gmail_extra"><br></div><div class="gmail_extra">void</div><div class="gmail_extra">brw_nir_lower_tcs_vertices_in_to_uniform(nir_shader *nir, const struct gl_program *prog, brw_tcs_prog_data *prog_data)</div><div class="gmail_extra">{</div><div class="gmail_extra"> int uniform = -1;</div><div class="gmail_extra"> nir_foreach_var_safe(var, &nir->system_values) {</div><div class="gmail_extra"> if (var->data.location != SYSTEM_VALUE_VERTICES_IN)</div><div class="gmail_extra"> continue;</div><br><div class="gmail_extra"> if (uniform < -1) {</div><div class="gmail_extra"><div class="gmail_extra"> gl_state_index tokens[5] = {</div><div class="gmail_extra"> STATE_INTERNAL,</div><div class="gmail_extra"> STATE_TESS_PATCH_VERTICES_IN,<br></div><div class="gmail_extra"> };</div> int index = _mesa_add_state_reference(prog->Parameters, tokens);</div><div class="gmail_extra"><br></div><div class="gmail_extra"> uniform = prog_data->nr_params;</div><div class="gmail_extra"> uint32_t *param = brw_stage_prog_data_add_params(&prog_data->base->base, 1);</div><div class="gmail_extra"> *param = BRW_PARAM_PARAMETER(index, SWIZZLE_XXXX);<br></div><div class="gmail_extra"> }</div><div class="gmail_extra"><br></div><div class="gmail_extra"> var->mode = nir_var_uniform;</div><div class="gmail_extra"> var->data.location = uniform;</div><div class="gmail_extra"> exec_node_remove(&var->node);</div><div class="gmail_extra"> exec_list_push_tail(&nir->uniforms, &var->node);<br></div><div class="gmail_extra"> }<br></div><div class="gmail_extra">}</div><div class="gmail_extra"><br></div><div class="gmail_extra">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.</div><div class="gmail_extra"><br></div><div class="gmail_extra">--Jason<br></div></div>