[Intel-gfx] [PATCH v3] drm/i915/ttm: Rework object initialization slightly
Matthew Auld
matthew.auld at intel.com
Fri Oct 1 08:33:47 UTC 2021
On 30/09/2021 12:32, Thomas Hellström wrote:
> We may end up in i915_ttm_bo_destroy() in an error path before the
> object is fully initialized. In that case it's not correct to call
> __i915_gem_free_object(), because that function
> a) Assumes the gem object refcount is 0, which it isn't.
> b) frees the placements which are owned by the caller until the
> init_object() region ops returns successfully. Fix this by providing
> a lightweight cleanup function __i915_gem_object_fini() which is also
> called by __i915_gem_free_object().
>
> While doing this, also make sure we call dma_resv_fini() as part of
> ordinary object destruction and not from the RCU callback that frees
> the object. This will help track down bugs where the object is incorrectly
> locked from an RCU lookup.
>
> Finally, make sure the object isn't put on the region list until it's
> either locked or fully initialized in order to block list processing of
> partially initialized objects.
>
> v2:
> - The TTM object backend memory was freed before the gem pages were
> put. Separate this functionality into __i915_gem_object_pages_fini()
> and call it from the TTM delete_mem_notify() callback.
> v3:
> - Include i915_gem_object_free_mmaps() in __i915_gem_object_pages_fini()
> to make sure we don't inadvertedly introduce a race.
>
> Signed-off-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
> Reviewed-by: Matthew Auld <matthew.auld at intel.com> #v1
R-b still stands.
> ---
> drivers/gpu/drm/i915/gem/i915_gem_object.c | 43 +++++++++++++++++++---
> drivers/gpu/drm/i915/gem/i915_gem_object.h | 5 +++
> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 36 +++++++++++-------
> 3 files changed, 64 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> index 6fb9afb65034..b88b121e244a 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -89,6 +89,22 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
> mutex_init(&obj->mm.get_dma_page.lock);
> }
>
> +/**
> + * i915_gem_object_fini - Clean up a GEM object initialization
> + * @obj: The gem object to cleanup
> + *
> + * This function cleans up gem object fields that are set up by
> + * drm_gem_private_object_init() and i915_gem_object_init().
> + * It's primarily intended as a helper for backends that need to
> + * clean up the gem object in separate steps.
> + */
> +void __i915_gem_object_fini(struct drm_i915_gem_object *obj)
> +{
> + mutex_destroy(&obj->mm.get_page.lock);
> + mutex_destroy(&obj->mm.get_dma_page.lock);
> + dma_resv_fini(&obj->base._resv);
> +}
> +
> /**
> * Mark up the object's coherency levels for a given cache_level
> * @obj: #drm_i915_gem_object
> @@ -174,7 +190,6 @@ void __i915_gem_free_object_rcu(struct rcu_head *head)
> container_of(head, typeof(*obj), rcu);
> struct drm_i915_private *i915 = to_i915(obj->base.dev);
>
> - dma_resv_fini(&obj->base._resv);
> i915_gem_object_free(obj);
>
> GEM_BUG_ON(!atomic_read(&i915->mm.free_count));
> @@ -204,10 +219,17 @@ static void __i915_gem_object_free_mmaps(struct drm_i915_gem_object *obj)
> }
> }
>
> -void __i915_gem_free_object(struct drm_i915_gem_object *obj)
> +/**
> + * __i915_gem_object_pages_fini - Clean up pages use of a gem object
> + * @obj: The gem object to clean up
> + *
> + * This function cleans up usage of the object mm.pages member. It
> + * is intended for backends that need to clean up a gem object in
> + * separate steps and needs to be called when the object is idle before
> + * the object's backing memory is freed.
> + */
> +void __i915_gem_object_pages_fini(struct drm_i915_gem_object *obj)
> {
> - trace_i915_gem_object_destroy(obj);
> -
> if (!list_empty(&obj->vma.list)) {
> struct i915_vma *vma;
>
> @@ -233,11 +255,17 @@ void __i915_gem_free_object(struct drm_i915_gem_object *obj)
>
> __i915_gem_object_free_mmaps(obj);
>
> - GEM_BUG_ON(!list_empty(&obj->lut_list));
> -
> atomic_set(&obj->mm.pages_pin_count, 0);
> __i915_gem_object_put_pages(obj);
> GEM_BUG_ON(i915_gem_object_has_pages(obj));
> +}
> +
> +void __i915_gem_free_object(struct drm_i915_gem_object *obj)
> +{
> + trace_i915_gem_object_destroy(obj);
> +
> + GEM_BUG_ON(!list_empty(&obj->lut_list));
> +
> bitmap_free(obj->bit_17);
>
> if (obj->base.import_attach)
> @@ -253,6 +281,8 @@ void __i915_gem_free_object(struct drm_i915_gem_object *obj)
>
> if (obj->shares_resv_from)
> i915_vm_resv_put(obj->shares_resv_from);
> +
> + __i915_gem_object_fini(obj);
> }
>
> static void __i915_gem_free_objects(struct drm_i915_private *i915,
> @@ -266,6 +296,7 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915,
> obj->ops->delayed_free(obj);
> continue;
> }
> + __i915_gem_object_pages_fini(obj);
> __i915_gem_free_object(obj);
>
> /* But keep the pointer alive for RCU-protected lookups */
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> index 3043fcbd31bd..7f9f2e5ba0ec 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> @@ -58,6 +58,9 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
> const struct drm_i915_gem_object_ops *ops,
> struct lock_class_key *key,
> unsigned alloc_flags);
> +
> +void __i915_gem_object_fini(struct drm_i915_gem_object *obj);
> +
> struct drm_i915_gem_object *
> i915_gem_object_create_shmem(struct drm_i915_private *i915,
> resource_size_t size);
> @@ -582,6 +585,8 @@ bool i915_gem_object_is_shmem(const struct drm_i915_gem_object *obj);
>
> void __i915_gem_free_object_rcu(struct rcu_head *head);
>
> +void __i915_gem_object_pages_fini(struct drm_i915_gem_object *obj);
> +
> void __i915_gem_free_object(struct drm_i915_gem_object *obj);
>
> bool i915_gem_object_evictable(struct drm_i915_gem_object *obj);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index b94497989995..a0ab5c44627b 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -367,8 +367,10 @@ static void i915_ttm_delete_mem_notify(struct ttm_buffer_object *bo)
> {
> struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
>
> - if (likely(obj))
> + if (likely(obj)) {
> + __i915_gem_object_pages_fini(obj);
> i915_ttm_free_cached_io_st(obj);
> + }
> }
>
> static struct intel_memory_region *
> @@ -813,12 +815,9 @@ static void i915_ttm_adjust_lru(struct drm_i915_gem_object *obj)
> */
> static void i915_ttm_delayed_free(struct drm_i915_gem_object *obj)
> {
> - if (obj->ttm.created) {
> - ttm_bo_put(i915_gem_to_ttm(obj));
> - } else {
> - __i915_gem_free_object(obj);
> - call_rcu(&obj->rcu, __i915_gem_free_object_rcu);
> - }
> + GEM_BUG_ON(!obj->ttm.created);
> +
> + ttm_bo_put(i915_gem_to_ttm(obj));
> }
>
> static vm_fault_t vm_fault_ttm(struct vm_fault *vmf)
> @@ -898,16 +897,19 @@ void i915_ttm_bo_destroy(struct ttm_buffer_object *bo)
> {
> struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
>
> - i915_ttm_backup_free(obj);
> -
> - /* This releases all gem object bindings to the backend. */
> - __i915_gem_free_object(obj);
> -
> i915_gem_object_release_memory_region(obj);
> mutex_destroy(&obj->ttm.get_io_page.lock);
>
> - if (obj->ttm.created)
> + if (obj->ttm.created) {
> + i915_ttm_backup_free(obj);
> +
> + /* This releases all gem object bindings to the backend. */
> + __i915_gem_free_object(obj);
> +
> call_rcu(&obj->rcu, __i915_gem_free_object_rcu);
> + } else {
> + __i915_gem_object_fini(obj);
> + }
> }
>
> /**
> @@ -936,7 +938,11 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
>
> drm_gem_private_object_init(&i915->drm, &obj->base, size);
> i915_gem_object_init(obj, &i915_gem_ttm_obj_ops, &lock_class, flags);
> - i915_gem_object_init_memory_region(obj, mem);
> +
> + /* Don't put on a region list until we're either locked or fully initialized. */
> + obj->mm.region = intel_memory_region_get(mem);
> + INIT_LIST_HEAD(&obj->mm.region_link);
> +
> 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);
> @@ -963,6 +969,8 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
> return i915_ttm_err_to_gem(ret);
>
> obj->ttm.created = true;
> + i915_gem_object_release_memory_region(obj);
> + i915_gem_object_init_memory_region(obj, mem);
> i915_ttm_adjust_domains_after_move(obj);
> i915_ttm_adjust_gem_after_move(obj);
> i915_gem_object_unlock(obj);
>
More information about the Intel-gfx
mailing list