On Tue, 2019-04-30 at 17:29 -0500, Jason Ekstrand wrote:
> 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.
Makes sense to me, I'll send the patch as soon as it comes back from
Jenkins.
> 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. :)
I think that might not be enough because when we setup the null render
target for this case we also set the bit for RT 0 in
color_outputs_valid, so we end up with the same color_outputs_valid as
in the case where we actually have a valid RT and we want the RGB
writes.
> --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
> > > listmesa-...@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
