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

[Jason Ekstrand]

On March 27, 2018 23:14:09 Iago Toral  wrote:
On Tue, 2018-03-27 at 09:06 -0700, Jason Ekstrand wrote:
I'm sorry I've been so incredibly out-to-lunch on reviewing this. :-(

On Tue, Mar 27, 2018 at 12:53 AM, 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.

v5 (Caio):
- Rebased.
- Initialize pending clear views to only have bits set for layers
that exist.
- Reset pending clear views in one go rather one bit at a time.
- Put "last subpass for this attachment" condition in a separate
function to simplify the conditional that resets pending_clear_aspects.

Fixes:
dEQP-VK.multiview.readback_implicit_clear.*
---
src/intel/vulkan/anv_private.h |  8 
src/intel/vulkan/genX_cmd_buffer.c | 91 --
2 files changed, 96 insertions(+), 3 deletions(-)

diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
index ee533581ab..0e209e1769 100644
--- a/src/intel/vulkan/anv_private.h
+++ b/src/intel/vulkan/anv_private.h
@@ -1724,6 +1724,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;

Thinking about this some more and reading our previous discussion, I do 
think that we could have a view_mask in anv_render_pass_attachment and just 
clear all the used views up-front at once instead of when they're first 
used.  It might make things slightly simpler and potentially more efficient 
in future since we could batch the BLORP ops up.  However, I've also been 
really lazy about reviewing so I don't really want to make you go rework 
this yet again.  I think this is correct and fairly reasonable so this 
version is


Actually, this was explictly discussed in the CTS working group some time 
after we talked about it, and the conclusion was that drivers that do that 
would not be following the spec. I am not sure that there is a good reason 
for the spec to request this behavior though...


I don't know how they would be able to tell so long as the driver was 
careful to only clear the views that actually get used.  In any case, this 
is fine.




Reviewed-by: Jason Ekstrand 

Thanks!, what about the other patch? :)

Rb for that one too


};

/** 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 b5741fb8dc..3c55cd964c 100644
--- a/src/intel/vulkan/genX_cmd_buffer.c
+++ b/src/intel/vulkan/genX_cmd_buffer.c
@@ -1250,6 +1250,9 @@ genX(cmd_buffer_setup_attachments)(struct 
anv_cmd_buffer *cmd_buffer,

anv_assert(iview->vk_format == att->format);
anv_assert(iview->n_planes == 1);

+ const uint32_t num_layers = iview->planes[0].isl.array_len;
+ state->attachments[i].pending_clear_views = (1 << num_layers) - 1;
+
union isl_color_value clear_color = { .u32 = { 0, } };
if (att_aspects & VK_IMAGE_ASPECT_ANY_COLOR_BIT_ANV) {
assert(att_aspects == VK_IMAGE_ASPECT_COLOR_BIT);
@@ -3414,6 +3417,42 @@ 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,

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

[Iago Toral]

On Tue, 2018-03-27 at 09:06 -0700, Jason Ekstrand wrote:
> I'm sorry I've been so incredibly out-to-lunch on reviewing this. :-(
> 
> On Tue, Mar 27, 2018 at 12:53 AM, Iago Toral Quiroga  om> 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.
> > 
> > 
> > 
> > v5 (Caio):
> > 
> >  - Rebased.
> > 
> >  - Initialize pending clear views to only have bits set for layers
> > 
> >that exist.
> > 
> >  - Reset pending clear views in one go rather one bit at a time.
> > 
> >  - Put "last subpass for this attachment" condition in a separate
> > 
> >function to simplify the conditional that resets
> > pending_clear_aspects.
> > 
> > 
> > 
> > Fixes:
> > 
> > dEQP-VK.multiview.readback_implicit_clear.*
> > 
> > ---
> > 
> >  src/intel/vulkan/anv_private.h |  8 
> > 
> >  src/intel/vulkan/genX_cmd_buffer.c | 91
> > --
> > 
> >  2 files changed, 96 insertions(+), 3 deletions(-)
> > 
> > 
> > 
> > diff --git a/src/intel/vulkan/anv_private.h
> > b/src/intel/vulkan/anv_private.h
> > 
> > index ee533581ab..0e209e1769 100644
> > 
> > --- a/src/intel/vulkan/anv_private.h
> > 
> > +++ b/src/intel/vulkan/anv_private.h
> > 
> > @@ -1724,6 +1724,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;
> 
> Thinking about this some more and reading our previous discussion, I
> do think that we could have a view_mask in anv_render_pass_attachment
> and just clear all the used views up-front at once instead of when
> they're first used.  It might make things slightly simpler and
> potentially more efficient in future since we could batch the BLORP
> ops up.  However, I've also been really lazy about reviewing so I
> don't really want to make you go rework this yet again.  I think this
> is correct and fairly reasonable so this version is

Actually, this was explictly discussed in the CTS working group some
time after we talked about it, and the conclusion was that drivers that
do that would not be following the spec. I am not sure that there is a
good reason for the spec to request this behavior though...
> Reviewed-by: Jason Ekstrand 

Thanks!, what about the other patch? :)
> >  };
> > 
> > 
> > 
> >  /** 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 b5741fb8dc..3c55cd964c 100644
> > 
> > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > 
> > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > 
> > @@ -1250,6 +1250,9 @@ genX(cmd_buffer_setup_attachments)(struct
> > anv_cmd_buffer *cmd_buffer,
> > 
> >   anv_assert(iview->vk_format == att->format);
> > 
> >   anv_assert(iview->n_planes == 1);
> > 
> > 
> > 
> > + const uint32_t num_layers = iview-
> > >planes[0].isl.array_len;
> > 
> > + state->attachments[i].pending_clear_views = (1 <<
> > num_layers) - 1;
> > 
> > +
> > 
> >   union isl_color_value clear_color = { .u32 = { 0, } };
> > 
> >   if (att_aspects & VK_IMAGE_ASPECT_ANY_COLOR_BIT_ANV) {
> > 
> >  assert(att_aspects == VK_IMAGE_ASPECT_COLOR_BIT);
> > 
> > @@ -3414,6 +3417,42 @@ cmd_buffer_emit_depth_stencil(struct
> > anv_cmd_buffer *cmd_buffer)
> > 
> > cmd_buffer->state.hiz_enabled = 

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

[Jason Ekstrand]

I'm sorry I've been so incredibly out-to-lunch on reviewing this. :-(

On Tue, Mar 27, 2018 at 12:53 AM, 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.
>
> v5 (Caio):
>  - Rebased.
>  - Initialize pending clear views to only have bits set for layers
>that exist.
>  - Reset pending clear views in one go rather one bit at a time.
>  - Put "last subpass for this attachment" condition in a separate
>function to simplify the conditional that resets pending_clear_aspects.
>
> Fixes:
> dEQP-VK.multiview.readback_implicit_clear.*
> ---
>  src/intel/vulkan/anv_private.h |  8 
>  src/intel/vulkan/genX_cmd_buffer.c | 91 ++
> ++--
>  2 files changed, 96 insertions(+), 3 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private
> .h
> index ee533581ab..0e209e1769 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -1724,6 +1724,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;
>

Thinking about this some more and reading our previous discussion, I do
think that we could have a view_mask in anv_render_pass_attachment and just
clear all the used views up-front at once instead of when they're first
used.  It might make things slightly simpler and potentially more efficient
in future since we could batch the BLORP ops up.  However, I've also been
really lazy about reviewing so I don't really want to make you go rework
this yet again.  I think this is correct and fairly reasonable so this
version is

Reviewed-by: Jason Ekstrand 


>  };
>
>  /** 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 b5741fb8dc..3c55cd964c 100644
> --- a/src/intel/vulkan/genX_cmd_buffer.c
> +++ b/src/intel/vulkan/genX_cmd_buffer.c
> @@ -1250,6 +1250,9 @@ genX(cmd_buffer_setup_attachments)(struct
> anv_cmd_buffer *cmd_buffer,
>   anv_assert(iview->vk_format == att->format);
>   anv_assert(iview->n_planes == 1);
>
> + const uint32_t num_layers = iview->planes[0].isl.array_len;
> + state->attachments[i].pending_clear_views = (1 << num_layers) -
> 1;
> +
>   union isl_color_value clear_color = { .u32 = { 0, } };
>   if (att_aspects & VK_IMAGE_ASPECT_ANY_COLOR_BIT_ANV) {
>  assert(att_aspects == VK_IMAGE_ASPECT_COLOR_BIT);
> @@ -3414,6 +3417,42 @@ 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 inline bool
> +current_subpass_is_last_for_attachment(const 

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

[Iago Toral Quiroga]

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.

v5 (Caio):
 - Rebased.
 - Initialize pending clear views to only have bits set for layers
   that exist.
 - Reset pending clear views in one go rather one bit at a time.
 - Put "last subpass for this attachment" condition in a separate
   function to simplify the conditional that resets pending_clear_aspects.

Fixes:
dEQP-VK.multiview.readback_implicit_clear.*
---
 src/intel/vulkan/anv_private.h |  8 
 src/intel/vulkan/genX_cmd_buffer.c | 91 --
 2 files changed, 96 insertions(+), 3 deletions(-)

diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
index ee533581ab..0e209e1769 100644
--- a/src/intel/vulkan/anv_private.h
+++ b/src/intel/vulkan/anv_private.h
@@ -1724,6 +1724,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 b5741fb8dc..3c55cd964c 100644
--- a/src/intel/vulkan/genX_cmd_buffer.c
+++ b/src/intel/vulkan/genX_cmd_buffer.c
@@ -1250,6 +1250,9 @@ genX(cmd_buffer_setup_attachments)(struct anv_cmd_buffer 
*cmd_buffer,
  anv_assert(iview->vk_format == att->format);
  anv_assert(iview->n_planes == 1);
 
+ const uint32_t num_layers = iview->planes[0].isl.array_len;
+ state->attachments[i].pending_clear_views = (1 << num_layers) - 1;
+
  union isl_color_value clear_color = { .u32 = { 0, } };
  if (att_aspects & VK_IMAGE_ASPECT_ANY_COLOR_BIT_ANV) {
 assert(att_aspects == VK_IMAGE_ASPECT_COLOR_BIT);
@@ -3414,6 +3417,42 @@ 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 inline bool
+current_subpass_is_last_for_attachment(const struct anv_cmd_state *cmd_state,
+   uint32_t att_idx)
+{
+   const uint32_t last_subpass_idx =
+  cmd_state->pass->attachments[att_idx].last_subpass_idx;
+   const struct anv_subpass *last_subpass =
+  _state->pass->subpasses[last_subpass_idx];
+   return last_subpass == cmd_state->subpass;
+}
+
 static void
 cmd_buffer_begin_subpass(struct anv_cmd_buffer *cmd_buffer,
  uint32_t subpass_id)
@@ -3452,6 +3491,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)
@@ -3519,7 +3560,8 @@