[Mesa-dev] [PATCH v4 1/2] anv/cmd_buffer: consider multiview masks for tracking pending clear aspects

[Caio Marcelo de Oliveira Filho]

Hi Iago,

> Fixes:
> dEQP-VK.multiview.readback_implicit_clear.*

Applied locally and verified this. Thanks for fixing those.

I have a couple of comments after reading the patch, feel free to take
them only if make sense to you :-)


> +   /* When multiview is active, attachments with a renderpass clear
> +    * operation have their respective layers cleared on the first
> +    * subpass that uses them, and only in that subpass. We keep track
> +    * of this using a bitfield to indicate which layers of an attachment
> +    * have not been cleared yet when multiview is active.
> +    */
> +   uint32_t                                     pending_clear_views;
>  };

(...)

> +         state->attachments[i].pending_clear_views = ~0;

I was expecting pending_clear_views to have bit set only for the views
that are being used -- i.e. it would be initalized with the
combination (with the '|' operator) of all the view_masks of the
subpasses. While setting all the bits to one works correctly, being
more precise here could aid future debugging.


> +            for_each_bit(layer_idx, pending_clear_mask) {
> +               uint32_t layer =
> +                  iview->planes[0].isl.base_array_layer + layer_idx;
> +
> +               anv_image_clear_color(cmd_buffer, image,
> +                                     VK_IMAGE_ASPECT_COLOR_BIT,
> +                                     att_state->aux_usage,
> +                                     iview->planes[0].isl.format,
> +                                     iview->planes[0].isl.swizzle,
> +                                     iview->planes[0].isl.base_level,
> +                                     layer, 1,
> +                                     render_area,
> +                                     vk_to_isl_color(att_state->clear_value.color));
> +
> +               att_state->pending_clear_views &= ~(1 << layer_idx);
> +            }

Consider resetting the pending_clear_views bits all at once after the
for_each_bit loop with something like

    att_state->pending_clear_views &= ~pending_clear_mask;

(That also applies to the other patch).


> @@ -3525,7 +3589,24 @@ cmd_buffer_begin_subpass(struct anv_cmd_buffer *cmd_buffer,
>           }
>        }
>  
> -      att_state->pending_clear_aspects = 0;
> +      /* If multiview is enabled, then we are only done clearing when we no
> +       * longer have pending layers to clear, or when we have processed the
> +       * last subpass that uses this attachment.
> +       */
> +      if (!is_multiview) {
> +         att_state->pending_clear_aspects = 0;
> +      } else if (att_state->pending_clear_views == 0) {
> +         att_state->pending_clear_aspects = 0;
> +      } else {
> +         uint32_t last_subpass_idx =
> +            cmd_state->pass->attachments[a].last_subpass_idx;
> +         const struct anv_subpass *last_subpass =
> +            &cmd_state->pass->subpasses[last_subpass_idx];
> +         if (last_subpass == cmd_state->subpass) {
> +            att_state->pending_clear_aspects = 0;
> +         }
> +      }
> +

Consider extracting the "last subpass for this attachment" condition
into a local variable or a function, and make a single if with the
combinations of the conditions.


Thanks,
Caio