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. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
