Re: [RFC][PATCH v6 1/7] drm: Add a sharable drm page-pool implementation

Am 05.02.21 um 09:06 schrieb John Stultz:

This adds a shrinker controlled page pool, closely following the ttm_pool logic, which is abstracted out a bit so it can be used by other non-ttm drivers.

Cc: Daniel Vetter daniel@ffwll.ch Cc: Christian Koenig christian.koenig@amd.com Cc: Sumit Semwal sumit.semwal@linaro.org Cc: Liam Mark lmark@codeaurora.org Cc: Chris Goldsworthy cgoldswo@codeaurora.org Cc: Laura Abbott labbott@kernel.org Cc: Brian Starkey Brian.Starkey@arm.com Cc: Hridya Valsaraju hridya@google.com Cc: Suren Baghdasaryan surenb@google.com Cc: Sandeep Patil sspatil@google.com Cc: Daniel Mentz danielmentz@google.com Cc: Ørjan Eide orjan.eide@arm.com Cc: Robin Murphy robin.murphy@arm.com Cc: Ezequiel Garcia ezequiel@collabora.com Cc: Simon Ser contact@emersion.fr Cc: James Jones jajones@nvidia.com Cc: linux-media@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Signed-off-by: John Stultz john.stultz@linaro.org

---

drivers/gpu/drm/Kconfig | 4 + drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/page_pool.c | 220 ++++++++++++++++++++++++++++++++++++ include/drm/page_pool.h | 54 +++++++++ 4 files changed, 279 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 0973f408d75f..d16bf340ed2e 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -174,6 +174,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 fefaff4c832d..877e0111ed34 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -32,6 +32,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o drm-$(CONFIG_PCI) += drm_pci.o drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o +drm-$(CONFIG_DRM_PAGE_POOL) += page_pool.o

drm_vram_helper-y := drm_gem_vram_helper.o obj-$(CONFIG_DRM_VRAM_HELPER) += drm_vram_helper.o diff --git a/drivers/gpu/drm/page_pool.c b/drivers/gpu/drm/page_pool.c new file mode 100644 index 000000000000..2139f86e6ca7 --- /dev/null +++ b/drivers/gpu/drm/page_pool.c @@ -0,0 +1,220 @@ +// SPDX-License-Identifier: GPL-2.0

Please use a BSD/MIT compatible license if you want to copy this from the TTM code.

+/*

• • DRM page pool system
• • Copyright (C) 2020 Linaro Ltd.
• • Based on the ION page pool code
• • Copyright (C) 2011 Google, Inc.
• • As well as the ttm_pool code
• • Copyright (C) 2020 Advanced Micro Devices, Inc.
• */

+#include <linux/freezer.h> +#include <linux/list.h> +#include <linux/slab.h> +#include <linux/swap.h> +#include <linux/sched/signal.h> +#include <drm/page_pool.h>

+static LIST_HEAD(pool_list); +static DEFINE_MUTEX(pool_list_lock); +static atomic_long_t total_pages; +static unsigned long page_pool_max; +MODULE_PARM_DESC(page_pool_max, "Number of pages in the WC/UC/DMA pool"); +module_param(page_pool_max, ulong, 0644);

+void drm_page_pool_set_max(unsigned long max) +{

• /* only write once */
• if (!page_pool_max)

• page_pool_max = max;
  

+}

+unsigned long drm_page_pool_get_max(void) +{

• return page_pool_max;

+}

+unsigned long drm_page_pool_get_total(void) +{

• return atomic_long_read(&total_pages);

+}

+int drm_page_pool_get_size(struct drm_page_pool *pool) +{

• int ret;
• spin_lock(&pool->lock);
• ret = pool->count;
• spin_unlock(&pool->lock);

Maybe use an atomic for the count instead?

• return ret;

+}

+static inline unsigned int drm_page_pool_free_pages(struct drm_page_pool *pool,

• 				    struct page *page)
  

+{

• return pool->free(page, pool->order);

+}

+static int drm_page_pool_shrink_one(void);

+void drm_page_pool_add(struct drm_page_pool *pool, struct page *page) +{

• spin_lock(&pool->lock);
• list_add_tail(&page->lru, &pool->items);
• pool->count++;
• atomic_long_add(1 << pool->order, &total_pages);
• spin_unlock(&pool->lock);
• mod_node_page_state(page_pgdat(page), NR_KERNEL_MISC_RECLAIMABLE,

• 	    1 << pool->order);
  

Hui what? What should that be good for?

• /* make sure we don't grow too large */
• while (page_pool_max && atomic_long_read(&total_pages) > page_pool_max)

• drm_page_pool_shrink_one();
  

+} +EXPORT_SYMBOL_GPL(drm_page_pool_add);

+static struct page *drm_page_pool_remove(struct drm_page_pool *pool) +{

• struct page *page;
• if (!pool->count)

• return NULL;
  

Better use list_first_entry_or_null instead of checking the count.

This way you can also pull the lock into the function.

• page = list_first_entry(&pool->items, struct page, lru);
• pool->count--;
• atomic_long_sub(1 << pool->order, &total_pages);
• list_del(&page->lru);
• mod_node_page_state(page_pgdat(page), NR_KERNEL_MISC_RECLAIMABLE,

• 	    -(1 << pool->order));
  

• return page;

+}

+struct page *drm_page_pool_fetch(struct drm_page_pool *pool) +{

• struct page *page = NULL;
• if (!pool) {

• WARN_ON(!pool);
  

• return NULL;
  

• }
• spin_lock(&pool->lock);
• page = drm_page_pool_remove(pool);
• spin_unlock(&pool->lock);
• return page;

+} +EXPORT_SYMBOL_GPL(drm_page_pool_fetch);

+struct drm_page_pool *drm_page_pool_create(unsigned int order,

• 			   int (*free_page)(struct page *p, unsigned int order))
  

+{

• struct drm_page_pool *pool = kmalloc(sizeof(*pool), GFP_KERNEL);

Why not making this an embedded object? We should not see much dynamic pool creation.

• if (!pool)

• return NULL;
  

• pool->count = 0;
• INIT_LIST_HEAD(&pool->items);
• pool->order = order;
• pool->free = free_page;
• spin_lock_init(&pool->lock);
• INIT_LIST_HEAD(&pool->list);
• mutex_lock(&pool_list_lock);
• list_add(&pool->list, &pool_list);
• mutex_unlock(&pool_list_lock);
• return pool;

+} +EXPORT_SYMBOL_GPL(drm_page_pool_create);

+void drm_page_pool_destroy(struct drm_page_pool *pool) +{

• struct page *page;
• /* Remove us from the pool list */
• mutex_lock(&pool_list_lock);
• list_del(&pool->list);
• mutex_unlock(&pool_list_lock);
• /* Free any remaining pages in the pool */
• spin_lock(&pool->lock);

Locking should be unnecessary when the pool is destroyed anyway.

• while (pool->count) {

• page = drm_page_pool_remove(pool);
  

• spin_unlock(&pool->lock);
  

• drm_page_pool_free_pages(pool, page);
  

• spin_lock(&pool->lock);
  

• }
• spin_unlock(&pool->lock);
• kfree(pool);

+} +EXPORT_SYMBOL_GPL(drm_page_pool_destroy);

+static int drm_page_pool_shrink_one(void) +{

• struct drm_page_pool *pool;
• struct page *page;
• int nr_freed = 0;
• mutex_lock(&pool_list_lock);
• pool = list_first_entry(&pool_list, typeof(*pool), list);
• spin_lock(&pool->lock);
• page = drm_page_pool_remove(pool);
• spin_unlock(&pool->lock);
• if (page)

• nr_freed = drm_page_pool_free_pages(pool, page);
  

• list_move_tail(&pool->list, &pool_list);

Better to move this up, directly after the list_first_entry().

• mutex_unlock(&pool_list_lock);
• return nr_freed;

+}

+static unsigned long drm_page_pool_shrink_count(struct shrinker *shrinker,

• 				struct shrink_control *sc)
  

+{

• unsigned long count = atomic_long_read(&total_pages);
• return count ? count : SHRINK_EMPTY;

+}

+static unsigned long drm_page_pool_shrink_scan(struct shrinker *shrinker,

• 			       struct shrink_control *sc)
  

+{

• int to_scan = sc->nr_to_scan;
• int nr_total = 0;
• if (to_scan == 0)

• return 0;
  

• do {

• int nr_freed = drm_page_pool_shrink_one();
  

• to_scan -= nr_freed;
  

• nr_total += nr_freed;
  

• } while (to_scan >= 0 && atomic_long_read(&total_pages));
• return nr_total;

+}

+static struct shrinker pool_shrinker = {

• .count_objects = drm_page_pool_shrink_count,
• .scan_objects = drm_page_pool_shrink_scan,
• .seeks = 1,
• .batch = 0,

+};

+int drm_page_pool_init_shrinker(void) +{

• return register_shrinker(&pool_shrinker);

+} +module_init(drm_page_pool_init_shrinker); +MODULE_LICENSE("GPL v2"); diff --git a/include/drm/page_pool.h b/include/drm/page_pool.h new file mode 100644 index 000000000000..47e240b2bc69 --- /dev/null +++ b/include/drm/page_pool.h @@ -0,0 +1,54 @@ +/* SPDX-License-Identifier: GPL-2.0 OR MIT */ +/*

• • Copyright 2020 Advanced Micro Devices, Inc.
• • 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
• */

+#ifndef _DRM_PAGE_POOL_H_ +#define _DRM_PAGE_POOL_H_

+#include <linux/mmzone.h> +#include <linux/llist.h> +#include <linux/spinlock.h>

+struct drm_page_pool {

• int count;
• struct list_head items;
• int order;
• int (*free)(struct page *p, unsigned int order);
• spinlock_t lock;
• struct list_head list;

+};

+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); +int drm_page_pool_get_size(struct drm_page_pool *pool); +struct page *drm_page_pool_fetch(struct drm_page_pool *pool); +void drm_page_pool_add(struct drm_page_pool *pool, struct page *page); +struct drm_page_pool *drm_page_pool_create(unsigned int order,

• 			   int (*free_page)(struct page *p, unsigned int order));
  

+void drm_page_pool_destroy(struct drm_page_pool *pool);

+#endif