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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 4248 bytes --]

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@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

[-- Attachment #1.1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 7535 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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