[PATCH 1/3] drm/nouveau: wait for moving fence after pinning

[Christian König]

Am 22.06.21 um 11:20 schrieb Daniel Vetter:
> On Mon, Jun 21, 2021 at 5:53 PM Daniel Vetter <daniel at ffwll.ch> wrote:
>> On Mon, Jun 21, 2021 at 5:49 PM Christian König
>> <ckoenig.leichtzumerken at gmail.com> wrote:
>>> Am 21.06.21 um 16:54 schrieb Daniel Vetter:
>>>> On Mon, Jun 21, 2021 at 03:03:26PM +0200, Christian König wrote:
>>>>> We actually need to wait for the moving fence after pinning
>>>>> the BO to make sure that the pin is completed.
>>>>>
>>>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>>>> CC: stable at kernel.org
>>>>> ---
>>>>>    drivers/gpu/drm/nouveau/nouveau_prime.c | 8 +++++++-
>>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c
>>>>> index 347488685f74..591738545eba 100644
>>>>> --- a/drivers/gpu/drm/nouveau/nouveau_prime.c
>>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c
>>>>> @@ -93,7 +93,13 @@ int nouveau_gem_prime_pin(struct drm_gem_object *obj)
>>>>>       if (ret)
>>>>>               return -EINVAL;
>>>>>
>>>>> -    return 0;
>>>>> +    if (nvbo->bo.moving) {
>>>> Don't we need to hold the dma_resv to read this? We can grab a reference
>>>> and then unlock, but I think just unlocked wait can go boom pretty easily
>>>> (since we don't hold a reference or lock so someone else can jump in and
>>>> free the moving fence).
>>> The moving fence is only modified while the BO is moved and since we
>>> have just successfully pinned it....
>> Yeah  ... so probably correct, but really tricky. Just wrapping a
>> ttm_bo_reserve/unreserve around the code you add should be enough and
>> get the job done?
> I think you distracted me a bit with the "it can't move", so yes
> there's a guarantee that no other fence can show up in ttm_bo->moving
> and confuse us. But it could get set to NULL because someone realized
> it signalled. We're not doing that systematically, but relying on
> fences never getting garbage-collected for correctness isn't great.

Yeah, that's what I essentially meant with it would be better in general 
to take the lock.

>
> Sot the ttm_bo_reserve/unreserve is definitely needed here around this
> bit of code. You don't need to merge it with the reserve/unreserve in
> the pin function though, it's just to protect against the
> use-after-free.

Ah, yes good point. That means I don't need to change the pin/unpin 
functions in nouveau at all.


BTW: What do you think of making dma_fence_is_signaled() and 
dma_fence_wait_timeout() save to passing in NULL as fence?

I think we have a lot of cases where we check "!fence || 
dma_fence_is_signaled(fence)" or similar.

Christian.

> -Daniel
>
>>> But in general I agree that it would be better to avoid this. I just
>>> didn't wanted to open a bigger can of worms by changing nouveau so much.
>> Yeah, but I'm kinda thinking of some helpers to wait for the move
>> fence (so that later on we can switch from having the exclusive fence
>> to the move fence do that, maybe). And then locking checks in there
>> would be nice.
>>
>> Also avoids the case of explaining why lockless here is fine, but
>> lockless wait for the exclusive fence in e.g. a dynami dma-buf
>> importer is very much not fine at all. Just all around less trouble.
>> -Daniel
>>
>>> Christian.
>>>
>>>> -Daniel
>>>>
>>>>> +            ret = dma_fence_wait(nvbo->bo.moving, true);
>>>>> +            if (ret)
>>>>> +                    nouveau_bo_unpin(nvbo);
>>>>> +    }
>>>>> +
>>>>> +    return ret;
>>>>>    }
>>>>>
>>>>>    void nouveau_gem_prime_unpin(struct drm_gem_object *obj)
>>>>> --
>>>>> 2.25.1
>>>>>
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
>
>