<div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, May 3, 2019 at 6:52 AM Iago Toral Quiroga <<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);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 when 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 its alpha component for alpha to coverage.<br>
In that case we will 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>
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 | 48 +++++++++++++++++++++++++--------<br>
1 file changed, 37 insertions(+), 11 deletions(-)<br>
<br>
diff --git a/src/intel/vulkan/anv_pipeline.c b/src/intel/vulkan/anv_pipeline.c<br>
index 20eab548fb2..f379dd2752e 100644<br>
--- a/src/intel/vulkan/anv_pipeline.c<br>
+++ b/src/intel/vulkan/anv_pipeline.c<br>
@@ -818,15 +818,28 @@ anv_pipeline_link_fs(const struct brw_compiler *compiler,<br>
memset(rt_used, 0, sizeof(rt_used));<br>
<br>
/* Flag used render targets */<br>
+ bool needs_null_rt_for_alpha_to_coverage = false;<br>
nir_foreach_variable_safe(var, &stage->nir->outputs) {<br>
if (var->data.location < FRAG_RESULT_DATA0)<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>
+ /* Unused */<br>
+ if (!(stage->key.wm.color_outputs_valid & (1 << rt))) {<br></blockquote><div><br></div><div>While we're here, I realized reading this code today that we're only checking one bit for the color attachment whereas we really should be comparing against BITFIELD_RANGE(rt, array_size) here.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ /* If this is the RT at location 0 and we have alpha to coverage<br>
+ * enabled, we'll have to create a null render target and it must<br>
+ * be at index 0.<br>
+ */<br>
+ if (rt == 0 && stage->key.wm.alpha_to_coverage)<br>
+ needs_null_rt_for_alpha_to_coverage = true;<br></blockquote><div><br></div><div>Why do we need all this needs_null_rt_for_alpha_to_coverage buisiness? Why not just let it fall through and set rt_used to true and then....<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+ continue;<br>
+ }<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>
@@ -835,7 +848,12 @@ anv_pipeline_link_fs(const struct brw_compiler *compiler,<br>
rt_used[rt + i] = true;<br>
}<br>
<br>
- /* Set new, compacted, location */<br>
+ /* Make sure we leave the first RT slot available for alpha to coverage<br>
+ * if we don't have a valid RT 0.<br>
+ */<br>
+ if (needs_null_rt_for_alpha_to_coverage)<br>
+ num_rts = 1;<br>
+<br>
for (unsigned i = 0; i < max_rt; i++) {<br>
if (!rt_used[i])<br>
continue;<br></blockquote><div><br></div><div>Down here just do</div><div><br></div><div>if (stage->key.wm.color_outputs_valid & (1 << i)) {</div><div> /* Set up a color attachment */</div><div>} else {</div><div> /* Set up a null attachment */<br></div><div>}</div><div>num_rts++;</div><div><br></div><div>This would also fix a bug that I think we have today if you have an array in the shader that only has some of it's inputs valid. I think today you'd end up crashing when we go to bind the non-existent color attachments.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
@@ -857,11 +875,15 @@ anv_pipeline_link_fs(const struct brw_compiler *compiler,<br>
const unsigned rt = var->data.location - FRAG_RESULT_DATA0;<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>
+ /* Unused or out-of-bounds, throw it away, unless it is the first<br>
+ * RT and we have alpha to coverage.<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>
+ }<br>
continue;<br>
}<br>
<br>
@@ -873,14 +895,18 @@ anv_pipeline_link_fs(const struct brw_compiler *compiler,<br>
if (deleted_output)<br>
nir_fixup_deref_modes(stage->nir);<br>
<br>
- if (num_rts == 0) {<br>
- /* If we have no render targets, we need a null render target */<br>
+ /* If we have no render targets or we need to create one for alpha to<br>
+ * coverage, we need a null render target.<br>
+ */<br>
+ if (num_rts == 0 || needs_null_rt_for_alpha_to_coverage) {<br>
rt_bindings[0] = (struct anv_pipeline_binding) {<br>
.set = ANV_DESCRIPTOR_SET_COLOR_ATTACHMENTS,<br>
.binding = 0,<br>
.index = UINT32_MAX,<br>
};<br>
- num_rts = 1;<br>
+<br>
+ if (num_rts == 0)<br>
+ num_rts = 1;<br>
}<br>
<br>
/* Now that we've determined the actual number of render targets, adjust<br>
-- <br>
2.17.1<br>
<br>
</blockquote></div></div>