On Fri, Sep 06, 2019 at 10:52:12AM +0200, Thomas Zimmermann wrote: > The kmap and kunmap operations of GEM VRAM buffers can now be called > in interleaving pairs. The first call to drm_gem_vram_kmap() maps the > buffer's memory to kernel address space and the final call to > drm_gem_vram_kunmap() unmaps the memory. Intermediate calls to these > functions increment or decrement a reference counter. > > This change allows for keeping buffer memory mapped for longer and > minimizes the amount of changes to TLB, page tables, etc. > > Signed-off-by: Thomas Zimmermann <[email protected]> > Cc: Davidlohr Bueso <[email protected]> > Reviewed-by: Gerd Hoffmann <[email protected]> > --- > drivers/gpu/drm/drm_gem_vram_helper.c | 74 ++++++++++++++++++++------- > include/drm/drm_gem_vram_helper.h | 19 +++++++ > 2 files changed, 75 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c > b/drivers/gpu/drm/drm_gem_vram_helper.c > index fd751078bae1..6c7912092876 100644 > --- a/drivers/gpu/drm/drm_gem_vram_helper.c > +++ b/drivers/gpu/drm/drm_gem_vram_helper.c > @@ -26,7 +26,11 @@ static void drm_gem_vram_cleanup(struct > drm_gem_vram_object *gbo) > * TTM buffer object in 'bo' has already been cleaned > * up; only release the GEM object. > */ > + > + WARN_ON(gbo->kmap_use_count); > + > drm_gem_object_release(&gbo->bo.base); > + mutex_destroy(&gbo->kmap_lock); > } > > static void drm_gem_vram_destroy(struct drm_gem_vram_object *gbo) > @@ -100,6 +104,8 @@ static int drm_gem_vram_init(struct drm_device *dev, > if (ret) > goto err_drm_gem_object_release; > > + mutex_init(&gbo->kmap_lock); > + > return 0; > > err_drm_gem_object_release: > @@ -283,6 +289,34 @@ int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo) > } > EXPORT_SYMBOL(drm_gem_vram_unpin); > > +static void *drm_gem_vram_kmap_locked(struct drm_gem_vram_object *gbo, > + bool map, bool *is_iomem) > +{ > + int ret; > + struct ttm_bo_kmap_obj *kmap = &gbo->kmap; > + > + if (gbo->kmap_use_count > 0) > + goto out; > + > + if (kmap->virtual || !map) > + goto out; > + > + ret = ttm_bo_kmap(&gbo->bo, 0, gbo->bo.num_pages, kmap); > + if (ret) > + return ERR_PTR(ret); > + > +out: > + if (!kmap->virtual) { > + if (is_iomem) > + *is_iomem = false; > + return NULL; /* not mapped; don't increment ref */ > + } > + ++gbo->kmap_use_count; > + if (is_iomem) > + return ttm_kmap_obj_virtual(kmap, is_iomem); > + return kmap->virtual; > +} > + > /** > * drm_gem_vram_kmap() - Maps a GEM VRAM object into kernel address space > * @gbo: the GEM VRAM object > @@ -304,40 +338,44 @@ void *drm_gem_vram_kmap(struct drm_gem_vram_object > *gbo, bool map, > bool *is_iomem) > { > int ret; > - struct ttm_bo_kmap_obj *kmap = &gbo->kmap; > - > - if (kmap->virtual || !map) > - goto out; > + void *virtual; > > - ret = ttm_bo_kmap(&gbo->bo, 0, gbo->bo.num_pages, kmap); > + ret = mutex_lock_interruptible(&gbo->kmap_lock); > if (ret) > return ERR_PTR(ret); > + virtual = drm_gem_vram_kmap_locked(gbo, map, is_iomem); > + mutex_unlock(&gbo->kmap_lock); > > -out: > - if (!is_iomem) > - return kmap->virtual; > - if (!kmap->virtual) { > - *is_iomem = false; > - return NULL; > - } > - return ttm_kmap_obj_virtual(kmap, is_iomem); > + return virtual; > } > EXPORT_SYMBOL(drm_gem_vram_kmap); > > -/** > - * drm_gem_vram_kunmap() - Unmaps a GEM VRAM object > - * @gbo: the GEM VRAM object > - */ > -void drm_gem_vram_kunmap(struct drm_gem_vram_object *gbo) > +static void drm_gem_vram_kunmap_locked(struct drm_gem_vram_object *gbo) > { > struct ttm_bo_kmap_obj *kmap = &gbo->kmap; > > + if (WARN_ON_ONCE(!gbo->kmap_use_count)) > + return; > + if (--gbo->kmap_use_count > 0) > + return; > + > if (!kmap->virtual) > return; > > ttm_bo_kunmap(kmap); > kmap->virtual = NULL; > } > + > +/** > + * drm_gem_vram_kunmap() - Unmaps a GEM VRAM object > + * @gbo: the GEM VRAM object > + */ > +void drm_gem_vram_kunmap(struct drm_gem_vram_object *gbo) > +{ > + mutex_lock(&gbo->kmap_lock); > + drm_gem_vram_kunmap_locked(gbo); > + mutex_unlock(&gbo->kmap_lock); > +} > EXPORT_SYMBOL(drm_gem_vram_kunmap); > > /** > diff --git a/include/drm/drm_gem_vram_helper.h > b/include/drm/drm_gem_vram_helper.h > index ac217d768456..8c08bc87b788 100644 > --- a/include/drm/drm_gem_vram_helper.h > +++ b/include/drm/drm_gem_vram_helper.h > @@ -34,11 +34,30 @@ struct vm_area_struct; > * backed by VRAM. It can be used for simple framebuffer devices with > * dedicated memory. The buffer object can be evicted to system memory if > * video memory becomes scarce. > + * > + * GEM VRAM objects perform reference counting for pin and mapping > + * operations. So a buffer object that has been pinned N times with > + * drm_gem_vram_pin() must be unpinned N times with > + * drm_gem_vram_unpin(). The same applies to pairs of > + * drm_gem_vram_kmap() and drm_gem_vram_kunmap(). > */ > struct drm_gem_vram_object { > struct ttm_buffer_object bo; > struct ttm_bo_kmap_obj kmap; > > + /** > + * @kmap_lock: Protects the kmap address and use count > + */ > + struct mutex kmap_lock;
Isn't the locking here a bit racy: The ttm side is protected by the dma_resv ww_mutex, but you have your own lock here. So if you race kmap on one side (from fbdev) with a modeset that evicts stuff (from ttm) things go boom. I think simpler to just reuse the ttm bo lock (reserve/unreserve). It's atm still a bit special, but Christian König has plans to make reserve/unreserve really nothing more than dma_resv_lock/unlock. -Daniel > + > + /** > + * @kmap_use_count: > + * > + * Reference count on the virtual address. > + * The address are un-mapped when the count reaches zero. > + */ > + unsigned int kmap_use_count; > + > /* Supported placements are %TTM_PL_VRAM and %TTM_PL_SYSTEM */ > struct ttm_placement placement; > struct ttm_place placements[2]; > -- > 2.23.0 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
