On Sat, Jan 13, 2018 at 10:12:53AM -0800, Jason Ekstrand wrote: > On Sat, Jan 13, 2018 at 9:55 AM, Jason Ekstrand <[email protected]> > wrote: > > > On Wed, Dec 13, 2017 at 4:46 PM, Nanley Chery <[email protected]> > > wrote: > > > >> On Mon, Nov 27, 2017 at 07:06:01PM -0800, Jason Ekstrand wrote: > >> > Currently, this helper does nothing but we call it every place where an > >> > image is written through the render pipeline. This will allow us to > >> > properly mark the aux state so that we can handle resolves correctly. > >> > --- > >> > src/intel/vulkan/anv_blorp.c | 36 ++++++++++++++++++++++++++++++ > >> ++++++ > >> > src/intel/vulkan/anv_cmd_buffer.c | 12 ++++++++++++ > >> > src/intel/vulkan/anv_genX.h | 6 ++++++ > >> > src/intel/vulkan/anv_private.h | 7 +++++++ > >> > src/intel/vulkan/genX_cmd_buffer.c | 17 +++++++++++++++++ > >> > 5 files changed, 78 insertions(+) > >> > > >> > diff --git a/src/intel/vulkan/anv_blorp.c b/src/intel/vulkan/anv_blorp.c > >> > index da273d6..46e2eb0 100644 > >> > --- a/src/intel/vulkan/anv_blorp.c > >> > +++ b/src/intel/vulkan/anv_blorp.c > >> > @@ -271,6 +271,7 @@ void anv_CmdCopyImage( > >> > > >> > assert(anv_image_aspects_compatible(src_mask, dst_mask)); > >> > > >> > + const uint32_t dst_level = pRegions[r].dstSubresource.mipLevel; > >> > if (_mesa_bitcount(src_mask) > 1) { > >> > uint32_t aspect_bit; > >> > anv_foreach_image_aspect_bit(aspect_bit, src_image, > >> src_mask) { > >> > @@ -281,6 +282,10 @@ void anv_CmdCopyImage( > >> > get_blorp_surf_for_anv_image(cmd_buffer->device, > >> > dst_image, 1UL << aspect_bit, > >> > ANV_AUX_USAGE_DEFAULT, > >> &dst_surf); > >> > + anv_cmd_buffer_mark_image_written(cmd_buffer, dst_image, > >> > + 1UL << aspect_bit, > >> > + dst_surf.aux_usage, > >> > + dst_level); > >> > > >> > for (unsigned i = 0; i < layer_count; i++) { > >> > blorp_copy(&batch, &src_surf, > >> pRegions[r].srcSubresource.mipLevel, > >> > @@ -298,6 +303,8 @@ void anv_CmdCopyImage( > >> > ANV_AUX_USAGE_DEFAULT, > >> &src_surf); > >> > get_blorp_surf_for_anv_image(cmd_buffer->device, dst_image, > >> dst_mask, > >> > ANV_AUX_USAGE_DEFAULT, > >> &dst_surf); > >> > + anv_cmd_buffer_mark_image_written(cmd_buffer, dst_image, > >> dst_mask, > >> > + dst_surf.aux_usage, > >> dst_level); > >> > > >> > for (unsigned i = 0; i < layer_count; i++) { > >> > blorp_copy(&batch, &src_surf, > >> pRegions[r].srcSubresource.mipLevel, > >> > @@ -387,6 +394,12 @@ copy_buffer_to_image(struct anv_cmd_buffer > >> *cmd_buffer, > >> > extent.width, extent.height, > >> > buffer_row_pitch, buffer_format, > >> > &buffer.surf, &buffer_isl_surf); > >> > + if (dst->surf.aux_usage != ISL_AUX_USAGE_NONE) { > >> > + assert(dst == &image); > >> > + anv_cmd_buffer_mark_image_written(cmd_buffer, anv_image, > >> > + aspect, dst->surf.aux_usage, > >> > + dst->level); > >> > + } > >> > > >> > for (unsigned z = 0; z < extent.depth; z++) { > >> > blorp_copy(&batch, &src->surf, src->level, src->offset.z, > >> > @@ -497,6 +510,10 @@ void anv_CmdBlitImage( > >> > get_blorp_surf_for_anv_image(cmd_buffer->device, > >> > dst_image, dst_res->aspectMask, > >> > ANV_AUX_USAGE_DEFAULT, &dst); > >> > + anv_cmd_buffer_mark_image_written(cmd_buffer, dst_image, > >> > + dst_res->aspectMask, > >> > + dst.aux_usage, > >> > + dst_res->mipLevel); > >> > > >> > struct anv_format_plane src_format = > >> > anv_get_format_plane(&cmd_buffer->device->info, > >> src_image->vk_format, > >> > @@ -820,6 +837,10 @@ void anv_CmdClearColorImage( > >> > layer_count = anv_minify(image->extent.depth, level); > >> > } > >> > > >> > + anv_cmd_buffer_mark_image_written(cmd_buffer, image, > >> > + pRanges[r].aspectMask, > >> > + surf.aux_usage, level); > >> > + > >> > blorp_clear(&batch, &surf, > >> > src_format.isl_format, src_format.swizzle, > >> > level, base_layer, layer_count, > >> > @@ -1215,6 +1236,11 @@ anv_cmd_buffer_clear_subpass(struct > >> anv_cmd_buffer *cmd_buffer) > >> > ANV_PIPE_RENDER_TARGET_CACHE_FLUSH_BIT | > >> ANV_PIPE_CS_STALL_BIT; > >> > } else { > >> > assert(image->n_planes == 1); > >> > + anv_cmd_buffer_mark_image_written(cmd_buffer, image, > >> > + VK_IMAGE_ASPECT_COLOR_BIT, > >> > + att_state->aux_usage, > >> > + > >> iview->planes[0].isl.base_level); > >> > + > >> > blorp_clear(&batch, &surf, iview->planes[0].isl.format, > >> > anv_swizzle_for_render(iview- > >> >planes[0].isl.swizzle), > >> > iview->planes[0].isl.base_level, > >> > @@ -1355,6 +1381,8 @@ resolve_image(struct anv_device *device, > >> > uint32_t src_x, uint32_t src_y, uint32_t dst_x, uint32_t > >> dst_y, > >> > uint32_t width, uint32_t height) > >> > { > >> > + struct anv_cmd_buffer *cmd_buffer = batch->driver_batch; > >> > + > >> > assert(src_image->type == VK_IMAGE_TYPE_2D); > >> > assert(src_image->samples > 1); > >> > assert(dst_image->type == VK_IMAGE_TYPE_2D); > >> > @@ -1369,6 +1397,10 @@ resolve_image(struct anv_device *device, > >> > ANV_AUX_USAGE_DEFAULT, &src_surf); > >> > get_blorp_surf_for_anv_image(device, dst_image, 1UL << > >> aspect_bit, > >> > ANV_AUX_USAGE_DEFAULT, &dst_surf); > >> > + anv_cmd_buffer_mark_image_written(cmd_buffer, dst_image, > >> > + 1UL << aspect_mask, > >> ^ > >> aspect_bit? > >> > > > > Yup. Fixed locally. > > > > > >> > + dst_surf.aux_usage, > >> > + dst_level); > >> > > >> > assert(!src_image->format->can_ycbcr); > >> > assert(!dst_image->format->can_ycbcr); > >> > @@ -1498,6 +1530,10 @@ anv_cmd_buffer_resolve_subpass(struct > >> anv_cmd_buffer *cmd_buffer) > >> > get_blorp_surf_for_anv_image(cmd_buffer->device, > >> dst_iview->image, > >> > VK_IMAGE_ASPECT_COLOR_BIT, > >> > dst_aux_usage, &dst_surf); > >> > + anv_cmd_buffer_mark_image_written(cmd_buffer, > >> dst_iview->image, > >> > + > >> VK_IMAGE_ASPECT_COLOR_BIT, > >> > + dst_surf.aux_usage, > >> > + > >> dst_iview->planes[0].isl.base_level); > >> > >> ^ > >> Indentation is off here. > >> > > > > Thanks. Fixed. > > > > > >> > > >> > assert(!src_iview->image->format->can_ycbcr); > >> > assert(!dst_iview->image->format->can_ycbcr); > >> > diff --git a/src/intel/vulkan/anv_cmd_buffer.c > >> b/src/intel/vulkan/anv_cmd_buffer.c > >> > index 7e7580c..2c9e919 100644 > >> > --- a/src/intel/vulkan/anv_cmd_buffer.c > >> > +++ b/src/intel/vulkan/anv_cmd_buffer.c > >> > @@ -353,6 +353,18 @@ anv_cmd_buffer_emit_state_base_address(struct > >> anv_cmd_buffer *cmd_buffer) > >> > cmd_buffer); > >> > } > >> > > >> > +void > >> > +anv_cmd_buffer_mark_image_written(struct anv_cmd_buffer *cmd_buffer, > >> > + const struct anv_image *image, > >> > + VkImageAspectFlagBits aspect, > >> > >> Some callers may pass more than one aspect. I think this should be > >> plural instead. > >> > > > > Sure, we can do that. > > > > I'm going back-and-forth on this. On the one hand, it would sort-of make > sense. On the other hand, there are only two cases in which multiple > aspects can even happen. If it's a depth/stencil image then we want to > call it twice anyway because AUX_USAGE_HIZ doesn't make sense for the > stencil aspect. If it's multi-planar (i.e. YcBcR), then we are going to be > working with each of the different planes differently. Also, a YcBcR image > may have different aux usages per-plane. That last argument, I think, > convinced me that just one aspect is best. > >
Sorry for the noise. I think you're right. I looked over all the relevant usages again and it seems like only one aspect would be passed in. With the fixes you've made, this patch is Reviewed-by: Nanley Chery <[email protected]> > > -Nanley > >> > >> > + enum isl_aux_usage aux_usage, > >> > + unsigned level) > >> > +{ > >> > + anv_genX_call(&cmd_buffer->device->info, > >> > + cmd_buffer_mark_image_written, > >> > + cmd_buffer, image, aspect, aux_usage, level); > >> > +} > >> > + > >> > void anv_CmdBindPipeline( > >> > VkCommandBuffer commandBuffer, > >> > VkPipelineBindPoint pipelineBindPoint, > >> > diff --git a/src/intel/vulkan/anv_genX.h b/src/intel/vulkan/anv_genX.h > >> > index 0b7322e..85b893e 100644 > >> > --- a/src/intel/vulkan/anv_genX.h > >> > +++ b/src/intel/vulkan/anv_genX.h > >> > @@ -58,6 +58,12 @@ void genX(cmd_buffer_flush_compute_state)(struct > >> anv_cmd_buffer *cmd_buffer); > >> > void genX(cmd_buffer_enable_pma_fix)(struct anv_cmd_buffer > >> *cmd_buffer, > >> > bool enable); > >> > > >> > +void genX(cmd_buffer_mark_image_written)(struct anv_cmd_buffer > >> *cmd_buffer, > >> > + const struct anv_image *image, > >> > + VkImageAspectFlagBits aspect, > >> > + enum isl_aux_usage aux_usage, > >> > + unsigned level); > >> > + > >> > void > >> > genX(emit_urb_setup)(struct anv_device *device, struct anv_batch > >> *batch, > >> > const struct gen_l3_config *l3_config, > >> > diff --git a/src/intel/vulkan/anv_private.h > >> b/src/intel/vulkan/anv_private.h > >> > index 461bfed..f805246 100644 > >> > --- a/src/intel/vulkan/anv_private.h > >> > +++ b/src/intel/vulkan/anv_private.h > >> > @@ -2530,6 +2530,13 @@ anv_can_sample_with_hiz(const struct > >> gen_device_info * const devinfo, > >> > } > >> > > >> > void > >> > +anv_cmd_buffer_mark_image_written(struct anv_cmd_buffer *cmd_buffer, > >> > + const struct anv_image *image, > >> > + VkImageAspectFlagBits aspect, > >> > + enum isl_aux_usage aux_usage, > >> > + unsigned level); > >> > + > >> > +void > >> > anv_image_hiz_op(struct anv_cmd_buffer *cmd_buffer, > >> > const struct anv_image *image, > >> > VkImageAspectFlagBits aspect, uint32_t level, > >> > diff --git a/src/intel/vulkan/genX_cmd_buffer.c > >> b/src/intel/vulkan/genX_cmd_buffer.c > >> > index 65cc85d..7d040bd 100644 > >> > --- a/src/intel/vulkan/genX_cmd_buffer.c > >> > +++ b/src/intel/vulkan/genX_cmd_buffer.c > >> > @@ -460,6 +460,15 @@ genX(load_needs_resolve_predicate)(struct > >> anv_cmd_buffer *cmd_buffer, > >> > } > >> > } > >> > > >> > +void > >> > +genX(cmd_buffer_mark_image_written)(struct anv_cmd_buffer *cmd_buffer, > >> > + const struct anv_image *image, > >> > + VkImageAspectFlagBits aspect, > >> > + enum isl_aux_usage aux_usage, > >> > + unsigned level) > >> > +{ > >> > +} > >> > + > >> > static void > >> > init_fast_clear_state_entry(struct anv_cmd_buffer *cmd_buffer, > >> > const struct anv_image *image, > >> > @@ -3014,6 +3023,14 @@ cmd_buffer_subpass_sync_fast_clear_values(struct > >> anv_cmd_buffer *cmd_buffer) > >> > false /* copy to ss */); > >> > } > >> > } > >> > + > >> > + /* We assume that if we're starting a subpass, we're going to do > >> some > >> > + * rendering so we may end up with compressed data. > >> > + */ > >> > + genX(cmd_buffer_mark_image_written)(cmd_buffer, iview->image, > >> > + VK_IMAGE_ASPECT_COLOR_BIT, > >> > + att_state->aux_usage, > >> > + > >> iview->planes[0].isl.base_level); > >> > } > >> > } > >> > > >> > -- > >> > 2.5.0.400.gff86faf > >> > > >> > _______________________________________________ > >> > mesa-dev mailing list > >> > [email protected] > >> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > >> > > > > _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
