On Thu, Nov 17, 2016 at 10:06:12PM -0800, Jason Ekstrand wrote:
> On Wed, Oct 19, 2016 at 10:47 AM, Nanley Chery <nanleych...@gmail.com>
> wrote:
>
> > The HZ sequence modifies less state than the blorp path and requires
> > less CPU time to generate the necessary packets.
> >
> > Signed-off-by: Nanley Chery <nanley.g.ch...@intel.com>
> > ---
> >
> > v2: Don't combine the depth alignment if statements
> >
> > src/intel/vulkan/gen8_cmd_buffer.c | 46 +++++++++++++++++++++++++++---
> > --------
> > 1 file changed, 33 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/intel/vulkan/gen8_cmd_buffer.c
> > b/src/intel/vulkan/gen8_cmd_buffer.c
> > index 204542e..d4410d4 100644
> > --- a/src/intel/vulkan/gen8_cmd_buffer.c
> > +++ b/src/intel/vulkan/gen8_cmd_buffer.c
> > @@ -350,15 +350,19 @@ genX(cmd_buffer_emit_hz_op)(struct anv_cmd_buffer
> > *cmd_buffer,
> > assert(cmd_state->render_area.offset.x == 0 &&
> > cmd_state->render_area.offset.y == 0);
> >
> > + bool depth_clear;
> > + bool stc_clear;
> >
>
> Mind calling this stencil_clear instead of the abbreviation. While stc is
> fairly obvious, it's not an abbreviation we usually use. Yes, it's used in
> the PRM in the docs for WM_HZ_OP, but this is the first time I'd seen it.
That sounds good. I actually wasn't very fond of using "stc" either.
> With that changed, all three are
>
> Reviewed-by: Jason Ekstrand <ja...@jlekstrand.net>
>
Thanks for the review.
> Sorry it took so long. :-/
It's okay.
>
> Feel free to ignore the comment below for now. I'm mostly just pointing it
> out. (Sorry if I've pointed it out before.)
>
> --Jason
>
>
> > +
> > /* This variable corresponds to the Pixel Dim column in the table
> > below */
> > struct isl_extent2d px_dim;
> >
>
> Pedanticism: I'd really rather we call this align_px or something because
> that's really what it is. Yes, it comes from the size of a HiZ block but
> the way we use it is as an alignment. We could split the difference and
> call it block_size_px or something.
>
>
align_px is a fine variable name. I don't mind us renaming it.
-Nanley
> > /* Validate that we can perform the HZ operation and that it's
> > necessary. */
> > switch (op) {
> > case BLORP_HIZ_OP_DEPTH_CLEAR:
> > - if (cmd_buffer->state.pass->attachments[ds].load_op !=
> > - VK_ATTACHMENT_LOAD_OP_CLEAR)
> > - return;
> > + stc_clear = VK_IMAGE_ASPECT_STENCIL_BIT &
> > + cmd_state->attachments[ds].pending_clear_aspects;
> > + depth_clear = VK_IMAGE_ASPECT_DEPTH_BIT &
> > + cmd_state->attachments[ds].pending_clear_aspects;
> >
> > /* Apply alignment restrictions. Despite the BDW PRM mentioning
> > this is
> > * only needed for a depth buffer surface type of D16_UNORM, testing
> > @@ -396,7 +400,7 @@ genX(cmd_buffer_emit_hz_op)(struct anv_cmd_buffer
> > *cmd_buffer,
> > px_dim = (struct isl_extent2d) { .w = 8, .h = 4};
> > #endif
> >
> > - if (!full_surface_op) {
> > + if (depth_clear && !full_surface_op) {
> > /* Fast depth clears clear an entire sample block at a time. As a
> > * result, the rectangle must be aligned to the pixel dimensions
> > of
> > * a sample block for a successful operation.
> > @@ -409,15 +413,25 @@ genX(cmd_buffer_emit_hz_op)(struct anv_cmd_buffer
> > *cmd_buffer,
> > */
> > if (cmd_state->render_area.offset.x % px_dim.w ||
> > cmd_state->render_area.offset.y % px_dim.h)
> > - return;
> > + depth_clear = false;
> > if (cmd_state->render_area.offset.x +
> > cmd_state->render_area.extent.width != iview->extent.width
> > &&
> > cmd_state->render_area.extent.width % px_dim.w)
> > - return;
> > + depth_clear = false;
> > if (cmd_state->render_area.offset.y +
> > cmd_state->render_area.extent.height !=
> > iview->extent.height &&
> > cmd_state->render_area.extent.height % px_dim.h)
> > + depth_clear = false;
> > + }
> > +
> > + if (!depth_clear) {
> > + if (stc_clear) {
> > + /* Stencil has no alignment requirements */
> > + px_dim = (struct isl_extent2d) { .w = 1, .h = 1};
> > + } else {
> > + /* Nothing to clear */
> > return;
> > + }
> >
> }
> > break;
> > case BLORP_HIZ_OP_DEPTH_RESOLVE:
> > @@ -448,10 +462,8 @@ genX(cmd_buffer_emit_hz_op)(struct anv_cmd_buffer
> > *cmd_buffer,
> > anv_batch_emit(&cmd_buffer->batch, GENX(3DSTATE_WM_HZ_OP), hzp) {
> > switch (op) {
> > case BLORP_HIZ_OP_DEPTH_CLEAR:
> > - hzp.StencilBufferClearEnable = VK_IMAGE_ASPECT_STENCIL_BIT &
> > - cmd_state->attachments[ds].
> > pending_clear_aspects;
> > - hzp.DepthBufferClearEnable = VK_IMAGE_ASPECT_DEPTH_BIT &
> > - cmd_state->attachments[ds].
> > pending_clear_aspects;
> > + hzp.StencilBufferClearEnable = stc_clear;
> > + hzp.DepthBufferClearEnable = depth_clear;
> > hzp.FullSurfaceDepthandStencilClear = full_surface_op;
> > hzp.StencilClearValue =
> > cmd_state->attachments[ds].clear_value.depthStencil.stencil
> > & 0xff;
> > @@ -503,16 +515,24 @@ genX(cmd_buffer_emit_hz_op)(struct anv_cmd_buffer
> > *cmd_buffer,
> >
> > anv_batch_emit(&cmd_buffer->batch, GENX(3DSTATE_WM_HZ_OP), hzp);
> >
> > + /* Perform clear specific flushing and state updates */
> > if (op == BLORP_HIZ_OP_DEPTH_CLEAR) {
> > - if (!full_surface_op) {
> > + if (depth_clear && !full_surface_op) {
> > anv_batch_emit(&cmd_buffer->batch, GENX(PIPE_CONTROL), pc) {
> > pc.DepthStallEnable = true;
> > pc.DepthCacheFlushEnable = true;
> > }
> > }
> >
> > - /* Mark aspects as cleared */
> > - cmd_state->attachments[ds].pending_clear_aspects = 0;
> > + /* Remove cleared aspects from the pending mask */
> > + if (stc_clear) {
> > + cmd_state->attachments[ds].pending_clear_aspects &=
> > + ~VK_IMAGE_ASPECT_STENCIL_BIT;
> > + }
> > + if (depth_clear) {
> > + cmd_state->attachments[ds].pending_clear_aspects &=
> > + ~VK_IMAGE_ASPECT_DEPTH_BIT;
> > + }
> > }
> > }
> >
> > --
> > 2.10.0
> >
> > _______________________________________________
> > 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
