On Mon, Oct 03, 2016 at 05:23:27PM -0700, Jason Ekstrand wrote:
> On Mon, Sep 26, 2016 at 5:10 PM, Nanley Chery <nanleych...@gmail.com> wrote:
>
> > Create a function that performs one of three HiZ operations -
> > depth/stencil clears, HiZ resolve, and depth resolves.
> >
> > Signed-off-by: Nanley Chery <nanley.g.ch...@intel.com>
> >
> > ---
> >
> > v2. Add documentation
> > Fix the alignment check
> > Don't minify clear rectangle (Jason)
> > Use blorp enums (Jason)
> > Enable depth stalls and flushes
> > Use full RT rectangle for resolve ops
> > Add stencil clear todo
> >
> > src/intel/vulkan/anv_genX.h | 3 +
> > src/intel/vulkan/gen7_cmd_buffer.c | 6 ++
> > src/intel/vulkan/gen8_cmd_buffer.c | 167 ++++++++++++++++++++++++++++++
> > +++++++
> > 3 files changed, 176 insertions(+)
> >
> > diff --git a/src/intel/vulkan/anv_genX.h b/src/intel/vulkan/anv_genX.h
> > index 02e79c2..ad3bec9 100644
> > --- a/src/intel/vulkan/anv_genX.h
> > +++ b/src/intel/vulkan/anv_genX.h
> > @@ -58,6 +58,9 @@ genX(emit_urb_setup)(struct anv_device *device, struct
> > anv_batch *batch,
> > unsigned vs_entry_size, unsigned gs_entry_size,
> > const struct gen_l3_config *l3_config);
> >
> > +void genX(cmd_buffer_do_hz_op)(struct anv_cmd_buffer *cmd_buffer,
> > + enum blorp_hiz_op op);
> > +
> > VkResult
> > genX(graphics_pipeline_create)(VkDevice _device,
> > struct anv_pipeline_cache *cache,
> > diff --git a/src/intel/vulkan/gen7_cmd_buffer.c
> > b/src/intel/vulkan/gen7_cmd_buffer.c
> > index b627ef0..78b5ac7 100644
> > --- a/src/intel/vulkan/gen7_cmd_buffer.c
> > +++ b/src/intel/vulkan/gen7_cmd_buffer.c
> > @@ -323,6 +323,12 @@ genX(cmd_buffer_flush_dynamic_state)(struct
> > anv_cmd_buffer *cmd_buffer)
> > cmd_buffer->state.dirty = 0;
> > }
> >
> > +void
> > +genX(cmd_buffer_do_hz_op)(struct anv_cmd_buffer *cmd_buffer,
> > + enum blorp_hiz_op op)
> > +{
> >
>
> This should have an anv_finishme in it.
>
>
I'll add this in V3.
> > +}
> > +
> > void genX(CmdSetEvent)(
> > VkCommandBuffer commandBuffer,
> > VkEvent event,
> > diff --git a/src/intel/vulkan/gen8_cmd_buffer.c
> > b/src/intel/vulkan/gen8_cmd_buffer.c
> > index 7058608..a13413c 100644
> > --- a/src/intel/vulkan/gen8_cmd_buffer.c
> > +++ b/src/intel/vulkan/gen8_cmd_buffer.c
> > @@ -399,6 +399,173 @@ genX(cmd_buffer_flush_compute_state)(struct
> > anv_cmd_buffer *cmd_buffer)
> > genX(cmd_buffer_apply_pipe_flushes)(cmd_buffer);
> > }
> >
> > +
> > +/**
> > + * Emit the HZ_OP packet in the sequence specified by the BDW PRM section
> > + * entitled: "Optimized Depth Buffer Clear and/or Stencil Buffer Clear."
> > + *
> > + * \todo Enable Stencil Buffer-only clears
> > + */
> > +void
> > +genX(cmd_buffer_do_hz_op)(struct anv_cmd_buffer *cmd_buffer,
> > + enum blorp_hiz_op op)
> > +{
> > + struct anv_cmd_state *cmd_state = &cmd_buffer->state;
> > + const struct anv_image_view *iview =
> > + anv_cmd_buffer_get_depth_stencil_view(cmd_buffer);
> > +
> > + if (iview == NULL || !anv_image_has_hiz(iview->image))
> > + return;
> > +
> > + const uint32_t ds = cmd_state->subpass->depth_stencil_attachment;
> > + const bool full_surface_op =
> > + cmd_state->render_area.extent.width == iview->extent.width
> > &&
> > + cmd_state->render_area.extent.height ==
> > iview->extent.height;
> >
>
> It's probably a bit redundant, but we might as well check
> render_area.offset == 0. I realize that, from API requirements, if the
> extents match then offset must be 0, but it's not incredibly obvious and
> the check won't hurt that much.
>
>
Sure. I'll add an assertion and a comment.
> > +
> > + /* 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;
> > +
> > + /* Apply alignment restrictions. Despite the BDW PRM mentioning
> > this is
> > + * only needed for a depth buffer surface type of D16_UNORM, testing
> > + * showed it to be necessary for other depth formats as well
> > + * (e.g., D32_FLOAT).
> > + */
> > + if (!full_surface_op) {
> > +
> > + struct isl_extent2d px_dim;
> >
>
> Would it be better to call this hiz_block_size_px? That follows the ISL
> naming convention a bit better.
>
>
The variable naming corresponds to the table headers below. I'll add
a comment about this.
> > +#if GEN_GEN == 8
> >
>
> Mind making this <= 8? I know it's in a gen8+ file, but <= 8 makes it
> clear that the else case is >= 9 and not != 8.
>
>
I see your point. I'll change the #else to #elif GEN_GEN >=9 as
I don't want to make the if case unspecific to make the else
case specific.
> > + /* Pre-SKL, HiZ has an 8x4 sample block. As the number of samples
> > + * increases, the number of pixels representable by this block
> > + * decreases by a factor of the sample dimensions. Sample
> > dimensions
> > + * scale following the MSAA interleaved pattern.
> > + *
> > + * Sample|Sample|Pixel
> > + * Count |Dim |Dim
> > + * ===================
> > + * 1 | 1x1 | 8x4
> > + * 2 | 2x1 | 4x4
> > + * 4 | 2x2 | 4x2
> > + * 8 | 4x2 | 2x2
> > + * 16 | 4x4 | 2x1
> > + *
> > + * Table: Pixel Dimensions in a HiZ Sample Block Pre-SKL
> > + */
> > + const struct isl_extent2d sa_dim =
> >
>
> Maybe call this px_size_sa?
>
>
I'll add a comment about this as well.
> > + isl_get_interleaved_msaa_px_size_sa(iview->image->samples);
> > + px_dim.w = 8 / sa_dim.w;
> > + px_dim.h = 4 / sa_dim.h;
> > +#else
> > + /* SKL+, the sample block becomes a "pixel block" so the expected
> > + * pixel dimension is a constant 8x4 px for all sample counts.
> > + */
> > + px_dim = (struct isl_extent2d) { .w = 8, .h = 4};
> > +#endif
> > +
> > + /* 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.
> > + */
> > + if (cmd_state->render_area.offset.x % px_dim.w ||
> > + cmd_state->render_area.offset.y % px_dim.h ||
> > + cmd_state->render_area.extent.width % px_dim.w ||
> > + cmd_state->render_area.extent.height % px_dim.h)
> > + return;
> >
>
> What about the case where the offset is aligned and we go to the far right
> or bottom of the image but the image size is not aligned? I think this
> case should be possible too.
>
>
Great point! This will be updated in V3.
> > + }
> > + break;
> > + case BLORP_HIZ_OP_DEPTH_RESOLVE:
> > + if (cmd_buffer->state.pass->attachments[ds].store_op !=
> > + VK_ATTACHMENT_STORE_OP_STORE)
> > + return;
> > + break;
> > + case BLORP_HIZ_OP_HIZ_RESOLVE:
> > + if (cmd_buffer->state.pass->attachments[ds].load_op !=
> > + VK_ATTACHMENT_LOAD_OP_LOAD)
> > + return;
> > + break;
> > + case BLORP_HIZ_OP_NONE:
> > + unreachable("Invalid HiZ OP");
> > + break;
> > + }
> > +
> > + 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.FullSurfaceDepthandStencilClear = full_surface_op;
> > + hzp.StencilClearValue = 0xff &
> > + cmd_state->attachments[ds].clear_value.depthStencil.
> > stencil;
> >
>
> nit: I would find having "clear_value & 0xff" and all on the second line
> more readable.
>
>
Updated in V3.
> > +
> > + /* Mark aspects as cleared */
> > + cmd_state->attachments[ds].pending_clear_aspects = 0;
> > + break;
> > + case BLORP_HIZ_OP_DEPTH_RESOLVE:
> > + hzp.DepthBufferResolveEnable = true;
> > + break;
> > + case BLORP_HIZ_OP_HIZ_RESOLVE:
> > + hzp.HierarchicalDepthBufferResolveEnable = true;
> > + break;
> > + case BLORP_HIZ_OP_NONE:
> > + unreachable("Invalid HiZ OP");
> > + break;
> > + }
> > +
> > + if (op != BLORP_HIZ_OP_DEPTH_CLEAR) {
> > + /* The Optimized HiZ resolve rectangle must be the size of the
> > full RT
> > + * and aligned to 8x4. The non-optimized Depth resolve rectangle
> > must
> > + * be the size of the full RT. The same alignment is assumed to
> > be
> > + * required.
> > + *
> > + * TODO:
> > + * Consider changing halign of non-D16 depth formats to 8 as mip
> > 2 may
> > + * get clobbered.
> >
>
> This shouldn't be needed on BDW+
>
I'll remove the TODO.
> Most of my comments were just nits.
>
> Reviewed-by: Jason Ekstrand <ja...@jlekstrand.net>
>
>
Thanks! I'll add a (v2) next your Rb. I'll also wait for your
confirmation that it still applies on v3 after it's been
released.
> > + */
> > + hzp.ClearRectangleXMax = align_u32(iview->extent.width, 8);
> > + hzp.ClearRectangleYMax = align_u32(iview->extent.height, 4);
> >
> + } else {
> > + /* This clear rectangle is aligned */
> > + hzp.ClearRectangleXMin = cmd_state->render_area.offset.x;
> > + hzp.ClearRectangleYMin = cmd_state->render_area.offset.y;
> > + hzp.ClearRectangleXMax = cmd_state->render_area.offset.x +
> > + cmd_state->render_area.extent.width;
> > + hzp.ClearRectangleYMax = cmd_state->render_area.offset.y +
> > + cmd_state->render_area.extent.height;
> > + }
> > +
> > +
> > + /* Due to a hardware issue, this bit MBZ */
> > + hzp.ScissorRectangleEnable = false;
> > + hzp.NumberofMultisamples = ffs(iview->image->samples) - 1;
> > + hzp.SampleMask = 0xFFFF;
> > + }
> > +
> > + anv_batch_emit(&cmd_buffer->batch, GENX(PIPE_CONTROL), pc) {
> > + pc.PostSyncOperation = WriteImmediateData;
> > + pc.Address =
> > + (struct anv_address){ &cmd_buffer->device->workaround_bo, 0 };
> > + }
> > +
> > + anv_batch_emit(&cmd_buffer->batch, GENX(3DSTATE_WM_HZ_OP), hzp);
> > +
> > + /* TODO:
> > + * Determine if a DepthCacheFlushEnable and DepthStallEnable is really
> > + * necessary for non-full_surface_op clears. Performing a HZ op without
> > + * this pipecontrol showed no impact on rendering results.
> > + */
> > + if (!full_surface_op && op == BLORP_HIZ_OP_DEPTH_CLEAR) {
> > + anv_batch_emit(&cmd_buffer->batch, GENX(PIPE_CONTROL), pc) {
> > + pc.DepthStallEnable = true;
> > + pc.DepthCacheFlushEnable = true;
> > + }
> > + }
> > +}
> > +
> > void genX(CmdSetEvent)(
> > VkCommandBuffer commandBuffer,
> > VkEvent _event,
> > --
> > 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
