On Fri, Jul 1, 2016 at 6:13 PM, Nanley Chery <nanleych...@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.ch...@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
