On Tue, Apr 30, 2019 at 4:36 AM Iago Toral <ito...@igalia.com> wrote:
> 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?
>
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.
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. :)
--Jason
>
> 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 <
> sigles...@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 <sigles...@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
> _______________________________________________
>
> mesa-dev mailing list
>
> mesa-dev@lists.freedesktop.org
>
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
