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

[Chad Versace]

On Wed 29 Nov 2017, Jason Ekstrand wrote:
> On Wed, Nov 29, 2017 at 2:24 PM, Chad Versace <[1]chadversary at chromium.org>
> wrote:
> 
>     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.
> 
> 
> Yes, that was a bit intentional because setting it any time other than image
> creation is racy.  If we were going to set it on import as well, we may as well
> just do that in the GL driver and call it a day.
>  
> 
>     Everything else in this patch looks correct to me, even though it makes
>     me sad.

I started writing a mini-essay in reply to this patch. Since that
reply's summary is/was

    In general, for the upcoming modifiers extension, this patch is
    incorrect. But, for the limited usecase of the pseudo WSI layer,
    everything should work correctly.

then I'm ok with landing this patch as-is in this series.

Reviewed-by: Chad Versace <chadversary at chromium.org>

But we should chat later (not while I'm making breakfast for my kid
(which is right now)) about the best way to fix the general problem,
which becomes real when VK_EXT_image_drm_format_modifier arrives.

I think I've reviewed all the patches now. If I've forgotten any, let me
know... or just push anyway.