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

[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.

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

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_0x680DC11D530B7A23.asc
Type: application/pgp-keys
Size: 7435 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20201124/c2cf0985/attachment-0001.key>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20201124/c2cf0985/attachment-0001.sig>