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

[Jason Ekstrand]

On Fri, Jul 1, 2016 at 6:13 PM, Nanley Chery <nanleychery at gmail.com> wrote:

> 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);
>

I think I like that.  The assert ensures that you don't try and combine
VK_IMAGE_TILING_LINEAR with ISL bits that contradict it.  On the other
hand, it does let you do VK_IMAGE_TILING_OPTIMAL with ISL_TILING_LINEAR_BIT
which I think is ok.

--Jason


> `
> What do you think?
>
> - Nanley
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160701/973b34e3/attachment-0001.html>