[Mesa-dev] [PATCH 19/29] anv/cmd_buffer: Move the rest of clear_subpass into begin_subpass

Jason Ekstrand jason at jlekstrand.net
Fri Feb 9 01:20:11 UTC 2018


On Fri, Jan 12, 2018 at 2:45 PM, Nanley Chery <nanleychery at gmail.com> wrote:

> On Mon, Nov 27, 2017 at 07:06:09PM -0800, Jason Ekstrand wrote:
> > ---
> >  src/intel/vulkan/anv_blorp.c       | 243 ++++++++++++++++--------------
> -------
> >  src/intel/vulkan/anv_private.h     |  17 ++-
> >  src/intel/vulkan/genX_cmd_buffer.c |  68 ++++++++++-
> >  3 files changed, 188 insertions(+), 140 deletions(-)
> >
> > diff --git a/src/intel/vulkan/anv_blorp.c b/src/intel/vulkan/anv_blorp.c
> > index 7401234..45d7b12 100644
> > --- a/src/intel/vulkan/anv_blorp.c
> > +++ b/src/intel/vulkan/anv_blorp.c
> > @@ -1132,143 +1132,6 @@ enum subpass_stage {
> >     SUBPASS_STAGE_RESOLVE,
> >  };
> >
> > -static bool
> > -subpass_needs_clear(const struct anv_cmd_buffer *cmd_buffer)
> > -{
> > -   const struct anv_cmd_state *cmd_state = &cmd_buffer->state;
> > -   uint32_t ds = cmd_state->subpass->depth_
> stencil_attachment.attachment;
> > -
> > -   if (ds != VK_ATTACHMENT_UNUSED) {
> > -      assert(ds < cmd_state->pass->attachment_count);
> > -      if (cmd_state->attachments[ds].pending_clear_aspects)
> > -         return true;
> > -   }
> > -
> > -   return false;
> > -}
> > -
> > -void
> > -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;
> > -
> > -   /* Because this gets called within a render pass, we tell blorp not
> to
> > -    * trash our depth and stencil buffers.
> > -    */
> > -   struct blorp_batch batch;
> > -   blorp_batch_init(&cmd_buffer->device->blorp, &batch, cmd_buffer,
> > -                    BLORP_BATCH_NO_EMIT_DEPTH_STENCIL);
> > -
> > -   VkClearRect clear_rect = {
> > -      .rect = cmd_buffer->state.render_area,
> > -      .baseArrayLayer = 0,
> > -      .layerCount = cmd_buffer->state.framebuffer->layers,
> > -   };
> > -
> > -   struct anv_framebuffer *fb = cmd_buffer->state.framebuffer;
> > -
> > -   const uint32_t ds = cmd_state->subpass->depth_
> stencil_attachment.attachment;
> > -   assert(ds == VK_ATTACHMENT_UNUSED || ds <
> cmd_state->pass->attachment_count);
> > -
> > -   if (ds != VK_ATTACHMENT_UNUSED &&
> > -       cmd_state->attachments[ds].pending_clear_aspects) {
> > -
> > -      VkClearAttachment clear_att = {
> > -         .aspectMask = cmd_state->attachments[ds].
> pending_clear_aspects,
> > -         .clearValue = cmd_state->attachments[ds].clear_value,
> > -      };
> > -
> > -
> > -      const uint8_t gen = cmd_buffer->device->info.gen;
> > -      bool clear_with_hiz = gen >= 8 && cmd_state->attachments[ds].aux_usage
> ==
> > -                            ISL_AUX_USAGE_HIZ;
> > -      const struct anv_image_view *iview = fb->attachments[ds];
> > -
> > -      if (clear_with_hiz) {
> > -         const bool clear_depth = clear_att.aspectMask &
> > -                                  VK_IMAGE_ASPECT_DEPTH_BIT;
> > -         const bool clear_stencil = clear_att.aspectMask &
> > -                                    VK_IMAGE_ASPECT_STENCIL_BIT;
> > -
> > -         /* Check against restrictions for depth buffer clearing. A
> great GPU
> > -          * performance benefit isn't expected when using the HZ
> sequence for
> > -          * stencil-only clears. Therefore, we don't emit a HZ op
> sequence for
> > -          * a stencil clear in addition to using the BLORP-fallback for
> depth.
> > -          */
> > -         if (clear_depth) {
> > -            if (!blorp_can_hiz_clear_depth(gen,
> iview->planes[0].isl.format,
> > -                                           iview->image->samples,
> > -                                           render_area.offset.x,
> > -                                           render_area.offset.y,
> > -                                           render_area.offset.x +
> > -                                           render_area.extent.width,
> > -                                           render_area.offset.y +
> > -                                           render_area.extent.height)) {
> > -               clear_with_hiz = false;
> > -            } else if (clear_att.clearValue.depthStencil.depth !=
> > -                       ANV_HZ_FC_VAL) {
> > -               /* Don't enable fast depth clears for any color not
> equal to
> > -                * ANV_HZ_FC_VAL.
> > -                */
> > -               clear_with_hiz = false;
> > -            } else if (gen == 8 &&
> > -                       anv_can_sample_with_hiz(&cmd_
> buffer->device->info,
> > -                                               iview->image)) {
> > -               /* Only gen9+ supports returning ANV_HZ_FC_VAL when
> sampling a
> > -                * fast-cleared portion of a HiZ buffer. Testing has
> revealed
> > -                * that Gen8 only supports returning 0.0f. Gens prior to
> gen8 do
> > -                * not support this feature at all.
> > -                */
> > -               clear_with_hiz = false;
> > -            }
> > -         }
> > -
> > -         if (clear_with_hiz) {
> > -            blorp_gen8_hiz_clear_attachments(&batch,
> iview->image->samples,
> > -                                             render_area.offset.x,
> > -                                             render_area.offset.y,
> > -                                             render_area.offset.x +
> > -                                             render_area.extent.width,
> > -                                             render_area.offset.y +
> > -                                             render_area.extent.height,
> > -                                             clear_depth, clear_stencil,
> > -                                             clear_att.clearValue.
> > -                                                depthStencil.stencil);
> > -
> > -            /* From the SKL PRM, Depth Buffer Clear:
> > -             *
> > -             * Depth Buffer Clear Workaround
> > -             * Depth buffer clear pass using any of the methods
> (WM_STATE,
> > -             * 3DSTATE_WM or 3DSTATE_WM_HZ_OP) must be followed by a
> > -             * PIPE_CONTROL command with DEPTH_STALL bit and Depth
> FLUSH bits
> > -             * “set” before starting to render. DepthStall and
> DepthFlush are
> > -             * not needed between consecutive depth clear passes nor is
> it
> > -             * required if the depth-clear pass was done with
> “full_surf_clear”
> > -             * bit set in the 3DSTATE_WM_HZ_OP.
> > -             */
> > -            if (clear_depth) {
> > -               cmd_buffer->state.pending_pipe_bits |=
> > -                  ANV_PIPE_DEPTH_CACHE_FLUSH_BIT |
> ANV_PIPE_DEPTH_STALL_BIT;
> > -            }
> > -         }
> > -      }
> > -
> > -      if (!clear_with_hiz) {
> > -         clear_depth_stencil_attachment(cmd_buffer, &batch,
> > -                                        &clear_att, 1, &clear_rect);
> > -      }
> > -
> > -      cmd_state->attachments[ds].pending_clear_aspects = 0;
> > -   }
> > -
> > -   blorp_batch_finish(&batch);
> > -}
> > -
> >  static void
> >  resolve_surface(struct blorp_batch *batch,
> >                  struct blorp_surf *src_surf,
> > @@ -1568,6 +1431,53 @@ anv_image_clear_color(struct anv_cmd_buffer
> *cmd_buffer,
> >  }
> >
> >  void
> > +anv_image_clear_depth_stencil(struct anv_cmd_buffer *cmd_buffer,
> > +                              const struct anv_image *image,
> > +                              VkImageAspectFlags aspects,
> > +                              enum isl_aux_usage depth_aux_usage,
> > +                              uint32_t level,
> > +                              uint32_t base_layer, uint32_t layer_count,
> > +                              VkRect2D area,
> > +                              float depth_value, uint8_t stencil_value)
> > +{
> > +   assert(aspects & (VK_IMAGE_ASPECT_DEPTH_BIT |
> > +                     VK_IMAGE_ASPECT_STENCIL_BIT));
> > +
> > +   struct blorp_batch batch;
> > +   blorp_batch_init(&cmd_buffer->device->blorp, &batch, cmd_buffer, 0);
> > +
> > +   struct blorp_surf depth, stencil;
> > +   if (image->aspects & VK_IMAGE_ASPECT_DEPTH_BIT) {
>
> Why use image->aspects instead of aspects?
>
> > +      get_blorp_surf_for_anv_image(cmd_buffer->device,
> > +                                   image, VK_IMAGE_ASPECT_DEPTH_BIT,
> > +                                   depth_aux_usage, &depth);
> > +      depth.clear_color.f32[0] = ANV_HZ_FC_VAL;
> > +   } else {
> > +      memset(&stencil, 0, sizeof(stencil));
>
> The depth blorp_surf should be memset here.
>

Fixed.  See reply to Topi


> > +   }
> > +
> > +   if (image->aspects & VK_IMAGE_ASPECT_STENCIL_BIT) {
> > +      get_blorp_surf_for_anv_image(cmd_buffer->device,
> > +                                   image, VK_IMAGE_ASPECT_STENCIL_BIT,
> > +                                   ISL_AUX_USAGE_NONE, &stencil);
> > +   } else {
> > +      memset(&stencil, 0, sizeof(stencil));
> > +   }
> > +
> > +   blorp_clear_depth_stencil(&batch, &depth, &stencil,
> > +                             level, base_layer, layer_count,
> > +                             area.offset.x, area.offset.y,
> > +                             area.offset.x + area.extent.width,
> > +                             area.offset.y + area.extent.height,
> > +                             aspects & VK_IMAGE_ASPECT_DEPTH_BIT,
> > +                             depth_value,
> > +                             (aspects & VK_IMAGE_ASPECT_STENCIL_BIT) ?
> 0xff : 0,
> > +                             stencil_value);
> > +
> > +   blorp_batch_finish(&batch);
> > +}
> > +
> > +void
> >  anv_image_hiz_op(struct anv_cmd_buffer *cmd_buffer,
> >                   const struct anv_image *image,
> >                   VkImageAspectFlagBits aspect, uint32_t level,
> > @@ -1595,6 +1505,65 @@ anv_image_hiz_op(struct anv_cmd_buffer
> *cmd_buffer,
> >  }
> >
> >  void
> > +anv_image_hiz_clear(struct anv_cmd_buffer *cmd_buffer,
> > +                    const struct anv_image *image,
> > +                    VkImageAspectFlags aspects,
> > +                    uint32_t level,
> > +                    uint32_t base_layer, uint32_t layer_count,
> > +                    VkRect2D area, uint8_t stencil_value)
> > +{
> > +   assert(aspects & (VK_IMAGE_ASPECT_DEPTH_BIT |
> > +                     VK_IMAGE_ASPECT_STENCIL_BIT));
> > +   assert(base_layer + layer_count <=
> > +          anv_image_aux_layers(image, VK_IMAGE_ASPECT_DEPTH_BIT, 0));
> > +
> > +   struct blorp_batch batch;
> > +   blorp_batch_init(&cmd_buffer->device->blorp, &batch, cmd_buffer, 0);
> > +
> > +   struct blorp_surf depth;
> > +   get_blorp_surf_for_anv_image(cmd_buffer->device,
> > +                                image, VK_IMAGE_ASPECT_DEPTH_BIT,
> > +                                ISL_AUX_USAGE_HIZ, &depth);
>
> Shouldn't we check if aspects contains depth before calling this?
>

This is a HiZ clear.  We had better have depth.  I'll adjust the assert at
the top.


> > +   depth.clear_color.f32[0] = ANV_HZ_FC_VAL;
> > +
> > +   struct blorp_surf stencil;
> > +   if (image->aspects & VK_IMAGE_ASPECT_STENCIL_BIT) {
>
> Why use image->aspects instead of aspects?
>

No good reason.  I'll change to aspects.


> > +      get_blorp_surf_for_anv_image(cmd_buffer->device,
> > +                                   image, VK_IMAGE_ASPECT_STENCIL_BIT,
> > +                                   ISL_AUX_USAGE_NONE, &stencil);
> > +   } else {
> > +      memset(&stencil, 0, sizeof(stencil));
> > +   }
> > +
> > +   blorp_hiz_clear_depth_stencil(&batch, &depth, &stencil,
> > +                                 level, base_layer, layer_count,
> > +                                 area.offset.x, area.offset.y,
> > +                                 area.offset.x + area.extent.width,
> > +                                 area.offset.y + area.extent.height,
> > +                                 aspects & VK_IMAGE_ASPECT_DEPTH_BIT,
> > +                                 ANV_HZ_FC_VAL,
> > +                                 aspects & VK_IMAGE_ASPECT_STENCIL_BIT,
> > +                                 stencil_value);
> > +
> > +   blorp_batch_finish(&batch);
> > +
> > +   /* From the SKL PRM, Depth Buffer Clear:
> > +    *
> > +    * Depth Buffer Clear Workaround
> > +    * Depth buffer clear pass using any of the methods (WM_STATE,
> 3DSTATE_WM
> > +    * or 3DSTATE_WM_HZ_OP) must be followed by a PIPE_CONTROL command
> with
> > +    * DEPTH_STALL bit and Depth FLUSH bits “set” before starting to
> render.
> > +    * DepthStall and DepthFlush are not needed between consecutive
> depth clear
> > +    * passes nor is it required if the depth-clear pass was done with
> > +    * “full_surf_clear” bit set in the 3DSTATE_WM_HZ_OP.
> > +    */
> > +   if (aspects & VK_IMAGE_ASPECT_DEPTH_BIT) {
> > +      cmd_buffer->state.pending_pipe_bits |=
> > +         ANV_PIPE_DEPTH_CACHE_FLUSH_BIT | ANV_PIPE_DEPTH_STALL_BIT;
> > +   }
> > +}
> > +
> > +void
> >  anv_image_mcs_op(struct anv_cmd_buffer *cmd_buffer,
> >                   const struct anv_image *image,
> >                   VkImageAspectFlagBits aspect,
> > diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_
> private.h
> > index bc355bb..b881157 100644
> > --- a/src/intel/vulkan/anv_private.h
> > +++ b/src/intel/vulkan/anv_private.h
> > @@ -1875,7 +1875,6 @@ anv_cmd_buffer_push_constants(struct
> anv_cmd_buffer *cmd_buffer,
> >  struct anv_state
> >  anv_cmd_buffer_cs_push_constants(struct anv_cmd_buffer *cmd_buffer);
> >
> > -void anv_cmd_buffer_clear_subpass(struct anv_cmd_buffer *cmd_buffer);
> >  void anv_cmd_buffer_resolve_subpass(struct anv_cmd_buffer *cmd_buffer);
> >
> >  const struct anv_image_view *
> > @@ -2545,12 +2544,28 @@ anv_image_clear_color(struct anv_cmd_buffer
> *cmd_buffer,
> >                        uint32_t level, uint32_t base_layer, uint32_t
> layer_count,
> >                        VkRect2D area, union isl_color_value clear_color);
> >  void
> > +anv_image_clear_depth_stencil(struct anv_cmd_buffer *cmd_buffer,
> > +                              const struct anv_image *image,
> > +                              VkImageAspectFlags aspects,
> > +                              enum isl_aux_usage depth_aux_usage,
> > +                              uint32_t level,
> > +                              uint32_t base_layer, uint32_t layer_count,
> > +                              VkRect2D area,
> > +                              float depth_value, uint8_t stencil_value);
> > +void
> >  anv_image_hiz_op(struct anv_cmd_buffer *cmd_buffer,
> >                   const struct anv_image *image,
> >                   VkImageAspectFlagBits aspect, uint32_t level,
> >                   uint32_t base_layer, uint32_t layer_count,
> >                   enum isl_aux_op hiz_op);
> >  void
> > +anv_image_hiz_clear(struct anv_cmd_buffer *cmd_buffer,
> > +                    const struct anv_image *image,
> > +                    VkImageAspectFlags aspects,
> > +                    uint32_t level,
> > +                    uint32_t base_layer, uint32_t layer_count,
> > +                    VkRect2D area, uint8_t stencil_value);
> > +void
> >  anv_image_mcs_op(struct anv_cmd_buffer *cmd_buffer,
> >                   const struct anv_image *image,
> >                   VkImageAspectFlagBits aspect,
> > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> b/src/intel/vulkan/genX_cmd_buffer.c
> > index 265ae44..57685bd 100644
> > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > @@ -3216,9 +3216,73 @@ cmd_buffer_begin_subpass(struct anv_cmd_buffer
> *cmd_buffer,
> >        att_state->pending_clear_aspects = 0;
> >     }
> >
> > -   cmd_buffer_emit_depth_stencil(cmd_buffer);
> > +   if (subpass->depth_stencil_attachment.attachment !=
> VK_ATTACHMENT_UNUSED) {
> > +      const uint32_t a = subpass->depth_stencil_attachment.attachment;
> > +
> > +      assert(a < cmd_state->pass->attachment_count);
> > +      struct anv_attachment_state *att_state =
> &cmd_state->attachments[a];
> > +      struct anv_image_view *iview = fb->attachments[a];
> > +      const struct anv_image *image = iview->image;
> > +
> > +      assert(image->aspects & (VK_IMAGE_ASPECT_DEPTH_BIT |
> > +                               VK_IMAGE_ASPECT_STENCIL_BIT));
> > +
> > +      if (att_state->pending_clear_aspects) {
> > +         bool clear_with_hiz = att_state->aux_usage ==
> ISL_AUX_USAGE_HIZ;
> > +         if (clear_with_hiz &&
> > +             (att_state->pending_clear_aspects &
> VK_IMAGE_ASPECT_DEPTH_BIT)) {
> > +            if (!blorp_can_hiz_clear_depth(GEN_GEN,
> > +                                           iview->planes[0].isl.format,
> > +                                           iview->image->samples,
> > +                                           render_area.offset.x,
> > +                                           render_area.offset.y,
> > +                                           render_area.offset.x +
> > +                                           render_area.extent.width,
> > +                                           render_area.offset.y +
> > +                                           render_area.extent.height)) {
>
> If GEN_GEN < 8, the first assert within the above function will be
> triggered. The original code checks the gen when assigning
> clear_with_hiz.
>

I know I had myself convinced at one point that I could drop the GEN_GEN >=
8 but I don't remember now why.  I'll add it back in.  If nothing else,
it'll let the compiler dead-code some stuff on HSW.


> > +               clear_with_hiz = false;
> > +            } else if (att_state->clear_value.depthStencil.depth !=
> ANV_HZ_FC_VAL) {
> > +               clear_with_hiz = false;
> > +            } else if (GEN_GEN == 8 &&
> > +                       anv_can_sample_with_hiz(&cmd_
> buffer->device->info,
> > +                                               iview->image)) {
> > +               /* Only gen9+ supports returning ANV_HZ_FC_VAL when
> sampling a
> > +                * fast-cleared portion of a HiZ buffer. Testing has
> revealed
> > +                * that Gen8 only supports returning 0.0f. Gens prior to
> gen8
> > +                * do not support this feature at all.
> > +                */
> > +               clear_with_hiz = false;
> > +            }
> > +         }
> >
> > -   anv_cmd_buffer_clear_subpass(cmd_buffer);
> > +         if (clear_with_hiz) {
> > +            /* We currently only support HiZ for single-layer images */
>                                                                 ^
>                                                         "and single-level"
> ?
>
> > +            assert(iview->planes[0].isl.base_level == 0);
> > +            assert(iview->planes[0].isl.base_array_layer == 0);
> > +            assert(fb->layers == 1);
> > +
> > +            anv_image_hiz_clear(cmd_buffer, image,
> > +                                att_state->pending_clear_aspects,
> > +                                iview->planes[0].isl.base_level,
> > +                                iview->planes[0].isl.base_array_layer,
> > +                                fb->layers, render_area,
> > +                                att_state->clear_value.
> depthStencil.stencil);
> > +         } else {
> > +            anv_image_clear_depth_stencil(cmd_buffer, image,
> > +                                          att_state->pending_clear_
> aspects,
> > +                                          att_state->aux_usage,
> > +                                          iview->planes[0].isl.base_
> level,
> > +                                          iview->planes[0].isl.base_
> array_layer,
> > +                                          fb->layers, render_area,
> > +                                          att_state->clear_value.
> depthStencil.depth,
> > +                                          att_state->clear_value.
> depthStencil.stencil);
>
> Do we need to do something special for multiview? I need to spend
> some time getting familiar with that extension...
>

Yes.  The igalia people have a bunch of multiview clear fixes.  I asked
them to hold off and rebase on this rework.


> > +         }
> > +      }
> > +
> > +      att_state->pending_clear_aspects = 0;
> > +   }
> > +
> > +   cmd_buffer_emit_depth_stencil(cmd_buffer);
> >  }
> >
> >  static void
> > --
> > 2.5.0.400.gff86faf
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180208/c0c76ef0/attachment-0001.html>


More information about the mesa-dev mailing list