<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 1, 2016 at 6:13 PM, Nanley Chery <span dir="ltr"><<a href="mailto:nanleychery@gmail.com" target="_blank">nanleychery@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Fri, Jul 01, 2016 at 02:24:19PM -0700, Chad Versace wrote:<br>
> On Mon 27 Jun 2016, Nanley Chery wrote:<br>
> > Signed-off-by: Nanley Chery <<a href="mailto:nanley.g.chery@intel.com">nanley.g.chery@intel.com</a>><br>
> > ---<br>
> >  src/intel/vulkan/anv_image.c | 10 ++++------<br>
> >  1 file changed, 4 insertions(+), 6 deletions(-)<br>
> ><br>
> > diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c<br>
> > index 77d9931..b3f5f5c 100644<br>
> > --- a/src/intel/vulkan/anv_image.c<br>
> > +++ b/src/intel/vulkan/anv_image.c<br>
> > @@ -120,10 +120,6 @@ make_surface(const struct anv_device *dev,<br>
> >        [VK_IMAGE_TYPE_3D] = ISL_SURF_DIM_3D,<br>
> >     };<br>
> ><br>
> > -   isl_tiling_flags_t tiling_flags = anv_info->isl_tiling_flags;<br>
> > -   if (vk_info->tiling == VK_IMAGE_TILING_LINEAR)<br>
> > -      tiling_flags = ISL_TILING_LINEAR_BIT;<br>
> > -<br>
> >     struct anv_surface *anv_surf = get_surface(image, aspect);<br>
> ><br>
> >     image->extent = anv_sanitize_image_extent(vk_info->imageType,<br>
> > @@ -142,7 +138,7 @@ make_surface(const struct anv_device *dev,<br>
> >        .min_alignment = 0,<br>
> >        .min_pitch = anv_info->stride,<br>
> >        .usage = choose_isl_surf_usage(image->usage, aspect),<br>
> > -      .tiling_flags = tiling_flags);<br>
> > +      .tiling_flags = anv_info->isl_tiling_flags);<br>
> ><br>
> >     /* isl_surf_init() will fail only if provided invalid input. Invalid input<br>
> >      * is illegal in Vulkan.<br>
> > @@ -260,7 +256,9 @@ anv_CreateImage(VkDevice device,<br>
> >     return anv_image_create(device,<br>
> >        &(struct anv_image_create_info) {<br>
> >           .vk_info = pCreateInfo,<br>
> > -         .isl_tiling_flags = ISL_TILING_ANY_MASK,<br>
> > +         .isl_tiling_flags = (pCreateInfo->tiling == VK_IMAGE_TILING_LINEAR) ?<br>
> > +                             ISL_TILING_LINEAR_BIT : ISL_TILING_ANY_MASK,<br>
> > +<br>
> >        },<br>
> >        pAllocator,<br>
> >        pImage);<br>
><br>
> I don't agree with this patch.<br>
><br>
> Locally, the patch look correct. But when you consider that<br>
> anv_image_create() is public within the driver, the patch makes the code<br>
> fragile. Pre-patch, if the caller of anv_image_create() sets<br>
> anv_image_create_info::vk_info::tiling and leaves<br>
> anv_image_create_info::isl_tiling_flags unset (which I believe should be<br>
> a valid combination), then anv_image_create() correctly converts the<br>
> VkImageTilingFlags to isl_tiling_flags. Post-patch, that's no longer the<br>
> case; anv_image_create() ignores its VkImageTiling input.<br>
<br>
</div></div>Thanks for finding that bug.<br>
<br>
Your description has actually pointed out an issue in the current code:<br>
If an internal caller specifies<br>
anv_image_create_info::vk_info::tiling = VK_IMAGE_TILING_OPTIMAL<br>
and leaves anv_image_create_info::isl_tiling_flags unset, then<br>
anv_image_create() ignores the VkImageTiling input and causes ISL to<br>
fail image creation later.<br>
<br>
To solve this problem, I think we should define ::isl_tiling_flags to be a<br>
opt-in bit-mask which works with the requested ::vk_info::tiling to provide<br>
more specificity on the actual desired tiling. With this in mind, we can drop<br>
the last two hunks from the above patch and replace the first with the<br>
following:<br>
`<br>
 isl_tiling_flags_t tiling_flags =<br>
    (pCreateInfo->tiling == VK_IMAGE_TILING_LINEAR ?<br>
    ISL_TILING_LINEAR_BIT : ISL_TILING_ANY_MASK);<br>
 if (anv_info->isl_tiling_flags)<br>
    tiling_flags &= anv_info->isl_tiling_flags;<br>
 assert (tiling_flags);<br></blockquote><div><br></div><div>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.<br><br></div><div>--Jason<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
`<br>
What do you think?<br>
<span class="HOEnZb"><font color="#888888"><br>
- Nanley<br>
</font></span><div class="HOEnZb"><div class="h5">_______________________________________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
</div></div></blockquote></div><br></div></div>