[Mesa-dev] [PATCH] anv: anv_gem_mmap() returns MAP_FAILED as mapping error

[Samuel Iglesias Gonsálvez]

On Wed, 2017-05-03 at 14:37 +0100, Emil Velikov wrote:
> On 3 May 2017 at 14:26, Samuel Iglesias Gonsálvez <siglesias at igalia.c
> om> wrote:
> > On Wed, 2017-05-03 at 14:15 +0100, Emil Velikov wrote:
> > > On 3 May 2017 at 12:33, Samuel Iglesias Gonsálvez <siglesias at igal
> > > ia.c
> > > om> wrote:
> > > > On Wed, 2017-05-03 at 11:50 +0100, Emil Velikov wrote:
> > > > > Hi Samuel,
> > > > > 
> > > > > On 3 May 2017 at 08:57, Samuel Iglesias Gonsálvez <siglesias@
> > > > > igal
> > > > > ia.c
> > > > > om> wrote:
> > > > > > Take it into account when checking if the mapping failed.
> > > > > > 
> > > > > > Signed-off-by: Samuel Iglesias Gonsálvez <siglesias at igalia.
> > > > > > com>
> > > > > > ---
> > > > > >  src/intel/vulkan/anv_allocator.c | 2 +-
> > > > > >  src/intel/vulkan/anv_image.c     | 4 ++++
> > > > > >  2 files changed, 5 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/src/intel/vulkan/anv_allocator.c
> > > > > > b/src/intel/vulkan/anv_allocator.c
> > > > > > index 554ca4ac5f..6ab2da5d64 100644
> > > > > > --- a/src/intel/vulkan/anv_allocator.c
> > > > > > +++ b/src/intel/vulkan/anv_allocator.c
> > > > > > @@ -889,7 +889,7 @@ anv_bo_pool_alloc(struct anv_bo_pool
> > > > > > *pool,
> > > > > > struct anv_bo *bo, uint32_t size)
> > > > > >     assert(new_bo.size == pow2_size);
> > > > > > 
> > > > > >     new_bo.map = anv_gem_mmap(pool->device,
> > > > > > new_bo.gem_handle,
> > > > > > 0,
> > > > > > pow2_size, 0);
> > > > > > -   if (new_bo.map == NULL) {
> > > > > > +   if (new_bo.map == MAP_FAILED) {
> > > > > >        anv_gem_close(pool->device, new_bo.gem_handle);
> > > > > >        return vk_error(VK_ERROR_MEMORY_MAP_FAILED);
> > > > > >     }
> > > > > > diff --git a/src/intel/vulkan/anv_image.c
> > > > > > b/src/intel/vulkan/anv_image.c
> > > > > > index 4874f2f3d3..d7d53f96a4 100644
> > > > > > --- a/src/intel/vulkan/anv_image.c
> > > > > > +++ b/src/intel/vulkan/anv_image.c
> > > > > > @@ -26,6 +26,7 @@
> > > > > >  #include <string.h>
> > > > > >  #include <unistd.h>
> > > > > >  #include <fcntl.h>
> > > > > > +#include <sys/mman.h>
> > > > > > 
> > > > > >  #include "anv_private.h"
> > > > > >  #include "util/debug.h"
> > > > > > @@ -369,6 +370,9 @@ VkResult anv_BindImageMemory(
> > > > > >        if (map == NULL)
> > > > > >           return vk_error(VK_ERROR_OUT_OF_HOST_MEMORY);
> > > > > > 
> > > > > 
> > > > > Shouldn't we drop this check alongside its comment? There's a
> > > > > another
> > > > > comment a few lines further up which should be updated as
> > > > > well.
> > > > 
> > > > I thought about doing the same when writing the patch. However,
> > > > that
> > > > comment gave me some doubts which is the reason I added Jason
> > > > in
> > > > Cc.
> > > > Let's see if he agrees that it is safe to remove this check
> > > > (and
> > > > its
> > > > comment), as he was the author of those lines.
> > > > 
> > > 
> > > Most likely it's a typo since if the kernel returns NULL w/o
> > > reporting
> > > an error we're in the deep.

Right.

> > > Regardless, please add the following tags.
> > > 
> > > Fixes: 6f3e3c715a7 ("vk/allocator: Add a BO pool")
> > > Fixes: 9919a2d34de ("anv/image: Memset hiz surfaces to 0 when
> > > binding
> > > memory")
> > > 
> > 
> > OK, I will add them.
> > 
> > Should I add the mesa-stable tag to this or you will pick it
> > without
> > it? If I need to add it... then it would be "17.0 17.1", right?
> > 
> 
> The Fixes tag somewhat supersedes mesa-stable, as the former
> nominates
> the patch for the correct branch(es).
> Adding mesa-stable won't hurt though :-)
> 

I finally pushed this patch to master, so it enters into next mesa
release.

Thanks,

Sam
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170504/d743e37c/attachment.sig>