On Sat, Jan 13, 2018 at 9:55 AM, Jason Ekstrand <ja...@jlekstrand.net>
wrote:
> On Wed, Dec 13, 2017 at 4:46 PM, Nanley Chery <nanleych...@gmail.com>
> 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.
> -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
>> > 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
