Messages in this thread |  | | Subject | Re: [RFC][PATCH v6 4/7] drm: ttm_pool: Rework ttm_pool to use drm_page_pool | From | Christian König <> | Date | Fri, 5 Feb 2021 09:50:49 +0100 |
| |
Am 05.02.21 um 09:06 schrieb John Stultz: > This patch reworks the ttm_pool logic to utilize the recently > added drm_page_pool code. > > Basically all the ttm_pool_type structures are replaced with > drm_page_pool pointers, and since the drm_page_pool logic has > its own shrinker, we can remove all of the ttm_pool shrinker > logic. > > NOTE: There is one mismatch in the interfaces I'm not totally > happy with. The ttm_pool tracks all of its pooled pages across > a number of different pools, and tries to keep this size under > the specified page_pool_size value. With the drm_page_pool, > there may other users, however there is still one global > shrinker list of pools. So we can't easily reduce the ttm > pool under the ttm specified size without potentially doing > a lot of shrinking to other non-ttm pools. So either we can: > 1) Try to split it so each user of drm_page_pools manages its > own pool shrinking. > 2) Push the max value into the drm_page_pool, and have it > manage shrinking to fit under that global max. Then share > those size/max values out so the ttm_pool debug output > can have more context. > > I've taken the second path in this patch set, but wanted to call > it out so folks could look closely.
Option 3: Move the debugging code into the drm_page_pool as well.
I strong think that will be a hard requirement from Daniel for upstreaming this.
Christian.
> > Thoughts would be greatly appreciated here! > > 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 | 1 + > drivers/gpu/drm/ttm/ttm_pool.c | 199 +++++++-------------------------- > include/drm/ttm/ttm_pool.h | 23 +--- > 3 files changed, 41 insertions(+), 182 deletions(-) > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index d16bf340ed2e..d427abefabfb 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -181,6 +181,7 @@ config DRM_PAGE_POOL > config DRM_TTM > tristate > depends on DRM && MMU > + select DRM_PAGE_POOL > help > GPU memory management subsystem for devices with multiple > GPU memory types. Will be enabled automatically if a device driver > diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c > index eca36678f967..dbbaf55ca5df 100644 > --- a/drivers/gpu/drm/ttm/ttm_pool.c > +++ b/drivers/gpu/drm/ttm/ttm_pool.c > @@ -37,6 +37,7 @@ > #ifdef CONFIG_X86 > #include <asm/set_memory.h> > #endif > +#include <drm/page_pool.h> > #include <drm/ttm/ttm_pool.h> > #include <drm/ttm/ttm_bo_driver.h> > #include <drm/ttm/ttm_tt.h> > @@ -63,15 +64,13 @@ module_param(page_pool_size, ulong, 0644); > > static atomic_long_t allocated_pages; > > -static struct ttm_pool_type global_write_combined[MAX_ORDER]; > -static struct ttm_pool_type global_uncached[MAX_ORDER]; > +static struct drm_page_pool *global_write_combined[MAX_ORDER]; > +static struct drm_page_pool *global_uncached[MAX_ORDER]; > > -static struct ttm_pool_type global_dma32_write_combined[MAX_ORDER]; > -static struct ttm_pool_type global_dma32_uncached[MAX_ORDER]; > +static struct drm_page_pool *global_dma32_write_combined[MAX_ORDER]; > +static struct drm_page_pool *global_dma32_uncached[MAX_ORDER]; > > static struct mutex shrinker_lock; > -static struct list_head shrinker_list; > -static struct shrinker mm_shrinker; > > /* Allocate pages of size 1 << order with the given gfp_flags */ > static struct page *ttm_pool_alloc_page(struct ttm_pool *pool, gfp_t gfp_flags, > @@ -223,79 +222,26 @@ static void ttm_pool_unmap(struct ttm_pool *pool, dma_addr_t dma_addr, > DMA_BIDIRECTIONAL); > } > > -/* Give pages into a specific pool_type */ > -static void ttm_pool_type_give(struct ttm_pool_type *pt, struct page *p) > -{ > - spin_lock(&pt->lock); > - list_add(&p->lru, &pt->pages); > - spin_unlock(&pt->lock); > - atomic_long_add(1 << pt->order, &allocated_pages); > -} > - > -/* Take pages from a specific pool_type, return NULL when nothing available */ > -static struct page *ttm_pool_type_take(struct ttm_pool_type *pt) > -{ > - struct page *p; > - > - spin_lock(&pt->lock); > - p = list_first_entry_or_null(&pt->pages, typeof(*p), lru); > - if (p) { > - atomic_long_sub(1 << pt->order, &allocated_pages); > - list_del(&p->lru); > - } > - spin_unlock(&pt->lock); > - > - return p; > -} > - > -/* Initialize and add a pool type to the global shrinker list */ > -static void ttm_pool_type_init(struct ttm_pool_type *pt, struct ttm_pool *pool, > - enum ttm_caching caching, unsigned int order) > -{ > - pt->pool = pool; > - pt->caching = caching; > - pt->order = order; > - spin_lock_init(&pt->lock); > - INIT_LIST_HEAD(&pt->pages); > - > - mutex_lock(&shrinker_lock); > - list_add_tail(&pt->shrinker_list, &shrinker_list); > - mutex_unlock(&shrinker_lock); > -} > - > -/* Remove a pool_type from the global shrinker list and free all pages */ > -static void ttm_pool_type_fini(struct ttm_pool_type *pt) > -{ > - struct page *p, *tmp; > - > - mutex_lock(&shrinker_lock); > - list_del(&pt->shrinker_list); > - mutex_unlock(&shrinker_lock); > - > - list_for_each_entry_safe(p, tmp, &pt->pages, lru) > - ttm_pool_free_page(p, pt->order); > -} > - > /* Return the pool_type to use for the given caching and order */ > -static struct ttm_pool_type *ttm_pool_select_type(struct ttm_pool *pool, > +static struct drm_page_pool *ttm_pool_select_type(struct ttm_pool *pool, > enum ttm_caching caching, > unsigned int order) > { > if (pool->use_dma_alloc) > - return &pool->caching[caching].orders[order]; > + return pool->caching[caching].orders[order]; > > #ifdef CONFIG_X86 > switch (caching) { > case ttm_write_combined: > if (pool->use_dma32) > - return &global_dma32_write_combined[order]; > + return global_dma32_write_combined[order]; > > - return &global_write_combined[order]; > + return global_write_combined[order]; > case ttm_uncached: > if (pool->use_dma32) > - return &global_dma32_uncached[order]; > + return global_dma32_uncached[order]; > > - return &global_uncached[order]; > + return global_uncached[order]; > default: > break; > } > @@ -304,30 +250,6 @@ static struct ttm_pool_type *ttm_pool_select_type(struct ttm_pool *pool, > return NULL; > } > > -/* Free pages using the global shrinker list */ > -static unsigned int ttm_pool_shrink(void) > -{ > - struct ttm_pool_type *pt; > - unsigned int num_freed; > - struct page *p; > - > - mutex_lock(&shrinker_lock); > - pt = list_first_entry(&shrinker_list, typeof(*pt), shrinker_list); > - > - p = ttm_pool_type_take(pt); > - if (p) { > - ttm_pool_free_page(p, pt->order); > - num_freed = 1 << pt->order; > - } else { > - num_freed = 0; > - } > - > - list_move_tail(&pt->shrinker_list, &shrinker_list); > - mutex_unlock(&shrinker_lock); > - > - return num_freed; > -} > - > /* Return the allocation order based for a page */ > static unsigned int ttm_pool_page_order(struct ttm_pool *pool, struct page *p) > { > @@ -377,10 +299,10 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt, > for (order = min(MAX_ORDER - 1UL, __fls(num_pages)); num_pages; > order = min_t(unsigned int, order, __fls(num_pages))) { > bool apply_caching = false; > - struct ttm_pool_type *pt; > + struct drm_page_pool *pt; > > pt = ttm_pool_select_type(pool, tt->caching, order); > - p = pt ? ttm_pool_type_take(pt) : NULL; > + p = pt ? drm_page_pool_fetch(pt) : NULL; > if (p) { > apply_caching = true; > } else { > @@ -462,7 +384,7 @@ void ttm_pool_free(struct ttm_pool *pool, struct ttm_tt *tt) > for (i = 0; i < tt->num_pages; ) { > struct page *p = tt->pages[i]; > unsigned int order, num_pages; > - struct ttm_pool_type *pt; > + struct drm_page_pool *pt; > > order = ttm_pool_page_order(pool, p); > num_pages = 1ULL << order; > @@ -473,15 +395,12 @@ void ttm_pool_free(struct ttm_pool *pool, struct ttm_tt *tt) > > pt = ttm_pool_select_type(pool, tt->caching, order); > if (pt) > - ttm_pool_type_give(pt, tt->pages[i]); > + drm_page_pool_add(pt, tt->pages[i]); > else > ttm_pool_free_page(tt->pages[i], order); > > i += num_pages; > } > - > - while (atomic_long_read(&allocated_pages) > page_pool_size) > - ttm_pool_shrink(); > } > EXPORT_SYMBOL(ttm_pool_free); > > @@ -508,8 +427,8 @@ void ttm_pool_init(struct ttm_pool *pool, struct device *dev, > > for (i = 0; i < TTM_NUM_CACHING_TYPES; ++i) > for (j = 0; j < MAX_ORDER; ++j) > - ttm_pool_type_init(&pool->caching[i].orders[j], > - pool, i, j); > + pool->caching[i].orders[j] = drm_page_pool_create(j, > + ttm_pool_free_page); > } > > /** > @@ -526,33 +445,18 @@ void ttm_pool_fini(struct ttm_pool *pool) > > for (i = 0; i < TTM_NUM_CACHING_TYPES; ++i) > for (j = 0; j < MAX_ORDER; ++j) > - ttm_pool_type_fini(&pool->caching[i].orders[j]); > + drm_page_pool_destroy(pool->caching[i].orders[j]); > } > > #ifdef CONFIG_DEBUG_FS > -/* Count the number of pages available in a pool_type */ > -static unsigned int ttm_pool_type_count(struct ttm_pool_type *pt) > -{ > - unsigned int count = 0; > - struct page *p; > - > - spin_lock(&pt->lock); > - /* Only used for debugfs, the overhead doesn't matter */ > - list_for_each_entry(p, &pt->pages, lru) > - ++count; > - spin_unlock(&pt->lock); > - > - return count; > -} > - > /* Dump information about the different pool types */ > -static void ttm_pool_debugfs_orders(struct ttm_pool_type *pt, > +static void ttm_pool_debugfs_orders(struct drm_page_pool **pt, > struct seq_file *m) > { > unsigned int i; > > for (i = 0; i < MAX_ORDER; ++i) > - seq_printf(m, " %8u", ttm_pool_type_count(&pt[i])); > + seq_printf(m, " %8u", drm_page_pool_get_size(pt[i])); > seq_puts(m, "\n"); > } > > @@ -602,7 +506,10 @@ int ttm_pool_debugfs(struct ttm_pool *pool, struct seq_file *m) > } > > seq_printf(m, "\ntotal\t: %8lu of %8lu\n", > - atomic_long_read(&allocated_pages), page_pool_size); > + atomic_long_read(&allocated_pages), > + drm_page_pool_get_max()); > + seq_printf(m, "(%8lu in non-ttm pools)\n", drm_page_pool_get_total() - > + atomic_long_read(&allocated_pages)); > > mutex_unlock(&shrinker_lock); > > @@ -612,28 +519,6 @@ EXPORT_SYMBOL(ttm_pool_debugfs); > > #endif > > -/* As long as pages are available make sure to release at least one */ > -static unsigned long ttm_pool_shrinker_scan(struct shrinker *shrink, > - struct shrink_control *sc) > -{ > - unsigned long num_freed = 0; > - > - do > - num_freed += ttm_pool_shrink(); > - while (!num_freed && atomic_long_read(&allocated_pages)); > - > - return num_freed; > -} > - > -/* Return the number of pages available or SHRINK_EMPTY if we have none */ > -static unsigned long ttm_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; > -} > - > /** > * ttm_pool_mgr_init - Initialize globals > * > @@ -648,24 +533,21 @@ int ttm_pool_mgr_init(unsigned long num_pages) > if (!page_pool_size) > page_pool_size = num_pages; > > + drm_page_pool_set_max(page_pool_size); > + > mutex_init(&shrinker_lock); > - INIT_LIST_HEAD(&shrinker_list); > > for (i = 0; i < MAX_ORDER; ++i) { > - ttm_pool_type_init(&global_write_combined[i], NULL, > - ttm_write_combined, i); > - ttm_pool_type_init(&global_uncached[i], NULL, ttm_uncached, i); > - > - ttm_pool_type_init(&global_dma32_write_combined[i], NULL, > - ttm_write_combined, i); > - ttm_pool_type_init(&global_dma32_uncached[i], NULL, > - ttm_uncached, i); > + global_write_combined[i] = drm_page_pool_create(i, > + ttm_pool_free_page); > + global_uncached[i] = drm_page_pool_create(i, > + ttm_pool_free_page); > + global_dma32_write_combined[i] = drm_page_pool_create(i, > + ttm_pool_free_page); > + global_dma32_uncached[i] = drm_page_pool_create(i, > + ttm_pool_free_page); > } > - > - mm_shrinker.count_objects = ttm_pool_shrinker_count; > - mm_shrinker.scan_objects = ttm_pool_shrinker_scan; > - mm_shrinker.seeks = 1; > - return register_shrinker(&mm_shrinker); > + return 0; > } > > /** > @@ -678,13 +560,10 @@ void ttm_pool_mgr_fini(void) > unsigned int i; > > for (i = 0; i < MAX_ORDER; ++i) { > - ttm_pool_type_fini(&global_write_combined[i]); > - ttm_pool_type_fini(&global_uncached[i]); > + drm_page_pool_destroy(global_write_combined[i]); > + drm_page_pool_destroy(global_uncached[i]); > > - ttm_pool_type_fini(&global_dma32_write_combined[i]); > - ttm_pool_type_fini(&global_dma32_uncached[i]); > + drm_page_pool_destroy(global_dma32_write_combined[i]); > + drm_page_pool_destroy(global_dma32_uncached[i]); > } > - > - unregister_shrinker(&mm_shrinker); > - WARN_ON(!list_empty(&shrinker_list)); > } > diff --git a/include/drm/ttm/ttm_pool.h b/include/drm/ttm/ttm_pool.h > index 4321728bdd11..460ab232fd22 100644 > --- a/include/drm/ttm/ttm_pool.h > +++ b/include/drm/ttm/ttm_pool.h > @@ -36,27 +36,6 @@ struct ttm_tt; > struct ttm_pool; > struct ttm_operation_ctx; > > -/** > - * ttm_pool_type - Pool for a certain memory type > - * > - * @pool: the pool we belong to, might be NULL for the global ones > - * @order: the allocation order our pages have > - * @caching: the caching type our pages have > - * @shrinker_list: our place on the global shrinker list > - * @lock: protection of the page list > - * @pages: the list of pages in the pool > - */ > -struct ttm_pool_type { > - struct ttm_pool *pool; > - unsigned int order; > - enum ttm_caching caching; > - > - struct list_head shrinker_list; > - > - spinlock_t lock; > - struct list_head pages; > -}; > - > /** > * ttm_pool - Pool for all caching and orders > * > @@ -71,7 +50,7 @@ struct ttm_pool { > bool use_dma32; > > struct { > - struct ttm_pool_type orders[MAX_ORDER]; > + struct drm_page_pool *orders[MAX_ORDER]; > } caching[TTM_NUM_CACHING_TYPES]; > }; >
|  |