[Mesa-dev] [PATCH 07/12] anv/image: Memset hiz surfaces to 0 when binding memory

[Chad Versace]

On Thu 01 Sep 2016, Jason Ekstrand wrote:
> 
> 
> On Wed, Aug 31, 2016 at 8:29 PM, Nanley Chery <nanleychery at gmail.com> wrote:
> 
>     From: Jason Ekstrand <jason.ekstrand at intel.com>
> 
>     Nanley Chery (amend):
>      - Change memset value from 0xff to 0 (a defined value for HiZ).

Yep. 0xff may hang the GPU, but 0x0 is safe.

> 
>     Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
>     ---
>      src/intel/vulkan/anv_image.c | 18 +++++++++++++++++-
>      1 file changed, 17 insertions(+), 1 deletion(-)
> 
>     diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c
>     index 5112c5d..9fbebd0 100644
>     --- a/src/intel/vulkan/anv_image.c
>     +++ b/src/intel/vulkan/anv_image.c
>     @@ -311,11 +311,12 @@ anv_DestroyImage(VkDevice _device, VkImage _image,
>      }
> 
>      VkResult anv_BindImageMemory(
>     -    VkDevice                                    device,
>     +    VkDevice                                    _device,
>          VkImage                                     _image,
>          VkDeviceMemory                              _memory,
>          VkDeviceSize                                memoryOffset)
>      {
>     +   ANV_FROM_HANDLE(anv_device, device, _device);
>         ANV_FROM_HANDLE(anv_device_memory, mem, _memory);
>         ANV_FROM_HANDLE(anv_image, image, _image);
> 
>     @@ -327,6 +328,21 @@ VkResult anv_BindImageMemory(
>            image->offset = 0;
>         }
> 
>     +   if (anv_image_has_hiz(image)) {
>     +      /* HiZ surfaces need to have their memory cleared to 0 before they
>     +       * can be used.  If we let it have garbage data, it can cause GPU
>     +       * hangs on some hardware.
>     +       */
>     +      void *map = anv_gem_mmap(device, image->bo->gem_handle,
>     +                               image->offset + image->hiz_surface.offset,
>     +                               image->hiz_surface.isl.size,
>     +                               device->info.has_llc ? 0 : I915_MMAP_WC);

IIUC, because the mapping is write-combined on non-LLC chipets, and thus
coherent, we don't need to follow anv_gem_munmap() with
__builtin_ia32_mfence(). Looks good to me.

As an aside, I found some weirdness elsewhere in the driver regarding
I915_MMAP_WC. I filed a bug:
https://bugs.freedesktop.org/show_bug.cgi?id=97579

> We should assert that offset and size are both divisible by 4096.  Otherwise,
> the kernel will return a NULL map and we'll segfault.  It should always be true
> because the surface is tiled, but we should assert none the less.

Also, I'm bothered that the code assumes anv_gem_mmap() always succeeds.
But that assumption seems prevalent throughout the driver. Is there any
situation where, given valid input, it could still fail? After all,
vkMapMemory's return type is VkResult, not void.

>  
> 
>     +
>     +      memset(map, 0, image->hiz_surface.isl.size);
>     +
>     +      anv_gem_munmap(map, image->hiz_surface.isl.size);
>     +   }
>     +
>         return VK_SUCCESS;
>      }
>    
>     --
>     2.9.3

Your git is newer than mine!
/me pulls git master and rebuilds