[PATCH 2/2] drm/ttm: track kmap io_reserve(), only unreserve on unmap where needed

[Ben Skeggs]

On Thu, 2010-11-04 at 19:34 +0100, Thomas Hellstrom wrote:
> Ben,
> 
> I had something like the attached in mind, although it might be more 
> beneficial to do the actual refcounting in drivers that needs it. Atomic 
> incs and decs are expensive, but I'm not sure how expensive relative to 
> function pointer calls.
Thomas,

Thanks for that :)  It looks good to me, and appears to work as it
should.

Ben.
> 
> Patch is only compile-tested
> 
> /Thomas
> 
> 
> On 11/04/2010 02:08 PM, Thomas Hellstrom wrote:
> > On 11/04/2010 01:03 AM, Ben Skeggs wrote:
> >> From: Ben Skeggs<bskeggs at redhat.com>
> >>
> >> If the driver kmaps an object userspace is expecting to be mapped, the
> >> unmap would have called down into the drivers io_unreserve() function
> >> and potentially unmapped the pages from its BARs (for example) and 
> >> they'd
> >> no longer be accessible for the userspace mapping.
> >>
> >> Signed-off-by: Ben Skeggs<bskeggs at redhat.com>
> >> ---
> >>   drivers/gpu/drm/ttm/ttm_bo_util.c |   14 ++++++++++----
> >>   include/drm/ttm/ttm_bo_api.h      |    1 +
> >>   2 files changed, 11 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c 
> >> b/drivers/gpu/drm/ttm/ttm_bo_util.c
> >> index ff358ad..e9dbe8b 100644
> >> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> >> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> >> @@ -467,9 +467,12 @@ int ttm_bo_kmap(struct ttm_buffer_object *bo,
> >>       if (num_pages>  1&&  !DRM_SUSER(DRM_CURPROC))
> >>           return -EPERM;
> >>   #endif
> >> -    ret = ttm_mem_io_reserve(bo->bdev,&bo->mem);
> >> -    if (ret)
> >> -        return ret;
> >> +    if (!bo->mem.bus.io_reserved) {
> >> +        ret = ttm_mem_io_reserve(bo->bdev,&bo->mem);
> >> +        if (ret)
> >> +            return ret;
> >> +        map->io_reserved = true;
> >> +    }
> >>       if (!bo->mem.bus.is_iomem) {
> >>           return ttm_bo_kmap_ttm(bo, start_page, num_pages, map);
> >>       } else {
> >> @@ -487,7 +490,10 @@ void ttm_bo_kunmap(struct ttm_bo_kmap_obj *map)
> >>       switch (map->bo_kmap_type) {
> >>       case ttm_bo_map_iomap:
> >>           iounmap(map->virtual);
> >> -        ttm_mem_io_free(map->bo->bdev,&map->bo->mem);
> >> +        if (map->io_reserved) {
> >> +            ttm_mem_io_free(map->bo->bdev,&map->bo->mem);
> >> +            map->io_reserved = false;
> >> +        }
> >>           break;
> >>       case ttm_bo_map_vmap:
> >>           vunmap(map->virtual);
> >> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> >> index 5afa5b5..ce998ac 100644
> >> --- a/include/drm/ttm/ttm_bo_api.h
> >> +++ b/include/drm/ttm/ttm_bo_api.h
> >> @@ -300,6 +300,7 @@ struct ttm_bo_kmap_obj {
> >>           ttm_bo_map_premapped    = 4 | TTM_BO_MAP_IOMEM_MASK,
> >>       } bo_kmap_type;
> >>       struct ttm_buffer_object *bo;
> >> +    bool io_reserved;
> >>   };
> >>
> >>   /**
> >>
> >
> > This doesn't solve the problem unfortunately. Consider the sequence
> >
> > kmap->io_mem_reserve
> > fault()->
> > kunmap->io_mem_free
> > user_space_access()-> Invalid.
> >
> > I think this needs to be fixed by us maintaining an 
> > mem:io_reserved_count, where all user-space triggered io_reserves 
> > count as 1. A mem::user_space_io_reserved flag could be protected by 
> > the bo::reserve lock, whereas a reserved_count can't, since strictly 
> > you're allowed to kmap a bo without reserving it, but only if it's pinned
> >
> > /Thomas
> > .
> >
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>