<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Mon, Sep 12, 2016 at 12:30 PM, Nanley Chery <<a href="mailto:nanleychery@gmail.com" target="_blank">nanleychery@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">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, </div></div>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. <div><div class="h5"></div></div></blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"> > + .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 = &image; > + } else { > + src = &image; > + 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; > + } </div></div>Always making the image 3D is a great idea. <div><div class="h5"> > + > + 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); </div></div>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. </blockquote><div> </div><div>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. </div><div>--Jason </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> - Nanley <div><div class="h5"> > + > + 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 > </div></div>> _______________________________________________ > mesa-dev mailing list > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>