Re: [PATCH 08/11] drm/ttm: introduce callback for ttm_tt populate & unpopulate

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Thu, Nov 03, 2011 at 07:39:12PM -0400, j.glisse@gmail.com wrote:
> From: Jerome Glisse <jglisse@redhat.com>
> 
> Move the page allocation and freeing to driver callback and
> provide ttm code helper function for those.
> 
> Most intrusive change, is the fact that we now only fully
> populate an object this simplify some of code designed around
> the page fault design.

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Signed-off-by: Jerome Glisse <jglisse@redhat.com>
> ---
>  drivers/gpu/drm/nouveau/nouveau_bo.c   |    3 +
>  drivers/gpu/drm/radeon/radeon_ttm.c    |    2 +
>  drivers/gpu/drm/ttm/ttm_bo_util.c      |   31 ++++++-----
>  drivers/gpu/drm/ttm/ttm_bo_vm.c        |   13 ++--
>  drivers/gpu/drm/ttm/ttm_page_alloc.c   |   42 ++++++++++++++
>  drivers/gpu/drm/ttm/ttm_tt.c           |   97 +++----------------------------
>  drivers/gpu/drm/vmwgfx/vmwgfx_buffer.c |    3 +
>  include/drm/ttm/ttm_bo_driver.h        |   41 ++++++++------
>  include/drm/ttm/ttm_page_alloc.h       |   18 ++++++
>  9 files changed, 125 insertions(+), 125 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
> index b060fa4..7e5ca3f 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_bo.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
> @@ -28,6 +28,7 @@
>   */
>  
>  #include "drmP.h"
> +#include "ttm/ttm_page_alloc.h"
>  
>  #include "nouveau_drm.h"
>  #include "nouveau_drv.h"
> @@ -1050,6 +1051,8 @@ nouveau_bo_fence(struct nouveau_bo *nvbo, struct nouveau_fence *fence)
>  
>  struct ttm_bo_driver nouveau_bo_driver = {
>  	.ttm_tt_create = &nouveau_ttm_tt_create,
> +	.ttm_tt_populate = &ttm_page_alloc_ttm_tt_populate,
> +	.ttm_tt_unpopulate = &ttm_page_alloc_ttm_tt_unpopulate,
>  	.invalidate_caches = nouveau_bo_invalidate_caches,
>  	.init_mem_type = nouveau_bo_init_mem_type,
>  	.evict_flags = nouveau_bo_evict_flags,
> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> index 53ff62b..490afce 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -584,6 +584,8 @@ struct ttm_tt *radeon_ttm_tt_create(struct ttm_bo_device *bdev,
>  
>  static struct ttm_bo_driver radeon_bo_driver = {
>  	.ttm_tt_create = &radeon_ttm_tt_create,
> +	.ttm_tt_populate = &ttm_page_alloc_ttm_tt_populate,
> +	.ttm_tt_unpopulate = &ttm_page_alloc_ttm_tt_unpopulate,
>  	.invalidate_caches = &radeon_invalidate_caches,
>  	.init_mem_type = &radeon_init_mem_type,
>  	.evict_flags = &radeon_evict_flags,
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
> index 082fcae..60f204d 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> @@ -244,7 +244,7 @@ static int ttm_copy_io_ttm_page(struct ttm_tt *ttm, void *src,
>  				unsigned long page,
>  				pgprot_t prot)
>  {
> -	struct page *d = ttm_tt_get_page(ttm, page);
> +	struct page *d = ttm->pages[page];
>  	void *dst;
>  
>  	if (!d)
> @@ -281,7 +281,7 @@ static int ttm_copy_ttm_io_page(struct ttm_tt *ttm, void *dst,
>  				unsigned long page,
>  				pgprot_t prot)
>  {
> -	struct page *s = ttm_tt_get_page(ttm, page);
> +	struct page *s = ttm->pages[page];
>  	void *src;
>  
>  	if (!s)
> @@ -342,6 +342,12 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo,
>  	if (old_iomap == NULL && ttm == NULL)
>  		goto out2;
>  
> +	if (ttm->state == tt_unpopulated) {
> +		ret = ttm->bdev->driver->ttm_tt_populate(ttm);
> +		if (ret)
> +			goto out1;
> +	}
> +
>  	add = 0;
>  	dir = 1;
>  
> @@ -502,10 +508,16 @@ static int ttm_bo_kmap_ttm(struct ttm_buffer_object *bo,
>  {
>  	struct ttm_mem_reg *mem = &bo->mem; pgprot_t prot;
>  	struct ttm_tt *ttm = bo->ttm;
> -	struct page *d;
> -	int i;
> +	int ret;
>  
>  	BUG_ON(!ttm);
> +
> +	if (ttm->state == tt_unpopulated) {
> +		ret = ttm->bdev->driver->ttm_tt_populate(ttm);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	if (num_pages == 1 && (mem->placement & TTM_PL_FLAG_CACHED)) {
>  		/*
>  		 * We're mapping a single page, and the desired
> @@ -513,18 +525,9 @@ static int ttm_bo_kmap_ttm(struct ttm_buffer_object *bo,
>  		 */
>  
>  		map->bo_kmap_type = ttm_bo_map_kmap;
> -		map->page = ttm_tt_get_page(ttm, start_page);
> +		map->page = ttm->pages[start_page];
>  		map->virtual = kmap(map->page);
>  	} else {
> -	    /*
> -	     * Populate the part we're mapping;
> -	     */
> -		for (i = start_page; i < start_page + num_pages; ++i) {
> -			d = ttm_tt_get_page(ttm, i);
> -			if (!d)
> -				return -ENOMEM;
> -		}
> -
>  		/*
>  		 * We need to use vmap to get the desired page protection
>  		 * or to make the buffer object look contiguous.
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> index 221b924..bc1d751 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> @@ -174,18 +174,19 @@ static int ttm_bo_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>  		vma->vm_page_prot = (bo->mem.placement & TTM_PL_FLAG_CACHED) ?
>  		    vm_get_page_prot(vma->vm_flags) :
>  		    ttm_io_prot(bo->mem.placement, vma->vm_page_prot);
> -	}
>  
> -	/*
> -	 * Speculatively prefault a number of pages. Only error on
> -	 * first page.
> -	 */
> +		/* Allocate all page at once, most common usage */
> +		if (ttm->bdev->driver->ttm_tt_populate(ttm)) {
> +			retval = VM_FAULT_OOM;
> +			goto out_io_unlock;
> +		}
> +	}
>  
>  	for (i = 0; i < TTM_BO_VM_NUM_PREFAULT; ++i) {
>  		if (bo->mem.bus.is_iomem)
>  			pfn = ((bo->mem.bus.base + bo->mem.bus.offset) >> PAGE_SHIFT) + page_offset;
>  		else {
> -			page = ttm_tt_get_page(ttm, page_offset);
> +			page = ttm->pages[page_offset];
>  			if (unlikely(!page && i == 0)) {
>  				retval = VM_FAULT_OOM;
>  				goto out_io_unlock;
> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c b/drivers/gpu/drm/ttm/ttm_page_alloc.c
> index c4f18b9..5b5b1e0 100644
> --- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
> @@ -852,6 +852,48 @@ void ttm_page_alloc_fini(void)
>  	_manager = NULL;
>  }
>  
> +int ttm_page_alloc_ttm_tt_populate(struct ttm_tt *ttm)
> +{
> +	struct ttm_mem_global *mem_glob = ttm->glob->mem_glob;
> +	int ret;
> +
> +	if (ttm->state != tt_unpopulated)
> +		return 0;
> +
> +	if (unlikely(ttm->page_flags & TTM_PAGE_FLAG_SWAPPED)) {
> +		ret = ttm_tt_swapin(ttm);
> +		if (unlikely(ret != 0))
> +			return ret;
> +	}
> +
> +	ret = ttm_get_pages(ttm->pages, ttm->num_pages, ttm->page_flags,
> +			    ttm->caching_state, ttm->dma_address);
> +	if (ret != 0)
> +		return -ENOMEM;
> +	ret = ttm_mem_global_alloc_pages(mem_glob, ttm->pages,
> +					 ttm->num_pages, false, false);
> +	if (unlikely(ret != 0)) {
> +		ttm_put_pages(ttm->pages, ttm->num_pages, ttm->page_flags,
> +			      ttm->caching_state, ttm->dma_address);
> +		return -ENOMEM;
> +	}
> +
> +	ttm->state = tt_unbound;
> +	return 0;
> +}
> +EXPORT_SYMBOL(ttm_page_alloc_ttm_tt_populate);
> +
> +void ttm_page_alloc_ttm_tt_unpopulate(struct ttm_tt *ttm)
> +{
> +	struct ttm_mem_global *glob = ttm->glob->mem_glob;
> +
> +	ttm_mem_global_free_pages(glob, ttm->pages, ttm->num_pages);
> +	ttm_put_pages(ttm->pages, ttm->num_pages, ttm->page_flags,
> +			ttm->caching_state, ttm->dma_address);
> +	ttm->state = tt_unpopulated;
> +}
> +EXPORT_SYMBOL(ttm_page_alloc_ttm_tt_unpopulate);
> +
>  int ttm_page_alloc_debugfs(struct seq_file *m, void *data)
>  {
>  	struct ttm_page_pool *p;
> diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c
> index b55e132..07e86c4 100644
> --- a/drivers/gpu/drm/ttm/ttm_tt.c
> +++ b/drivers/gpu/drm/ttm/ttm_tt.c
> @@ -42,8 +42,6 @@
>  #include "ttm/ttm_placement.h"
>  #include "ttm/ttm_page_alloc.h"
>  
> -static int ttm_tt_swapin(struct ttm_tt *ttm);
> -
>  /**
>   * Allocates storage for pointers to the pages that back the ttm.
>   */
> @@ -62,75 +60,6 @@ static void ttm_tt_free_page_directory(struct ttm_tt *ttm)
>  	ttm->dma_address = NULL;
>  }
>  
> -static struct page *__ttm_tt_get_page(struct ttm_tt *ttm, int index)
> -{
> -	struct page *p;
> -	struct ttm_mem_global *mem_glob = ttm->glob->mem_glob;
> -	int ret;
> -
> -	if (NULL == (p = ttm->pages[index])) {
> -
> -		ret = ttm_get_pages(&ttm->pages[index], 1, ttm->page_flags,
> -				    ttm->caching_state,
> -				    &ttm->dma_address[index]);
> -		if (ret != 0)
> -			return NULL;
> -
> -		ret = ttm_mem_global_alloc_pages(mem_glob, &ttm->pages[index],
> -						 1, false, false);
> -		if (unlikely(ret != 0))
> -			goto out_err;
> -	}
> -	return p;
> -out_err:
> -	ttm_put_pages(&ttm->pages[index], 1, ttm->page_flags,
> -		      ttm->caching_state, &ttm->dma_address[index]);
> -	return NULL;
> -}
> -
> -struct page *ttm_tt_get_page(struct ttm_tt *ttm, int index)
> -{
> -	int ret;
> -
> -	if (unlikely(ttm->page_flags & TTM_PAGE_FLAG_SWAPPED)) {
> -		ret = ttm_tt_swapin(ttm);
> -		if (unlikely(ret != 0))
> -			return NULL;
> -	}
> -	return __ttm_tt_get_page(ttm, index);
> -}
> -
> -int ttm_tt_populate(struct ttm_tt *ttm)
> -{
> -	struct ttm_mem_global *mem_glob = ttm->glob->mem_glob;
> -	int ret;
> -
> -	if (ttm->state != tt_unpopulated)
> -		return 0;
> -
> -	if (unlikely(ttm->page_flags & TTM_PAGE_FLAG_SWAPPED)) {
> -		ret = ttm_tt_swapin(ttm);
> -		if (unlikely(ret != 0))
> -			return ret;
> -	}
> -
> -	ret = ttm_get_pages(ttm->pages, ttm->num_pages, ttm->page_flags,
> -			    ttm->caching_state, ttm->dma_address);
> -	if (ret != 0)
> -		return -ENOMEM;
> -	ret = ttm_mem_global_alloc_pages(mem_glob, ttm->pages,
> -					 ttm->num_pages, false, false);
> -	if (unlikely(ret != 0)) {
> -		ttm_put_pages(ttm->pages, ttm->num_pages, ttm->page_flags,
> -			      ttm->caching_state, ttm->dma_address);
> -		return -ENOMEM;
> -	}
> -
> -	ttm->state = tt_unbound;
> -	return 0;
> -}
> -EXPORT_SYMBOL(ttm_tt_populate);
> -
>  #ifdef CONFIG_X86
>  static inline int ttm_tt_set_page_caching(struct page *p,
>  					  enum ttm_caching_state c_old,
> @@ -232,23 +161,13 @@ int ttm_tt_set_placement_caching(struct ttm_tt *ttm, uint32_t placement)
>  }
>  EXPORT_SYMBOL(ttm_tt_set_placement_caching);
>  
> -static void ttm_tt_free_alloced_pages(struct ttm_tt *ttm)
> -{
> -	struct ttm_mem_global *glob = ttm->glob->mem_glob;
> -
> -	ttm_mem_global_free_pages(glob, ttm->pages, ttm->num_pages);
> -	ttm_put_pages(ttm->pages, ttm->num_pages, ttm->page_flags,
> -			ttm->caching_state, ttm->dma_address);
> -	ttm->state = tt_unpopulated;
> -}
> -
>  void ttm_tt_destroy(struct ttm_tt *ttm)
>  {
>  	if (unlikely(ttm == NULL))
>  		return;
>  
>  	if (likely(ttm->pages != NULL)) {
> -		ttm_tt_free_alloced_pages(ttm);
> +		ttm->bdev->driver->ttm_tt_unpopulate(ttm);
>  		ttm_tt_free_page_directory(ttm);
>  	}
>  
> @@ -303,7 +222,7 @@ int ttm_tt_bind(struct ttm_tt *ttm, struct ttm_mem_reg *bo_mem)
>  	if (ttm->state == tt_bound)
>  		return 0;
>  
> -	ret = ttm_tt_populate(ttm);
> +	ret = ttm->bdev->driver->ttm_tt_populate(ttm);
>  	if (ret)
>  		return ret;
>  
> @@ -317,7 +236,7 @@ int ttm_tt_bind(struct ttm_tt *ttm, struct ttm_mem_reg *bo_mem)
>  }
>  EXPORT_SYMBOL(ttm_tt_bind);
>  
> -static int ttm_tt_swapin(struct ttm_tt *ttm)
> +int ttm_tt_swapin(struct ttm_tt *ttm)
>  {
>  	struct address_space *swap_space;
>  	struct file *swap_storage;
> @@ -331,6 +250,10 @@ static int ttm_tt_swapin(struct ttm_tt *ttm)
>  	swap_storage = ttm->swap_storage;
>  	BUG_ON(swap_storage == NULL);
>  
> +	ret = ttm->bdev->driver->ttm_tt_populate(ttm);
> +	if (ret)
> +		return ret;
> +
>  	swap_space = swap_storage->f_path.dentry->d_inode->i_mapping;
>  
>  	for (i = 0; i < ttm->num_pages; ++i) {
> @@ -339,7 +262,7 @@ static int ttm_tt_swapin(struct ttm_tt *ttm)
>  			ret = PTR_ERR(from_page);
>  			goto out_err;
>  		}
> -		to_page = __ttm_tt_get_page(ttm, i);
> +		to_page = ttm->pages[i];
>  		if (unlikely(to_page == NULL))
>  			goto out_err;
>  
> @@ -360,7 +283,7 @@ static int ttm_tt_swapin(struct ttm_tt *ttm)
>  
>  	return 0;
>  out_err:
> -	ttm_tt_free_alloced_pages(ttm);
> +	ttm->bdev->driver->ttm_tt_unpopulate(ttm);
>  	return ret;
>  }
>  
> @@ -412,7 +335,7 @@ int ttm_tt_swapout(struct ttm_tt *ttm, struct file *persistent_swap_storage)
>  		page_cache_release(to_page);
>  	}
>  
> -	ttm_tt_free_alloced_pages(ttm);
> +	ttm->bdev->driver->ttm_tt_unpopulate(ttm);
>  	ttm->swap_storage = swap_storage;
>  	ttm->page_flags |= TTM_PAGE_FLAG_SWAPPED;
>  	if (persistent_swap_storage)
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_buffer.c
> index cc72435..383993b 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_buffer.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_buffer.c
> @@ -28,6 +28,7 @@
>  #include "vmwgfx_drv.h"
>  #include "ttm/ttm_bo_driver.h"
>  #include "ttm/ttm_placement.h"
> +#include "ttm/ttm_page_alloc.h"
>  
>  static uint32_t vram_placement_flags = TTM_PL_FLAG_VRAM |
>  	TTM_PL_FLAG_CACHED;
> @@ -334,6 +335,8 @@ static int vmw_sync_obj_wait(void *sync_obj, void *sync_arg,
>  
>  struct ttm_bo_driver vmw_bo_driver = {
>  	.ttm_tt_create = &vmw_ttm_tt_create,
> +	.ttm_tt_populate = &ttm_page_alloc_ttm_tt_populate,
> +	.ttm_tt_unpopulate = &ttm_page_alloc_ttm_tt_unpopulate,
>  	.invalidate_caches = vmw_invalidate_caches,
>  	.init_mem_type = vmw_init_mem_type,
>  	.evict_flags = vmw_evict_flags,
> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> index 6b8c5cd..ae06e42 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -319,6 +319,26 @@ struct ttm_bo_driver {
>  					struct page *dummy_read_page);
>  
>  	/**
> +	 * ttm_tt_populate
> +	 *
> +	 * @ttm: The struct ttm_tt to contain the backing pages.
> +	 *
> +	 * Allocate all backing pages
> +	 * Returns:
> +	 * -ENOMEM: Out of memory.
> +	 */
> +	int (*ttm_tt_populate)(struct ttm_tt *ttm);
> +
> +	/**
> +	 * ttm_tt_unpopulate
> +	 *
> +	 * @ttm: The struct ttm_tt to contain the backing pages.
> +	 *
> +	 * Free all backing page
> +	 */
> +	void (*ttm_tt_unpopulate)(struct ttm_tt *ttm);
> +
> +	/**
>  	 * struct ttm_bo_driver member invalidate_caches
>  	 *
>  	 * @bdev: the buffer object device.
> @@ -585,15 +605,6 @@ extern int ttm_tt_init(struct ttm_tt *ttm, struct ttm_bo_device *bdev,
>  extern int ttm_tt_bind(struct ttm_tt *ttm, struct ttm_mem_reg *bo_mem);
>  
>  /**
> - * ttm_tt_populate:
> - *
> - * @ttm: The struct ttm_tt to contain the backing pages.
> - *
> - * Add backing pages to all of @ttm
> - */
> -extern int ttm_tt_populate(struct ttm_tt *ttm);
> -
> -/**
>   * ttm_ttm_destroy:
>   *
>   * @ttm: The struct ttm_tt.
> @@ -612,19 +623,13 @@ extern void ttm_tt_destroy(struct ttm_tt *ttm);
>  extern void ttm_tt_unbind(struct ttm_tt *ttm);
>  
>  /**
> - * ttm_ttm_destroy:
> + * ttm_tt_swapin:
>   *
>   * @ttm: The struct ttm_tt.
> - * @index: Index of the desired page.
>   *
> - * Return a pointer to the struct page backing @ttm at page
> - * index @index. If the page is unpopulated, one will be allocated to
> - * populate that index.
> - *
> - * Returns:
> - * NULL on OOM.
> + * Swap in a previously swap out ttm_tt.
>   */
> -extern struct page *ttm_tt_get_page(struct ttm_tt *ttm, int index);
> +extern int ttm_tt_swapin(struct ttm_tt *ttm);
>  
>  /**
>   * ttm_tt_cache_flush:
> diff --git a/include/drm/ttm/ttm_page_alloc.h b/include/drm/ttm/ttm_page_alloc.h
> index fffb3bd..18a2d6b 100644
> --- a/include/drm/ttm/ttm_page_alloc.h
> +++ b/include/drm/ttm/ttm_page_alloc.h
> @@ -67,6 +67,24 @@ int ttm_page_alloc_init(struct ttm_mem_global *glob, unsigned max_pages);
>  void ttm_page_alloc_fini(void);
>  
>  /**
> + * ttm_page_alloc_ttm_tt_populate:
> + *
> + * @ttm: The struct ttm_tt to contain the backing pages.
> + *
> + * Add backing pages to all of @ttm
> + */
> +extern int ttm_page_alloc_ttm_tt_populate(struct ttm_tt *ttm);
> +
> +/**
> + * ttm_page_alloc_ttm_tt_unpopulate:
> + *
> + * @ttm: The struct ttm_tt which to free backing pages.
> + *
> + * Free all pages of @ttm
> + */
> +extern void ttm_page_alloc_ttm_tt_unpopulate(struct ttm_tt *ttm);
> +
> +/**
>   * Output the state of pools to debugfs file
>   */
>  extern int ttm_page_alloc_debugfs(struct seq_file *m, void *data);
> -- 
> 1.7.7.1