<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Mar 6, 2017 at 10:18 AM, Chad Versace <<a href="mailto:chadversary@chromium.org" target="_blank">chadversary@chromium.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Creation of hiz, ccs, and mcs surfaces was encoded by a giant 'if' tree at the tail of make_surface(). This patch extracts that 'if' tree into the new functions: make_hiz_surface_maybe() make_ccs_surface_maybe() make_mcs_surface_maybe() </blockquote><div> </div><div>How about "try_make_hiz_surface"? _maybe doesn't sit right with me. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> For clarity, also rename make_surface() to make_main_surface(). --- src/intel/vulkan/anv_image.c | 175 ++++++++++++++++++++++++++----------------- 1 file changed, 107 insertions(+), 68 deletions(-) diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c index 8f4fbd56a16..52a126fe995 100644 --- a/src/intel/vulkan/anv_image.c +++ b/src/intel/vulkan/anv_image.c @@ -131,6 +131,106 @@ add_surface(struct anv_image *image, struct anv_surface *surf) image->alignment = MAX2(image->alignment, surf->isl.alignment); } +static void +make_hiz_surface_maybe(const struct anv_device *dev, + const VkImageCreateInfo *base_info, + struct anv_image *image) +{ + bool ok; + + assert(image->aux_surface.isl.size == 0); + assert(!(image->usage & VK_IMAGE_USAGE_STORAGE_BIT)); + + /* Allow the user to control HiZ enabling through environment variables. + * Disable by default on gen7 because resolves are not currently + * implemented pre-BDW. + */ + if (!(image->usage & VK_IMAGE_USAGE_DEPTH_STENCIL_ATTACHMENT_BIT)) { + /* It will never be used as an attachment, HiZ is pointless. */ + } else if (!env_var_as_boolean("INTEL_VK_HIZ", dev->info.gen >= 8)) { + anv_finishme("Implement gen7 HiZ"); + } else if (base_info->mipLevels > 1) { + anv_finishme("Test multi-LOD HiZ"); + } else if (base_info->arrayLayers > 1) { + anv_finishme("Implement multi-arrayLayer HiZ clears and resolves"); + } else if (dev->info.gen == 8 && base_info->samples > 1) { + anv_finishme("Test gen8 multisampled HiZ"); + } else { + ok = isl_surf_get_hiz_surf(&dev->isl_dev, &image->depth_surface.isl, + &image->aux_surface.isl); + assert(ok); + add_surface(image, &image->aux_surface); + image->aux_usage = ISL_AUX_USAGE_HIZ; + } +} + +static void +make_ccs_surface_maybe(const struct anv_device *dev, + const VkImageCreateInfo *base_info, + struct anv_image *image) +{ + bool ok; + + assert(image->aux_surface.isl.size == 0); + + if (unlikely(INTEL_DEBUG & DEBUG_NO_RBC)) + return; + + ok = isl_surf_get_ccs_surf(&dev->isl_dev, &image->color_surface.isl, + &image->aux_surface.isl); + if (!ok) + return; + + add_surface(image, &image->aux_surface); + + /* For images created without MUTABLE_FORMAT_BIT set, we know that they will + * always be used with the original format. In particular, they will always + * be used with a format that supports color compression. If it's never + * used as a storage image, then it will only be used through the sampler or + * the as a render target. This means that it's safe to just leave + * compression on at all times for these formats. + */ + if (!(base_info->usage & VK_IMAGE_USAGE_STORAGE_BIT) && + !(base_info->flags & VK_IMAGE_CREATE_MUTABLE_FORMAT_BIT) && + isl_format_supports_ccs_e(&dev->info, image->color_surface.isl.format)) { + image->aux_usage = ISL_AUX_USAGE_CCS_E; + } +} + +static void +make_mcs_surface_maybe(const struct anv_device *dev, + const VkImageCreateInfo *base_info, + struct anv_image *image) +{ + bool ok; + + assert(image->aux_surface.isl.size == 0); + assert(!(base_info->usage & VK_IMAGE_USAGE_STORAGE_BIT)); + + ok = isl_surf_get_mcs_surf(&dev->isl_dev, &image->color_surface.isl, + &image->aux_surface.isl); + if (!ok) + return; + + add_surface(image, &image->aux_surface); + image->aux_usage = ISL_AUX_USAGE_MCS; +} + +static void +make_aux_surface_maybe(const struct anv_device *dev, + const VkImageCreateInfo *base_info, + VkImageAspectFlags aspect, + struct anv_image *image) </blockquote><div> </div><div>This one could probably get inlined below but I think it also makes sense on its own or as part of make_surface. Whatever... </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> +{ + if (aspect == VK_IMAGE_ASPECT_DEPTH_BIT) { + make_hiz_surface_maybe(dev, base_info, image); + } else if (aspect == VK_IMAGE_ASPECT_COLOR_BIT && base_info->samples == 1) { + make_ccs_surface_maybe(dev, base_info, image); + } else if (aspect == VK_IMAGE_ASPECT_COLOR_BIT && base_info->samples > 1) { + make_mcs_surface_maybe(dev, base_info, image); + } +} + /** * Initialize the anv_image::*_surface selected by \a aspect. Then update the * image's memory requirements (that is, the image's size and alignment). @@ -138,10 +238,10 @@ add_surface(struct anv_image *image, struct anv_surface *surf) * Exactly one bit must be set in \a aspect. */ static void -make_surface(const struct anv_device *dev, - struct anv_image *image, - const struct anv_image_create_info *anv_info, - VkImageAspectFlags aspect) +make_main_surface(const struct anv_device *dev, + const struct anv_image_create_info *anv_info, + VkImageAspectFlags aspect, + struct anv_image *image) { const VkImageCreateInfo *base_info = anv_info->vk_info; bool ok UNUSED; @@ -182,69 +282,6 @@ make_surface(const struct anv_device *dev, assert(ok); add_surface(image, anv_surf); - - /* Add a HiZ surface to a depth buffer that will be used for rendering. - */ - if (aspect == VK_IMAGE_ASPECT_DEPTH_BIT) { - /* We don't advertise that depth buffers could be used as storage - * images. - */ - assert(!(image->usage & VK_IMAGE_USAGE_STORAGE_BIT)); - - /* Allow the user to control HiZ enabling. Disable by default on gen7 - * because resolves are not currently implemented pre-BDW. - */ - if (!(image->usage & VK_IMAGE_USAGE_DEPTH_STENCIL_ATTACHMENT_BIT)) { - /* It will never be used as an attachment, HiZ is pointless. */ - } else if (!env_var_as_boolean("INTEL_VK_HIZ", dev->info.gen >= 8)) { - anv_finishme("Implement gen7 HiZ"); - } else if (base_info->mipLevels > 1) { - anv_finishme("Test multi-LOD HiZ"); - } else if (base_info->arrayLayers > 1) { - anv_finishme("Implement multi-arrayLayer HiZ clears and resolves"); - } else if (dev->info.gen == 8 && base_info->samples > 1) { - anv_finishme("Test gen8 multisampled HiZ"); - } else { - assert(image->aux_surface.isl.size == 0); - ok = isl_surf_get_hiz_surf(&dev->isl_dev, &image->depth_surface.isl, - &image->aux_surface.isl); - assert(ok); - add_surface(image, &image->aux_surface); - image->aux_usage = ISL_AUX_USAGE_HIZ; - } - } else if (aspect == VK_IMAGE_ASPECT_COLOR_BIT && base_info->samples == 1) { - if (!unlikely(INTEL_DEBUG & DEBUG_NO_RBC)) { - assert(image->aux_surface.isl.size == 0); - ok = isl_surf_get_ccs_surf(&dev->isl_dev, &anv_surf->isl, - &image->aux_surface.isl); - if (ok) { - add_surface(image, &image->aux_surface); - - /* For images created without MUTABLE_FORMAT_BIT set, we know that - * they will always be used with the original format. In - * particular, they will always be used with a format that - * supports color compression. If it's never used as a storage - * image, then it will only be used through the sampler or the as - * a render target. This means that it's safe to just leave - * compression on at all times for these formats. - */ - if (!(base_info->usage & VK_IMAGE_USAGE_STORAGE_BIT) && - !(base_info->flags & VK_IMAGE_CREATE_MUTABLE_FORMAT_BIT) && - isl_format_supports_ccs_e(&dev->info, format)) { - image->aux_usage = ISL_AUX_USAGE_CCS_E; - } - } - } - } else if (aspect == VK_IMAGE_ASPECT_COLOR_BIT && base_info->samples > 1) { - assert(image->aux_surface.isl.size == 0); - assert(!(base_info->usage & VK_IMAGE_USAGE_STORAGE_BIT)); - ok = isl_surf_get_mcs_surf(&dev->isl_dev, &anv_surf->isl, - &image->aux_surface.isl); - if (ok) { - add_surface(image, &image->aux_surface); - image->aux_usage = ISL_AUX_USAGE_MCS; - } - } } VkResult @@ -285,7 +322,9 @@ anv_image_create(VkDevice _device, uint32_t b; for_each_bit(b, image->aspects) { - make_surface(device, image, anv_info, (1 << b)); + VkImageAspectFlagBits aspect = 1 << b; + make_main_surface(device, anv_info, aspect, image); + make_aux_surface_maybe(device, base_info, aspect, image); } *pImage = anv_image_to_handle(image); -- 2.12.0 _______________________________________________ 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" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>