[PATCH 4/7] drm/radeon: Pin buffers while they are vmap'ed

[Christian König]

Am 24.11.20 um 14:56 schrieb Thomas Zimmermann:
> Hi
>
> Am 24.11.20 um 14:36 schrieb Christian König:
>> Am 24.11.20 um 13:15 schrieb Thomas Zimmermann:
>>> [SNIP]
>>>>>>> First I wanted to put this into drm_gem_ttm_vmap/vunmap(), but 
>>>>>>> then wondered why ttm_bo_vmap() doe not acquire the lock 
>>>>>>> internally? I'd expect that vmap/vunmap are close together and 
>>>>>>> do not overlap for the same BO. 
>>>>>>
>>>>>> We have use cases like the following during command submission:
>>>>>>
>>>>>> 1. lock
>>>>>> 2. map
>>>>>> 3. copy parts of the BO content somewhere else or patch it with 
>>>>>> additional information
>>>>>> 4. unmap
>>>>>> 5. submit BO to the hardware
>>>>>> 6. add hardware fence to the BO to make sure it doesn't move
>>>>>> 7. unlock
>>>>>>
>>>>>> That use case won't be possible with vmap/vunmap if we move the 
>>>>>> lock/unlock into it and I hope to replace the kmap/kunmap 
>>>>>> functions with them in the near term.
>>>>>>
>>>>>>> Otherwise, acquiring the reservation lock would require another 
>>>>>>> ref-counting variable or per-driver code.
>>>>>>
>>>>>> Hui, why that? Just put this into drm_gem_ttm_vmap/vunmap() 
>>>>>> helper as you initially planned.
>>>>>
>>>>> Given your example above, step one would acquire the lock, and 
>>>>> step two would also acquire the lock as part of the vmap 
>>>>> implementation. Wouldn't this fail (At least during unmap or 
>>>>> unlock steps) ?
>>>>
>>>> Oh, so you want to nest them? No, that is a rather bad no-go.
>>>
>>> I don't want to nest/overlap them. My question was whether that 
>>> would be required. Apparently not.
>>>
>>> While the console's BO is being set for scanout, it's protected from 
>>> movement via the pin/unpin implementation, right?
>>
>> Yes, correct.
>>
>>> The driver does not acquire the resv lock for longer periods. I'm 
>>> asking because this would prevent any console-buffer updates while 
>>> the console is being displayed.
>>
>> Correct as well, we only hold the lock for things like command 
>> submission, pinning, unpinning etc etc....
>>
>
> Thanks for answering my questions.
>
>>>
>>>>
>>>> You need to make sure that the lock is only taken from the FB path 
>>>> which wants to vmap the object.
>>>>
>>>> Why don't you lock the GEM object from the caller in the generic FB 
>>>> implementation?
>>>
>>> With the current blitter code, it breaks abstraction. if vmap/vunmap 
>>> hold the lock implicitly, things would be easier.
>>
>> Do you have a link to the code?
>
> It's the damage blitter in the fbdev code. [1] While it flushes the 
> shadow buffer into the BO, the BO has to be kept in place. I already 
> changed it to lock struct drm_fb_helper.lock, but I don't think this 
> is enough. TTM could still evict the BO concurrently.

Yeah, that's correct.

But I still don't fully understand the problem. You just need to change 
the code like this:

     mutex_lock(&fb_helper->lock);
     dma_resv_lock(buffer->gem->resv, NULL);

     ret = drm_client_buffer_vmap(buffer, &map);
     if (ret)
         goto out;

     dst = map;
     drm_fb_helper_damage_blit_real(fb_helper, clip, &dst);

     drm_client_buffer_vunmap(buffer);

out:
     dma_resv_unlock(buffer->gem->resv);
     mutex_unlock(&fb_helper->lock);


You could abstract that in drm_client functions as well, but I don't 
really see the value in that.

Regards,
Christian.

> There's no recursion taking place, so I guess the reservation lock 
> could be acquired/release in drm_client_buffer_vmap/vunmap(), or a 
> separate pair of DRM client functions could do the locking.
>
> Best regards
> Thomas
>
> [1] 
> https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/drm_fb_helper.c?id=ac60f3f3090115d21f028bffa2dcfb67f695c4f2#n394
>
>>
>> Please note that the reservation lock you need to take here is part 
>> of the GEM object.
>>
>> Usually we design things in the way that the code needs to take a 
>> lock which protects an object, then do some operations with the 
>> object and then release the lock again.
>>
>> Having in the lock inside the operation can be done as well, but 
>> returning with it is kind of unusual design.
>>
>>> Sorry for the noob questions. I'm still trying to understand the 
>>> implications of acquiring these locks.
>>
>> Well this is the reservation lock of the GEM object we are talking 
>> about here. We need to take that for a couple of different 
>> operations, vmap/vunmap doesn't sound like a special case to me.
>>
>> Regards,
>> Christian.
>>
>>>
>>> Best regards
>>> Thomas
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>