<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Aug 31, 2016 at 8:29 PM, Nanley Chery <<a href="mailto:nanleychery@gmail.com" target="_blank">nanleychery@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">From: Jason Ekstrand <<a href="mailto:jason.ekstrand@intel.com">jason.ekstrand@intel.com</a>> </blockquote><div> </div>First off, this is your patch not mine. The patch of mine you based this on was little more than a skeleton that demonstrated how to use PIPE_CONTROL. All of the interesting stuff in here is yours. <div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Nanley Chery: (rebase) - Resolve conflicts with the new anv_batch_emit macro (amend) - Update commit title - Combine all HZ operations into one function - Add code for performing HiZ resolve operations - Add proper stencil and multisampling support - Set the proper clear rectangles - Add required cases for aborting an HZ operation Signed-off-by: Nanley Chery <<a href="mailto:nanley.g.chery@intel.com">nanley.g.chery@intel.com</a>> --- src/intel/vulkan/anv_genX.h | 3 + src/intel/vulkan/anv_private.h | 6 ++ src/intel/vulkan/gen7_cmd_buffer.c | 5 ++ src/intel/vulkan/gen8_cmd_buffer.c | 124 +++++++++++++++++++++++++++++++++++++ 4 files changed, 138 insertions(+) diff --git a/src/intel/vulkan/anv_genX.h b/src/intel/vulkan/anv_genX.h index cf5a232..16de990 100644 --- a/src/intel/vulkan/anv_genX.h +++ b/src/intel/vulkan/anv_genX.h @@ -54,6 +54,9 @@ void genX(cmd_buffer_flush_dynamic_state)(struct anv_cmd_buffer *cmd_buffer); void genX(cmd_buffer_flush_compute_state)(struct anv_cmd_buffer *cmd_buffer); +void genX(cmd_buffer_do_hz_op)(struct anv_cmd_buffer *cmd_buffer, + enum anv_hz_op op); + VkResult genX(graphics_pipeline_create)(VkDevice _device, struct anv_pipeline_cache *cache, diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h index 5718a19..40325fd 100644 --- a/src/intel/vulkan/anv_private.h +++ b/src/intel/vulkan/anv_private.h @@ -1401,6 +1401,12 @@ anv_cmd_buffer_get_depth_stencil_view(const struct anv_cmd_buffer *cmd_buffer); void anv_cmd_buffer_dump(struct anv_cmd_buffer *cmd_buffer); +enum anv_hz_op { + ANV_HZ_OP_CLEAR, + ANV_HZ_OP_HIZ_RESOLVE, + ANV_HZ_OP_DEPTH_RESOLVE, +}; </blockquote><div> </div><div>Now that blorp is in its own folder, we could use the blorp_hiz_op enum instead of rolling our own. That'll make it easier to add gen7 support. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + struct anv_fence { struct anv_bo bo; struct drm_i915_gem_execbuffer2 execbuf; diff --git a/src/intel/vulkan/gen7_cmd_buffer.c b/src/intel/vulkan/gen7_cmd_buffer.c index 61778aa..a057a04 100644 --- a/src/intel/vulkan/gen7_cmd_buffer.c +++ b/src/intel/vulkan/gen7_cmd_buffer.c @@ -323,6 +323,11 @@ 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 anv_hz_op op) +{ +} + 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 e22b4e2..4f27350 100644 --- a/src/intel/vulkan/gen8_cmd_buffer.c +++ b/src/intel/vulkan/gen8_cmd_buffer.c @@ -399,6 +399,130 @@ 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." + */ +void +genX(cmd_buffer_do_hz_op)(struct anv_cmd_buffer *cmd_buffer, enum anv_hz_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; </blockquote><div> </div><div>This looks like something that would be better as an assert. Silently doing nothing is probably fine for resolves. For clears on the other hand, it means silently *not* clearing which would be bad. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + + 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; </blockquote><div> </div><div>I think you also need render_area.offset == 0 checks? </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + + /* Validate that we can perform the HZ operation and that it's necessary. */ + switch (op) { + case ANV_HZ_OP_CLEAR: + if (cmd_buffer->state.pass->attachments[ds].load_op != + VK_ATTACHMENT_LOAD_OP_CLEAR) + return; + + /* Apply alignment restrictions. For a sample count of 16, the formulas + * reduce to identity and indicate that no alignment is required. + */ </blockquote><div> </div><div>I think you're being a bit over-restrictive here. From the Sky Lake PRM Vol. 7 "Hierarchical Depth Buffer" (p. 626 in my pdf): "The minimum granularity of clear is one pixel, but all samples of the pixel must be cleared. Clearing partial samples of a pixel is not supported. If a newly allocated depth buffer is not padded to an integer multiple of 8x4 pixels, and if the first operation on the depth buffer does not clear the entire width and height of the surface, then first a HiZ ambiguate must be done on the portions of the depth buffer that are not cleared. If the depth buffer clear operation does clear the entire width and height of the surface, then the “full surface clear” bit in 3DSTATE_WM_OP must be set to 1." </div><div>In other words, the granularity of the HiZ operation is 1 pixel. However, you need to ensure that the the HiZ buffer is valid for all 8x4 blocks or else strange things may happen. We already do this with a memset at BindImageMemory time so our HiZ buffer should always contain valid data. </div><div>In the Broadwell PRM, same section, it does list that alignment requirement but says that it only applies to D16_UNORM surfaces. For all other surface formats, Broadwell appears to follow the Sky Lake rules where, as long as the HiZ buffer contains valid data, everything goes. </div><div>I think we need a crucible test for partial (maybe even multisampled) depth clears. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + if (!full_surface_op && iview->image->samples < 16) { </blockquote><div> </div><div>If my understanding of Sky Lake is correct, the samples < 16 doesn't matter because the alignment restrictions don't apply to Sky Lake anyway. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + uint32_t align_w = 1; + uint32_t align_h = 1; + + if (iview->image->samples > 1) { + isl_msaa_interleaved_scale_px_to_sa(iview->image->samples, + &align_w, &align_h); + } + + align_w = 8 / align_w; + align_h = 4 / align_h; + + if (cmd_state->render_area.offset.x % align_w || + cmd_state->render_area.offset.y % align_h || + cmd_state->render_area.extent.width % align_w || + cmd_state->render_area.extent.height % align_h) + return; + } + break; + case ANV_HZ_OP_DEPTH_RESOLVE: + if (cmd_buffer->state.pass->attachments[ds].store_op != + VK_ATTACHMENT_STORE_OP_STORE) + return; + break; + case ANV_HZ_OP_HIZ_RESOLVE: + if (cmd_buffer->state.pass->attachments[ds].load_op != + VK_ATTACHMENT_LOAD_OP_LOAD) + return; + break; + } + + anv_batch_emit(&cmd_buffer->batch, GENX(3DSTATE_WM_HZ_OP), hzp) { + switch (op) { + case ANV_HZ_OP_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; + + /* Mark aspects as cleared */ + cmd_state->attachments[ds].pending_clear_aspects = 0; + break; + case ANV_HZ_OP_DEPTH_RESOLVE: + hzp.DepthBufferResolveEnable = true; + break; + case ANV_HZ_OP_HIZ_RESOLVE: + hzp.HierarchicalDepthBufferResolveEnable = true; + break; + } + + /* The depth resolve rectangle must match the size of the previous clear + * rectangle. + * + * The HiZ resolve rectangle is specified as needing to be the + * size of the full RT and aligned to 8x4, these requirements are in + * conflict if the RT extent is not a multiple of 8x4. Testing shows + * that setting the rectangle to match the render area works just fine. + * + * In a manner similar to i965, we'd like to diverge from the PRMs here + * to reduce the number of HiZ blocks written to. </blockquote><div> </div><div>Not aligning to 8x4 shouldn't affect the number of HiZ blocks written to. Maybe I'm confused :/ </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + */ + hzp.ClearRectangleXMin = anv_minify(cmd_state->render_area.offset.x, + iview->base_mip); + hzp.ClearRectangleYMin = anv_minify(cmd_state->render_area.offset.y, + iview->base_mip); + hzp.ClearRectangleXMax = anv_minify(cmd_state->render_area.offset.x + + cmd_state->render_area.extent.width, + iview->base_mip); + hzp.ClearRectangleYMax = anv_minify(cmd_state->render_area.offset.y + + cmd_state->render_area.extent.height, + iview->base_mip); </blockquote><div> </div><div>This isn't correct. The size of the render area should already take into account the minification. When you render into a non-zero LOD of an image, the size of your framebuffer is the size of that LOD and the render area must be a subset of that. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + + /* 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 DepthFlush and DepthStall is really necessary for + * non-full_surface_op clears. + */ +} + void genX(CmdSetEvent)( VkCommandBuffer commandBuffer, VkEvent _event, -- 2.9.3 </blockquote></div> </div></div>