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

[Jason Ekstrand]

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.

With that change,

Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>


>        }
>
>        /* Give it the new location */
> --
> 2.17.1
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190506/ebef8fe7/attachment-0001.html>