Re: [PATCH 2/4] drm/ttm: use the parent resv for ghost objects v2

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 08.10.19 um 11:25 schrieb Daniel Vetter:
> On Thu, Aug 29, 2019 at 04:29:15PM +0200, Christian König wrote:
>> This way we can even pipeline imported BO evictions.
>>
>> v2: Limit this to only cases when the parent object uses a separate
>>      reservation object as well. This fixes another OOM problem.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
> Since I read quite a bit of ttm I figured I'll review this too, but I'm
> totally lost. And git blame gives me at best commits with one-liner commit
> messages, and the docs aren't explaining much at all either (and generally
> they didn't get updated at all with all the changes in the past years).
>
> I have a vague idea of what you're doing here, but not enough to do review
> with any confidence. And from other ttm patches from amd it feels a lot
> like we have essentially a bus factor of 1 for all things ttm :-/

Yeah, that's one of a couple of reasons why I want to get rid of TTM in 
the long term.

Basically this is a bug fix for delay freeing ttm objects. When we hang 
the ttm object on a ghost object to be freed and the ttm object is an 
imported DMA-buf we run into the problem that we want to drop the 
mapping, but have the wrong lock taken (the lock of the ghost and not of 
the parent).

Regards,
Christian.

> -Daniel
>
>> ---
>>   drivers/gpu/drm/ttm/ttm_bo_util.c | 16 +++++++++-------
>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
>> index fe81c565e7ef..2ebe9fe7f6c8 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
>> @@ -517,7 +517,9 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo,
>>   	kref_init(&fbo->base.kref);
>>   	fbo->base.destroy = &ttm_transfered_destroy;
>>   	fbo->base.acc_size = 0;
>> -	fbo->base.base.resv = &fbo->base.base._resv;
>> +	if (bo->base.resv == &bo->base._resv)
>> +		fbo->base.base.resv = &fbo->base.base._resv;
>> +
>>   	dma_resv_init(fbo->base.base.resv);
>>   	ret = dma_resv_trylock(fbo->base.base.resv);
>>   	WARN_ON(!ret);
>> @@ -716,7 +718,7 @@ int ttm_bo_move_accel_cleanup(struct ttm_buffer_object *bo,
>>   		if (ret)
>>   			return ret;
>>   
>> -		dma_resv_add_excl_fence(ghost_obj->base.resv, fence);
>> +		dma_resv_add_excl_fence(&ghost_obj->base._resv, fence);
>>   
>>   		/**
>>   		 * If we're not moving to fixed memory, the TTM object
>> @@ -729,7 +731,7 @@ int ttm_bo_move_accel_cleanup(struct ttm_buffer_object *bo,
>>   		else
>>   			bo->ttm = NULL;
>>   
>> -		ttm_bo_unreserve(ghost_obj);
>> +		dma_resv_unlock(&ghost_obj->base._resv);
>>   		ttm_bo_put(ghost_obj);
>>   	}
>>   
>> @@ -772,7 +774,7 @@ int ttm_bo_pipeline_move(struct ttm_buffer_object *bo,
>>   		if (ret)
>>   			return ret;
>>   
>> -		dma_resv_add_excl_fence(ghost_obj->base.resv, fence);
>> +		dma_resv_add_excl_fence(&ghost_obj->base._resv, fence);
>>   
>>   		/**
>>   		 * If we're not moving to fixed memory, the TTM object
>> @@ -785,7 +787,7 @@ int ttm_bo_pipeline_move(struct ttm_buffer_object *bo,
>>   		else
>>   			bo->ttm = NULL;
>>   
>> -		ttm_bo_unreserve(ghost_obj);
>> +		dma_resv_unlock(&ghost_obj->base._resv);
>>   		ttm_bo_put(ghost_obj);
>>   
>>   	} else if (from->flags & TTM_MEMTYPE_FLAG_FIXED) {
>> @@ -841,7 +843,7 @@ int ttm_bo_pipeline_gutting(struct ttm_buffer_object *bo)
>>   	if (ret)
>>   		return ret;
>>   
>> -	ret = dma_resv_copy_fences(ghost->base.resv, bo->base.resv);
>> +	ret = dma_resv_copy_fences(&ghost->base._resv, bo->base.resv);
>>   	/* Last resort, wait for the BO to be idle when we are OOM */
>>   	if (ret)
>>   		ttm_bo_wait(bo, false, false);
>> @@ -850,7 +852,7 @@ int ttm_bo_pipeline_gutting(struct ttm_buffer_object *bo)
>>   	bo->mem.mem_type = TTM_PL_SYSTEM;
>>   	bo->ttm = NULL;
>>   
>> -	ttm_bo_unreserve(ghost);
>> +	dma_resv_unlock(&ghost->base._resv);
>>   	ttm_bo_put(ghost);
>>   
>>   	return 0;
>> -- 
>> 2.17.1
>>