On Wed, 2018-01-24 at 01:51 -0800, Jason Ekstrand wrote:
> Thanks for the detailed explanation. I have a couple of comments:
>
> 1) This is going to conflict with some of the stuff I've got in-
> flight that reworks fast-clears significantly. I think it will make
> everything easier if we land the reworks first.
Ok, no problem. Let me know when that stuff lands and I'll re-work this
on top. Should I also hold back the first patch in this series?
> 2) It may be better to add a pending_clear_views to
> anv_attachment_state and use that for "pending" state and switch
> pending_clear_aspects to clear_aspects.
Yes, after I wrote this patch I was also wondering whether that would
make things cleaner. I'll do this in a v3.
ThanksIago
On Wed, Jan 24, 2018 at 12:44 AM, Iago Toral <ito...@igalia.com> wrote:
> That's a good question, I was asking this myself when I did this. The
> spec says:
>
> "loadOp is a VkAttachmentLoadOp value specifying how the
> contents of color and depth components of the attachment are
> treated at
> the beginning of the subpass where it is first used."
>
> So I figured the correct thing to do that follows the spirit of this
> was to wait until the first subpass that uses a particular view to
> clear it. Now, I have checked the spec (with KHX_multiview included)
> and it seems to have some more text in place to address this
> question:bit more:
>
> "The load and store operations apply to attachments on a per-view
> basis.
> For example, an attachment using VK_ATTACHMENT_LOAD_OP_CLEAR will
> have
> each view cleared on first use, but the first use of one view may be
> temporally distant from the first use of another view.
> (...)
> This interpretation motivates the answers to questions like “when
> does the
> load op apply” - it is on the first use of each view of an
> attachment, as
> if each view were a separate attachment."
>
> I understand this is consistent with the implementation in this patch
> (although I wonder if there are any scenarios where this has actual
> implications...)
>
> Iago
> On Tue, 2018-01-23 at 23:57 -0800, Jason Ekstrand wrote:
> > Do we really want to wait until a subpass that uses the given view
> > or do we just want to clear all of the relevant slices in the first
> > subpass that touches it? I haven't really thought about this much.
> >
> > On Fri, Jan 5, 2018 at 8:38 AM, Iago Toral Quiroga <itoral@igalia.c
> > 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.
> > >
> > >
> > >
> > > Fixes work-in-progress CTS tests:
> > >
> > > dEQP-VK.multiview.readback_implicit_clear.*
> > >
> > > ---
> > >
> > > src/intel/vulkan/anv_blorp.c | 56
> > > ++++++++++++++++++++++++++++++++++++++++----
> > >
> > > 1 file changed, 52 insertions(+), 4 deletions(-)
> > >
> > >
> > >
> > > diff --git a/src/intel/vulkan/anv_blorp.c
> > > b/src/intel/vulkan/anv_blorp.c
> > >
> > > index 18fa4a4ae5..0dac20ea80 100644
> > >
> > > --- a/src/intel/vulkan/anv_blorp.c
> > >
> > > +++ b/src/intel/vulkan/anv_blorp.c
> > >
> > > @@ -1110,6 +1110,39 @@ enum subpass_stage {
> > >
> > > SUBPASS_STAGE_RESOLVE,
> > >
> > > };
> > >
> > >
> > >
> > > +/**
> > >
> > > + * Goes through all the previous subpasses to the current
> > > subpass and collects
> > >
> > > + * information about the layers (views) of the given attachment
> > > that have
> > >
> > > + * already been cleared by previous subpasses. The function
> > > return the layers
> > >
> > > + * (views), if any, that still need to be cleared when current
> > > subpass starts.
> > >
> > > + */
> > >
> > > +static uint32_t
> > >
> > > +subpass_views_with_pending_clear(const struct anv_cmd_buffer
> > > *cmd_buffer,
> > >
> > > + const uint32_t att_idx)
> > >
> > > +{
> > >
> > > + const struct anv_cmd_state *cmd_state = &cmd_buffer->state;
> > >
> > > + const struct anv_render_pass *pass = cmd_state->pass;
> > >
> > > + const struct anv_subpass *subpass = cmd_state->subpass;
> > >
> > > +
> > >
> > > + if (!subpass->view_mask)
> > >
> > > + return 0;
> > >
> > > +
> > >
> > > + uint32_t cleared_views = 0;
> > >
> > > + uint32_t subpass_idx = 0;
> > >
> > > + const struct anv_subpass *s = &pass->subpasses[subpass_idx];
> > >
> > > + while (s != subpass) {
> > >
> > > + for (uint32_t i = 0; i < s->color_count; i++) {
> > >
> > > + uint32_t idx = s->color_attachments[i].attachment;
> > >
> > > + if (idx != att_idx)
> > >
> > > + continue;
> > >
> > > + cleared_views |= s->view_mask;
> > >
> > > + }
> > >
> > > + s = &pass->subpasses[++subpass_idx];
> > >
> > > + }
> > >
> > > +
> > >
> > > + return subpass->view_mask & (~cleared_views);
> > >
> > > +}
> > >
> > > +
> > >
> > > static bool
> > >
> > > subpass_needs_clear(const struct anv_cmd_buffer *cmd_buffer)
> > >
> > > {
> > >
> > > @@ -1142,7 +1175,6 @@ anv_cmd_buffer_clear_subpass(struct
> > > anv_cmd_buffer *cmd_buffer)
> > >
> > > const struct anv_cmd_state *cmd_state = &cmd_buffer->state;
> > >
> > > const VkRect2D render_area = cmd_buffer->state.render_area;
> > >
> > >
> > >
> > > -
> > >
> > > if (!subpass_needs_clear(cmd_buffer))
> > >
> > > return;
> > >
> > >
> > >
> > > @@ -1205,7 +1237,9 @@ anv_cmd_buffer_clear_subpass(struct
> > > anv_cmd_buffer *cmd_buffer)
> > >
> > > assert(image->n_planes == 1);
> > >
> > > if (cmd_state->subpass->view_mask) {
> > >
> > > uint32_t view_idx;
> > >
> > > - for_each_bit(view_idx, cmd_state->subpass-
> > > >view_mask) {
> > >
> > > + uint32_t pending_clear_mask =
> > >
> > > + subpass_views_with_pending_clear(cmd_buffer, a);
> > >
> > > + for_each_bit(view_idx, pending_clear_mask) {
> > >
> > > blorp_fast_clear(&batch, &surf, iview-
> > > >planes[0].isl.format,
> > >
> > > iview->planes[0].isl.base_level,
> > >
> > > view_idx, 1,
> > >
> > > @@ -1227,7 +1261,9 @@ anv_cmd_buffer_clear_subpass(struct
> > > anv_cmd_buffer *cmd_buffer)
> > >
> > > assert(image->n_planes == 1);
> > >
> > > if (cmd_state->subpass->view_mask) {
> > >
> > > uint32_t view_idx;
> > >
> > > - for_each_bit(view_idx, cmd_state->subpass-
> > > >view_mask) {
> > >
> > > + uint32_t pending_clear_mask =
> > >
> > > + subpass_views_with_pending_clear(cmd_buffer, a);
> > >
> > > + for_each_bit(view_idx, pending_clear_mask) {
> > >
> > > blorp_clear(&batch, &surf, iview-
> > > >planes[0].isl.format,
> > >
> > > anv_swizzle_for_render(iview-
> > > >planes[0].isl.swizzle),
> > >
> > > iview->planes[0].isl.base_level,
> > >
> > > @@ -1249,7 +1285,19 @@ anv_cmd_buffer_clear_subpass(struct
> > > anv_cmd_buffer *cmd_buffer)
> > >
> > > }
> > >
> > > }
> > >
> > >
> > >
> > > - att_state->pending_clear_aspects = 0;
> > >
> > > + if (!cmd_state->subpass->view_mask) {
> > >
> > > + att_state->pending_clear_aspects = 0;
> > >
> > > + } else {
> > >
> > > + /* If multiview is enabled, then we are only done
> > > clearing when the
> > >
> > > + * last subpass that uses this attachment has been
> > > processed,
> > >
> > > + */
> > >
> > > + 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;
> > >
> > > + }
> > >
> > > }
> > >
> > >
> > >
> > > const uint32_t ds = cmd_state->subpass-
> > > >depth_stencil_attachment.attachment;
> > >
> > >
> > > --
> > >
> > > 2.11.0
> > >
> > >
> > > _______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
