From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Hellstrom Subject: Re: [PATCH 2/2] drm/ttm: track kmap io_reserve(), only unreserve on unmap where needed Date: Tue, 09 Nov 2010 08:18:41 +0100 Message-ID: <4CD8F5D1.2010107@shipmail.org> References: <1288829027-23239-1-git-send-email-skeggsb@gmail.com> <1288829027-23239-2-git-send-email-skeggsb@gmail.com> <4CD2B030.1010306@shipmail.org> <4CD2FCCF.9090309@shipmail.org> <1289271811.6605.0.camel@nisroch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from smtp-outbound-2.vmware.com (smtp-outbound-2.vmware.com [65.115.85.73]) by gabe.freedesktop.org (Postfix) with ESMTP id 6E12D9E7C3 for ; Mon, 8 Nov 2010 23:18:53 -0800 (PST) In-Reply-To: <1289271811.6605.0.camel@nisroch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: skeggsb@gmail.com Cc: "dri-devel@lists.freedesktop.org" List-Id: dri-devel@lists.freedesktop.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 >>>> >>>> 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 >>>> --- >>>> 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 >>> >> > >