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

[Thomas Hellstrom <thomas@shipmail.org>]

On 11/09/2010 04:03 AM, Ben Skeggs wrote:
> 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.
>    
Great.

I have a question, though. (CC'ing Jerome as well)

Seems to me like something is missing from the mem_reserve interface. 
Let's say you have a programmable VRAM aperture and it's full, so you 
can't honor bo map request. You'd then want to traverse a list and call 
unmap_mapping_range() to kill user-space maps and free VRAM aperture 
space, but you can't really do that since you don't have access to the 
mapping range in question...?

/Thomas







>> 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@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@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@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>        
>>      
>
>