[Mesa-dev] [PATCH 27/33] anv: Use blorp for CopyImageToBuffer
Jason Ekstrand
jason at jlekstrand.net
Mon Sep 12 19:45:51 UTC 2016
On Mon, Sep 12, 2016 at 12:30 PM, Nanley Chery <nanleychery at gmail.com>
wrote:
> On Wed, Aug 31, 2016 at 02:22:46PM -0700, Jason Ekstrand wrote:
> > ---
> > src/intel/vulkan/anv_blorp.c | 134 ++++++++++++++++++++++++++++++
> +++++++++
> > src/intel/vulkan/anv_meta_copy.c | 16 -----
> > 2 files changed, 134 insertions(+), 16 deletions(-)
> >
> > diff --git a/src/intel/vulkan/anv_blorp.c b/src/intel/vulkan/anv_blorp.c
> > index 5d715fc..a838b55 100644
> > --- a/src/intel/vulkan/anv_blorp.c
> > +++ b/src/intel/vulkan/anv_blorp.c
> > @@ -120,6 +120,38 @@ anv_device_finish_blorp(struct anv_device *device)
> > }
> >
> > static void
> > +get_blorp_surf_for_anv_buffer(struct anv_device *device,
> > + struct anv_buffer *buffer, uint64_t
> offset,
> > + uint32_t width, uint32_t height,
> > + uint32_t row_pitch, enum isl_format
> format,
> > + struct blorp_surf *blorp_surf,
> > + struct isl_surf *isl_surf)
> > +{
> > + *blorp_surf = (struct blorp_surf) {
> > + .surf = isl_surf,
> > + .addr = {
> > + .buffer = buffer->bo,
> > + .offset = buffer->offset + offset,
> > + },
> > + };
> > +
> > + isl_surf_init(&device->isl_dev, isl_surf,
> > + .dim = ISL_SURF_DIM_2D,
> > + .format = format,
> > + .width = width,
> > + .height = height,
> > + .depth = 1,
> > + .levels = 1,
> > + .array_len = 1,
> > + .samples = 1,
> > + .min_pitch = row_pitch,
>
> I don't think we want to set the min_pitch field. If ISL creates a
> surface with a pitch smaller than the required pitch, I think we'd
> want to know about it rather than silently aligning up. If it does
> get aligned up I wouldn't be confident that the HW will render or
> texture from the image correctly. Just having the assertion would
> provide such confidence whoever.
>
This is one of the places where the blorp code and the old meta code
differ. The blorp code just creates an image with the actual width and
height of the copy and pulls the row_pitch from bufferRowLength.
> > + .usage = ISL_SURF_USAGE_TEXTURE_BIT |
> > + ISL_SURF_USAGE_RENDER_TARGET_BIT,
> > + .tiling_flags = ISL_TILING_LINEAR_BIT);
> > + assert(isl_surf->row_pitch == row_pitch);
> > +}
> > +
> > +static void
> > get_blorp_surf_for_anv_image(const struct anv_image *image,
> > VkImageAspectFlags aspect,
> > struct blorp_surf *blorp_surf)
> > @@ -136,6 +168,108 @@ get_blorp_surf_for_anv_image(const struct
> anv_image *image,
> > };
> > }
> >
> > +static void
> > +copy_buffer_to_image(struct anv_cmd_buffer *cmd_buffer,
> > + struct anv_buffer *anv_buffer,
> > + struct anv_image *anv_image,
> > + uint32_t regionCount,
> > + const VkBufferImageCopy* pRegions,
> > + bool buffer_to_image)
> > +{
> > + struct blorp_batch batch;
> > + blorp_batch_init(&cmd_buffer->device->blorp, &batch, cmd_buffer);
> > +
> > + struct {
> > + struct blorp_surf surf;
> > + uint32_t level;
> > + VkOffset3D offset;
> > + } image, buffer, *src, *dst;
> > +
> > + buffer.level = 0;
> > + buffer.offset = (VkOffset3D) { 0, 0, 0 };
> > +
> > + if (buffer_to_image) {
> > + src = &buffer;
> > + dst = ℑ
> > + } else {
> > + src = ℑ
> > + dst = &buffer;
> > + }
> > +
> > + for (unsigned r = 0; r < regionCount; r++) {
> > + const VkImageAspectFlags aspect = pRegions[r].imageSubresource.
> aspectMask;
> > +
> > + get_blorp_surf_for_anv_image(anv_image, aspect, &image.surf);
> > + image.offset =
> > + anv_sanitize_image_offset(anv_image->type,
> pRegions[r].imageOffset);
> > + image.level = pRegions[r].imageSubresource.mipLevel;
> > +
> > + VkExtent3D extent =
> > + anv_sanitize_image_extent(anv_image->type,
> pRegions[r].imageExtent);
> > + if (anv_image->type != VK_IMAGE_TYPE_3D) {
> > + image.offset.z = pRegions[r].imageSubresource.baseArrayLayer;
> > + extent.depth = pRegions[r].imageSubresource.layerCount;
> > + }
>
> Always making the image 3D is a great idea.
>
> > +
> > + const enum isl_format buffer_format =
> > + anv_get_isl_format(&cmd_buffer->device->info,
> anv_image->vk_format,
> > + aspect, VK_IMAGE_TILING_LINEAR);
> > +
> > + const VkExtent3D bufferImageExtent = {
> > + .width = pRegions[r].bufferRowLength ?
> > + pRegions[r].bufferRowLength : extent.width,
> > + .height = pRegions[r].bufferImageHeight ?
> > + pRegions[r].bufferImageHeight : extent.height,
> > + };
> > +
> > + const struct isl_format_layout *buffer_fmtl =
> > + isl_format_get_layout(buffer_format);
> > +
> > + const uint32_t buffer_row_pitch =
> > + DIV_ROUND_UP(bufferImageExtent.width, buffer_fmtl->bw) *
> > + (buffer_fmtl->bpb / 8);
> > +
> > + const uint32_t buffer_layer_stride =
> > + DIV_ROUND_UP(bufferImageExtent.height, buffer_fmtl->bh) *
> > + buffer_row_pitch;
> > +
> > + struct isl_surf buffer_isl_surf;
> > + get_blorp_surf_for_anv_buffer(cmd_buffer->device,
> > + anv_buffer,
> pRegions[r].bufferOffset,
> > + extent.width, extent.height,
> > + buffer_row_pitch, buffer_format,
> > + &buffer.surf, &buffer_isl_surf);
>
> I think this would be clearer if buffer_isl_surf was a struct local to
> get_blorp_surf_for_anv_buffer(). It's not used anywhere outside of that
> function and it gets initialized from inside it.
>
That's not possible because blorp_surf has an isl_surf * not an isl_surf.
If we created the surface in get_blorp_surf_for_anv_buffer, it would go
out-of-scope. Yes, it's kind-of annoying. I fought with myself
back-and-forth for a long time as to whether blorp_surf should embed the
isl_surf or just have a pointer to one. I settled on pointer. We could
change it in the future, but until then, get_blorp_surf_for_* functions
will always have to take that annoying isl_surf parameter.
--Jason
> - Nanley
>
> > +
> > + 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,
> > + src->offset.x, src->offset.y, dst->offset.x,
> dst->offset.y,
> > + extent.width, extent.height);
> > +
> > + image.offset.z++;
> > + buffer.surf.addr.offset += buffer_layer_stride;
> > + }
> > + }
> > +
> > + blorp_batch_finish(&batch);
> > +}
> > +
> > +void anv_CmdCopyImageToBuffer(
> > + VkCommandBuffer commandBuffer,
> > + VkImage srcImage,
> > + VkImageLayout srcImageLayout,
> > + VkBuffer dstBuffer,
> > + uint32_t regionCount,
> > + const VkBufferImageCopy* pRegions)
> > +{
> > + ANV_FROM_HANDLE(anv_cmd_buffer, cmd_buffer, commandBuffer);
> > + ANV_FROM_HANDLE(anv_image, src_image, srcImage);
> > + ANV_FROM_HANDLE(anv_buffer, dst_buffer, dstBuffer);
> > +
> > + copy_buffer_to_image(cmd_buffer, dst_buffer, src_image,
> > + regionCount, pRegions, false);
> > +}
> > +
> > static bool
> > flip_coords(unsigned *src0, unsigned *src1, unsigned *dst0, unsigned
> *dst1)
> > {
> > diff --git a/src/intel/vulkan/anv_meta_copy.c
> b/src/intel/vulkan/anv_meta_copy.c
> > index 3f548e6..a17dd63 100644
> > --- a/src/intel/vulkan/anv_meta_copy.c
> > +++ b/src/intel/vulkan/anv_meta_copy.c
> > @@ -232,22 +232,6 @@ void anv_CmdCopyBufferToImage(
> > regionCount, pRegions, true);
> > }
> >
> > -void anv_CmdCopyImageToBuffer(
> > - VkCommandBuffer commandBuffer,
> > - VkImage srcImage,
> > - VkImageLayout srcImageLayout,
> > - VkBuffer destBuffer,
> > - uint32_t regionCount,
> > - const VkBufferImageCopy* pRegions)
> > -{
> > - ANV_FROM_HANDLE(anv_cmd_buffer, cmd_buffer, commandBuffer);
> > - ANV_FROM_HANDLE(anv_image, src_image, srcImage);
> > - ANV_FROM_HANDLE(anv_buffer, dst_buffer, destBuffer);
> > -
> > - meta_copy_buffer_to_image(cmd_buffer, dst_buffer, src_image,
> > - regionCount, pRegions, false);
> > -}
> > -
> > void anv_CmdCopyImage(
> > VkCommandBuffer commandBuffer,
> > VkImage srcImage,
> > --
> > 2.5.0.400.gff86faf
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160912/5fbb3e2f/attachment.html>
More information about the mesa-dev
mailing list