[Mesa-dev] [PATCH v3] anv: fix bug when using component qualifier in FS outputs

[Jason Ekstrand]

On Mon, Dec 11, 2017 at 2:59 AM, Samuel Iglesias Gonsálvez <
siglesias at igalia.com> wrote:

> We can write to the same output but in different components, like
> in this example:
>
> layout(location = 0, component = 0) out ivec2 dEQP_FragColor_0;
> layout(location = 0, component = 2) out ivec2 dEQP_FragColor_1;
>
> Therefore, they are not two different outputs but only one.
>
> Fixes:
>
> dEQP-VK.glsl.440.linkage.varying.component.frag_out.*
>
> v3:
> - Remove FRAG_RESULT_MAX.
> - Add const and use sizeof (Ian).
> - Do three-pass to set properly the locations of fragment
>   outputs when having arrays (Jason).
>
> Signed-off-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
> ---
>
> I wrote CL#2003 adding more tests to check this fix.
>
>  src/intel/vulkan/anv_pipeline.c | 63 ++++++++++++++++++++++++++++--
> -----------
>  1 file changed, 44 insertions(+), 19 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_pipeline.c b/src/intel/vulkan/anv_
> pipeline.c
> index f2d4d113219..407f2970199 100644
> --- a/src/intel/vulkan/anv_pipeline.c
> +++ b/src/intel/vulkan/anv_pipeline.c
> @@ -878,13 +878,51 @@ anv_pipeline_compile_fs(struct anv_pipeline
> *pipeline,
>        }
>
>        unsigned num_rts = 0;
> -      struct anv_pipeline_binding rt_bindings[8];
> +      const int max_rt = FRAG_RESULT_DATA7 - FRAG_RESULT_DATA0 + 1;
> +      struct anv_pipeline_binding rt_bindings[max_rt];
>        nir_function_impl *impl = nir_shader_get_entrypoint(nir);
> +      int rt_to_bindings[max_rt];
> +      memset(rt_to_bindings, -1, sizeof(rt_to_bindings));
> +      bool rt_used[max_rt];
>

This could be a bitfield but I don't really care.


> +      memset(rt_used, 0, sizeof(rt_used));
> +
> +      /* Flag used render targets */
> +      nir_foreach_variable_safe(var, &nir->outputs) {
> +         if (var->data.location < FRAG_RESULT_DATA0)
> +            continue;
> +
> +         const unsigned rt = var->data.location - FRAG_RESULT_DATA0;
> +         /* Out-of-bounds */
> +         if (rt >= key.nr_color_regions)
> +            continue;
> +
> +         const unsigned array_len =
> +            glsl_type_is_array(var->type) ? glsl_get_length(var->type) :
> 1;
> +         assert(rt + array_len <= max_rt);
> +
> +         for (unsigned i = 0; i < array_len; i++)
> +            rt_used[rt + i] = true;
> +      }
> +
> +      /* Set new, compacted, location */
> +      for (unsigned i = 0; i < max_rt; i++) {
> +         if (!rt_used[i])
> +            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,
> +         };
> +         num_rts++;
> +      }
> +
>        nir_foreach_variable_safe(var, &nir->outputs) {
>           if (var->data.location < FRAG_RESULT_DATA0)
>              continue;
>
> -         unsigned rt = var->data.location - FRAG_RESULT_DATA0;
> +         const unsigned rt = var->data.location - FRAG_RESULT_DATA0;
>           if (rt >= key.nr_color_regions) {
>              /* Out-of-bounds, throw it away */
>              var->data.mode = nir_var_local;
> @@ -893,22 +931,9 @@ anv_pipeline_compile_fs(struct anv_pipeline *pipeline,
>              continue;
>           }
>
> -         /* Give it a new, compacted, location */
> -         var->data.location = FRAG_RESULT_DATA0 + num_rts;
> -
> -         unsigned array_len =
> -            glsl_type_is_array(var->type) ? glsl_get_length(var->type) :
> 1;
> -         assert(num_rts + array_len <= 8);
> -
> -         for (unsigned i = 0; i < array_len; i++) {
> -            rt_bindings[num_rts + i] = (struct anv_pipeline_binding) {
> -               .set = ANV_DESCRIPTOR_SET_COLOR_ATTACHMENTS,
> -               .binding = 0,
> -               .index = rt + i,
> -            };
> -         }
> -
> -         num_rts += array_len;
> +         /* Give it the new location */
> +         assert(rt_to_bindings[rt] != -1);
> +         var->data.location = rt_to_bindings[rt] + FRAG_RESULT_DATA0;
>

This should do the trick.

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


>        }
>
>        if (num_rts == 0) {
> @@ -921,7 +946,7 @@ anv_pipeline_compile_fs(struct anv_pipeline *pipeline,
>           num_rts = 1;
>        }
>
> -      assert(num_rts <= 8);
> +      assert(num_rts <= max_rt);
>        map.surface_to_descriptor -= num_rts;
>        map.surface_count += num_rts;
>        assert(map.surface_count <= 256);
> --
> 2.11.0
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171211/5d9f31d6/attachment.html>