Re: [PATCH 12/23] dma-buf/drivers: make reserving a shared slot mandatory v3

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

Am 28.03.22 um 19:14 schrieb Daniel Vetter:
> On Mon, Mar 21, 2022 at 02:58:45PM +0100, Christian König wrote:
>> [SNIP]
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> index ea0cde4904f0..2f808decd8d9 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -1384,6 +1384,14 @@ void amdgpu_bo_fence(struct amdgpu_bo *bo, struct dma_fence *fence,
>>   		     bool shared)
>>   {
>>   	struct dma_resv *resv = bo->tbo.base.resv;
>> +	int r;
>> +
>> +	r = dma_resv_reserve_fences(resv, 1);
> This is quite a hack, but I did scroll through all the callers of
> amdgpu_bo_fence and I think it's fine - i.e. no recursion into the
> shrinker from a calling context where recursion into shrinker/memalloc
> isn't allowed.
>
> But it aint pretty :-/

Yeah, but one long term goal of this is to remove all the hacky handling 
of manually adding fences to the resv object using this function. I 
could add a TODO if that helps.

> [SNIP]
>> 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 ee9612a3ee5e..4de6500f3c55 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
>> @@ -596,7 +596,11 @@ int i915_gem_obj_copy_ttm(struct drm_i915_gem_object *dst,
>>   	assert_object_held(src);
>>   	i915_deps_init(&deps, GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN);
>>   
>> -	ret = dma_resv_reserve_shared(src_bo->base.resv, 1);
>> +	ret = dma_resv_reserve_fences(src_bo->base.resv, 1);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = dma_resv_reserve_fences(dst_bo->base.resv, 1);
> Can't we just reserve 2 slots instead of doing this 2x?

*handing you some coffee* We reserve one one slot on the source and one 
on the destination buffer :)

> [SNIP]
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index a6925dbb6224..c34114560e49 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -247,6 +247,10 @@ static int panfrost_acquire_object_fences(struct drm_gem_object **bos,
>   	int i, ret;
>   
>   	for (i = 0; i < bo_count; i++) {
> +		ret = dma_resv_reserve_fences(bos[i]->resv, 1);
> +		if (ret)
> +			return ret;
> +
>   		/* panfrost always uses write mode in its current uapi */
>   		ret = drm_sched_job_add_implicit_dependencies(job, bos[i],
> I wonder whether we shouldn't move the dma-resv reserving into some shared
> helpers eventually ...

I was going back and forth adding this to 
drm_sched_job_add_implicit_dependencies(), but then decided against that 
because it is really two independent functionalities.

>> [SNIP]
>> @@ -120,9 +119,9 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket,
>>   			ret = ttm_bo_reserve_slowpath(bo, intr, ticket);
>>   		}
>>   
>> -		if (!ret && entry->num_shared)
>> -			ret = dma_resv_reserve_shared(bo->base.resv,
>> -								entry->num_shared);
>> +		if (!ret)
>> +			ret = dma_resv_reserve_fences(bo->base.resv,
>> +						      num_fences);
>>   
>>   		if (unlikely(ret != 0)) {
>>   			if (ticket) {
> I didn't fine the corresponding reserve for the dma_resv_add_excl_fence()
> in ttm_bo_move_accel_cleanup(). Was that an oversight?

Mhm, need to double check as well. Could be that I missed that one.

>> [SNIP]
>> diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
>> index 4abf10b66fe8..594bd6bb00d2 100644
>> --- a/drivers/gpu/drm/vc4/vc4_gem.c
>> +++ b/drivers/gpu/drm/vc4/vc4_gem.c
>> @@ -644,7 +644,7 @@ vc4_lock_bo_reservations(struct drm_device *dev,
>>   	for (i = 0; i < exec->bo_count; i++) {
>>   		bo = &exec->bo[i]->base;
>>   
>> -		ret = dma_resv_reserve_shared(bo->resv, 1);
>> +		ret = dma_resv_reserve_fences(bo->resv, 1);
>>   		if (ret) {
>>   			vc4_unlock_bo_reservations(dev, exec, acquire_ctx);
>>   			return ret;
> v3d and vc4 are missing in the conversion. I think for both you need to
> add it before the call to  like
> with etnaviv.

Both drivers already have the necessary calls. See 
vc4_lock_bo_reservations() and v3d_lock_bo_reservations().

>> [SNIP]
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
>> index 48d3c9955f0d..1820ca6cf673 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_gem.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
>> @@ -214,6 +214,7 @@ void virtio_gpu_array_add_obj(struct virtio_gpu_object_array *objs,
>>   
>>   int virtio_gpu_array_lock_resv(struct virtio_gpu_object_array *objs)
>>   {
>> +	unsigned int i;
>>   	int ret;
>>   
>>   	if (objs->nents == 1) {
>> @@ -222,6 +223,14 @@ int virtio_gpu_array_lock_resv(struct virtio_gpu_object_array *objs)
>>   		ret = drm_gem_lock_reservations(objs->objs, objs->nents,
>>   						&objs->ticket);
>>   	}
>> +	if (ret)
>> +		return ret;
>> +
>> +	for (i = 0; i < objs->nents; ++i) {
>> +		ret = dma_resv_reserve_fences(objs->objs[i]->resv, 1);
> I think you could stuff this into the same loop, but also probably doesn't
> matter.

Na, that loop is inside drm_gem_lock_reservations().

> [SNIP]
>
> I found a few things, but with those (vc4 and v3d plus the ttm question,
> the other stuff is just comments) corrected this gets my

Going to double check the TTM case once more, but apart from that I 
think its solid.

> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks,
Christian.

>
>> -- 
>> 2.25.1
>>