lkml.org 
[lkml]   [2021]   [Feb]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC][PATCH v6 1/7] drm: Add a sharable drm page-pool implementation
From
Date
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

\
 
 \ /
  Last update: 2021-02-05 09:49    [W:0.251 / U:0.108 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site