Re: [Intel-gfx] [PATCH 1/5] drm/i915: Update object placement flags to be mutable
On 6/3/21 1:17 PM, Matthew Auld wrote:
On Wed, 2 Jun 2021 at 18:08, Thomas Hellström
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
---
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(>drm, >base, size);
- i915_gem_object_init(obj, _gem_object_internal_ops, _class,
-I915_BO_ALLOC_STRUCT_PAGE);
+ i915_gem_object_init(obj, _gem_object_internal_ops, _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))
Re: [Intel-gfx] [PATCH 1/5] drm/i915: Update object placement flags to be mutable
On Wed, 2 Jun 2021 at 18:08, Thomas Hellström
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
> ---
> 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(>drm, >base, size);
> - i915_gem_object_init(obj, _gem_object_internal_ops, _class,
> -I915_BO_ALLOC_STRUCT_PAGE);
> + i915_gem_object_init(obj, _gem_object_internal_ops, _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);
[Intel-gfx] [PATCH 1/5] drm/i915: Update object placement flags to be mutable
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
---
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(>drm, >base, size);
- i915_gem_object_init(obj, _gem_object_internal_ops, _class,
-I915_BO_ALLOC_STRUCT_PAGE);
+ i915_gem_object_init(obj, _gem_object_internal_ops, _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(>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
+++
