<div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, May 2, 2019 at 5:46 AM Iago Toral Quiroga <<a href="mailto:itoral@igalia.com">itoral@igalia.com</a>> wrote: </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>> There tests in CTS for for alpha to coverage without a color attachment. First the test draws a primitive with alpha 0 and a subpass with only a depth buffer. No writes to a depth buffer are expected. Then a second draw with a color buffer and the same depth buffer is done to verify the depth buffer still has the original clear values. This behavior is not explicitly forbidden by the Vulkan spec, so it seems it is allowed. When there is no color attachment for a given output, we discard it so at the end we have an FS assembly like: Native code for unnamed fragment shader (null) SIMD16 shader: 1 instructions. 0 loops. 4 cycles. 0:0 spills:fills. Promoted 0 constants. Compacted 16 to 16 bytes (0%) START B0 (4 cycles) sendc(16) null<1>UW g120<0,1,0>F 0x90031000 render MsgDesc: RT write SIMD16 LastRT Surface = 0 mlen 8 rlen 0 { align1 1H EOT }; As g120 is not initialized, we see random writes to the depth buffer due to the alphaToCoverage enablement. This patch fixes that by keeping the output in that case. v2: - No need to create a null render target, the driver is already doing that (Jason) - Simplified code a bit (Iago) Fixes the following CTS tests: dEQP-VK.pipeline.multisample.alpha_to_coverage_no_color_attachment.* Signed-off-by: Samuel Iglesias Gonsálvez <<a href="mailto:siglesias@igalia.com" target="_blank">siglesias@igalia.com</a>> Signed-off-by: Iago Toral Quiroga <<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>> --- src/intel/vulkan/anv_pipeline.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/src/intel/vulkan/anv_pipeline.c b/src/intel/vulkan/anv_pipeline.c index b9c9bfd7598..07f1a939e43 100644 --- a/src/intel/vulkan/anv_pipeline.c +++ b/src/intel/vulkan/anv_pipeline.c @@ -808,7 +808,9 @@ anv_pipeline_compile_gs(const struct brw_compiler *compiler, static void anv_pipeline_link_fs(const struct brw_compiler *compiler, - struct anv_pipeline_stage *stage) + struct anv_pipeline_stage *stage, + bool has_depth_stencil_att, + bool has_alpha_to_coverage) { unsigned num_rts = 0; const int max_rt = FRAG_RESULT_DATA7 - FRAG_RESULT_DATA0 + 1; @@ -859,11 +861,17 @@ anv_pipeline_link_fs(const struct brw_compiler *compiler, const unsigned rt = var->data.location - FRAG_RESULT_DATA0; if (rt >= MAX_RTS || !(stage->key.wm.color_outputs_valid & (1 << rt))) { - /* Unused or out-of-bounds, throw it away */ - deleted_output = true; - var->data.mode = nir_var_function_temp; - exec_node_remove(&var->node); - exec_list_push_tail(&impl->locals, &var->node); + /* Unused or out-of-bounds, throw it away. The exception is depth-only + * rendering with alphaToCoverage, as in this case we need to keep the + * fragment output in location 0, which we will bind later to a null + * render target. + */ + if (rt != 0 || !has_alpha_to_coverage || !has_depth_stencil_att) { + deleted_output = true; + var->data.mode = nir_var_function_temp; + exec_node_remove(&var->node); + exec_list_push_tail(&impl->locals, &var->node); + } continue; } @@ -1120,7 +1128,10 @@ anv_pipeline_compile_graphics(struct anv_pipeline *pipeline, anv_pipeline_link_gs(compiler, &stages[s], next_stage); break; case MESA_SHADER_FRAGMENT: - anv_pipeline_link_fs(compiler, &stages[s]); + anv_pipeline_link_fs(compiler, &stages[s], + pipeline->subpass->depth_stencil_attachment, + info->pMultisampleState && + info->pMultisampleState->alphaToCoverageEnable); </blockquote><div> </div><div>You don't need to pass alphaToCoverageEnable through because it's already in the key. For pipeline->subpass->depth_stencil_attachment, I'm inclined to just not have that in the calculation. It's not in the key, so we'd have to add it, and the calculation should be correct regardless of whether we take depth stencil attachments into account. If they have no attachments whatsoever, then why are they setting alphaToCoverage? Also, I think you could similar issues if someone had no attacyment to rt 0 but did have an attachment at rt 1 which they wrote with a color value.</div><div> </div><div>--Jason </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"> break; default: unreachable("Invalid graphics shader stage"); -- 2.17.1 </blockquote></div></div>