[Mesa-dev] [PATCH v4] anv: fix alphaToCoverage when there is no color attachment

[Iago Toral]

On Mon, 2019-05-06 at 14:32 -0500, Jason Ekstrand wrote:
> On Mon, May 6, 2019 at 9:01 AM Iago Toral Quiroga <itoral at igalia.com>
> wrote:
> > From: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
> > 
> > 
> > 
> > There are tests in CTS for alpha to coverage without a color
> > attachment
> > 
> > that are failing. This happens because we remove the shader color
> > 
> > outputs when we don't have a valid color attachment for them, but
> > when
> > 
> > alpha to coverage is enabled we still want to preserve the the
> > output
> > 
> > at location 0 since we need the alpha component. In that case we
> > will
> > 
> > also need to create a null render target for RT 0.
> > 
> > 
> > 
> > v2:
> > 
> >   - We already create a null rt when we don't have any, so reuse
> > that
> > 
> >     for this case (Jason)
> > 
> >   - Simplify the code a bit (Iago)
> > 
> > 
> > 
> > v3:
> > 
> >   - Take alpha to coverage from the key and don't tie this to
> > depth-only
> > 
> >     rendering only, we want the same behavior if we have multiple
> > render
> > 
> >     targets but the one at location 0 is not used. (Jason).
> > 
> >   - Rewrite commit message (Iago)
> > 
> > 
> > 
> > v4:
> > 
> >   - Make sure we take into account the array length of the shader
> > outputs,
> > 
> >     which we were no handling correctly either and make sure we
> > also
> > 
> >     create null render targets for any invalid array entries too.
> > (Jason)
> > 
> > 
> > 
> > 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 | 56 ++++++++++++++++++++++++-----
> > ----
> > 
> >  1 file changed, 42 insertions(+), 14 deletions(-)
> > 
> > 
> > 
> > diff --git a/src/intel/vulkan/anv_pipeline.c
> > b/src/intel/vulkan/anv_pipeline.c
> > 
> > index 20eab548fb2..f15f0896266 100644
> > 
> > --- a/src/intel/vulkan/anv_pipeline.c
> > 
> > +++ b/src/intel/vulkan/anv_pipeline.c
> > 
> > @@ -823,14 +823,24 @@ anv_pipeline_link_fs(const struct
> > brw_compiler *compiler,
> > 
> >           continue;
> > 
> > 
> > 
> >        const unsigned rt = var->data.location - FRAG_RESULT_DATA0;
> > 
> > -      /* Unused or out-of-bounds */
> > 
> > -      if (rt >= MAX_RTS || !(stage->key.wm.color_outputs_valid &
> > (1 << rt)))
> > 
> > +      /* Out-of-bounds */
> > 
> > +      if (rt >= MAX_RTS)
> > 
> >           continue;
> > 
> > 
> > 
> >        const unsigned array_len =
> > 
> >           glsl_type_is_array(var->type) ? glsl_get_length(var-
> > >type) : 1;
> > 
> >        assert(rt + array_len <= max_rt);
> > 
> > 
> > 
> > +      /* Unused */
> > 
> > +      if (!(stage->key.wm.color_outputs_valid & BITFIELD_RANGE(rt,
> > array_len))) {
> > 
> > +         /* If this is the RT at location 0 and we have alpha to
> > coverage
> > 
> > +          * enabled we will have to create a null RT for it, so
> > mark it as
> > 
> > +          * used.
> > 
> > +          */
> > 
> > +         if (rt > 0 || !stage->key.wm.alpha_to_coverage)
> > 
> > +            continue;
> > 
> > +      }
> > 
> > +
> > 
> >        for (unsigned i = 0; i < array_len; i++)
> > 
> >           rt_used[rt + i] = true;
> > 
> >     }
> > 
> > @@ -841,11 +851,22 @@ anv_pipeline_link_fs(const struct
> > brw_compiler *compiler,
> > 
> >           continue;
> > 
> > 
> > 
> >        rt_to_bindings[i] = num_rts;
> > 
> > -      rt_bindings[rt_to_bindings[i]] = (struct
> > anv_pipeline_binding) {
> > 
> > -         .set = ANV_DESCRIPTOR_SET_COLOR_ATTACHMENTS,
> > 
> > -         .binding = 0,
> > 
> > -         .index = i,
> > 
> > -      };
> > 
> > +
> > 
> > +      if (stage->key.wm.color_outputs_valid & (1 << i)) {
> > 
> > +         rt_bindings[rt_to_bindings[i]] = (struct
> > anv_pipeline_binding) {
> > 
> > +            .set = ANV_DESCRIPTOR_SET_COLOR_ATTACHMENTS,
> > 
> > +            .binding = 0,
> > 
> > +            .index = i,
> > 
> > +         };
> > 
> > +      } else {
> > 
> > +         /* Setup a null render target */
> > 
> > +         rt_bindings[rt_to_bindings[i]] = (struct
> > anv_pipeline_binding) {
> > 
> > +            .set = ANV_DESCRIPTOR_SET_COLOR_ATTACHMENTS,
> > 
> > +            .binding = 0,
> > 
> > +            .index = UINT32_MAX,
> > 
> > +         };
> > 
> > +      }
> > 
> > +
> > 
> >        num_rts++;
> > 
> >     }
> > 
> > 
> > 
> > @@ -855,14 +876,21 @@ anv_pipeline_link_fs(const struct
> > brw_compiler *compiler,
> > 
> >           continue;
> > 
> > 
> > 
> >        const unsigned rt = var->data.location - FRAG_RESULT_DATA0;
> > 
> > +      const unsigned array_len =
> > 
> > +         glsl_type_is_array(var->type) ? glsl_get_length(var-
> > >type) : 1;
> > 
> > +
> > 
> >        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;
> > 
> > +          !(stage->key.wm.color_outputs_valid & BITFIELD_RANGE(rt,
> > array_len))) {
> > 
> > +         /* Unused or out-of-bounds, throw it away, unless it is
> > the first
> > 
> > +          * RT and we have alpha to coverage enabled.
> > 
> > +          */
> > 
> > +         if (rt != 0 || !stage->key.wm.alpha_to_coverage) {
> > 
> > +            deleted_output = true;
> > 
> > +            var->data.mode = nir_var_function_temp;
> > 
> > +            exec_node_remove(&var->node);
> > 
> > +            exec_list_push_tail(&impl->locals, &var->node);
> > 
> > +            continue;
> > 
> > +         }
> 
> I think we can simplify this yet further and just do
> 
> if (rt >= MAX_RTS || !rt_used[rt])
> 
> That way, all the logic stays in one place.

Right, that's much better.
> Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>

thanks!
Iago
> >        }
> > 
> > 
> > 
> >        /* Give it the new location */
> > 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190507/51d470e5/attachment-0001.html>