On Mon, May 6, 2019 at 9:01 AM Iago Toral Quiroga <ito...@igalia.com> wrote:
> From: Samuel Iglesias Gonsálvez <sigles...@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 <sigles...@igalia.com>
> Signed-off-by: Iago Toral Quiroga <ito...@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 <ja...@jlekstrand.net>
> }
>
> /* Give it the new location */
> --
> 2.17.1
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
