On Thu, Apr 06, 2017 at 11:48:04AM -0700, Jason Ekstrand wrote:
> On Thu, Mar 30, 2017 at 2:06 PM, Nanley Chery <nanleych...@gmail.com> wrote:
>
> > On Tue, Mar 14, 2017 at 07:55:53AM -0700, Jason Ekstrand wrote:
> > > Instead of figuring it all out ourselves, just use the information given
> > > to us by the client.
> > > ---
> > > src/intel/vulkan/anv_blorp.c | 88 ++++--------------------------
> > --------
> > > src/intel/vulkan/genX_cmd_buffer.c | 10 +++++
> > > 2 files changed, 18 insertions(+), 80 deletions(-)
> > >
> > > diff --git a/src/intel/vulkan/anv_blorp.c b/src/intel/vulkan/anv_blorp.c
> > > index 8de339c..41966d6 100644
> > > --- a/src/intel/vulkan/anv_blorp.c
> > > +++ b/src/intel/vulkan/anv_blorp.c
> > > @@ -1070,80 +1070,6 @@ enum subpass_stage {
> > > };
> > >
> > > static bool
> > > -attachment_needs_flush(struct anv_cmd_buffer *cmd_buffer,
> > > - struct anv_render_pass_attachment *att,
> > > - enum subpass_stage stage)
> > > -{
> > > - struct anv_render_pass *pass = cmd_buffer->state.pass;
> > > - const uint32_t subpass_idx = anv_get_subpass_id(&cmd_buffer->state);
> > > -
> > > - /* We handle this subpass specially based on the current stage */
> > > - enum anv_subpass_usage usage = att->subpass_usage[subpass_idx];
> > > - switch (stage) {
> > > - case SUBPASS_STAGE_LOAD:
> > > - if (usage & (ANV_SUBPASS_USAGE_INPUT |
> > ANV_SUBPASS_USAGE_RESOLVE_SRC))
> > > - return true;
> > > - break;
> > > -
> > > - case SUBPASS_STAGE_DRAW:
> > > - if (usage & ANV_SUBPASS_USAGE_RESOLVE_SRC)
> > > - return true;
> > > - break;
> > > -
> > > - default:
> > > - break;
> > > - }
> > > -
> > > - for (uint32_t s = subpass_idx + 1; s < pass->subpass_count; s++) {
> > > - usage = att->subpass_usage[s];
> > > -
> > > - /* If this attachment is going to be used as an input in this or
> > any
> > > - * future subpass, then we need to flush its cache and invalidate
> > the
> > > - * texture cache.
> > > - */
> > > - if (att->subpass_usage[s] & ANV_SUBPASS_USAGE_INPUT)
> > > - return true;
> > > -
> > > - if (usage & (ANV_SUBPASS_USAGE_DRAW |
> > ANV_SUBPASS_USAGE_RESOLVE_DST)) {
> > > - /* We found another subpass that draws to this attachment.
> > We'll
> > > - * wait to resolve until then.
> > > - */
> > > - return false;
> > > - }
> > > - }
> > > -
> > > - return false;
> > > -}
> > > -
> > > -static void
> > > -anv_cmd_buffer_flush_attachments(struct anv_cmd_buffer *cmd_buffer,
> > > - enum subpass_stage stage)
> > > -{
> > > - struct anv_subpass *subpass = cmd_buffer->state.subpass;
> > > - struct anv_render_pass *pass = cmd_buffer->state.pass;
> > > -
> > > - for (uint32_t i = 0; i < subpass->color_count; ++i) {
> > > - uint32_t att = subpass->color_attachments[i].attachment;
> > > - assert(att < pass->attachment_count);
> > > - if (attachment_needs_flush(cmd_buffer, &pass->attachments[att],
> > stage)) {
> > > - cmd_buffer->state.pending_pipe_bits |=
> > > - ANV_PIPE_TEXTURE_CACHE_INVALIDATE_BIT |
> > > - ANV_PIPE_RENDER_TARGET_CACHE_FLUSH_BIT;
> > > - }
> > > - }
> > > -
> > > - if (subpass->depth_stencil_attachment.attachment !=
> > VK_ATTACHMENT_UNUSED) {
> > > - uint32_t att = subpass->depth_stencil_attachment.attachment;
> > > - assert(att < pass->attachment_count);
> > > - if (attachment_needs_flush(cmd_buffer, &pass->attachments[att],
> > stage)) {
> > > - cmd_buffer->state.pending_pipe_bits |=
> > > - ANV_PIPE_TEXTURE_CACHE_INVALIDATE_BIT |
> > > - ANV_PIPE_DEPTH_CACHE_FLUSH_BIT;
> > > - }
> > > - }
> > > -}
> > > -
> > > -static bool
> > > subpass_needs_clear(const struct anv_cmd_buffer *cmd_buffer)
> > > {
> > > const struct anv_cmd_state *cmd_state = &cmd_buffer->state;
> > > @@ -1327,8 +1253,6 @@ anv_cmd_buffer_clear_subpass(struct
> > anv_cmd_buffer *cmd_buffer)
> > > }
> > >
> > > blorp_batch_finish(&batch);
> > > -
> > > - anv_cmd_buffer_flush_attachments(cmd_buffer, SUBPASS_STAGE_LOAD);
> > > }
> > >
> > > static void
> > > @@ -1554,9 +1478,15 @@ anv_cmd_buffer_resolve_subpass(struct
> > anv_cmd_buffer *cmd_buffer)
> > > subpass->color_attachments[i].attachment);
> > > }
> > >
> > > - anv_cmd_buffer_flush_attachments(cmd_buffer, SUBPASS_STAGE_DRAW);
> > > -
> > > if (subpass->has_resolve) {
> > > + /* We are about to do some MSAA resolves. We need to flush so
> > that the
> > > + * result of writes to the MSAA color attachments show up in the
> > sampler
> > > + * when we blit to the single-sampled resolve target.
> > > + */
> > > + cmd_buffer->state.pending_pipe_bits |=
> > > + ANV_PIPE_TEXTURE_CACHE_INVALIDATE_BIT |
> > > + ANV_PIPE_RENDER_TARGET_CACHE_FLUSH_BIT;
> > > +
> > > for (uint32_t i = 0; i < subpass->color_count; ++i) {
> > > uint32_t src_att = subpass->color_attachments[i].attachment;
> > > uint32_t dst_att = subpass->resolve_attachments[i].attachment;
> > > @@ -1594,8 +1524,6 @@ anv_cmd_buffer_resolve_subpass(struct
> > anv_cmd_buffer *cmd_buffer)
> > >
> > > ccs_resolve_attachment(cmd_buffer, &batch, dst_att);
> > > }
> > > -
> > > - anv_cmd_buffer_flush_attachments(cmd_buffer,
> > SUBPASS_STAGE_RESOLVE);
> > > }
> > >
> > > blorp_batch_finish(&batch);
> > > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> > b/src/intel/vulkan/genX_cmd_buffer.c
> > > index acb59d5..02dff44 100644
> > > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > > @@ -2456,6 +2456,9 @@ void genX(CmdBeginRenderPass)(
> > > genX(flush_pipeline_select_3d)(cmd_buffer);
> > >
> > > genX(cmd_buffer_set_subpass)(cmd_buffer, pass->subpasses);
> > > +
> > > + cmd_buffer->state.pending_pipe_bits |=
> > > + cmd_buffer->state.pass->subpass_flushes[0];
> > > }
> > >
> > > void genX(CmdNextSubpass)(
> > > @@ -2473,6 +2476,10 @@ void genX(CmdNextSubpass)(
> > > cmd_buffer_subpass_transition_layouts(cmd_buffer, true);
> > >
> > > genX(cmd_buffer_set_subpass)(cmd_buffer, cmd_buffer->state.subpass
> > + 1);
> > > +
> > > + uint32_t subpass_id = anv_get_subpass_id(&cmd_buffer->state);
> > > + cmd_buffer->state.pending_pipe_bits |=
> > > + cmd_buffer->state.pass->subpass_flushes[subpass_id];
> >
> > Looks like we could move this hunk into genX(cmd_buffer_set_subpass)().
> >
>
> We could, yes, and I thought about doing so. I elected not to because the
> subpass flushes are really something that happens *between* subpasses so
> you have to have it Begin, Next, and End. If we rolled it into
> set_subpass, I think it would be more confusing what's going on.
>
>
Makes sense. This patch is
Reviewed-by: Nanley Chery <nanley.g.ch...@intel.com>
> > -Nanley
> >
> > > }
> > >
> > > void genX(CmdEndRenderPass)(
> > > @@ -2486,6 +2493,9 @@ void genX(CmdEndRenderPass)(
> > > */
> > > cmd_buffer_subpass_transition_layouts(cmd_buffer, true);
> > >
> > > + cmd_buffer->state.pending_pipe_bits |=
> > > + cmd_buffer->state.pass->subpass_flushes[cmd_buffer->
> > state.pass->subpass_count];
> > > +
> > > cmd_buffer->state.hiz_enabled = false;
> > >
> > > #ifndef NDEBUG
> > > --
> > > 2.5.0.400.gff86faf
> > >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
