Re: [RFC][PATCH 2/3] dma-buf: system_heap: Add pagepool support to system heap

[Daniel Vetter]

On Thu, Dec 17, 2020 at 11:06:11PM +0000, John Stultz wrote:
> Reuse/abuse the pagepool code from the network code to speed
> up allocation performance.
> 
> This is similar to the ION pagepool usage, but tries to
> utilize generic code instead of a custom implementation.
> 
> Cc: Sumit Semwal <sumit.semwal@xxxxxxxxxx>
> Cc: Liam Mark <lmark@xxxxxxxxxxxxxx>
> Cc: Chris Goldsworthy <cgoldswo@xxxxxxxxxxxxxx>
> Cc: Laura Abbott <labbott@xxxxxxxxxx>
> Cc: Brian Starkey <Brian.Starkey@xxxxxxx>
> Cc: Hridya Valsaraju <hridya@xxxxxxxxxx>
> Cc: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> Cc: Sandeep Patil <sspatil@xxxxxxxxxx>
> Cc: Daniel Mentz <danielmentz@xxxxxxxxxx>
> Cc: Ørjan Eide <orjan.eide@xxxxxxx>
> Cc: Robin Murphy <robin.murphy@xxxxxxx>
> Cc: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
> Cc: Simon Ser <contact@xxxxxxxxxxx>
> Cc: James Jones <jajones@xxxxxxxxxx>
> Cc: linux-media@xxxxxxxxxxxxxxx
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>

We also have one of these in ttm. I think we should have at most one of
these for the gpu ecosystem overall, maybe as a helper that can be plugged
into all the places.

Or I'm kinda missing something, which could be since I only glanced at
yours for a bit. But it's also called page pool for buffer allocations,
and I don't think there's that many ways to implement that really :-)
-Daniel

> ---
>  drivers/dma-buf/heaps/Kconfig       |  1 +
>  drivers/dma-buf/heaps/system_heap.c | 68 +++++++++++++++++++++++++++--
>  2 files changed, 65 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
> index ecf65204f714..fa5e1c330cce 100644
> --- a/drivers/dma-buf/heaps/Kconfig
> +++ b/drivers/dma-buf/heaps/Kconfig
> @@ -4,6 +4,7 @@ config DMABUF_HEAPS_DEFERRED_FREE
>  config DMABUF_HEAPS_SYSTEM
>  	bool "DMA-BUF System Heap"
>  	depends on DMABUF_HEAPS
> +	select PAGE_POOL
>  	help
>  	  Choose this option to enable the system dmabuf heap. The system heap
>  	  is backed by pages from the buddy allocator. If in doubt, say Y.
> diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
> index 17e0e9a68baf..885e30894b77 100644
> --- a/drivers/dma-buf/heaps/system_heap.c
> +++ b/drivers/dma-buf/heaps/system_heap.c
> @@ -20,6 +20,7 @@
>  #include <linux/scatterlist.h>
>  #include <linux/slab.h>
>  #include <linux/vmalloc.h>
> +#include <net/page_pool.h>
>  
>  static struct dma_heap *sys_heap;
>  
> @@ -53,6 +54,7 @@ static gfp_t order_flags[] = {HIGH_ORDER_GFP, LOW_ORDER_GFP, LOW_ORDER_GFP};
>   */
>  static const unsigned int orders[] = {8, 4, 0};
>  #define NUM_ORDERS ARRAY_SIZE(orders)
> +struct page_pool *pools[NUM_ORDERS];
>  
>  static struct sg_table *dup_sg_table(struct sg_table *table)
>  {
> @@ -281,18 +283,59 @@ static void system_heap_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map)
>  	dma_buf_map_clear(map);
>  }
>  
> +static int system_heap_clear_pages(struct page **pages, int num, pgprot_t pgprot)
> +{
> +	void *addr = vmap(pages, num, VM_MAP, pgprot);
> +
> +	if (!addr)
> +		return -ENOMEM;
> +	memset(addr, 0, PAGE_SIZE * num);
> +	vunmap(addr);
> +	return 0;
> +}
> +
> +static int system_heap_zero_buffer(struct system_heap_buffer *buffer)
> +{
> +	struct sg_table *sgt = &buffer->sg_table;
> +	struct sg_page_iter piter;
> +	struct page *pages[32];
> +	int p = 0;
> +	int ret = 0;
> +
> +	for_each_sgtable_page(sgt, &piter, 0) {
> +		pages[p++] = sg_page_iter_page(&piter);
> +		if (p == ARRAY_SIZE(pages)) {
> +			ret = system_heap_clear_pages(pages, p, PAGE_KERNEL);
> +			if (ret)
> +				return ret;
> +			p = 0;
> +		}
> +	}
> +	if (p)
> +		ret = system_heap_clear_pages(pages, p, PAGE_KERNEL);
> +
> +	return ret;
> +}
> +
>  static void system_heap_dma_buf_release(struct dma_buf *dmabuf)
>  {
>  	struct system_heap_buffer *buffer = dmabuf->priv;
>  	struct sg_table *table;
>  	struct scatterlist *sg;
> -	int i;
> +	int i, j;
> +
> +	/* Zero the buffer pages before adding back to the pool */
> +	system_heap_zero_buffer(buffer);
>  
>  	table = &buffer->sg_table;
>  	for_each_sg(table->sgl, sg, table->nents, i) {
>  		struct page *page = sg_page(sg);
>  
> -		__free_pages(page, compound_order(page));
> +		for (j = 0; j < NUM_ORDERS; j++) {
> +			if (compound_order(page) == orders[j])
> +				break;
> +		}
> +		page_pool_put_full_page(pools[j], page, false);
>  	}
>  	sg_free_table(table);
>  	kfree(buffer);
> @@ -322,8 +365,7 @@ static struct page *alloc_largest_available(unsigned long size,
>  			continue;
>  		if (max_order < orders[i])
>  			continue;
> -
> -		page = alloc_pages(order_flags[i], orders[i]);
> +		page = page_pool_alloc_pages(pools[i], order_flags[i]);
>  		if (!page)
>  			continue;
>  		return page;
> @@ -428,6 +470,24 @@ static const struct dma_heap_ops system_heap_ops = {
>  static int system_heap_create(void)
>  {
>  	struct dma_heap_export_info exp_info;
> +	int i;
> +
> +	for (i = 0; i < NUM_ORDERS; i++) {
> +		struct page_pool_params pp;
> +
> +		memset(&pp, 0, sizeof(pp));
> +		pp.order = orders[i];
> +		pools[i] = page_pool_create(&pp);
> +
> +		if (IS_ERR(pools[i])) {
> +			int j;
> +
> +			pr_err("%s: page pool creation failed!\n", __func__);
> +			for (j = 0; j < i; j++)
> +				page_pool_destroy(pools[j]);
> +			return PTR_ERR(pools[i]);
> +		}
> +	}
>  
>  	exp_info.name = "system";
>  	exp_info.ops = &system_heap_ops;
> -- 
> 2.17.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch