On Wed, 26 Jan 2022, Matthew Auld matthew.auld@intel.com wrote:

From: Arunpravin Arunpravin.PaneerSelvam@amd.com

• Make drm_buddy_alloc a single function to handle range allocation and non-range allocation demands

• Implemented a new function alloc_range() which allocates the requested power-of-two block comply with range limitations

• Moved order computation and memory alignment logic from i915 driver to drm buddy

v2: merged below changes to keep the build unbroken

• drm_buddy_alloc_range() becomes obsolete and may be removed
• enable ttm range allocation (fpfn / lpfn) support in i915 driver
• apply enhanced drm_buddy_alloc() function to i915 driver

v3(Matthew Auld):

• Fix alignment issues and remove unnecessary list_empty check
• add more validation checks for input arguments
• make alloc_range() block allocations as bottom-up
• optimize order computation logic
• replace uint64_t with u64, which is preferred in the kernel

v4(Matthew Auld):

• keep drm_buddy_alloc_range() function implementation for generic actual range allocations
• keep alloc_range() implementation for end bias allocations

v5(Matthew Auld):

• modify drm_buddy_alloc() passing argument place->lpfn to lpfn as place->lpfn will currently always be zero for i915

v6(Matthew Auld):

• fixup potential uaf - If we are unlucky and can't allocate enough memory when splitting blocks, where we temporarily end up with the given block and its buddy on the respective free list, then we need to ensure we delete both blocks, and no just the buddy, before potentially freeing them

• fix warnings reported by kernel test robot lkp@intel.com

Signed-off-by: Arunpravin Arunpravin.PaneerSelvam@amd.com

drivers/gpu/drm/drm_buddy.c | 326 +++++++++++++----- drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 67 ++-- drivers/gpu/drm/i915/i915_ttm_buddy_manager.h | 2 + include/drm/drm_buddy.h | 22 +- 4 files changed, 293 insertions(+), 124 deletions(-)

diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c index d60878bc9c20..954e31962c74 100644 --- a/drivers/gpu/drm/drm_buddy.c +++ b/drivers/gpu/drm/drm_buddy.c @@ -282,23 +282,99 @@ void drm_buddy_free_list(struct drm_buddy *mm, struct list_head *objects) } EXPORT_SYMBOL(drm_buddy_free_list);

-/**

• • drm_buddy_alloc_blocks - allocate power-of-two blocks
• • @mm: DRM buddy manager to allocate from
• • @order: size of the allocation
• • The order value here translates to:
• • 0 = 2^0 * mm->chunk_size
• • 1 = 2^1 * mm->chunk_size
• • 2 = 2^2 * mm->chunk_size
• • Returns:
• • allocated ptr to the &drm_buddy_block on success
• */

-struct drm_buddy_block * -drm_buddy_alloc_blocks(struct drm_buddy *mm, unsigned int order) +static inline bool overlaps(u64 s1, u64 e1, u64 s2, u64 e2) +{

• return s1 <= e2 && e1 >= s2;

+}

+static inline bool contains(u64 s1, u64 e1, u64 s2, u64 e2) +{

• return s1 <= s2 && e1 >= e2;

+}

+static struct drm_buddy_block * +alloc_range_bias(struct drm_buddy *mm,

•  u64 start, u64 end,
  

•  unsigned int order)
  

+{

• struct drm_buddy_block *block;
• struct drm_buddy_block *buddy;
• LIST_HEAD(dfs);
• int err;
• int i;
• end = end - 1;
• for (i = 0; i < mm->n_roots; ++i)

• list_add_tail(&mm->roots[i]->tmp_link, &dfs);
  

• do {

• u64 block_start;
  

• u64 block_end;
  

• block = list_first_entry_or_null(&dfs,
  

• 				 struct drm_buddy_block,
  

• 				 tmp_link);
  

• if (!block)
  

• 	break;
  

• list_del(&block->tmp_link);
  

• if (drm_buddy_block_order(block) < order)
  

• 	continue;
  

• block_start = drm_buddy_block_offset(block);
  

• block_end = block_start + drm_buddy_block_size(mm, block) - 1;
  

• if (!overlaps(start, end, block_start, block_end))
  

• 	continue;
  

• if (drm_buddy_block_is_allocated(block))
  

• 	continue;
  

• if (contains(start, end, block_start, block_end) &&
  

•     order == drm_buddy_block_order(block)) {
  

• 	/*
  

• 	 * Find the free block within the range.
  

• 	 */
  

• 	if (drm_buddy_block_is_free(block))
  

• 		return block;
  

• 	continue;
  

• }
  

• if (!drm_buddy_block_is_split(block)) {
  

• 	err = split_block(mm, block);
  

• 	if (unlikely(err))
  

• 		goto err_undo;
  

• }
  

• list_add(&block->right->tmp_link, &dfs);
  

• list_add(&block->left->tmp_link, &dfs);
  

• } while (1);
• return ERR_PTR(-ENOSPC);

+err_undo:

• /*

• * We really don't want to leave around a bunch of split blocks, since
  

• * bigger is better, so make sure we merge everything back before we
  

• * free the allocated blocks.
  

• */
  

• buddy = get_buddy(block);
• if (buddy &&

•    (drm_buddy_block_is_free(block) &&
  

•     drm_buddy_block_is_free(buddy))) {
  

• list_del(&block->link);
  

• __drm_buddy_free(mm, block);
  

• }
• return ERR_PTR(err);

+}

+static struct drm_buddy_block * +alloc_from_freelist(struct drm_buddy *mm,

•     unsigned int order,
  

•     unsigned long flags)
  

{ struct drm_buddy_block *block = NULL; unsigned int i; @@ -320,78 +396,30 @@ drm_buddy_alloc_blocks(struct drm_buddy *mm, unsigned int order) while (i != order) { err = split_block(mm, block); if (unlikely(err))

• 	goto out_free;
  

• 	goto err_undo;
  
  

• /* Go low */
  

• block = block->left;
  

• block = block->right;
  

  i--; }

• mark_allocated(block);
• mm->avail -= drm_buddy_block_size(mm, block);
• kmemleak_update_trace(block); return block;

-out_free:

• if (i != order)

+err_undo:

• if (i != order) {

• list_del(&block->link);
  

  __drm_buddy_free(mm, block);
• } return ERR_PTR(err);

} -EXPORT_SYMBOL(drm_buddy_alloc_blocks);

-static inline bool overlaps(u64 s1, u64 e1, u64 s2, u64 e2) -{

• return s1 <= e2 && e1 >= s2;

-}

-static inline bool contains(u64 s1, u64 e1, u64 s2, u64 e2) -{

• return s1 <= s2 && e1 >= e2;

-}

-/**

• • drm_buddy_alloc_range - allocate range
• • @mm: DRM buddy manager to allocate from
• • @blocks: output list head to add allocated blocks
• • @start: start of the allowed range for this block
• • @size: size of the allocation
• • Intended for pre-allocating portions of the address space, for example to
• • reserve a block for the initial framebuffer or similar, hence the expectation
• • here is that drm_buddy_alloc_blocks() is still the main vehicle for
• • allocations, so if that's not the case then the drm_mm range allocator is
• • probably a much better fit, and so you should probably go use that instead.
• • Note that it's safe to chain together multiple alloc_ranges
• • with the same blocks list
• • Returns:
• • 0 on success, error code on failure.
• */

-int drm_buddy_alloc_range(struct drm_buddy *mm,

• 	  struct list_head *blocks,
  

• 	  u64 start, u64 size)
  

+static int __alloc_range(struct drm_buddy *mm,

• 	 struct list_head *dfs,
  

• 	 u64 start, u64 size,
  

• 	 struct list_head *blocks)
  

{ struct drm_buddy_block *block; struct drm_buddy_block *buddy;

• LIST_HEAD(allocated);

• LIST_HEAD(dfs); u64 end; int err;

• int i;

• if (size < mm->chunk_size)

• return -EINVAL;
  

• if (!IS_ALIGNED(size | start, mm->chunk_size))

• return -EINVAL;
  

• if (range_overflows(start, size, mm->size))

• return -EINVAL;
  

• for (i = 0; i < mm->n_roots; ++i)

• list_add_tail(&mm->roots[i]->tmp_link, &dfs);
  
  

  end = start + size - 1;

@@ -399,7 +427,7 @@ int drm_buddy_alloc_range(struct drm_buddy *mm, u64 block_start; u64 block_end;

• block = list_first_entry_or_null(&dfs,
  

• block = list_first_entry_or_null(dfs,
  			 struct drm_buddy_block,
  			 tmp_link);
  

  if (!block)

@@ -426,7 +454,7 @@ int drm_buddy_alloc_range(struct drm_buddy *mm,

	mark_allocated(block);
	mm->avail -= drm_buddy_block_size(mm, block);

• 	list_add_tail(&block->link, &allocated);
  

• 	list_add_tail(&block->link, blocks);
  continue;
  

  }

@@ -436,11 +464,10 @@ int drm_buddy_alloc_range(struct drm_buddy *mm, goto err_undo; }

• list_add(&block->right->tmp_link, &dfs);
  

• list_add(&block->left->tmp_link, &dfs);
  

• list_add(&block->right->tmp_link, dfs);
  

• list_add(&block->left->tmp_link, dfs);
  

  } while (1);

• list_splice_tail(&allocated, blocks); return 0;

err_undo: @@ -452,14 +479,149 @@ int drm_buddy_alloc_range(struct drm_buddy *mm, buddy = get_buddy(block); if (buddy && (drm_buddy_block_is_free(block) &&

•     drm_buddy_block_is_free(buddy)))
  

•     drm_buddy_block_is_free(buddy))) {
  

• list_del(&block->link);
  

  __drm_buddy_free(mm, block);
• }

+err_free:

• drm_buddy_free_list(mm, blocks);
• return err;

+}

+/**

• • __drm_buddy_alloc_range - actual range allocation
• • @mm: DRM buddy manager to allocate from
• • @start: start of the allowed range for this block
• • @size: size of the allocation
• • @blocks: output list head to add allocated blocks
• • Intended for pre-allocating portions of the address space, for example to
• • reserve a block for the initial framebuffer or similar
• • Note that it's safe to chain together multiple alloc_ranges
• • with the same blocks list
• • Returns:
• • 0 on success, error code on failure.
• */

+static int __drm_buddy_alloc_range(struct drm_buddy *mm,

• 		   u64 start,
  

• 		   u64 size,
  

• 		   struct list_head *blocks)
  

+{

• LIST_HEAD(dfs);
• int i;
• for (i = 0; i < mm->n_roots; ++i)

• list_add_tail(&mm->roots[i]->tmp_link, &dfs);
  

• return __alloc_range(mm, &dfs, start, size, blocks);

+}

+/**

• • drm_buddy_alloc_blocks - allocate power-of-two blocks
• • @mm: DRM buddy manager to allocate from
• • @start: start of the allowed range for this block
• • @end: end of the allowed range for this block
• • @size: size of the allocation
• • @min_page_size: alignment of the allocation
• • @blocks: output list head to add allocated blocks
• • @flags: DRM_BUDDY_*_ALLOCATION flags
• • alloc_range_bias() called on range limitations, which traverses
• • the tree and returns the desired block.
• • alloc_from_freelist() called when *no* range restrictions
• • are enforced, which picks the block from the freelist.
• • blocks are allocated in order, the order value here translates to:
• • 0 = 2^0 * mm->chunk_size
• • 1 = 2^1 * mm->chunk_size
• • 2 = 2^2 * mm->chunk_size
• • Returns:
• • 0 on success, error code on failure.
• */

+int drm_buddy_alloc_blocks(struct drm_buddy *mm,

• 	   u64 start, u64 end, u64 size,
  

• 	   u64 min_page_size,
  

• 	   struct list_head *blocks,
  

• 	   unsigned long flags)
  

+{

• struct drm_buddy_block *block = NULL;
• unsigned int min_order, order;
• unsigned long pages;
• LIST_HEAD(allocated);
• int err;
• if (size < mm->chunk_size)

• return -EINVAL;
  

• if (min_page_size < mm->chunk_size)

• return -EINVAL;
  

• if (!is_power_of_2(min_page_size))

• return -EINVAL;
  

• if (!IS_ALIGNED(start | end | size, mm->chunk_size))

• return -EINVAL;
  

• if (check_range_overflow(start, end, size, mm->size))

• return -EINVAL;
  

• /* Actual range allocation */
• if (start + size == end)

• return __drm_buddy_alloc_range(mm, start, size, blocks);
  

• pages = size >> ilog2(mm->chunk_size);
• order = fls(pages) - 1;
• min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
• do {

• order = min(order, (unsigned int)fls(pages) - 1);
  

• BUG_ON(order > mm->max_order);
  

• BUG_ON(order < min_order);
  

• do {
  

• 	if (flags & DRM_BUDDY_RANGE_ALLOCATION)
  

• 		/* Allocate traversing within the range */
  

• 		block = alloc_range_bias(mm, start, end, order);
  

• 	else
  

• 		/* Allocate from freelist */
  

• 		block = alloc_from_freelist(mm, order, flags);
  

• 	if (!IS_ERR(block))
  

• 		break;
  

• 	if (order-- == min_order) {
  

• 		err = -ENOSPC;
  

• 		goto err_free;
  

• 	}
  

• } while (1);
  

• mark_allocated(block);
  

• mm->avail -= drm_buddy_block_size(mm, block);
  

• kmemleak_update_trace(block);
  

• list_add_tail(&block->link, &allocated);
  

• pages -= BIT(order);
  

• if (!pages)
  

• 	break;
  

• } while (1);
• list_splice_tail(&allocated, blocks);
• return 0;

err_free: drm_buddy_free_list(mm, &allocated); return err; } -EXPORT_SYMBOL(drm_buddy_alloc_range); +EXPORT_SYMBOL(drm_buddy_alloc_blocks);

/**

• drm_buddy_block_print - print block information

diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c index 6ba314f9836a..1411f4cf1f21 100644 --- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c +++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c @@ -36,13 +36,14 @@ static int i915_ttm_buddy_man_alloc(struct ttm_resource_manager *man, struct i915_ttm_buddy_manager *bman = to_buddy_manager(man); struct i915_ttm_buddy_resource *bman_res; struct drm_buddy *mm = &bman->mm;

• unsigned long n_pages;
• unsigned int min_order;

• unsigned long n_pages, lpfn; u64 min_page_size; u64 size; int err;

• GEM_BUG_ON(place->fpfn || place->lpfn);

• lpfn = place->lpfn;

• if (!lpfn)

• lpfn = man->size;
  
  

  bman_res = kzalloc(sizeof(*bman_res), GFP_KERNEL); if (!bman_res)

@@ -52,6 +53,9 @@ static int i915_ttm_buddy_man_alloc(struct ttm_resource_manager *man, INIT_LIST_HEAD(&bman_res->blocks); bman_res->mm = mm;

• if (place->fpfn || lpfn != man->size)

• bman_res->flags |= DRM_BUDDY_RANGE_ALLOCATION;
  

• GEM_BUG_ON(!bman_res->base.num_pages); size = bman_res->base.num_pages << PAGE_SHIFT;

@@ -60,10 +64,16 @@ static int i915_ttm_buddy_man_alloc(struct ttm_resource_manager *man, min_page_size = bo->page_alignment << PAGE_SHIFT;

GEM_BUG_ON(min_page_size < mm->chunk_size);

• min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);

• if (place->flags & TTM_PL_FLAG_CONTIGUOUS) {

• unsigned long pages;
  

• size = roundup_pow_of_two(size);

• min_order = ilog2(size) - ilog2(mm->chunk_size);
  

• min_page_size = size;
  

• pages = size >> ilog2(mm->chunk_size);
  

• if (pages > lpfn)
  

• 	lpfn = pages;
  

  }

  if (size > mm->size) {

@@ -73,34 +83,16 @@ static int i915_ttm_buddy_man_alloc(struct ttm_resource_manager *man,

n_pages = size >> ilog2(mm->chunk_size);

• do {

• struct drm_buddy_block *block;
  

• unsigned int order;
  

• order = fls(n_pages) - 1;
  

• GEM_BUG_ON(order > mm->max_order);
  

• GEM_BUG_ON(order < min_order);
  

• do {
  

• 	mutex_lock(&bman->lock);
  

• 	block = drm_buddy_alloc_blocks(mm, order);
  

• 	mutex_unlock(&bman->lock);
  

• 	if (!IS_ERR(block))
  

• 		break;
  

• 	if (order-- == min_order) {
  

• 		err = -ENOSPC;
  

• 		goto err_free_blocks;
  

• 	}
  

• } while (1);
  

• n_pages -= BIT(order);
  

• list_add_tail(&block->link, &bman_res->blocks);
  

• if (!n_pages)
  

• 	break;
  

• } while (1);

• mutex_lock(&bman->lock);

• err = drm_buddy_alloc_blocks(mm, (u64)place->fpfn << PAGE_SHIFT,

• 		    (u64)lpfn << PAGE_SHIFT,
  

• 		    (u64)n_pages << PAGE_SHIFT,
  

• 		     min_page_size,
  

• 		     &bman_res->blocks,
  

• 		     bman_res->flags);
  

• mutex_unlock(&bman->lock);

• if (unlikely(err))

• goto err_free_blocks;
  
  

  *res = &bman_res->base; return 0;

@@ -266,10 +258,17 @@ int i915_ttm_buddy_man_reserve(struct ttm_resource_manager *man, { struct i915_ttm_buddy_manager *bman = to_buddy_manager(man); struct drm_buddy *mm = &bman->mm;

• unsigned long flags = 0; int ret;

• flags |= DRM_BUDDY_RANGE_ALLOCATION;

• mutex_lock(&bman->lock);

• ret = drm_buddy_alloc_range(mm, &bman->reserved, start, size);

• ret = drm_buddy_alloc_blocks(mm, start,

• 		     start + size,
  

• 		     size, mm->chunk_size,
  

• 		     &bman->reserved,
  

• 		     flags);
  

  mutex_unlock(&bman->lock);

  return ret;

diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.h b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.h index 312077941411..72c90b432e87 100644 --- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.h +++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.h @@ -20,6 +20,7 @@ struct drm_buddy;

• @base: struct ttm_resource base class we extend
• @blocks: the list of struct i915_buddy_block for this resource/allocation

• • @flags: DRM_BUDDY_*_ALLOCATION flags
  • @mm: the struct i915_buddy_mm for this resource
  • Extends the struct ttm_resource to manage an address space allocation with

@@ -28,6 +29,7 @@ struct drm_buddy; struct i915_ttm_buddy_resource { struct ttm_resource base; struct list_head blocks;

• unsigned long flags; struct drm_buddy *mm;

};

diff --git a/include/drm/drm_buddy.h b/include/drm/drm_buddy.h index f524db152413..865664b90a8a 100644 --- a/include/drm/drm_buddy.h +++ b/include/drm/drm_buddy.h @@ -13,15 +13,22 @@

#include <drm/drm_print.h>

-#define range_overflows(start, size, max) ({ \ +#define check_range_overflow(start, end, size, max) ({ \

Random drive-by comment, why the rename? The old one is understandable, it's a bool, while "check", to me, implies it might *do* something in case of overflow.

typeof(start) start__ = (start); \

• typeof(end) end__ = (end);\ typeof(size) size__ = (size); \ typeof(max) max__ = (max); \ (void)(&start__ == &size__); \ (void)(&start__ == &max__); \

• start__ >= max__ || size__ > max__ - start__; \

• (void)(&start__ == &end__); \
• (void)(&end__ == &size__); \
• (void)(&end__ == &max__); \
• start__ >= max__ || end__ > max__ || \
• size__ > end__ - start__; \

})

+#define DRM_BUDDY_RANGE_ALLOCATION (1 << 0)

struct drm_buddy_block { #define DRM_BUDDY_HEADER_OFFSET GENMASK_ULL(63, 12) #define DRM_BUDDY_HEADER_STATE GENMASK_ULL(11, 10) @@ -131,12 +138,11 @@ int drm_buddy_init(struct drm_buddy *mm, u64 size, u64 chunk_size);

void drm_buddy_fini(struct drm_buddy *mm);

-struct drm_buddy_block * -drm_buddy_alloc_blocks(struct drm_buddy *mm, unsigned int order);

-int drm_buddy_alloc_range(struct drm_buddy *mm,

• 	  struct list_head *blocks,
  

• 	  u64 start, u64 size);
  

+int drm_buddy_alloc_blocks(struct drm_buddy *mm,

• 	   u64 start, u64 end, u64 size,
  

• 	   u64 min_page_size,
  

• 	   struct list_head *blocks,
  

• 	   unsigned long flags);
  
  

void drm_buddy_free_block(struct drm_buddy *mm, struct drm_buddy_block *block);

On 26/01/2022 15:21, Matthew Auld wrote:

From: Arunpravin Arunpravin.PaneerSelvam@amd.com

Implemented a function which walk through the order list, compares the offset and returns the maximum offset block, this method is unpredictable in obtaining the high range address blocks which depends on allocation and deallocation. for instance, if driver requests address at a low specific range, allocator traverses from the root block and splits the larger blocks until it reaches the specific block and in the process of splitting, lower orders in the freelist are occupied with low range address blocks and for the subsequent TOPDOWN memory request we may return the low range blocks.To overcome this issue, we may go with the below approach.

The other approach, sorting each order list entries in ascending order and compares the last entry of each order list in the freelist and return the max block. This creates sorting overhead on every drm_buddy_free() request and split up of larger blocks for a single page request.

ooc, why did you choose to implement this as an alloc flag? Seems to me like it would be a good candidate for a new memory region. That way allocation algorithms wouldn't need exta logic and ttm can already handle migrations.

v2:

• Fix alignment issues(Matthew Auld)
• Remove unnecessary list_empty check(Matthew Auld)
• merged the below patch to see the feature in action
  • add top-down alloc support to i915 driver

Signed-off-by: Arunpravin Arunpravin.PaneerSelvam@amd.com

drivers/gpu/drm/drm_buddy.c | 36 ++++++++++++++++--- drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 3 ++ include/drm/drm_buddy.h | 1 + 3 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c index 954e31962c74..6aa5c1ce25bf 100644 --- a/drivers/gpu/drm/drm_buddy.c +++ b/drivers/gpu/drm/drm_buddy.c @@ -371,6 +371,26 @@ alloc_range_bias(struct drm_buddy *mm, return ERR_PTR(err); }

+static struct drm_buddy_block * +get_maxblock(struct list_head *head) +{

• struct drm_buddy_block *max_block = NULL, *node;
• max_block = list_first_entry_or_null(head,

• 			     struct drm_buddy_block,
  

• 			     link);
  

• if (!max_block)

• return NULL;
  

• list_for_each_entry(node, head, link) {

• if (drm_buddy_block_offset(node) >
  

•     drm_buddy_block_offset(max_block))
  

• 	max_block = node;
  

• }
• return max_block;

+}

• static struct drm_buddy_block * alloc_from_freelist(struct drm_buddy *mm, unsigned int order,

@@ -381,11 +401,17 @@ alloc_from_freelist(struct drm_buddy *mm, int err;

for (i = order; i <= mm->max_order; ++i) {

• block = list_first_entry_or_null(&mm->free_list[i],
  

• 				 struct drm_buddy_block,
  

• 				 link);
  

• if (block)
  

• 	break;
  

• if (flags & DRM_BUDDY_TOPDOWN_ALLOCATION) {
  

• 	block = get_maxblock(&mm->free_list[i]);
  

• 	if (block)
  

• 		break;
  

• } else {
  

• 	block = list_first_entry_or_null(&mm->free_list[i],
  

• 					 struct drm_buddy_block,
  

• 					 link);
  

• 	if (block)
  

• 		break;
  

• }
  

  }

  if (!block)

diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c index 1411f4cf1f21..3662434b64bb 100644 --- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c +++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c @@ -53,6 +53,9 @@ static int i915_ttm_buddy_man_alloc(struct ttm_resource_manager *man, INIT_LIST_HEAD(&bman_res->blocks); bman_res->mm = mm;

• if (place->flags & TTM_PL_FLAG_TOPDOWN)

• bman_res->flags |= DRM_BUDDY_TOPDOWN_ALLOCATION;
  

• if (place->fpfn || lpfn != man->size) bman_res->flags |= DRM_BUDDY_RANGE_ALLOCATION;

diff --git a/include/drm/drm_buddy.h b/include/drm/drm_buddy.h index 865664b90a8a..424fc443115e 100644 --- a/include/drm/drm_buddy.h +++ b/include/drm/drm_buddy.h @@ -28,6 +28,7 @@ })

#define DRM_BUDDY_RANGE_ALLOCATION (1 << 0) +#define DRM_BUDDY_TOPDOWN_ALLOCATION (1 << 1)

struct drm_buddy_block { #define DRM_BUDDY_HEADER_OFFSET GENMASK_ULL(63, 12)

On 1/26/22 16:21, Matthew Auld wrote:

If the user doesn't require CPU access for the buffer, then ALLOC_TOPDOWN should be used, in order to prioritise allocating in the non-mappable portion of LMEM.

Signed-off-by: Matthew Auld matthew.auld@intel.com Cc: Thomas Hellström thomas.hellstrom@linux.intel.com

I was wondering how this would work best with user-space not supplying any hints. Thinking that mappable LMEM buffers would be a minority, wouldn't it be better to have TOPDOWN behaviour set by default. It would then be migrated to mappable only if needed. And if the first usage is a cpu-map it would either be mapped in system or immediately migrated from pageless to mappable LMEM?

---

drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 15 +++++++++++---- drivers/gpu/drm/i915/gem/i915_gem_pages.c | 3 +++ drivers/gpu/drm/i915/gem/i915_gem_region.c | 5 +++++ drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 13 ++++++++++--- drivers/gpu/drm/i915/gt/intel_gt.c | 4 +++- drivers/gpu/drm/i915/i915_vma.c | 3 +++ drivers/gpu/drm/i915/intel_region_ttm.c | 11 ++++++++--- drivers/gpu/drm/i915/selftests/mock_region.c | 7 +------ 8 files changed, 44 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h index 71e778ecaeb8..29285aaf0477 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h @@ -319,15 +319,22 @@ struct drm_i915_gem_object { #define I915_BO_ALLOC_PM_VOLATILE BIT(4) /* Object needs to be restored early using memcpy during resume */ #define I915_BO_ALLOC_PM_EARLY BIT(5) +/*

• • Object is likely never accessed by the CPU. This will prioritise the BO to be
• • allocated in the non-mappable portion of lmem. This is merely a hint, and if
• • dealing with userspace objects the CPU fault handler is free to ignore this.
• */

+#define I915_BO_ALLOC_TOPDOWN BIT(6) #define I915_BO_ALLOC_FLAGS (I915_BO_ALLOC_CONTIGUOUS | \ I915_BO_ALLOC_VOLATILE | \ I915_BO_ALLOC_CPU_CLEAR | \ I915_BO_ALLOC_USER | \ I915_BO_ALLOC_PM_VOLATILE | \

• 	     I915_BO_ALLOC_PM_EARLY)
  

-#define I915_BO_READONLY BIT(6) -#define I915_TILING_QUIRK_BIT 7 /* unknown swizzling; do not release! */ -#define I915_BO_PROTECTED BIT(8)

• 	     I915_BO_ALLOC_PM_EARLY | \
  

• 	     I915_BO_ALLOC_TOPDOWN)
  

+#define I915_BO_READONLY BIT(7) +#define I915_TILING_QUIRK_BIT 8 /* unknown swizzling; do not release! */ +#define I915_BO_PROTECTED BIT(9) /** * @mem_flags - Mutable placement-related flags * diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c index 7d2211fbe548..a95b4d72619f 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c @@ -346,6 +346,9 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj, !i915_gem_object_has_iomem(obj)) return ERR_PTR(-ENXIO);

• if (WARN_ON_ONCE(obj->flags & I915_BO_ALLOC_TOPDOWN))

• return ERR_PTR(-EINVAL);
  

• assert_object_held(obj);

  pinned = !(type & I915_MAP_OVERRIDE);

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.c b/drivers/gpu/drm/i915/gem/i915_gem_region.c index a4350227e9ae..f91e5a9c759d 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_region.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_region.c @@ -45,6 +45,11 @@ i915_gem_object_create_region(struct intel_memory_region *mem,

GEM_BUG_ON(flags & ~I915_BO_ALLOC_FLAGS);

• if (WARN_ON_ONCE(flags & I915_BO_ALLOC_TOPDOWN &&

• 	 (flags & I915_BO_ALLOC_CPU_CLEAR ||
  

• 	  flags & I915_BO_ALLOC_PM_EARLY)))
  

• return ERR_PTR(-EINVAL);
  

• if (!mem) return ERR_PTR(-ENODEV);

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index d9a04c7d41b1..e60b677ecd54 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -127,10 +127,14 @@ i915_ttm_place_from_region(const struct intel_memory_region *mr, place->mem_type = intel_region_to_ttm_type(mr);

if (flags & I915_BO_ALLOC_CONTIGUOUS)

• place->flags = TTM_PL_FLAG_CONTIGUOUS;
  

• place->flags |= TTM_PL_FLAG_CONTIGUOUS;
  

  if (mr->io_size && mr->io_size < mr->total) {

• place->fpfn = 0;
  

• place->lpfn = mr->io_size >> PAGE_SHIFT;
  

• if (flags & I915_BO_ALLOC_TOPDOWN) {
  

• 	place->flags |= TTM_PL_FLAG_TOPDOWN;
  

• } else {
  

• 	place->fpfn = 0;
  

• 	place->lpfn = mr->io_size >> PAGE_SHIFT;
  

• }
  

  } }

@@ -890,6 +894,9 @@ static vm_fault_t vm_fault_ttm(struct vm_fault *vmf) if (!obj) return VM_FAULT_SIGBUS;

• if (obj->flags & I915_BO_ALLOC_TOPDOWN)

• return -EINVAL;
  

• /* Sanity check that we allow writing into this object */ if (unlikely(i915_gem_object_is_readonly(obj) && area->vm_flags & VM_WRITE))

diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c index 622cdfed8a8b..8b83a771a2f7 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.c +++ b/drivers/gpu/drm/i915/gt/intel_gt.c @@ -454,7 +454,9 @@ static int intel_gt_init_scratch(struct intel_gt *gt, unsigned int size) struct i915_vma *vma; int ret;

• obj = i915_gem_object_create_lmem(i915, size, I915_BO_ALLOC_VOLATILE);

• obj = i915_gem_object_create_lmem(i915, size,

• 			  I915_BO_ALLOC_VOLATILE |
  

• 			  I915_BO_ALLOC_TOPDOWN);
  

  if (IS_ERR(obj)) obj = i915_gem_object_create_stolen(i915, size); if (IS_ERR(obj))

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index b1816a835abf..b2fdaa74e4b6 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -528,6 +528,9 @@ void __iomem *i915_vma_pin_iomap(struct i915_vma *vma) void __iomem *ptr; int err;

• if (WARN_ON_ONCE(vma->obj->flags & I915_BO_ALLOC_TOPDOWN))

• return IO_ERR_PTR(-EINVAL);
  

• if (!i915_gem_object_is_lmem(vma->obj)) { if (GEM_WARN_ON(!i915_vma_is_map_and_fenceable(vma))) { err = -ENODEV;

diff --git a/drivers/gpu/drm/i915/intel_region_ttm.c b/drivers/gpu/drm/i915/intel_region_ttm.c index 4689192d5e8d..282802aed174 100644 --- a/drivers/gpu/drm/i915/intel_region_ttm.c +++ b/drivers/gpu/drm/i915/intel_region_ttm.c @@ -199,13 +199,18 @@ intel_region_ttm_resource_alloc(struct intel_memory_region *mem, struct ttm_resource *res; int ret;

• if (flags & I915_BO_ALLOC_CONTIGUOUS)

• place.flags |= TTM_PL_FLAG_CONTIGUOUS;
  

  if (mem->io_size && mem->io_size < mem->total) {

• place.fpfn = 0;
  

• place.lpfn = mem->io_size >> PAGE_SHIFT;
  

• if (flags & I915_BO_ALLOC_TOPDOWN) {
  

• 	place.flags |= TTM_PL_FLAG_TOPDOWN;
  

• } else {
  

• 	place.fpfn = 0;
  

• 	place.lpfn = mem->io_size >> PAGE_SHIFT;
  

• }
  

  }

  mock_bo.base.size = size;

• place.flags = flags;

  ret = man->func->alloc(man, &mock_bo, &place, &res); if (ret == -ENOSPC)

diff --git a/drivers/gpu/drm/i915/selftests/mock_region.c b/drivers/gpu/drm/i915/selftests/mock_region.c index 467eeae6d5f0..f64325491f35 100644 --- a/drivers/gpu/drm/i915/selftests/mock_region.c +++ b/drivers/gpu/drm/i915/selftests/mock_region.c @@ -22,17 +22,12 @@ static void mock_region_put_pages(struct drm_i915_gem_object *obj,

static int mock_region_get_pages(struct drm_i915_gem_object *obj) {

• unsigned int flags; struct sg_table *pages; int err;

• flags = 0;

• if (obj->flags & I915_BO_ALLOC_CONTIGUOUS)

• flags |= TTM_PL_FLAG_CONTIGUOUS;
  

• obj->mm.res = intel_region_ttm_resource_alloc(obj->mm.region, obj->base.size,

• 				      flags);
  

• 				      obj->flags);
  

  if (IS_ERR(obj->mm.res)) return PTR_ERR(obj->mm.res);

On 03/02/2022 09:01, Thomas Hellström wrote:

On 1/26/22 16:21, Matthew Auld wrote:

...

Exercise each of the migration scenarios, verifying that the final placement and buffer contents match our expectations.

Signed-off-by: Matthew Auld matthew.auld@intel.com Cc: Thomas Hellström thomas.hellstrom@linux.intel.com

---

.../drm/i915/gem/selftests/i915_gem_mman.c    | 306 ++++++++++++++++++   1 file changed, 306 insertions(+)

diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c index ba29767348be..d2c1071df98a 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c @@ -10,6 +10,7 @@   #include "gt/intel_gpu_commands.h"   #include "gt/intel_gt.h"   #include "gt/intel_gt_pm.h" +#include "gt/intel_migrate.h"   #include "gem/i915_gem_region.h"   #include "huge_gem_object.h"   #include "i915_selftest.h" @@ -999,6 +1000,310 @@ static int igt_mmap(void *arg)       return 0;   } +static void igt_close_objects(struct drm_i915_private *i915, +                  struct list_head *objects) +{ +    struct drm_i915_gem_object *obj, *on;

+    list_for_each_entry_safe(obj, on, objects, st_link) { +        i915_gem_object_lock(obj, NULL); +        if (i915_gem_object_has_pinned_pages(obj)) +            i915_gem_object_unpin_pages(obj); +        /* No polluting the memory region between tests */ +        __i915_gem_object_put_pages(obj); +        i915_gem_object_unlock(obj); +        list_del(&obj->st_link); +        i915_gem_object_put(obj); +    }

+    cond_resched();

+    i915_gem_drain_freed_objects(i915); +}

+static void igt_make_evictable(struct list_head *objects) +{ +    struct drm_i915_gem_object *obj;

+    list_for_each_entry(obj, objects, st_link) { +        i915_gem_object_lock(obj, NULL); +        if (i915_gem_object_has_pinned_pages(obj)) +            i915_gem_object_unpin_pages(obj); +        i915_gem_object_unlock(obj); +    }

+    cond_resched(); +}

+static int igt_fill_mappable(struct intel_memory_region *mr, +                 struct list_head *objects) +{ +    u64 size, total; +    int err;

+    total = 0; +    size = mr->io_size; +    do { +        struct drm_i915_gem_object *obj;

+        obj = i915_gem_object_create_region(mr, size, 0, 0); +        if (IS_ERR(obj)) { +            err = PTR_ERR(obj); +            goto err_close; +        }

+        list_add(&obj->st_link, objects);

+        err = i915_gem_object_pin_pages_unlocked(obj); +        if (err) { +            if (err != -ENXIO && err != -ENOMEM) +                goto err_close;

+            if (size == mr->min_page_size) { +                err = 0; +                break; +            }

+            size >>= 1; +            continue; +        }

+        total += obj->base.size; +    } while (1);

+    pr_info("%s filled=%lluMiB\n", __func__, total >> 20); +    return 0;

+err_close: +    igt_close_objects(mr->i915, objects); +    return err; +}

+static int ___igt_mmap_migrate(struct drm_i915_private *i915, +                   struct drm_i915_gem_object *obj, +                   unsigned long addr, +                   bool unfaultable) +{ +    struct vm_area_struct *area; +    int err = 0, i;

+    pr_info("igt_mmap(%s, %d) @ %lx\n", +         obj->mm.region->name, I915_MMAP_TYPE_FIXED, addr);

+    mmap_read_lock(current->mm); +    area = vma_lookup(current->mm, addr); +    mmap_read_unlock(current->mm); +    if (!area) { +        pr_err("%s: Did not create a vm_area_struct for the mmap\n", +               obj->mm.region->name); +        err = -EINVAL; +        goto out_unmap; +    }

+    for (i = 0; i < obj->base.size / sizeof(u32); i++) { +        u32 __user *ux = u64_to_user_ptr((u64)(addr + i * sizeof(*ux))); +        u32 x;

+        if (get_user(x, ux)) { +            err = -EFAULT; +            if (!unfaultable) { +                pr_err("%s: Unable to read from mmap, offset:%zd\n", +                       obj->mm.region->name, i * sizeof(x)); +                goto out_unmap; +            }

+            continue; +        }

+        if (unfaultable) { +            pr_err("%s: Faulted unmappable memory\n", +                   obj->mm.region->name); +            err = -EINVAL; +            goto out_unmap; +        }

+        if (x != expand32(POISON_INUSE)) { +            pr_err("%s: Read incorrect value from mmap, offset:%zd, found:%x, expected:%x\n", +                   obj->mm.region->name, +                   i * sizeof(x), x, expand32(POISON_INUSE)); +            err = -EINVAL; +            goto out_unmap; +        }

+        x = expand32(POISON_FREE); +        if (put_user(x, ux)) { +            pr_err("%s: Unable to write to mmap, offset:%zd\n", +                   obj->mm.region->name, i * sizeof(x)); +            err = -EFAULT; +            goto out_unmap; +        } +    }

+    if (unfaultable) { +        if (err == -EFAULT) +            err = 0; +    } else { +        obj->flags &= ~I915_BO_ALLOC_TOPDOWN; +        err = wc_check(obj); +    } +out_unmap: +    vm_munmap(addr, obj->base.size); +    return err; +}

+#define IGT_MMAP_MIGRATE_TOPDOWN     (1<<0) +#define IGT_MMAP_MIGRATE_FILL        (1<<1) +#define IGT_MMAP_MIGRATE_EVICTABLE   (1<<2) +#define IGT_MMAP_MIGRATE_UNFAULTABLE (1<<3) +static int __igt_mmap_migrate(struct intel_memory_region **placements, +                  int n_placements, +                  struct intel_memory_region *expected_mr, +                  unsigned int flags) +{ +    struct drm_i915_private *i915 = placements[0]->i915; +    struct drm_i915_gem_object *obj; +    struct i915_gem_ww_ctx ww; +    struct i915_request *rq = NULL; +    unsigned long addr; +    LIST_HEAD(objects); +    u64 offset; +    int err;

+    obj = __i915_gem_object_create_user(i915, PAGE_SIZE, +                        placements, +                        n_placements); +    if (IS_ERR(obj)) +        return PTR_ERR(obj);

+    if (flags & IGT_MMAP_MIGRATE_TOPDOWN) +        obj->flags |= I915_BO_ALLOC_TOPDOWN;

+    err = __assign_mmap_offset(obj, I915_MMAP_TYPE_FIXED, &offset, NULL); +    if (err) +        goto out_put;

+    /* +     * This will eventually create a GEM context, due to opening dummy drm +     * file, which needs a tiny amount of mappable device memory for the top +     * level paging structures(and perhaps scratch), so make sure we +     * allocate early, to avoid tears. +     */ +    addr = igt_mmap_offset(i915, offset, obj->base.size, +                   PROT_WRITE, MAP_SHARED); +    if (IS_ERR_VALUE(addr)) { +        err = addr; +        goto out_put; +    }

+    if (flags & IGT_MMAP_MIGRATE_FILL) { +        err = igt_fill_mappable(placements[0], &objects); +        if (err) +            goto out_put; +    }

+    for_i915_gem_ww(&ww, err, true) {

Do we need a full ww transaction here? Sufficient to only lock the object with NULL?

I think so, will change.

...

+        err = i915_gem_object_lock(obj, &ww); +        if (err) +            continue;

+        err = i915_gem_object_pin_pages(obj); +        if (err) +            continue;

+        err = intel_context_migrate_clear(to_gt(i915)->migrate.context, NULL, +                          obj->mm.pages->sgl, obj->cache_level, +                          i915_gem_object_is_lmem(obj), +                          expand32(POISON_INUSE), &rq); +        i915_gem_object_unpin_pages(obj); +        if (rq) { +            dma_resv_add_excl_fence(obj->base.resv, &rq->fence); +            i915_gem_object_set_moving_fence(obj, &rq->fence); +            i915_request_put(rq); +        } +        if (err) +            continue;

Not needed?

...

+    } +    if (err) +        goto out_put;

+    if (flags & IGT_MMAP_MIGRATE_EVICTABLE) +        igt_make_evictable(&objects);

+    err = ___igt_mmap_migrate(i915, obj, addr, +                  flags & IGT_MMAP_MIGRATE_UNFAULTABLE); +    if (!err && obj->mm.region != expected_mr) { +        pr_err("%s region mismatch %s\n", __func__, expected_mr->name); +        err = -EINVAL; +    }

+out_put: +    i915_gem_object_put(obj); +    igt_close_objects(i915, &objects); +    return err; +}

+static int igt_mmap_migrate(void *arg) +{ +    struct drm_i915_private *i915 = arg; +    struct intel_memory_region *system = i915->mm.regions[INTEL_REGION_SMEM]; +    struct intel_memory_region *mr; +    enum intel_region_id id;

+    for_each_memory_region(mr, i915, id) { +        struct intel_memory_region *mixed[] = { mr, system }; +        struct intel_memory_region *single[] = { mr }; +        int err;

+        if (mr->private) +            continue;

+        if (!mr->io_size || mr->io_size == mr->total) +            continue;

+        /* +         * Allocate in the mappable portion, should be no suprises here. +         */ +        err = __igt_mmap_migrate(mixed, ARRAY_SIZE(mixed), mr, 0); +        if (err) +            return err;

+        /* +         * Allocate in the non-mappable portion, but force migrating to +         * the mappable portion on fault (LMEM -> LMEM) +         */ +        err = __igt_mmap_migrate(single, ARRAY_SIZE(single), mr, +                     IGT_MMAP_MIGRATE_TOPDOWN | +                     IGT_MMAP_MIGRATE_FILL | +                     IGT_MMAP_MIGRATE_EVICTABLE); +        if (err) +            return err;

+        /* +         * Allocate in the non-mappable portion, but force spilling into +         * system memory on fault (LMEM -> SMEM) +         */ +        err = __igt_mmap_migrate(mixed, ARRAY_SIZE(mixed), system, +                     IGT_MMAP_MIGRATE_TOPDOWN | +                     IGT_MMAP_MIGRATE_FILL); +        if (err) +            return err;

+        /* +         * Allocate in the non-mappable portion, but since the mappable +         * portion is already full, and we can't spill to system memory, +         * then we should expect the fault to fail. +         */ +        err = __igt_mmap_migrate(single, ARRAY_SIZE(single), mr, +                     IGT_MMAP_MIGRATE_TOPDOWN | +                     IGT_MMAP_MIGRATE_FILL | +                     IGT_MMAP_MIGRATE_UNFAULTABLE); +        if (err) +            return err; +    }

+    return 0; +}

static const char *repr_mmap_type(enum i915_mmap_type type)   {       switch (type) { @@ -1424,6 +1729,7 @@ int i915_gem_mman_live_selftests(struct drm_i915_private *i915)           SUBTEST(igt_smoke_tiling),           SUBTEST(igt_mmap_offset_exhaustion),           SUBTEST(igt_mmap), +        SUBTEST(igt_mmap_migrate),           SUBTEST(igt_mmap_access),           SUBTEST(igt_mmap_revoke),           SUBTEST(igt_mmap_gpu),

On 1/26/22 16:21, Matthew Auld wrote:

If we have to contend with non-mappable LMEM, then we need to ensure the object fits within the mappable portion, like in the selftests, where we later try to CPU access the pages. However if it can't then we need to gracefully handle this, without throwing an error.

Also it looks like TTM will return -ENOMEM if the object can't be placed.

We should probably have a look at why that happens. I thought ttm would return -ENOSPC, which we then converted to -ENXIO in i915_ttm_err_to_gem().

/Thomas

Signed-off-by: Matthew Auld matthew.auld@intel.com Cc: Thomas Hellström thomas.hellstrom@linux.intel.com

---

drivers/gpu/drm/i915/gem/selftests/huge_pages.c | 2 +- drivers/gpu/drm/i915/selftests/intel_memory_region.c | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c index 42db9cd30978..3caa178bbd07 100644 --- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c +++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c @@ -1344,7 +1344,7 @@ static int igt_ppgtt_smoke_huge(void *arg)

err = i915_gem_object_pin_pages_unlocked(obj);
if (err) {

• 	if (err == -ENXIO || err == -E2BIG) {
  

• 	if (err == -ENXIO || err == -E2BIG || err == -ENOMEM) {
  	i915_gem_object_put(obj);
  	size >>= 1;
  	goto try_again;
  

diff --git a/drivers/gpu/drm/i915/selftests/intel_memory_region.c b/drivers/gpu/drm/i915/selftests/intel_memory_region.c index 04ae29779206..87bff7f83554 100644 --- a/drivers/gpu/drm/i915/selftests/intel_memory_region.c +++ b/drivers/gpu/drm/i915/selftests/intel_memory_region.c @@ -822,8 +822,14 @@ static int igt_lmem_create_with_ps(void *arg)

i915_gem_object_lock(obj, NULL);
err = i915_gem_object_pin_pages(obj);

• if (err)
  

• if (err) {
  

• 	if (err == -ENXIO || err == -E2BIG || err == -ENOMEM) {
  

• 		pr_info("%s not enough lmem for ps(%u) err=%d\n",
  

• 			__func__, ps, err);
  

• 		err = 0;
  

• 	}
  goto out_put;
  

• }
  
  

  daddr = i915_gem_object_get_dma_address(obj, 0); if (!IS_ALIGNED(daddr, ps)) {

On 03/02/2022 09:17, Thomas Hellström wrote:

On 1/26/22 16:21, Matthew Auld wrote:

...

Starting from DG2+, when dealing with LMEM, we assume that by default all userspace allocations should be placed in the non-mappable portion of LMEM.  Note that dumb buffers are not included here, since these are not "GPU accelerated" and likely need CPU access.

In a later patch userspace will be able to provide a hint if CPU access to the buffer is needed.

Signed-off-by: Matthew Auld matthew.auld@intel.com Cc: Thomas Hellström thomas.hellstrom@linux.intel.com

---

drivers/gpu/drm/i915/gem/i915_gem_create.c | 9 +++++++++   1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c index 9402d4bf4ffc..e7456443f163 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c @@ -424,6 +424,15 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data,           ext_data.n_placements = 1;       } +    /* +     * TODO: add a userspace hint to force CPU_ACCESS for the object, which +     * can override this. +     */ +    if (!IS_DG1(i915) && (ext_data.n_placements > 1 || +                  ext_data.placements[0]->type != +                  INTEL_MEMORY_SYSTEM)) +        ext_data.flags |= I915_BO_ALLOC_TOPDOWN;

Perhaps we should include DG1 here as well, so that the same paths are taken regardless whether this is only a test on DG1?

I think the only reason was EXEC_CAPTURE, where atm we just reject anything marked with I915_BO_ALLOC_TOPDOWN, but that must not break existing DG1 uapi.

...

obj = __i915_gem_object_create_user_ext(i915, args->size,                           ext_data.placements,                           ext_data.n_placements,

On 1/26/22 16:21, Matthew Auld wrote:

If set, force the allocation to be placed in the mappable portion of LMEM. One big restriction here is that system memory must be given as a potential placement for the object, that way we can always spill the object into system memory if we can't make space.

XXX: Still very much WIP and needs IGTs. Including now just for the sake of having more complete picture.

Signed-off-by: Matthew Auld matthew.auld@intel.com Cc: Thomas Hellström thomas.hellstrom@linux.intel.com

---

drivers/gpu/drm/i915/gem/i915_gem_create.c | 28 ++++++++++++------- include/uapi/drm/i915_drm.h | 31 +++++++++++++++++++++- 2 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c index e7456443f163..98d63cb21e94 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c @@ -238,6 +238,7 @@ struct create_ext { struct drm_i915_private *i915; struct intel_memory_region *placements[INTEL_REGION_UNKNOWN]; unsigned int n_placements;

• unsigned int placement_mask; unsigned long flags; };

@@ -334,6 +335,7 @@ static int set_placements(struct drm_i915_gem_create_ext_memory_regions *args, for (i = 0; i < args->num_regions; i++) ext_data->placements[i] = placements[i];

• ext_data->placement_mask = mask; return 0;

  out_dump:

@@ -408,7 +410,7 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data, struct drm_i915_gem_object *obj; int ret;

• if (args->flags)

• if (args->flags & ~I915_GEM_CREATE_EXT_FLAG_NEEDS_CPU_ACCESS) return -EINVAL;

  ret = i915_user_extensions(u64_to_user_ptr(args->extensions),

@@ -424,14 +426,22 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data, ext_data.n_placements = 1; }

• /*

• * TODO: add a userspace hint to force CPU_ACCESS for the object, which
  

• * can override this.
  

• */
  

• if (!IS_DG1(i915) && (ext_data.n_placements > 1 ||

• 	      ext_data.placements[0]->type !=
  

• 	      INTEL_MEMORY_SYSTEM))
  

• ext_data.flags |= I915_BO_ALLOC_TOPDOWN;
  

• if (args->flags & I915_GEM_CREATE_EXT_FLAG_NEEDS_CPU_ACCESS) {

• if (ext_data.n_placements == 1)
  

• 	return -EINVAL;
  

• /*
  

•  * We always need to be able to spill to system memory, if we
  

•  * can't place in the mappable part of LMEM.
  

•  */
  

• if (!(ext_data.placement_mask & BIT(INTEL_REGION_SMEM)))
  

• 	return -EINVAL;
  

• } else {

• if (!IS_DG1(i915) && (ext_data.n_placements > 1 ||
  

• 		      ext_data.placements[0]->type !=
  

• 		      INTEL_MEMORY_SYSTEM))
  

• 	ext_data.flags |= I915_BO_ALLOC_TOPDOWN;
  

• }

  obj = __i915_gem_object_create_user_ext(i915, args->size, ext_data.placements,

diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 914ebd9290e5..ecfa805549a7 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -3157,7 +3157,36 @@ struct drm_i915_gem_create_ext { * Object handles are nonzero. */ __u32 handle;

• /** @flags: MBZ */

• /**

• * @flags: Optional flags.
  

• *
  

• * Supported values:
  

• *
  

• * I915_GEM_CREATE_EXT_FLAG_NEEDS_CPU_ACCESS - Signal to the kernel that
  

• * the object will need to be accessed via the CPU.
  

• *
  

• * Only valid when placing objects in I915_MEMORY_CLASS_DEVICE, and
  

• * only strictly required on platforms where only some of the device
  

• * memory is directly visible or mappable through the CPU, like on DG2+.
  

• *
  

• * One of the placements MUST also be I915_MEMORY_CLASS_SYSTEM, to
  

• * ensure we can always spill the allocation to system memory, if we
  

• * can't place the object in the mappable part of
  

• * I915_MEMORY_CLASS_DEVICE.
  

• *
  

• * Note that buffers that need to be captured with EXEC_OBJECT_CAPTURE,
  

• * will need to enable this hint, if the object can also be placed in
  

• * I915_MEMORY_CLASS_DEVICE, starting from DG2+. The execbuf call will
  

• * throw an error otherwise. This also means that such objects will need
  

• * I915_MEMORY_CLASS_SYSTEM set as a possible placement.
  

• *
  

I wonder, should we try to migrate capture objects at execbuf time instead on an on-demand basis? If migration fails, then we just skip capturing that object, similar to how the capture code handles errors?

• * Without this hint, the kernel will assume that non-mappable
  

• * I915_MEMORY_CLASS_DEVICE is preferred for this object. Note that the
  

• * kernel can still migrate the object to the mappable part, as a last
  

• * resort, if userspace ever CPU faults this object, but this might be
  

• * expensive, and so ideally should be avoided.
  

• */
  

+#define I915_GEM_CREATE_EXT_FLAG_NEEDS_CPU_ACCESS (1<<0) __u32 flags; /** * @extensions: The chain of extensions to apply to this object.

On Thu, 2022-02-03 at 11:38 +0000, Matthew Auld wrote:

On 03/02/2022 09:28, Thomas Hellström wrote:

...

On 1/26/22 16:21, Matthew Auld wrote:

...

If set, force the allocation to be placed in the mappable portion of LMEM. One big restriction here is that system memory must be given as a potential placement for the object, that way we can always spill the object into system memory if we can't make space.

XXX: Still very much WIP and needs IGTs. Including now just for the sake of having more complete picture.

Signed-off-by: Matthew Auld matthew.auld@intel.com Cc: Thomas Hellström thomas.hellstrom@linux.intel.com

---

drivers/gpu/drm/i915/gem/i915_gem_create.c | 28 ++++++++++++---

include/uapi/drm/i915_drm.h                | 31 +++++++++++++++++++++-   2 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c index e7456443f163..98d63cb21e94 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c @@ -238,6 +238,7 @@ struct create_ext {       struct drm_i915_private *i915;       struct intel_memory_region *placements[INTEL_REGION_UNKNOWN];       unsigned int n_placements; +    unsigned int placement_mask;       unsigned long flags;   }; @@ -334,6 +335,7 @@ static int set_placements(struct drm_i915_gem_create_ext_memory_regions *args,       for (i = 0; i < args->num_regions; i++)           ext_data->placements[i] = placements[i]; +    ext_data->placement_mask = mask;       return 0;   out_dump: @@ -408,7 +410,7 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data,       struct drm_i915_gem_object *obj;       int ret; -    if (args->flags) +    if (args->flags & ~I915_GEM_CREATE_EXT_FLAG_NEEDS_CPU_ACCESS)           return -EINVAL;       ret = i915_user_extensions(u64_to_user_ptr(args-

...

extensions),

@@ -424,14 +426,22 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data,           ext_data.n_placements = 1;       } -    /* -     * TODO: add a userspace hint to force CPU_ACCESS for the object, which -     * can override this. -     */ -    if (!IS_DG1(i915) && (ext_data.n_placements > 1 || -                  ext_data.placements[0]->type != -                  INTEL_MEMORY_SYSTEM)) -        ext_data.flags |= I915_BO_ALLOC_TOPDOWN; +    if (args->flags & I915_GEM_CREATE_EXT_FLAG_NEEDS_CPU_ACCESS) { +        if (ext_data.n_placements == 1) +            return -EINVAL;

+        /* +         * We always need to be able to spill to system memory, if we +         * can't place in the mappable part of LMEM. +         */ +        if (!(ext_data.placement_mask & BIT(INTEL_REGION_SMEM))) +            return -EINVAL; +    } else { +        if (!IS_DG1(i915) && (ext_data.n_placements > 1 || +                      ext_data.placements[0]->type != +                      INTEL_MEMORY_SYSTEM)) +            ext_data.flags |= I915_BO_ALLOC_TOPDOWN; +    }       obj = __i915_gem_object_create_user_ext(i915, args->size,                           ext_data.placements, diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 914ebd9290e5..ecfa805549a7 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -3157,7 +3157,36 @@ struct drm_i915_gem_create_ext {        * Object handles are nonzero.        */       __u32 handle; -    /** @flags: MBZ */ +    /** +     * @flags: Optional flags. +     * +     * Supported values: +     * +     * I915_GEM_CREATE_EXT_FLAG_NEEDS_CPU_ACCESS - Signal to the kernel that +     * the object will need to be accessed via the CPU. +     * +     * Only valid when placing objects in I915_MEMORY_CLASS_DEVICE, and +     * only strictly required on platforms where only some of the device +     * memory is directly visible or mappable through the CPU, like on DG2+. +     * +     * One of the placements MUST also be I915_MEMORY_CLASS_SYSTEM, to +     * ensure we can always spill the allocation to system memory, if we +     * can't place the object in the mappable part of +     * I915_MEMORY_CLASS_DEVICE. +     * +     * Note that buffers that need to be captured with EXEC_OBJECT_CAPTURE, +     * will need to enable this hint, if the object can also be placed in +     * I915_MEMORY_CLASS_DEVICE, starting from DG2+. The execbuf call will +     * throw an error otherwise. This also means that such objects will need +     * I915_MEMORY_CLASS_SYSTEM set as a possible placement. +     *

I wonder, should we try to migrate capture objects at execbuf time instead on an on-demand basis? If migration fails, then we just skip capturing that object, similar to how the capture code handles errors?

So IIUC if the object has been marked for capture, unmark the TOPDOWN annotation, if it has been set, to force allocating in the mappable portion, or spill to system memory(if the placements allow it)? I think that should work.

Yes that would temporarily mean drop the TOPDOWN flag and migrate the object if needed (we can do that async from inside execbuf AFAICT). We'd need to make the TOPDOWN flag mutable and part of object-

mem_flags

Jon any thoughts

And on that subject, the TOPDOWN flag name is IMHO a bit misleading. It really has a "GPU_ONLY" meaning, which translates to TOPDOWN allocations in some memory regions only. For others (no small bar, small bar + multi-tile, it might translate to nothing or to tile selection)?

/Thomas

On 1/26/22 16:21, Matthew Auld wrote:

On platforms where there might be non-mappable LMEM, force userspace to mark the buffers with the correct hint. When dumping the BO contents during capture we need CPU access. Note this only applies to buffers that can be placed in LMEM, and also doesn't impact DG1.

Oddly enough this seems to break DG1. We probably need to understand why.

/Thomas

Signed-off-by: Matthew Auld matthew.auld@intel.com Cc: Thomas Hellström thomas.hellstrom@linux.intel.com

---

drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 498b458fd784..3c8083852620 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -1965,7 +1965,7 @@ eb_find_first_request_added(struct i915_execbuffer *eb) #if IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR)

/* Stage with GFP_KERNEL allocations before we enter the signaling critical path */ -static void eb_capture_stage(struct i915_execbuffer *eb) +static int eb_capture_stage(struct i915_execbuffer *eb) { const unsigned int count = eb->buffer_count; unsigned int i = count, j; @@ -1978,6 +1978,9 @@ static void eb_capture_stage(struct i915_execbuffer *eb) if (!(flags & EXEC_OBJECT_CAPTURE)) continue;

• if (vma->obj->flags & I915_BO_ALLOC_TOPDOWN)
  

• 	return -EINVAL;
  

• for_each_batch_create_order(eb, j) { struct i915_capture_list *capture;

@@ -1990,6 +1993,8 @@ static void eb_capture_stage(struct i915_execbuffer *eb) eb->capture_lists[j] = capture; } }

• return 0; }

  /* Commit once we're in the critical path */

@@ -3418,7 +3423,9 @@ i915_gem_do_execbuffer(struct drm_device *dev, }

ww_acquire_done(&eb.ww.ctx);

• eb_capture_stage(&eb);

• err = eb_capture_stage(&eb);

• if (err)

• goto err_vma;
  
  

  out_fence = eb_requests_create(&eb, in_fence, out_fence_fd); if (IS_ERR(out_fence)) {

On 03/02/2022 09:48, Thomas Hellström wrote:

On 1/26/22 16:21, Matthew Auld wrote:

...

Just pass along the probed io_size. The backend should be able to utilize the entire range here, even if some of it is non-mappable.

Changes here LGTM.

...

It does leave open with what to do with stolen local-memory.

Are objects in stolen local required to be mappable?

From a quick look I don't really see such users on discrete, outside of maybe intelfb_create(), where I guess the initial fb might be located in stolen on DG1. But from DG2+ it looks like it will just be located in normal LMEM. For that I was thinking we add something like i915_gem_object_create_region_at(), and somehow wire that up to the {fpfn, lpfn}...

/Thomas

...

Signed-off-by: Matthew Auld matthew.auld@intel.com Cc: Thomas Hellström thomas.hellstrom@linux.intel.com

---

drivers/gpu/drm/i915/gt/intel_region_lmem.c | 6 ++++--   1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_region_lmem.c b/drivers/gpu/drm/i915/gt/intel_region_lmem.c index 2c7ec7ff79fd..b788fc2b3df8 100644 --- a/drivers/gpu/drm/i915/gt/intel_region_lmem.c +++ b/drivers/gpu/drm/i915/gt/intel_region_lmem.c @@ -200,6 +200,7 @@ static struct intel_memory_region *setup_lmem(struct intel_gt *gt)       struct intel_memory_region *mem;       resource_size_t min_page_size;       resource_size_t io_start; +    resource_size_t io_size;       resource_size_t lmem_size;       int err; @@ -210,7 +211,8 @@ static struct intel_memory_region *setup_lmem(struct intel_gt *gt)       lmem_size = intel_uncore_read64(uncore, GEN12_GSMBASE);       io_start = pci_resource_start(pdev, 2); -    if (GEM_WARN_ON(lmem_size > pci_resource_len(pdev, 2))) +    io_size = min(pci_resource_len(pdev, 2), lmem_size); +    if (!io_size)           return ERR_PTR(-ENODEV);       min_page_size = HAS_64K_PAGES(i915) ? I915_GTT_PAGE_SIZE_64K : @@ -220,7 +222,7 @@ static struct intel_memory_region *setup_lmem(struct intel_gt *gt)                        lmem_size,                        min_page_size,                        io_start, -                     lmem_size, +                     io_size,                        INTEL_MEMORY_LOCAL,                        0,                        &intel_region_lmem_ops);