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

[Nanley Chery]

On Fri, Sep 02, 2016 at 12:46:30PM -0700, Chad Versace wrote:
> 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.
> 

As I understand it, yes, anv_gem_mmap() can fail if we run out of
host CPU PTEs to perform the mapping. My V2 will address this.

Nanley

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