[v4] anv: fix alphaToCoverage when there is no color attachment

Submitted by Iago Toral Quiroga on May 6, 2019, 2:01 p.m.

Details

Message ID 20190506140141.4955-1-itoral@igalia.com
State New
Headers show
Series "anv: fix alphaToCoverage when there is no color attachment" ( rev: 4 ) in Mesa

Not browsing as part of any series.

Commit Message

Iago Toral Quiroga May 6, 2019, 2:01 p.m.
From: Samuel Iglesias Gonsálvez <siglesias@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@igalia.com>
Signed-off-by: Iago Toral Quiroga <itoral@igalia.com>
---
 src/intel/vulkan/anv_pipeline.c | 56 ++++++++++++++++++++++++---------
 1 file changed, 42 insertions(+), 14 deletions(-)

Patch hide | download patch | download mbox

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;
+         }
       }
 
       /* Give it the new location */

Comments



On Mon, 2019-05-06 at 16:01 +0200, Iago Toral Quiroga wrote:
> From: Samuel Iglesias Gonsálvez <siglesias@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.
> 

I'm adding this commit to the 19.1 stable queue.


	J.A.

> 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@igalia.com>
> Signed-off-by: Iago Toral Quiroga <itoral@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;
> +         }
>        }
>  
>        /* Give it the new location */