Messages in this thread | | | Subject | Re: [RFC][PATCH v6 1/7] drm: Add a sharable drm page-pool implementation | From | Christian König <> | Date | Fri, 5 Feb 2021 09:46:54 +0100 |
| |
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
| |