Re: [Mesa-dev] [PATCH 2/4] anv/image: Simplify initialization of the isl_tiling
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
Re: [Mesa-dev] [PATCH 2/4] anv/image: Simplify initialization of the isl_tiling
On Fri, Jul 1, 2016 at 6:13 PM, Nanley Chery 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 > > > --- > > > 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
Re: [Mesa-dev] [PATCH 2/4] anv/image: Simplify initialization of the isl_tiling
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 > > --- > > 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); ` What do you think? - Nanley ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/4] anv/image: Simplify initialization of the isl_tiling
On Mon 27 Jun 2016, Nanley Chery wrote: > Signed-off-by: Nanley Chery > --- > 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. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 2/4] anv/image: Simplify initialization of the isl_tiling
Signed-off-by: Nanley Chery --- 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); -- 2.9.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev