[Mesa-dev] [PATCH 2/4] anv/image: Simplify initialization of the isl_tiling

[Nanley Chery]

On Fri, Jul 01, 2016 at 02:24:19PM -0700, Chad Versace wrote:
> On Mon 27 Jun 2016, Nanley Chery wrote:
> > Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
> > ---
> >  src/intel/vulkan/anv_image.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c
> > index 77d9931..b3f5f5c 100644
> > --- a/src/intel/vulkan/anv_image.c
> > +++ b/src/intel/vulkan/anv_image.c
> > @@ -120,10 +120,6 @@ make_surface(const struct anv_device *dev,
> >        [VK_IMAGE_TYPE_3D] = ISL_SURF_DIM_3D,
> >     };
> >  
> > -   isl_tiling_flags_t tiling_flags = anv_info->isl_tiling_flags;
> > -   if (vk_info->tiling == VK_IMAGE_TILING_LINEAR)
> > -      tiling_flags = ISL_TILING_LINEAR_BIT;
> > -
> >     struct anv_surface *anv_surf = get_surface(image, aspect);
> >  
> >     image->extent = anv_sanitize_image_extent(vk_info->imageType,
> > @@ -142,7 +138,7 @@ make_surface(const struct anv_device *dev,
> >        .min_alignment = 0,
> >        .min_pitch = anv_info->stride,
> >        .usage = choose_isl_surf_usage(image->usage, aspect),
> > -      .tiling_flags = tiling_flags);
> > +      .tiling_flags = anv_info->isl_tiling_flags);
> >  
> >     /* isl_surf_init() will fail only if provided invalid input. Invalid input
> >      * is illegal in Vulkan.
> > @@ -260,7 +256,9 @@ anv_CreateImage(VkDevice device,
> >     return anv_image_create(device,
> >        &(struct anv_image_create_info) {
> >           .vk_info = pCreateInfo,
> > -         .isl_tiling_flags = ISL_TILING_ANY_MASK,
> > +         .isl_tiling_flags = (pCreateInfo->tiling == VK_IMAGE_TILING_LINEAR) ?
> > +                             ISL_TILING_LINEAR_BIT : ISL_TILING_ANY_MASK,
> > +
> >        },
> >        pAllocator,
> >        pImage);
> 
> I don't agree with this patch.
> 
> Locally, the patch look correct. But when you consider that
> anv_image_create() is public within the driver, the patch makes the code
> fragile. Pre-patch, if the caller of anv_image_create() sets
> anv_image_create_info::vk_info::tiling and leaves
> anv_image_create_info::isl_tiling_flags unset (which I believe should be
> a valid combination), then anv_image_create() correctly converts the
> VkImageTilingFlags to isl_tiling_flags. Post-patch, that's no longer the
> case; anv_image_create() ignores its VkImageTiling input.

Thanks for finding that bug.

Your description has actually pointed out an issue in the current code:
If an internal caller specifies
anv_image_create_info::vk_info::tiling = VK_IMAGE_TILING_OPTIMAL
and leaves anv_image_create_info::isl_tiling_flags unset, then
anv_image_create() ignores the VkImageTiling input and causes ISL to
fail image creation later.

To solve this problem, I think we should define ::isl_tiling_flags to be a
opt-in bit-mask which works with the requested ::vk_info::tiling to provide
more specificity on the actual desired tiling. With this in mind, we can drop
the last two hunks from the above patch and replace the first with the
following:
`
 isl_tiling_flags_t tiling_flags =
    (pCreateInfo->tiling == VK_IMAGE_TILING_LINEAR ? 
    ISL_TILING_LINEAR_BIT : ISL_TILING_ANY_MASK);
 if (anv_info->isl_tiling_flags)
    tiling_flags &= anv_info->isl_tiling_flags;
 assert (tiling_flags);
`
What do you think?

- Nanley