[PATCH 08/12] drm/ttm: introduce callback for ttm_tt populate & unpopulate
Thomas Hellstrom
thellstrom at vmware.com
Tue Nov 8 00:29:48 PST 2011
On 11/08/2011 12:40 AM, j.glisse at gmail.com wrote:
> From: Jerome Glisse<jglisse at 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.
>
>
Same DOS vulnerability as mentioned previously.
Also, one minor thing below, otherwise looks good.
/Thomas
> Signed-off-by: Jerome Glisse<jglisse at redhat.com>
> Reviewed-by: Konrad Rzeszutek Wilk<konrad.wilk at oracle.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.
> - */
>
A pretty important comment removed above ^. I think it should be
retained since it has nothing to do
with the page allocation strategy.
> + /* 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);
>
DOS
> + 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);
>
More information about the dri-devel
mailing list