Re: [Intel-gfx] [PATCH v2 15/15] drm/i915/ttm: Add io sgt caching to i915_ttm_io_mem_pfn

On 5/18/21 10:27 AM, Thomas Hellström wrote:

From: Maarten Lankhorst maarten.lankhorst@linux.intel.com

Instead of walking the sg table manually, use our caching helpers to do the sgt caching. To prevent lifetime issues of ttm_bo vs i915_gem_object, we will use a separate member, instead of re-using the dma page member.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com

drivers/gpu/drm/i915/gem/i915_gem_object.h | 6 +-- .../gpu/drm/i915/gem/i915_gem_object_types.h | 1 + drivers/gpu/drm/i915/gem/i915_gem_pages.c | 3 +- drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 46 ++++++++++--------- 4 files changed, 30 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index a3ad8cf4eefd..ff59e6c640e6 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -342,14 +342,14 @@ struct scatterlist * __i915_gem_object_get_sg(struct drm_i915_gem_object *obj, struct i915_gem_object_page_iter *iter, unsigned int n,

• 	 unsigned int *offset, bool allow_alloc);
  

• 	 unsigned int *offset, bool allow_alloc, bool dma);
  
  

  static inline struct scatterlist * i915_gem_object_get_sg(struct drm_i915_gem_object *obj, unsigned int n, unsigned int *offset, bool allow_alloc) {

• return __i915_gem_object_get_sg(obj, &obj->mm.get_page, n, offset, allow_alloc);

• return __i915_gem_object_get_sg(obj, &obj->mm.get_page, n, offset, allow_alloc, false); }

  static inline struct scatterlist *

@@ -357,7 +357,7 @@ i915_gem_object_get_sg_dma(struct drm_i915_gem_object *obj, unsigned int n, unsigned int *offset, bool allow_alloc) {

• return __i915_gem_object_get_sg(obj, &obj->mm.get_dma_page, n, offset, allow_alloc);

• return __i915_gem_object_get_sg(obj, &obj->mm.get_dma_page, n, offset, allow_alloc, true); }

  struct page *

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 31d828e91cf4..828310802b9f 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h @@ -324,6 +324,7 @@ struct drm_i915_gem_object {

struct { struct sg_table *cached_io_st;

• struct i915_gem_object_page_iter get_io_page;
  

  } ttm;

  /** Record of address bit 17 of each page at last unbind. */

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c index 62ee2185a41b..577352b4f2f6 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c @@ -465,9 +465,8 @@ __i915_gem_object_get_sg(struct drm_i915_gem_object *obj, struct i915_gem_object_page_iter *iter, unsigned int n, unsigned int *offset,

• 	 bool allow_alloc)
  

• 	 bool allow_alloc, bool dma)
  

  {

• const bool dma = iter == &obj->mm.get_dma_page; struct scatterlist *sg; unsigned int idx, count;

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index fe9ac50b2470..1eaefb89e859 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -167,11 +167,20 @@ static int i915_ttm_move_notify(struct ttm_buffer_object *bo)

static void i915_ttm_free_cached_io_st(struct drm_i915_gem_object *obj) {

• if (obj->ttm.cached_io_st) {

• sg_free_table(obj->ttm.cached_io_st);
  

• kfree(obj->ttm.cached_io_st);
  

• obj->ttm.cached_io_st = NULL;
  

• }

• struct radix_tree_iter iter;

• void __rcu **slot;

• if (!obj->ttm.cached_io_st)

• return;
  

• rcu_read_lock();

• radix_tree_for_each_slot(slot, &obj->ttm.get_io_page.radix, &iter, 0)

• radix_tree_delete(&obj->ttm.get_io_page.radix, iter.index);
  

• rcu_read_unlock();

• sg_free_table(obj->ttm.cached_io_st);

• kfree(obj->ttm.cached_io_st);

• obj->ttm.cached_io_st = NULL; }

  static void i915_ttm_purge(struct drm_i915_gem_object *obj)

@@ -340,8 +349,11 @@ static int i915_ttm_move(struct ttm_buffer_object *bo, bool evict, i915_ttm_move_memcpy(bo, new_mem, new_iter, old_iter); i915_ttm_free_cached_io_st(obj);

• if (!new_man->use_tt)

• if (!new_man->use_tt) { obj->ttm.cached_io_st = new_st;

• obj->ttm.get_io_page.sg_pos = new_st->sgl;
  

• obj->ttm.get_io_page.sg_idx = 0;
  

• }

  return 0; }

@@ -362,26 +374,15 @@ static unsigned long i915_ttm_io_mem_pfn(struct ttm_buffer_object *bo, unsigned long page_offset) { struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);

• struct sg_table *sgt = obj->ttm.cached_io_st;

• unsigned long base = obj->mm.region->iomap.base - obj->mm.region->region.start; struct scatterlist *sg;

• unsigned int i;

• unsigned int ofs;

  GEM_WARN_ON(bo->ttm);

• for_each_sgtable_dma_sg(sgt, sg, i) {

• unsigned long sg_max = sg->length >> PAGE_SHIFT;
  

• sg = __i915_gem_object_get_sg(obj, &obj->ttm.get_io_page, page_offset, &ofs, true, true);

• if (page_offset < sg_max) {
  

• 	unsigned long base =
  

• 		obj->mm.region->iomap.base - obj->mm.region->region.start;
  

• 	return ((base + sg_dma_address(sg)) >> PAGE_SHIFT) + page_offset;
  

• }
  

• page_offset -= sg_max;
  

• }
• GEM_BUG_ON(1);
• return 0;

• return ((base + sg_dma_address(sg)) >> PAGE_SHIFT) + ofs; }

  struct ttm_device_funcs i915_ttm_bo_driver = {

@@ -613,6 +614,9 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem, obj->read_domains = I915_GEM_DOMAIN_WC | I915_GEM_DOMAIN_GTT; i915_gem_object_set_cache_coherency(obj, I915_CACHE_NONE);

• INIT_RADIX_TREE(&obj->ttm.get_io_page.radix, GFP_KERNEL | __GFP_NOWARN);
• mutex_init(&obj->ttm.get_io_page.lock);

Perhaps a mutex_destroy() in error path and destructor for full debugging purposes?

Otherwise LGTM,

Reviewed-by: Thomas Hellström thomas.hellstrom@linux.intel.com

ret = ttm_bo_init(&i915->bdev, i915_gem_to_ttm(obj), size, ttm_bo_type_kernel, &i915_sys_placement, alignment, true, NULL, NULL, i915_ttm_bo_destroy);