<html dir="ltr"><head></head><body style="text-align:left; direction:ltr;"><div>On Mon, 2019-05-06 at 14:32 -0500, Jason Ekstrand wrote:</div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, May 6, 2019 at 9:01 AM Iago Toral Quiroga <<a href="mailto:itoral@igalia.com">itoral@igalia.com</a>> wrote:<br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">From: Samuel Iglesias Gonsálvez <<a href="mailto:siglesias@igalia.com" target="_blank">siglesias@igalia.com</a>><br>
<br>
There are tests in CTS for alpha to coverage without a color attachment<br>
that are failing. This happens because we remove the shader color<br>
outputs when we don't have a valid color attachment for them, but when<br>
alpha to coverage is enabled we still want to preserve the the output<br>
at location 0 since we need the alpha component. In that case we will<br>
also need to create a null render target for RT 0.<br>
<br>
v2:<br>
- We already create a null rt when we don't have any, so reuse that<br>
for this case (Jason)<br>
- Simplify the code a bit (Iago)<br>
<br>
v3:<br>
- Take alpha to coverage from the key and don't tie this to depth-only<br>
rendering only, we want the same behavior if we have multiple render<br>
targets but the one at location 0 is not used. (Jason).<br>
- Rewrite commit message (Iago)<br>
<br>
v4:<br>
- Make sure we take into account the array length of the shader outputs,<br>
which we were no handling correctly either and make sure we also<br>
create null render targets for any invalid array entries too. (Jason)<br>
<br>
Fixes the following CTS tests:<br>
dEQP-VK.pipeline.multisample.alpha_to_coverage_no_color_attachment.*<br>
<br>
Signed-off-by: Samuel Iglesias Gonsálvez <<a href="mailto:siglesias@igalia.com" target="_blank">siglesias@igalia.com</a>><br>
Signed-off-by: Iago Toral Quiroga <<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>><br>
---<br>
src/intel/vulkan/anv_pipeline.c | 56 ++++++++++++++++++++++++---------<br>
1 file changed, 42 insertions(+), 14 deletions(-)<br>
<br>
diff --git a/src/intel/vulkan/anv_pipeline.c b/src/intel/vulkan/anv_pipeline.c<br>
index 20eab548fb2..f15f0896266 100644<br>
--- a/src/intel/vulkan/anv_pipeline.c<br>
+++ b/src/intel/vulkan/anv_pipeline.c<br>
@@ -823,14 +823,24 @@ anv_pipeline_link_fs(const struct brw_compiler *compiler,<br>
continue;<br>
<br>
const unsigned rt = var->data.location - FRAG_RESULT_DATA0;<br>
- /* Unused or out-of-bounds */<br>
- if (rt >= MAX_RTS || !(stage->key.wm.color_outputs_valid & (1 << rt)))<br>
+ /* Out-of-bounds */<br>
+ if (rt >= MAX_RTS)<br>
continue;<br>
<br>
const unsigned array_len =<br>
glsl_type_is_array(var->type) ? glsl_get_length(var->type) : 1;<br>
assert(rt + array_len <= max_rt);<br>
<br>
+ /* Unused */<br>
+ if (!(stage->key.wm.color_outputs_valid & BITFIELD_RANGE(rt, array_len))) {<br>
+ /* If this is the RT at location 0 and we have alpha to coverage<br>
+ * enabled we will have to create a null RT for it, so mark it as<br>
+ * used.<br>
+ */<br>
+ if (rt > 0 || !stage->key.wm.alpha_to_coverage)<br>
+ continue;<br>
+ }<br>
+<br>
for (unsigned i = 0; i < array_len; i++)<br>
rt_used[rt + i] = true;<br>
}<br>
@@ -841,11 +851,22 @@ anv_pipeline_link_fs(const struct brw_compiler *compiler,<br>
continue;<br>
<br>
rt_to_bindings[i] = num_rts;<br>
- rt_bindings[rt_to_bindings[i]] = (struct anv_pipeline_binding) {<br>
- .set = ANV_DESCRIPTOR_SET_COLOR_ATTACHMENTS,<br>
- .binding = 0,<br>
- .index = i,<br>
- };<br>
+<br>
+ if (stage->key.wm.color_outputs_valid & (1 << i)) {<br>
+ rt_bindings[rt_to_bindings[i]] = (struct anv_pipeline_binding) {<br>
+ .set = ANV_DESCRIPTOR_SET_COLOR_ATTACHMENTS,<br>
+ .binding = 0,<br>
+ .index = i,<br>
+ };<br>
+ } else {<br>
+ /* Setup a null render target */<br>
+ rt_bindings[rt_to_bindings[i]] = (struct anv_pipeline_binding) {<br>
+ .set = ANV_DESCRIPTOR_SET_COLOR_ATTACHMENTS,<br>
+ .binding = 0,<br>
+ .index = UINT32_MAX,<br>
+ };<br>
+ }<br>
+<br>
num_rts++;<br>
}<br>
<br>
@@ -855,14 +876,21 @@ anv_pipeline_link_fs(const struct brw_compiler *compiler,<br>
continue;<br>
<br>
const unsigned rt = var->data.location - FRAG_RESULT_DATA0;<br>
+ const unsigned array_len =<br>
+ glsl_type_is_array(var->type) ? glsl_get_length(var->type) : 1;<br>
+<br>
if (rt >= MAX_RTS ||<br>
- !(stage->key.wm.color_outputs_valid & (1 << rt))) {<br>
- /* Unused or out-of-bounds, throw it away */<br>
- deleted_output = true;<br>
- var->data.mode = nir_var_function_temp;<br>
- exec_node_remove(&var->node);<br>
- exec_list_push_tail(&impl->locals, &var->node);<br>
- continue;<br>
+ !(stage->key.wm.color_outputs_valid & BITFIELD_RANGE(rt, array_len))) {<br>
+ /* Unused or out-of-bounds, throw it away, unless it is the first<br>
+ * RT and we have alpha to coverage enabled.<br>
+ */<br>
+ if (rt != 0 || !stage->key.wm.alpha_to_coverage) {<br>
+ deleted_output = true;<br>
+ var->data.mode = nir_var_function_temp;<br>
+ exec_node_remove(&var->node);<br>
+ exec_list_push_tail(&impl->locals, &var->node);<br>
+ continue;<br>
+ }<br></blockquote><div><br></div><div>I think we can simplify this yet further and just do</div><div><br></div><div>if (rt >= MAX_RTS || !rt_used[rt])</div><div><br></div><div>That way, all the logic stays in one place.</div></div></div></blockquote><div><br></div><div>Right, that's much better.</div><div><br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><br></div><div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>><br></div></div></div></blockquote><div><br></div><div>thanks!</div><div><br></div><div>Iago</div><div><br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
}<br>
<br>
/* Give it the new location */<br>
</blockquote></div></div></blockquote></body></html>