On Thu, Apr 27, 2017 at 11:32 AM, Nanley Chery <nanleych...@gmail.com>
wrote:
> Signed-off-by: Nanley Chery <nanley.g.ch...@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.
> + if (image->aspects == VK_IMAGE_ASPECT_COLOR_BIT &&
> + (device->info.gen <= 8 || image->samples > 1)) {
> + for (uint8_t lod = 0; lod < anv_color_aux_levels(image); ++lod) {
> + anv_fill_ccs_resolve_ss(device, map +
> image->aux_surface.isl.size +
> + device->isl_dev.ss.size * lod,
> + image, lod, 0);
> + }
> + }
> +
> + anv_gem_munmap(map, image_map_size);
> }
>
> return VK_SUCCESS;
> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_
> private.h
> index ac71537e88..12531264d5 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -2019,6 +2019,11 @@ anv_color_aux_layers(const struct anv_image * const
> image,
> image->aux_surface.isl.logical_level0_px.depth >>
> miplevel);
> }
>
> +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);
> +
> /* Returns true if a HiZ-enabled depth buffer can be sampled from. */
> static inline bool
> anv_can_sample_with_hiz(const struct gen_device_info * const devinfo,
> --
> 2.12.2
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
