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

[Chad Versace]

On Fri 01 Jul 2016, Nanley Chery wrote:
> On Fri, Jul 01, 2016 at 02:24:19PM -0700, Chad Versace wrote:

> > 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?

Yes, I like that change.