[Mesa-dev] [PATCH v2 09/32] anv: Require a dedicated allocation for modified images

[Chad Versace]

On Tue 28 Nov 2017, Jason Ekstrand wrote:
> This lets us set the BO tiling when we allocate the memory.  This is
> required for GL to work properly.
> ---
>  src/intel/vulkan/anv_device.c | 53 +++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 49 insertions(+), 4 deletions(-)
> 
> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
> index df929e4..d82d1f7 100644
> --- a/src/intel/vulkan/anv_device.c
> +++ b/src/intel/vulkan/anv_device.c
> @@ -29,6 +29,7 @@
>  #include <unistd.h>
>  #include <fcntl.h>
>  #include <xf86drm.h>
> +#include <drm_fourcc.h>
>  
>  #include "anv_private.h"
>  #include "util/strtod.h"
> @@ -1612,6 +1613,40 @@ VkResult anv_AllocateMemory(
>                                    &mem->bo);
>        if (result != VK_SUCCESS)
>           goto fail;
> +
> +      const VkMemoryDedicatedAllocateInfoKHR *dedicated_info =
> +         vk_find_struct_const(pAllocateInfo->pNext, MEMORY_DEDICATED_ALLOCATE_INFO_KHR);
> +      if (dedicated_info && dedicated_info->image != VK_NULL_HANDLE) {
> +         ANV_FROM_HANDLE(anv_image, image, dedicated_info->image);
> +
> +         /* For images using modifiers, we require a dedicated allocation
> +          * and we set the BO tiling to match the tiling of the underlying
> +          * modifier.  This is a bit unfortunate as this is completely
> +          * pointless for Vulkan.  However, GL needs to be able to map things
> +          * so it needs the tiling to be set.  The only way to do this in a
> +          * non-racy way is to set the tiling in the creator of the BO so that
> +          * makes it our job.
> +          *
> +          * One of these days, once the GL driver learns to not map things
> +          * through the GTT in random places, we can drop this and start
> +          * allowing multiple modified images in the same BO.
> +          */
> +         if (image->drm_format_mod != DRM_FORMAT_MOD_INVALID) {
> +            assert(isl_drm_modifier_get_info(image->drm_format_mod)->tiling ==
> +                   image->planes[0].surface.isl.tiling);
> +            const uint32_t i915_tiling =
> +               isl_tiling_to_i915_tiling(image->planes[0].surface.isl.tiling);
> +            int ret = anv_gem_set_tiling(device, mem->bo->gem_handle,
> +                                         image->planes[0].surface.isl.row_pitch,
> +                                         i915_tiling);
> +            if (ret) {
> +               anv_bo_cache_release(device, &device->bo_cache, mem->bo);
> +               return vk_errorf(device->instance, NULL,
> +                                VK_ERROR_OUT_OF_DEVICE_MEMORY,
> +                                "failed to set BO tiling: %m");
> +            }
> +         }
> +      }
>     }

This hunk of code is run only for non-imported memory. It's in the
'else' branch of `if (fd_info && fd_info->handleTypes)`. I believe it
needs to be run in both branches. Otherwise, if an app creates
a dma_buf, imports it into Vulkan, binds the image to the dma_buf, and
afterwards imports the same dma_buf into GL as an EGLImage, then
intel_create_image_from_fds_common() will fail to discover the bo's
tiling.

Everything else in this patch looks correct to me, even though it makes
me sad.