[Mesa-dev] [PATCH 27/33] anv: Use blorp for CopyImageToBuffer
Jason Ekstrand
jason at jlekstrand.net
Mon Sep 12 20:48:41 UTC 2016
On Sep 12, 2016 1:31 PM, "Nanley Chery" <nanleychery at gmail.com> wrote:
>
> On Mon, Sep 12, 2016 at 12:45:51PM -0700, Jason Ekstrand wrote:
> > 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.
> >
> >
>
> I forgot that bufferRowLength could be greater than the image's
> width * block_size. Using min_pitch is in fact necessary.
>
> > > > + .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.
> >
>
> Ah, got it.
>
> Patches 27 and 28 are,
>
> Reviewed-by: Nanley Chery <nanley.g.chery at intel.com>
Thanks!
> > --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/0e8b0b63/attachment-0001.html>
More information about the mesa-dev
mailing list