[Mesa-dev] [PATCH 07/22] anv/image: Initialize the clear values buffer

[Nanley Chery]

On Tue, May 02, 2017 at 04:03:36PM -0700, Jason Ekstrand wrote:
> On Thu, Apr 27, 2017 at 11:32 AM, Nanley Chery <nanleychery at gmail.com>
> wrote:
> 
> > Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
> > ---
> >  src/intel/vulkan/anv_image.c   | 75 ++++++++++++++++++++++++++++++
> > +++++-------
> >  src/intel/vulkan/anv_private.h |  5 +++
> >  2 files changed, 69 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c
> > index 9f3eb52a37..751f2d6026 100644
> > --- a/src/intel/vulkan/anv_image.c
> > +++ b/src/intel/vulkan/anv_image.c
> > @@ -179,6 +179,37 @@ add_clear_value_buffer(struct anv_image * const image,
> >     image->size += device->isl_dev.ss.size * anv_color_aux_levels(image);
> >  }
> >
> > +/* Populates a buffer with a surface state object that can be used for
> > + * resolving the specified subresource of a CCS buffer.
> > + */
> > +void
> > +anv_fill_ccs_resolve_ss(const struct anv_device * const device,
> > +                        void * const data, const struct anv_image * const
> > image,
> > +                        const uint8_t level, const uint32_t layer)
> > +{
> > +   assert(device && image && data);
> > +
> > +   /* The image subresource must have a color auxiliary buffer. */
> > +   assert(level < anv_color_aux_levels(image));
> > +   assert(layer < anv_color_aux_layers(image, level));
> > +
> > +   isl_surf_fill_state(&device->isl_dev, data,
> > +                       .surf = &image->color_surface.isl,
> > +                       .view = &(struct isl_view) {
> > +                           .usage = ISL_SURF_USAGE_RENDER_TARGET_BIT,
> > +                           .format = image->color_surface.isl.format,
> > +                           .swizzle = ISL_SWIZZLE_IDENTITY,
> > +                           .base_level = level,
> > +                           .levels = 1,
> > +                           .base_array_layer = layer,
> > +                           .array_len = 1,
> > +                        },
> > +                       .aux_surf = &image->aux_surface.isl,
> > +                       .aux_usage = image->aux_usage ==
> > ISL_AUX_USAGE_NONE ?
> > +                                    ISL_AUX_USAGE_CCS_D :
> > image->aux_usage,
> > +                       .mocs = device->default_mocs);
> > +}
> > +
> >  /**
> >   * Initialize the anv_image::*_surface selected by \a aspect. Then update
> > the
> >   * image's memory requirements (that is, the image's size and alignment).
> > @@ -411,6 +442,9 @@ VkResult anv_BindImageMemory(
> >     image->bo = &mem->bo;
> >     image->offset = memoryOffset;
> >
> > +   /* The data after the main surface must be initialized for various
> > +    * reasons.
> > +    */
> >     if (image->aux_surface.isl.size > 0) {
> >
> >        /* The offset must be a multiple of 4K or else the anv_gem_mmap call
> > @@ -418,16 +452,11 @@ VkResult anv_BindImageMemory(
> >         */
> >        assert((image->offset + image->aux_surface.offset) % 4096 == 0);
> >
> > -      /* Auxiliary surfaces need to have their memory cleared to 0 before
> > they
> > -       * can be used.  For CCS surfaces, this puts them in the "resolved"
> > -       * state so they can be used with CCS enabled before we ever touch
> > it
> > -       * from the GPU.  For HiZ, we need something valid or else we may
> > get
> > -       * GPU hangs on some hardware and 0 works fine.
> > -       */
> > -      void *map = anv_gem_mmap(device, image->bo->gem_handle,
> > -                               image->offset + image->aux_surface.offset,
> > -                               image->aux_surface.isl.size,
> > -                               device->info.has_llc ? 0 : I915_MMAP_WC);
> > +      const uint32_t image_map_size = image->size -
> > image->aux_surface.offset;
> > +      void * const map = anv_gem_mmap(device, image->bo->gem_handle,
> > +                                      image->offset +
> > image->aux_surface.offset,
> > +                                      image_map_size,
> > +                                      device->info.has_llc ? 0 :
> > I915_MMAP_WC);
> >
> >        /* If anv_gem_mmap returns NULL, it's likely that the kernel was
> >         * not able to find space on the host to create a proper mapping.
> > @@ -435,9 +464,33 @@ VkResult anv_BindImageMemory(
> >        if (map == NULL)
> >           return vk_error(VK_ERROR_OUT_OF_HOST_MEMORY);
> >
> > +      /* Auxiliary surfaces need to have their memory cleared to 0 before
> > they
> > +       * can be used.  For CCS surfaces, this puts them in the "resolved"
> > +       * state so they can be used with CCS enabled before we ever touch
> > it
> > +       * from the GPU.  For HiZ, we need something valid or else we may
> > get
> > +       * GPU hangs on some hardware and 0 works fine.
> > +       */
> >        memset(map, 0, image->aux_surface.isl.size);
> >
> > -      anv_gem_munmap(map, image->aux_surface.isl.size);
> > +      /* For color auxiliary surfaces, the clear values buffer must be
> > +       * initialized. This is because a render pass attachment's loadOp
> > may be
> > +       * LOAD_OP_LOAD, triggering a GPU memcpy from the clear values
> > buffer
> > +       * into the surface state object. Pre-SKL, the dword containing the
> > clear
> > +       * values also contains other fields, so we need to initialize those
> > +       * fields to match the values for a color attachment. On SKL+, the
> > MCS
> > +       * surface state only allows 1/0 clear colors. Using the fill
> > function
> > +       * for a CCS resolve state also gives the desired result for MCS
> > images.
> > +       */
> >
> 
> Ugh... So, I now have a good argument for not storing full surface states
> but I don't really like it....
> 
> Short version: We really badly need to stop initializing things from the
> CPU in BindImageMemory.
> 
> Longer version: Vulkan allows the client to alias memory.  In other words,
> they can have, for instance a depth image and a color image bound to
> overlapping memory locations so long as they transition things properly.
> In particular, if they most recently used it as a color image and they want
> to use it as a depth image, they have to transition from UNDEFINED to
> whatever layout they want.  Then, when they want to go back to using the
> depth image, they transition it from UNDEFINED to whatever.  This may sound
> crazy but it can allow an application to significantly reduce its memory
> footprint if it has temporary images that it uses in its rendering pipeline
> but never at the same time.  Today, thanks to memset hacks, we don't
> support this properly.
> 
> We really need to switch depth and color over to doing the initialization
> during the UNDEFINED -> whatever layout transition instead of the memset.
> For the clear color values, this means that we need to either initialize
> the whole thing from the GPU on the UNDEFINED transition or else we need to
> make sure that we don't store anything important in the clear color values
> other than the clear color itself.  It should be fairly easy to use the CS
> ALU to mask the clear color and OR it into the surface state packet.
> 
> 

Agreed.