[PATCH v8 1/5] drm: Add a sharable drm page-pool implementation

[Christian König]

Am 05.03.21 um 00:20 schrieb John Stultz:
> This adds a shrinker controlled page pool, extracted
> out of the ttm_pool logic, and abstracted out a bit
> so it can be used by other non-ttm drivers.

In general please keep the kernel doc which is in TTMs pool.

>
> Cc: Daniel Vetter <daniel at ffwll.ch>
> Cc: Christian Koenig <christian.koenig at amd.com>
> Cc: Sumit Semwal <sumit.semwal at linaro.org>
> Cc: Liam Mark <lmark at codeaurora.org>
> Cc: Chris Goldsworthy <cgoldswo at codeaurora.org>
> Cc: Laura Abbott <labbott at kernel.org>
> Cc: Brian Starkey <Brian.Starkey at arm.com>
> Cc: Hridya Valsaraju <hridya at google.com>
> Cc: Suren Baghdasaryan <surenb at google.com>
> Cc: Sandeep Patil <sspatil at google.com>
> Cc: Daniel Mentz <danielmentz at google.com>
> Cc: Ørjan Eide <orjan.eide at arm.com>
> Cc: Robin Murphy <robin.murphy at arm.com>
> Cc: Ezequiel Garcia <ezequiel at collabora.com>
> Cc: Simon Ser <contact at emersion.fr>
> Cc: James Jones <jajones at nvidia.com>
> Cc: linux-media at vger.kernel.org
> Cc: dri-devel at lists.freedesktop.org
> Signed-off-by: John Stultz <john.stultz at linaro.org>
> ---
> v8:
> * Completely rewritten from scratch, using only the
>    ttm_pool logic so it can be dual licensed.
> ---
>   drivers/gpu/drm/Kconfig     |   4 +
>   drivers/gpu/drm/Makefile    |   2 +
>   drivers/gpu/drm/page_pool.c | 214 ++++++++++++++++++++++++++++++++++++
>   include/drm/page_pool.h     |  65 +++++++++++
>   4 files changed, 285 insertions(+)
>   create mode 100644 drivers/gpu/drm/page_pool.c
>   create mode 100644 include/drm/page_pool.h
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index e392a90ca687..7cbcecb8f7df 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -177,6 +177,10 @@ config DRM_DP_CEC
>   	  Note: not all adapters support this feature, and even for those
>   	  that do support this they often do not hook up the CEC pin.
>   
> +config DRM_PAGE_POOL
> +	bool
> +	depends on DRM
> +
>   config DRM_TTM
>   	tristate
>   	depends on DRM && MMU
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 926adef289db..2dc7b2fe3fe5 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -39,6 +39,8 @@ obj-$(CONFIG_DRM_VRAM_HELPER) += drm_vram_helper.o
>   drm_ttm_helper-y := drm_gem_ttm_helper.o
>   obj-$(CONFIG_DRM_TTM_HELPER) += drm_ttm_helper.o
>   
> +drm-$(CONFIG_DRM_PAGE_POOL) += page_pool.o
> +
>   drm_kms_helper-y := drm_bridge_connector.o drm_crtc_helper.o drm_dp_helper.o \
>   		drm_dsc.o drm_probe_helper.o \
>   		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
> diff --git a/drivers/gpu/drm/page_pool.c b/drivers/gpu/drm/page_pool.c
> new file mode 100644
> index 000000000000..a60b954cfe0f
> --- /dev/null
> +++ b/drivers/gpu/drm/page_pool.c
> @@ -0,0 +1,214 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + * Sharable page pool implementation
> + *
> + * Extracted from drivers/gpu/drm/ttm/ttm_pool.c
> + * Copyright 2020 Advanced Micro Devices, Inc.
> + * Copyright 2021 Linaro Ltd.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors: Christian König, John Stultz
> + */
> +
> +#include <linux/module.h>
> +#include <linux/highmem.h>
> +#include <linux/shrinker.h>
> +#include <drm/page_pool.h>
> +
> +static unsigned long page_pool_size;
> +
> +MODULE_PARM_DESC(page_pool_size, "Number of pages in the WC/UC/DMA pool");
> +module_param(page_pool_size, ulong, 0644);
> +
> +static atomic_long_t allocated_pages;
> +
> +static struct mutex shrinker_lock;
> +static struct list_head shrinker_list;
> +static struct shrinker mm_shrinker;
> +
> +void drm_page_pool_set_max(unsigned long max)

This function and a whole bunch of other can be static.

And in general I'm not sure if we really need wrappers for functionality 
like this, e.g. it is only used once during startup IIRC.

> +{
> +	if (!page_pool_size)
> +		page_pool_size = max;
> +}
> +
> +unsigned long drm_page_pool_get_max(void)
> +{
> +	return page_pool_size;
> +}
> +
> +unsigned long drm_page_pool_get_total(void)
> +{
> +	return atomic_long_read(&allocated_pages);
> +}
> +
> +unsigned long drm_page_pool_get_size(struct drm_page_pool *pool)
> +{
> +	unsigned long size;
> +
> +	spin_lock(&pool->lock);
> +	size = pool->page_count;
> +	spin_unlock(&pool->lock);
> +	return size;
> +}
> +
> +/* Give pages into a specific pool */
> +void drm_page_pool_add(struct drm_page_pool *pool, struct page *p)
> +{
> +	unsigned int i, num_pages = 1 << pool->order;
> +
> +	for (i = 0; i < num_pages; ++i) {
> +		if (PageHighMem(p))
> +			clear_highpage(p + i);
> +		else
> +			clear_page(page_address(p + i));
> +	}
> +
> +	spin_lock(&pool->lock);
> +	list_add(&p->lru, &pool->pages);
> +	pool->page_count += 1 << pool->order;
> +	spin_unlock(&pool->lock);
> +	atomic_long_add(1 << pool->order, &allocated_pages);
> +}
> +
> +/* Take pages from a specific pool, return NULL when nothing available */
> +struct page *drm_page_pool_remove(struct drm_page_pool *pool)
> +{
> +	struct page *p;
> +
> +	spin_lock(&pool->lock);
> +	p = list_first_entry_or_null(&pool->pages, typeof(*p), lru);
> +	if (p) {
> +		atomic_long_sub(1 << pool->order, &allocated_pages);
> +		pool->page_count -= 1 << pool->order;
> +		list_del(&p->lru);
> +	}
> +	spin_unlock(&pool->lock);
> +
> +	return p;
> +}
> +
> +/* Initialize and add a pool type to the global shrinker list */
> +void drm_page_pool_init(struct drm_page_pool *pool, unsigned int order,
> +			unsigned long (*free_page)(struct drm_page_pool *pool, struct page *p))
> +{
> +	pool->order = order;
> +	spin_lock_init(&pool->lock);
> +	INIT_LIST_HEAD(&pool->pages);
> +	pool->free = free_page;
> +	pool->page_count = 0;
> +
> +	mutex_lock(&shrinker_lock);
> +	list_add_tail(&pool->shrinker_list, &shrinker_list);
> +	mutex_unlock(&shrinker_lock);
> +}
> +
> +/* Remove a pool_type from the global shrinker list and free all pages */
> +void drm_page_pool_fini(struct drm_page_pool *pool)
> +{
> +	struct page *p, *tmp;
> +
> +	mutex_lock(&shrinker_lock);
> +	list_del(&pool->shrinker_list);
> +	mutex_unlock(&shrinker_lock);
> +
> +	list_for_each_entry_safe(p, tmp, &pool->pages, lru)

This is buggy, see the recently committed patch to the TTM page pool on 
drm-misc-next.

> +		pool->free(pool, p);
> +}
> +
> +/* Free pages using the global shrinker list */
> +static unsigned int drm_page_pool_shrink(void)
> +{
> +	struct drm_page_pool *pool;
> +	unsigned int num_freed;
> +	struct page *p;
> +
> +	mutex_lock(&shrinker_lock);
> +	pool = list_first_entry(&shrinker_list, typeof(*pool), shrinker_list);
> +
> +	p = drm_page_pool_remove(pool);
> +
> +	list_move_tail(&pool->shrinker_list, &shrinker_list);
> +	mutex_unlock(&shrinker_lock);
> +
> +	if (p) {
> +		pool->free(pool, p);
> +		num_freed = 1 << pool->order;
> +	} else {
> +		num_freed = 0;
> +	}
> +
> +	return num_freed;

The problem with this approach is that you somehow need to make sure 
that the pool isn't freed while the shrinker is run.

The easiest way to archive that is to add a new helper to the shrinker 
code which just grabs the write side of their semaphore and release it 
again.

With that done the shrinker_lock can also be switched back to a spinlock 
again.

> +}
> +
> +/* As long as pages are available make sure to release at least one */
> +static unsigned long drm_page_pool_shrinker_scan(struct shrinker *shrink,
> +						 struct shrink_control *sc)
> +{
> +	unsigned long num_freed = 0;
> +
> +	do
> +		num_freed += drm_page_pool_shrink();
> +	while (!num_freed && atomic_long_read(&allocated_pages));
> +
> +	return num_freed;

IIRC this also need a "num_pages ? num_pages : SHRINK_EMPTY", but please 
double check with the shrinker code.

> +}
> +
> +/* Return the number of pages available or SHRINK_EMPTY if we have none */
> +static unsigned long drm_page_pool_shrinker_count(struct shrinker *shrink,
> +						  struct shrink_control *sc)
> +{
> +	unsigned long num_pages = atomic_long_read(&allocated_pages);
> +
> +	return num_pages ? num_pages : SHRINK_EMPTY;
> +}
> +
> +/**
> + * drm_page_pool_shrinker_setup - Initialize globals
> + *
> + * @num_pages: default number of pages
> + *
> + * Initialize the global locks and lists for the shrinker.
> + */
> +int drm_page_pool_shrinker_setup(void)

Please call that one _init() and maybe give the pool size as parameter.

> +{
> +	mutex_init(&shrinker_lock);
> +	INIT_LIST_HEAD(&shrinker_list);
> +
> +	mm_shrinker.count_objects = drm_page_pool_shrinker_count;
> +	mm_shrinker.scan_objects = drm_page_pool_shrinker_scan;
> +	mm_shrinker.seeks = 1;
> +	return register_shrinker(&mm_shrinker);
> +}
> +
> +/**
> + * drm_page_pool_shrinker_teardown - Finalize globals
> + *
> + * Unregister the shrinker.
> + */
> +void drm_page_pool_shrinker_teardown(void)

Please call that _fini().

Regards,
Christian.

> +{
> +	unregister_shrinker(&mm_shrinker);
> +	WARN_ON(!list_empty(&shrinker_list));
> +}
> +
> +module_init(drm_page_pool_shrinker_setup);
> +module_exit(drm_page_pool_shrinker_teardown);
> +MODULE_LICENSE("Dual MIT/GPL");
> diff --git a/include/drm/page_pool.h b/include/drm/page_pool.h
> new file mode 100644
> index 000000000000..d8b8a8415629
> --- /dev/null
> +++ b/include/drm/page_pool.h
> @@ -0,0 +1,65 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> +/*
> + * Extracted from include/drm/ttm/ttm_pool.h
> + * Copyright 2020 Advanced Micro Devices, Inc.
> + * Copyright 2021 Linaro Ltd
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors: Christian König, John Stultz
> + */
> +
> +#ifndef _DRM_PAGE_POOL_H_
> +#define _DRM_PAGE_POOL_H_
> +
> +#include <linux/mmzone.h>
> +#include <linux/llist.h>
> +#include <linux/spinlock.h>
> +
> +/**
> + * drm_page_pool - Page Pool for a certain memory type
> + *
> + * @order: the allocation order our pages have
> + * @pages: the list of pages in the pool
> + * @shrinker_list: our place on the global shrinker list
> + * @lock: protection of the page list
> + * @page_count: number of pages currently in the pool
> + * @free: Function pointer to free the page
> + */
> +struct drm_page_pool {
> +	unsigned int order;
> +	struct list_head pages;
> +	struct list_head shrinker_list;
> +	spinlock_t lock;
> +
> +	unsigned long page_count;
> +	unsigned long (*free)(struct drm_page_pool *pool, struct page *p);
> +};
> +
> +void drm_page_pool_set_max(unsigned long max);
> +unsigned long drm_page_pool_get_max(void);
> +unsigned long drm_page_pool_get_total(void);
> +unsigned long drm_page_pool_get_size(struct drm_page_pool *pool);
> +void drm_page_pool_add(struct drm_page_pool *pool, struct page *p);
> +struct page *drm_page_pool_remove(struct drm_page_pool *pool);
> +void drm_page_pool_init(struct drm_page_pool *pool, unsigned int order,
> +			unsigned long (*free_page)(struct drm_page_pool *pool, struct page *p));
> +void drm_page_pool_fini(struct drm_page_pool *pool);
> +
> +#endif