* [Intel-gfx] [PATCH 0/5] drm/i915: Move system memory to TTM for discrete
@ 2021-06-02 17:07 ` Thomas Hellström
0 siblings, 0 replies; 27+ messages in thread
From: Thomas Hellström @ 2021-06-02 17:07 UTC (permalink / raw)
To: intel-gfx, dri-devel; +Cc: Thomas Hellström
Early implementation of moving system memory for discrete cards over to
TTM. We first add the notion of objects being migratable under the object
lock to i915 gem, and add some asserts to verify that objects are either
locked or pinned when the placement is checked by the gem code.
Patch 2 and 3 deals with updating the i915 gem bookkeeping after a TTM move,
Patch 4 moves system over from shmem to TTM for discrete and finally the
last patch is more to be considered an RFC for migration implementation.
Much of this code is not testing covered, so we might have to add some
selftests as well. Meanwhile this should not be merged but may well be
reviewed.
Note that the mock device doesn't consider itself discrete so the TTM
system path is not checked by the mock selftests.
Also testing if CI can handle base-commit: and prerequisite-patch-id:
Thomas Hellström (5):
drm/i915: Update object placement flags to be mutable
drm/i915/ttm: Adjust gem flags and caching settings after a move
drm/i915/ttm: Calculate the object placement at get_pages time
drm/i915/ttm: Use TTM for system memory
drm/i915/ttm: Implement object migration
drivers/gpu/drm/i915/gem/i915_gem_internal.c | 4 +-
drivers/gpu/drm/i915/gem/i915_gem_mman.c | 7 +-
drivers/gpu/drm/i915/gem/i915_gem_object.c | 138 +++++++++
drivers/gpu/drm/i915/gem/i915_gem_object.h | 24 +-
.../gpu/drm/i915/gem/i915_gem_object_types.h | 27 +-
drivers/gpu/drm/i915/gem/i915_gem_pages.c | 2 +-
drivers/gpu/drm/i915/gem/i915_gem_phys.c | 2 +-
drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 10 +-
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 267 +++++++++++++++---
drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 4 +-
.../drm/i915/gem/selftests/huge_gem_object.c | 4 +-
.../gpu/drm/i915/gem/selftests/huge_pages.c | 5 +-
.../drm/i915/gem/selftests/i915_gem_mman.c | 4 +-
.../drm/i915/gem/selftests/i915_gem_phys.c | 3 +-
drivers/gpu/drm/i915/i915_drv.h | 3 -
drivers/gpu/drm/i915/intel_memory_region.c | 7 +-
drivers/gpu/drm/i915/intel_memory_region.h | 8 +
drivers/gpu/drm/i915/intel_region_ttm.c | 8 +-
drivers/gpu/drm/i915/intel_region_ttm.h | 2 +
19 files changed, 437 insertions(+), 92 deletions(-)
base-commit: cd6eb5f605478f2fff85ec7ac39b7cf445d3deb9
prerequisite-patch-id: b3e7766bc492d04a5ace71fd0d4259689f3bd749
prerequisite-patch-id: 7978430e513e204addd7bc9bef63fd3a9ea25d7c
prerequisite-patch-id: f6aa8d141b22b8e532f8152e53f9c913f98458df
prerequisite-patch-id: 6486c9db619d88a3e9a4ffc01a89bdc722941a67
prerequisite-patch-id: b71878b47e3be71748aacb5d1b3955853bf80de5
prerequisite-patch-id: 1b67c82c32c02dfdb1217f224652276e044fb549
prerequisite-patch-id: 5229db39ec27eb77a8417ed7c9d54af40e0e9f33
prerequisite-patch-id: b66b1893401cc526b4466ea3c427512261d33dfd
prerequisite-patch-id: 706521b6f4858cf4d9ecd83102b347e695b96bb7
prerequisite-patch-id: 686dc7cfea6361c4adabc963996dc3d0d41b28b1
prerequisite-patch-id: 7be87140130a9c2d62a190ceafdacdebd51c5196
--
2.31.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 1/5] drm/i915: Update object placement flags to be mutable
2021-06-02 17:07 ` [Intel-gfx] " Thomas Hellström
@ 2021-06-02 17:07 ` Thomas Hellström
-1 siblings, 0 replies; 27+ messages in thread
From: Thomas Hellström @ 2021-06-02 17:07 UTC (permalink / raw)
To: intel-gfx, dri-devel; +Cc: Thomas Hellström
The object ops i915_GEM_OBJECT_HAS_IOMEM and the object
I915_BO_ALLOC_STRUCT_PAGE flags are considered immutable by
much of our code. Introduce a new mem_flags member to hold these
and make sure checks for these flags being set are either done
under the object lock or with pages properly pinned. The flags
will change during migration under the object lock.
Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
drivers/gpu/drm/i915/gem/i915_gem_internal.c | 4 +-
drivers/gpu/drm/i915/gem/i915_gem_mman.c | 7 +++-
drivers/gpu/drm/i915/gem/i915_gem_object.c | 38 +++++++++++++++++++
drivers/gpu/drm/i915/gem/i915_gem_object.h | 14 ++-----
.../gpu/drm/i915/gem/i915_gem_object_types.h | 20 +++++-----
drivers/gpu/drm/i915/gem/i915_gem_pages.c | 2 +-
drivers/gpu/drm/i915/gem/i915_gem_phys.c | 2 +-
drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 10 +++--
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 2 +-
drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 4 +-
.../drm/i915/gem/selftests/huge_gem_object.c | 4 +-
.../gpu/drm/i915/gem/selftests/huge_pages.c | 5 +--
.../drm/i915/gem/selftests/i915_gem_mman.c | 4 +-
.../drm/i915/gem/selftests/i915_gem_phys.c | 3 +-
14 files changed, 78 insertions(+), 41 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
index ce6b664b10aa..13b217f75055 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
@@ -177,8 +177,8 @@ i915_gem_object_create_internal(struct drm_i915_private *i915,
return ERR_PTR(-ENOMEM);
drm_gem_private_object_init(&i915->drm, &obj->base, size);
- i915_gem_object_init(obj, &i915_gem_object_internal_ops, &lock_class,
- I915_BO_ALLOC_STRUCT_PAGE);
+ i915_gem_object_init(obj, &i915_gem_object_internal_ops, &lock_class, 0);
+ obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
/*
* Mark the object as volatile, such that the pages are marked as
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index d1de97e4adfd..171a21ca930c 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -683,7 +683,7 @@ __assign_mmap_offset(struct drm_i915_gem_object *obj,
if (mmap_type != I915_MMAP_TYPE_GTT &&
!i915_gem_object_has_struct_page(obj) &&
- !i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM))
+ !i915_gem_object_has_iomem(obj))
return -ENODEV;
mmo = mmap_offset_attach(obj, mmap_type, file);
@@ -707,7 +707,12 @@ __assign_mmap_offset_handle(struct drm_file *file,
if (!obj)
return -ENOENT;
+ err = i915_gem_object_lock_interruptible(obj, NULL);
+ if (err)
+ goto out_put;
err = __assign_mmap_offset(obj, mmap_type, offset, file);
+ i915_gem_object_unlock(obj);
+out_put:
i915_gem_object_put(obj);
return err;
}
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index cf18c430d51f..07e8ff9a8aae 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -475,6 +475,44 @@ bool i915_gem_object_migratable(struct drm_i915_gem_object *obj)
return obj->mm.n_placements > 1;
}
+/**
+ * i915_gem_object_has_struct_page - Whether the object is page-backed
+ * @obj: The object to query.
+ *
+ * This function should only be called while the object is locked or pinned,
+ * otherwise the page backing may change under the caller.
+ *
+ * Return: True if page-backed, false otherwise.
+ */
+bool i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj)
+{
+#ifdef CONFIG_LOCKDEP
+ if (IS_DGFX(to_i915(obj->base.dev)) &&
+ i915_gem_object_evictable((void __force *)obj))
+ assert_object_held_shared(obj);
+#endif
+ return obj->mem_flags & I915_BO_FLAG_STRUCT_PAGE;
+}
+
+/**
+ * i915_gem_object_has_iomem - Whether the object is iomem-backed
+ * @obj: The object to query.
+ *
+ * This function should only be called while the object is locked or pinned,
+ * otherwise the iomem backing may change under the caller.
+ *
+ * Return: True if iomem-backed, false otherwise.
+ */
+bool i915_gem_object_has_iomem(const struct drm_i915_gem_object *obj)
+{
+#ifdef CONFIG_LOCKDEP
+ if (IS_DGFX(to_i915(obj->base.dev)) &&
+ i915_gem_object_evictable((void __force *)obj))
+ assert_object_held_shared(obj);
+#endif
+ return obj->mem_flags & I915_BO_FLAG_IOMEM;
+}
+
void i915_gem_init__objects(struct drm_i915_private *i915)
{
INIT_WORK(&i915->mm.free_work, __i915_gem_free_work);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index ff59e6c640e6..23e26f6b1db9 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -147,7 +147,7 @@ i915_gem_object_put(struct drm_i915_gem_object *obj)
/*
* If more than one potential simultaneous locker, assert held.
*/
-static inline void assert_object_held_shared(struct drm_i915_gem_object *obj)
+static inline void assert_object_held_shared(const struct drm_i915_gem_object *obj)
{
/*
* Note mm list lookup is protected by
@@ -261,17 +261,9 @@ i915_gem_object_type_has(const struct drm_i915_gem_object *obj,
return obj->ops->flags & flags;
}
-static inline bool
-i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj)
-{
- return obj->flags & I915_BO_ALLOC_STRUCT_PAGE;
-}
+bool i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj);
-static inline bool
-i915_gem_object_has_iomem(const struct drm_i915_gem_object *obj)
-{
- return i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM);
-}
+bool i915_gem_object_has_iomem(const struct drm_i915_gem_object *obj);
static inline bool
i915_gem_object_is_shrinkable(const struct drm_i915_gem_object *obj)
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 2a23b77424b3..fb9ccc3f50e7 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
@@ -33,10 +33,9 @@ struct i915_lut_handle {
struct drm_i915_gem_object_ops {
unsigned int flags;
-#define I915_GEM_OBJECT_HAS_IOMEM BIT(1)
-#define I915_GEM_OBJECT_IS_SHRINKABLE BIT(2)
-#define I915_GEM_OBJECT_IS_PROXY BIT(3)
-#define I915_GEM_OBJECT_NO_MMAP BIT(4)
+#define I915_GEM_OBJECT_IS_SHRINKABLE BIT(1)
+#define I915_GEM_OBJECT_IS_PROXY BIT(2)
+#define I915_GEM_OBJECT_NO_MMAP BIT(3)
/* Interface between the GEM object and its backing storage.
* get_pages() is called once prior to the use of the associated set
@@ -201,17 +200,18 @@ struct drm_i915_gem_object {
unsigned long flags;
#define I915_BO_ALLOC_CONTIGUOUS BIT(0)
#define I915_BO_ALLOC_VOLATILE BIT(1)
-#define I915_BO_ALLOC_STRUCT_PAGE BIT(2)
-#define I915_BO_ALLOC_CPU_CLEAR BIT(3)
-#define I915_BO_ALLOC_USER BIT(4)
+#define I915_BO_ALLOC_CPU_CLEAR BIT(2)
+#define I915_BO_ALLOC_USER BIT(3)
#define I915_BO_ALLOC_FLAGS (I915_BO_ALLOC_CONTIGUOUS | \
I915_BO_ALLOC_VOLATILE | \
- I915_BO_ALLOC_STRUCT_PAGE | \
I915_BO_ALLOC_CPU_CLEAR | \
I915_BO_ALLOC_USER)
-#define I915_BO_READONLY BIT(5)
-#define I915_TILING_QUIRK_BIT 6 /* unknown swizzling; do not release! */
+#define I915_BO_READONLY BIT(4)
+#define I915_TILING_QUIRK_BIT 5 /* unknown swizzling; do not release! */
+ unsigned int mem_flags:2;
+#define I915_BO_FLAG_STRUCT_PAGE BIT(0)
+#define I915_BO_FLAG_IOMEM BIT(1)
/*
* Is the object to be mapped as read-only to the GPU
* Only honoured if hardware has relevant pte bit
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
index 086005c1c7ea..f2f850e31b8e 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
@@ -351,7 +351,7 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj,
int err;
if (!i915_gem_object_has_struct_page(obj) &&
- !i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM))
+ !i915_gem_object_has_iomem(obj))
return ERR_PTR(-ENXIO);
assert_object_held(obj);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
index be72ad0634ba..7986612f48fa 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
@@ -76,7 +76,7 @@ static int i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj)
intel_gt_chipset_flush(&to_i915(obj->base.dev)->gt);
/* We're no longer struct page backed */
- obj->flags &= ~I915_BO_ALLOC_STRUCT_PAGE;
+ obj->mem_flags &= ~I915_BO_FLAG_STRUCT_PAGE;
__i915_gem_object_set_pages(obj, st, sg->length);
return 0;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index 5d16c4462fda..3648ae1d6628 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -284,6 +284,7 @@ __i915_gem_object_release_shmem(struct drm_i915_gem_object *obj,
bool needs_clflush)
{
GEM_BUG_ON(obj->mm.madv == __I915_MADV_PURGED);
+ GEM_WARN_ON(IS_DGFX(to_i915(obj->base.dev)));
if (obj->mm.madv == I915_MADV_DONTNEED)
obj->mm.dirty = false;
@@ -302,6 +303,7 @@ void i915_gem_object_put_pages_shmem(struct drm_i915_gem_object *obj, struct sg_
struct pagevec pvec;
struct page *page;
+ GEM_WARN_ON(IS_DGFX(to_i915(obj->base.dev)));
__i915_gem_object_release_shmem(obj, pages, true);
i915_gem_gtt_finish_pages(obj, pages);
@@ -444,7 +446,7 @@ shmem_pread(struct drm_i915_gem_object *obj,
static void shmem_release(struct drm_i915_gem_object *obj)
{
- if (obj->flags & I915_BO_ALLOC_STRUCT_PAGE)
+ if (i915_gem_object_has_struct_page(obj))
i915_gem_object_release_memory_region(obj);
fput(obj->base.filp);
@@ -513,9 +515,8 @@ static int shmem_object_init(struct intel_memory_region *mem,
mapping_set_gfp_mask(mapping, mask);
GEM_BUG_ON(!(mapping_gfp_mask(mapping) & __GFP_RECLAIM));
- i915_gem_object_init(obj, &i915_gem_shmem_ops, &lock_class,
- I915_BO_ALLOC_STRUCT_PAGE);
-
+ i915_gem_object_init(obj, &i915_gem_shmem_ops, &lock_class, 0);
+ obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
obj->write_domain = I915_GEM_DOMAIN_CPU;
obj->read_domains = I915_GEM_DOMAIN_CPU;
@@ -561,6 +562,7 @@ i915_gem_object_create_shmem_from_data(struct drm_i915_private *dev_priv,
resource_size_t offset;
int err;
+ GEM_WARN_ON(IS_DGFX(dev_priv));
obj = i915_gem_object_create_shmem(dev_priv, round_up(size, PAGE_SIZE));
if (IS_ERR(obj))
return obj;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 3748098b42d5..ae12a2be11a2 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -563,7 +563,6 @@ static u64 i915_ttm_mmap_offset(struct drm_i915_gem_object *obj)
const struct drm_i915_gem_object_ops i915_gem_ttm_obj_ops = {
.name = "i915_gem_object_ttm",
- .flags = I915_GEM_OBJECT_HAS_IOMEM,
.get_pages = i915_ttm_get_pages,
.put_pages = i915_ttm_put_pages,
@@ -620,6 +619,7 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
i915_gem_object_init_memory_region(obj, mem);
i915_gem_object_make_unshrinkable(obj);
obj->read_domains = I915_GEM_DOMAIN_WC | I915_GEM_DOMAIN_GTT;
+ obj->mem_flags |= I915_BO_FLAG_IOMEM;
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);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
index 602f0ed983ec..41dfcb75f05b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
@@ -538,8 +538,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
return -ENOMEM;
drm_gem_private_object_init(dev, &obj->base, args->user_size);
- i915_gem_object_init(obj, &i915_gem_userptr_ops, &lock_class,
- I915_BO_ALLOC_STRUCT_PAGE);
+ i915_gem_object_init(obj, &i915_gem_userptr_ops, &lock_class, 0);
+ obj->mem_flags = I915_BO_FLAG_STRUCT_PAGE;
obj->read_domains = I915_GEM_DOMAIN_CPU;
obj->write_domain = I915_GEM_DOMAIN_CPU;
i915_gem_object_set_cache_coherency(obj, I915_CACHE_LLC);
diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
index 0c8ecfdf5405..f963b8e1e37b 100644
--- a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
@@ -114,8 +114,8 @@ huge_gem_object(struct drm_i915_private *i915,
return ERR_PTR(-ENOMEM);
drm_gem_private_object_init(&i915->drm, &obj->base, dma_size);
- i915_gem_object_init(obj, &huge_ops, &lock_class,
- I915_BO_ALLOC_STRUCT_PAGE);
+ i915_gem_object_init(obj, &huge_ops, &lock_class, 0);
+ obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
obj->read_domains = I915_GEM_DOMAIN_CPU;
obj->write_domain = I915_GEM_DOMAIN_CPU;
diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
index dadd485bc52f..ccc67ed1a84b 100644
--- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
+++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
@@ -167,9 +167,8 @@ huge_pages_object(struct drm_i915_private *i915,
return ERR_PTR(-ENOMEM);
drm_gem_private_object_init(&i915->drm, &obj->base, size);
- i915_gem_object_init(obj, &huge_page_ops, &lock_class,
- I915_BO_ALLOC_STRUCT_PAGE);
-
+ i915_gem_object_init(obj, &huge_page_ops, &lock_class, 0);
+ obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
i915_gem_object_set_volatile(obj);
obj->write_domain = I915_GEM_DOMAIN_CPU;
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 ca69a29b7f2a..bfb35270a1f0 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
@@ -837,7 +837,7 @@ static bool can_mmap(struct drm_i915_gem_object *obj, enum i915_mmap_type type)
if (type != I915_MMAP_TYPE_GTT &&
!i915_gem_object_has_struct_page(obj) &&
- !i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM))
+ !i915_gem_object_has_iomem(obj))
return false;
return true;
@@ -991,7 +991,7 @@ static const char *repr_mmap_type(enum i915_mmap_type type)
static bool can_access(const struct drm_i915_gem_object *obj)
{
return i915_gem_object_has_struct_page(obj) ||
- i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM);
+ i915_gem_object_has_iomem(obj);
}
static int __igt_mmap_access(struct drm_i915_private *i915,
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c
index 3a6ce87f8b52..d43d8dae0f69 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c
@@ -25,13 +25,14 @@ static int mock_phys_object(void *arg)
goto out;
}
+ i915_gem_object_lock(obj, NULL);
if (!i915_gem_object_has_struct_page(obj)) {
+ i915_gem_object_unlock(obj);
err = -EINVAL;
pr_err("shmem has no struct page\n");
goto out_obj;
}
- i915_gem_object_lock(obj, NULL);
err = i915_gem_object_attach_phys(obj, PAGE_SIZE);
i915_gem_object_unlock(obj);
if (err) {
--
2.31.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [Intel-gfx] [PATCH 1/5] drm/i915: Update object placement flags to be mutable
@ 2021-06-02 17:07 ` Thomas Hellström
0 siblings, 0 replies; 27+ messages in thread
From: Thomas Hellström @ 2021-06-02 17:07 UTC (permalink / raw)
To: intel-gfx, dri-devel; +Cc: Thomas Hellström
The object ops i915_GEM_OBJECT_HAS_IOMEM and the object
I915_BO_ALLOC_STRUCT_PAGE flags are considered immutable by
much of our code. Introduce a new mem_flags member to hold these
and make sure checks for these flags being set are either done
under the object lock or with pages properly pinned. The flags
will change during migration under the object lock.
Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
drivers/gpu/drm/i915/gem/i915_gem_internal.c | 4 +-
drivers/gpu/drm/i915/gem/i915_gem_mman.c | 7 +++-
drivers/gpu/drm/i915/gem/i915_gem_object.c | 38 +++++++++++++++++++
drivers/gpu/drm/i915/gem/i915_gem_object.h | 14 ++-----
.../gpu/drm/i915/gem/i915_gem_object_types.h | 20 +++++-----
drivers/gpu/drm/i915/gem/i915_gem_pages.c | 2 +-
drivers/gpu/drm/i915/gem/i915_gem_phys.c | 2 +-
drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 10 +++--
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 2 +-
drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 4 +-
.../drm/i915/gem/selftests/huge_gem_object.c | 4 +-
.../gpu/drm/i915/gem/selftests/huge_pages.c | 5 +--
.../drm/i915/gem/selftests/i915_gem_mman.c | 4 +-
.../drm/i915/gem/selftests/i915_gem_phys.c | 3 +-
14 files changed, 78 insertions(+), 41 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
index ce6b664b10aa..13b217f75055 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
@@ -177,8 +177,8 @@ i915_gem_object_create_internal(struct drm_i915_private *i915,
return ERR_PTR(-ENOMEM);
drm_gem_private_object_init(&i915->drm, &obj->base, size);
- i915_gem_object_init(obj, &i915_gem_object_internal_ops, &lock_class,
- I915_BO_ALLOC_STRUCT_PAGE);
+ i915_gem_object_init(obj, &i915_gem_object_internal_ops, &lock_class, 0);
+ obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
/*
* Mark the object as volatile, such that the pages are marked as
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index d1de97e4adfd..171a21ca930c 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -683,7 +683,7 @@ __assign_mmap_offset(struct drm_i915_gem_object *obj,
if (mmap_type != I915_MMAP_TYPE_GTT &&
!i915_gem_object_has_struct_page(obj) &&
- !i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM))
+ !i915_gem_object_has_iomem(obj))
return -ENODEV;
mmo = mmap_offset_attach(obj, mmap_type, file);
@@ -707,7 +707,12 @@ __assign_mmap_offset_handle(struct drm_file *file,
if (!obj)
return -ENOENT;
+ err = i915_gem_object_lock_interruptible(obj, NULL);
+ if (err)
+ goto out_put;
err = __assign_mmap_offset(obj, mmap_type, offset, file);
+ i915_gem_object_unlock(obj);
+out_put:
i915_gem_object_put(obj);
return err;
}
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index cf18c430d51f..07e8ff9a8aae 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -475,6 +475,44 @@ bool i915_gem_object_migratable(struct drm_i915_gem_object *obj)
return obj->mm.n_placements > 1;
}
+/**
+ * i915_gem_object_has_struct_page - Whether the object is page-backed
+ * @obj: The object to query.
+ *
+ * This function should only be called while the object is locked or pinned,
+ * otherwise the page backing may change under the caller.
+ *
+ * Return: True if page-backed, false otherwise.
+ */
+bool i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj)
+{
+#ifdef CONFIG_LOCKDEP
+ if (IS_DGFX(to_i915(obj->base.dev)) &&
+ i915_gem_object_evictable((void __force *)obj))
+ assert_object_held_shared(obj);
+#endif
+ return obj->mem_flags & I915_BO_FLAG_STRUCT_PAGE;
+}
+
+/**
+ * i915_gem_object_has_iomem - Whether the object is iomem-backed
+ * @obj: The object to query.
+ *
+ * This function should only be called while the object is locked or pinned,
+ * otherwise the iomem backing may change under the caller.
+ *
+ * Return: True if iomem-backed, false otherwise.
+ */
+bool i915_gem_object_has_iomem(const struct drm_i915_gem_object *obj)
+{
+#ifdef CONFIG_LOCKDEP
+ if (IS_DGFX(to_i915(obj->base.dev)) &&
+ i915_gem_object_evictable((void __force *)obj))
+ assert_object_held_shared(obj);
+#endif
+ return obj->mem_flags & I915_BO_FLAG_IOMEM;
+}
+
void i915_gem_init__objects(struct drm_i915_private *i915)
{
INIT_WORK(&i915->mm.free_work, __i915_gem_free_work);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index ff59e6c640e6..23e26f6b1db9 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -147,7 +147,7 @@ i915_gem_object_put(struct drm_i915_gem_object *obj)
/*
* If more than one potential simultaneous locker, assert held.
*/
-static inline void assert_object_held_shared(struct drm_i915_gem_object *obj)
+static inline void assert_object_held_shared(const struct drm_i915_gem_object *obj)
{
/*
* Note mm list lookup is protected by
@@ -261,17 +261,9 @@ i915_gem_object_type_has(const struct drm_i915_gem_object *obj,
return obj->ops->flags & flags;
}
-static inline bool
-i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj)
-{
- return obj->flags & I915_BO_ALLOC_STRUCT_PAGE;
-}
+bool i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj);
-static inline bool
-i915_gem_object_has_iomem(const struct drm_i915_gem_object *obj)
-{
- return i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM);
-}
+bool i915_gem_object_has_iomem(const struct drm_i915_gem_object *obj);
static inline bool
i915_gem_object_is_shrinkable(const struct drm_i915_gem_object *obj)
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 2a23b77424b3..fb9ccc3f50e7 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
@@ -33,10 +33,9 @@ struct i915_lut_handle {
struct drm_i915_gem_object_ops {
unsigned int flags;
-#define I915_GEM_OBJECT_HAS_IOMEM BIT(1)
-#define I915_GEM_OBJECT_IS_SHRINKABLE BIT(2)
-#define I915_GEM_OBJECT_IS_PROXY BIT(3)
-#define I915_GEM_OBJECT_NO_MMAP BIT(4)
+#define I915_GEM_OBJECT_IS_SHRINKABLE BIT(1)
+#define I915_GEM_OBJECT_IS_PROXY BIT(2)
+#define I915_GEM_OBJECT_NO_MMAP BIT(3)
/* Interface between the GEM object and its backing storage.
* get_pages() is called once prior to the use of the associated set
@@ -201,17 +200,18 @@ struct drm_i915_gem_object {
unsigned long flags;
#define I915_BO_ALLOC_CONTIGUOUS BIT(0)
#define I915_BO_ALLOC_VOLATILE BIT(1)
-#define I915_BO_ALLOC_STRUCT_PAGE BIT(2)
-#define I915_BO_ALLOC_CPU_CLEAR BIT(3)
-#define I915_BO_ALLOC_USER BIT(4)
+#define I915_BO_ALLOC_CPU_CLEAR BIT(2)
+#define I915_BO_ALLOC_USER BIT(3)
#define I915_BO_ALLOC_FLAGS (I915_BO_ALLOC_CONTIGUOUS | \
I915_BO_ALLOC_VOLATILE | \
- I915_BO_ALLOC_STRUCT_PAGE | \
I915_BO_ALLOC_CPU_CLEAR | \
I915_BO_ALLOC_USER)
-#define I915_BO_READONLY BIT(5)
-#define I915_TILING_QUIRK_BIT 6 /* unknown swizzling; do not release! */
+#define I915_BO_READONLY BIT(4)
+#define I915_TILING_QUIRK_BIT 5 /* unknown swizzling; do not release! */
+ unsigned int mem_flags:2;
+#define I915_BO_FLAG_STRUCT_PAGE BIT(0)
+#define I915_BO_FLAG_IOMEM BIT(1)
/*
* Is the object to be mapped as read-only to the GPU
* Only honoured if hardware has relevant pte bit
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
index 086005c1c7ea..f2f850e31b8e 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
@@ -351,7 +351,7 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj,
int err;
if (!i915_gem_object_has_struct_page(obj) &&
- !i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM))
+ !i915_gem_object_has_iomem(obj))
return ERR_PTR(-ENXIO);
assert_object_held(obj);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
index be72ad0634ba..7986612f48fa 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
@@ -76,7 +76,7 @@ static int i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj)
intel_gt_chipset_flush(&to_i915(obj->base.dev)->gt);
/* We're no longer struct page backed */
- obj->flags &= ~I915_BO_ALLOC_STRUCT_PAGE;
+ obj->mem_flags &= ~I915_BO_FLAG_STRUCT_PAGE;
__i915_gem_object_set_pages(obj, st, sg->length);
return 0;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index 5d16c4462fda..3648ae1d6628 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -284,6 +284,7 @@ __i915_gem_object_release_shmem(struct drm_i915_gem_object *obj,
bool needs_clflush)
{
GEM_BUG_ON(obj->mm.madv == __I915_MADV_PURGED);
+ GEM_WARN_ON(IS_DGFX(to_i915(obj->base.dev)));
if (obj->mm.madv == I915_MADV_DONTNEED)
obj->mm.dirty = false;
@@ -302,6 +303,7 @@ void i915_gem_object_put_pages_shmem(struct drm_i915_gem_object *obj, struct sg_
struct pagevec pvec;
struct page *page;
+ GEM_WARN_ON(IS_DGFX(to_i915(obj->base.dev)));
__i915_gem_object_release_shmem(obj, pages, true);
i915_gem_gtt_finish_pages(obj, pages);
@@ -444,7 +446,7 @@ shmem_pread(struct drm_i915_gem_object *obj,
static void shmem_release(struct drm_i915_gem_object *obj)
{
- if (obj->flags & I915_BO_ALLOC_STRUCT_PAGE)
+ if (i915_gem_object_has_struct_page(obj))
i915_gem_object_release_memory_region(obj);
fput(obj->base.filp);
@@ -513,9 +515,8 @@ static int shmem_object_init(struct intel_memory_region *mem,
mapping_set_gfp_mask(mapping, mask);
GEM_BUG_ON(!(mapping_gfp_mask(mapping) & __GFP_RECLAIM));
- i915_gem_object_init(obj, &i915_gem_shmem_ops, &lock_class,
- I915_BO_ALLOC_STRUCT_PAGE);
-
+ i915_gem_object_init(obj, &i915_gem_shmem_ops, &lock_class, 0);
+ obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
obj->write_domain = I915_GEM_DOMAIN_CPU;
obj->read_domains = I915_GEM_DOMAIN_CPU;
@@ -561,6 +562,7 @@ i915_gem_object_create_shmem_from_data(struct drm_i915_private *dev_priv,
resource_size_t offset;
int err;
+ GEM_WARN_ON(IS_DGFX(dev_priv));
obj = i915_gem_object_create_shmem(dev_priv, round_up(size, PAGE_SIZE));
if (IS_ERR(obj))
return obj;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 3748098b42d5..ae12a2be11a2 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -563,7 +563,6 @@ static u64 i915_ttm_mmap_offset(struct drm_i915_gem_object *obj)
const struct drm_i915_gem_object_ops i915_gem_ttm_obj_ops = {
.name = "i915_gem_object_ttm",
- .flags = I915_GEM_OBJECT_HAS_IOMEM,
.get_pages = i915_ttm_get_pages,
.put_pages = i915_ttm_put_pages,
@@ -620,6 +619,7 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
i915_gem_object_init_memory_region(obj, mem);
i915_gem_object_make_unshrinkable(obj);
obj->read_domains = I915_GEM_DOMAIN_WC | I915_GEM_DOMAIN_GTT;
+ obj->mem_flags |= I915_BO_FLAG_IOMEM;
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);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
index 602f0ed983ec..41dfcb75f05b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
@@ -538,8 +538,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
return -ENOMEM;
drm_gem_private_object_init(dev, &obj->base, args->user_size);
- i915_gem_object_init(obj, &i915_gem_userptr_ops, &lock_class,
- I915_BO_ALLOC_STRUCT_PAGE);
+ i915_gem_object_init(obj, &i915_gem_userptr_ops, &lock_class, 0);
+ obj->mem_flags = I915_BO_FLAG_STRUCT_PAGE;
obj->read_domains = I915_GEM_DOMAIN_CPU;
obj->write_domain = I915_GEM_DOMAIN_CPU;
i915_gem_object_set_cache_coherency(obj, I915_CACHE_LLC);
diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
index 0c8ecfdf5405..f963b8e1e37b 100644
--- a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
@@ -114,8 +114,8 @@ huge_gem_object(struct drm_i915_private *i915,
return ERR_PTR(-ENOMEM);
drm_gem_private_object_init(&i915->drm, &obj->base, dma_size);
- i915_gem_object_init(obj, &huge_ops, &lock_class,
- I915_BO_ALLOC_STRUCT_PAGE);
+ i915_gem_object_init(obj, &huge_ops, &lock_class, 0);
+ obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
obj->read_domains = I915_GEM_DOMAIN_CPU;
obj->write_domain = I915_GEM_DOMAIN_CPU;
diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
index dadd485bc52f..ccc67ed1a84b 100644
--- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
+++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
@@ -167,9 +167,8 @@ huge_pages_object(struct drm_i915_private *i915,
return ERR_PTR(-ENOMEM);
drm_gem_private_object_init(&i915->drm, &obj->base, size);
- i915_gem_object_init(obj, &huge_page_ops, &lock_class,
- I915_BO_ALLOC_STRUCT_PAGE);
-
+ i915_gem_object_init(obj, &huge_page_ops, &lock_class, 0);
+ obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
i915_gem_object_set_volatile(obj);
obj->write_domain = I915_GEM_DOMAIN_CPU;
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 ca69a29b7f2a..bfb35270a1f0 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
@@ -837,7 +837,7 @@ static bool can_mmap(struct drm_i915_gem_object *obj, enum i915_mmap_type type)
if (type != I915_MMAP_TYPE_GTT &&
!i915_gem_object_has_struct_page(obj) &&
- !i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM))
+ !i915_gem_object_has_iomem(obj))
return false;
return true;
@@ -991,7 +991,7 @@ static const char *repr_mmap_type(enum i915_mmap_type type)
static bool can_access(const struct drm_i915_gem_object *obj)
{
return i915_gem_object_has_struct_page(obj) ||
- i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM);
+ i915_gem_object_has_iomem(obj);
}
static int __igt_mmap_access(struct drm_i915_private *i915,
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c
index 3a6ce87f8b52..d43d8dae0f69 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c
@@ -25,13 +25,14 @@ static int mock_phys_object(void *arg)
goto out;
}
+ i915_gem_object_lock(obj, NULL);
if (!i915_gem_object_has_struct_page(obj)) {
+ i915_gem_object_unlock(obj);
err = -EINVAL;
pr_err("shmem has no struct page\n");
goto out_obj;
}
- i915_gem_object_lock(obj, NULL);
err = i915_gem_object_attach_phys(obj, PAGE_SIZE);
i915_gem_object_unlock(obj);
if (err) {
--
2.31.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [Intel-gfx] [PATCH 1/5] drm/i915: Update object placement flags to be mutable
2021-06-02 17:07 ` [Intel-gfx] " Thomas Hellström
@ 2021-06-03 11:17 ` Matthew Auld
-1 siblings, 0 replies; 27+ messages in thread
From: Matthew Auld @ 2021-06-03 11:17 UTC (permalink / raw)
To: Thomas Hellström; +Cc: Intel Graphics Development, ML dri-devel
On Wed, 2 Jun 2021 at 18:08, Thomas Hellström
<thomas.hellstrom@linux.intel.com> wrote:
>
> The object ops i915_GEM_OBJECT_HAS_IOMEM and the object
> I915_BO_ALLOC_STRUCT_PAGE flags are considered immutable by
> much of our code. Introduce a new mem_flags member to hold these
> and make sure checks for these flags being set are either done
> under the object lock or with pages properly pinned. The flags
> will change during migration under the object lock.
What are the rules for the gem_object_ops flags? Like is_shrinkable?
Can't we just move these there(IOMEM vs PAGE)?
>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_internal.c | 4 +-
> drivers/gpu/drm/i915/gem/i915_gem_mman.c | 7 +++-
> drivers/gpu/drm/i915/gem/i915_gem_object.c | 38 +++++++++++++++++++
> drivers/gpu/drm/i915/gem/i915_gem_object.h | 14 ++-----
> .../gpu/drm/i915/gem/i915_gem_object_types.h | 20 +++++-----
> drivers/gpu/drm/i915/gem/i915_gem_pages.c | 2 +-
> drivers/gpu/drm/i915/gem/i915_gem_phys.c | 2 +-
> drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 10 +++--
> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 2 +-
> drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 4 +-
> .../drm/i915/gem/selftests/huge_gem_object.c | 4 +-
> .../gpu/drm/i915/gem/selftests/huge_pages.c | 5 +--
> .../drm/i915/gem/selftests/i915_gem_mman.c | 4 +-
> .../drm/i915/gem/selftests/i915_gem_phys.c | 3 +-
> 14 files changed, 78 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> index ce6b664b10aa..13b217f75055 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> @@ -177,8 +177,8 @@ i915_gem_object_create_internal(struct drm_i915_private *i915,
> return ERR_PTR(-ENOMEM);
>
> drm_gem_private_object_init(&i915->drm, &obj->base, size);
> - i915_gem_object_init(obj, &i915_gem_object_internal_ops, &lock_class,
> - I915_BO_ALLOC_STRUCT_PAGE);
> + i915_gem_object_init(obj, &i915_gem_object_internal_ops, &lock_class, 0);
> + obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
>
> /*
> * Mark the object as volatile, such that the pages are marked as
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> index d1de97e4adfd..171a21ca930c 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> @@ -683,7 +683,7 @@ __assign_mmap_offset(struct drm_i915_gem_object *obj,
>
> if (mmap_type != I915_MMAP_TYPE_GTT &&
> !i915_gem_object_has_struct_page(obj) &&
> - !i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM))
> + !i915_gem_object_has_iomem(obj))
> return -ENODEV;
>
> mmo = mmap_offset_attach(obj, mmap_type, file);
> @@ -707,7 +707,12 @@ __assign_mmap_offset_handle(struct drm_file *file,
> if (!obj)
> return -ENOENT;
>
> + err = i915_gem_object_lock_interruptible(obj, NULL);
> + if (err)
> + goto out_put;
> err = __assign_mmap_offset(obj, mmap_type, offset, file);
> + i915_gem_object_unlock(obj);
> +out_put:
> i915_gem_object_put(obj);
> return err;
> }
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> index cf18c430d51f..07e8ff9a8aae 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -475,6 +475,44 @@ bool i915_gem_object_migratable(struct drm_i915_gem_object *obj)
> return obj->mm.n_placements > 1;
> }
>
> +/**
> + * i915_gem_object_has_struct_page - Whether the object is page-backed
> + * @obj: The object to query.
> + *
> + * This function should only be called while the object is locked or pinned,
> + * otherwise the page backing may change under the caller.
> + *
> + * Return: True if page-backed, false otherwise.
> + */
> +bool i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj)
> +{
> +#ifdef CONFIG_LOCKDEP
> + if (IS_DGFX(to_i915(obj->base.dev)) &&
> + i915_gem_object_evictable((void __force *)obj))
> + assert_object_held_shared(obj);
> +#endif
> + return obj->mem_flags & I915_BO_FLAG_STRUCT_PAGE;
> +}
> +
> +/**
> + * i915_gem_object_has_iomem - Whether the object is iomem-backed
> + * @obj: The object to query.
> + *
> + * This function should only be called while the object is locked or pinned,
> + * otherwise the iomem backing may change under the caller.
> + *
> + * Return: True if iomem-backed, false otherwise.
> + */
> +bool i915_gem_object_has_iomem(const struct drm_i915_gem_object *obj)
> +{
> +#ifdef CONFIG_LOCKDEP
> + if (IS_DGFX(to_i915(obj->base.dev)) &&
> + i915_gem_object_evictable((void __force *)obj))
> + assert_object_held_shared(obj);
> +#endif
> + return obj->mem_flags & I915_BO_FLAG_IOMEM;
> +}
> +
> void i915_gem_init__objects(struct drm_i915_private *i915)
> {
> INIT_WORK(&i915->mm.free_work, __i915_gem_free_work);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> index ff59e6c640e6..23e26f6b1db9 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> @@ -147,7 +147,7 @@ i915_gem_object_put(struct drm_i915_gem_object *obj)
> /*
> * If more than one potential simultaneous locker, assert held.
> */
> -static inline void assert_object_held_shared(struct drm_i915_gem_object *obj)
> +static inline void assert_object_held_shared(const struct drm_i915_gem_object *obj)
> {
> /*
> * Note mm list lookup is protected by
> @@ -261,17 +261,9 @@ i915_gem_object_type_has(const struct drm_i915_gem_object *obj,
> return obj->ops->flags & flags;
> }
>
> -static inline bool
> -i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj)
> -{
> - return obj->flags & I915_BO_ALLOC_STRUCT_PAGE;
> -}
> +bool i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj);
>
> -static inline bool
> -i915_gem_object_has_iomem(const struct drm_i915_gem_object *obj)
> -{
> - return i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM);
> -}
> +bool i915_gem_object_has_iomem(const struct drm_i915_gem_object *obj);
>
> static inline bool
> i915_gem_object_is_shrinkable(const struct drm_i915_gem_object *obj)
> 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 2a23b77424b3..fb9ccc3f50e7 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> @@ -33,10 +33,9 @@ struct i915_lut_handle {
>
> struct drm_i915_gem_object_ops {
> unsigned int flags;
> -#define I915_GEM_OBJECT_HAS_IOMEM BIT(1)
> -#define I915_GEM_OBJECT_IS_SHRINKABLE BIT(2)
> -#define I915_GEM_OBJECT_IS_PROXY BIT(3)
> -#define I915_GEM_OBJECT_NO_MMAP BIT(4)
> +#define I915_GEM_OBJECT_IS_SHRINKABLE BIT(1)
> +#define I915_GEM_OBJECT_IS_PROXY BIT(2)
> +#define I915_GEM_OBJECT_NO_MMAP BIT(3)
>
> /* Interface between the GEM object and its backing storage.
> * get_pages() is called once prior to the use of the associated set
> @@ -201,17 +200,18 @@ struct drm_i915_gem_object {
> unsigned long flags;
> #define I915_BO_ALLOC_CONTIGUOUS BIT(0)
> #define I915_BO_ALLOC_VOLATILE BIT(1)
> -#define I915_BO_ALLOC_STRUCT_PAGE BIT(2)
> -#define I915_BO_ALLOC_CPU_CLEAR BIT(3)
> -#define I915_BO_ALLOC_USER BIT(4)
> +#define I915_BO_ALLOC_CPU_CLEAR BIT(2)
> +#define I915_BO_ALLOC_USER BIT(3)
> #define I915_BO_ALLOC_FLAGS (I915_BO_ALLOC_CONTIGUOUS | \
> I915_BO_ALLOC_VOLATILE | \
> - I915_BO_ALLOC_STRUCT_PAGE | \
> I915_BO_ALLOC_CPU_CLEAR | \
> I915_BO_ALLOC_USER)
> -#define I915_BO_READONLY BIT(5)
> -#define I915_TILING_QUIRK_BIT 6 /* unknown swizzling; do not release! */
> +#define I915_BO_READONLY BIT(4)
> +#define I915_TILING_QUIRK_BIT 5 /* unknown swizzling; do not release! */
>
> + unsigned int mem_flags:2;
> +#define I915_BO_FLAG_STRUCT_PAGE BIT(0)
> +#define I915_BO_FLAG_IOMEM BIT(1)
> /*
> * Is the object to be mapped as read-only to the GPU
> * Only honoured if hardware has relevant pte bit
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> index 086005c1c7ea..f2f850e31b8e 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> @@ -351,7 +351,7 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj,
> int err;
>
> if (!i915_gem_object_has_struct_page(obj) &&
> - !i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM))
> + !i915_gem_object_has_iomem(obj))
> return ERR_PTR(-ENXIO);
>
> assert_object_held(obj);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> index be72ad0634ba..7986612f48fa 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> @@ -76,7 +76,7 @@ static int i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj)
> intel_gt_chipset_flush(&to_i915(obj->base.dev)->gt);
>
> /* We're no longer struct page backed */
> - obj->flags &= ~I915_BO_ALLOC_STRUCT_PAGE;
> + obj->mem_flags &= ~I915_BO_FLAG_STRUCT_PAGE;
> __i915_gem_object_set_pages(obj, st, sg->length);
>
> return 0;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> index 5d16c4462fda..3648ae1d6628 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> @@ -284,6 +284,7 @@ __i915_gem_object_release_shmem(struct drm_i915_gem_object *obj,
> bool needs_clflush)
> {
> GEM_BUG_ON(obj->mm.madv == __I915_MADV_PURGED);
> + GEM_WARN_ON(IS_DGFX(to_i915(obj->base.dev)));
>
> if (obj->mm.madv == I915_MADV_DONTNEED)
> obj->mm.dirty = false;
> @@ -302,6 +303,7 @@ void i915_gem_object_put_pages_shmem(struct drm_i915_gem_object *obj, struct sg_
> struct pagevec pvec;
> struct page *page;
>
> + GEM_WARN_ON(IS_DGFX(to_i915(obj->base.dev)));
> __i915_gem_object_release_shmem(obj, pages, true);
>
> i915_gem_gtt_finish_pages(obj, pages);
> @@ -444,7 +446,7 @@ shmem_pread(struct drm_i915_gem_object *obj,
>
> static void shmem_release(struct drm_i915_gem_object *obj)
> {
> - if (obj->flags & I915_BO_ALLOC_STRUCT_PAGE)
> + if (i915_gem_object_has_struct_page(obj))
> i915_gem_object_release_memory_region(obj);
>
> fput(obj->base.filp);
> @@ -513,9 +515,8 @@ static int shmem_object_init(struct intel_memory_region *mem,
> mapping_set_gfp_mask(mapping, mask);
> GEM_BUG_ON(!(mapping_gfp_mask(mapping) & __GFP_RECLAIM));
>
> - i915_gem_object_init(obj, &i915_gem_shmem_ops, &lock_class,
> - I915_BO_ALLOC_STRUCT_PAGE);
> -
> + i915_gem_object_init(obj, &i915_gem_shmem_ops, &lock_class, 0);
> + obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
> obj->write_domain = I915_GEM_DOMAIN_CPU;
> obj->read_domains = I915_GEM_DOMAIN_CPU;
>
> @@ -561,6 +562,7 @@ i915_gem_object_create_shmem_from_data(struct drm_i915_private *dev_priv,
> resource_size_t offset;
> int err;
>
> + GEM_WARN_ON(IS_DGFX(dev_priv));
> obj = i915_gem_object_create_shmem(dev_priv, round_up(size, PAGE_SIZE));
> if (IS_ERR(obj))
> return obj;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index 3748098b42d5..ae12a2be11a2 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -563,7 +563,6 @@ static u64 i915_ttm_mmap_offset(struct drm_i915_gem_object *obj)
>
> const struct drm_i915_gem_object_ops i915_gem_ttm_obj_ops = {
> .name = "i915_gem_object_ttm",
> - .flags = I915_GEM_OBJECT_HAS_IOMEM,
>
> .get_pages = i915_ttm_get_pages,
> .put_pages = i915_ttm_put_pages,
> @@ -620,6 +619,7 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
> i915_gem_object_init_memory_region(obj, mem);
> i915_gem_object_make_unshrinkable(obj);
> obj->read_domains = I915_GEM_DOMAIN_WC | I915_GEM_DOMAIN_GTT;
> + obj->mem_flags |= I915_BO_FLAG_IOMEM;
> 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);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index 602f0ed983ec..41dfcb75f05b 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -538,8 +538,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
> return -ENOMEM;
>
> drm_gem_private_object_init(dev, &obj->base, args->user_size);
> - i915_gem_object_init(obj, &i915_gem_userptr_ops, &lock_class,
> - I915_BO_ALLOC_STRUCT_PAGE);
> + i915_gem_object_init(obj, &i915_gem_userptr_ops, &lock_class, 0);
> + obj->mem_flags = I915_BO_FLAG_STRUCT_PAGE;
> obj->read_domains = I915_GEM_DOMAIN_CPU;
> obj->write_domain = I915_GEM_DOMAIN_CPU;
> i915_gem_object_set_cache_coherency(obj, I915_CACHE_LLC);
> diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
> index 0c8ecfdf5405..f963b8e1e37b 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
> @@ -114,8 +114,8 @@ huge_gem_object(struct drm_i915_private *i915,
> return ERR_PTR(-ENOMEM);
>
> drm_gem_private_object_init(&i915->drm, &obj->base, dma_size);
> - i915_gem_object_init(obj, &huge_ops, &lock_class,
> - I915_BO_ALLOC_STRUCT_PAGE);
> + i915_gem_object_init(obj, &huge_ops, &lock_class, 0);
> + obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
>
> obj->read_domains = I915_GEM_DOMAIN_CPU;
> obj->write_domain = I915_GEM_DOMAIN_CPU;
> diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
> index dadd485bc52f..ccc67ed1a84b 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
> @@ -167,9 +167,8 @@ huge_pages_object(struct drm_i915_private *i915,
> return ERR_PTR(-ENOMEM);
>
> drm_gem_private_object_init(&i915->drm, &obj->base, size);
> - i915_gem_object_init(obj, &huge_page_ops, &lock_class,
> - I915_BO_ALLOC_STRUCT_PAGE);
> -
> + i915_gem_object_init(obj, &huge_page_ops, &lock_class, 0);
> + obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
> i915_gem_object_set_volatile(obj);
>
> obj->write_domain = I915_GEM_DOMAIN_CPU;
> 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 ca69a29b7f2a..bfb35270a1f0 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> @@ -837,7 +837,7 @@ static bool can_mmap(struct drm_i915_gem_object *obj, enum i915_mmap_type type)
>
> if (type != I915_MMAP_TYPE_GTT &&
> !i915_gem_object_has_struct_page(obj) &&
> - !i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM))
> + !i915_gem_object_has_iomem(obj))
> return false;
>
> return true;
> @@ -991,7 +991,7 @@ static const char *repr_mmap_type(enum i915_mmap_type type)
> static bool can_access(const struct drm_i915_gem_object *obj)
> {
> return i915_gem_object_has_struct_page(obj) ||
> - i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM);
> + i915_gem_object_has_iomem(obj);
> }
>
> static int __igt_mmap_access(struct drm_i915_private *i915,
> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c
> index 3a6ce87f8b52..d43d8dae0f69 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c
> @@ -25,13 +25,14 @@ static int mock_phys_object(void *arg)
> goto out;
> }
>
> + i915_gem_object_lock(obj, NULL);
> if (!i915_gem_object_has_struct_page(obj)) {
> + i915_gem_object_unlock(obj);
> err = -EINVAL;
> pr_err("shmem has no struct page\n");
> goto out_obj;
> }
>
> - i915_gem_object_lock(obj, NULL);
> err = i915_gem_object_attach_phys(obj, PAGE_SIZE);
> i915_gem_object_unlock(obj);
> if (err) {
> --
> 2.31.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Intel-gfx] [PATCH 1/5] drm/i915: Update object placement flags to be mutable
@ 2021-06-03 11:17 ` Matthew Auld
0 siblings, 0 replies; 27+ messages in thread
From: Matthew Auld @ 2021-06-03 11:17 UTC (permalink / raw)
To: Thomas Hellström; +Cc: Intel Graphics Development, ML dri-devel
On Wed, 2 Jun 2021 at 18:08, Thomas Hellström
<thomas.hellstrom@linux.intel.com> wrote:
>
> The object ops i915_GEM_OBJECT_HAS_IOMEM and the object
> I915_BO_ALLOC_STRUCT_PAGE flags are considered immutable by
> much of our code. Introduce a new mem_flags member to hold these
> and make sure checks for these flags being set are either done
> under the object lock or with pages properly pinned. The flags
> will change during migration under the object lock.
What are the rules for the gem_object_ops flags? Like is_shrinkable?
Can't we just move these there(IOMEM vs PAGE)?
>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_internal.c | 4 +-
> drivers/gpu/drm/i915/gem/i915_gem_mman.c | 7 +++-
> drivers/gpu/drm/i915/gem/i915_gem_object.c | 38 +++++++++++++++++++
> drivers/gpu/drm/i915/gem/i915_gem_object.h | 14 ++-----
> .../gpu/drm/i915/gem/i915_gem_object_types.h | 20 +++++-----
> drivers/gpu/drm/i915/gem/i915_gem_pages.c | 2 +-
> drivers/gpu/drm/i915/gem/i915_gem_phys.c | 2 +-
> drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 10 +++--
> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 2 +-
> drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 4 +-
> .../drm/i915/gem/selftests/huge_gem_object.c | 4 +-
> .../gpu/drm/i915/gem/selftests/huge_pages.c | 5 +--
> .../drm/i915/gem/selftests/i915_gem_mman.c | 4 +-
> .../drm/i915/gem/selftests/i915_gem_phys.c | 3 +-
> 14 files changed, 78 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> index ce6b664b10aa..13b217f75055 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> @@ -177,8 +177,8 @@ i915_gem_object_create_internal(struct drm_i915_private *i915,
> return ERR_PTR(-ENOMEM);
>
> drm_gem_private_object_init(&i915->drm, &obj->base, size);
> - i915_gem_object_init(obj, &i915_gem_object_internal_ops, &lock_class,
> - I915_BO_ALLOC_STRUCT_PAGE);
> + i915_gem_object_init(obj, &i915_gem_object_internal_ops, &lock_class, 0);
> + obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
>
> /*
> * Mark the object as volatile, such that the pages are marked as
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> index d1de97e4adfd..171a21ca930c 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> @@ -683,7 +683,7 @@ __assign_mmap_offset(struct drm_i915_gem_object *obj,
>
> if (mmap_type != I915_MMAP_TYPE_GTT &&
> !i915_gem_object_has_struct_page(obj) &&
> - !i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM))
> + !i915_gem_object_has_iomem(obj))
> return -ENODEV;
>
> mmo = mmap_offset_attach(obj, mmap_type, file);
> @@ -707,7 +707,12 @@ __assign_mmap_offset_handle(struct drm_file *file,
> if (!obj)
> return -ENOENT;
>
> + err = i915_gem_object_lock_interruptible(obj, NULL);
> + if (err)
> + goto out_put;
> err = __assign_mmap_offset(obj, mmap_type, offset, file);
> + i915_gem_object_unlock(obj);
> +out_put:
> i915_gem_object_put(obj);
> return err;
> }
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> index cf18c430d51f..07e8ff9a8aae 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -475,6 +475,44 @@ bool i915_gem_object_migratable(struct drm_i915_gem_object *obj)
> return obj->mm.n_placements > 1;
> }
>
> +/**
> + * i915_gem_object_has_struct_page - Whether the object is page-backed
> + * @obj: The object to query.
> + *
> + * This function should only be called while the object is locked or pinned,
> + * otherwise the page backing may change under the caller.
> + *
> + * Return: True if page-backed, false otherwise.
> + */
> +bool i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj)
> +{
> +#ifdef CONFIG_LOCKDEP
> + if (IS_DGFX(to_i915(obj->base.dev)) &&
> + i915_gem_object_evictable((void __force *)obj))
> + assert_object_held_shared(obj);
> +#endif
> + return obj->mem_flags & I915_BO_FLAG_STRUCT_PAGE;
> +}
> +
> +/**
> + * i915_gem_object_has_iomem - Whether the object is iomem-backed
> + * @obj: The object to query.
> + *
> + * This function should only be called while the object is locked or pinned,
> + * otherwise the iomem backing may change under the caller.
> + *
> + * Return: True if iomem-backed, false otherwise.
> + */
> +bool i915_gem_object_has_iomem(const struct drm_i915_gem_object *obj)
> +{
> +#ifdef CONFIG_LOCKDEP
> + if (IS_DGFX(to_i915(obj->base.dev)) &&
> + i915_gem_object_evictable((void __force *)obj))
> + assert_object_held_shared(obj);
> +#endif
> + return obj->mem_flags & I915_BO_FLAG_IOMEM;
> +}
> +
> void i915_gem_init__objects(struct drm_i915_private *i915)
> {
> INIT_WORK(&i915->mm.free_work, __i915_gem_free_work);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> index ff59e6c640e6..23e26f6b1db9 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> @@ -147,7 +147,7 @@ i915_gem_object_put(struct drm_i915_gem_object *obj)
> /*
> * If more than one potential simultaneous locker, assert held.
> */
> -static inline void assert_object_held_shared(struct drm_i915_gem_object *obj)
> +static inline void assert_object_held_shared(const struct drm_i915_gem_object *obj)
> {
> /*
> * Note mm list lookup is protected by
> @@ -261,17 +261,9 @@ i915_gem_object_type_has(const struct drm_i915_gem_object *obj,
> return obj->ops->flags & flags;
> }
>
> -static inline bool
> -i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj)
> -{
> - return obj->flags & I915_BO_ALLOC_STRUCT_PAGE;
> -}
> +bool i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj);
>
> -static inline bool
> -i915_gem_object_has_iomem(const struct drm_i915_gem_object *obj)
> -{
> - return i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM);
> -}
> +bool i915_gem_object_has_iomem(const struct drm_i915_gem_object *obj);
>
> static inline bool
> i915_gem_object_is_shrinkable(const struct drm_i915_gem_object *obj)
> 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 2a23b77424b3..fb9ccc3f50e7 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> @@ -33,10 +33,9 @@ struct i915_lut_handle {
>
> struct drm_i915_gem_object_ops {
> unsigned int flags;
> -#define I915_GEM_OBJECT_HAS_IOMEM BIT(1)
> -#define I915_GEM_OBJECT_IS_SHRINKABLE BIT(2)
> -#define I915_GEM_OBJECT_IS_PROXY BIT(3)
> -#define I915_GEM_OBJECT_NO_MMAP BIT(4)
> +#define I915_GEM_OBJECT_IS_SHRINKABLE BIT(1)
> +#define I915_GEM_OBJECT_IS_PROXY BIT(2)
> +#define I915_GEM_OBJECT_NO_MMAP BIT(3)
>
> /* Interface between the GEM object and its backing storage.
> * get_pages() is called once prior to the use of the associated set
> @@ -201,17 +200,18 @@ struct drm_i915_gem_object {
> unsigned long flags;
> #define I915_BO_ALLOC_CONTIGUOUS BIT(0)
> #define I915_BO_ALLOC_VOLATILE BIT(1)
> -#define I915_BO_ALLOC_STRUCT_PAGE BIT(2)
> -#define I915_BO_ALLOC_CPU_CLEAR BIT(3)
> -#define I915_BO_ALLOC_USER BIT(4)
> +#define I915_BO_ALLOC_CPU_CLEAR BIT(2)
> +#define I915_BO_ALLOC_USER BIT(3)
> #define I915_BO_ALLOC_FLAGS (I915_BO_ALLOC_CONTIGUOUS | \
> I915_BO_ALLOC_VOLATILE | \
> - I915_BO_ALLOC_STRUCT_PAGE | \
> I915_BO_ALLOC_CPU_CLEAR | \
> I915_BO_ALLOC_USER)
> -#define I915_BO_READONLY BIT(5)
> -#define I915_TILING_QUIRK_BIT 6 /* unknown swizzling; do not release! */
> +#define I915_BO_READONLY BIT(4)
> +#define I915_TILING_QUIRK_BIT 5 /* unknown swizzling; do not release! */
>
> + unsigned int mem_flags:2;
> +#define I915_BO_FLAG_STRUCT_PAGE BIT(0)
> +#define I915_BO_FLAG_IOMEM BIT(1)
> /*
> * Is the object to be mapped as read-only to the GPU
> * Only honoured if hardware has relevant pte bit
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> index 086005c1c7ea..f2f850e31b8e 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> @@ -351,7 +351,7 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj,
> int err;
>
> if (!i915_gem_object_has_struct_page(obj) &&
> - !i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM))
> + !i915_gem_object_has_iomem(obj))
> return ERR_PTR(-ENXIO);
>
> assert_object_held(obj);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> index be72ad0634ba..7986612f48fa 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
> @@ -76,7 +76,7 @@ static int i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj)
> intel_gt_chipset_flush(&to_i915(obj->base.dev)->gt);
>
> /* We're no longer struct page backed */
> - obj->flags &= ~I915_BO_ALLOC_STRUCT_PAGE;
> + obj->mem_flags &= ~I915_BO_FLAG_STRUCT_PAGE;
> __i915_gem_object_set_pages(obj, st, sg->length);
>
> return 0;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> index 5d16c4462fda..3648ae1d6628 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> @@ -284,6 +284,7 @@ __i915_gem_object_release_shmem(struct drm_i915_gem_object *obj,
> bool needs_clflush)
> {
> GEM_BUG_ON(obj->mm.madv == __I915_MADV_PURGED);
> + GEM_WARN_ON(IS_DGFX(to_i915(obj->base.dev)));
>
> if (obj->mm.madv == I915_MADV_DONTNEED)
> obj->mm.dirty = false;
> @@ -302,6 +303,7 @@ void i915_gem_object_put_pages_shmem(struct drm_i915_gem_object *obj, struct sg_
> struct pagevec pvec;
> struct page *page;
>
> + GEM_WARN_ON(IS_DGFX(to_i915(obj->base.dev)));
> __i915_gem_object_release_shmem(obj, pages, true);
>
> i915_gem_gtt_finish_pages(obj, pages);
> @@ -444,7 +446,7 @@ shmem_pread(struct drm_i915_gem_object *obj,
>
> static void shmem_release(struct drm_i915_gem_object *obj)
> {
> - if (obj->flags & I915_BO_ALLOC_STRUCT_PAGE)
> + if (i915_gem_object_has_struct_page(obj))
> i915_gem_object_release_memory_region(obj);
>
> fput(obj->base.filp);
> @@ -513,9 +515,8 @@ static int shmem_object_init(struct intel_memory_region *mem,
> mapping_set_gfp_mask(mapping, mask);
> GEM_BUG_ON(!(mapping_gfp_mask(mapping) & __GFP_RECLAIM));
>
> - i915_gem_object_init(obj, &i915_gem_shmem_ops, &lock_class,
> - I915_BO_ALLOC_STRUCT_PAGE);
> -
> + i915_gem_object_init(obj, &i915_gem_shmem_ops, &lock_class, 0);
> + obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
> obj->write_domain = I915_GEM_DOMAIN_CPU;
> obj->read_domains = I915_GEM_DOMAIN_CPU;
>
> @@ -561,6 +562,7 @@ i915_gem_object_create_shmem_from_data(struct drm_i915_private *dev_priv,
> resource_size_t offset;
> int err;
>
> + GEM_WARN_ON(IS_DGFX(dev_priv));
> obj = i915_gem_object_create_shmem(dev_priv, round_up(size, PAGE_SIZE));
> if (IS_ERR(obj))
> return obj;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index 3748098b42d5..ae12a2be11a2 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -563,7 +563,6 @@ static u64 i915_ttm_mmap_offset(struct drm_i915_gem_object *obj)
>
> const struct drm_i915_gem_object_ops i915_gem_ttm_obj_ops = {
> .name = "i915_gem_object_ttm",
> - .flags = I915_GEM_OBJECT_HAS_IOMEM,
>
> .get_pages = i915_ttm_get_pages,
> .put_pages = i915_ttm_put_pages,
> @@ -620,6 +619,7 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
> i915_gem_object_init_memory_region(obj, mem);
> i915_gem_object_make_unshrinkable(obj);
> obj->read_domains = I915_GEM_DOMAIN_WC | I915_GEM_DOMAIN_GTT;
> + obj->mem_flags |= I915_BO_FLAG_IOMEM;
> 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);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index 602f0ed983ec..41dfcb75f05b 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -538,8 +538,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
> return -ENOMEM;
>
> drm_gem_private_object_init(dev, &obj->base, args->user_size);
> - i915_gem_object_init(obj, &i915_gem_userptr_ops, &lock_class,
> - I915_BO_ALLOC_STRUCT_PAGE);
> + i915_gem_object_init(obj, &i915_gem_userptr_ops, &lock_class, 0);
> + obj->mem_flags = I915_BO_FLAG_STRUCT_PAGE;
> obj->read_domains = I915_GEM_DOMAIN_CPU;
> obj->write_domain = I915_GEM_DOMAIN_CPU;
> i915_gem_object_set_cache_coherency(obj, I915_CACHE_LLC);
> diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
> index 0c8ecfdf5405..f963b8e1e37b 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
> @@ -114,8 +114,8 @@ huge_gem_object(struct drm_i915_private *i915,
> return ERR_PTR(-ENOMEM);
>
> drm_gem_private_object_init(&i915->drm, &obj->base, dma_size);
> - i915_gem_object_init(obj, &huge_ops, &lock_class,
> - I915_BO_ALLOC_STRUCT_PAGE);
> + i915_gem_object_init(obj, &huge_ops, &lock_class, 0);
> + obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
>
> obj->read_domains = I915_GEM_DOMAIN_CPU;
> obj->write_domain = I915_GEM_DOMAIN_CPU;
> diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
> index dadd485bc52f..ccc67ed1a84b 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
> @@ -167,9 +167,8 @@ huge_pages_object(struct drm_i915_private *i915,
> return ERR_PTR(-ENOMEM);
>
> drm_gem_private_object_init(&i915->drm, &obj->base, size);
> - i915_gem_object_init(obj, &huge_page_ops, &lock_class,
> - I915_BO_ALLOC_STRUCT_PAGE);
> -
> + i915_gem_object_init(obj, &huge_page_ops, &lock_class, 0);
> + obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
> i915_gem_object_set_volatile(obj);
>
> obj->write_domain = I915_GEM_DOMAIN_CPU;
> 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 ca69a29b7f2a..bfb35270a1f0 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> @@ -837,7 +837,7 @@ static bool can_mmap(struct drm_i915_gem_object *obj, enum i915_mmap_type type)
>
> if (type != I915_MMAP_TYPE_GTT &&
> !i915_gem_object_has_struct_page(obj) &&
> - !i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM))
> + !i915_gem_object_has_iomem(obj))
> return false;
>
> return true;
> @@ -991,7 +991,7 @@ static const char *repr_mmap_type(enum i915_mmap_type type)
> static bool can_access(const struct drm_i915_gem_object *obj)
> {
> return i915_gem_object_has_struct_page(obj) ||
> - i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM);
> + i915_gem_object_has_iomem(obj);
> }
>
> static int __igt_mmap_access(struct drm_i915_private *i915,
> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c
> index 3a6ce87f8b52..d43d8dae0f69 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c
> @@ -25,13 +25,14 @@ static int mock_phys_object(void *arg)
> goto out;
> }
>
> + i915_gem_object_lock(obj, NULL);
> if (!i915_gem_object_has_struct_page(obj)) {
> + i915_gem_object_unlock(obj);
> err = -EINVAL;
> pr_err("shmem has no struct page\n");
> goto out_obj;
> }
>
> - i915_gem_object_lock(obj, NULL);
> err = i915_gem_object_attach_phys(obj, PAGE_SIZE);
> i915_gem_object_unlock(obj);
> if (err) {
> --
> 2.31.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Intel-gfx] [PATCH 1/5] drm/i915: Update object placement flags to be mutable
2021-06-03 11:17 ` Matthew Auld
@ 2021-06-03 14:08 ` Thomas Hellström
-1 siblings, 0 replies; 27+ messages in thread
From: Thomas Hellström @ 2021-06-03 14:08 UTC (permalink / raw)
To: Matthew Auld; +Cc: Intel Graphics Development, ML dri-devel
On 6/3/21 1:17 PM, Matthew Auld wrote:
> On Wed, 2 Jun 2021 at 18:08, Thomas Hellström
> <thomas.hellstrom@linux.intel.com> wrote:
>> The object ops i915_GEM_OBJECT_HAS_IOMEM and the object
>> I915_BO_ALLOC_STRUCT_PAGE flags are considered immutable by
>> much of our code. Introduce a new mem_flags member to hold these
>> and make sure checks for these flags being set are either done
>> under the object lock or with pages properly pinned. The flags
>> will change during migration under the object lock.
> What are the rules for the gem_object_ops flags? Like is_shrinkable?
> Can't we just move these there(IOMEM vs PAGE)?
But the ops flags are immutable, right? We expect the IOMEM and PAGE
flags to change when an object is migrated. We could of course flip the
ops under the lock, but I'd consider that a bit risky?
>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/gem/i915_gem_internal.c | 4 +-
>> drivers/gpu/drm/i915/gem/i915_gem_mman.c | 7 +++-
>> drivers/gpu/drm/i915/gem/i915_gem_object.c | 38 +++++++++++++++++++
>> drivers/gpu/drm/i915/gem/i915_gem_object.h | 14 ++-----
>> .../gpu/drm/i915/gem/i915_gem_object_types.h | 20 +++++-----
>> drivers/gpu/drm/i915/gem/i915_gem_pages.c | 2 +-
>> drivers/gpu/drm/i915/gem/i915_gem_phys.c | 2 +-
>> drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 10 +++--
>> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 2 +-
>> drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 4 +-
>> .../drm/i915/gem/selftests/huge_gem_object.c | 4 +-
>> .../gpu/drm/i915/gem/selftests/huge_pages.c | 5 +--
>> .../drm/i915/gem/selftests/i915_gem_mman.c | 4 +-
>> .../drm/i915/gem/selftests/i915_gem_phys.c | 3 +-
>> 14 files changed, 78 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
>> index ce6b664b10aa..13b217f75055 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
>> @@ -177,8 +177,8 @@ i915_gem_object_create_internal(struct drm_i915_private *i915,
>> return ERR_PTR(-ENOMEM);
>>
>> drm_gem_private_object_init(&i915->drm, &obj->base, size);
>> - i915_gem_object_init(obj, &i915_gem_object_internal_ops, &lock_class,
>> - I915_BO_ALLOC_STRUCT_PAGE);
>> + i915_gem_object_init(obj, &i915_gem_object_internal_ops, &lock_class, 0);
>> + obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
>>
>> /*
>> * Mark the object as volatile, such that the pages are marked as
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> index d1de97e4adfd..171a21ca930c 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> @@ -683,7 +683,7 @@ __assign_mmap_offset(struct drm_i915_gem_object *obj,
>>
>> if (mmap_type != I915_MMAP_TYPE_GTT &&
>> !i915_gem_object_has_struct_page(obj) &&
>> - !i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM))
>> + !i915_gem_object_has_iomem(obj))
>> return -ENODEV;
>>
>> mmo = mmap_offset_attach(obj, mmap_type, file);
>> @@ -707,7 +707,12 @@ __assign_mmap_offset_handle(struct drm_file *file,
>> if (!obj)
>> return -ENOENT;
>>
>> + err = i915_gem_object_lock_interruptible(obj, NULL);
>> + if (err)
>> + goto out_put;
>> err = __assign_mmap_offset(obj, mmap_type, offset, file);
>> + i915_gem_object_unlock(obj);
>> +out_put:
>> i915_gem_object_put(obj);
>> return err;
>> }
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> index cf18c430d51f..07e8ff9a8aae 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> @@ -475,6 +475,44 @@ bool i915_gem_object_migratable(struct drm_i915_gem_object *obj)
>> return obj->mm.n_placements > 1;
>> }
>>
>> +/**
>> + * i915_gem_object_has_struct_page - Whether the object is page-backed
>> + * @obj: The object to query.
>> + *
>> + * This function should only be called while the object is locked or pinned,
>> + * otherwise the page backing may change under the caller.
>> + *
>> + * Return: True if page-backed, false otherwise.
>> + */
>> +bool i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj)
>> +{
>> +#ifdef CONFIG_LOCKDEP
>> + if (IS_DGFX(to_i915(obj->base.dev)) &&
>> + i915_gem_object_evictable((void __force *)obj))
>> + assert_object_held_shared(obj);
>> +#endif
>> + return obj->mem_flags & I915_BO_FLAG_STRUCT_PAGE;
>> +}
>> +
>> +/**
>> + * i915_gem_object_has_iomem - Whether the object is iomem-backed
>> + * @obj: The object to query.
>> + *
>> + * This function should only be called while the object is locked or pinned,
>> + * otherwise the iomem backing may change under the caller.
>> + *
>> + * Return: True if iomem-backed, false otherwise.
>> + */
>> +bool i915_gem_object_has_iomem(const struct drm_i915_gem_object *obj)
>> +{
>> +#ifdef CONFIG_LOCKDEP
>> + if (IS_DGFX(to_i915(obj->base.dev)) &&
>> + i915_gem_object_evictable((void __force *)obj))
>> + assert_object_held_shared(obj);
>> +#endif
>> + return obj->mem_flags & I915_BO_FLAG_IOMEM;
>> +}
>> +
>> void i915_gem_init__objects(struct drm_i915_private *i915)
>> {
>> INIT_WORK(&i915->mm.free_work, __i915_gem_free_work);
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
>> index ff59e6c640e6..23e26f6b1db9 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
>> @@ -147,7 +147,7 @@ i915_gem_object_put(struct drm_i915_gem_object *obj)
>> /*
>> * If more than one potential simultaneous locker, assert held.
>> */
>> -static inline void assert_object_held_shared(struct drm_i915_gem_object *obj)
>> +static inline void assert_object_held_shared(const struct drm_i915_gem_object *obj)
>> {
>> /*
>> * Note mm list lookup is protected by
>> @@ -261,17 +261,9 @@ i915_gem_object_type_has(const struct drm_i915_gem_object *obj,
>> return obj->ops->flags & flags;
>> }
>>
>> -static inline bool
>> -i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj)
>> -{
>> - return obj->flags & I915_BO_ALLOC_STRUCT_PAGE;
>> -}
>> +bool i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj);
>>
>> -static inline bool
>> -i915_gem_object_has_iomem(const struct drm_i915_gem_object *obj)
>> -{
>> - return i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM);
>> -}
>> +bool i915_gem_object_has_iomem(const struct drm_i915_gem_object *obj);
>>
>> static inline bool
>> i915_gem_object_is_shrinkable(const struct drm_i915_gem_object *obj)
>> 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 2a23b77424b3..fb9ccc3f50e7 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>> @@ -33,10 +33,9 @@ struct i915_lut_handle {
>>
>> struct drm_i915_gem_object_ops {
>> unsigned int flags;
>> -#define I915_GEM_OBJECT_HAS_IOMEM BIT(1)
>> -#define I915_GEM_OBJECT_IS_SHRINKABLE BIT(2)
>> -#define I915_GEM_OBJECT_IS_PROXY BIT(3)
>> -#define I915_GEM_OBJECT_NO_MMAP BIT(4)
>> +#define I915_GEM_OBJECT_IS_SHRINKABLE BIT(1)
>> +#define I915_GEM_OBJECT_IS_PROXY BIT(2)
>> +#define I915_GEM_OBJECT_NO_MMAP BIT(3)
>>
>> /* Interface between the GEM object and its backing storage.
>> * get_pages() is called once prior to the use of the associated set
>> @@ -201,17 +200,18 @@ struct drm_i915_gem_object {
>> unsigned long flags;
>> #define I915_BO_ALLOC_CONTIGUOUS BIT(0)
>> #define I915_BO_ALLOC_VOLATILE BIT(1)
>> -#define I915_BO_ALLOC_STRUCT_PAGE BIT(2)
>> -#define I915_BO_ALLOC_CPU_CLEAR BIT(3)
>> -#define I915_BO_ALLOC_USER BIT(4)
>> +#define I915_BO_ALLOC_CPU_CLEAR BIT(2)
>> +#define I915_BO_ALLOC_USER BIT(3)
>> #define I915_BO_ALLOC_FLAGS (I915_BO_ALLOC_CONTIGUOUS | \
>> I915_BO_ALLOC_VOLATILE | \
>> - I915_BO_ALLOC_STRUCT_PAGE | \
>> I915_BO_ALLOC_CPU_CLEAR | \
>> I915_BO_ALLOC_USER)
>> -#define I915_BO_READONLY BIT(5)
>> -#define I915_TILING_QUIRK_BIT 6 /* unknown swizzling; do not release! */
>> +#define I915_BO_READONLY BIT(4)
>> +#define I915_TILING_QUIRK_BIT 5 /* unknown swizzling; do not release! */
>>
>> + unsigned int mem_flags:2;
>> +#define I915_BO_FLAG_STRUCT_PAGE BIT(0)
>> +#define I915_BO_FLAG_IOMEM BIT(1)
>> /*
>> * Is the object to be mapped as read-only to the GPU
>> * Only honoured if hardware has relevant pte bit
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
>> index 086005c1c7ea..f2f850e31b8e 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
>> @@ -351,7 +351,7 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj,
>> int err;
>>
>> if (!i915_gem_object_has_struct_page(obj) &&
>> - !i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM))
>> + !i915_gem_object_has_iomem(obj))
>> return ERR_PTR(-ENXIO);
>>
>> assert_object_held(obj);
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
>> index be72ad0634ba..7986612f48fa 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
>> @@ -76,7 +76,7 @@ static int i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj)
>> intel_gt_chipset_flush(&to_i915(obj->base.dev)->gt);
>>
>> /* We're no longer struct page backed */
>> - obj->flags &= ~I915_BO_ALLOC_STRUCT_PAGE;
>> + obj->mem_flags &= ~I915_BO_FLAG_STRUCT_PAGE;
>> __i915_gem_object_set_pages(obj, st, sg->length);
>>
>> return 0;
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> index 5d16c4462fda..3648ae1d6628 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> @@ -284,6 +284,7 @@ __i915_gem_object_release_shmem(struct drm_i915_gem_object *obj,
>> bool needs_clflush)
>> {
>> GEM_BUG_ON(obj->mm.madv == __I915_MADV_PURGED);
>> + GEM_WARN_ON(IS_DGFX(to_i915(obj->base.dev)));
>>
>> if (obj->mm.madv == I915_MADV_DONTNEED)
>> obj->mm.dirty = false;
>> @@ -302,6 +303,7 @@ void i915_gem_object_put_pages_shmem(struct drm_i915_gem_object *obj, struct sg_
>> struct pagevec pvec;
>> struct page *page;
>>
>> + GEM_WARN_ON(IS_DGFX(to_i915(obj->base.dev)));
>> __i915_gem_object_release_shmem(obj, pages, true);
>>
>> i915_gem_gtt_finish_pages(obj, pages);
>> @@ -444,7 +446,7 @@ shmem_pread(struct drm_i915_gem_object *obj,
>>
>> static void shmem_release(struct drm_i915_gem_object *obj)
>> {
>> - if (obj->flags & I915_BO_ALLOC_STRUCT_PAGE)
>> + if (i915_gem_object_has_struct_page(obj))
>> i915_gem_object_release_memory_region(obj);
>>
>> fput(obj->base.filp);
>> @@ -513,9 +515,8 @@ static int shmem_object_init(struct intel_memory_region *mem,
>> mapping_set_gfp_mask(mapping, mask);
>> GEM_BUG_ON(!(mapping_gfp_mask(mapping) & __GFP_RECLAIM));
>>
>> - i915_gem_object_init(obj, &i915_gem_shmem_ops, &lock_class,
>> - I915_BO_ALLOC_STRUCT_PAGE);
>> -
>> + i915_gem_object_init(obj, &i915_gem_shmem_ops, &lock_class, 0);
>> + obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
>> obj->write_domain = I915_GEM_DOMAIN_CPU;
>> obj->read_domains = I915_GEM_DOMAIN_CPU;
>>
>> @@ -561,6 +562,7 @@ i915_gem_object_create_shmem_from_data(struct drm_i915_private *dev_priv,
>> resource_size_t offset;
>> int err;
>>
>> + GEM_WARN_ON(IS_DGFX(dev_priv));
>> obj = i915_gem_object_create_shmem(dev_priv, round_up(size, PAGE_SIZE));
>> if (IS_ERR(obj))
>> return obj;
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> index 3748098b42d5..ae12a2be11a2 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> @@ -563,7 +563,6 @@ static u64 i915_ttm_mmap_offset(struct drm_i915_gem_object *obj)
>>
>> const struct drm_i915_gem_object_ops i915_gem_ttm_obj_ops = {
>> .name = "i915_gem_object_ttm",
>> - .flags = I915_GEM_OBJECT_HAS_IOMEM,
>>
>> .get_pages = i915_ttm_get_pages,
>> .put_pages = i915_ttm_put_pages,
>> @@ -620,6 +619,7 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
>> i915_gem_object_init_memory_region(obj, mem);
>> i915_gem_object_make_unshrinkable(obj);
>> obj->read_domains = I915_GEM_DOMAIN_WC | I915_GEM_DOMAIN_GTT;
>> + obj->mem_flags |= I915_BO_FLAG_IOMEM;
>> 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);
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>> index 602f0ed983ec..41dfcb75f05b 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>> @@ -538,8 +538,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
>> return -ENOMEM;
>>
>> drm_gem_private_object_init(dev, &obj->base, args->user_size);
>> - i915_gem_object_init(obj, &i915_gem_userptr_ops, &lock_class,
>> - I915_BO_ALLOC_STRUCT_PAGE);
>> + i915_gem_object_init(obj, &i915_gem_userptr_ops, &lock_class, 0);
>> + obj->mem_flags = I915_BO_FLAG_STRUCT_PAGE;
>> obj->read_domains = I915_GEM_DOMAIN_CPU;
>> obj->write_domain = I915_GEM_DOMAIN_CPU;
>> i915_gem_object_set_cache_coherency(obj, I915_CACHE_LLC);
>> diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
>> index 0c8ecfdf5405..f963b8e1e37b 100644
>> --- a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
>> +++ b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
>> @@ -114,8 +114,8 @@ huge_gem_object(struct drm_i915_private *i915,
>> return ERR_PTR(-ENOMEM);
>>
>> drm_gem_private_object_init(&i915->drm, &obj->base, dma_size);
>> - i915_gem_object_init(obj, &huge_ops, &lock_class,
>> - I915_BO_ALLOC_STRUCT_PAGE);
>> + i915_gem_object_init(obj, &huge_ops, &lock_class, 0);
>> + obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
>>
>> obj->read_domains = I915_GEM_DOMAIN_CPU;
>> obj->write_domain = I915_GEM_DOMAIN_CPU;
>> diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
>> index dadd485bc52f..ccc67ed1a84b 100644
>> --- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
>> +++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
>> @@ -167,9 +167,8 @@ huge_pages_object(struct drm_i915_private *i915,
>> return ERR_PTR(-ENOMEM);
>>
>> drm_gem_private_object_init(&i915->drm, &obj->base, size);
>> - i915_gem_object_init(obj, &huge_page_ops, &lock_class,
>> - I915_BO_ALLOC_STRUCT_PAGE);
>> -
>> + i915_gem_object_init(obj, &huge_page_ops, &lock_class, 0);
>> + obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
>> i915_gem_object_set_volatile(obj);
>>
>> obj->write_domain = I915_GEM_DOMAIN_CPU;
>> 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 ca69a29b7f2a..bfb35270a1f0 100644
>> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
>> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
>> @@ -837,7 +837,7 @@ static bool can_mmap(struct drm_i915_gem_object *obj, enum i915_mmap_type type)
>>
>> if (type != I915_MMAP_TYPE_GTT &&
>> !i915_gem_object_has_struct_page(obj) &&
>> - !i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM))
>> + !i915_gem_object_has_iomem(obj))
>> return false;
>>
>> return true;
>> @@ -991,7 +991,7 @@ static const char *repr_mmap_type(enum i915_mmap_type type)
>> static bool can_access(const struct drm_i915_gem_object *obj)
>> {
>> return i915_gem_object_has_struct_page(obj) ||
>> - i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM);
>> + i915_gem_object_has_iomem(obj);
>> }
>>
>> static int __igt_mmap_access(struct drm_i915_private *i915,
>> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c
>> index 3a6ce87f8b52..d43d8dae0f69 100644
>> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c
>> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c
>> @@ -25,13 +25,14 @@ static int mock_phys_object(void *arg)
>> goto out;
>> }
>>
>> + i915_gem_object_lock(obj, NULL);
>> if (!i915_gem_object_has_struct_page(obj)) {
>> + i915_gem_object_unlock(obj);
>> err = -EINVAL;
>> pr_err("shmem has no struct page\n");
>> goto out_obj;
>> }
>>
>> - i915_gem_object_lock(obj, NULL);
>> err = i915_gem_object_attach_phys(obj, PAGE_SIZE);
>> i915_gem_object_unlock(obj);
>> if (err) {
>> --
>> 2.31.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Intel-gfx] [PATCH 1/5] drm/i915: Update object placement flags to be mutable
@ 2021-06-03 14:08 ` Thomas Hellström
0 siblings, 0 replies; 27+ messages in thread
From: Thomas Hellström @ 2021-06-03 14:08 UTC (permalink / raw)
To: Matthew Auld; +Cc: Intel Graphics Development, ML dri-devel
On 6/3/21 1:17 PM, Matthew Auld wrote:
> On Wed, 2 Jun 2021 at 18:08, Thomas Hellström
> <thomas.hellstrom@linux.intel.com> wrote:
>> The object ops i915_GEM_OBJECT_HAS_IOMEM and the object
>> I915_BO_ALLOC_STRUCT_PAGE flags are considered immutable by
>> much of our code. Introduce a new mem_flags member to hold these
>> and make sure checks for these flags being set are either done
>> under the object lock or with pages properly pinned. The flags
>> will change during migration under the object lock.
> What are the rules for the gem_object_ops flags? Like is_shrinkable?
> Can't we just move these there(IOMEM vs PAGE)?
But the ops flags are immutable, right? We expect the IOMEM and PAGE
flags to change when an object is migrated. We could of course flip the
ops under the lock, but I'd consider that a bit risky?
>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/gem/i915_gem_internal.c | 4 +-
>> drivers/gpu/drm/i915/gem/i915_gem_mman.c | 7 +++-
>> drivers/gpu/drm/i915/gem/i915_gem_object.c | 38 +++++++++++++++++++
>> drivers/gpu/drm/i915/gem/i915_gem_object.h | 14 ++-----
>> .../gpu/drm/i915/gem/i915_gem_object_types.h | 20 +++++-----
>> drivers/gpu/drm/i915/gem/i915_gem_pages.c | 2 +-
>> drivers/gpu/drm/i915/gem/i915_gem_phys.c | 2 +-
>> drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 10 +++--
>> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 2 +-
>> drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 4 +-
>> .../drm/i915/gem/selftests/huge_gem_object.c | 4 +-
>> .../gpu/drm/i915/gem/selftests/huge_pages.c | 5 +--
>> .../drm/i915/gem/selftests/i915_gem_mman.c | 4 +-
>> .../drm/i915/gem/selftests/i915_gem_phys.c | 3 +-
>> 14 files changed, 78 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
>> index ce6b664b10aa..13b217f75055 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
>> @@ -177,8 +177,8 @@ i915_gem_object_create_internal(struct drm_i915_private *i915,
>> return ERR_PTR(-ENOMEM);
>>
>> drm_gem_private_object_init(&i915->drm, &obj->base, size);
>> - i915_gem_object_init(obj, &i915_gem_object_internal_ops, &lock_class,
>> - I915_BO_ALLOC_STRUCT_PAGE);
>> + i915_gem_object_init(obj, &i915_gem_object_internal_ops, &lock_class, 0);
>> + obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
>>
>> /*
>> * Mark the object as volatile, such that the pages are marked as
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> index d1de97e4adfd..171a21ca930c 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> @@ -683,7 +683,7 @@ __assign_mmap_offset(struct drm_i915_gem_object *obj,
>>
>> if (mmap_type != I915_MMAP_TYPE_GTT &&
>> !i915_gem_object_has_struct_page(obj) &&
>> - !i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM))
>> + !i915_gem_object_has_iomem(obj))
>> return -ENODEV;
>>
>> mmo = mmap_offset_attach(obj, mmap_type, file);
>> @@ -707,7 +707,12 @@ __assign_mmap_offset_handle(struct drm_file *file,
>> if (!obj)
>> return -ENOENT;
>>
>> + err = i915_gem_object_lock_interruptible(obj, NULL);
>> + if (err)
>> + goto out_put;
>> err = __assign_mmap_offset(obj, mmap_type, offset, file);
>> + i915_gem_object_unlock(obj);
>> +out_put:
>> i915_gem_object_put(obj);
>> return err;
>> }
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> index cf18c430d51f..07e8ff9a8aae 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> @@ -475,6 +475,44 @@ bool i915_gem_object_migratable(struct drm_i915_gem_object *obj)
>> return obj->mm.n_placements > 1;
>> }
>>
>> +/**
>> + * i915_gem_object_has_struct_page - Whether the object is page-backed
>> + * @obj: The object to query.
>> + *
>> + * This function should only be called while the object is locked or pinned,
>> + * otherwise the page backing may change under the caller.
>> + *
>> + * Return: True if page-backed, false otherwise.
>> + */
>> +bool i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj)
>> +{
>> +#ifdef CONFIG_LOCKDEP
>> + if (IS_DGFX(to_i915(obj->base.dev)) &&
>> + i915_gem_object_evictable((void __force *)obj))
>> + assert_object_held_shared(obj);
>> +#endif
>> + return obj->mem_flags & I915_BO_FLAG_STRUCT_PAGE;
>> +}
>> +
>> +/**
>> + * i915_gem_object_has_iomem - Whether the object is iomem-backed
>> + * @obj: The object to query.
>> + *
>> + * This function should only be called while the object is locked or pinned,
>> + * otherwise the iomem backing may change under the caller.
>> + *
>> + * Return: True if iomem-backed, false otherwise.
>> + */
>> +bool i915_gem_object_has_iomem(const struct drm_i915_gem_object *obj)
>> +{
>> +#ifdef CONFIG_LOCKDEP
>> + if (IS_DGFX(to_i915(obj->base.dev)) &&
>> + i915_gem_object_evictable((void __force *)obj))
>> + assert_object_held_shared(obj);
>> +#endif
>> + return obj->mem_flags & I915_BO_FLAG_IOMEM;
>> +}
>> +
>> void i915_gem_init__objects(struct drm_i915_private *i915)
>> {
>> INIT_WORK(&i915->mm.free_work, __i915_gem_free_work);
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
>> index ff59e6c640e6..23e26f6b1db9 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
>> @@ -147,7 +147,7 @@ i915_gem_object_put(struct drm_i915_gem_object *obj)
>> /*
>> * If more than one potential simultaneous locker, assert held.
>> */
>> -static inline void assert_object_held_shared(struct drm_i915_gem_object *obj)
>> +static inline void assert_object_held_shared(const struct drm_i915_gem_object *obj)
>> {
>> /*
>> * Note mm list lookup is protected by
>> @@ -261,17 +261,9 @@ i915_gem_object_type_has(const struct drm_i915_gem_object *obj,
>> return obj->ops->flags & flags;
>> }
>>
>> -static inline bool
>> -i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj)
>> -{
>> - return obj->flags & I915_BO_ALLOC_STRUCT_PAGE;
>> -}
>> +bool i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj);
>>
>> -static inline bool
>> -i915_gem_object_has_iomem(const struct drm_i915_gem_object *obj)
>> -{
>> - return i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM);
>> -}
>> +bool i915_gem_object_has_iomem(const struct drm_i915_gem_object *obj);
>>
>> static inline bool
>> i915_gem_object_is_shrinkable(const struct drm_i915_gem_object *obj)
>> 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 2a23b77424b3..fb9ccc3f50e7 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>> @@ -33,10 +33,9 @@ struct i915_lut_handle {
>>
>> struct drm_i915_gem_object_ops {
>> unsigned int flags;
>> -#define I915_GEM_OBJECT_HAS_IOMEM BIT(1)
>> -#define I915_GEM_OBJECT_IS_SHRINKABLE BIT(2)
>> -#define I915_GEM_OBJECT_IS_PROXY BIT(3)
>> -#define I915_GEM_OBJECT_NO_MMAP BIT(4)
>> +#define I915_GEM_OBJECT_IS_SHRINKABLE BIT(1)
>> +#define I915_GEM_OBJECT_IS_PROXY BIT(2)
>> +#define I915_GEM_OBJECT_NO_MMAP BIT(3)
>>
>> /* Interface between the GEM object and its backing storage.
>> * get_pages() is called once prior to the use of the associated set
>> @@ -201,17 +200,18 @@ struct drm_i915_gem_object {
>> unsigned long flags;
>> #define I915_BO_ALLOC_CONTIGUOUS BIT(0)
>> #define I915_BO_ALLOC_VOLATILE BIT(1)
>> -#define I915_BO_ALLOC_STRUCT_PAGE BIT(2)
>> -#define I915_BO_ALLOC_CPU_CLEAR BIT(3)
>> -#define I915_BO_ALLOC_USER BIT(4)
>> +#define I915_BO_ALLOC_CPU_CLEAR BIT(2)
>> +#define I915_BO_ALLOC_USER BIT(3)
>> #define I915_BO_ALLOC_FLAGS (I915_BO_ALLOC_CONTIGUOUS | \
>> I915_BO_ALLOC_VOLATILE | \
>> - I915_BO_ALLOC_STRUCT_PAGE | \
>> I915_BO_ALLOC_CPU_CLEAR | \
>> I915_BO_ALLOC_USER)
>> -#define I915_BO_READONLY BIT(5)
>> -#define I915_TILING_QUIRK_BIT 6 /* unknown swizzling; do not release! */
>> +#define I915_BO_READONLY BIT(4)
>> +#define I915_TILING_QUIRK_BIT 5 /* unknown swizzling; do not release! */
>>
>> + unsigned int mem_flags:2;
>> +#define I915_BO_FLAG_STRUCT_PAGE BIT(0)
>> +#define I915_BO_FLAG_IOMEM BIT(1)
>> /*
>> * Is the object to be mapped as read-only to the GPU
>> * Only honoured if hardware has relevant pte bit
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
>> index 086005c1c7ea..f2f850e31b8e 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
>> @@ -351,7 +351,7 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj,
>> int err;
>>
>> if (!i915_gem_object_has_struct_page(obj) &&
>> - !i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM))
>> + !i915_gem_object_has_iomem(obj))
>> return ERR_PTR(-ENXIO);
>>
>> assert_object_held(obj);
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
>> index be72ad0634ba..7986612f48fa 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
>> @@ -76,7 +76,7 @@ static int i915_gem_object_get_pages_phys(struct drm_i915_gem_object *obj)
>> intel_gt_chipset_flush(&to_i915(obj->base.dev)->gt);
>>
>> /* We're no longer struct page backed */
>> - obj->flags &= ~I915_BO_ALLOC_STRUCT_PAGE;
>> + obj->mem_flags &= ~I915_BO_FLAG_STRUCT_PAGE;
>> __i915_gem_object_set_pages(obj, st, sg->length);
>>
>> return 0;
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> index 5d16c4462fda..3648ae1d6628 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> @@ -284,6 +284,7 @@ __i915_gem_object_release_shmem(struct drm_i915_gem_object *obj,
>> bool needs_clflush)
>> {
>> GEM_BUG_ON(obj->mm.madv == __I915_MADV_PURGED);
>> + GEM_WARN_ON(IS_DGFX(to_i915(obj->base.dev)));
>>
>> if (obj->mm.madv == I915_MADV_DONTNEED)
>> obj->mm.dirty = false;
>> @@ -302,6 +303,7 @@ void i915_gem_object_put_pages_shmem(struct drm_i915_gem_object *obj, struct sg_
>> struct pagevec pvec;
>> struct page *page;
>>
>> + GEM_WARN_ON(IS_DGFX(to_i915(obj->base.dev)));
>> __i915_gem_object_release_shmem(obj, pages, true);
>>
>> i915_gem_gtt_finish_pages(obj, pages);
>> @@ -444,7 +446,7 @@ shmem_pread(struct drm_i915_gem_object *obj,
>>
>> static void shmem_release(struct drm_i915_gem_object *obj)
>> {
>> - if (obj->flags & I915_BO_ALLOC_STRUCT_PAGE)
>> + if (i915_gem_object_has_struct_page(obj))
>> i915_gem_object_release_memory_region(obj);
>>
>> fput(obj->base.filp);
>> @@ -513,9 +515,8 @@ static int shmem_object_init(struct intel_memory_region *mem,
>> mapping_set_gfp_mask(mapping, mask);
>> GEM_BUG_ON(!(mapping_gfp_mask(mapping) & __GFP_RECLAIM));
>>
>> - i915_gem_object_init(obj, &i915_gem_shmem_ops, &lock_class,
>> - I915_BO_ALLOC_STRUCT_PAGE);
>> -
>> + i915_gem_object_init(obj, &i915_gem_shmem_ops, &lock_class, 0);
>> + obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
>> obj->write_domain = I915_GEM_DOMAIN_CPU;
>> obj->read_domains = I915_GEM_DOMAIN_CPU;
>>
>> @@ -561,6 +562,7 @@ i915_gem_object_create_shmem_from_data(struct drm_i915_private *dev_priv,
>> resource_size_t offset;
>> int err;
>>
>> + GEM_WARN_ON(IS_DGFX(dev_priv));
>> obj = i915_gem_object_create_shmem(dev_priv, round_up(size, PAGE_SIZE));
>> if (IS_ERR(obj))
>> return obj;
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> index 3748098b42d5..ae12a2be11a2 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> @@ -563,7 +563,6 @@ static u64 i915_ttm_mmap_offset(struct drm_i915_gem_object *obj)
>>
>> const struct drm_i915_gem_object_ops i915_gem_ttm_obj_ops = {
>> .name = "i915_gem_object_ttm",
>> - .flags = I915_GEM_OBJECT_HAS_IOMEM,
>>
>> .get_pages = i915_ttm_get_pages,
>> .put_pages = i915_ttm_put_pages,
>> @@ -620,6 +619,7 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
>> i915_gem_object_init_memory_region(obj, mem);
>> i915_gem_object_make_unshrinkable(obj);
>> obj->read_domains = I915_GEM_DOMAIN_WC | I915_GEM_DOMAIN_GTT;
>> + obj->mem_flags |= I915_BO_FLAG_IOMEM;
>> 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);
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>> index 602f0ed983ec..41dfcb75f05b 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>> @@ -538,8 +538,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
>> return -ENOMEM;
>>
>> drm_gem_private_object_init(dev, &obj->base, args->user_size);
>> - i915_gem_object_init(obj, &i915_gem_userptr_ops, &lock_class,
>> - I915_BO_ALLOC_STRUCT_PAGE);
>> + i915_gem_object_init(obj, &i915_gem_userptr_ops, &lock_class, 0);
>> + obj->mem_flags = I915_BO_FLAG_STRUCT_PAGE;
>> obj->read_domains = I915_GEM_DOMAIN_CPU;
>> obj->write_domain = I915_GEM_DOMAIN_CPU;
>> i915_gem_object_set_cache_coherency(obj, I915_CACHE_LLC);
>> diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
>> index 0c8ecfdf5405..f963b8e1e37b 100644
>> --- a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
>> +++ b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
>> @@ -114,8 +114,8 @@ huge_gem_object(struct drm_i915_private *i915,
>> return ERR_PTR(-ENOMEM);
>>
>> drm_gem_private_object_init(&i915->drm, &obj->base, dma_size);
>> - i915_gem_object_init(obj, &huge_ops, &lock_class,
>> - I915_BO_ALLOC_STRUCT_PAGE);
>> + i915_gem_object_init(obj, &huge_ops, &lock_class, 0);
>> + obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
>>
>> obj->read_domains = I915_GEM_DOMAIN_CPU;
>> obj->write_domain = I915_GEM_DOMAIN_CPU;
>> diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
>> index dadd485bc52f..ccc67ed1a84b 100644
>> --- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
>> +++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
>> @@ -167,9 +167,8 @@ huge_pages_object(struct drm_i915_private *i915,
>> return ERR_PTR(-ENOMEM);
>>
>> drm_gem_private_object_init(&i915->drm, &obj->base, size);
>> - i915_gem_object_init(obj, &huge_page_ops, &lock_class,
>> - I915_BO_ALLOC_STRUCT_PAGE);
>> -
>> + i915_gem_object_init(obj, &huge_page_ops, &lock_class, 0);
>> + obj->mem_flags |= I915_BO_FLAG_STRUCT_PAGE;
>> i915_gem_object_set_volatile(obj);
>>
>> obj->write_domain = I915_GEM_DOMAIN_CPU;
>> 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 ca69a29b7f2a..bfb35270a1f0 100644
>> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
>> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
>> @@ -837,7 +837,7 @@ static bool can_mmap(struct drm_i915_gem_object *obj, enum i915_mmap_type type)
>>
>> if (type != I915_MMAP_TYPE_GTT &&
>> !i915_gem_object_has_struct_page(obj) &&
>> - !i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM))
>> + !i915_gem_object_has_iomem(obj))
>> return false;
>>
>> return true;
>> @@ -991,7 +991,7 @@ static const char *repr_mmap_type(enum i915_mmap_type type)
>> static bool can_access(const struct drm_i915_gem_object *obj)
>> {
>> return i915_gem_object_has_struct_page(obj) ||
>> - i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM);
>> + i915_gem_object_has_iomem(obj);
>> }
>>
>> static int __igt_mmap_access(struct drm_i915_private *i915,
>> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c
>> index 3a6ce87f8b52..d43d8dae0f69 100644
>> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c
>> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c
>> @@ -25,13 +25,14 @@ static int mock_phys_object(void *arg)
>> goto out;
>> }
>>
>> + i915_gem_object_lock(obj, NULL);
>> if (!i915_gem_object_has_struct_page(obj)) {
>> + i915_gem_object_unlock(obj);
>> err = -EINVAL;
>> pr_err("shmem has no struct page\n");
>> goto out_obj;
>> }
>>
>> - i915_gem_object_lock(obj, NULL);
>> err = i915_gem_object_attach_phys(obj, PAGE_SIZE);
>> i915_gem_object_unlock(obj);
>> if (err) {
>> --
>> 2.31.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 2/5] drm/i915/ttm: Adjust gem flags and caching settings after a move
2021-06-02 17:07 ` [Intel-gfx] " Thomas Hellström
@ 2021-06-02 17:07 ` Thomas Hellström
-1 siblings, 0 replies; 27+ messages in thread
From: Thomas Hellström @ 2021-06-02 17:07 UTC (permalink / raw)
To: intel-gfx, dri-devel; +Cc: Thomas Hellström
After a TTM move we need to update the i915 gem flags and caching
settings to reflect the new placement.
Also introduce gpu_binds_iomem() and cpu_maps_iomem() to clean up the
various ways we previously used to detect this.
Finally, initialize the TTM object reserved to be able to update
flags and caching before anyone else gets hold of the object.
Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 112 +++++++++++++++++++-----
1 file changed, 90 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index ae12a2be11a2..c73c51755c20 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -70,6 +70,17 @@ static struct ttm_placement i915_sys_placement = {
.busy_placement = &lmem0_sys_placement_flags[1],
};
+static bool gpu_binds_iomem(struct ttm_resource *mem)
+{
+ return (mem->mem_type != TTM_PL_SYSTEM);
+}
+
+static bool cpu_maps_iomem(struct ttm_resource *mem)
+{
+ /* Once / if we support GGTT, this is also false for cached ttm_tts */
+ return (mem->mem_type != TTM_PL_SYSTEM);
+}
+
static void i915_ttm_adjust_lru(struct drm_i915_gem_object *obj);
static struct ttm_tt *i915_ttm_tt_create(struct ttm_buffer_object *bo,
@@ -175,6 +186,41 @@ static void i915_ttm_free_cached_io_st(struct drm_i915_gem_object *obj)
obj->ttm.cached_io_st = NULL;
}
+static void
+i915_ttm_adjust_domains_after_cpu_move(struct drm_i915_gem_object *obj)
+{
+ struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
+
+ if (cpu_maps_iomem(&bo->mem) || bo->ttm->caching != ttm_cached) {
+ obj->write_domain = I915_GEM_DOMAIN_WC;
+ obj->read_domains = I915_GEM_DOMAIN_WC;
+ } else {
+ obj->write_domain = I915_GEM_DOMAIN_CPU;
+ obj->read_domains = I915_GEM_DOMAIN_CPU;
+ }
+}
+
+static void i915_ttm_adjust_gem_after_move(struct drm_i915_gem_object *obj)
+{
+ struct drm_i915_private *i915 = to_i915(obj->base.dev);
+ struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
+ unsigned int cache_level;
+
+ obj->mem_flags &= ~(I915_BO_FLAG_STRUCT_PAGE | I915_BO_FLAG_IOMEM);
+
+ obj->mem_flags |= cpu_maps_iomem(&bo->mem) ? I915_BO_FLAG_IOMEM :
+ I915_BO_FLAG_STRUCT_PAGE;
+
+ if ((HAS_LLC(i915) || HAS_SNOOP(i915)) && !gpu_binds_iomem(&bo->mem) &&
+ bo->ttm->caching == ttm_cached) {
+ cache_level = I915_CACHE_LLC;
+ } else {
+ cache_level = I915_CACHE_NONE;
+ }
+
+ i915_gem_object_set_cache_coherency(obj, cache_level);
+}
+
static void i915_ttm_purge(struct drm_i915_gem_object *obj)
{
struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
@@ -190,8 +236,10 @@ static void i915_ttm_purge(struct drm_i915_gem_object *obj)
/* TTM's purge interface. Note that we might be reentering. */
ret = ttm_bo_validate(bo, &place, &ctx);
-
if (!ret) {
+ obj->write_domain = 0;
+ obj->read_domains = 0;
+ i915_ttm_adjust_gem_after_move(obj);
i915_ttm_free_cached_io_st(obj);
obj->mm.madv = __I915_MADV_PURGED;
}
@@ -273,12 +321,15 @@ i915_ttm_resource_get_st(struct drm_i915_gem_object *obj,
struct ttm_resource *res)
{
struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
- struct ttm_resource_manager *man =
- ttm_manager_type(bo->bdev, res->mem_type);
- if (man->use_tt)
+ if (!gpu_binds_iomem(res))
return i915_ttm_tt_get_st(bo->ttm);
+ /*
+ * If CPU mapping differs, we need to add the ttm_tt pages to
+ * the resulting st. Might make sense for GGTT.
+ */
+ GEM_WARN_ON(!cpu_maps_iomem(res));
return intel_region_ttm_node_to_st(obj->mm.region, res->mm_node);
}
@@ -290,8 +341,6 @@ static int i915_ttm_move(struct ttm_buffer_object *bo, bool evict,
struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
struct ttm_resource_manager *dst_man =
ttm_manager_type(bo->bdev, dst_mem->mem_type);
- struct ttm_resource_manager *src_man =
- ttm_manager_type(bo->bdev, bo->mem.mem_type);
struct intel_memory_region *dst_reg, *src_reg;
union {
struct ttm_kmap_iter_tt tt;
@@ -332,34 +381,36 @@ static int i915_ttm_move(struct ttm_buffer_object *bo, bool evict,
if (IS_ERR(dst_st))
return PTR_ERR(dst_st);
- /* If we start mapping GGTT, we can no longer use man::use_tt here. */
- dst_iter = dst_man->use_tt ?
+ dst_iter = !cpu_maps_iomem(dst_mem) ?
ttm_kmap_iter_tt_init(&_dst_iter.tt, bo->ttm) :
ttm_kmap_iter_iomap_init(&_dst_iter.io, &dst_reg->iomap,
dst_st, dst_reg->region.start);
- src_iter = src_man->use_tt ?
+ src_iter = !cpu_maps_iomem(&bo->mem) ?
ttm_kmap_iter_tt_init(&_src_iter.tt, bo->ttm) :
ttm_kmap_iter_iomap_init(&_src_iter.io, &src_reg->iomap,
obj->ttm.cached_io_st,
src_reg->region.start);
ttm_move_memcpy(bo, dst_mem->num_pages, dst_iter, src_iter);
+ /* Below dst_mem becomes bo->mem. */
ttm_bo_move_sync_cleanup(bo, dst_mem);
+ i915_ttm_adjust_domains_after_cpu_move(obj);
i915_ttm_free_cached_io_st(obj);
- if (!dst_man->use_tt) {
+ if (gpu_binds_iomem(dst_mem) || cpu_maps_iomem(dst_mem)) {
obj->ttm.cached_io_st = dst_st;
obj->ttm.get_io_page.sg_pos = dst_st->sgl;
obj->ttm.get_io_page.sg_idx = 0;
}
+ i915_ttm_adjust_gem_after_move(obj);
return 0;
}
static int i915_ttm_io_mem_reserve(struct ttm_device *bdev, struct ttm_resource *mem)
{
- if (mem->mem_type < I915_PL_LMEM0)
+ if (!cpu_maps_iomem(mem))
return 0;
mem->bus.caching = ttm_write_combined;
@@ -421,6 +472,16 @@ static int i915_ttm_get_pages(struct drm_i915_gem_object *obj)
if (ret)
return ret == -ENOSPC ? -ENXIO : ret;
+ i915_ttm_adjust_lru(obj);
+ if (bo->ttm && !ttm_tt_is_populated(bo->ttm)) {
+ ret = ttm_tt_populate(bo->bdev, bo->ttm, &ctx);
+ if (ret)
+ return ret;
+
+ i915_ttm_adjust_domains_after_cpu_move(obj);
+ i915_ttm_adjust_gem_after_move(obj);
+ }
+
/* Object either has a page vector or is an iomem object */
st = bo->ttm ? i915_ttm_tt_get_st(bo->ttm) : obj->ttm.cached_io_st;
if (IS_ERR(st))
@@ -428,8 +489,6 @@ static int i915_ttm_get_pages(struct drm_i915_gem_object *obj)
__i915_gem_object_set_pages(obj, st, i915_sg_dma_sizes(st->sgl));
- i915_ttm_adjust_lru(obj);
-
return ret;
}
@@ -563,6 +622,7 @@ static u64 i915_ttm_mmap_offset(struct drm_i915_gem_object *obj)
const struct drm_i915_gem_object_ops i915_gem_ttm_obj_ops = {
.name = "i915_gem_object_ttm",
+ .flags = I915_GEM_OBJECT_IS_SHRINKABLE,
.get_pages = i915_ttm_get_pages,
.put_pages = i915_ttm_put_pages,
@@ -599,6 +659,10 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
{
static struct lock_class_key lock_class;
struct drm_i915_private *i915 = mem->i915;
+ struct ttm_operation_ctx ctx = {
+ .interruptible = true,
+ .no_wait_gpu = false,
+ };
enum ttm_bo_type bo_type;
size_t alignment = 0;
int ret;
@@ -618,15 +682,14 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
i915_gem_object_init(obj, &i915_gem_ttm_obj_ops, &lock_class, flags);
i915_gem_object_init_memory_region(obj, mem);
i915_gem_object_make_unshrinkable(obj);
- obj->read_domains = I915_GEM_DOMAIN_WC | I915_GEM_DOMAIN_GTT;
- obj->mem_flags |= I915_BO_FLAG_IOMEM;
- 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);
bo_type = (obj->flags & I915_BO_ALLOC_USER) ? ttm_bo_type_device :
ttm_bo_type_kernel;
+ obj->base.vma_node.driver_private = i915_gem_to_ttm(obj);
+
/*
* If this function fails, it will call the destructor, but
* our caller still owns the object. So no freeing in the
@@ -634,14 +697,19 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
* Similarly, in delayed_destroy, we can't call ttm_bo_put()
* until successful initialization.
*/
- obj->base.vma_node.driver_private = i915_gem_to_ttm(obj);
- ret = ttm_bo_init(&i915->bdev, i915_gem_to_ttm(obj), size,
- bo_type, &i915_sys_placement, alignment,
- true, NULL, NULL, i915_ttm_bo_destroy);
+ ret = ttm_bo_init_reserved(&i915->bdev, i915_gem_to_ttm(obj), size,
+ bo_type, &i915_sys_placement, alignment,
+ &ctx, NULL, NULL, i915_ttm_bo_destroy);
+
+ if (ret)
+ goto out;
- if (!ret)
- obj->ttm.created = true;
+ obj->ttm.created = true;
+ i915_ttm_adjust_domains_after_cpu_move(obj);
+ i915_ttm_adjust_gem_after_move(obj);
+ i915_gem_object_unlock(obj);
+out:
/* i915 wants -ENXIO when out of memory region space. */
return (ret == -ENOSPC) ? -ENXIO : ret;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [Intel-gfx] [PATCH 2/5] drm/i915/ttm: Adjust gem flags and caching settings after a move
@ 2021-06-02 17:07 ` Thomas Hellström
0 siblings, 0 replies; 27+ messages in thread
From: Thomas Hellström @ 2021-06-02 17:07 UTC (permalink / raw)
To: intel-gfx, dri-devel; +Cc: Thomas Hellström
After a TTM move we need to update the i915 gem flags and caching
settings to reflect the new placement.
Also introduce gpu_binds_iomem() and cpu_maps_iomem() to clean up the
various ways we previously used to detect this.
Finally, initialize the TTM object reserved to be able to update
flags and caching before anyone else gets hold of the object.
Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 112 +++++++++++++++++++-----
1 file changed, 90 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index ae12a2be11a2..c73c51755c20 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -70,6 +70,17 @@ static struct ttm_placement i915_sys_placement = {
.busy_placement = &lmem0_sys_placement_flags[1],
};
+static bool gpu_binds_iomem(struct ttm_resource *mem)
+{
+ return (mem->mem_type != TTM_PL_SYSTEM);
+}
+
+static bool cpu_maps_iomem(struct ttm_resource *mem)
+{
+ /* Once / if we support GGTT, this is also false for cached ttm_tts */
+ return (mem->mem_type != TTM_PL_SYSTEM);
+}
+
static void i915_ttm_adjust_lru(struct drm_i915_gem_object *obj);
static struct ttm_tt *i915_ttm_tt_create(struct ttm_buffer_object *bo,
@@ -175,6 +186,41 @@ static void i915_ttm_free_cached_io_st(struct drm_i915_gem_object *obj)
obj->ttm.cached_io_st = NULL;
}
+static void
+i915_ttm_adjust_domains_after_cpu_move(struct drm_i915_gem_object *obj)
+{
+ struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
+
+ if (cpu_maps_iomem(&bo->mem) || bo->ttm->caching != ttm_cached) {
+ obj->write_domain = I915_GEM_DOMAIN_WC;
+ obj->read_domains = I915_GEM_DOMAIN_WC;
+ } else {
+ obj->write_domain = I915_GEM_DOMAIN_CPU;
+ obj->read_domains = I915_GEM_DOMAIN_CPU;
+ }
+}
+
+static void i915_ttm_adjust_gem_after_move(struct drm_i915_gem_object *obj)
+{
+ struct drm_i915_private *i915 = to_i915(obj->base.dev);
+ struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
+ unsigned int cache_level;
+
+ obj->mem_flags &= ~(I915_BO_FLAG_STRUCT_PAGE | I915_BO_FLAG_IOMEM);
+
+ obj->mem_flags |= cpu_maps_iomem(&bo->mem) ? I915_BO_FLAG_IOMEM :
+ I915_BO_FLAG_STRUCT_PAGE;
+
+ if ((HAS_LLC(i915) || HAS_SNOOP(i915)) && !gpu_binds_iomem(&bo->mem) &&
+ bo->ttm->caching == ttm_cached) {
+ cache_level = I915_CACHE_LLC;
+ } else {
+ cache_level = I915_CACHE_NONE;
+ }
+
+ i915_gem_object_set_cache_coherency(obj, cache_level);
+}
+
static void i915_ttm_purge(struct drm_i915_gem_object *obj)
{
struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
@@ -190,8 +236,10 @@ static void i915_ttm_purge(struct drm_i915_gem_object *obj)
/* TTM's purge interface. Note that we might be reentering. */
ret = ttm_bo_validate(bo, &place, &ctx);
-
if (!ret) {
+ obj->write_domain = 0;
+ obj->read_domains = 0;
+ i915_ttm_adjust_gem_after_move(obj);
i915_ttm_free_cached_io_st(obj);
obj->mm.madv = __I915_MADV_PURGED;
}
@@ -273,12 +321,15 @@ i915_ttm_resource_get_st(struct drm_i915_gem_object *obj,
struct ttm_resource *res)
{
struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
- struct ttm_resource_manager *man =
- ttm_manager_type(bo->bdev, res->mem_type);
- if (man->use_tt)
+ if (!gpu_binds_iomem(res))
return i915_ttm_tt_get_st(bo->ttm);
+ /*
+ * If CPU mapping differs, we need to add the ttm_tt pages to
+ * the resulting st. Might make sense for GGTT.
+ */
+ GEM_WARN_ON(!cpu_maps_iomem(res));
return intel_region_ttm_node_to_st(obj->mm.region, res->mm_node);
}
@@ -290,8 +341,6 @@ static int i915_ttm_move(struct ttm_buffer_object *bo, bool evict,
struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
struct ttm_resource_manager *dst_man =
ttm_manager_type(bo->bdev, dst_mem->mem_type);
- struct ttm_resource_manager *src_man =
- ttm_manager_type(bo->bdev, bo->mem.mem_type);
struct intel_memory_region *dst_reg, *src_reg;
union {
struct ttm_kmap_iter_tt tt;
@@ -332,34 +381,36 @@ static int i915_ttm_move(struct ttm_buffer_object *bo, bool evict,
if (IS_ERR(dst_st))
return PTR_ERR(dst_st);
- /* If we start mapping GGTT, we can no longer use man::use_tt here. */
- dst_iter = dst_man->use_tt ?
+ dst_iter = !cpu_maps_iomem(dst_mem) ?
ttm_kmap_iter_tt_init(&_dst_iter.tt, bo->ttm) :
ttm_kmap_iter_iomap_init(&_dst_iter.io, &dst_reg->iomap,
dst_st, dst_reg->region.start);
- src_iter = src_man->use_tt ?
+ src_iter = !cpu_maps_iomem(&bo->mem) ?
ttm_kmap_iter_tt_init(&_src_iter.tt, bo->ttm) :
ttm_kmap_iter_iomap_init(&_src_iter.io, &src_reg->iomap,
obj->ttm.cached_io_st,
src_reg->region.start);
ttm_move_memcpy(bo, dst_mem->num_pages, dst_iter, src_iter);
+ /* Below dst_mem becomes bo->mem. */
ttm_bo_move_sync_cleanup(bo, dst_mem);
+ i915_ttm_adjust_domains_after_cpu_move(obj);
i915_ttm_free_cached_io_st(obj);
- if (!dst_man->use_tt) {
+ if (gpu_binds_iomem(dst_mem) || cpu_maps_iomem(dst_mem)) {
obj->ttm.cached_io_st = dst_st;
obj->ttm.get_io_page.sg_pos = dst_st->sgl;
obj->ttm.get_io_page.sg_idx = 0;
}
+ i915_ttm_adjust_gem_after_move(obj);
return 0;
}
static int i915_ttm_io_mem_reserve(struct ttm_device *bdev, struct ttm_resource *mem)
{
- if (mem->mem_type < I915_PL_LMEM0)
+ if (!cpu_maps_iomem(mem))
return 0;
mem->bus.caching = ttm_write_combined;
@@ -421,6 +472,16 @@ static int i915_ttm_get_pages(struct drm_i915_gem_object *obj)
if (ret)
return ret == -ENOSPC ? -ENXIO : ret;
+ i915_ttm_adjust_lru(obj);
+ if (bo->ttm && !ttm_tt_is_populated(bo->ttm)) {
+ ret = ttm_tt_populate(bo->bdev, bo->ttm, &ctx);
+ if (ret)
+ return ret;
+
+ i915_ttm_adjust_domains_after_cpu_move(obj);
+ i915_ttm_adjust_gem_after_move(obj);
+ }
+
/* Object either has a page vector or is an iomem object */
st = bo->ttm ? i915_ttm_tt_get_st(bo->ttm) : obj->ttm.cached_io_st;
if (IS_ERR(st))
@@ -428,8 +489,6 @@ static int i915_ttm_get_pages(struct drm_i915_gem_object *obj)
__i915_gem_object_set_pages(obj, st, i915_sg_dma_sizes(st->sgl));
- i915_ttm_adjust_lru(obj);
-
return ret;
}
@@ -563,6 +622,7 @@ static u64 i915_ttm_mmap_offset(struct drm_i915_gem_object *obj)
const struct drm_i915_gem_object_ops i915_gem_ttm_obj_ops = {
.name = "i915_gem_object_ttm",
+ .flags = I915_GEM_OBJECT_IS_SHRINKABLE,
.get_pages = i915_ttm_get_pages,
.put_pages = i915_ttm_put_pages,
@@ -599,6 +659,10 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
{
static struct lock_class_key lock_class;
struct drm_i915_private *i915 = mem->i915;
+ struct ttm_operation_ctx ctx = {
+ .interruptible = true,
+ .no_wait_gpu = false,
+ };
enum ttm_bo_type bo_type;
size_t alignment = 0;
int ret;
@@ -618,15 +682,14 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
i915_gem_object_init(obj, &i915_gem_ttm_obj_ops, &lock_class, flags);
i915_gem_object_init_memory_region(obj, mem);
i915_gem_object_make_unshrinkable(obj);
- obj->read_domains = I915_GEM_DOMAIN_WC | I915_GEM_DOMAIN_GTT;
- obj->mem_flags |= I915_BO_FLAG_IOMEM;
- 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);
bo_type = (obj->flags & I915_BO_ALLOC_USER) ? ttm_bo_type_device :
ttm_bo_type_kernel;
+ obj->base.vma_node.driver_private = i915_gem_to_ttm(obj);
+
/*
* If this function fails, it will call the destructor, but
* our caller still owns the object. So no freeing in the
@@ -634,14 +697,19 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
* Similarly, in delayed_destroy, we can't call ttm_bo_put()
* until successful initialization.
*/
- obj->base.vma_node.driver_private = i915_gem_to_ttm(obj);
- ret = ttm_bo_init(&i915->bdev, i915_gem_to_ttm(obj), size,
- bo_type, &i915_sys_placement, alignment,
- true, NULL, NULL, i915_ttm_bo_destroy);
+ ret = ttm_bo_init_reserved(&i915->bdev, i915_gem_to_ttm(obj), size,
+ bo_type, &i915_sys_placement, alignment,
+ &ctx, NULL, NULL, i915_ttm_bo_destroy);
+
+ if (ret)
+ goto out;
- if (!ret)
- obj->ttm.created = true;
+ obj->ttm.created = true;
+ i915_ttm_adjust_domains_after_cpu_move(obj);
+ i915_ttm_adjust_gem_after_move(obj);
+ i915_gem_object_unlock(obj);
+out:
/* i915 wants -ENXIO when out of memory region space. */
return (ret == -ENOSPC) ? -ENXIO : ret;
}
--
2.31.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [Intel-gfx] [PATCH 2/5] drm/i915/ttm: Adjust gem flags and caching settings after a move
2021-06-02 17:07 ` [Intel-gfx] " Thomas Hellström
@ 2021-06-03 10:35 ` Matthew Auld
-1 siblings, 0 replies; 27+ messages in thread
From: Matthew Auld @ 2021-06-03 10:35 UTC (permalink / raw)
To: Thomas Hellström; +Cc: Intel Graphics Development, ML dri-devel
On Wed, 2 Jun 2021 at 18:08, Thomas Hellström
<thomas.hellstrom@linux.intel.com> wrote:
>
> After a TTM move we need to update the i915 gem flags and caching
> settings to reflect the new placement.
> Also introduce gpu_binds_iomem() and cpu_maps_iomem() to clean up the
> various ways we previously used to detect this.
> Finally, initialize the TTM object reserved to be able to update
> flags and caching before anyone else gets hold of the object.
>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 112 +++++++++++++++++++-----
> 1 file changed, 90 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index ae12a2be11a2..c73c51755c20 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -70,6 +70,17 @@ static struct ttm_placement i915_sys_placement = {
> .busy_placement = &lmem0_sys_placement_flags[1],
> };
>
> +static bool gpu_binds_iomem(struct ttm_resource *mem)
What does gpu_binds mean here? It's mapped though the GTT?
> +{
> + return (mem->mem_type != TTM_PL_SYSTEM);
> +}
> +
> +static bool cpu_maps_iomem(struct ttm_resource *mem)
> +{
> + /* Once / if we support GGTT, this is also false for cached ttm_tts */
> + return (mem->mem_type != TTM_PL_SYSTEM);
> +}
Can drop the extra brackets.
> +
> static void i915_ttm_adjust_lru(struct drm_i915_gem_object *obj);
>
> static struct ttm_tt *i915_ttm_tt_create(struct ttm_buffer_object *bo,
> @@ -175,6 +186,41 @@ static void i915_ttm_free_cached_io_st(struct drm_i915_gem_object *obj)
> obj->ttm.cached_io_st = NULL;
> }
>
> +static void
> +i915_ttm_adjust_domains_after_cpu_move(struct drm_i915_gem_object *obj)
> +{
> + struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
> +
> + if (cpu_maps_iomem(&bo->mem) || bo->ttm->caching != ttm_cached) {
> + obj->write_domain = I915_GEM_DOMAIN_WC;
> + obj->read_domains = I915_GEM_DOMAIN_WC;
> + } else {
> + obj->write_domain = I915_GEM_DOMAIN_CPU;
> + obj->read_domains = I915_GEM_DOMAIN_CPU;
> + }
> +}
> +
> +static void i915_ttm_adjust_gem_after_move(struct drm_i915_gem_object *obj)
> +{
> + struct drm_i915_private *i915 = to_i915(obj->base.dev);
> + struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
> + unsigned int cache_level;
> +
> + obj->mem_flags &= ~(I915_BO_FLAG_STRUCT_PAGE | I915_BO_FLAG_IOMEM);
> +
> + obj->mem_flags |= cpu_maps_iomem(&bo->mem) ? I915_BO_FLAG_IOMEM :
> + I915_BO_FLAG_STRUCT_PAGE;
> +
> + if ((HAS_LLC(i915) || HAS_SNOOP(i915)) && !gpu_binds_iomem(&bo->mem) &&
I think all modern hw has support for snooping or the fast shared LLC.
Are we expecting to hit this path for non-dgpu? Also by default we
will choose ttm_cached at least for platforms like dg1? Also pin_map()
is still a separate interface at this point?
> + bo->ttm->caching == ttm_cached) {
> + cache_level = I915_CACHE_LLC;
> + } else {
> + cache_level = I915_CACHE_NONE;
> + }
> +
> + i915_gem_object_set_cache_coherency(obj, cache_level);
> +}
> +
> static void i915_ttm_purge(struct drm_i915_gem_object *obj)
> {
> struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
> @@ -190,8 +236,10 @@ static void i915_ttm_purge(struct drm_i915_gem_object *obj)
>
> /* TTM's purge interface. Note that we might be reentering. */
> ret = ttm_bo_validate(bo, &place, &ctx);
> -
> if (!ret) {
> + obj->write_domain = 0;
> + obj->read_domains = 0;
> + i915_ttm_adjust_gem_after_move(obj);
> i915_ttm_free_cached_io_st(obj);
> obj->mm.madv = __I915_MADV_PURGED;
> }
> @@ -273,12 +321,15 @@ i915_ttm_resource_get_st(struct drm_i915_gem_object *obj,
> struct ttm_resource *res)
> {
> struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
> - struct ttm_resource_manager *man =
> - ttm_manager_type(bo->bdev, res->mem_type);
>
> - if (man->use_tt)
> + if (!gpu_binds_iomem(res))
> return i915_ttm_tt_get_st(bo->ttm);
>
> + /*
> + * If CPU mapping differs, we need to add the ttm_tt pages to
> + * the resulting st. Might make sense for GGTT.
> + */
> + GEM_WARN_ON(!cpu_maps_iomem(res));
> return intel_region_ttm_node_to_st(obj->mm.region, res->mm_node);
> }
>
> @@ -290,8 +341,6 @@ static int i915_ttm_move(struct ttm_buffer_object *bo, bool evict,
> struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
> struct ttm_resource_manager *dst_man =
> ttm_manager_type(bo->bdev, dst_mem->mem_type);
> - struct ttm_resource_manager *src_man =
> - ttm_manager_type(bo->bdev, bo->mem.mem_type);
> struct intel_memory_region *dst_reg, *src_reg;
> union {
> struct ttm_kmap_iter_tt tt;
> @@ -332,34 +381,36 @@ static int i915_ttm_move(struct ttm_buffer_object *bo, bool evict,
> if (IS_ERR(dst_st))
> return PTR_ERR(dst_st);
>
> - /* If we start mapping GGTT, we can no longer use man::use_tt here. */
> - dst_iter = dst_man->use_tt ?
> + dst_iter = !cpu_maps_iomem(dst_mem) ?
> ttm_kmap_iter_tt_init(&_dst_iter.tt, bo->ttm) :
> ttm_kmap_iter_iomap_init(&_dst_iter.io, &dst_reg->iomap,
> dst_st, dst_reg->region.start);
>
> - src_iter = src_man->use_tt ?
> + src_iter = !cpu_maps_iomem(&bo->mem) ?
> ttm_kmap_iter_tt_init(&_src_iter.tt, bo->ttm) :
> ttm_kmap_iter_iomap_init(&_src_iter.io, &src_reg->iomap,
> obj->ttm.cached_io_st,
> src_reg->region.start);
>
> ttm_move_memcpy(bo, dst_mem->num_pages, dst_iter, src_iter);
> + /* Below dst_mem becomes bo->mem. */
> ttm_bo_move_sync_cleanup(bo, dst_mem);
> + i915_ttm_adjust_domains_after_cpu_move(obj);
> i915_ttm_free_cached_io_st(obj);
>
> - if (!dst_man->use_tt) {
> + if (gpu_binds_iomem(dst_mem) || cpu_maps_iomem(dst_mem)) {
> obj->ttm.cached_io_st = dst_st;
> obj->ttm.get_io_page.sg_pos = dst_st->sgl;
> obj->ttm.get_io_page.sg_idx = 0;
> }
>
> + i915_ttm_adjust_gem_after_move(obj);
> return 0;
> }
>
> static int i915_ttm_io_mem_reserve(struct ttm_device *bdev, struct ttm_resource *mem)
> {
> - if (mem->mem_type < I915_PL_LMEM0)
> + if (!cpu_maps_iomem(mem))
> return 0;
>
> mem->bus.caching = ttm_write_combined;
> @@ -421,6 +472,16 @@ static int i915_ttm_get_pages(struct drm_i915_gem_object *obj)
> if (ret)
> return ret == -ENOSPC ? -ENXIO : ret;
>
> + i915_ttm_adjust_lru(obj);
> + if (bo->ttm && !ttm_tt_is_populated(bo->ttm)) {
> + ret = ttm_tt_populate(bo->bdev, bo->ttm, &ctx);
> + if (ret)
> + return ret;
> +
> + i915_ttm_adjust_domains_after_cpu_move(obj);
> + i915_ttm_adjust_gem_after_move(obj);
> + }
> +
> /* Object either has a page vector or is an iomem object */
> st = bo->ttm ? i915_ttm_tt_get_st(bo->ttm) : obj->ttm.cached_io_st;
> if (IS_ERR(st))
> @@ -428,8 +489,6 @@ static int i915_ttm_get_pages(struct drm_i915_gem_object *obj)
>
> __i915_gem_object_set_pages(obj, st, i915_sg_dma_sizes(st->sgl));
>
> - i915_ttm_adjust_lru(obj);
> -
> return ret;
> }
>
> @@ -563,6 +622,7 @@ static u64 i915_ttm_mmap_offset(struct drm_i915_gem_object *obj)
>
> const struct drm_i915_gem_object_ops i915_gem_ttm_obj_ops = {
> .name = "i915_gem_object_ttm",
> + .flags = I915_GEM_OBJECT_IS_SHRINKABLE,
>
> .get_pages = i915_ttm_get_pages,
> .put_pages = i915_ttm_put_pages,
> @@ -599,6 +659,10 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
> {
> static struct lock_class_key lock_class;
> struct drm_i915_private *i915 = mem->i915;
> + struct ttm_operation_ctx ctx = {
> + .interruptible = true,
> + .no_wait_gpu = false,
> + };
> enum ttm_bo_type bo_type;
> size_t alignment = 0;
> int ret;
> @@ -618,15 +682,14 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
> i915_gem_object_init(obj, &i915_gem_ttm_obj_ops, &lock_class, flags);
> i915_gem_object_init_memory_region(obj, mem);
> i915_gem_object_make_unshrinkable(obj);
> - obj->read_domains = I915_GEM_DOMAIN_WC | I915_GEM_DOMAIN_GTT;
> - obj->mem_flags |= I915_BO_FLAG_IOMEM;
> - 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);
>
> bo_type = (obj->flags & I915_BO_ALLOC_USER) ? ttm_bo_type_device :
> ttm_bo_type_kernel;
>
> + obj->base.vma_node.driver_private = i915_gem_to_ttm(obj);
> +
> /*
> * If this function fails, it will call the destructor, but
> * our caller still owns the object. So no freeing in the
> @@ -634,14 +697,19 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
> * Similarly, in delayed_destroy, we can't call ttm_bo_put()
> * until successful initialization.
> */
> - obj->base.vma_node.driver_private = i915_gem_to_ttm(obj);
> - ret = ttm_bo_init(&i915->bdev, i915_gem_to_ttm(obj), size,
> - bo_type, &i915_sys_placement, alignment,
> - true, NULL, NULL, i915_ttm_bo_destroy);
> + ret = ttm_bo_init_reserved(&i915->bdev, i915_gem_to_ttm(obj), size,
> + bo_type, &i915_sys_placement, alignment,
> + &ctx, NULL, NULL, i915_ttm_bo_destroy);
> +
> + if (ret)
> + goto out;
>
> - if (!ret)
> - obj->ttm.created = true;
> + obj->ttm.created = true;
> + i915_ttm_adjust_domains_after_cpu_move(obj);
> + i915_ttm_adjust_gem_after_move(obj);
> + i915_gem_object_unlock(obj);
>
> +out:
> /* i915 wants -ENXIO when out of memory region space. */
> return (ret == -ENOSPC) ? -ENXIO : ret;
> }
> --
> 2.31.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Intel-gfx] [PATCH 2/5] drm/i915/ttm: Adjust gem flags and caching settings after a move
@ 2021-06-03 10:35 ` Matthew Auld
0 siblings, 0 replies; 27+ messages in thread
From: Matthew Auld @ 2021-06-03 10:35 UTC (permalink / raw)
To: Thomas Hellström; +Cc: Intel Graphics Development, ML dri-devel
On Wed, 2 Jun 2021 at 18:08, Thomas Hellström
<thomas.hellstrom@linux.intel.com> wrote:
>
> After a TTM move we need to update the i915 gem flags and caching
> settings to reflect the new placement.
> Also introduce gpu_binds_iomem() and cpu_maps_iomem() to clean up the
> various ways we previously used to detect this.
> Finally, initialize the TTM object reserved to be able to update
> flags and caching before anyone else gets hold of the object.
>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 112 +++++++++++++++++++-----
> 1 file changed, 90 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index ae12a2be11a2..c73c51755c20 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -70,6 +70,17 @@ static struct ttm_placement i915_sys_placement = {
> .busy_placement = &lmem0_sys_placement_flags[1],
> };
>
> +static bool gpu_binds_iomem(struct ttm_resource *mem)
What does gpu_binds mean here? It's mapped though the GTT?
> +{
> + return (mem->mem_type != TTM_PL_SYSTEM);
> +}
> +
> +static bool cpu_maps_iomem(struct ttm_resource *mem)
> +{
> + /* Once / if we support GGTT, this is also false for cached ttm_tts */
> + return (mem->mem_type != TTM_PL_SYSTEM);
> +}
Can drop the extra brackets.
> +
> static void i915_ttm_adjust_lru(struct drm_i915_gem_object *obj);
>
> static struct ttm_tt *i915_ttm_tt_create(struct ttm_buffer_object *bo,
> @@ -175,6 +186,41 @@ static void i915_ttm_free_cached_io_st(struct drm_i915_gem_object *obj)
> obj->ttm.cached_io_st = NULL;
> }
>
> +static void
> +i915_ttm_adjust_domains_after_cpu_move(struct drm_i915_gem_object *obj)
> +{
> + struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
> +
> + if (cpu_maps_iomem(&bo->mem) || bo->ttm->caching != ttm_cached) {
> + obj->write_domain = I915_GEM_DOMAIN_WC;
> + obj->read_domains = I915_GEM_DOMAIN_WC;
> + } else {
> + obj->write_domain = I915_GEM_DOMAIN_CPU;
> + obj->read_domains = I915_GEM_DOMAIN_CPU;
> + }
> +}
> +
> +static void i915_ttm_adjust_gem_after_move(struct drm_i915_gem_object *obj)
> +{
> + struct drm_i915_private *i915 = to_i915(obj->base.dev);
> + struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
> + unsigned int cache_level;
> +
> + obj->mem_flags &= ~(I915_BO_FLAG_STRUCT_PAGE | I915_BO_FLAG_IOMEM);
> +
> + obj->mem_flags |= cpu_maps_iomem(&bo->mem) ? I915_BO_FLAG_IOMEM :
> + I915_BO_FLAG_STRUCT_PAGE;
> +
> + if ((HAS_LLC(i915) || HAS_SNOOP(i915)) && !gpu_binds_iomem(&bo->mem) &&
I think all modern hw has support for snooping or the fast shared LLC.
Are we expecting to hit this path for non-dgpu? Also by default we
will choose ttm_cached at least for platforms like dg1? Also pin_map()
is still a separate interface at this point?
> + bo->ttm->caching == ttm_cached) {
> + cache_level = I915_CACHE_LLC;
> + } else {
> + cache_level = I915_CACHE_NONE;
> + }
> +
> + i915_gem_object_set_cache_coherency(obj, cache_level);
> +}
> +
> static void i915_ttm_purge(struct drm_i915_gem_object *obj)
> {
> struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
> @@ -190,8 +236,10 @@ static void i915_ttm_purge(struct drm_i915_gem_object *obj)
>
> /* TTM's purge interface. Note that we might be reentering. */
> ret = ttm_bo_validate(bo, &place, &ctx);
> -
> if (!ret) {
> + obj->write_domain = 0;
> + obj->read_domains = 0;
> + i915_ttm_adjust_gem_after_move(obj);
> i915_ttm_free_cached_io_st(obj);
> obj->mm.madv = __I915_MADV_PURGED;
> }
> @@ -273,12 +321,15 @@ i915_ttm_resource_get_st(struct drm_i915_gem_object *obj,
> struct ttm_resource *res)
> {
> struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
> - struct ttm_resource_manager *man =
> - ttm_manager_type(bo->bdev, res->mem_type);
>
> - if (man->use_tt)
> + if (!gpu_binds_iomem(res))
> return i915_ttm_tt_get_st(bo->ttm);
>
> + /*
> + * If CPU mapping differs, we need to add the ttm_tt pages to
> + * the resulting st. Might make sense for GGTT.
> + */
> + GEM_WARN_ON(!cpu_maps_iomem(res));
> return intel_region_ttm_node_to_st(obj->mm.region, res->mm_node);
> }
>
> @@ -290,8 +341,6 @@ static int i915_ttm_move(struct ttm_buffer_object *bo, bool evict,
> struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
> struct ttm_resource_manager *dst_man =
> ttm_manager_type(bo->bdev, dst_mem->mem_type);
> - struct ttm_resource_manager *src_man =
> - ttm_manager_type(bo->bdev, bo->mem.mem_type);
> struct intel_memory_region *dst_reg, *src_reg;
> union {
> struct ttm_kmap_iter_tt tt;
> @@ -332,34 +381,36 @@ static int i915_ttm_move(struct ttm_buffer_object *bo, bool evict,
> if (IS_ERR(dst_st))
> return PTR_ERR(dst_st);
>
> - /* If we start mapping GGTT, we can no longer use man::use_tt here. */
> - dst_iter = dst_man->use_tt ?
> + dst_iter = !cpu_maps_iomem(dst_mem) ?
> ttm_kmap_iter_tt_init(&_dst_iter.tt, bo->ttm) :
> ttm_kmap_iter_iomap_init(&_dst_iter.io, &dst_reg->iomap,
> dst_st, dst_reg->region.start);
>
> - src_iter = src_man->use_tt ?
> + src_iter = !cpu_maps_iomem(&bo->mem) ?
> ttm_kmap_iter_tt_init(&_src_iter.tt, bo->ttm) :
> ttm_kmap_iter_iomap_init(&_src_iter.io, &src_reg->iomap,
> obj->ttm.cached_io_st,
> src_reg->region.start);
>
> ttm_move_memcpy(bo, dst_mem->num_pages, dst_iter, src_iter);
> + /* Below dst_mem becomes bo->mem. */
> ttm_bo_move_sync_cleanup(bo, dst_mem);
> + i915_ttm_adjust_domains_after_cpu_move(obj);
> i915_ttm_free_cached_io_st(obj);
>
> - if (!dst_man->use_tt) {
> + if (gpu_binds_iomem(dst_mem) || cpu_maps_iomem(dst_mem)) {
> obj->ttm.cached_io_st = dst_st;
> obj->ttm.get_io_page.sg_pos = dst_st->sgl;
> obj->ttm.get_io_page.sg_idx = 0;
> }
>
> + i915_ttm_adjust_gem_after_move(obj);
> return 0;
> }
>
> static int i915_ttm_io_mem_reserve(struct ttm_device *bdev, struct ttm_resource *mem)
> {
> - if (mem->mem_type < I915_PL_LMEM0)
> + if (!cpu_maps_iomem(mem))
> return 0;
>
> mem->bus.caching = ttm_write_combined;
> @@ -421,6 +472,16 @@ static int i915_ttm_get_pages(struct drm_i915_gem_object *obj)
> if (ret)
> return ret == -ENOSPC ? -ENXIO : ret;
>
> + i915_ttm_adjust_lru(obj);
> + if (bo->ttm && !ttm_tt_is_populated(bo->ttm)) {
> + ret = ttm_tt_populate(bo->bdev, bo->ttm, &ctx);
> + if (ret)
> + return ret;
> +
> + i915_ttm_adjust_domains_after_cpu_move(obj);
> + i915_ttm_adjust_gem_after_move(obj);
> + }
> +
> /* Object either has a page vector or is an iomem object */
> st = bo->ttm ? i915_ttm_tt_get_st(bo->ttm) : obj->ttm.cached_io_st;
> if (IS_ERR(st))
> @@ -428,8 +489,6 @@ static int i915_ttm_get_pages(struct drm_i915_gem_object *obj)
>
> __i915_gem_object_set_pages(obj, st, i915_sg_dma_sizes(st->sgl));
>
> - i915_ttm_adjust_lru(obj);
> -
> return ret;
> }
>
> @@ -563,6 +622,7 @@ static u64 i915_ttm_mmap_offset(struct drm_i915_gem_object *obj)
>
> const struct drm_i915_gem_object_ops i915_gem_ttm_obj_ops = {
> .name = "i915_gem_object_ttm",
> + .flags = I915_GEM_OBJECT_IS_SHRINKABLE,
>
> .get_pages = i915_ttm_get_pages,
> .put_pages = i915_ttm_put_pages,
> @@ -599,6 +659,10 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
> {
> static struct lock_class_key lock_class;
> struct drm_i915_private *i915 = mem->i915;
> + struct ttm_operation_ctx ctx = {
> + .interruptible = true,
> + .no_wait_gpu = false,
> + };
> enum ttm_bo_type bo_type;
> size_t alignment = 0;
> int ret;
> @@ -618,15 +682,14 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
> i915_gem_object_init(obj, &i915_gem_ttm_obj_ops, &lock_class, flags);
> i915_gem_object_init_memory_region(obj, mem);
> i915_gem_object_make_unshrinkable(obj);
> - obj->read_domains = I915_GEM_DOMAIN_WC | I915_GEM_DOMAIN_GTT;
> - obj->mem_flags |= I915_BO_FLAG_IOMEM;
> - 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);
>
> bo_type = (obj->flags & I915_BO_ALLOC_USER) ? ttm_bo_type_device :
> ttm_bo_type_kernel;
>
> + obj->base.vma_node.driver_private = i915_gem_to_ttm(obj);
> +
> /*
> * If this function fails, it will call the destructor, but
> * our caller still owns the object. So no freeing in the
> @@ -634,14 +697,19 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
> * Similarly, in delayed_destroy, we can't call ttm_bo_put()
> * until successful initialization.
> */
> - obj->base.vma_node.driver_private = i915_gem_to_ttm(obj);
> - ret = ttm_bo_init(&i915->bdev, i915_gem_to_ttm(obj), size,
> - bo_type, &i915_sys_placement, alignment,
> - true, NULL, NULL, i915_ttm_bo_destroy);
> + ret = ttm_bo_init_reserved(&i915->bdev, i915_gem_to_ttm(obj), size,
> + bo_type, &i915_sys_placement, alignment,
> + &ctx, NULL, NULL, i915_ttm_bo_destroy);
> +
> + if (ret)
> + goto out;
>
> - if (!ret)
> - obj->ttm.created = true;
> + obj->ttm.created = true;
> + i915_ttm_adjust_domains_after_cpu_move(obj);
> + i915_ttm_adjust_gem_after_move(obj);
> + i915_gem_object_unlock(obj);
>
> +out:
> /* i915 wants -ENXIO when out of memory region space. */
> return (ret == -ENOSPC) ? -ENXIO : ret;
> }
> --
> 2.31.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Intel-gfx] [PATCH 2/5] drm/i915/ttm: Adjust gem flags and caching settings after a move
2021-06-03 10:35 ` Matthew Auld
@ 2021-06-03 13:40 ` Thomas Hellström
-1 siblings, 0 replies; 27+ messages in thread
From: Thomas Hellström @ 2021-06-03 13:40 UTC (permalink / raw)
To: Matthew Auld; +Cc: Intel Graphics Development, ML dri-devel
Thanks for reviewing, Matthew.
On 6/3/21 12:35 PM, Matthew Auld wrote:
> On Wed, 2 Jun 2021 at 18:08, Thomas Hellström
> <thomas.hellstrom@linux.intel.com> wrote:
>> After a TTM move we need to update the i915 gem flags and caching
>> settings to reflect the new placement.
>> Also introduce gpu_binds_iomem() and cpu_maps_iomem() to clean up the
>> various ways we previously used to detect this.
>> Finally, initialize the TTM object reserved to be able to update
>> flags and caching before anyone else gets hold of the object.
>>
>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 112 +++++++++++++++++++-----
>> 1 file changed, 90 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> index ae12a2be11a2..c73c51755c20 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> @@ -70,6 +70,17 @@ static struct ttm_placement i915_sys_placement = {
>> .busy_placement = &lmem0_sys_placement_flags[1],
>> };
>>
>> +static bool gpu_binds_iomem(struct ttm_resource *mem)
> What does gpu_binds mean here? It's mapped though the GTT?
gpu_bind_XX would here translate to "make xx visible to the gpu", We
could of course use the term "gpu_maps" if you think that fits better.
>
>> +{
>> + return (mem->mem_type != TTM_PL_SYSTEM);
>> +}
>> +
>> +static bool cpu_maps_iomem(struct ttm_resource *mem)
>> +{
>> + /* Once / if we support GGTT, this is also false for cached ttm_tts */
>> + return (mem->mem_type != TTM_PL_SYSTEM);
>> +}
> Can drop the extra brackets.
>
>> +
>> static void i915_ttm_adjust_lru(struct drm_i915_gem_object *obj);
>>
>> static struct ttm_tt *i915_ttm_tt_create(struct ttm_buffer_object *bo,
>> @@ -175,6 +186,41 @@ static void i915_ttm_free_cached_io_st(struct drm_i915_gem_object *obj)
>> obj->ttm.cached_io_st = NULL;
>> }
>>
>> +static void
>> +i915_ttm_adjust_domains_after_cpu_move(struct drm_i915_gem_object *obj)
>> +{
>> + struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
>> +
>> + if (cpu_maps_iomem(&bo->mem) || bo->ttm->caching != ttm_cached) {
>> + obj->write_domain = I915_GEM_DOMAIN_WC;
>> + obj->read_domains = I915_GEM_DOMAIN_WC;
>> + } else {
>> + obj->write_domain = I915_GEM_DOMAIN_CPU;
>> + obj->read_domains = I915_GEM_DOMAIN_CPU;
>> + }
>> +}
>> +
>> +static void i915_ttm_adjust_gem_after_move(struct drm_i915_gem_object *obj)
>> +{
>> + struct drm_i915_private *i915 = to_i915(obj->base.dev);
>> + struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
>> + unsigned int cache_level;
>> +
>> + obj->mem_flags &= ~(I915_BO_FLAG_STRUCT_PAGE | I915_BO_FLAG_IOMEM);
>> +
>> + obj->mem_flags |= cpu_maps_iomem(&bo->mem) ? I915_BO_FLAG_IOMEM :
>> + I915_BO_FLAG_STRUCT_PAGE;
>> +
>> + if ((HAS_LLC(i915) || HAS_SNOOP(i915)) && !gpu_binds_iomem(&bo->mem) &&
> I think all modern hw has support for snooping or the fast shared LLC.
> Are we expecting to hit this path for non-dgpu?
Not initially, no. Possibly later.
> Also by default we
> will choose ttm_cached at least for platforms like dg1?
Not for evicted LMEM. They still have ttm_write_combine. User-space can
still access those through mmap() while evicted.
> Also pin_map()
> is still a separate interface at this point?
Yes, I had a comment on that in your caching patch that I reviewed, we
need to make sure even for pin_map that we only map using the single
supported caching attributes for the BO.
>> + bo->ttm->caching == ttm_cached) {
>> + cache_level = I915_CACHE_LLC;
>> + } else {
>> + cache_level = I915_CACHE_NONE;
>> + }
>> +
>> + i915_gem_object_set_cache_coherency(obj, cache_level);
>> +}
>> +
>> static void i915_ttm_purge(struct drm_i915_gem_object *obj)
>> {
>> struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
>> @@ -190,8 +236,10 @@ static void i915_ttm_purge(struct drm_i915_gem_object *obj)
>>
>> /* TTM's purge interface. Note that we might be reentering. */
>> ret = ttm_bo_validate(bo, &place, &ctx);
>> -
>> if (!ret) {
>> + obj->write_domain = 0;
>> + obj->read_domains = 0;
>> + i915_ttm_adjust_gem_after_move(obj);
>> i915_ttm_free_cached_io_st(obj);
>> obj->mm.madv = __I915_MADV_PURGED;
>> }
>> @@ -273,12 +321,15 @@ i915_ttm_resource_get_st(struct drm_i915_gem_object *obj,
>> struct ttm_resource *res)
>> {
>> struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
>> - struct ttm_resource_manager *man =
>> - ttm_manager_type(bo->bdev, res->mem_type);
>>
>> - if (man->use_tt)
>> + if (!gpu_binds_iomem(res))
>> return i915_ttm_tt_get_st(bo->ttm);
>>
>> + /*
>> + * If CPU mapping differs, we need to add the ttm_tt pages to
>> + * the resulting st. Might make sense for GGTT.
>> + */
>> + GEM_WARN_ON(!cpu_maps_iomem(res));
>> return intel_region_ttm_node_to_st(obj->mm.region, res->mm_node);
>> }
>>
>> @@ -290,8 +341,6 @@ static int i915_ttm_move(struct ttm_buffer_object *bo, bool evict,
>> struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
>> struct ttm_resource_manager *dst_man =
>> ttm_manager_type(bo->bdev, dst_mem->mem_type);
>> - struct ttm_resource_manager *src_man =
>> - ttm_manager_type(bo->bdev, bo->mem.mem_type);
>> struct intel_memory_region *dst_reg, *src_reg;
>> union {
>> struct ttm_kmap_iter_tt tt;
>> @@ -332,34 +381,36 @@ static int i915_ttm_move(struct ttm_buffer_object *bo, bool evict,
>> if (IS_ERR(dst_st))
>> return PTR_ERR(dst_st);
>>
>> - /* If we start mapping GGTT, we can no longer use man::use_tt here. */
>> - dst_iter = dst_man->use_tt ?
>> + dst_iter = !cpu_maps_iomem(dst_mem) ?
>> ttm_kmap_iter_tt_init(&_dst_iter.tt, bo->ttm) :
>> ttm_kmap_iter_iomap_init(&_dst_iter.io, &dst_reg->iomap,
>> dst_st, dst_reg->region.start);
>>
>> - src_iter = src_man->use_tt ?
>> + src_iter = !cpu_maps_iomem(&bo->mem) ?
>> ttm_kmap_iter_tt_init(&_src_iter.tt, bo->ttm) :
>> ttm_kmap_iter_iomap_init(&_src_iter.io, &src_reg->iomap,
>> obj->ttm.cached_io_st,
>> src_reg->region.start);
>>
>> ttm_move_memcpy(bo, dst_mem->num_pages, dst_iter, src_iter);
>> + /* Below dst_mem becomes bo->mem. */
>> ttm_bo_move_sync_cleanup(bo, dst_mem);
>> + i915_ttm_adjust_domains_after_cpu_move(obj);
>> i915_ttm_free_cached_io_st(obj);
>>
>> - if (!dst_man->use_tt) {
>> + if (gpu_binds_iomem(dst_mem) || cpu_maps_iomem(dst_mem)) {
>> obj->ttm.cached_io_st = dst_st;
>> obj->ttm.get_io_page.sg_pos = dst_st->sgl;
>> obj->ttm.get_io_page.sg_idx = 0;
>> }
>>
>> + i915_ttm_adjust_gem_after_move(obj);
>> return 0;
>> }
>>
>> static int i915_ttm_io_mem_reserve(struct ttm_device *bdev, struct ttm_resource *mem)
>> {
>> - if (mem->mem_type < I915_PL_LMEM0)
>> + if (!cpu_maps_iomem(mem))
>> return 0;
>>
>> mem->bus.caching = ttm_write_combined;
>> @@ -421,6 +472,16 @@ static int i915_ttm_get_pages(struct drm_i915_gem_object *obj)
>> if (ret)
>> return ret == -ENOSPC ? -ENXIO : ret;
>>
>> + i915_ttm_adjust_lru(obj);
>> + if (bo->ttm && !ttm_tt_is_populated(bo->ttm)) {
>> + ret = ttm_tt_populate(bo->bdev, bo->ttm, &ctx);
>> + if (ret)
>> + return ret;
>> +
>> + i915_ttm_adjust_domains_after_cpu_move(obj);
>> + i915_ttm_adjust_gem_after_move(obj);
>> + }
>> +
>> /* Object either has a page vector or is an iomem object */
>> st = bo->ttm ? i915_ttm_tt_get_st(bo->ttm) : obj->ttm.cached_io_st;
>> if (IS_ERR(st))
>> @@ -428,8 +489,6 @@ static int i915_ttm_get_pages(struct drm_i915_gem_object *obj)
>>
>> __i915_gem_object_set_pages(obj, st, i915_sg_dma_sizes(st->sgl));
>>
>> - i915_ttm_adjust_lru(obj);
>> -
>> return ret;
>> }
>>
>> @@ -563,6 +622,7 @@ static u64 i915_ttm_mmap_offset(struct drm_i915_gem_object *obj)
>>
>> const struct drm_i915_gem_object_ops i915_gem_ttm_obj_ops = {
>> .name = "i915_gem_object_ttm",
>> + .flags = I915_GEM_OBJECT_IS_SHRINKABLE,
>>
>> .get_pages = i915_ttm_get_pages,
>> .put_pages = i915_ttm_put_pages,
>> @@ -599,6 +659,10 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
>> {
>> static struct lock_class_key lock_class;
>> struct drm_i915_private *i915 = mem->i915;
>> + struct ttm_operation_ctx ctx = {
>> + .interruptible = true,
>> + .no_wait_gpu = false,
>> + };
>> enum ttm_bo_type bo_type;
>> size_t alignment = 0;
>> int ret;
>> @@ -618,15 +682,14 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
>> i915_gem_object_init(obj, &i915_gem_ttm_obj_ops, &lock_class, flags);
>> i915_gem_object_init_memory_region(obj, mem);
>> i915_gem_object_make_unshrinkable(obj);
>> - obj->read_domains = I915_GEM_DOMAIN_WC | I915_GEM_DOMAIN_GTT;
>> - obj->mem_flags |= I915_BO_FLAG_IOMEM;
>> - 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);
>>
>> bo_type = (obj->flags & I915_BO_ALLOC_USER) ? ttm_bo_type_device :
>> ttm_bo_type_kernel;
>>
>> + obj->base.vma_node.driver_private = i915_gem_to_ttm(obj);
>> +
>> /*
>> * If this function fails, it will call the destructor, but
>> * our caller still owns the object. So no freeing in the
>> @@ -634,14 +697,19 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
>> * Similarly, in delayed_destroy, we can't call ttm_bo_put()
>> * until successful initialization.
>> */
>> - obj->base.vma_node.driver_private = i915_gem_to_ttm(obj);
>> - ret = ttm_bo_init(&i915->bdev, i915_gem_to_ttm(obj), size,
>> - bo_type, &i915_sys_placement, alignment,
>> - true, NULL, NULL, i915_ttm_bo_destroy);
>> + ret = ttm_bo_init_reserved(&i915->bdev, i915_gem_to_ttm(obj), size,
>> + bo_type, &i915_sys_placement, alignment,
>> + &ctx, NULL, NULL, i915_ttm_bo_destroy);
>> +
>> + if (ret)
>> + goto out;
>>
>> - if (!ret)
>> - obj->ttm.created = true;
>> + obj->ttm.created = true;
>> + i915_ttm_adjust_domains_after_cpu_move(obj);
>> + i915_ttm_adjust_gem_after_move(obj);
>> + i915_gem_object_unlock(obj);
>>
>> +out:
>> /* i915 wants -ENXIO when out of memory region space. */
>> return (ret == -ENOSPC) ? -ENXIO : ret;
>> }
>> --
>> 2.31.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Intel-gfx] [PATCH 2/5] drm/i915/ttm: Adjust gem flags and caching settings after a move
@ 2021-06-03 13:40 ` Thomas Hellström
0 siblings, 0 replies; 27+ messages in thread
From: Thomas Hellström @ 2021-06-03 13:40 UTC (permalink / raw)
To: Matthew Auld; +Cc: Intel Graphics Development, ML dri-devel
Thanks for reviewing, Matthew.
On 6/3/21 12:35 PM, Matthew Auld wrote:
> On Wed, 2 Jun 2021 at 18:08, Thomas Hellström
> <thomas.hellstrom@linux.intel.com> wrote:
>> After a TTM move we need to update the i915 gem flags and caching
>> settings to reflect the new placement.
>> Also introduce gpu_binds_iomem() and cpu_maps_iomem() to clean up the
>> various ways we previously used to detect this.
>> Finally, initialize the TTM object reserved to be able to update
>> flags and caching before anyone else gets hold of the object.
>>
>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 112 +++++++++++++++++++-----
>> 1 file changed, 90 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> index ae12a2be11a2..c73c51755c20 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> @@ -70,6 +70,17 @@ static struct ttm_placement i915_sys_placement = {
>> .busy_placement = &lmem0_sys_placement_flags[1],
>> };
>>
>> +static bool gpu_binds_iomem(struct ttm_resource *mem)
> What does gpu_binds mean here? It's mapped though the GTT?
gpu_bind_XX would here translate to "make xx visible to the gpu", We
could of course use the term "gpu_maps" if you think that fits better.
>
>> +{
>> + return (mem->mem_type != TTM_PL_SYSTEM);
>> +}
>> +
>> +static bool cpu_maps_iomem(struct ttm_resource *mem)
>> +{
>> + /* Once / if we support GGTT, this is also false for cached ttm_tts */
>> + return (mem->mem_type != TTM_PL_SYSTEM);
>> +}
> Can drop the extra brackets.
>
>> +
>> static void i915_ttm_adjust_lru(struct drm_i915_gem_object *obj);
>>
>> static struct ttm_tt *i915_ttm_tt_create(struct ttm_buffer_object *bo,
>> @@ -175,6 +186,41 @@ static void i915_ttm_free_cached_io_st(struct drm_i915_gem_object *obj)
>> obj->ttm.cached_io_st = NULL;
>> }
>>
>> +static void
>> +i915_ttm_adjust_domains_after_cpu_move(struct drm_i915_gem_object *obj)
>> +{
>> + struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
>> +
>> + if (cpu_maps_iomem(&bo->mem) || bo->ttm->caching != ttm_cached) {
>> + obj->write_domain = I915_GEM_DOMAIN_WC;
>> + obj->read_domains = I915_GEM_DOMAIN_WC;
>> + } else {
>> + obj->write_domain = I915_GEM_DOMAIN_CPU;
>> + obj->read_domains = I915_GEM_DOMAIN_CPU;
>> + }
>> +}
>> +
>> +static void i915_ttm_adjust_gem_after_move(struct drm_i915_gem_object *obj)
>> +{
>> + struct drm_i915_private *i915 = to_i915(obj->base.dev);
>> + struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
>> + unsigned int cache_level;
>> +
>> + obj->mem_flags &= ~(I915_BO_FLAG_STRUCT_PAGE | I915_BO_FLAG_IOMEM);
>> +
>> + obj->mem_flags |= cpu_maps_iomem(&bo->mem) ? I915_BO_FLAG_IOMEM :
>> + I915_BO_FLAG_STRUCT_PAGE;
>> +
>> + if ((HAS_LLC(i915) || HAS_SNOOP(i915)) && !gpu_binds_iomem(&bo->mem) &&
> I think all modern hw has support for snooping or the fast shared LLC.
> Are we expecting to hit this path for non-dgpu?
Not initially, no. Possibly later.
> Also by default we
> will choose ttm_cached at least for platforms like dg1?
Not for evicted LMEM. They still have ttm_write_combine. User-space can
still access those through mmap() while evicted.
> Also pin_map()
> is still a separate interface at this point?
Yes, I had a comment on that in your caching patch that I reviewed, we
need to make sure even for pin_map that we only map using the single
supported caching attributes for the BO.
>> + bo->ttm->caching == ttm_cached) {
>> + cache_level = I915_CACHE_LLC;
>> + } else {
>> + cache_level = I915_CACHE_NONE;
>> + }
>> +
>> + i915_gem_object_set_cache_coherency(obj, cache_level);
>> +}
>> +
>> static void i915_ttm_purge(struct drm_i915_gem_object *obj)
>> {
>> struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
>> @@ -190,8 +236,10 @@ static void i915_ttm_purge(struct drm_i915_gem_object *obj)
>>
>> /* TTM's purge interface. Note that we might be reentering. */
>> ret = ttm_bo_validate(bo, &place, &ctx);
>> -
>> if (!ret) {
>> + obj->write_domain = 0;
>> + obj->read_domains = 0;
>> + i915_ttm_adjust_gem_after_move(obj);
>> i915_ttm_free_cached_io_st(obj);
>> obj->mm.madv = __I915_MADV_PURGED;
>> }
>> @@ -273,12 +321,15 @@ i915_ttm_resource_get_st(struct drm_i915_gem_object *obj,
>> struct ttm_resource *res)
>> {
>> struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
>> - struct ttm_resource_manager *man =
>> - ttm_manager_type(bo->bdev, res->mem_type);
>>
>> - if (man->use_tt)
>> + if (!gpu_binds_iomem(res))
>> return i915_ttm_tt_get_st(bo->ttm);
>>
>> + /*
>> + * If CPU mapping differs, we need to add the ttm_tt pages to
>> + * the resulting st. Might make sense for GGTT.
>> + */
>> + GEM_WARN_ON(!cpu_maps_iomem(res));
>> return intel_region_ttm_node_to_st(obj->mm.region, res->mm_node);
>> }
>>
>> @@ -290,8 +341,6 @@ static int i915_ttm_move(struct ttm_buffer_object *bo, bool evict,
>> struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
>> struct ttm_resource_manager *dst_man =
>> ttm_manager_type(bo->bdev, dst_mem->mem_type);
>> - struct ttm_resource_manager *src_man =
>> - ttm_manager_type(bo->bdev, bo->mem.mem_type);
>> struct intel_memory_region *dst_reg, *src_reg;
>> union {
>> struct ttm_kmap_iter_tt tt;
>> @@ -332,34 +381,36 @@ static int i915_ttm_move(struct ttm_buffer_object *bo, bool evict,
>> if (IS_ERR(dst_st))
>> return PTR_ERR(dst_st);
>>
>> - /* If we start mapping GGTT, we can no longer use man::use_tt here. */
>> - dst_iter = dst_man->use_tt ?
>> + dst_iter = !cpu_maps_iomem(dst_mem) ?
>> ttm_kmap_iter_tt_init(&_dst_iter.tt, bo->ttm) :
>> ttm_kmap_iter_iomap_init(&_dst_iter.io, &dst_reg->iomap,
>> dst_st, dst_reg->region.start);
>>
>> - src_iter = src_man->use_tt ?
>> + src_iter = !cpu_maps_iomem(&bo->mem) ?
>> ttm_kmap_iter_tt_init(&_src_iter.tt, bo->ttm) :
>> ttm_kmap_iter_iomap_init(&_src_iter.io, &src_reg->iomap,
>> obj->ttm.cached_io_st,
>> src_reg->region.start);
>>
>> ttm_move_memcpy(bo, dst_mem->num_pages, dst_iter, src_iter);
>> + /* Below dst_mem becomes bo->mem. */
>> ttm_bo_move_sync_cleanup(bo, dst_mem);
>> + i915_ttm_adjust_domains_after_cpu_move(obj);
>> i915_ttm_free_cached_io_st(obj);
>>
>> - if (!dst_man->use_tt) {
>> + if (gpu_binds_iomem(dst_mem) || cpu_maps_iomem(dst_mem)) {
>> obj->ttm.cached_io_st = dst_st;
>> obj->ttm.get_io_page.sg_pos = dst_st->sgl;
>> obj->ttm.get_io_page.sg_idx = 0;
>> }
>>
>> + i915_ttm_adjust_gem_after_move(obj);
>> return 0;
>> }
>>
>> static int i915_ttm_io_mem_reserve(struct ttm_device *bdev, struct ttm_resource *mem)
>> {
>> - if (mem->mem_type < I915_PL_LMEM0)
>> + if (!cpu_maps_iomem(mem))
>> return 0;
>>
>> mem->bus.caching = ttm_write_combined;
>> @@ -421,6 +472,16 @@ static int i915_ttm_get_pages(struct drm_i915_gem_object *obj)
>> if (ret)
>> return ret == -ENOSPC ? -ENXIO : ret;
>>
>> + i915_ttm_adjust_lru(obj);
>> + if (bo->ttm && !ttm_tt_is_populated(bo->ttm)) {
>> + ret = ttm_tt_populate(bo->bdev, bo->ttm, &ctx);
>> + if (ret)
>> + return ret;
>> +
>> + i915_ttm_adjust_domains_after_cpu_move(obj);
>> + i915_ttm_adjust_gem_after_move(obj);
>> + }
>> +
>> /* Object either has a page vector or is an iomem object */
>> st = bo->ttm ? i915_ttm_tt_get_st(bo->ttm) : obj->ttm.cached_io_st;
>> if (IS_ERR(st))
>> @@ -428,8 +489,6 @@ static int i915_ttm_get_pages(struct drm_i915_gem_object *obj)
>>
>> __i915_gem_object_set_pages(obj, st, i915_sg_dma_sizes(st->sgl));
>>
>> - i915_ttm_adjust_lru(obj);
>> -
>> return ret;
>> }
>>
>> @@ -563,6 +622,7 @@ static u64 i915_ttm_mmap_offset(struct drm_i915_gem_object *obj)
>>
>> const struct drm_i915_gem_object_ops i915_gem_ttm_obj_ops = {
>> .name = "i915_gem_object_ttm",
>> + .flags = I915_GEM_OBJECT_IS_SHRINKABLE,
>>
>> .get_pages = i915_ttm_get_pages,
>> .put_pages = i915_ttm_put_pages,
>> @@ -599,6 +659,10 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
>> {
>> static struct lock_class_key lock_class;
>> struct drm_i915_private *i915 = mem->i915;
>> + struct ttm_operation_ctx ctx = {
>> + .interruptible = true,
>> + .no_wait_gpu = false,
>> + };
>> enum ttm_bo_type bo_type;
>> size_t alignment = 0;
>> int ret;
>> @@ -618,15 +682,14 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
>> i915_gem_object_init(obj, &i915_gem_ttm_obj_ops, &lock_class, flags);
>> i915_gem_object_init_memory_region(obj, mem);
>> i915_gem_object_make_unshrinkable(obj);
>> - obj->read_domains = I915_GEM_DOMAIN_WC | I915_GEM_DOMAIN_GTT;
>> - obj->mem_flags |= I915_BO_FLAG_IOMEM;
>> - 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);
>>
>> bo_type = (obj->flags & I915_BO_ALLOC_USER) ? ttm_bo_type_device :
>> ttm_bo_type_kernel;
>>
>> + obj->base.vma_node.driver_private = i915_gem_to_ttm(obj);
>> +
>> /*
>> * If this function fails, it will call the destructor, but
>> * our caller still owns the object. So no freeing in the
>> @@ -634,14 +697,19 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
>> * Similarly, in delayed_destroy, we can't call ttm_bo_put()
>> * until successful initialization.
>> */
>> - obj->base.vma_node.driver_private = i915_gem_to_ttm(obj);
>> - ret = ttm_bo_init(&i915->bdev, i915_gem_to_ttm(obj), size,
>> - bo_type, &i915_sys_placement, alignment,
>> - true, NULL, NULL, i915_ttm_bo_destroy);
>> + ret = ttm_bo_init_reserved(&i915->bdev, i915_gem_to_ttm(obj), size,
>> + bo_type, &i915_sys_placement, alignment,
>> + &ctx, NULL, NULL, i915_ttm_bo_destroy);
>> +
>> + if (ret)
>> + goto out;
>>
>> - if (!ret)
>> - obj->ttm.created = true;
>> + obj->ttm.created = true;
>> + i915_ttm_adjust_domains_after_cpu_move(obj);
>> + i915_ttm_adjust_gem_after_move(obj);
>> + i915_gem_object_unlock(obj);
>>
>> +out:
>> /* i915 wants -ENXIO when out of memory region space. */
>> return (ret == -ENOSPC) ? -ENXIO : ret;
>> }
>> --
>> 2.31.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 3/5] drm/i915/ttm: Calculate the object placement at get_pages time
2021-06-02 17:07 ` [Intel-gfx] " Thomas Hellström
@ 2021-06-02 17:07 ` Thomas Hellström
-1 siblings, 0 replies; 27+ messages in thread
From: Thomas Hellström @ 2021-06-02 17:07 UTC (permalink / raw)
To: intel-gfx, dri-devel; +Cc: Thomas Hellström
Instead of relying on a static placement, calculate at get_pages() time.
This should work for LMEM regions and system for now. For stolen we need
to take preallocated range into account. That well be added later.
Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 92 ++++++++++++++++++-------
drivers/gpu/drm/i915/intel_region_ttm.c | 8 ++-
drivers/gpu/drm/i915/intel_region_ttm.h | 2 +
3 files changed, 75 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index c73c51755c20..8e1c01168c6d 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -24,6 +24,11 @@
#define I915_TTM_PRIO_NO_PAGES 1
#define I915_TTM_PRIO_HAS_PAGES 2
+/*
+ * Size of struct ttm_place vector in on-stack struct ttm_placement allocs
+ */
+#define I915_TTM_MAX_PLACEMENTS 10
+
/**
* struct i915_ttm_tt - TTM page vector with additional private information
* @ttm: The base TTM page vector.
@@ -42,32 +47,18 @@ struct i915_ttm_tt {
struct sg_table *cached_st;
};
-static const struct ttm_place lmem0_sys_placement_flags[] = {
- {
- .fpfn = 0,
- .lpfn = 0,
- .mem_type = I915_PL_LMEM0,
- .flags = 0,
- }, {
- .fpfn = 0,
- .lpfn = 0,
- .mem_type = I915_PL_SYSTEM,
- .flags = 0,
- }
-};
-
-static struct ttm_placement i915_lmem0_placement = {
- .num_placement = 1,
- .placement = &lmem0_sys_placement_flags[0],
- .num_busy_placement = 1,
- .busy_placement = &lmem0_sys_placement_flags[0],
+static const struct ttm_place sys_placement_flags = {
+ .fpfn = 0,
+ .lpfn = 0,
+ .mem_type = I915_PL_SYSTEM,
+ .flags = 0,
};
static struct ttm_placement i915_sys_placement = {
.num_placement = 1,
- .placement = &lmem0_sys_placement_flags[1],
+ .placement = &sys_placement_flags,
.num_busy_placement = 1,
- .busy_placement = &lmem0_sys_placement_flags[1],
+ .busy_placement = &sys_placement_flags,
};
static bool gpu_binds_iomem(struct ttm_resource *mem)
@@ -83,6 +74,55 @@ static bool cpu_maps_iomem(struct ttm_resource *mem)
static void i915_ttm_adjust_lru(struct drm_i915_gem_object *obj);
+static enum ttm_caching
+i915_ttm_select_tt_caching(const struct drm_i915_gem_object *obj)
+{
+ /*
+ * Objects only allowed in system get cached cpu-mappings.
+ * Other objects get WC mapping for now. Even if in system.
+ */
+ if (obj->mm.region->type == INTEL_MEMORY_SYSTEM &&
+ obj->mm.n_placements <= 1)
+ return ttm_cached;
+
+ return ttm_write_combined;
+}
+
+static void
+i915_ttm_place_from_region(const struct intel_memory_region *mr,
+ struct ttm_place *place)
+{
+ memset(place, 0, sizeof(*place));
+ place->mem_type = intel_region_to_ttm_type(mr);
+}
+
+static void
+i915_ttm_placement_from_obj(const struct drm_i915_gem_object *obj,
+ struct ttm_place *requested,
+ struct ttm_place *busy,
+ struct ttm_placement *placement)
+{
+ unsigned int i;
+ unsigned int num_allowed = obj->mm.n_placements;
+
+ placement->num_placement = 1;
+ i915_ttm_place_from_region(num_allowed ? obj->mm.placements[0] :
+ obj->mm.region, requested);
+
+ /* Cache this on object? */
+ placement->num_busy_placement = num_allowed;
+ for (i = 0; i < placement->num_busy_placement; ++i)
+ i915_ttm_place_from_region(obj->mm.placements[i], busy + i);
+
+ if (num_allowed == 0) {
+ *busy = *requested;
+ placement->num_busy_placement = 1;
+ }
+
+ placement->placement = requested;
+ placement->busy_placement = busy;
+}
+
static struct ttm_tt *i915_ttm_tt_create(struct ttm_buffer_object *bo,
uint32_t page_flags)
{
@@ -100,7 +140,8 @@ static struct ttm_tt *i915_ttm_tt_create(struct ttm_buffer_object *bo,
man->use_tt)
page_flags |= TTM_PAGE_FLAG_ZERO_ALLOC;
- ret = ttm_tt_init(&i915_tt->ttm, bo, page_flags, ttm_write_combined);
+ ret = ttm_tt_init(&i915_tt->ttm, bo, page_flags,
+ i915_ttm_select_tt_caching(obj));
if (ret) {
kfree(i915_tt);
return NULL;
@@ -465,10 +506,13 @@ static int i915_ttm_get_pages(struct drm_i915_gem_object *obj)
.no_wait_gpu = false,
};
struct sg_table *st;
+ struct ttm_place requested, busy[I915_TTM_MAX_PLACEMENTS];
+ struct ttm_placement placement;
int ret;
/* Move to the requested placement. */
- ret = ttm_bo_validate(bo, &i915_lmem0_placement, &ctx);
+ i915_ttm_placement_from_obj(obj, &requested, busy, &placement);
+ ret = ttm_bo_validate(bo, &placement, &ctx);
if (ret)
return ret == -ENOSPC ? -ENXIO : ret;
@@ -684,7 +728,6 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
i915_gem_object_make_unshrinkable(obj);
INIT_RADIX_TREE(&obj->ttm.get_io_page.radix, GFP_KERNEL | __GFP_NOWARN);
mutex_init(&obj->ttm.get_io_page.lock);
-
bo_type = (obj->flags & I915_BO_ALLOC_USER) ? ttm_bo_type_device :
ttm_bo_type_kernel;
@@ -708,7 +751,6 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
i915_ttm_adjust_domains_after_cpu_move(obj);
i915_ttm_adjust_gem_after_move(obj);
i915_gem_object_unlock(obj);
-
out:
/* i915 wants -ENXIO when out of memory region space. */
return (ret == -ENOSPC) ? -ENXIO : ret;
diff --git a/drivers/gpu/drm/i915/intel_region_ttm.c b/drivers/gpu/drm/i915/intel_region_ttm.c
index 0b41a1545570..bc58ea942ef9 100644
--- a/drivers/gpu/drm/i915/intel_region_ttm.c
+++ b/drivers/gpu/drm/i915/intel_region_ttm.c
@@ -49,12 +49,16 @@ void intel_region_ttm_device_fini(struct drm_i915_private *dev_priv)
* driver-private types for now, reserving TTM_PL_VRAM for stolen
* memory and TTM_PL_TT for GGTT use if decided to implement this.
*/
-static int intel_region_to_ttm_type(struct intel_memory_region *mem)
+int intel_region_to_ttm_type(const struct intel_memory_region *mem)
{
int type;
GEM_BUG_ON(mem->type != INTEL_MEMORY_LOCAL &&
- mem->type != INTEL_MEMORY_MOCK);
+ mem->type != INTEL_MEMORY_MOCK &&
+ mem->type != INTEL_MEMORY_SYSTEM);
+
+ if (mem->type == INTEL_MEMORY_SYSTEM)
+ return TTM_PL_SYSTEM;
type = mem->instance + TTM_PL_PRIV;
GEM_BUG_ON(type >= TTM_NUM_MEM_TYPES);
diff --git a/drivers/gpu/drm/i915/intel_region_ttm.h b/drivers/gpu/drm/i915/intel_region_ttm.h
index eaa3eccfa252..88960ae6cff6 100644
--- a/drivers/gpu/drm/i915/intel_region_ttm.h
+++ b/drivers/gpu/drm/i915/intel_region_ttm.h
@@ -27,6 +27,8 @@ struct sg_table *intel_region_ttm_node_to_st(struct intel_memory_region *mem,
void intel_region_ttm_node_free(struct intel_memory_region *mem,
void *node);
+int intel_region_to_ttm_type(const struct intel_memory_region *mem);
+
struct ttm_device_funcs *i915_ttm_driver(void);
#ifdef CONFIG_DRM_I915_SELFTEST
--
2.31.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [Intel-gfx] [PATCH 3/5] drm/i915/ttm: Calculate the object placement at get_pages time
@ 2021-06-02 17:07 ` Thomas Hellström
0 siblings, 0 replies; 27+ messages in thread
From: Thomas Hellström @ 2021-06-02 17:07 UTC (permalink / raw)
To: intel-gfx, dri-devel; +Cc: Thomas Hellström
Instead of relying on a static placement, calculate at get_pages() time.
This should work for LMEM regions and system for now. For stolen we need
to take preallocated range into account. That well be added later.
Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 92 ++++++++++++++++++-------
drivers/gpu/drm/i915/intel_region_ttm.c | 8 ++-
drivers/gpu/drm/i915/intel_region_ttm.h | 2 +
3 files changed, 75 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index c73c51755c20..8e1c01168c6d 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -24,6 +24,11 @@
#define I915_TTM_PRIO_NO_PAGES 1
#define I915_TTM_PRIO_HAS_PAGES 2
+/*
+ * Size of struct ttm_place vector in on-stack struct ttm_placement allocs
+ */
+#define I915_TTM_MAX_PLACEMENTS 10
+
/**
* struct i915_ttm_tt - TTM page vector with additional private information
* @ttm: The base TTM page vector.
@@ -42,32 +47,18 @@ struct i915_ttm_tt {
struct sg_table *cached_st;
};
-static const struct ttm_place lmem0_sys_placement_flags[] = {
- {
- .fpfn = 0,
- .lpfn = 0,
- .mem_type = I915_PL_LMEM0,
- .flags = 0,
- }, {
- .fpfn = 0,
- .lpfn = 0,
- .mem_type = I915_PL_SYSTEM,
- .flags = 0,
- }
-};
-
-static struct ttm_placement i915_lmem0_placement = {
- .num_placement = 1,
- .placement = &lmem0_sys_placement_flags[0],
- .num_busy_placement = 1,
- .busy_placement = &lmem0_sys_placement_flags[0],
+static const struct ttm_place sys_placement_flags = {
+ .fpfn = 0,
+ .lpfn = 0,
+ .mem_type = I915_PL_SYSTEM,
+ .flags = 0,
};
static struct ttm_placement i915_sys_placement = {
.num_placement = 1,
- .placement = &lmem0_sys_placement_flags[1],
+ .placement = &sys_placement_flags,
.num_busy_placement = 1,
- .busy_placement = &lmem0_sys_placement_flags[1],
+ .busy_placement = &sys_placement_flags,
};
static bool gpu_binds_iomem(struct ttm_resource *mem)
@@ -83,6 +74,55 @@ static bool cpu_maps_iomem(struct ttm_resource *mem)
static void i915_ttm_adjust_lru(struct drm_i915_gem_object *obj);
+static enum ttm_caching
+i915_ttm_select_tt_caching(const struct drm_i915_gem_object *obj)
+{
+ /*
+ * Objects only allowed in system get cached cpu-mappings.
+ * Other objects get WC mapping for now. Even if in system.
+ */
+ if (obj->mm.region->type == INTEL_MEMORY_SYSTEM &&
+ obj->mm.n_placements <= 1)
+ return ttm_cached;
+
+ return ttm_write_combined;
+}
+
+static void
+i915_ttm_place_from_region(const struct intel_memory_region *mr,
+ struct ttm_place *place)
+{
+ memset(place, 0, sizeof(*place));
+ place->mem_type = intel_region_to_ttm_type(mr);
+}
+
+static void
+i915_ttm_placement_from_obj(const struct drm_i915_gem_object *obj,
+ struct ttm_place *requested,
+ struct ttm_place *busy,
+ struct ttm_placement *placement)
+{
+ unsigned int i;
+ unsigned int num_allowed = obj->mm.n_placements;
+
+ placement->num_placement = 1;
+ i915_ttm_place_from_region(num_allowed ? obj->mm.placements[0] :
+ obj->mm.region, requested);
+
+ /* Cache this on object? */
+ placement->num_busy_placement = num_allowed;
+ for (i = 0; i < placement->num_busy_placement; ++i)
+ i915_ttm_place_from_region(obj->mm.placements[i], busy + i);
+
+ if (num_allowed == 0) {
+ *busy = *requested;
+ placement->num_busy_placement = 1;
+ }
+
+ placement->placement = requested;
+ placement->busy_placement = busy;
+}
+
static struct ttm_tt *i915_ttm_tt_create(struct ttm_buffer_object *bo,
uint32_t page_flags)
{
@@ -100,7 +140,8 @@ static struct ttm_tt *i915_ttm_tt_create(struct ttm_buffer_object *bo,
man->use_tt)
page_flags |= TTM_PAGE_FLAG_ZERO_ALLOC;
- ret = ttm_tt_init(&i915_tt->ttm, bo, page_flags, ttm_write_combined);
+ ret = ttm_tt_init(&i915_tt->ttm, bo, page_flags,
+ i915_ttm_select_tt_caching(obj));
if (ret) {
kfree(i915_tt);
return NULL;
@@ -465,10 +506,13 @@ static int i915_ttm_get_pages(struct drm_i915_gem_object *obj)
.no_wait_gpu = false,
};
struct sg_table *st;
+ struct ttm_place requested, busy[I915_TTM_MAX_PLACEMENTS];
+ struct ttm_placement placement;
int ret;
/* Move to the requested placement. */
- ret = ttm_bo_validate(bo, &i915_lmem0_placement, &ctx);
+ i915_ttm_placement_from_obj(obj, &requested, busy, &placement);
+ ret = ttm_bo_validate(bo, &placement, &ctx);
if (ret)
return ret == -ENOSPC ? -ENXIO : ret;
@@ -684,7 +728,6 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
i915_gem_object_make_unshrinkable(obj);
INIT_RADIX_TREE(&obj->ttm.get_io_page.radix, GFP_KERNEL | __GFP_NOWARN);
mutex_init(&obj->ttm.get_io_page.lock);
-
bo_type = (obj->flags & I915_BO_ALLOC_USER) ? ttm_bo_type_device :
ttm_bo_type_kernel;
@@ -708,7 +751,6 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
i915_ttm_adjust_domains_after_cpu_move(obj);
i915_ttm_adjust_gem_after_move(obj);
i915_gem_object_unlock(obj);
-
out:
/* i915 wants -ENXIO when out of memory region space. */
return (ret == -ENOSPC) ? -ENXIO : ret;
diff --git a/drivers/gpu/drm/i915/intel_region_ttm.c b/drivers/gpu/drm/i915/intel_region_ttm.c
index 0b41a1545570..bc58ea942ef9 100644
--- a/drivers/gpu/drm/i915/intel_region_ttm.c
+++ b/drivers/gpu/drm/i915/intel_region_ttm.c
@@ -49,12 +49,16 @@ void intel_region_ttm_device_fini(struct drm_i915_private *dev_priv)
* driver-private types for now, reserving TTM_PL_VRAM for stolen
* memory and TTM_PL_TT for GGTT use if decided to implement this.
*/
-static int intel_region_to_ttm_type(struct intel_memory_region *mem)
+int intel_region_to_ttm_type(const struct intel_memory_region *mem)
{
int type;
GEM_BUG_ON(mem->type != INTEL_MEMORY_LOCAL &&
- mem->type != INTEL_MEMORY_MOCK);
+ mem->type != INTEL_MEMORY_MOCK &&
+ mem->type != INTEL_MEMORY_SYSTEM);
+
+ if (mem->type == INTEL_MEMORY_SYSTEM)
+ return TTM_PL_SYSTEM;
type = mem->instance + TTM_PL_PRIV;
GEM_BUG_ON(type >= TTM_NUM_MEM_TYPES);
diff --git a/drivers/gpu/drm/i915/intel_region_ttm.h b/drivers/gpu/drm/i915/intel_region_ttm.h
index eaa3eccfa252..88960ae6cff6 100644
--- a/drivers/gpu/drm/i915/intel_region_ttm.h
+++ b/drivers/gpu/drm/i915/intel_region_ttm.h
@@ -27,6 +27,8 @@ struct sg_table *intel_region_ttm_node_to_st(struct intel_memory_region *mem,
void intel_region_ttm_node_free(struct intel_memory_region *mem,
void *node);
+int intel_region_to_ttm_type(const struct intel_memory_region *mem);
+
struct ttm_device_funcs *i915_ttm_driver(void);
#ifdef CONFIG_DRM_I915_SELFTEST
--
2.31.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [Intel-gfx] [PATCH 3/5] drm/i915/ttm: Calculate the object placement at get_pages time
2021-06-02 17:07 ` [Intel-gfx] " Thomas Hellström
@ 2021-06-03 10:58 ` Matthew Auld
-1 siblings, 0 replies; 27+ messages in thread
From: Matthew Auld @ 2021-06-03 10:58 UTC (permalink / raw)
To: Thomas Hellström; +Cc: Intel Graphics Development, ML dri-devel
On Wed, 2 Jun 2021 at 18:08, Thomas Hellström
<thomas.hellstrom@linux.intel.com> wrote:
>
> Instead of relying on a static placement, calculate at get_pages() time.
> This should work for LMEM regions and system for now. For stolen we need
> to take preallocated range into account. That well be added later.
>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Can we split this patch out and merge it? We can use this for feeding
in the per BO flags.
> ---
> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 92 ++++++++++++++++++-------
> drivers/gpu/drm/i915/intel_region_ttm.c | 8 ++-
> drivers/gpu/drm/i915/intel_region_ttm.h | 2 +
> 3 files changed, 75 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index c73c51755c20..8e1c01168c6d 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -24,6 +24,11 @@
> #define I915_TTM_PRIO_NO_PAGES 1
> #define I915_TTM_PRIO_HAS_PAGES 2
>
> +/*
> + * Size of struct ttm_place vector in on-stack struct ttm_placement allocs
> + */
> +#define I915_TTM_MAX_PLACEMENTS 10
> +
> /**
> * struct i915_ttm_tt - TTM page vector with additional private information
> * @ttm: The base TTM page vector.
> @@ -42,32 +47,18 @@ struct i915_ttm_tt {
> struct sg_table *cached_st;
> };
>
> -static const struct ttm_place lmem0_sys_placement_flags[] = {
> - {
> - .fpfn = 0,
> - .lpfn = 0,
> - .mem_type = I915_PL_LMEM0,
> - .flags = 0,
> - }, {
> - .fpfn = 0,
> - .lpfn = 0,
> - .mem_type = I915_PL_SYSTEM,
> - .flags = 0,
> - }
> -};
> -
> -static struct ttm_placement i915_lmem0_placement = {
> - .num_placement = 1,
> - .placement = &lmem0_sys_placement_flags[0],
> - .num_busy_placement = 1,
> - .busy_placement = &lmem0_sys_placement_flags[0],
> +static const struct ttm_place sys_placement_flags = {
> + .fpfn = 0,
> + .lpfn = 0,
> + .mem_type = I915_PL_SYSTEM,
> + .flags = 0,
> };
>
> static struct ttm_placement i915_sys_placement = {
> .num_placement = 1,
> - .placement = &lmem0_sys_placement_flags[1],
> + .placement = &sys_placement_flags,
> .num_busy_placement = 1,
> - .busy_placement = &lmem0_sys_placement_flags[1],
> + .busy_placement = &sys_placement_flags,
> };
>
> static bool gpu_binds_iomem(struct ttm_resource *mem)
> @@ -83,6 +74,55 @@ static bool cpu_maps_iomem(struct ttm_resource *mem)
>
> static void i915_ttm_adjust_lru(struct drm_i915_gem_object *obj);
>
> +static enum ttm_caching
> +i915_ttm_select_tt_caching(const struct drm_i915_gem_object *obj)
> +{
> + /*
> + * Objects only allowed in system get cached cpu-mappings.
> + * Other objects get WC mapping for now. Even if in system.
> + */
> + if (obj->mm.region->type == INTEL_MEMORY_SYSTEM &&
> + obj->mm.n_placements <= 1)
> + return ttm_cached;
> +
> + return ttm_write_combined;
> +}
> +
> +static void
> +i915_ttm_place_from_region(const struct intel_memory_region *mr,
> + struct ttm_place *place)
> +{
> + memset(place, 0, sizeof(*place));
> + place->mem_type = intel_region_to_ttm_type(mr);
> +}
> +
> +static void
> +i915_ttm_placement_from_obj(const struct drm_i915_gem_object *obj,
> + struct ttm_place *requested,
> + struct ttm_place *busy,
> + struct ttm_placement *placement)
> +{
> + unsigned int i;
> + unsigned int num_allowed = obj->mm.n_placements;
Style nit: Christmas tree
> +
> + placement->num_placement = 1;
> + i915_ttm_place_from_region(num_allowed ? obj->mm.placements[0] :
> + obj->mm.region, requested);
> +
> + /* Cache this on object? */
> + placement->num_busy_placement = num_allowed;
> + for (i = 0; i < placement->num_busy_placement; ++i)
> + i915_ttm_place_from_region(obj->mm.placements[i], busy + i);
> +
> + if (num_allowed == 0) {
> + *busy = *requested;
> + placement->num_busy_placement = 1;
> + }
> +
> + placement->placement = requested;
> + placement->busy_placement = busy;
> +}
> +
> static struct ttm_tt *i915_ttm_tt_create(struct ttm_buffer_object *bo,
> uint32_t page_flags)
> {
> @@ -100,7 +140,8 @@ static struct ttm_tt *i915_ttm_tt_create(struct ttm_buffer_object *bo,
> man->use_tt)
> page_flags |= TTM_PAGE_FLAG_ZERO_ALLOC;
>
> - ret = ttm_tt_init(&i915_tt->ttm, bo, page_flags, ttm_write_combined);
> + ret = ttm_tt_init(&i915_tt->ttm, bo, page_flags,
> + i915_ttm_select_tt_caching(obj));
> if (ret) {
> kfree(i915_tt);
> return NULL;
> @@ -465,10 +506,13 @@ static int i915_ttm_get_pages(struct drm_i915_gem_object *obj)
> .no_wait_gpu = false,
> };
> struct sg_table *st;
> + struct ttm_place requested, busy[I915_TTM_MAX_PLACEMENTS];
> + struct ttm_placement placement;
> int ret;
>
> /* Move to the requested placement. */
> - ret = ttm_bo_validate(bo, &i915_lmem0_placement, &ctx);
> + i915_ttm_placement_from_obj(obj, &requested, busy, &placement);
> + ret = ttm_bo_validate(bo, &placement, &ctx);
> if (ret)
> return ret == -ENOSPC ? -ENXIO : ret;
>
> @@ -684,7 +728,6 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
> i915_gem_object_make_unshrinkable(obj);
> INIT_RADIX_TREE(&obj->ttm.get_io_page.radix, GFP_KERNEL | __GFP_NOWARN);
> mutex_init(&obj->ttm.get_io_page.lock);
> -
> bo_type = (obj->flags & I915_BO_ALLOC_USER) ? ttm_bo_type_device :
> ttm_bo_type_kernel;
>
> @@ -708,7 +751,6 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
> i915_ttm_adjust_domains_after_cpu_move(obj);
> i915_ttm_adjust_gem_after_move(obj);
> i915_gem_object_unlock(obj);
> -
> out:
> /* i915 wants -ENXIO when out of memory region space. */
> return (ret == -ENOSPC) ? -ENXIO : ret;
> diff --git a/drivers/gpu/drm/i915/intel_region_ttm.c b/drivers/gpu/drm/i915/intel_region_ttm.c
> index 0b41a1545570..bc58ea942ef9 100644
> --- a/drivers/gpu/drm/i915/intel_region_ttm.c
> +++ b/drivers/gpu/drm/i915/intel_region_ttm.c
> @@ -49,12 +49,16 @@ void intel_region_ttm_device_fini(struct drm_i915_private *dev_priv)
> * driver-private types for now, reserving TTM_PL_VRAM for stolen
> * memory and TTM_PL_TT for GGTT use if decided to implement this.
> */
> -static int intel_region_to_ttm_type(struct intel_memory_region *mem)
> +int intel_region_to_ttm_type(const struct intel_memory_region *mem)
> {
> int type;
>
> GEM_BUG_ON(mem->type != INTEL_MEMORY_LOCAL &&
> - mem->type != INTEL_MEMORY_MOCK);
> + mem->type != INTEL_MEMORY_MOCK &&
> + mem->type != INTEL_MEMORY_SYSTEM);
> +
> + if (mem->type == INTEL_MEMORY_SYSTEM)
> + return TTM_PL_SYSTEM;
>
> type = mem->instance + TTM_PL_PRIV;
> GEM_BUG_ON(type >= TTM_NUM_MEM_TYPES);
> diff --git a/drivers/gpu/drm/i915/intel_region_ttm.h b/drivers/gpu/drm/i915/intel_region_ttm.h
> index eaa3eccfa252..88960ae6cff6 100644
> --- a/drivers/gpu/drm/i915/intel_region_ttm.h
> +++ b/drivers/gpu/drm/i915/intel_region_ttm.h
> @@ -27,6 +27,8 @@ struct sg_table *intel_region_ttm_node_to_st(struct intel_memory_region *mem,
> void intel_region_ttm_node_free(struct intel_memory_region *mem,
> void *node);
>
> +int intel_region_to_ttm_type(const struct intel_memory_region *mem);
> +
> struct ttm_device_funcs *i915_ttm_driver(void);
>
> #ifdef CONFIG_DRM_I915_SELFTEST
> --
> 2.31.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Intel-gfx] [PATCH 3/5] drm/i915/ttm: Calculate the object placement at get_pages time
@ 2021-06-03 10:58 ` Matthew Auld
0 siblings, 0 replies; 27+ messages in thread
From: Matthew Auld @ 2021-06-03 10:58 UTC (permalink / raw)
To: Thomas Hellström; +Cc: Intel Graphics Development, ML dri-devel
On Wed, 2 Jun 2021 at 18:08, Thomas Hellström
<thomas.hellstrom@linux.intel.com> wrote:
>
> Instead of relying on a static placement, calculate at get_pages() time.
> This should work for LMEM regions and system for now. For stolen we need
> to take preallocated range into account. That well be added later.
>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Can we split this patch out and merge it? We can use this for feeding
in the per BO flags.
> ---
> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 92 ++++++++++++++++++-------
> drivers/gpu/drm/i915/intel_region_ttm.c | 8 ++-
> drivers/gpu/drm/i915/intel_region_ttm.h | 2 +
> 3 files changed, 75 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index c73c51755c20..8e1c01168c6d 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -24,6 +24,11 @@
> #define I915_TTM_PRIO_NO_PAGES 1
> #define I915_TTM_PRIO_HAS_PAGES 2
>
> +/*
> + * Size of struct ttm_place vector in on-stack struct ttm_placement allocs
> + */
> +#define I915_TTM_MAX_PLACEMENTS 10
> +
> /**
> * struct i915_ttm_tt - TTM page vector with additional private information
> * @ttm: The base TTM page vector.
> @@ -42,32 +47,18 @@ struct i915_ttm_tt {
> struct sg_table *cached_st;
> };
>
> -static const struct ttm_place lmem0_sys_placement_flags[] = {
> - {
> - .fpfn = 0,
> - .lpfn = 0,
> - .mem_type = I915_PL_LMEM0,
> - .flags = 0,
> - }, {
> - .fpfn = 0,
> - .lpfn = 0,
> - .mem_type = I915_PL_SYSTEM,
> - .flags = 0,
> - }
> -};
> -
> -static struct ttm_placement i915_lmem0_placement = {
> - .num_placement = 1,
> - .placement = &lmem0_sys_placement_flags[0],
> - .num_busy_placement = 1,
> - .busy_placement = &lmem0_sys_placement_flags[0],
> +static const struct ttm_place sys_placement_flags = {
> + .fpfn = 0,
> + .lpfn = 0,
> + .mem_type = I915_PL_SYSTEM,
> + .flags = 0,
> };
>
> static struct ttm_placement i915_sys_placement = {
> .num_placement = 1,
> - .placement = &lmem0_sys_placement_flags[1],
> + .placement = &sys_placement_flags,
> .num_busy_placement = 1,
> - .busy_placement = &lmem0_sys_placement_flags[1],
> + .busy_placement = &sys_placement_flags,
> };
>
> static bool gpu_binds_iomem(struct ttm_resource *mem)
> @@ -83,6 +74,55 @@ static bool cpu_maps_iomem(struct ttm_resource *mem)
>
> static void i915_ttm_adjust_lru(struct drm_i915_gem_object *obj);
>
> +static enum ttm_caching
> +i915_ttm_select_tt_caching(const struct drm_i915_gem_object *obj)
> +{
> + /*
> + * Objects only allowed in system get cached cpu-mappings.
> + * Other objects get WC mapping for now. Even if in system.
> + */
> + if (obj->mm.region->type == INTEL_MEMORY_SYSTEM &&
> + obj->mm.n_placements <= 1)
> + return ttm_cached;
> +
> + return ttm_write_combined;
> +}
> +
> +static void
> +i915_ttm_place_from_region(const struct intel_memory_region *mr,
> + struct ttm_place *place)
> +{
> + memset(place, 0, sizeof(*place));
> + place->mem_type = intel_region_to_ttm_type(mr);
> +}
> +
> +static void
> +i915_ttm_placement_from_obj(const struct drm_i915_gem_object *obj,
> + struct ttm_place *requested,
> + struct ttm_place *busy,
> + struct ttm_placement *placement)
> +{
> + unsigned int i;
> + unsigned int num_allowed = obj->mm.n_placements;
Style nit: Christmas tree
> +
> + placement->num_placement = 1;
> + i915_ttm_place_from_region(num_allowed ? obj->mm.placements[0] :
> + obj->mm.region, requested);
> +
> + /* Cache this on object? */
> + placement->num_busy_placement = num_allowed;
> + for (i = 0; i < placement->num_busy_placement; ++i)
> + i915_ttm_place_from_region(obj->mm.placements[i], busy + i);
> +
> + if (num_allowed == 0) {
> + *busy = *requested;
> + placement->num_busy_placement = 1;
> + }
> +
> + placement->placement = requested;
> + placement->busy_placement = busy;
> +}
> +
> static struct ttm_tt *i915_ttm_tt_create(struct ttm_buffer_object *bo,
> uint32_t page_flags)
> {
> @@ -100,7 +140,8 @@ static struct ttm_tt *i915_ttm_tt_create(struct ttm_buffer_object *bo,
> man->use_tt)
> page_flags |= TTM_PAGE_FLAG_ZERO_ALLOC;
>
> - ret = ttm_tt_init(&i915_tt->ttm, bo, page_flags, ttm_write_combined);
> + ret = ttm_tt_init(&i915_tt->ttm, bo, page_flags,
> + i915_ttm_select_tt_caching(obj));
> if (ret) {
> kfree(i915_tt);
> return NULL;
> @@ -465,10 +506,13 @@ static int i915_ttm_get_pages(struct drm_i915_gem_object *obj)
> .no_wait_gpu = false,
> };
> struct sg_table *st;
> + struct ttm_place requested, busy[I915_TTM_MAX_PLACEMENTS];
> + struct ttm_placement placement;
> int ret;
>
> /* Move to the requested placement. */
> - ret = ttm_bo_validate(bo, &i915_lmem0_placement, &ctx);
> + i915_ttm_placement_from_obj(obj, &requested, busy, &placement);
> + ret = ttm_bo_validate(bo, &placement, &ctx);
> if (ret)
> return ret == -ENOSPC ? -ENXIO : ret;
>
> @@ -684,7 +728,6 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
> i915_gem_object_make_unshrinkable(obj);
> INIT_RADIX_TREE(&obj->ttm.get_io_page.radix, GFP_KERNEL | __GFP_NOWARN);
> mutex_init(&obj->ttm.get_io_page.lock);
> -
> bo_type = (obj->flags & I915_BO_ALLOC_USER) ? ttm_bo_type_device :
> ttm_bo_type_kernel;
>
> @@ -708,7 +751,6 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
> i915_ttm_adjust_domains_after_cpu_move(obj);
> i915_ttm_adjust_gem_after_move(obj);
> i915_gem_object_unlock(obj);
> -
> out:
> /* i915 wants -ENXIO when out of memory region space. */
> return (ret == -ENOSPC) ? -ENXIO : ret;
> diff --git a/drivers/gpu/drm/i915/intel_region_ttm.c b/drivers/gpu/drm/i915/intel_region_ttm.c
> index 0b41a1545570..bc58ea942ef9 100644
> --- a/drivers/gpu/drm/i915/intel_region_ttm.c
> +++ b/drivers/gpu/drm/i915/intel_region_ttm.c
> @@ -49,12 +49,16 @@ void intel_region_ttm_device_fini(struct drm_i915_private *dev_priv)
> * driver-private types for now, reserving TTM_PL_VRAM for stolen
> * memory and TTM_PL_TT for GGTT use if decided to implement this.
> */
> -static int intel_region_to_ttm_type(struct intel_memory_region *mem)
> +int intel_region_to_ttm_type(const struct intel_memory_region *mem)
> {
> int type;
>
> GEM_BUG_ON(mem->type != INTEL_MEMORY_LOCAL &&
> - mem->type != INTEL_MEMORY_MOCK);
> + mem->type != INTEL_MEMORY_MOCK &&
> + mem->type != INTEL_MEMORY_SYSTEM);
> +
> + if (mem->type == INTEL_MEMORY_SYSTEM)
> + return TTM_PL_SYSTEM;
>
> type = mem->instance + TTM_PL_PRIV;
> GEM_BUG_ON(type >= TTM_NUM_MEM_TYPES);
> diff --git a/drivers/gpu/drm/i915/intel_region_ttm.h b/drivers/gpu/drm/i915/intel_region_ttm.h
> index eaa3eccfa252..88960ae6cff6 100644
> --- a/drivers/gpu/drm/i915/intel_region_ttm.h
> +++ b/drivers/gpu/drm/i915/intel_region_ttm.h
> @@ -27,6 +27,8 @@ struct sg_table *intel_region_ttm_node_to_st(struct intel_memory_region *mem,
> void intel_region_ttm_node_free(struct intel_memory_region *mem,
> void *node);
>
> +int intel_region_to_ttm_type(const struct intel_memory_region *mem);
> +
> struct ttm_device_funcs *i915_ttm_driver(void);
>
> #ifdef CONFIG_DRM_I915_SELFTEST
> --
> 2.31.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 4/5] drm/i915/ttm: Use TTM for system memory
2021-06-02 17:07 ` [Intel-gfx] " Thomas Hellström
@ 2021-06-02 17:07 ` Thomas Hellström
-1 siblings, 0 replies; 27+ messages in thread
From: Thomas Hellström @ 2021-06-02 17:07 UTC (permalink / raw)
To: intel-gfx, dri-devel; +Cc: Thomas Hellström
For discrete, use TTM for both cached and WC system memory. That means
we currently rely on the TTM memory accounting / shrinker. For cached
system memory we should consider remaining shmem-backed, which can be
implemented from our ttm_tt_populate calback. We can then also reuse our
own very elaborate shrinker for that memory.
Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 22 ++++++++++++++++++++++
drivers/gpu/drm/i915/i915_drv.h | 3 ---
drivers/gpu/drm/i915/intel_memory_region.c | 7 ++++++-
drivers/gpu/drm/i915/intel_memory_region.h | 8 ++++++++
4 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 8e1c01168c6d..42e89bf43708 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -755,3 +755,25 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
/* i915 wants -ENXIO when out of memory region space. */
return (ret == -ENOSPC) ? -ENXIO : ret;
}
+
+static const struct intel_memory_region_ops ttm_system_region_ops = {
+ .init_object = __i915_gem_ttm_object_init,
+};
+
+struct intel_memory_region *
+i915_gem_ttm_system_setup(struct drm_i915_private *i915,
+ u16 type, u16 instance)
+{
+ struct intel_memory_region *mr;
+
+ mr = intel_memory_region_create(i915, 0,
+ totalram_pages() << PAGE_SHIFT,
+ PAGE_SIZE, 0,
+ type, instance,
+ &ttm_system_region_ops);
+ if (IS_ERR_OR_NULL(mr))
+ return mr;
+
+ intel_memory_region_set_name(mr, "system-ttm");
+ return mr;
+}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 524aaeb0e842..c6cc16ccce36 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1768,9 +1768,6 @@ void i915_gem_cleanup_userptr(struct drm_i915_private *dev_priv);
void i915_gem_init_early(struct drm_i915_private *dev_priv);
void i915_gem_cleanup_early(struct drm_i915_private *dev_priv);
-struct intel_memory_region *i915_gem_shmem_setup(struct drm_i915_private *i915,
- u16 type, u16 instance);
-
static inline void i915_gem_drain_freed_objects(struct drm_i915_private *i915)
{
/*
diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c
index bd27e897d4d0..a42bb36c2aea 100644
--- a/drivers/gpu/drm/i915/intel_memory_region.c
+++ b/drivers/gpu/drm/i915/intel_memory_region.c
@@ -220,7 +220,12 @@ int intel_memory_regions_hw_probe(struct drm_i915_private *i915)
instance = intel_region_map[i].instance;
switch (type) {
case INTEL_MEMORY_SYSTEM:
- mem = i915_gem_shmem_setup(i915, type, instance);
+ if (IS_DGFX(i915))
+ mem = i915_gem_ttm_system_setup(i915, type,
+ instance);
+ else
+ mem = i915_gem_shmem_setup(i915, type,
+ instance);
break;
case INTEL_MEMORY_STOLEN_LOCAL:
mem = i915_gem_stolen_lmem_setup(i915, type, instance);
diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h
index 7b5fa97c0b59..4d084424b55c 100644
--- a/drivers/gpu/drm/i915/intel_memory_region.h
+++ b/drivers/gpu/drm/i915/intel_memory_region.h
@@ -142,4 +142,12 @@ void intel_memory_region_unreserve(struct intel_memory_region *mem);
int intel_memory_region_reserve(struct intel_memory_region *mem,
resource_size_t offset,
resource_size_t size);
+
+struct intel_memory_region *
+i915_gem_ttm_system_setup(struct drm_i915_private *i915,
+ u16 type, u16 instance);
+struct intel_memory_region *
+i915_gem_shmem_setup(struct drm_i915_private *i915,
+ u16 type, u16 instance);
+
#endif
--
2.31.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [Intel-gfx] [PATCH 4/5] drm/i915/ttm: Use TTM for system memory
@ 2021-06-02 17:07 ` Thomas Hellström
0 siblings, 0 replies; 27+ messages in thread
From: Thomas Hellström @ 2021-06-02 17:07 UTC (permalink / raw)
To: intel-gfx, dri-devel; +Cc: Thomas Hellström
For discrete, use TTM for both cached and WC system memory. That means
we currently rely on the TTM memory accounting / shrinker. For cached
system memory we should consider remaining shmem-backed, which can be
implemented from our ttm_tt_populate calback. We can then also reuse our
own very elaborate shrinker for that memory.
Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 22 ++++++++++++++++++++++
drivers/gpu/drm/i915/i915_drv.h | 3 ---
drivers/gpu/drm/i915/intel_memory_region.c | 7 ++++++-
drivers/gpu/drm/i915/intel_memory_region.h | 8 ++++++++
4 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 8e1c01168c6d..42e89bf43708 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -755,3 +755,25 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
/* i915 wants -ENXIO when out of memory region space. */
return (ret == -ENOSPC) ? -ENXIO : ret;
}
+
+static const struct intel_memory_region_ops ttm_system_region_ops = {
+ .init_object = __i915_gem_ttm_object_init,
+};
+
+struct intel_memory_region *
+i915_gem_ttm_system_setup(struct drm_i915_private *i915,
+ u16 type, u16 instance)
+{
+ struct intel_memory_region *mr;
+
+ mr = intel_memory_region_create(i915, 0,
+ totalram_pages() << PAGE_SHIFT,
+ PAGE_SIZE, 0,
+ type, instance,
+ &ttm_system_region_ops);
+ if (IS_ERR_OR_NULL(mr))
+ return mr;
+
+ intel_memory_region_set_name(mr, "system-ttm");
+ return mr;
+}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 524aaeb0e842..c6cc16ccce36 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1768,9 +1768,6 @@ void i915_gem_cleanup_userptr(struct drm_i915_private *dev_priv);
void i915_gem_init_early(struct drm_i915_private *dev_priv);
void i915_gem_cleanup_early(struct drm_i915_private *dev_priv);
-struct intel_memory_region *i915_gem_shmem_setup(struct drm_i915_private *i915,
- u16 type, u16 instance);
-
static inline void i915_gem_drain_freed_objects(struct drm_i915_private *i915)
{
/*
diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c
index bd27e897d4d0..a42bb36c2aea 100644
--- a/drivers/gpu/drm/i915/intel_memory_region.c
+++ b/drivers/gpu/drm/i915/intel_memory_region.c
@@ -220,7 +220,12 @@ int intel_memory_regions_hw_probe(struct drm_i915_private *i915)
instance = intel_region_map[i].instance;
switch (type) {
case INTEL_MEMORY_SYSTEM:
- mem = i915_gem_shmem_setup(i915, type, instance);
+ if (IS_DGFX(i915))
+ mem = i915_gem_ttm_system_setup(i915, type,
+ instance);
+ else
+ mem = i915_gem_shmem_setup(i915, type,
+ instance);
break;
case INTEL_MEMORY_STOLEN_LOCAL:
mem = i915_gem_stolen_lmem_setup(i915, type, instance);
diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h
index 7b5fa97c0b59..4d084424b55c 100644
--- a/drivers/gpu/drm/i915/intel_memory_region.h
+++ b/drivers/gpu/drm/i915/intel_memory_region.h
@@ -142,4 +142,12 @@ void intel_memory_region_unreserve(struct intel_memory_region *mem);
int intel_memory_region_reserve(struct intel_memory_region *mem,
resource_size_t offset,
resource_size_t size);
+
+struct intel_memory_region *
+i915_gem_ttm_system_setup(struct drm_i915_private *i915,
+ u16 type, u16 instance);
+struct intel_memory_region *
+i915_gem_shmem_setup(struct drm_i915_private *i915,
+ u16 type, u16 instance);
+
#endif
--
2.31.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [Intel-gfx] [PATCH 4/5] drm/i915/ttm: Use TTM for system memory
2021-06-02 17:07 ` [Intel-gfx] " Thomas Hellström
@ 2021-06-03 9:48 ` Matthew Auld
-1 siblings, 0 replies; 27+ messages in thread
From: Matthew Auld @ 2021-06-03 9:48 UTC (permalink / raw)
To: Thomas Hellström; +Cc: Intel Graphics Development, ML dri-devel
On Wed, 2 Jun 2021 at 18:08, Thomas Hellström
<thomas.hellstrom@linux.intel.com> wrote:
>
> For discrete, use TTM for both cached and WC system memory. That means
> we currently rely on the TTM memory accounting / shrinker. For cached
> system memory we should consider remaining shmem-backed, which can be
> implemented from our ttm_tt_populate calback. We can then also reuse our
> own very elaborate shrinker for that memory.
>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 22 ++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_drv.h | 3 ---
> drivers/gpu/drm/i915/intel_memory_region.c | 7 ++++++-
> drivers/gpu/drm/i915/intel_memory_region.h | 8 ++++++++
> 4 files changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index 8e1c01168c6d..42e89bf43708 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -755,3 +755,25 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
> /* i915 wants -ENXIO when out of memory region space. */
> return (ret == -ENOSPC) ? -ENXIO : ret;
> }
> +
> +static const struct intel_memory_region_ops ttm_system_region_ops = {
> + .init_object = __i915_gem_ttm_object_init,
> +};
> +
> +struct intel_memory_region *
> +i915_gem_ttm_system_setup(struct drm_i915_private *i915,
> + u16 type, u16 instance)
> +{
> + struct intel_memory_region *mr;
> +
> + mr = intel_memory_region_create(i915, 0,
> + totalram_pages() << PAGE_SHIFT,
> + PAGE_SIZE, 0,
> + type, instance,
> + &ttm_system_region_ops);
> + if (IS_ERR_OR_NULL(mr))
region_create can't return NULL.
> + return mr;
> +
> + intel_memory_region_set_name(mr, "system-ttm");
> + return mr;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 524aaeb0e842..c6cc16ccce36 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1768,9 +1768,6 @@ void i915_gem_cleanup_userptr(struct drm_i915_private *dev_priv);
> void i915_gem_init_early(struct drm_i915_private *dev_priv);
> void i915_gem_cleanup_early(struct drm_i915_private *dev_priv);
>
> -struct intel_memory_region *i915_gem_shmem_setup(struct drm_i915_private *i915,
> - u16 type, u16 instance);
> -
> static inline void i915_gem_drain_freed_objects(struct drm_i915_private *i915)
> {
> /*
> diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c
> index bd27e897d4d0..a42bb36c2aea 100644
> --- a/drivers/gpu/drm/i915/intel_memory_region.c
> +++ b/drivers/gpu/drm/i915/intel_memory_region.c
> @@ -220,7 +220,12 @@ int intel_memory_regions_hw_probe(struct drm_i915_private *i915)
> instance = intel_region_map[i].instance;
> switch (type) {
> case INTEL_MEMORY_SYSTEM:
> - mem = i915_gem_shmem_setup(i915, type, instance);
> + if (IS_DGFX(i915))
> + mem = i915_gem_ttm_system_setup(i915, type,
> + instance);
> + else
> + mem = i915_gem_shmem_setup(i915, type,
> + instance);
> break;
> case INTEL_MEMORY_STOLEN_LOCAL:
> mem = i915_gem_stolen_lmem_setup(i915, type, instance);
> diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h
> index 7b5fa97c0b59..4d084424b55c 100644
> --- a/drivers/gpu/drm/i915/intel_memory_region.h
> +++ b/drivers/gpu/drm/i915/intel_memory_region.h
> @@ -142,4 +142,12 @@ void intel_memory_region_unreserve(struct intel_memory_region *mem);
> int intel_memory_region_reserve(struct intel_memory_region *mem,
> resource_size_t offset,
> resource_size_t size);
> +
> +struct intel_memory_region *
> +i915_gem_ttm_system_setup(struct drm_i915_private *i915,
> + u16 type, u16 instance);
> +struct intel_memory_region *
> +i915_gem_shmem_setup(struct drm_i915_private *i915,
> + u16 type, u16 instance);
> +
> #endif
> --
> 2.31.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Intel-gfx] [PATCH 4/5] drm/i915/ttm: Use TTM for system memory
@ 2021-06-03 9:48 ` Matthew Auld
0 siblings, 0 replies; 27+ messages in thread
From: Matthew Auld @ 2021-06-03 9:48 UTC (permalink / raw)
To: Thomas Hellström; +Cc: Intel Graphics Development, ML dri-devel
On Wed, 2 Jun 2021 at 18:08, Thomas Hellström
<thomas.hellstrom@linux.intel.com> wrote:
>
> For discrete, use TTM for both cached and WC system memory. That means
> we currently rely on the TTM memory accounting / shrinker. For cached
> system memory we should consider remaining shmem-backed, which can be
> implemented from our ttm_tt_populate calback. We can then also reuse our
> own very elaborate shrinker for that memory.
>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 22 ++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_drv.h | 3 ---
> drivers/gpu/drm/i915/intel_memory_region.c | 7 ++++++-
> drivers/gpu/drm/i915/intel_memory_region.h | 8 ++++++++
> 4 files changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index 8e1c01168c6d..42e89bf43708 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -755,3 +755,25 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
> /* i915 wants -ENXIO when out of memory region space. */
> return (ret == -ENOSPC) ? -ENXIO : ret;
> }
> +
> +static const struct intel_memory_region_ops ttm_system_region_ops = {
> + .init_object = __i915_gem_ttm_object_init,
> +};
> +
> +struct intel_memory_region *
> +i915_gem_ttm_system_setup(struct drm_i915_private *i915,
> + u16 type, u16 instance)
> +{
> + struct intel_memory_region *mr;
> +
> + mr = intel_memory_region_create(i915, 0,
> + totalram_pages() << PAGE_SHIFT,
> + PAGE_SIZE, 0,
> + type, instance,
> + &ttm_system_region_ops);
> + if (IS_ERR_OR_NULL(mr))
region_create can't return NULL.
> + return mr;
> +
> + intel_memory_region_set_name(mr, "system-ttm");
> + return mr;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 524aaeb0e842..c6cc16ccce36 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1768,9 +1768,6 @@ void i915_gem_cleanup_userptr(struct drm_i915_private *dev_priv);
> void i915_gem_init_early(struct drm_i915_private *dev_priv);
> void i915_gem_cleanup_early(struct drm_i915_private *dev_priv);
>
> -struct intel_memory_region *i915_gem_shmem_setup(struct drm_i915_private *i915,
> - u16 type, u16 instance);
> -
> static inline void i915_gem_drain_freed_objects(struct drm_i915_private *i915)
> {
> /*
> diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c
> index bd27e897d4d0..a42bb36c2aea 100644
> --- a/drivers/gpu/drm/i915/intel_memory_region.c
> +++ b/drivers/gpu/drm/i915/intel_memory_region.c
> @@ -220,7 +220,12 @@ int intel_memory_regions_hw_probe(struct drm_i915_private *i915)
> instance = intel_region_map[i].instance;
> switch (type) {
> case INTEL_MEMORY_SYSTEM:
> - mem = i915_gem_shmem_setup(i915, type, instance);
> + if (IS_DGFX(i915))
> + mem = i915_gem_ttm_system_setup(i915, type,
> + instance);
> + else
> + mem = i915_gem_shmem_setup(i915, type,
> + instance);
> break;
> case INTEL_MEMORY_STOLEN_LOCAL:
> mem = i915_gem_stolen_lmem_setup(i915, type, instance);
> diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h
> index 7b5fa97c0b59..4d084424b55c 100644
> --- a/drivers/gpu/drm/i915/intel_memory_region.h
> +++ b/drivers/gpu/drm/i915/intel_memory_region.h
> @@ -142,4 +142,12 @@ void intel_memory_region_unreserve(struct intel_memory_region *mem);
> int intel_memory_region_reserve(struct intel_memory_region *mem,
> resource_size_t offset,
> resource_size_t size);
> +
> +struct intel_memory_region *
> +i915_gem_ttm_system_setup(struct drm_i915_private *i915,
> + u16 type, u16 instance);
> +struct intel_memory_region *
> +i915_gem_shmem_setup(struct drm_i915_private *i915,
> + u16 type, u16 instance);
> +
> #endif
> --
> 2.31.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Intel-gfx] [PATCH 4/5] drm/i915/ttm: Use TTM for system memory
2021-06-03 9:48 ` Matthew Auld
@ 2021-06-03 14:27 ` Thomas Hellström
-1 siblings, 0 replies; 27+ messages in thread
From: Thomas Hellström @ 2021-06-03 14:27 UTC (permalink / raw)
To: Matthew Auld; +Cc: Intel Graphics Development, ML dri-devel
On 6/3/21 11:48 AM, Matthew Auld wrote:
> On Wed, 2 Jun 2021 at 18:08, Thomas Hellström
> <thomas.hellstrom@linux.intel.com> wrote:
>> For discrete, use TTM for both cached and WC system memory. That means
>> we currently rely on the TTM memory accounting / shrinker. For cached
>> system memory we should consider remaining shmem-backed, which can be
>> implemented from our ttm_tt_populate calback. We can then also reuse our
>> own very elaborate shrinker for that memory.
>>
>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 22 ++++++++++++++++++++++
>> drivers/gpu/drm/i915/i915_drv.h | 3 ---
>> drivers/gpu/drm/i915/intel_memory_region.c | 7 ++++++-
>> drivers/gpu/drm/i915/intel_memory_region.h | 8 ++++++++
>> 4 files changed, 36 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> index 8e1c01168c6d..42e89bf43708 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> @@ -755,3 +755,25 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
>> /* i915 wants -ENXIO when out of memory region space. */
>> return (ret == -ENOSPC) ? -ENXIO : ret;
>> }
>> +
>> +static const struct intel_memory_region_ops ttm_system_region_ops = {
>> + .init_object = __i915_gem_ttm_object_init,
>> +};
>> +
>> +struct intel_memory_region *
>> +i915_gem_ttm_system_setup(struct drm_i915_private *i915,
>> + u16 type, u16 instance)
>> +{
>> + struct intel_memory_region *mr;
>> +
>> + mr = intel_memory_region_create(i915, 0,
>> + totalram_pages() << PAGE_SHIFT,
>> + PAGE_SIZE, 0,
>> + type, instance,
>> + &ttm_system_region_ops);
>> + if (IS_ERR_OR_NULL(mr))
> region_create can't return NULL.
OK, will fix.
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Intel-gfx] [PATCH 4/5] drm/i915/ttm: Use TTM for system memory
@ 2021-06-03 14:27 ` Thomas Hellström
0 siblings, 0 replies; 27+ messages in thread
From: Thomas Hellström @ 2021-06-03 14:27 UTC (permalink / raw)
To: Matthew Auld; +Cc: Intel Graphics Development, ML dri-devel
On 6/3/21 11:48 AM, Matthew Auld wrote:
> On Wed, 2 Jun 2021 at 18:08, Thomas Hellström
> <thomas.hellstrom@linux.intel.com> wrote:
>> For discrete, use TTM for both cached and WC system memory. That means
>> we currently rely on the TTM memory accounting / shrinker. For cached
>> system memory we should consider remaining shmem-backed, which can be
>> implemented from our ttm_tt_populate calback. We can then also reuse our
>> own very elaborate shrinker for that memory.
>>
>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 22 ++++++++++++++++++++++
>> drivers/gpu/drm/i915/i915_drv.h | 3 ---
>> drivers/gpu/drm/i915/intel_memory_region.c | 7 ++++++-
>> drivers/gpu/drm/i915/intel_memory_region.h | 8 ++++++++
>> 4 files changed, 36 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> index 8e1c01168c6d..42e89bf43708 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> @@ -755,3 +755,25 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
>> /* i915 wants -ENXIO when out of memory region space. */
>> return (ret == -ENOSPC) ? -ENXIO : ret;
>> }
>> +
>> +static const struct intel_memory_region_ops ttm_system_region_ops = {
>> + .init_object = __i915_gem_ttm_object_init,
>> +};
>> +
>> +struct intel_memory_region *
>> +i915_gem_ttm_system_setup(struct drm_i915_private *i915,
>> + u16 type, u16 instance)
>> +{
>> + struct intel_memory_region *mr;
>> +
>> + mr = intel_memory_region_create(i915, 0,
>> + totalram_pages() << PAGE_SHIFT,
>> + PAGE_SIZE, 0,
>> + type, instance,
>> + &ttm_system_region_ops);
>> + if (IS_ERR_OR_NULL(mr))
> region_create can't return NULL.
OK, will fix.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 5/5] drm/i915/ttm: Implement object migration
2021-06-02 17:07 ` [Intel-gfx] " Thomas Hellström
@ 2021-06-02 17:07 ` Thomas Hellström
-1 siblings, 0 replies; 27+ messages in thread
From: Thomas Hellström @ 2021-06-02 17:07 UTC (permalink / raw)
To: intel-gfx, dri-devel; +Cc: Thomas Hellström
Implement object migration, needed primarily for dma-buf exports of
objects currently residing in LMEM, until we land p2pdma.
There are no users yet of this code.
Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
drivers/gpu/drm/i915/gem/i915_gem_object.c | 100 ++++++++++++++++++
drivers/gpu/drm/i915/gem/i915_gem_object.h | 10 ++
.../gpu/drm/i915/gem/i915_gem_object_types.h | 7 ++
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 51 ++++++++-
4 files changed, 164 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 07e8ff9a8aae..1589053ea99e 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -475,6 +475,106 @@ bool i915_gem_object_migratable(struct drm_i915_gem_object *obj)
return obj->mm.n_placements > 1;
}
+bool i915_gem_object_can_migrate_to_region(struct drm_i915_gem_object *obj,
+ struct intel_memory_region *mr,
+ unsigned int *placement_index)
+{
+ unsigned int i;
+ unsigned int num_allowed = obj->mm.n_placements;
+
+ if (!i915_gem_object_evictable(obj))
+ return false;
+
+ if (num_allowed == 0 && mr != obj->mm.region)
+ return false;
+
+ if (num_allowed == 1 && mr != obj->mm.placements[0])
+ return false;
+
+ for (i = 0; i < num_allowed; ++i) {
+ if (mr == obj->mm.placements[i]) {
+ if (placement_index)
+ *placement_index = i;
+ return true;
+ }
+ }
+
+ return false;
+}
+
+/**
+ * i915_gem_object_migrate_to_region_lazy - Lazily migrate an object
+ * @obj: The object to migrate.
+ * @mr: The region to migrate to.
+ *
+ * Check that @obj can migrate to @mr, and update all data necessary to
+ * make that happen on the next get_pages(). We sync and unbind gpu bindings
+ * and put pages. The word "lazy" means that the actual migration blit
+ * is not triggered by this function.
+ *
+ * Return: Zero on success, negative error code on failure.
+ */
+int i915_gem_object_migrate_to_region_lazy(struct drm_i915_gem_object *obj,
+ struct intel_memory_region *mr)
+{
+ unsigned int index;
+ int ret;
+
+ if (obj->mm.region == mr)
+ return 0;
+
+ if (!i915_gem_object_can_migrate_to_region(obj, mr, &index))
+ return -EINVAL;
+
+ ret = i915_gem_object_unbind(obj, I915_GEM_OBJECT_UNBIND_ACTIVE);
+ if (ret)
+ return ret;
+
+ ret = __i915_gem_object_put_pages(obj);
+ if (ret)
+ return ret;
+
+ /*
+ * The next get_pages() will pick up the new desired placement
+ * and migrate.
+ */
+ if (obj->mm.override_region) {
+ intel_memory_region_put(obj->mm.override_region);
+ obj->mm.override_region = NULL;
+ }
+
+ if (index != 0)
+ obj->mm.override_region =
+ intel_memory_region_get(obj->mm.placements[index]);
+
+ return 0;
+}
+
+/**
+ * i915_gem_object_migrate_to_region - Migrate an object
+ * @obj: The object to migrate.
+ * @mr: The region to migrate to.
+ *
+ * Check that @obj can migrate to @mr, and migrate the object.
+ * The caller needs to check that the final region was the
+ * desired one since the object may have ended up elsewhere on
+ * lack of space in the desired region, and if there are other
+ * allowed placements.
+ *
+ * Return: Zero on success, negative error code on failure.
+ */
+int i915_gem_object_migrate_to_region(struct drm_i915_gem_object *obj,
+ struct intel_memory_region *mr)
+{
+ int ret;
+
+ ret = i915_gem_object_migrate_to_region_lazy(obj, mr);
+ if (ret)
+ return ret;
+
+ return ____i915_gem_object_get_pages(obj);
+}
+
/**
* i915_gem_object_has_struct_page - Whether the object is page-backed
* @obj: The object to query.
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index 23e26f6b1db9..dfab5b080c54 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -592,6 +592,16 @@ bool i915_gem_object_migratable(struct drm_i915_gem_object *obj);
bool i915_gem_object_validates_to_lmem(struct drm_i915_gem_object *obj);
+bool i915_gem_object_can_migrate_to_region(struct drm_i915_gem_object *obj,
+ struct intel_memory_region *mr,
+ unsigned int *placement_index);
+
+int i915_gem_object_migrate_to_region_lazy(struct drm_i915_gem_object *obj,
+ struct intel_memory_region *mr);
+
+int i915_gem_object_migrate_to_region(struct drm_i915_gem_object *obj,
+ struct intel_memory_region *mr);
+
#ifdef CONFIG_MMU_NOTIFIER
static inline bool
i915_gem_object_is_userptr(struct drm_i915_gem_object *obj)
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 fb9ccc3f50e7..d645fa6f4c37 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
@@ -264,6 +264,13 @@ struct drm_i915_gem_object {
*/
struct intel_memory_region *region;
+ /**
+ * Override memory region for this object. Use to
+ * override the order of the placement list to migrate
+ * an object to the desired region.
+ */
+ struct intel_memory_region *override_region;
+
/**
* Memory manager node allocated for this object.
*/
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 42e89bf43708..c798470173e2 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -104,19 +104,45 @@ i915_ttm_placement_from_obj(const struct drm_i915_gem_object *obj,
{
unsigned int i;
unsigned int num_allowed = obj->mm.n_placements;
+ struct intel_memory_region *requested_mr;
+ unsigned int swap_index = 0;
placement->num_placement = 1;
- i915_ttm_place_from_region(num_allowed ? obj->mm.placements[0] :
- obj->mm.region, requested);
- /* Cache this on object? */
+ /*
+ * We migrate by setting the override region to something sensible.
+ * Warn if the override region is not in the allowed regions list
+ */
+ if (obj->mm.override_region && !GEM_WARN_ON(!num_allowed)) {
+ requested_mr = obj->mm.override_region;
+ swap_index = num_allowed;
+ } else if (num_allowed) {
+ requested_mr = obj->mm.placements[0];
+ } else {
+ requested_mr = obj->mm.region;
+ }
+ i915_ttm_place_from_region(requested_mr, requested);
+
+ /* In the future we might want to cache the busy list on the object? */
placement->num_busy_placement = num_allowed;
- for (i = 0; i < placement->num_busy_placement; ++i)
+ for (i = 0; i < placement->num_busy_placement; ++i) {
i915_ttm_place_from_region(obj->mm.placements[i], busy + i);
+ if (requested_mr == obj->mm.placements[i])
+ swap_index = i;
+ }
+
+ /* Override region was not in placement list. */
+ if (num_allowed && GEM_WARN_ON(swap_index == num_allowed)) {
+ swap_index = 0;
+ i915_ttm_place_from_region(obj->mm.placements[0], requested);
+ }
if (num_allowed == 0) {
*busy = *requested;
placement->num_busy_placement = 1;
+ } else if (swap_index != 0) {
+ /* Put the override placement highest in the busy list */
+ swap(busy[0], busy[swap_index]);
}
placement->placement = requested;
@@ -246,6 +272,23 @@ static void i915_ttm_adjust_gem_after_move(struct drm_i915_gem_object *obj)
struct drm_i915_private *i915 = to_i915(obj->base.dev);
struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
unsigned int cache_level;
+ unsigned int i;
+
+ /*
+ * If object was moved to an allowable region, update the object
+ * region to consider it migrated. Note that if it's currently not
+ * in an allowable region, it's evicted and we don't update the
+ * object region.
+ */
+ for (i = 0; i < obj->mm.n_placements; ++i) {
+ struct intel_memory_region *mr = obj->mm.placements[i];
+
+ if (intel_region_to_ttm_type(mr) == bo->mem.mem_type &&
+ mr != obj->mm.region) {
+ intel_memory_region_put(obj->mm.region);
+ obj->mm.region = intel_memory_region_get(mr);
+ }
+ }
obj->mem_flags &= ~(I915_BO_FLAG_STRUCT_PAGE | I915_BO_FLAG_IOMEM);
--
2.31.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* [Intel-gfx] [PATCH 5/5] drm/i915/ttm: Implement object migration
@ 2021-06-02 17:07 ` Thomas Hellström
0 siblings, 0 replies; 27+ messages in thread
From: Thomas Hellström @ 2021-06-02 17:07 UTC (permalink / raw)
To: intel-gfx, dri-devel; +Cc: Thomas Hellström
Implement object migration, needed primarily for dma-buf exports of
objects currently residing in LMEM, until we land p2pdma.
There are no users yet of this code.
Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
drivers/gpu/drm/i915/gem/i915_gem_object.c | 100 ++++++++++++++++++
drivers/gpu/drm/i915/gem/i915_gem_object.h | 10 ++
.../gpu/drm/i915/gem/i915_gem_object_types.h | 7 ++
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 51 ++++++++-
4 files changed, 164 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 07e8ff9a8aae..1589053ea99e 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -475,6 +475,106 @@ bool i915_gem_object_migratable(struct drm_i915_gem_object *obj)
return obj->mm.n_placements > 1;
}
+bool i915_gem_object_can_migrate_to_region(struct drm_i915_gem_object *obj,
+ struct intel_memory_region *mr,
+ unsigned int *placement_index)
+{
+ unsigned int i;
+ unsigned int num_allowed = obj->mm.n_placements;
+
+ if (!i915_gem_object_evictable(obj))
+ return false;
+
+ if (num_allowed == 0 && mr != obj->mm.region)
+ return false;
+
+ if (num_allowed == 1 && mr != obj->mm.placements[0])
+ return false;
+
+ for (i = 0; i < num_allowed; ++i) {
+ if (mr == obj->mm.placements[i]) {
+ if (placement_index)
+ *placement_index = i;
+ return true;
+ }
+ }
+
+ return false;
+}
+
+/**
+ * i915_gem_object_migrate_to_region_lazy - Lazily migrate an object
+ * @obj: The object to migrate.
+ * @mr: The region to migrate to.
+ *
+ * Check that @obj can migrate to @mr, and update all data necessary to
+ * make that happen on the next get_pages(). We sync and unbind gpu bindings
+ * and put pages. The word "lazy" means that the actual migration blit
+ * is not triggered by this function.
+ *
+ * Return: Zero on success, negative error code on failure.
+ */
+int i915_gem_object_migrate_to_region_lazy(struct drm_i915_gem_object *obj,
+ struct intel_memory_region *mr)
+{
+ unsigned int index;
+ int ret;
+
+ if (obj->mm.region == mr)
+ return 0;
+
+ if (!i915_gem_object_can_migrate_to_region(obj, mr, &index))
+ return -EINVAL;
+
+ ret = i915_gem_object_unbind(obj, I915_GEM_OBJECT_UNBIND_ACTIVE);
+ if (ret)
+ return ret;
+
+ ret = __i915_gem_object_put_pages(obj);
+ if (ret)
+ return ret;
+
+ /*
+ * The next get_pages() will pick up the new desired placement
+ * and migrate.
+ */
+ if (obj->mm.override_region) {
+ intel_memory_region_put(obj->mm.override_region);
+ obj->mm.override_region = NULL;
+ }
+
+ if (index != 0)
+ obj->mm.override_region =
+ intel_memory_region_get(obj->mm.placements[index]);
+
+ return 0;
+}
+
+/**
+ * i915_gem_object_migrate_to_region - Migrate an object
+ * @obj: The object to migrate.
+ * @mr: The region to migrate to.
+ *
+ * Check that @obj can migrate to @mr, and migrate the object.
+ * The caller needs to check that the final region was the
+ * desired one since the object may have ended up elsewhere on
+ * lack of space in the desired region, and if there are other
+ * allowed placements.
+ *
+ * Return: Zero on success, negative error code on failure.
+ */
+int i915_gem_object_migrate_to_region(struct drm_i915_gem_object *obj,
+ struct intel_memory_region *mr)
+{
+ int ret;
+
+ ret = i915_gem_object_migrate_to_region_lazy(obj, mr);
+ if (ret)
+ return ret;
+
+ return ____i915_gem_object_get_pages(obj);
+}
+
/**
* i915_gem_object_has_struct_page - Whether the object is page-backed
* @obj: The object to query.
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index 23e26f6b1db9..dfab5b080c54 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -592,6 +592,16 @@ bool i915_gem_object_migratable(struct drm_i915_gem_object *obj);
bool i915_gem_object_validates_to_lmem(struct drm_i915_gem_object *obj);
+bool i915_gem_object_can_migrate_to_region(struct drm_i915_gem_object *obj,
+ struct intel_memory_region *mr,
+ unsigned int *placement_index);
+
+int i915_gem_object_migrate_to_region_lazy(struct drm_i915_gem_object *obj,
+ struct intel_memory_region *mr);
+
+int i915_gem_object_migrate_to_region(struct drm_i915_gem_object *obj,
+ struct intel_memory_region *mr);
+
#ifdef CONFIG_MMU_NOTIFIER
static inline bool
i915_gem_object_is_userptr(struct drm_i915_gem_object *obj)
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 fb9ccc3f50e7..d645fa6f4c37 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
@@ -264,6 +264,13 @@ struct drm_i915_gem_object {
*/
struct intel_memory_region *region;
+ /**
+ * Override memory region for this object. Use to
+ * override the order of the placement list to migrate
+ * an object to the desired region.
+ */
+ struct intel_memory_region *override_region;
+
/**
* Memory manager node allocated for this object.
*/
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 42e89bf43708..c798470173e2 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -104,19 +104,45 @@ i915_ttm_placement_from_obj(const struct drm_i915_gem_object *obj,
{
unsigned int i;
unsigned int num_allowed = obj->mm.n_placements;
+ struct intel_memory_region *requested_mr;
+ unsigned int swap_index = 0;
placement->num_placement = 1;
- i915_ttm_place_from_region(num_allowed ? obj->mm.placements[0] :
- obj->mm.region, requested);
- /* Cache this on object? */
+ /*
+ * We migrate by setting the override region to something sensible.
+ * Warn if the override region is not in the allowed regions list
+ */
+ if (obj->mm.override_region && !GEM_WARN_ON(!num_allowed)) {
+ requested_mr = obj->mm.override_region;
+ swap_index = num_allowed;
+ } else if (num_allowed) {
+ requested_mr = obj->mm.placements[0];
+ } else {
+ requested_mr = obj->mm.region;
+ }
+ i915_ttm_place_from_region(requested_mr, requested);
+
+ /* In the future we might want to cache the busy list on the object? */
placement->num_busy_placement = num_allowed;
- for (i = 0; i < placement->num_busy_placement; ++i)
+ for (i = 0; i < placement->num_busy_placement; ++i) {
i915_ttm_place_from_region(obj->mm.placements[i], busy + i);
+ if (requested_mr == obj->mm.placements[i])
+ swap_index = i;
+ }
+
+ /* Override region was not in placement list. */
+ if (num_allowed && GEM_WARN_ON(swap_index == num_allowed)) {
+ swap_index = 0;
+ i915_ttm_place_from_region(obj->mm.placements[0], requested);
+ }
if (num_allowed == 0) {
*busy = *requested;
placement->num_busy_placement = 1;
+ } else if (swap_index != 0) {
+ /* Put the override placement highest in the busy list */
+ swap(busy[0], busy[swap_index]);
}
placement->placement = requested;
@@ -246,6 +272,23 @@ static void i915_ttm_adjust_gem_after_move(struct drm_i915_gem_object *obj)
struct drm_i915_private *i915 = to_i915(obj->base.dev);
struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
unsigned int cache_level;
+ unsigned int i;
+
+ /*
+ * If object was moved to an allowable region, update the object
+ * region to consider it migrated. Note that if it's currently not
+ * in an allowable region, it's evicted and we don't update the
+ * object region.
+ */
+ for (i = 0; i < obj->mm.n_placements; ++i) {
+ struct intel_memory_region *mr = obj->mm.placements[i];
+
+ if (intel_region_to_ttm_type(mr) == bo->mem.mem_type &&
+ mr != obj->mm.region) {
+ intel_memory_region_put(obj->mm.region);
+ obj->mm.region = intel_memory_region_get(mr);
+ }
+ }
obj->mem_flags &= ~(I915_BO_FLAG_STRUCT_PAGE | I915_BO_FLAG_IOMEM);
--
2.31.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915: Move system memory to TTM for discrete
2021-06-02 17:07 ` [Intel-gfx] " Thomas Hellström
` (5 preceding siblings ...)
(?)
@ 2021-06-02 17:31 ` Patchwork
-1 siblings, 0 replies; 27+ messages in thread
From: Patchwork @ 2021-06-02 17:31 UTC (permalink / raw)
To: Thomas Hellström; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Move system memory to TTM for discrete
URL : https://patchwork.freedesktop.org/series/90898/
State : failure
== Summary ==
Applying: drm/i915: Update object placement flags to be mutable
Using index info to reconstruct a base tree...
M drivers/gpu/drm/i915/gem/i915_gem_mman.c
M drivers/gpu/drm/i915/gem/i915_gem_object.c
M drivers/gpu/drm/i915/gem/i915_gem_object.h
M drivers/gpu/drm/i915/gem/i915_gem_object_types.h
M drivers/gpu/drm/i915/gem/i915_gem_pages.c
A drivers/gpu/drm/i915/gem/i915_gem_ttm.c
M drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
CONFLICT (modify/delete): drivers/gpu/drm/i915/gem/i915_gem_ttm.c deleted in HEAD and modified in drm/i915: Update object placement flags to be mutable. Version drm/i915: Update object placement flags to be mutable of drivers/gpu/drm/i915/gem/i915_gem_ttm.c left in tree.
Auto-merging drivers/gpu/drm/i915/gem/i915_gem_pages.c
Auto-merging drivers/gpu/drm/i915/gem/i915_gem_object_types.h
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/gem/i915_gem_object_types.h
Auto-merging drivers/gpu/drm/i915/gem/i915_gem_object.h
Auto-merging drivers/gpu/drm/i915/gem/i915_gem_object.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/gem/i915_gem_object.c
Auto-merging drivers/gpu/drm/i915/gem/i915_gem_mman.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/gem/i915_gem_mman.c
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 drm/i915: Update object placement flags to be mutable
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 27+ messages in thread