[Mesa-dev] [PATCH 30/33] anv: Use blorp for CopyBuffer and UpdateBuffer

Nanley Chery nanleychery at gmail.com
Mon Sep 12 17:22:04 UTC 2016


On Fri, Sep 09, 2016 at 07:25:46PM -0700, Jason Ekstrand wrote:
> On Fri, Sep 9, 2016 at 5:47 PM, Nanley Chery <nanleychery at gmail.com> wrote:
> 
> > On Wed, Aug 31, 2016 at 02:22:49PM -0700, Jason Ekstrand wrote:
> > > ---
> > >  src/intel/vulkan/Makefile.sources |   1 -
> > >  src/intel/vulkan/anv_blorp.c      | 184 ++++++++++++++++++++++++++++++
> > ++++++++
> > >  src/intel/vulkan/anv_meta_copy.c  | 180 ------------------------------
> > -------
> > >  3 files changed, 184 insertions(+), 181 deletions(-)
> > >  delete mode 100644 src/intel/vulkan/anv_meta_copy.c
> > >
> > > diff --git a/src/intel/vulkan/Makefile.sources
> > b/src/intel/vulkan/Makefile.sources
> > > index 35e15f6..6c9853b 100644
> > > --- a/src/intel/vulkan/Makefile.sources
> > > +++ b/src/intel/vulkan/Makefile.sources
> > > @@ -35,7 +35,6 @@ VULKAN_FILES := \
> > >       anv_meta.h \
> > >       anv_meta_blit2d.c \
> > >       anv_meta_clear.c \
> > > -     anv_meta_copy.c \
> > >       anv_meta_resolve.c \
> > >       anv_nir.h \
> > >       anv_nir_apply_dynamic_offsets.c \
> > > diff --git a/src/intel/vulkan/anv_blorp.c b/src/intel/vulkan/anv_blorp.c
> > > index 89ff3b3..e2b6672 100644
> > > --- a/src/intel/vulkan/anv_blorp.c
> > > +++ b/src/intel/vulkan/anv_blorp.c
> > > @@ -479,3 +479,187 @@ void anv_CmdBlitImage(
> > >
> > >     blorp_batch_finish(&batch);
> > >  }
> > > +
> > > +static void
> > > +do_buffer_copy(struct blorp_batch *batch,
> > > +               struct anv_bo *src, uint64_t src_offset,
> > > +               struct anv_bo *dst, uint64_t dst_offset,
> > > +               int width, int height, int block_size)
> > > +{
> > > +   struct anv_device *device = batch->blorp->driver_ctx;
> > > +
> > > +   /* The actual format we pick doesn't matter as blorp will throw it
> > away.
> > > +    * The only thing that actually matters is the size.
> > > +    */
> > > +   enum isl_format format;
> > > +   switch (block_size) {
> > > +   case 1:  format = ISL_FORMAT_R8_UINT;              break;
> > > +   case 2:  format = ISL_FORMAT_R8G8_UINT;            break;
> > > +   case 4:  format = ISL_FORMAT_R8G8B8A8_UNORM;       break;
> > > +   case 8:  format = ISL_FORMAT_R16G16B16A16_UNORM;   break;
> > > +   case 16: format = ISL_FORMAT_R32G32B32A32_UINT;    break;
> > > +   default:
> > > +      unreachable("Not a power-of-two format size");
> > > +   }
> > > +
> > > +   struct isl_surf surf;
> > > +   isl_surf_init(&device->isl_dev, &surf,
> > > +                 .dim = ISL_SURF_DIM_2D,
> > > +                 .format = format,
> > > +                 .width = width,
> > > +                 .height = height,
> > > +                 .depth = 1,
> > > +                 .levels = 1,
> > > +                 .array_len = 1,
> > > +                 .samples = 1,
> > > +                 .usage = ISL_SURF_USAGE_TEXTURE_BIT,
> >
> > Shouldn't we also OR in ISL_SURF_USAGE_RENDER_TARGET_BIT?
> >
> 
> Yes we should.
> 
> 
> > > +                 .tiling_flags = ISL_TILING_LINEAR_BIT);
> > > +   assert(surf.row_pitch == width * block_size);
> > > +
> > > +   struct blorp_surf src_blorp_surf = {
> > > +      .surf = &surf,
> > > +      .addr = {
> > > +         .buffer = src,
> > > +         .offset = src_offset,
> > > +      },
> > > +   };
> > > +
> > > +   struct blorp_surf dst_blorp_surf = {
> > > +      .surf = &surf,
> > > +      .addr = {
> > > +         .buffer = dst,
> > > +         .offset = dst_offset,
> > > +      },
> > > +   };
> > > +
> > > +   blorp_copy(batch, &src_blorp_surf, 0, 0, &dst_blorp_surf, 0, 0,
> > > +              0, 0, 0, 0, width, height);
> > > +}
> > > +
> > > +void anv_CmdCopyBuffer(
> > > +    VkCommandBuffer                             commandBuffer,
> > > +    VkBuffer                                    srcBuffer,
> > > +    VkBuffer                                    dstBuffer,
> > > +    uint32_t                                    regionCount,
> > > +    const VkBufferCopy*                         pRegions)
> > > +{
> > > +   ANV_FROM_HANDLE(anv_cmd_buffer, cmd_buffer, commandBuffer);
> > > +   ANV_FROM_HANDLE(anv_buffer, src_buffer, srcBuffer);
> > > +   ANV_FROM_HANDLE(anv_buffer, dst_buffer, dstBuffer);
> > > +
> > > +   struct blorp_batch batch;
> > > +   blorp_batch_init(&cmd_buffer->device->blorp, &batch, cmd_buffer);
> > > +
> > > +   for (unsigned r = 0; r < regionCount; r++) {
> > > +      uint64_t src_offset = src_buffer->offset + pRegions[r].srcOffset;
> > > +      uint64_t dst_offset = dst_buffer->offset + pRegions[r].dstOffset;
> > > +      uint64_t copy_size = pRegions[r].size;
> > > +
> > > +      /* First, we compute the biggest format that can be used with the
> > > +       * given offsets and size.
> > > +       */
> > > +      int bs = 16;
> > > +
> > > +      int fs = ffs(src_offset) - 1;
> > > +      if (fs != -1)
> > > +         bs = MIN2(bs, 1 << fs);
> > > +      assert(src_offset % bs == 0);
> >
> > I think we could replace this math and some math in
> > anv_CmdUpdateBuffer() with logic that's a bit simpler:
> >
> > /* Get the greatest power-of-two multiple between a number and a
> >  * power-of-two upper-bound.
> >  */
> > #define gcm_pow2(val, max_pow2m) (1 << (ffs((val)|(max_pow2m)) - 1))
> >
> 
> That's a good idea.  I just pushed to the branch with two new patches for
> you to look at.  One which uses gcm_pow2 and one which sets the usage bits
> as requested above.  Let me know what you think.
> 

The usage bit patch looks good.

The helper function patch should update the title to say
"[...]Use gcd_pow2" instead of gcm_pow2. Thanks for correcting my 
terminology.

I'm including the diff to make review easier. Patches on the list are
appreciated.

+/**
+ * Returns the greatest common divisor of a and b that is a power of two.
+ */
+static inline unsigned
+gcd_pow2(unsigned a, unsigned b)

In anv_CmdCopyBuffer(), we pass in uint64_t's to this function. The
parameters and return type should match this type and ffsll should be
used below. I missed this case in my suggestion as well and it was a
bug present in the original version of the code.

+{
+   if (a == 0)
+      return b;

I don't think we can perform this shortcut. If a == 0 && b == 3, 3 will
be returned but it is not a power of two.  If we allow the code below to
execute instead, we'll end up with 1 which is a power of two. Note also
that if a and b equal 0, we'll return 1 << UINT_MAX. Instead, we should
assert that we can return a representable value that is also a power of
two:
    assert (a > 0 || b > 0);

+
+   unsigned a_log2 = ffs(a) - 1;
+   unsigned b_log2 = ffs(b) - 1;
+
+   /* If b == 0 then b_log2 will be UINT_MAX and the MIN2 will take a_log2 */
+   return 1 << MIN2(a_log2, b_log2);

While this isn't exactly the function I had in mind (I suppose that
would have been called gcd_pow2_clamp), this isn't a performance
critical path, so a more generic function is sufficient and may be more
reusable.

+}

There's an extra line in this hunk (compared to the similar hunk below
it):

       int bs = 16;

-      int fs = ffs(src_offset) - 1;


bs could also be declared as a uint8_t, but such a change isn't
necessary.

Nanley

> 
> >       bs = gcm_pow2(src_offset, bs);
> >       assert(src_offset % bs == 0);
> >
> > > +
> > > +      fs = ffs(dst_offset) - 1;
> > > +      if (fs != -1)
> > > +         bs = MIN2(bs, 1 << fs);
> > > +      assert(dst_offset % bs == 0);
> > > +
> > > +      fs = ffs(pRegions[r].size) - 1;
> > > +      if (fs != -1)
> > > +         bs = MIN2(bs, 1 << fs);
> > > +      assert(pRegions[r].size % bs == 0);
> > > +
> > > +      /* This is maximum possible width/height our HW can handle */
> > > +      uint64_t max_surface_dim = 1 << 14;
> > > +
> > > +      /* First, we make a bunch of max-sized copies */
> > > +      uint64_t max_copy_size = max_surface_dim * max_surface_dim * bs;
> > > +      while (copy_size >= max_copy_size) {
> > > +         do_buffer_copy(&batch, src_buffer->bo, src_offset,
> > > +                        dst_buffer->bo, dst_offset,
> > > +                        max_surface_dim, max_surface_dim, bs);
> > > +         copy_size -= max_copy_size;
> > > +         src_offset += max_copy_size;
> > > +         dst_offset += max_copy_size;
> > > +      }
> > > +
> > > +      uint64_t height = copy_size / (max_surface_dim * bs);
> > > +      assert(height < max_surface_dim);
> > > +      if (height != 0) {
> > > +         uint64_t rect_copy_size = height * max_surface_dim * bs;
> > > +         do_buffer_copy(&batch, src_buffer->bo, src_offset,
> > > +                        dst_buffer->bo, dst_offset,
> > > +                        max_surface_dim, height, bs);
> > > +         copy_size -= rect_copy_size;
> > > +         src_offset += rect_copy_size;
> > > +         dst_offset += rect_copy_size;
> > > +      }
> > > +
> > > +      if (copy_size != 0) {
> > > +         do_buffer_copy(&batch, src_buffer->bo, src_offset,
> > > +                        dst_buffer->bo, dst_offset,
> > > +                        copy_size / bs, 1, bs);
> > > +      }
> > > +   }
> > > +
> > > +   blorp_batch_finish(&batch);
> > > +}
> > > +
> > > +void anv_CmdUpdateBuffer(
> > > +    VkCommandBuffer                             commandBuffer,
> > > +    VkBuffer                                    dstBuffer,
> > > +    VkDeviceSize                                dstOffset,
> > > +    VkDeviceSize                                dataSize,
> > > +    const uint32_t*                             pData)
> > > +{
> > > +   ANV_FROM_HANDLE(anv_cmd_buffer, cmd_buffer, commandBuffer);
> > > +   ANV_FROM_HANDLE(anv_buffer, dst_buffer, dstBuffer);
> > > +
> > > +   struct blorp_batch batch;
> > > +   blorp_batch_init(&cmd_buffer->device->blorp, &batch, cmd_buffer);
> > > +
> > > +   /* We can't quite grab a full block because the state stream needs a
> > > +    * little data at the top to build its linked list.
> > > +    */
> > > +   const uint32_t max_update_size =
> > > +      cmd_buffer->device->dynamic_state_block_pool.block_size - 64;
> > > +
> > > +   assert(max_update_size < (1 << 14) * 4);
> > > +
> > > +   while (dataSize) {
> > > +      const uint32_t copy_size = MIN2(dataSize, max_update_size);
> > > +
> > > +      struct anv_state tmp_data =
> > > +         anv_cmd_buffer_alloc_dynamic_state(cmd_buffer, copy_size, 64);
> > > +
> > > +      memcpy(tmp_data.map, pData, copy_size);
> > > +
> > > +      int bs;
> > > +      if ((copy_size & 15) == 0 && (dstOffset & 15) == 0) {
> > > +         bs = 16;
> > > +      } else if ((copy_size & 7) == 0 && (dstOffset & 7) == 0) {
> > > +         bs = 8;
> > > +      } else {
> > > +         assert((copy_size & 3) == 0 && (dstOffset & 3) == 0);
> > > +         bs = 4;
> > > +      }
> >
> > We can also replace this with two calls of the macro, like so:
> >       bs = gcm_pow2(copy_size, 16);
> >       bs = gcm_pow2(dstOffset, bs);
> >
> > - Nanley
> >
> > > +
> > > +      do_buffer_copy(&batch,
> > > +                     &cmd_buffer->device->dynamic_state_block_pool.bo,
> > > +                     tmp_data.offset,
> > > +                     dst_buffer->bo, dst_buffer->offset + dstOffset,
> > > +                     copy_size / bs, 1, bs);
> > > +
> > > +      dataSize -= copy_size;
> > > +      dstOffset += copy_size;
> > > +      pData = (void *)pData + copy_size;
> > > +   }
> > > +
> > > +   blorp_batch_finish(&batch);
> > > +}
> > > diff --git a/src/intel/vulkan/anv_meta_copy.c
> > b/src/intel/vulkan/anv_meta_copy.c
> > > deleted file mode 100644
> > > index b33273e..0000000
> > > --- a/src/intel/vulkan/anv_meta_copy.c
> > > +++ /dev/null
> > > @@ -1,180 +0,0 @@
> > > -/*
> > > - * Copyright © 2016 Intel Corporation
> > > - *
> > > - * Permission is hereby granted, free of charge, to any person
> > obtaining a
> > > - * copy of this software and associated documentation files (the
> > "Software"),
> > > - * to deal in the Software without restriction, including without
> > limitation
> > > - * the rights to use, copy, modify, merge, publish, distribute,
> > sublicense,
> > > - * and/or sell copies of the Software, and to permit persons to whom the
> > > - * Software is furnished to do so, subject to the following conditions:
> > > - *
> > > - * The above copyright notice and this permission notice (including the
> > next
> > > - * paragraph) shall be included in all copies or substantial portions
> > of the
> > > - * Software.
> > > - *
> > > - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > EXPRESS OR
> > > - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY,
> > > - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT
> > SHALL
> > > - * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> > OTHER
> > > - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > ARISING
> > > - * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > DEALINGS
> > > - * IN THE SOFTWARE.
> > > - */
> > > -
> > > -#include "anv_meta.h"
> > > -
> > > -static void
> > > -do_buffer_copy(struct anv_cmd_buffer *cmd_buffer,
> > > -               struct anv_bo *src, uint64_t src_offset,
> > > -               struct anv_bo *dest, uint64_t dest_offset,
> > > -               int width, int height, int bs)
> > > -{
> > > -   struct anv_meta_blit2d_surf b_src = {
> > > -      .bo = src,
> > > -      .tiling = ISL_TILING_LINEAR,
> > > -      .base_offset = src_offset,
> > > -      .bs = bs,
> > > -      .pitch = width * bs,
> > > -   };
> > > -   struct anv_meta_blit2d_surf b_dst = {
> > > -      .bo = dest,
> > > -      .tiling = ISL_TILING_LINEAR,
> > > -      .base_offset = dest_offset,
> > > -      .bs = bs,
> > > -      .pitch = width * bs,
> > > -   };
> > > -   struct anv_meta_blit2d_rect rect = {
> > > -      .width = width,
> > > -      .height = height,
> > > -   };
> > > -   anv_meta_blit2d(cmd_buffer, &b_src, &b_dst, 1, &rect);
> > > -}
> > > -
> > > -void anv_CmdCopyBuffer(
> > > -    VkCommandBuffer                             commandBuffer,
> > > -    VkBuffer                                    srcBuffer,
> > > -    VkBuffer                                    destBuffer,
> > > -    uint32_t                                    regionCount,
> > > -    const VkBufferCopy*                         pRegions)
> > > -{
> > > -   ANV_FROM_HANDLE(anv_cmd_buffer, cmd_buffer, commandBuffer);
> > > -   ANV_FROM_HANDLE(anv_buffer, src_buffer, srcBuffer);
> > > -   ANV_FROM_HANDLE(anv_buffer, dest_buffer, destBuffer);
> > > -
> > > -   struct anv_meta_saved_state saved_state;
> > > -
> > > -   anv_meta_begin_blit2d(cmd_buffer, &saved_state);
> > > -
> > > -   for (unsigned r = 0; r < regionCount; r++) {
> > > -      uint64_t src_offset = src_buffer->offset + pRegions[r].srcOffset;
> > > -      uint64_t dest_offset = dest_buffer->offset +
> > pRegions[r].dstOffset;
> > > -      uint64_t copy_size = pRegions[r].size;
> > > -
> > > -      /* First, we compute the biggest format that can be used with the
> > > -       * given offsets and size.
> > > -       */
> > > -      int bs = 16;
> > > -
> > > -      int fs = ffs(src_offset) - 1;
> > > -      if (fs != -1)
> > > -         bs = MIN2(bs, 1 << fs);
> > > -      assert(src_offset % bs == 0);
> > > -
> > > -      fs = ffs(dest_offset) - 1;
> > > -      if (fs != -1)
> > > -         bs = MIN2(bs, 1 << fs);
> > > -      assert(dest_offset % bs == 0);
> > > -
> > > -      fs = ffs(pRegions[r].size) - 1;
> > > -      if (fs != -1)
> > > -         bs = MIN2(bs, 1 << fs);
> > > -      assert(pRegions[r].size % bs == 0);
> > > -
> > > -      /* This is maximum possible width/height our HW can handle */
> > > -      uint64_t max_surface_dim = 1 << 14;
> > > -
> > > -      /* First, we make a bunch of max-sized copies */
> > > -      uint64_t max_copy_size = max_surface_dim * max_surface_dim * bs;
> > > -      while (copy_size >= max_copy_size) {
> > > -         do_buffer_copy(cmd_buffer, src_buffer->bo, src_offset,
> > > -                        dest_buffer->bo, dest_offset,
> > > -                        max_surface_dim, max_surface_dim, bs);
> > > -         copy_size -= max_copy_size;
> > > -         src_offset += max_copy_size;
> > > -         dest_offset += max_copy_size;
> > > -      }
> > > -
> > > -      uint64_t height = copy_size / (max_surface_dim * bs);
> > > -      assert(height < max_surface_dim);
> > > -      if (height != 0) {
> > > -         uint64_t rect_copy_size = height * max_surface_dim * bs;
> > > -         do_buffer_copy(cmd_buffer, src_buffer->bo, src_offset,
> > > -                        dest_buffer->bo, dest_offset,
> > > -                        max_surface_dim, height, bs);
> > > -         copy_size -= rect_copy_size;
> > > -         src_offset += rect_copy_size;
> > > -         dest_offset += rect_copy_size;
> > > -      }
> > > -
> > > -      if (copy_size != 0) {
> > > -         do_buffer_copy(cmd_buffer, src_buffer->bo, src_offset,
> > > -                        dest_buffer->bo, dest_offset,
> > > -                        copy_size / bs, 1, bs);
> > > -      }
> > > -   }
> > > -
> > > -   anv_meta_end_blit2d(cmd_buffer, &saved_state);
> > > -}
> > > -
> > > -void anv_CmdUpdateBuffer(
> > > -    VkCommandBuffer                             commandBuffer,
> > > -    VkBuffer                                    dstBuffer,
> > > -    VkDeviceSize                                dstOffset,
> > > -    VkDeviceSize                                dataSize,
> > > -    const uint32_t*                             pData)
> > > -{
> > > -   ANV_FROM_HANDLE(anv_cmd_buffer, cmd_buffer, commandBuffer);
> > > -   ANV_FROM_HANDLE(anv_buffer, dst_buffer, dstBuffer);
> > > -   struct anv_meta_saved_state saved_state;
> > > -
> > > -   anv_meta_begin_blit2d(cmd_buffer, &saved_state);
> > > -
> > > -   /* We can't quite grab a full block because the state stream needs a
> > > -    * little data at the top to build its linked list.
> > > -    */
> > > -   const uint32_t max_update_size =
> > > -      cmd_buffer->device->dynamic_state_block_pool.block_size - 64;
> > > -
> > > -   assert(max_update_size < (1 << 14) * 4);
> > > -
> > > -   while (dataSize) {
> > > -      const uint32_t copy_size = MIN2(dataSize, max_update_size);
> > > -
> > > -      struct anv_state tmp_data =
> > > -         anv_cmd_buffer_alloc_dynamic_state(cmd_buffer, copy_size, 64);
> > > -
> > > -      memcpy(tmp_data.map, pData, copy_size);
> > > -
> > > -      int bs;
> > > -      if ((copy_size & 15) == 0 && (dstOffset & 15) == 0) {
> > > -         bs = 16;
> > > -      } else if ((copy_size & 7) == 0 && (dstOffset & 7) == 0) {
> > > -         bs = 8;
> > > -      } else {
> > > -         assert((copy_size & 3) == 0 && (dstOffset & 3) == 0);
> > > -         bs = 4;
> > > -      }
> > > -
> > > -      do_buffer_copy(cmd_buffer,
> > > -                     &cmd_buffer->device->dynamic_state_block_pool.bo,
> > > -                     tmp_data.offset,
> > > -                     dst_buffer->bo, dst_buffer->offset + dstOffset,
> > > -                     copy_size / bs, 1, bs);
> > > -
> > > -      dataSize -= copy_size;
> > > -      dstOffset += copy_size;
> > > -      pData = (void *)pData + copy_size;
> > > -   }
> > > -
> > > -   anv_meta_end_blit2d(cmd_buffer, &saved_state);
> > > -}
> > > --
> > > 2.5.0.400.gff86faf
> > >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >


More information about the mesa-dev mailing list