On Tue, May 02, 2017 at 04:03:36PM -0700, Jason Ekstrand wrote: > 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. > >
Agreed. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev