[PATCH libdrm 1/2] amdgpu: prevent an integer wraparound of cpu_map_count

[Marek Olšák]

You and I discussed this extensively internally a while ago. It's expected
and correct behavior. Mesa doesn't unmap some buffers and never will.

Marek

On Wed, Oct 24, 2018 at 3:45 AM Christian König <
ckoenig.leichtzumerken at gmail.com> wrote:

> That looks really ugly to me. Mapping the same BO so often is illegal
> and should be handled as error.
>
> Otherwise we will never be able to cleanly recover from a GPU lockup
> with lost state by reloading the client library.
>
> Christian.
>
> Am 23.10.18 um 21:07 schrieb Marek Olšák:
> > From: Marek Olšák <marek.olsak at amd.com>
> >
> > ---
> >   amdgpu/amdgpu_bo.c | 19 +++++++++++++++++--
> >   1 file changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
> > index c0f42e81..81f8a5f7 100644
> > --- a/amdgpu/amdgpu_bo.c
> > +++ b/amdgpu/amdgpu_bo.c
> > @@ -22,20 +22,21 @@
> >    *
> >    */
> >
> >   #include <stdlib.h>
> >   #include <stdio.h>
> >   #include <stdint.h>
> >   #include <string.h>
> >   #include <errno.h>
> >   #include <fcntl.h>
> >   #include <unistd.h>
> > +#include <limits.h>
> >   #include <sys/ioctl.h>
> >   #include <sys/mman.h>
> >   #include <sys/time.h>
> >
> >   #include "libdrm_macros.h"
> >   #include "xf86drm.h"
> >   #include "amdgpu_drm.h"
> >   #include "amdgpu_internal.h"
> >   #include "util_math.h"
> >
> > @@ -442,21 +443,29 @@ drm_public int amdgpu_bo_cpu_map(amdgpu_bo_handle
> bo, void **cpu)
> >   {
> >       union drm_amdgpu_gem_mmap args;
> >       void *ptr;
> >       int r;
> >
> >       pthread_mutex_lock(&bo->cpu_access_mutex);
> >
> >       if (bo->cpu_ptr) {
> >               /* already mapped */
> >               assert(bo->cpu_map_count > 0);
> > -             bo->cpu_map_count++;
> > +
> > +             /* If the counter has already reached INT_MAX, don't
> increment
> > +              * it and assume that the buffer will be mapped
> indefinitely.
> > +              * The buffer is pretty unlikely to get unmapped by the
> user
> > +              * at this point.
> > +              */
> > +             if (bo->cpu_map_count != INT_MAX)
> > +                     bo->cpu_map_count++;
> > +
> >               *cpu = bo->cpu_ptr;
> >               pthread_mutex_unlock(&bo->cpu_access_mutex);
> >               return 0;
> >       }
> >
> >       assert(bo->cpu_map_count == 0);
> >
> >       memset(&args, 0, sizeof(args));
> >
> >       /* Query the buffer address (args.addr_ptr).
> > @@ -492,21 +501,27 @@ drm_public int
> amdgpu_bo_cpu_unmap(amdgpu_bo_handle bo)
> >
> >       pthread_mutex_lock(&bo->cpu_access_mutex);
> >       assert(bo->cpu_map_count >= 0);
> >
> >       if (bo->cpu_map_count == 0) {
> >               /* not mapped */
> >               pthread_mutex_unlock(&bo->cpu_access_mutex);
> >               return -EINVAL;
> >       }
> >
> > -     bo->cpu_map_count--;
> > +     /* If the counter has already reached INT_MAX, don't decrement it.
> > +      * This is because amdgpu_bo_cpu_map doesn't increment it past
> > +      * INT_MAX.
> > +      */
> > +     if (bo->cpu_map_count != INT_MAX)
> > +             bo->cpu_map_count--;
> > +
> >       if (bo->cpu_map_count > 0) {
> >               /* mapped multiple times */
> >               pthread_mutex_unlock(&bo->cpu_access_mutex);
> >               return 0;
> >       }
> >
> >       r = drm_munmap(bo->cpu_ptr, bo->alloc_size) == 0 ? 0 : -errno;
> >       bo->cpu_ptr = NULL;
> >       pthread_mutex_unlock(&bo->cpu_access_mutex);
> >       return r;
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20181029/7cd07063/attachment.html>