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

[Jason Ekstrand]

On Fri, May 3, 2019 at 6:52 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 when 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 its alpha component for alpha to coverage.
> 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)
>
> 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 | 48 +++++++++++++++++++++++++--------
>  1 file changed, 37 insertions(+), 11 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_pipeline.c
> b/src/intel/vulkan/anv_pipeline.c
> index 20eab548fb2..f379dd2752e 100644
> --- a/src/intel/vulkan/anv_pipeline.c
> +++ b/src/intel/vulkan/anv_pipeline.c
> @@ -818,15 +818,28 @@ anv_pipeline_link_fs(const struct brw_compiler
> *compiler,
>     memset(rt_used, 0, sizeof(rt_used));
>
>     /* Flag used render targets */
> +   bool needs_null_rt_for_alpha_to_coverage = false;
>     nir_foreach_variable_safe(var, &stage->nir->outputs) {
>        if (var->data.location < FRAG_RESULT_DATA0)
>           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;
>
> +      /* Unused */
> +      if (!(stage->key.wm.color_outputs_valid & (1 << rt))) {
>

While we're here, I realized reading this code today that we're only
checking one bit for the color attachment whereas we really should be
comparing against BITFIELD_RANGE(rt, array_size) here.


> +         /* If this is the RT at location 0 and we have alpha to coverage
> +          * enabled, we'll have to create a null render target and it must
> +          * be at index 0.
> +          */
> +         if (rt == 0 && stage->key.wm.alpha_to_coverage)
> +            needs_null_rt_for_alpha_to_coverage = true;
>

Why do we need all this needs_null_rt_for_alpha_to_coverage buisiness?  Why
not just let it fall through and set rt_used to true and then....


> +
> +         continue;
> +      }
> +
>        const unsigned array_len =
>           glsl_type_is_array(var->type) ? glsl_get_length(var->type) : 1;
>        assert(rt + array_len <= max_rt);
> @@ -835,7 +848,12 @@ anv_pipeline_link_fs(const struct brw_compiler
> *compiler,
>           rt_used[rt + i] = true;
>     }
>
> -   /* Set new, compacted, location */
> +   /* Make sure we leave the first RT slot available for alpha to coverage
> +    * if we don't have a valid RT 0.
> +    */
> +   if (needs_null_rt_for_alpha_to_coverage)
> +      num_rts = 1;
> +
>     for (unsigned i = 0; i < max_rt; i++) {
>        if (!rt_used[i])
>           continue;
>

Down here just do

if (stage->key.wm.color_outputs_valid & (1 << i)) {
   /* Set up a color attachment */
} else {
   /* Set up a null attachment */
}
num_rts++;

This would also fix a bug that I think we have today if you have an array
in the shader that only has some of it's inputs valid.  I think today you'd
end up crashing when we go to bind the non-existent color attachments.


> @@ -857,11 +875,15 @@ 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);
> +         /* Unused or out-of-bounds, throw it away, unless it is the first
> +          * RT and we have alpha to coverage.
> +          */
> +         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;
>        }
>
> @@ -873,14 +895,18 @@ anv_pipeline_link_fs(const struct brw_compiler
> *compiler,
>     if (deleted_output)
>        nir_fixup_deref_modes(stage->nir);
>
> -   if (num_rts == 0) {
> -      /* If we have no render targets, we need a null render target */
> +   /* If we have no render targets or we need to create one for alpha to
> +    * coverage, we need a null render target.
> +    */
> +   if (num_rts == 0 || needs_null_rt_for_alpha_to_coverage) {
>        rt_bindings[0] = (struct anv_pipeline_binding) {
>           .set = ANV_DESCRIPTOR_SET_COLOR_ATTACHMENTS,
>           .binding = 0,
>           .index = UINT32_MAX,
>        };
> -      num_rts = 1;
> +
> +      if (num_rts == 0)
> +         num_rts = 1;
>     }
>
>     /* Now that we've determined the actual number of render targets,
> adjust
> --
> 2.17.1
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190503/182de6d4/attachment.html>