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

[Koenig, Christian]

Am 01.04.19 um 16:04 schrieb Zhou, David(ChunMing):
> 在 2019/4/1 21:05, Christian König 写道:
>> Am 01.04.19 um 04:54 schrieb Zhou, David(ChunMing):
>>>> -----Original Message-----
>>>> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of
>>>> Christian K?nig
>>>> Sent: Saturday, March 30, 2019 2:33 AM
>>>> To: amd-gfx at 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 at 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.
> but that is most unlikely case, isn't it?  That ptr is NULL seems only
> when before first fence emitted.

No, the pointer is set to NULL when the fence is processed. See 
amdgpu_fence_process.

Christian.

>
>
> -David
>
>> 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 at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx