[Mesa-dev] [PATCH v2] anv: fix alphaToCoverage when there is no color attachment
Iago Toral
itoral at igalia.com
Fri May 3 07:40:32 UTC 2019
On Thu, 2019-05-02 at 09:03 -0500, Jason Ekstrand wrote:
> 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?
yes, that's fair enough I guess.
> 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.
I was wondering about that. If rt 0 is VK_ATTACHMENT_UNUSED then writes
to that output are discarded. I was not sure if that meant that we
would lose alpha to coverage too, the spec doesn't clarify this aspect,
but I guess that in the end one can consider that alpha to coverage is
orthogonal to whether the memory write happens or not, since that is in
fact the case for depth-only rendering after all. I think we probably
don't have any tests in CTS for this at the moment, so I'll double-
check with Khronos and if they confirm this is the intended behavior
I'll file a test request for this too.
Iago
> --Jason
>
> > break;
> >
> > default:
> >
> > unreachable("Invalid graphics shader stage");
> >
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190503/a8314839/attachment.html>
More information about the mesa-dev
mailing list