[Mesa-dev] [PATCH] anv: fix alphaToCoverage when there is no color attachment
Iago Toral
itoral at igalia.com
Tue Apr 30 09:36:06 UTC 2019
Hi Jason,
it seems that this was never addressed so I'll try to take it from
here. A couple of comments regarding your feedback:
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.
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?
Iago
On Fri, 2019-03-22 at 10:06 -0500, Jason Ekstrand wrote:
> 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
>
> _______________________________________________mesa-dev mailing
> listmesa-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/20190430/d35633f5/attachment-0001.html>
More information about the mesa-dev
mailing list