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

[Lionel Landwerlin]

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).

>
> 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;