On Tue, Dec 17, 2024 at 03:58:46PM +0100, Thomas Hellström wrote:
> Simplify the pool allocation code somewhat by merging loop arguments
> used by multiple functions together in a struct and simplifying the
> loop. Also add documentation.
> This hopefully makes the behaviour of the allocation loop
> simplier to understand, but above all paves the way for upcoming
> restore-while-allocating functionality.
>
> There are no functional changes, but the "allow_pools" bool
> introduced to keep current functionality could be removed as a
> follow up, which would enable using write-back cached pools when
> allocating memory for other caching modes, rather than to resort
> to allocating from the system directly.
>
> v15:
> - Introduce this patch to simplify the upcoming patch that introduces
> restore while allocating.
>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
Reviewed-by: Matthew Brost <matthew.brost@xxxxxxxxx>
> ---
> drivers/gpu/drm/ttm/ttm_pool.c | 183 +++++++++++++++++++--------------
> 1 file changed, 108 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
> index 8504dbe19c1a..c9eba76d5143 100644
> --- a/drivers/gpu/drm/ttm/ttm_pool.c
> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
> @@ -58,6 +58,23 @@ struct ttm_pool_dma {
> unsigned long vaddr;
> };
>
> +/**
> + * struct ttm_pool_alloc_state - Current state of the tt page allocation process
> + * @pages: Pointer to the next tt page pointer to populate.
> + * @caching_divide: Pointer to the first page pointer whose page has a staged but
> + * not committed caching transition from write-back to @tt_caching.
> + * @dma_addr: Pointer to the next tt dma_address entry to populate if any.
> + * @remaining_pages: Remaining pages to populate.
> + * @tt_caching: The requested cpu-caching for the pages allocated.
> + */
> +struct ttm_pool_alloc_state {
> + struct page **pages;
> + struct page **caching_divide;
> + dma_addr_t *dma_addr;
> + pgoff_t remaining_pages;
> + enum ttm_caching tt_caching;
> +};
> +
> static unsigned long page_pool_size;
>
> MODULE_PARM_DESC(page_pool_size, "Number of pages in the WC/UC/DMA pool");
> @@ -160,25 +177,25 @@ static void ttm_pool_free_page(struct ttm_pool *pool, enum ttm_caching caching,
> kfree(dma);
> }
>
> -/* Apply a new caching to an array of pages */
> -static int ttm_pool_apply_caching(struct page **first, struct page **last,
> - enum ttm_caching caching)
> +/* Apply any cpu-caching deferred during page allocation */
> +static int ttm_pool_apply_caching(struct ttm_pool_alloc_state *alloc)
> {
> #ifdef CONFIG_X86
> - unsigned int num_pages = last - first;
> + unsigned int num_pages = alloc->pages - alloc->caching_divide;
>
> if (!num_pages)
> return 0;
>
> - switch (caching) {
> + switch (alloc->tt_caching) {
> case ttm_cached:
> break;
> case ttm_write_combined:
> - return set_pages_array_wc(first, num_pages);
> + return set_pages_array_wc(alloc->caching_divide, num_pages);
> case ttm_uncached:
> - return set_pages_array_uc(first, num_pages);
> + return set_pages_array_uc(alloc->caching_divide, num_pages);
> }
> #endif
> + alloc->caching_divide = alloc->pages;
> return 0;
> }
>
> @@ -354,24 +371,41 @@ static unsigned int ttm_pool_page_order(struct ttm_pool *pool, struct page *p)
> return p->private;
> }
>
> -/* Called when we got a page, either from a pool or newly allocated */
> +/*
> + * Called when we got a page, either from a pool or newly allocated.
> + * if needed, dma map the page and populate the dma address array.
> + * Populate the page address array.
> + * If the caching is consistent, update any deferred caching. Otherwise
> + * stage this page for an upcoming deferred caching update.
> + */
> static int ttm_pool_page_allocated(struct ttm_pool *pool, unsigned int order,
> - struct page *p, dma_addr_t **dma_addr,
> - unsigned long *num_pages,
> - struct page ***pages)
> + struct page *p, enum ttm_caching page_caching,
> + struct ttm_pool_alloc_state *alloc)
> {
> - unsigned int i;
> - int r;
> + pgoff_t i, nr = 1UL << order;
> + bool caching_consistent;
> + int r = 0;
> +
> + caching_consistent = (page_caching == alloc->tt_caching) || PageHighMem(p);
> +
> + if (caching_consistent) {
> + r = ttm_pool_apply_caching(alloc);
> + if (r)
> + return r;
> + }
>
> - if (*dma_addr) {
> - r = ttm_pool_map(pool, order, p, dma_addr);
> + if (alloc->dma_addr) {
> + r = ttm_pool_map(pool, order, p, &alloc->dma_addr);
> if (r)
> return r;
> }
>
> - *num_pages -= 1 << order;
> - for (i = 1 << order; i; --i, ++(*pages), ++p)
> - **pages = p;
> + alloc->remaining_pages -= nr;
> + for (i = 0; i < nr; ++i)
> + *alloc->pages++ = p++;
> +
> + if (caching_consistent)
> + alloc->caching_divide = alloc->pages;
>
> return 0;
> }
> @@ -413,6 +447,26 @@ static void ttm_pool_free_range(struct ttm_pool *pool, struct ttm_tt *tt,
> }
> }
>
> +static void ttm_pool_alloc_state_init(const struct ttm_tt *tt,
> + struct ttm_pool_alloc_state *alloc)
> +{
> + alloc->pages = tt->pages;
> + alloc->caching_divide = tt->pages;
> + alloc->dma_addr = tt->dma_address;
> + alloc->remaining_pages = tt->num_pages;
> + alloc->tt_caching = tt->caching;
> +}
> +
> +/*
> + * Find a suitable allocation order based on highest desired order
> + * and number of remaining pages
> + */
> +static unsigned int ttm_pool_alloc_find_order(unsigned int highest,
> + const struct ttm_pool_alloc_state *alloc)
> +{
> + return min_t(unsigned int, highest, __fls(alloc->remaining_pages));
> +}
> +
> /**
> * ttm_pool_alloc - Fill a ttm_tt object
> *
> @@ -428,19 +482,19 @@ static void ttm_pool_free_range(struct ttm_pool *pool, struct ttm_tt *tt,
> int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt,
> struct ttm_operation_ctx *ctx)
> {
> - pgoff_t num_pages = tt->num_pages;
> - dma_addr_t *dma_addr = tt->dma_address;
> - struct page **caching = tt->pages;
> - struct page **pages = tt->pages;
> + struct ttm_pool_alloc_state alloc;
> enum ttm_caching page_caching;
> gfp_t gfp_flags = GFP_USER;
> pgoff_t caching_divide;
> unsigned int order;
> + bool allow_pools;
> struct page *p;
> int r;
>
> - WARN_ON(!num_pages || ttm_tt_is_populated(tt));
> - WARN_ON(dma_addr && !pool->dev);
> + ttm_pool_alloc_state_init(tt, &alloc);
> +
> + WARN_ON(!alloc.remaining_pages || ttm_tt_is_populated(tt));
> + WARN_ON(alloc.dma_addr && !pool->dev);
>
> if (tt->page_flags & TTM_TT_FLAG_ZERO_ALLOC)
> gfp_flags |= __GFP_ZERO;
> @@ -453,67 +507,46 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt,
> else
> gfp_flags |= GFP_HIGHUSER;
>
> - for (order = min_t(unsigned int, MAX_PAGE_ORDER, __fls(num_pages));
> - num_pages;
> - order = min_t(unsigned int, order, __fls(num_pages))) {
> + page_caching = tt->caching;
> + allow_pools = true;
> + for (order = ttm_pool_alloc_find_order(MAX_PAGE_ORDER, &alloc);
> + alloc.remaining_pages;
> + order = ttm_pool_alloc_find_order(order, &alloc)) {
> struct ttm_pool_type *pt;
>
> - page_caching = tt->caching;
> - pt = ttm_pool_select_type(pool, tt->caching, order);
> - p = pt ? ttm_pool_type_take(pt) : NULL;
> - if (p) {
> - r = ttm_pool_apply_caching(caching, pages,
> - tt->caching);
> - if (r)
> - goto error_free_page;
> -
> - caching = pages;
> - do {
> - r = ttm_pool_page_allocated(pool, order, p,
> - &dma_addr,
> - &num_pages,
> - &pages);
> - if (r)
> - goto error_free_page;
> -
> - caching = pages;
> - if (num_pages < (1 << order))
> - break;
> -
> - p = ttm_pool_type_take(pt);
> - } while (p);
> - }
> -
> - page_caching = ttm_cached;
> - while (num_pages >= (1 << order) &&
> - (p = ttm_pool_alloc_page(pool, gfp_flags, order))) {
> -
> - if (PageHighMem(p)) {
> - r = ttm_pool_apply_caching(caching, pages,
> - tt->caching);
> - if (r)
> - goto error_free_page;
> - caching = pages;
> - }
> - r = ttm_pool_page_allocated(pool, order, p, &dma_addr,
> - &num_pages, &pages);
> - if (r)
> - goto error_free_page;
> - if (PageHighMem(p))
> - caching = pages;
> + /* First, try to allocate a page from a pool if one exists. */
> + p = NULL;
> + pt = ttm_pool_select_type(pool, page_caching, order);
> + if (pt && allow_pools)
> + p = ttm_pool_type_take(pt);
> + /*
> + * If that fails or previously failed, allocate from system.
> + * Note that this also disallows additional pool allocations using
> + * write-back cached pools of the same order. Consider removing
> + * that behaviour.
> + */
> + if (!p) {
> + page_caching = ttm_cached;
> + allow_pools = false;
> + p = ttm_pool_alloc_page(pool, gfp_flags, order);
> }
> -
> + /* If that fails, lower the order if possible and retry. */
> if (!p) {
> if (order) {
> --order;
> + page_caching = tt->caching;
> + allow_pools = true;
> continue;
> }
> r = -ENOMEM;
> goto error_free_all;
> }
> + r = ttm_pool_page_allocated(pool, order, p, page_caching, &alloc);
> + if (r)
> + goto error_free_page;
> }
>
> - r = ttm_pool_apply_caching(caching, pages, tt->caching);
> + r = ttm_pool_apply_caching(&alloc);
> if (r)
> goto error_free_all;
>
> @@ -523,10 +556,10 @@ int ttm_pool_alloc(struct ttm_pool *pool, struct ttm_tt *tt,
> ttm_pool_free_page(pool, page_caching, order, p);
>
> error_free_all:
> - num_pages = tt->num_pages - num_pages;
> - caching_divide = caching - tt->pages;
> + caching_divide = alloc.caching_divide - tt->pages;
> ttm_pool_free_range(pool, tt, tt->caching, 0, caching_divide);
> - ttm_pool_free_range(pool, tt, ttm_cached, caching_divide, num_pages);
> + ttm_pool_free_range(pool, tt, ttm_cached, caching_divide,
> + tt->num_pages - alloc.remaining_pages);
>
> return r;
> }
> --
> 2.47.1
>
