[Mesa-dev] [PATCH v2] anv: fix alphaToCoverage when there is no color attachment

[Jason Ekstrand]

On Thu, May 2, 2019 at 5:46 AM Iago Toral Quiroga <itoral at igalia.com> wrote:

> From: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
>
> 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 <siglesias at igalia.com>
> Signed-off-by: Iago Toral Quiroga <itoral at igalia.com>
> ---
>  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);
>

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.

--Jason


>           break;
>        default:
>           unreachable("Invalid graphics shader stage");
> --
> 2.17.1
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190502/a60130e2/attachment.html>