[Mesa-dev] [PATCH v2 12/24] anv/cmd_buffer: Add a mark_image_written helper
Nanley Chery
nanleychery at gmail.com
Tue Jan 23 23:02:21 UTC 2018
On Mon, Jan 22, 2018 at 05:25:31PM -0800, Jason Ekstrand wrote:
> On Mon, Jan 22, 2018 at 3:22 PM, Nanley Chery <nanleychery at gmail.com> wrote:
>
> > On Fri, Jan 19, 2018 at 03:47:29PM -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 | 44 ++++++++++++++++++++++++++++++
> > +++++++-
> > > src/intel/vulkan/anv_cmd_buffer.c | 15 +++++++++++++
> > > src/intel/vulkan/anv_genX.h | 8 +++++++
> > > src/intel/vulkan/anv_private.h | 9 ++++++++
> > > src/intel/vulkan/genX_cmd_buffer.c | 44 ++++++++++++++++++++++++++++++
> > ++++++++
> > > 5 files changed, 119 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/intel/vulkan/anv_blorp.c b/src/intel/vulkan/anv_blorp.c
> > > index e4e4135..05efc6d 100644
> > > --- a/src/intel/vulkan/anv_blorp.c
> > > +++ b/src/intel/vulkan/anv_blorp.c
> > > @@ -283,6 +283,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,
> > > + dst_base_layer,
> > layer_count);
> > >
> > > for (unsigned i = 0; i < layer_count; i++) {
> > > blorp_copy(&batch, &src_surf, src_level, src_base_layer
> > + i,
> > > @@ -298,6 +302,9 @@ 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,
> > > + dst_base_layer, layer_count);
> > >
> > > for (unsigned i = 0; i < layer_count; i++) {
> > > blorp_copy(&batch, &src_surf, src_level, src_base_layer + i,
> > > @@ -386,6 +393,13 @@ copy_buffer_to_image(struct anv_cmd_buffer
> > *cmd_buffer,
> > > buffer_row_pitch, buffer_format,
> > > &buffer.surf, &buffer_isl_surf);
> > >
> > > + if (&image == dst) {
> > > + anv_cmd_buffer_mark_image_written(cmd_buffer, anv_image,
> > > + aspect, dst->surf.aux_usage,
> > > + dst->level,
> > > + dst->offset.z, extent.depth);
> > > + }
> > > +
> > > for (unsigned z = 0; z < extent.depth; z++) {
> > > blorp_copy(&batch, &src->surf, src->level, src->offset.z,
> > > &dst->surf, dst->level, dst->offset.z,
> > > @@ -545,6 +559,12 @@ void anv_CmdBlitImage(
> > > bool flip_y = flip_coords(&src_y0, &src_y1, &dst_y0, &dst_y1);
> > >
> > > const unsigned num_layers = dst_end - dst_start;
> > > + anv_cmd_buffer_mark_image_written(cmd_buffer, dst_image,
> > > + dst_res->aspectMask,
> > > + dst.aux_usage,
> > > + dst_res->mipLevel,
> > > + dst_start, num_layers);
> > > +
> > > for (unsigned i = 0; i < num_layers; i++) {
> > > unsigned dst_z = dst_start + i;
> > > unsigned src_z = src_start + i * src_z_step;
> > > @@ -558,7 +578,6 @@ void anv_CmdBlitImage(
> > > dst_x0, dst_y0, dst_x1, dst_y1,
> > > gl_filter, flip_x, flip_y);
> > > }
> > > -
> >
> > ^
> > Random line deletion? Not a big deal.
> >
>
> Yup. Rebase fail. Dropped.
>
>
> > > }
> > >
> > > blorp_batch_finish(&batch);
> > > @@ -818,6 +837,11 @@ 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,
> > > + base_layer, layer_count);
> > > +
> > > blorp_clear(&batch, &surf,
> > > src_format.isl_format, src_format.swizzle,
> > > level, base_layer, layer_count,
> > > @@ -1215,6 +1239,13 @@ 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,
> > > + iview->planes[0].isl.base_
> > array_layer,
> > > + fb->layers);
> > > +
> > > 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 +1386,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 +1402,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_bit,
> > > + dst_surf.aux_usage,
> > > + dst_level, dst_layer, 1);
> > >
> > > assert(!src_image->format->can_ycbcr);
> > > assert(!dst_image->format->can_ycbcr);
> > > @@ -1498,6 +1535,11 @@ 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,
> > > +
> > dst_iview->planes[0].isl.base_array_layer, 1);
> >
> > As we discussed in the office, this entire function (including this
> > hunk) should be resolving fb->layers. This function was already broken
> > and this new hunk doesn't break it any more, so it's fine to leave it as
> > is. I've filed a bug about the lack of testing for this.
> >
>
> Sounds good.
>
>
> > >
> > > 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..7480ac2 100644
> > > --- a/src/intel/vulkan/anv_cmd_buffer.c
> > > +++ b/src/intel/vulkan/anv_cmd_buffer.c
> > > @@ -353,6 +353,21 @@ 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,
> > > + enum isl_aux_usage aux_usage,
> > > + uint32_t level,
> > > + uint32_t base_layer,
> > > + uint32_t layer_count)
> > > +{
> > > + anv_genX_call(&cmd_buffer->device->info,
> > > + cmd_buffer_mark_image_written,
> > > + cmd_buffer, image, aspect, aux_usage,
> > > + level, base_layer, layer_count);
> > > +}
> > > +
> > > 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..9b56658 100644
> > > --- a/src/intel/vulkan/anv_genX.h
> > > +++ b/src/intel/vulkan/anv_genX.h
> > > @@ -58,6 +58,14 @@ 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,
> > > + uint32_t level,
> > > + uint32_t base_layer,
> > > + uint32_t layer_count);
> > > +
> > > 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 f81f8e1..f0251e2 100644
> > > --- a/src/intel/vulkan/anv_private.h
> > > +++ b/src/intel/vulkan/anv_private.h
> > > @@ -2542,6 +2542,15 @@ 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,
> > > + uint32_t level,
> > > + uint32_t base_layer,
> > > + uint32_t layer_count);
> > > +
> > > +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 fd27463..ad7b9fc 100644
> > > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > > @@ -460,6 +460,19 @@ 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,
> > > + uint32_t level,
> > > + uint32_t base_layer,
> > > + uint32_t layer_count)
> > > +{
> > > + /* The aspect must be exactly one of the image aspects. */
> > > + assert(_mesa_bitcount(aspect) == 1 && (aspect & image->aspects));
> > > +}
> > > +
> > > static void
> > > init_fast_clear_state_entry(struct anv_cmd_buffer *cmd_buffer,
> > > const struct anv_image *image,
> > > @@ -2976,6 +2989,27 @@ cmd_buffer_emit_depth_stencil(struct
> > anv_cmd_buffer *cmd_buffer)
> > > isl_emit_depth_stencil_hiz_s(&device->isl_dev, dw, &info);
> > >
> > > cmd_buffer->state.hiz_enabled = info.hiz_usage == ISL_AUX_USAGE_HIZ;
> > > +
> > > + /* We may be writing depth or stencil so we need to mark the surface.
> > > + * Unfortunately, there's no way to know at this point whether the
> > depth or
> > > + * stencil tests used will actually write to the surface.
> > > + */
> > > + if (image && (image->aspects & VK_IMAGE_ASPECT_DEPTH_BIT)) {
> > > + genX(cmd_buffer_mark_image_written)(cmd_buffer, image,
> > > + VK_IMAGE_ASPECT_DEPTH_BIT,
> > > + info.hiz_usage,
> > > + info.view->base_level,
> > > + info.view->base_array_layer,
> > > + info.view->array_len);
> > > + }
> > > + if (image && (image->aspects & VK_IMAGE_ASPECT_STENCIL_BIT)) {
> > > + genX(cmd_buffer_mark_image_written)(cmd_buffer, image,
> > > + VK_IMAGE_ASPECT_STENCIL_BIT,
> > > + ISL_AUX_USAGE_NONE,
> > > + info.view->base_level,
> > > + info.view->base_array_layer,
> > > + info.view->array_len);
> > > + }
> > > }
> > >
> > >
> > > @@ -3174,6 +3208,16 @@ 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,
> > > + iview->planes[0].isl.base_
> > array_layer,
> > > + iview->planes[0].isl.array_
> > len);
> >
> > The last argument should be fb->layers right?
> >
>
> Yup. Thanks for catching that. Fixed locally.
>
>
With your local fixes applied, This patch is
Reviewed-by: Nanley Chery <nanley.g.chery at intel.com>
> > > }
> > > }
> > >
> > > --
> > > 2.5.0.400.gff86faf
> > >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
More information about the mesa-dev
mailing list