Re: [PATCH] drm/amdgpu: fix old fence check in amdgpu_fence_emit

["Christian König" <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Am 01.04.19 um 04:54 schrieb Zhou, David(ChunMing):
>
>> -----Original Message-----
>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>> Christian K?nig
>> Sent: Saturday, March 30, 2019 2:33 AM
>> To: amd-gfx@lists.freedesktop.org
>> Subject: [PATCH] drm/amdgpu: fix old fence check in amdgpu_fence_emit
>>
>> We don't hold a reference to the old fence, so it can go away any time we are
>> waiting for it to signal.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 24 ++++++++++++++++-
>> ------
>>   1 file changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>> index ee47c11e92ce..4dee2326b29c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>> @@ -136,8 +136,9 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring,
>> struct dma_fence **f,  {
>>   	struct amdgpu_device *adev = ring->adev;
>>   	struct amdgpu_fence *fence;
>> -	struct dma_fence *old, **ptr;
>> +	struct dma_fence __rcu **ptr;
>>   	uint32_t seq;
>> +	int r;
>>
>>   	fence = kmem_cache_alloc(amdgpu_fence_slab, GFP_KERNEL);
>>   	if (fence == NULL)
>> @@ -153,15 +154,24 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring,
>> struct dma_fence **f,
>>   			       seq, flags | AMDGPU_FENCE_FLAG_INT);
>>
>>   	ptr = &ring->fence_drv.fences[seq & ring-
>>> fence_drv.num_fences_mask];
>> +	if (unlikely(rcu_dereference_protected(*ptr, 1))) {
> Isn't this line redundant with dma_fence_get_rcu_safe? I think it's unnecessary.
> Otherwise looks ok to me.

The key point is lock()+dma_fence_get_rcu_safe(ptr)+unlock() is rather 
expensive for something which is really unlikely.

So we check here if we already see the variable as NULL and if that is 
true, then we can just skip the whole expensive dance.

Christian.

>
> -David
>> +		struct dma_fence *old;
>> +
>> +		rcu_read_lock();
>> +		old = dma_fence_get_rcu_safe(ptr);
>> +		rcu_read_unlock();
>> +
>> +		if (old) {
>> +			r = dma_fence_wait(old, false);
>> +			dma_fence_put(old);
>> +			if (r)
>> +				return r;
>> +		}
>> +	}
>> +
>>   	/* This function can't be called concurrently anyway, otherwise
>>   	 * emitting the fence would mess up the hardware ring buffer.
>>   	 */
>> -	old = rcu_dereference_protected(*ptr, 1);
>> -	if (old && !dma_fence_is_signaled(old)) {
>> -		DRM_INFO("rcu slot is busy\n");
>> -		dma_fence_wait(old, false);
>> -	}
>> -
>>   	rcu_assign_pointer(*ptr, dma_fence_get(&fence->base));
>>
>>   	*f = &fence->base;
>> --
>> 2.17.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx