[Intel-gfx] [PATCH] drm/i915: Refactor ttm ghost obj detection

[Matthew Auld]

On 14/10/2022 14:14, Nirmoy Das wrote:
> Currently i915_ttm_to_gem() returns NULL for ttm ghost
> object which makes it unclear when we should add a NULL
> check for a caller of i915_ttm_to_gem() as ttm ghost
> objects are expected behaviour for certain cases.
> 
> Create a separate function to detect ttm ghost object and
> use that in places where we expect a ghost obj from ttm.
> 
> Signed-off-by: Nirmoy Das <nirmoy.das at intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_ttm.c      | 21 ++++++++++----------
>   drivers/gpu/drm/i915/gem/i915_gem_ttm.h      | 18 ++++++++++++-----
>   drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c |  2 +-
>   3 files changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index 6b60b99461e2..0a85651c654d 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -279,7 +279,7 @@ static struct ttm_tt *i915_ttm_tt_create(struct ttm_buffer_object *bo,
>   	struct i915_ttm_tt *i915_tt;
>   	int ret;
>   
> -	if (!obj)
> +	if (i915_ttm_is_ghost_object(bo))
>   		return NULL;
>   
>   	i915_tt = kzalloc(sizeof(*i915_tt), GFP_KERNEL);
> @@ -362,7 +362,7 @@ static bool i915_ttm_eviction_valuable(struct ttm_buffer_object *bo,
>   {
>   	struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
>   
> -	if (!obj)
> +	if (i915_ttm_is_ghost_object(bo))
>   		return false;
>   
>   	/*
> @@ -511,7 +511,7 @@ static void i915_ttm_delete_mem_notify(struct ttm_buffer_object *bo)
>   	struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
>   	intel_wakeref_t wakeref = 0;
>   
> -	if (bo->resource && likely(obj)) {
> +	if (bo->resource && !i915_ttm_is_ghost_object(bo)) {
>   		/* ttm_bo_release() already has dma_resv_lock */
>   		if (i915_ttm_cpu_maps_iomem(bo->resource))
>   			wakeref = intel_runtime_pm_get(&to_i915(obj->base.dev)->runtime_pm);
> @@ -624,7 +624,7 @@ static void i915_ttm_swap_notify(struct ttm_buffer_object *bo)
>   	struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
>   	int ret;
>   
> -	if (!obj)
> +	if (i915_ttm_is_ghost_object(bo))
>   		return;
>   
>   	ret = i915_ttm_move_notify(bo);
> @@ -657,7 +657,7 @@ static int i915_ttm_io_mem_reserve(struct ttm_device *bdev, struct ttm_resource
>   	struct drm_i915_gem_object *obj = i915_ttm_to_gem(mem->bo);
>   	bool unknown_state;
>   
> -	if (!obj)
> +	if (i915_ttm_is_ghost_object(mem->bo))
>   		return -EINVAL;
>   
>   	if (!kref_get_unless_zero(&obj->base.refcount))
> @@ -690,7 +690,7 @@ static unsigned long i915_ttm_io_mem_pfn(struct ttm_buffer_object *bo,
>   	unsigned long base;
>   	unsigned int ofs;
>   
> -	GEM_BUG_ON(!obj);
> +	GEM_BUG_ON(i915_ttm_is_ghost_object(bo));
>   	GEM_WARN_ON(bo->ttm);
>   
>   	base = obj->mm.region->iomap.base - obj->mm.region->region.start;
> @@ -1035,13 +1035,12 @@ static vm_fault_t vm_fault_ttm(struct vm_fault *vmf)
>   	struct vm_area_struct *area = vmf->vma;
>   	struct ttm_buffer_object *bo = area->vm_private_data;
>   	struct drm_device *dev = bo->base.dev;
> -	struct drm_i915_gem_object *obj;
> +	struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
>   	intel_wakeref_t wakeref = 0;
>   	vm_fault_t ret;
>   	int idx;
>   
> -	obj = i915_ttm_to_gem(bo);
> -	if (!obj)
> +	if (i915_ttm_is_ghost_object(bo))
>   		return VM_FAULT_SIGBUS;

I think this one can be dropped, maybe in a separate patch?

Otherwise looks good to me,
Reviewed-by: Matthew Auld <matthew.auld at intel.com>

>   
>   	/* Sanity check that we allow writing into this object */
> @@ -1141,7 +1140,7 @@ static void ttm_vm_open(struct vm_area_struct *vma)
>   	struct drm_i915_gem_object *obj =
>   		i915_ttm_to_gem(vma->vm_private_data);
>   
> -	GEM_BUG_ON(!obj);
> +	GEM_BUG_ON(i915_ttm_is_ghost_object(vma->vm_private_data));
>   	i915_gem_object_get(obj);
>   }
>   
> @@ -1150,7 +1149,7 @@ static void ttm_vm_close(struct vm_area_struct *vma)
>   	struct drm_i915_gem_object *obj =
>   		i915_ttm_to_gem(vma->vm_private_data);
>   
> -	GEM_BUG_ON(!obj);
> +	GEM_BUG_ON(i915_ttm_is_ghost_object(vma->vm_private_data));
>   	i915_gem_object_put(obj);
>   }
>   
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.h b/drivers/gpu/drm/i915/gem/i915_gem_ttm.h
> index e4842b4296fc..2a94a99ef76b 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.h
> @@ -27,19 +27,27 @@ i915_gem_to_ttm(struct drm_i915_gem_object *obj)
>    */
>   void i915_ttm_bo_destroy(struct ttm_buffer_object *bo);
>   
> +/**
> + * i915_ttm_is_ghost_object - Check if the ttm bo is a ghost object.
> + * @bo: Pointer to the ttm buffer object
> + *
> + * Return: True if the ttm bo is not a i915 object but a ghost ttm object,
> + * False otherwise.
> + */
> +static inline bool i915_ttm_is_ghost_object(struct ttm_buffer_object *bo)
> +{
> +	return bo->destroy != i915_ttm_bo_destroy;
> +}
> +
>   /**
>    * i915_ttm_to_gem - Convert a struct ttm_buffer_object to an embedding
>    * struct drm_i915_gem_object.
>    *
> - * Return: Pointer to the embedding struct ttm_buffer_object, or NULL
> - * if the object was not an i915 ttm object.
> + * Return: Pointer to the embedding struct ttm_buffer_object.
>    */
>   static inline struct drm_i915_gem_object *
>   i915_ttm_to_gem(struct ttm_buffer_object *bo)
>   {
> -	if (bo->destroy != i915_ttm_bo_destroy)
> -		return NULL;
> -
>   	return container_of(bo, struct drm_i915_gem_object, __do_not_access);
>   }
>   
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
> index 9a7e50534b84..f59f812dc6d2 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
> @@ -560,7 +560,7 @@ int i915_ttm_move(struct ttm_buffer_object *bo, bool evict,
>   	bool clear;
>   	int ret;
>   
> -	if (GEM_WARN_ON(!obj)) {
> +	if (GEM_WARN_ON(i915_ttm_is_ghost_object(bo))) {
>   		ttm_bo_move_null(bo, dst_mem);
>   		return 0;
>   	}