[Mesa-dev] [PATCH] anv/genX: Solve the anv_CmdClearColorImage crash issue

[Xu, Randy]

Thanks,
Randy

> -----Original Message-----
> From: Landwerlin, Lionel G
> Sent: Monday, March 27, 2017 10:06 PM
> To: Xu, Randy <randy.xu at intel.com>; mesa-dev at lists.freedesktop.org
> Cc: mesa-stable at lists.freedesktop.org
> Subject: Re: [Mesa-dev] [PATCH] anv/genX: Solve the
> anv_CmdClearColorImage crash issue
> 
> On 27/03/17 12:51, Xu, Randy wrote:
> > Thanks, Lionel
> >
> > See my comments below
> >
> >
> > Thanks,
> > Randy
> >
> >> -----Original Message-----
> >> From: Landwerlin, Lionel G
> >> Sent: Monday, March 27, 2017 5:14 PM
> >> To: Xu, Randy <randy.xu at intel.com>; mesa-dev at lists.freedesktop.org
> >> Subject: Re: [Mesa-dev] [PATCH] anv/genX: Solve the
> >> anv_CmdClearColorImage crash issue
> >>
> >> Hi Randy,
> >>
> >> This patch looks good to me.
> >>
> >> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> >> Cc: "17.0 13.0" <mesa-stable at lists.freedesktop.org>
> >>
> >> Are you planning to send a follow up patch to fix the remaining
> >> layoutCount accesses too?
> > I guess you means the layerCount access I checked the code, there are
> > two kinds of layerCount access, as member of VkImageSubresourceLayers
> or VkImageSubresourceRange.
> > Only the layerCount in VkImageSubresourceRange can be
> VK_REMAINING_ARRAY_LAYERS, and should be accessed through function
> anv_get_levelCount, two places have been covered in this patch.
> 
> Thanks, the spec wasn't clear to me since it's not explicitly said that
> REMAINING is an invalid value (unless I missed something).

IMHO, the REMAINING (~0) is valid but has special meaning " it can set levelCount and layerCount to the special values VK_REMAINING_MIP_LEVELS and VK_REMAINING_ARRAY_LAYERS without knowing the exact number of mip levels or layers.", so we need use anv_get_levelCount() function to decode the VkImageSubresourceRange::layerCount variable.

> 
> >
> > typedef struct VkImageSubresourceLayers {
> >      VkImageAspectFlags    aspectMask;
> >      uint32_t              mipLevel;
> >      uint32_t              baseArrayLayer;
> >      uint32_t              layerCount;
> > } VkImageSubresourceLayers;
> >
> > typedef struct VkImageSubresourceRange {
> >      VkImageAspectFlags    aspectMask;
> >      uint32_t              baseMipLevel;
> >      uint32_t              levelCount;
> >      uint32_t              baseArrayLayer;
> >      uint32_t              layerCount;
> > } VkImageSubresourceRange;
> >
> >
> >> Cheers,
> >>
> >> -
> >> Lionel
> >>
> >> On 20/03/17 07:31, Randy Xu wrote:
> >>> From: Xu Randy <randy.xu at intel.com>
> >>>
> >>> We should use anv_get_layerCount() to access layerCount of
> >>> VkImageSub- resourceRange in anv_CmdClearColorImage and
> >> anv_CmdClearDepthStencil-
> >>> Image, which handles the VK_REMAINING_ARRAY_LAYERS (~0) case.
> >>>
> >>> Test: Sample multithreadcmdbuf from LunarG can run without crash
> >>>
> >>> Signed-off-by: Xu Randy <randy.xu at intel.com>
> >>> ---
> >>>    src/intel/vulkan/anv_blorp.c | 4 ++--
> >>>    1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/src/intel/vulkan/anv_blorp.c
> >>> b/src/intel/vulkan/anv_blorp.c index 1f4fec5..9b3910f 100644
> >>> --- a/src/intel/vulkan/anv_blorp.c
> >>> +++ b/src/intel/vulkan/anv_blorp.c
> >>> @@ -830,7 +830,7 @@ void anv_CmdClearColorImage(
> >>>                            VK_IMAGE_ASPECT_COLOR_BIT,
> >>> image->tiling);
> >>>
> >>>          unsigned base_layer = pRanges[r].baseArrayLayer;
> >>> -      unsigned layer_count = pRanges[r].layerCount;
> >>> +      unsigned layer_count = anv_get_layerCount(image,
> >>> + &pRanges[r]);
> >>>
> >>>          for (unsigned i = 0; i < anv_get_levelCount(image, &pRanges[r]); i++)
> {
> >>>             const unsigned level = pRanges[r].baseMipLevel + i; @@
> >>> -890,7 +890,7 @@ void anv_CmdClearDepthStencilImage(
> >>>          bool clear_stencil = pRanges[r].aspectMask &
> >>> VK_IMAGE_ASPECT_STENCIL_BIT;
> >>>
> >>>          unsigned base_layer = pRanges[r].baseArrayLayer;
> >>> -      unsigned layer_count = pRanges[r].layerCount;
> >>> +      unsigned layer_count = anv_get_layerCount(image,
> >>> + &pRanges[r]);
> >>>
> >>>          for (unsigned i = 0; i < anv_get_levelCount(image, &pRanges[r]); i++)
> {
> >>>             const unsigned level = pRanges[r].baseMipLevel + i;
>