[Mesa-dev] [PATCH v3 05/16] anv/cmd_buffer: Restrict fast clears in the GENERAL layout
Nanley Chery
nanleychery at gmail.com
Mon Jul 10 21:53:18 UTC 2017
On Mon, Jul 10, 2017 at 09:28:05AM -0700, Jason Ekstrand wrote:
> On Wed, Jun 28, 2017 at 2:14 PM, Nanley Chery <nanleychery at gmail.com> wrote:
>
> > v2: Remove ::first_subpass_layout assertion (Jason Ekstrand).
> > v3: Allow some fast clears in the GENERAL layout.
> >
> > Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
> > ---
> > src/intel/vulkan/anv_pass.c | 22 ++++++++++++++++++++++
> > src/intel/vulkan/anv_private.h | 2 ++
> > src/intel/vulkan/genX_cmd_buffer.c | 17 ++++++++++++++++-
> > 3 files changed, 40 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/intel/vulkan/anv_pass.c b/src/intel/vulkan/anv_pass.c
> > index 1b30c1409d..ab0733fc10 100644
> > --- a/src/intel/vulkan/anv_pass.c
> > +++ b/src/intel/vulkan/anv_pass.c
> > @@ -34,6 +34,16 @@ num_subpass_attachments(const VkSubpassDescription
> > *desc)
> > (desc->pDepthStencilAttachment != NULL);
> > }
> >
> > +static void
> > +init_first_subpass_layout(struct anv_render_pass_attachment * const att,
> > + const VkAttachmentReference att_ref)
> > +{
> > + if (att->first_subpass_layout == VK_IMAGE_LAYOUT_UNDEFINED) {
> > + att->first_subpass_layout = att_ref.layout;
> > + assert(att->first_subpass_layout != VK_IMAGE_LAYOUT_UNDEFINED);
> > + }
> > +}
> > +
> > VkResult anv_CreateRenderPass(
> > VkDevice _device,
> > const VkRenderPassCreateInfo* pCreateInfo,
> > @@ -91,6 +101,7 @@ VkResult anv_CreateRenderPass(
> > att->stencil_load_op = pCreateInfo->pAttachments[i].stencilLoadOp;
> > att->initial_layout = pCreateInfo->pAttachments[i].initialLayout;
> > att->final_layout = pCreateInfo->pAttachments[i].finalLayout;
> > + att->first_subpass_layout = VK_IMAGE_LAYOUT_UNDEFINED;
> > att->subpass_usage = subpass_usages;
> > subpass_usages += pass->subpass_count;
> > }
> > @@ -119,6 +130,8 @@ VkResult anv_CreateRenderPass(
> > pass->attachments[a].subpass_usage[i] |=
> > ANV_SUBPASS_USAGE_INPUT;
> > pass->attachments[a].last_subpass_idx = i;
> >
> > + init_first_subpass_layout(&pass->attachments[a],
> > + desc->pInputAttachments[j]);
> > if (desc->pDepthStencilAttachment &&
> > a == desc->pDepthStencilAttachment->attachment)
> > subpass->has_ds_self_dep = true;
> > @@ -138,6 +151,9 @@ VkResult anv_CreateRenderPass(
> > pass->attachments[a].usage |= VK_IMAGE_USAGE_COLOR_
> > ATTACHMENT_BIT;
> > pass->attachments[a].subpass_usage[i] |=
> > ANV_SUBPASS_USAGE_DRAW;
> > pass->attachments[a].last_subpass_idx = i;
> > +
> > + init_first_subpass_layout(&pass->attachments[a],
> > + desc->pColorAttachments[j]);
> > }
> > }
> > }
> > @@ -162,6 +178,9 @@ VkResult anv_CreateRenderPass(
> > pass->attachments[a].subpass_usage[i] |=
> > ANV_SUBPASS_USAGE_RESOLVE_DST;
> > pass->attachments[a].last_subpass_idx = i;
> > +
> > + init_first_subpass_layout(&pass->attachments[a],
> > + desc->pResolveAttachments[j]);
> > }
> > }
> > }
> > @@ -176,6 +195,9 @@ VkResult anv_CreateRenderPass(
> > VK_IMAGE_USAGE_DEPTH_STENCIL_ATTACHMENT_BIT;
> > pass->attachments[a].subpass_usage[i] |=
> > ANV_SUBPASS_USAGE_DRAW;
> > pass->attachments[a].last_subpass_idx = i;
> > +
> > + init_first_subpass_layout(&pass->attachments[a],
> > + *desc->pDepthStencilAttachment);
> > }
> > } else {
> > subpass->depth_stencil_attachment.attachment =
> > VK_ATTACHMENT_UNUSED;
> > diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_
> > private.h
> > index a95188ac30..c5a2ba0888 100644
> > --- a/src/intel/vulkan/anv_private.h
> > +++ b/src/intel/vulkan/anv_private.h
> > @@ -1518,6 +1518,7 @@ struct anv_attachment_state {
> > bool fast_clear;
> > VkClearValue clear_value;
> > bool clear_color_is_zero_one;
> > + bool clear_color_is_zero;
> > };
> >
> > /** State required while building cmd buffer */
> > @@ -2336,6 +2337,7 @@ struct anv_render_pass_attachment {
> > VkAttachmentLoadOp stencil_load_op;
> > VkImageLayout initial_layout;
> > VkImageLayout final_layout;
> > + VkImageLayout first_subpass_layout;
> >
> > /* An array, indexed by subpass id, of how the attachment will be
> > used. */
> > enum anv_subpass_usage * subpass_usage;
> > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> > b/src/intel/vulkan/genX_cmd_buffer.c
> > index 15927d32ad..253e68cd1f 100644
> > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > @@ -253,7 +253,12 @@ color_attachment_compute_aux_usage(struct anv_device
> > * device,
> >
> > assert(iview->image->aux_surface.isl.usage & ISL_SURF_USAGE_CCS_BIT);
> >
> > - att_state->clear_color_is_zero_one =
> > + att_state->clear_color_is_zero =
> > + att_state->clear_value.color.uint32[0] == 0 &&
> > + att_state->clear_value.color.uint32[1] == 0 &&
> > + att_state->clear_value.color.uint32[2] == 0 &&
> > + att_state->clear_value.color.uint32[3] == 0;
> > + att_state->clear_color_is_zero_one = att_state->clear_color_is_zero ||
> >
>
> I doubt the || actually gains us anything here. function inlining, CSE,
> and the processor's normal pipelineing will most likely make all of the
> theoretical performance gain go away.
>
>
I'll get rid of it.
> > color_is_zero_one(att_state->clear_value.color, iview->isl.format);
> >
> > if (att_state->pending_clear_aspects == VK_IMAGE_ASPECT_COLOR_BIT) {
> > @@ -299,6 +304,16 @@ color_attachment_compute_aux_usage(struct anv_device
> > * device,
> > }
> > }
> >
> > + /* We only allow fast clears in the GENERAL layout if the auxiliary
> > + * buffer is always enabled and the fast-clear value is all 0's. See
> > + * add_fast_clear_state_buffer() for more information.
> > + */
> > + if (cmd_state->pass->attachments[att].first_subpass_layout ==
> > + VK_IMAGE_LAYOUT_GENERAL && (!att_state->clear_color_is_zero ||
> > + iview->image->aux_usage == ISL_AUX_USAGE_NONE)) {
> >
>
> Mind line-breaking this differently?
>
> + if (cmd_state->pass->attachments[att].first_subpass_layout ==
> + VK_IMAGE_LAYOUT_GENERAL &&
> + (!att_state->clear_color_is_zero ||
> + iview->image->aux_usage == ISL_AUX_USAGE_NONE)) {
>
> That way it's a bit clearer where the boolean logic is.
>
> --Jason
>
Sure.
Thanks,
Nanley
> + att_state->fast_clear = false;
> > + }
> > +
> > if (att_state->fast_clear) {
> > memcpy(fast_clear_color->u32, att_state->clear_value.color.
> > uint32,
> > sizeof(fast_clear_color->u32));
> > --
> > 2.13.1
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
More information about the mesa-dev
mailing list