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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxx>
Cc: Christian Koenig <christian.koenig@xxxxxxx>
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>
---
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

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux