On Sep 12, 2016 1:31 PM, "Nanley Chery" <<a href="mailto:nanleychery@gmail.com">nanleychery@gmail.com</a>> 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 <<a href="mailto:nanleychery@gmail.com">nanleychery@gmail.com</a>> > > 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 = &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; > > > > + } > > > > > > 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 <<a href="mailto:nanley.g.chery@intel.com">nanley.g.chery@intel.com</a>> 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 > > > > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > > > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > >