Re: [Mesa-dev] [PATCH v4 1/2] anv/cmd_buffer: consider multiview masks for tracking pending clear aspects
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 =
> +_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
Re: [Mesa-dev] [PATCH v4 1/2] anv/cmd_buffer: consider multiview masks for tracking pending clear aspects
Hi Jason, can you have a look at these two? We have some tests failing
because of this.
Iago
On Wed, 2018-03-07 at 10:33 +0100, Iago Toral wrote:
> This patches are still waiting for review.
>
> On Wed, 2018-02-28 at 09:57 +0100, Iago Toral Quiroga wrote:
> > When multiview is active a subpass clear may only clear a subset of
> > the
> > attachment layers. Other subpasses in the same render pass may also
> > clear too and we want to honor those clears as well, however, we
> > need
> > to
> > ensure that we only clear a layer once, on the first subpass that
> > uses
> > a particular layer (view) of a given attachment.
> >
> > This means that when we check if a subpass attachment needs to be
> > cleared
> > we need to check if all the layers used by that subpass (as
> > indicated
> > by
> > its view_mask) have already been cleared in previous subpasses or
> > not, in
> > which case, we must clear any pending layers used by the subpass,
> > and
> > only
> > those pending.
> >
> > v2:
> > - track pending clear views in the attachment state (Jason)
> > - rebased on top of fast-clear rework.
> >
> > v3:
> > - rebased on top of subpass rework.
> >
> > v4: rebased.
> >
> > Fixes:
> > dEQP-VK.multiview.readback_implicit_clear.*
> > ---
> > src/intel/vulkan/anv_private.h | 8
> > src/intel/vulkan/genX_cmd_buffer.c | 87
> > --
> > 2 files changed, 92 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/intel/vulkan/anv_private.h
> > b/src/intel/vulkan/anv_private.h
> > index fb4fd19178..352f0483f0 100644
> > --- a/src/intel/vulkan/anv_private.h
> > +++ b/src/intel/vulkan/anv_private.h
> > @@ -1696,6 +1696,14 @@ struct anv_attachment_state {
> > VkClearValue clear_value;
> > bool clear_color_is_zer
> > o_
> > one;
> > bool clear_color_is_zer
> > o;
> > +
> > + /* 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_view
> > s;
> > };
> >
> > /** State tracking for particular pipeline bind point
> > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> > b/src/intel/vulkan/genX_cmd_buffer.c
> > index ce546249b3..c422ec8e1a 100644
> > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > @@ -1167,6 +1167,8 @@ genX(cmd_buffer_setup_attachments)(struct
> > anv_cmd_buffer *cmd_buffer,
> > if (clear_aspects)
> > state->attachments[i].clear_value = begin-
> > > pClearValues[i];
> >
> >
> > + state->attachments[i].pending_clear_views = ~0;
> > +
> > struct anv_image_view *iview = framebuffer-
> > >attachments[i];
> > anv_assert(iview->vk_format == att->format);
> > anv_assert(iview->n_planes == 1);
> > @@ -3288,6 +3290,31 @@ cmd_buffer_emit_depth_stencil(struct
> > anv_cmd_buffer *cmd_buffer)
> > cmd_buffer->state.hiz_enabled = info.hiz_usage ==
> > ISL_AUX_USAGE_HIZ;
> > }
> >
> > +/**
> > + * This ANDs the view mask of the current subpass with the pending
> > clear
> > + * views in the attachment to get the mask of views active in the
> > subpass
> > + * that still need to be cleared.
> > + */
> > +static inline uint32_t
> > +get_multiview_subpass_clear_mask(const struct anv_cmd_state
> > *cmd_state,
> > + const struct anv_attachment_state
> > *att_state)
> > +{
> > + return cmd_state->subpass->view_mask & att_state-
> > > pending_clear_views;
> >
> > +}
> > +
> > +static inline bool
> > +do_first_layer_clear(const struct anv_cmd_state *cmd_state,
> > + const struct anv_attachment_state *att_state)
> > +{
> > + if (!cmd_state->subpass->view_mask)
> > + return true;
> > +
> > + uint32_t pending_clear_mask =
> > + get_multiview_subpass_clear_mask(cmd_state, att_state);
> > +
> > + return pending_clear_mask & 1;
> > +}
> > +
> > static void
> > cmd_buffer_begin_subpass(struct anv_cmd_buffer *cmd_buffer,
> > uint32_t subpass_id)
> > @@ -3326,6 +3353,8 @@ cmd_buffer_begin_subpass(struct
> > anv_cmd_buffer
> > *cmd_buffer,
> > VkRect2D render_area = cmd_buffer->state.render_area;
> > struct anv_framebuffer *fb = cmd_buffer->state.framebuffer;
> >
> > + bool is_multiview = subpass->view_mask != 0;
> > +
> > for (uint32_t i = 0; i < subpass->attachment_count; ++i) {
> >const uint32_t a = subpass->attachments[i].attachment;
> >if (a == VK_ATTACHMENT_UNUSED)
> > @@ -3393,7 +3422,8 @@ cmd_buffer_begin_subpass(struct
> >
Re: [Mesa-dev] [PATCH v4 1/2] anv/cmd_buffer: consider multiview masks for tracking pending clear aspects
This patches are still waiting for review.
On Wed, 2018-02-28 at 09:57 +0100, Iago Toral Quiroga wrote:
> When multiview is active a subpass clear may only clear a subset of
> the
> attachment layers. Other subpasses in the same render pass may also
> clear too and we want to honor those clears as well, however, we need
> to
> ensure that we only clear a layer once, on the first subpass that
> uses
> a particular layer (view) of a given attachment.
>
> This means that when we check if a subpass attachment needs to be
> cleared
> we need to check if all the layers used by that subpass (as indicated
> by
> its view_mask) have already been cleared in previous subpasses or
> not, in
> which case, we must clear any pending layers used by the subpass, and
> only
> those pending.
>
> v2:
> - track pending clear views in the attachment state (Jason)
> - rebased on top of fast-clear rework.
>
> v3:
> - rebased on top of subpass rework.
>
> v4: rebased.
>
> Fixes:
> dEQP-VK.multiview.readback_implicit_clear.*
> ---
> src/intel/vulkan/anv_private.h | 8
> src/intel/vulkan/genX_cmd_buffer.c | 87
> --
> 2 files changed, 92 insertions(+), 3 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_private.h
> b/src/intel/vulkan/anv_private.h
> index fb4fd19178..352f0483f0 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -1696,6 +1696,14 @@ struct anv_attachment_state {
> VkClearValue clear_value;
> bool clear_color_is_zero_
> one;
> bool clear_color_is_zero;
> +
> + /* 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 tracking for particular pipeline bind point
> diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> b/src/intel/vulkan/genX_cmd_buffer.c
> index ce546249b3..c422ec8e1a 100644
> --- a/src/intel/vulkan/genX_cmd_buffer.c
> +++ b/src/intel/vulkan/genX_cmd_buffer.c
> @@ -1167,6 +1167,8 @@ genX(cmd_buffer_setup_attachments)(struct
> anv_cmd_buffer *cmd_buffer,
> if (clear_aspects)
> state->attachments[i].clear_value = begin-
> >pClearValues[i];
>
> + state->attachments[i].pending_clear_views = ~0;
> +
> struct anv_image_view *iview = framebuffer->attachments[i];
> anv_assert(iview->vk_format == att->format);
> anv_assert(iview->n_planes == 1);
> @@ -3288,6 +3290,31 @@ cmd_buffer_emit_depth_stencil(struct
> anv_cmd_buffer *cmd_buffer)
> cmd_buffer->state.hiz_enabled = info.hiz_usage ==
> ISL_AUX_USAGE_HIZ;
> }
>
> +/**
> + * This ANDs the view mask of the current subpass with the pending
> clear
> + * views in the attachment to get the mask of views active in the
> subpass
> + * that still need to be cleared.
> + */
> +static inline uint32_t
> +get_multiview_subpass_clear_mask(const struct anv_cmd_state
> *cmd_state,
> + const struct anv_attachment_state
> *att_state)
> +{
> + return cmd_state->subpass->view_mask & att_state-
> >pending_clear_views;
> +}
> +
> +static inline bool
> +do_first_layer_clear(const struct anv_cmd_state *cmd_state,
> + const struct anv_attachment_state *att_state)
> +{
> + if (!cmd_state->subpass->view_mask)
> + return true;
> +
> + uint32_t pending_clear_mask =
> + get_multiview_subpass_clear_mask(cmd_state, att_state);
> +
> + return pending_clear_mask & 1;
> +}
> +
> static void
> cmd_buffer_begin_subpass(struct anv_cmd_buffer *cmd_buffer,
> uint32_t subpass_id)
> @@ -3326,6 +3353,8 @@ cmd_buffer_begin_subpass(struct anv_cmd_buffer
> *cmd_buffer,
> VkRect2D render_area = cmd_buffer->state.render_area;
> struct anv_framebuffer *fb = cmd_buffer->state.framebuffer;
>
> + bool is_multiview = subpass->view_mask != 0;
> +
> for (uint32_t i = 0; i < subpass->attachment_count; ++i) {
>const uint32_t a = subpass->attachments[i].attachment;
>if (a == VK_ATTACHMENT_UNUSED)
> @@ -3393,7 +3422,8 @@ cmd_buffer_begin_subpass(struct anv_cmd_buffer
> *cmd_buffer,
> uint32_t base_clear_layer = iview-
> >planes[0].isl.base_array_layer;
> uint32_t clear_layer_count = fb->layers;
>
> - if (att_state->fast_clear) {
> + if (att_state->fast_clear &&
> + do_first_layer_clear(cmd_state, att_state)) {
> /* We only support fast-clears on the first layer */
> assert(iview->planes[0].isl.base_level == 0);
>
[Mesa-dev] [PATCH v4 1/2] anv/cmd_buffer: consider multiview masks for tracking pending clear aspects
When multiview is active a subpass clear may only clear a subset of the
attachment layers. Other subpasses in the same render pass may also
clear too and we want to honor those clears as well, however, we need to
ensure that we only clear a layer once, on the first subpass that uses
a particular layer (view) of a given attachment.
This means that when we check if a subpass attachment needs to be cleared
we need to check if all the layers used by that subpass (as indicated by
its view_mask) have already been cleared in previous subpasses or not, in
which case, we must clear any pending layers used by the subpass, and only
those pending.
v2:
- track pending clear views in the attachment state (Jason)
- rebased on top of fast-clear rework.
v3:
- rebased on top of subpass rework.
v4: rebased.
Fixes:
dEQP-VK.multiview.readback_implicit_clear.*
---
src/intel/vulkan/anv_private.h | 8
src/intel/vulkan/genX_cmd_buffer.c | 87 --
2 files changed, 92 insertions(+), 3 deletions(-)
diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
index fb4fd19178..352f0483f0 100644
--- a/src/intel/vulkan/anv_private.h
+++ b/src/intel/vulkan/anv_private.h
@@ -1696,6 +1696,14 @@ struct anv_attachment_state {
VkClearValue clear_value;
bool clear_color_is_zero_one;
bool clear_color_is_zero;
+
+ /* 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 tracking for particular pipeline bind point
diff --git a/src/intel/vulkan/genX_cmd_buffer.c
b/src/intel/vulkan/genX_cmd_buffer.c
index ce546249b3..c422ec8e1a 100644
--- a/src/intel/vulkan/genX_cmd_buffer.c
+++ b/src/intel/vulkan/genX_cmd_buffer.c
@@ -1167,6 +1167,8 @@ genX(cmd_buffer_setup_attachments)(struct anv_cmd_buffer
*cmd_buffer,
if (clear_aspects)
state->attachments[i].clear_value = begin->pClearValues[i];
+ state->attachments[i].pending_clear_views = ~0;
+
struct anv_image_view *iview = framebuffer->attachments[i];
anv_assert(iview->vk_format == att->format);
anv_assert(iview->n_planes == 1);
@@ -3288,6 +3290,31 @@ cmd_buffer_emit_depth_stencil(struct anv_cmd_buffer
*cmd_buffer)
cmd_buffer->state.hiz_enabled = info.hiz_usage == ISL_AUX_USAGE_HIZ;
}
+/**
+ * This ANDs the view mask of the current subpass with the pending clear
+ * views in the attachment to get the mask of views active in the subpass
+ * that still need to be cleared.
+ */
+static inline uint32_t
+get_multiview_subpass_clear_mask(const struct anv_cmd_state *cmd_state,
+ const struct anv_attachment_state *att_state)
+{
+ return cmd_state->subpass->view_mask & att_state->pending_clear_views;
+}
+
+static inline bool
+do_first_layer_clear(const struct anv_cmd_state *cmd_state,
+ const struct anv_attachment_state *att_state)
+{
+ if (!cmd_state->subpass->view_mask)
+ return true;
+
+ uint32_t pending_clear_mask =
+ get_multiview_subpass_clear_mask(cmd_state, att_state);
+
+ return pending_clear_mask & 1;
+}
+
static void
cmd_buffer_begin_subpass(struct anv_cmd_buffer *cmd_buffer,
uint32_t subpass_id)
@@ -3326,6 +3353,8 @@ cmd_buffer_begin_subpass(struct anv_cmd_buffer
*cmd_buffer,
VkRect2D render_area = cmd_buffer->state.render_area;
struct anv_framebuffer *fb = cmd_buffer->state.framebuffer;
+ bool is_multiview = subpass->view_mask != 0;
+
for (uint32_t i = 0; i < subpass->attachment_count; ++i) {
const uint32_t a = subpass->attachments[i].attachment;
if (a == VK_ATTACHMENT_UNUSED)
@@ -3393,7 +3422,8 @@ cmd_buffer_begin_subpass(struct anv_cmd_buffer
*cmd_buffer,
uint32_t base_clear_layer = iview->planes[0].isl.base_array_layer;
uint32_t clear_layer_count = fb->layers;
- if (att_state->fast_clear) {
+ if (att_state->fast_clear &&
+ do_first_layer_clear(cmd_state, att_state)) {
/* We only support fast-clears on the first layer */
assert(iview->planes[0].isl.base_level == 0);
assert(iview->planes[0].isl.base_array_layer == 0);
@@ -3402,6 +3432,8 @@ cmd_buffer_begin_subpass(struct anv_cmd_buffer
*cmd_buffer,
0, 0, 1, ISL_AUX_OP_FAST_CLEAR, false);
base_clear_layer++;
clear_layer_count--;
+if (is_multiview)
+ att_state->pending_clear_views &= ~1;
