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

[Chad Versace]

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

[Jason Ekstrand]

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

[Nanley Chery]

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

[Chad Versace]

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

[Nanley Chery]

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