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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
