<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, May 10, 2017 at 11:30 AM, 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"><div class="gmail-HOEnZb"><div class="gmail-h5">On Wed, May 03, 2017 at 02:24:19PM -0700, Jason Ekstrand wrote: > On Thu, Apr 27, 2017 at 11:32 AM, Nanley Chery <<a href="mailto:nanleychery@gmail.com">nanleychery@gmail.com</a>> > wrote: > > > Signed-off-by: Nanley Chery <<a href="mailto:nanley.g.chery@intel.com">nanley.g.chery@intel.com</a>> > > --- > > src/intel/vulkan/genX_cmd_buffer.c | 93 ++++++++++++++++++++++++++++++ > > ++++++++ > > 1 file changed, 93 insertions(+) > > > > diff --git a/src/intel/vulkan/genX_cmd_buffer.c > > b/src/intel/vulkan/genX_cmd_buffer.c > > index d5cc358aec..1ae0c3256e 100644 > > --- a/src/intel/vulkan/genX_cmd_buffer.c > > +++ b/src/intel/vulkan/genX_cmd_buffer.c > > @@ -454,6 +454,99 @@ genX(transfer_clear_value)(struct anv_cmd_buffer * > > const cmd_buffer, > > } > > } > > > > +/* Transitions a color buffer from one layout to another. */ > > +static void > > +transition_color_buffer(struct anv_cmd_buffer * const cmd_buffer, > > + const struct anv_image * const image, > > + const uint32_t base_level, uint32_t level_count, > > + const uint32_t base_layer, uint32_t layer_count, > > + const VkImageLayout initial_layout, > > + const VkImageLayout final_layout) > > +{ > > + assert(cmd_buffer && image); > > + > > + /* This must be a color image. */ > > + assert(image->aspects == VK_IMAGE_ASPECT_COLOR_BIT); > > + > > + /* Only color buffers with CCS need resolving. */ > > + if (image->aux_surface.isl.size == 0 || image->samples > 1) > > + return; > > + > > + /* Don't transition this subresource range if it lacks auxiliary data. > > */ > > + if (base_level >= anv_color_aux_levels(image) || > > + base_layer >= anv_color_aux_layers(image, base_level)) > > + return; > > + > > + /* The undefined layout indicates that the user doesn't care about the > > + * data that's currently in the buffer. The pre-initialized layout is > > + * equivalent to the undefined layout for optimally-tiled images. > > + * > > + * We can only skip the resolve for CCS_E images in this layout > > because it > > + * is enabled outside of render passes. This allows previously > > fast-cleared > > + * and undefined buffers to be defined with transfer operations. > > + */ > > + const bool is_ccs_e = image->aux_usage == ISL_AUX_USAGE_CCS_E; > > + const bool undef_layout = initial_layout == VK_IMAGE_LAYOUT_UNDEFINED > > || > > + initial_layout == VK_IMAGE_LAYOUT_ > > PREINITIALIZED; > > + if (is_ccs_e && undef_layout) > > + return; > > + > > + /* A resolve isn't necessary when transitioning from a layout that > > doesn't > > + * have fast-clear data or to a layout that will be aware of the > > fast-clear > > + * value. > > + */ > > + const bool maybe_fast_cleared = undef_layout || initial_layout == > > + VK_IMAGE_LAYOUT_COLOR_ > > ATTACHMENT_OPTIMAL; > > + if (!maybe_fast_cleared || final_layout == > > + VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL) > > + return; > > + > > + /* Determine the optimal resolve operation. For now we only need to > > resolve > > + * the clear color. > > + */ > > + const enum blorp_fast_clear_op op = is_ccs_e ? > > + BLORP_FAST_CLEAR_OP_RESOLVE_PARTIAL > > : > > + BLORP_FAST_CLEAR_OP_RESOLVE_FULL; > > + > > + /* The actual range that will be transitioned is limited by the number > > of > > + * subresources that have auxiliary data. > > + */ > > + level_count = MIN2(level_count, anv_color_aux_levels(image)); > > + > > + /* A write cache flush with an end-of-pipe sync is required between > > + * rendering, clearing, and resolving operations. Perform a flush of > > the > > + * write cache before and after the resolve operation to meet this > > + * requirement. > > + */ > > > > anv_blorp.c has a more detailed comment about this. I think I'd rather we > copy it here so that it doesn't get lost when we delete > ccs_resolve_attachments. > > Also, I have no idea whether we need to do it per-fast-clear-op or once > before we do fast-clear ops and once afterwards. I think this is probably > fine. > > </div></div>Contrary to the comment in anv_blorp, I found documentation that provides us more information as to what's needed. How about I rewrite my comment to this instead? From the Sky Lake PRM Vol. 7, "MCS Buffer for Render Target(s)": Any transition from any value in {Clear, Render, Resolve} to a different value in {Clear, Render, Resolve} requires end of pipe synchronization. Perform a flush of the write cache before and after the resolve operation to meet this requirement. </blockquote><div> </div><div>That PRM citation is fine and maybe better than the one I used (though they say about the same thing). The part I was hoping to preserve is * This comment is a bit cryptic and doesn't really tell you what's going * or what's really needed. It appears that fast clear ops are not * properly synchronized with other drawing. We need to use a PIPE_CONTROL * to ensure that the contents of the previous draw hit the render target * before we resolve and then use a second PIPE_CONTROL after the resolve * to ensure that it is completed before any additional drawing occurs. </div><div>Yes, the PRM text tells you that an end of pipe sync is needed, but it doesn't tell you where. It's empirical evidence from hunting down bugs that tells us we need it on both sides. </div><div>--Jason </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"> -Nanley <div class="gmail-HOEnZb"><div class="gmail-h5"> > > + cmd_buffer->state.pending_pipe_bits |= > > + ANV_PIPE_RENDER_TARGET_CACHE_FLUSH_BIT | ANV_PIPE_CS_STALL_BIT; > > + > > + for (uint32_t level = base_level; level < base_level + level_count; > > level++) { > > + > > + layer_count = MIN2(layer_count, anv_color_aux_layers(image, level)); > > + for (uint32_t layer = base_layer; layer < base_layer + layer_count; > > layer++) { > > + > > + /* Create a surface state with the right clear color and perform > > the > > + * resolve. > > + */ > > + struct anv_state surface_state = > > + anv_cmd_buffer_alloc_surface_state(cmd_buffer); > > + anv_fill_ccs_resolve_ss(cmd_buffer->device, surface_state.map, > > image, > > + level, layer); > > + add_image_relocs(cmd_buffer, image, VK_IMAGE_ASPECT_COLOR_BIT, > > + is_ccs_e ? ISL_AUX_USAGE_CCS_E : > > ISL_AUX_USAGE_CCS_D, > > + surface_state); > > + anv_state_flush(cmd_buffer->device, surface_state); > > + genX(transfer_clear_value)(cmd_buffer, surface_state, image, > > level, > > + false); > > + anv_ccs_resolve(cmd_buffer, surface_state, image, level, layer, > > op); > > + } > > + } > > + > > + cmd_buffer->state.pending_pipe_bits |= > > + ANV_PIPE_RENDER_TARGET_CACHE_FLUSH_BIT | ANV_PIPE_CS_STALL_BIT; > > +} > > + > > /** > > * Setup anv_cmd_state::attachments for vkCmdBeginRenderPass. > > */ > > -- > > 2.12.2 > > > > _______________________________________________ > > mesa-dev mailing list > > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > </div></div></blockquote></div> </div></div>