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

Das, Nirmoy nirmoy.das at intel.com
Fri Oct 14 15:10:56 UTC 2022


On 10/14/2022 4:58 PM, Matthew Auld wrote:
> 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?


Yes, I can send a patch to fix that up.

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


Thanks,


Nirmoy

>
>>         /* 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;
>>       }


More information about the dri-devel mailing list