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

[Jason Ekstrand]

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.


On Mon, Mar 4, 2019 at 4:56 AM Samuel Iglesias Gonsálvez <
siglesias at igalia.com> wrote:

> Still unreviewed.
>
> Sam
>
> On Thu, 2019-02-21 at 12:08 +0100, Samuel Iglesias Gonsálvez wrote:
> > CL#3532 added a test 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 commit fixes that by
> > keeping the output and creating a null render target for it.
> >
> > Fixes tests:
> >
> > dEQP-VK.pipeline.multisample.alpha_to_coverage_no_color_attachment.*
> >
> > Signed-off-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
> > ---
> >  src/intel/vulkan/anv_pipeline.c | 35 +++++++++++++++++++++++++----
> > ----
> >  1 file changed, 27 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/intel/vulkan/anv_pipeline.c
> > b/src/intel/vulkan/anv_pipeline.c
> > index e2024212bd9..70a958bf3a8 100644
> > --- a/src/intel/vulkan/anv_pipeline.c
> > +++ b/src/intel/vulkan/anv_pipeline.c
> > @@ -792,7 +792,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,
> > +                     const struct anv_subpass *subpass,
> > +                     const VkPipelineMultisampleStateCreateInfo
> > *ms_info)
> >  {
> >     unsigned num_rts = 0;
> >     const int max_rt = FRAG_RESULT_DATA7 - FRAG_RESULT_DATA0 + 1;
> > @@ -843,12 +845,28 @@ 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);
> > -         continue;
> > +         if (rt == 0 && ms_info && ms_info->alphaToCoverageEnable &&
> > +             subpass->depth_stencil_attachment && rt_to_bindings[rt]
> > == -1) {
> > +            /* Don't throw away the unused output, because we needed
> > it for
> > +             * calculate correctly the alpha to coverage samples
> > when there
> > +             * is no color buffer attached at location 0.
> > +             */
> > +            rt_to_bindings[rt] = num_rts;
> > +            /* We need a null render target */
> > +            rt_bindings[rt_to_bindings[rt]] = (struct
> > anv_pipeline_binding) {
> > +               .set = ANV_DESCRIPTOR_SET_COLOR_ATTACHMENTS,
> > +               .binding = 0,
> > +               .index = UINT32_MAX,
> > +            };
> > +            num_rts++;
> > +         } else {
> > +            /* 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);
> > +            continue;
> > +         }
> >        }
> >
> >        /* Give it the new location */
> > @@ -1075,7 +1093,8 @@ 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, info-
> > >pMultisampleState);
> >           break;
> >        default:
> >           unreachable("Invalid graphics shader stage");
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190322/8cdd831a/attachment.html>