[PATCH v3 1/3] drm/vram: Add kmap ref-counting to GEM VRAM objects

[Daniel Vetter]

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 <tzimmermann at suse.de>
> Cc: Davidlohr Bueso <dave at stgolabs.net>
> Reviewed-by: Gerd Hoffmann <kraxel at redhat.com>
> ---
>  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