Re: [Mesa-dev] [PATCH] anv: fix alphaToCoverage when there is no color attachment
On Tue, 2019-04-30 at 17:29 -0500, Jason Ekstrand wrote:
> On Tue, Apr 30, 2019 at 4:36 AM Iago Toral 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>F0x90031000
> > > >
> > > > >
> > > >
> > > > > 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
Re: [Mesa-dev] [PATCH] anv: fix alphaToCoverage when there is no color attachment
On Tue, Apr 30, 2019 at 4:36 AM Iago Toral 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>F0x90031000
> >
> > 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
> > ---
> > 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
Re: [Mesa-dev] [PATCH] anv: fix alphaToCoverage when there is no color attachment
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 <
> 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>F0x90031000
> >
> > >
> >
> > > 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
> >
> > > ---
> >
> > > 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;
> >
> > >
Re: [Mesa-dev] [PATCH] anv: fix alphaToCoverage when there is no color attachment
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?
It is the latter.
> 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.
>
OK, thanks for the suggestion!
Sam
>
> 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>F0x90031000
> > >
> > > 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
> > > ---
> > > 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(>node);
> > > - exec_list_push_tail(>locals, >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(>node);
> > > +exec_list_push_tail(>locals, >node);
Re: [Mesa-dev] [PATCH] anv: fix alphaToCoverage when there is no color attachment
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>F0x90031000
> >
> > 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
> > ---
> > 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(>node);
> > - exec_list_push_tail(>locals, >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(>node);
> > +exec_list_push_tail(>locals, >node);
> > +continue;
> > + }
> >}
> >
> >/* Give it the new location */
> > @@ -1075,7 +1093,8 @@ anv_pipeline_compile_graphics(struct
> > anv_pipeline *pipeline,
> > anv_pipeline_link_gs(compiler, [s], next_stage);
> > break;
> >case MESA_SHADER_FRAGMENT:
> > - anv_pipeline_link_fs(compiler, [s]);
> > +
Re: [Mesa-dev] [PATCH] anv: fix alphaToCoverage when there is no color attachment
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>F0x90031000
>
> 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
> ---
> 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(>node);
> - exec_list_push_tail(>locals, >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(>node);
> +exec_list_push_tail(>locals, >node);
> +continue;
> + }
>}
>
>/* Give it the new location */
> @@ -1075,7 +1093,8 @@ anv_pipeline_compile_graphics(struct
> anv_pipeline *pipeline,
> anv_pipeline_link_gs(compiler, [s], next_stage);
> break;
>case MESA_SHADER_FRAGMENT:
> - anv_pipeline_link_fs(compiler, [s]);
> + anv_pipeline_link_fs(compiler, [s],
> + pipeline->subpass, info-
> >pMultisampleState);
> break;
>default:
> unreachable("Invalid graphics shader stage");
signature.asc
Description: This is a digitally signed message part
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] anv: fix alphaToCoverage when there is no color attachment
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>F0x90031000
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
---
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(>node);
- exec_list_push_tail(>locals, >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(>node);
+exec_list_push_tail(>locals, >node);
+continue;
+ }
}
/* Give it the new location */
@@ -1075,7 +1093,8 @@ anv_pipeline_compile_graphics(struct anv_pipeline
*pipeline,
anv_pipeline_link_gs(compiler, [s], next_stage);
break;
case MESA_SHADER_FRAGMENT:
- anv_pipeline_link_fs(compiler, [s]);
+ anv_pipeline_link_fs(compiler, [s],
+ pipeline->subpass, info->pMultisampleState);
break;
default:
unreachable("Invalid graphics shader stage");
--
2.19.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
