<div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Apr 30, 2019 at 4:36 AM Iago Toral <<a href="mailto:itoral@igalia.com">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"><div style="text-align:left;direction:ltr"><div>Hi Jason,</div><div><br></div><div>it seems that this was never addressed so I'll try to take it from here. A couple of comments regarding your feedback:</div><div><br></div><div>We always setup a null render target if we don't have any, so I think that part of the patch is not necessary. I checked that simply making sure that we don't remove the output when we are in this situation is enough to make the tests pass.</div><div><br></div><div>However, you make another suggestion below about only writing the Alpha channel in that case. I understand that you meant this as an optimization for this particular scenario since it is not really a functional requirement for this to work, since we are using a null render target in this case. Anyway, I have been looking into it and I believe that implementing this would be slightly trickier since we'd also need to make sure that we compile a different version of the shader when it is used with a valid color attachment (since in that case we do need to emit the RGB writes). Therefore, it would require that we include a bit for this sceneario in brw_wm_prog_key. We could do that very easily at the same moment we make the decision that we need a null render target in anv_pipeline_link_fs() (where we are already editing nr_color_regions and color_outputs_valid in the key). If we do it like this, then we can implement the optimization trivially when we handle implement nir_intrinsic_store_output in brw_fs_nir.cpp by simply skipping the MOVs for the RGB components when this key flag is set. Does this sound good to you?</div></div></blockquote><div><br></div><div>Yeah, it may not be worth it. It was just a thought. Let's focus on plugging the current hole. The optimization can come later.</div><div><br></div><div>I'm not sure that we would actually need a bit. This would be an implicit combination of the color_outputs_valid and dual-src blending which is already part of the shader. That might need a big fat comment though. :)</div><div><br></div><div>--Jason<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"><div style="text-align:left;direction:ltr"><div><br></div><div>Iago</div><div><br></div><div>On Fri, 2019-03-22 at 10:06 -0500, Jason Ekstrand wrote:</div><blockquote type="cite" style="margin:0px 0px 0px 0.8ex;border-left:2px solid rgb(114,159,207);padding-left:1ex"><div dir="ltr"><div dir="ltr">I'm confused. Don't we always have a NULL render target at location 0? Is the problem really that we need the NULL render target or is it that we can't throw away the alpha component of the RT write in the shader? If it's that we can't throw away the alpha component of the RT write, then I'd suggest a slightly different workaround which does just that. We can rewrite the store_output intrinsic (or store_var; not sure which it is at that point) so that it writes vec4(undef, undef, undef, alpha) to help with linking but then keep the one output var so it we still get the write in the shader.<br><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Mar 4, 2019 at 4:56 AM Samuel Iglesias Gonsálvez <<a href="mailto:siglesias@igalia.com" target="_blank">siglesias@igalia.com</a>> wrote:<br></div><blockquote type="cite" style="margin:0px 0px 0px 0.8ex;border-left:2px solid rgb(114,159,207);padding-left:1ex">Still unreviewed.<br>
<br>
Sam<br>
<br>
On Thu, 2019-02-21 at 12:08 +0100, Samuel Iglesias Gonsálvez wrote:<br>
> CL#3532 added a test for alpha to coverage without a color<br>
> attachment.<br>
> First the test draws a primitive with alpha 0 and a subpass with only<br>
> a depth buffer. No writes to a depth buffer are expected. Then a<br>
> second draw with a color buffer and the same depth buffer is done to<br>
> verify the depth buffer still has the original clear values.<br>
> <br>
> This behavior is not explicitly forbidden by the Vulkan spec, so<br>
> it seems it is allowed.<br>
> <br>
> When there is no color attachment for a given output, we discard it<br>
> so at the end we have an FS assembly like:<br>
> <br>
> Native code for unnamed fragment shader (null)<br>
> SIMD16 shader: 1 instructions. 0 loops. 4 cycles. 0:0 spills:fills.<br>
> Promoted 0 constants. Compacted 16 to 16 bytes (0%)<br>
> START B0 (4 cycles)<br>
> sendc(16) null<1>UW g120<0,1,0>F 0x90031000<br>
> <br>
> render MsgDesc: RT write SIMD16 LastRT Surface = 0 mlen 8 rlen 0 {<br>
> align1 1H EOT };<br>
> <br>
> As g120 is not initialized, we see random writes to the depth buffer<br>
> due to the alphaToCoverage enablement. This commit fixes that by<br>
> keeping the output and creating a null render target for it.<br>
> <br>
> Fixes tests:<br>
> <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>
> ---<br>
> src/intel/vulkan/anv_pipeline.c | 35 +++++++++++++++++++++++++----<br>
> ----<br>
> 1 file changed, 27 insertions(+), 8 deletions(-)<br>
> <br>
> diff --git a/src/intel/vulkan/anv_pipeline.c<br>
> b/src/intel/vulkan/anv_pipeline.c<br>
> index e2024212bd9..70a958bf3a8 100644<br>
> --- a/src/intel/vulkan/anv_pipeline.c<br>
> +++ b/src/intel/vulkan/anv_pipeline.c<br>
> @@ -792,7 +792,9 @@ anv_pipeline_compile_gs(const struct brw_compiler<br>
> *compiler,<br>
> <br>
> static void<br>
> anv_pipeline_link_fs(const struct brw_compiler *compiler,<br>
> - struct anv_pipeline_stage *stage)<br>
> + struct anv_pipeline_stage *stage,<br>
> + const struct anv_subpass *subpass,<br>
> + const VkPipelineMultisampleStateCreateInfo<br>
> *ms_info)<br>
> {<br>
> unsigned num_rts = 0;<br>
> const int max_rt = FRAG_RESULT_DATA7 - FRAG_RESULT_DATA0 + 1;<br>
> @@ -843,12 +845,28 @@ anv_pipeline_link_fs(const struct brw_compiler<br>
> *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>
> - continue;<br>
> + if (rt == 0 && ms_info && ms_info->alphaToCoverageEnable &&<br>
> + subpass->depth_stencil_attachment && rt_to_bindings[rt]<br>
> == -1) {<br>
> + /* Don't throw away the unused output, because we needed<br>
> it for<br>
> + * calculate correctly the alpha to coverage samples<br>
> when there<br>
> + * is no color buffer attached at location 0.<br>
> + */<br>
> + rt_to_bindings[rt] = num_rts;<br>
> + /* We need a null render target */<br>
> + rt_bindings[rt_to_bindings[rt]] = (struct<br>
> anv_pipeline_binding) {<br>
> + .set = ANV_DESCRIPTOR_SET_COLOR_ATTACHMENTS,<br>
> + .binding = 0,<br>
> + .index = UINT32_MAX,<br>
> + };<br>
> + num_rts++;<br>
> + } else {<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>
> + }<br>
> }<br>
> <br>
> /* Give it the new location */<br>
> @@ -1075,7 +1093,8 @@ anv_pipeline_compile_graphics(struct<br>
> anv_pipeline *pipeline,<br>
> anv_pipeline_link_gs(compiler, &stages[s], next_stage);<br>
> break;<br>
> case MESA_SHADER_FRAGMENT:<br>
> - anv_pipeline_link_fs(compiler, &stages[s]);<br>
> + anv_pipeline_link_fs(compiler, &stages[s],<br>
> + pipeline->subpass, info-<br>
> >pMultisampleState);<br>
> break;<br>
> default:<br>
> unreachable("Invalid graphics shader stage");<br>
_______________________________________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a></blockquote></div>
<pre>_______________________________________________</pre><pre>mesa-dev mailing list</pre><pre><a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a></pre><pre><a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a></pre></blockquote></div>
</blockquote></div></div>