[PATCH 1/8] drm/gem: Write down some rules for vmap usage

[Thomas Zimmermann]

Hi

Am 01.12.20 um 13:38 schrieb Christian König:
> Am 01.12.20 um 13:33 schrieb Thomas Zimmermann:
>> Hi
>>
>> Am 01.12.20 um 13:14 schrieb Christian König:
>>> Am 01.12.20 um 12:30 schrieb Thomas Zimmermann:
>>>> Hi
>>>>
>>>> Am 01.12.20 um 11:34 schrieb Christian König:
>>>>>> [...]
>>>>>> In patch 6 of this series, there's ast cursor code that acquires 
>>>>>> two BO's reservation locks and vmaps them afterwards. That's 
>>>>>> probably how you intend to use dma_buf_vmap_local.
>>>>>>
>>>>>> However, I think it's more logically to have a vmap callback that 
>>>>>> only does the actual vmap. This is all that exporters would have 
>>>>>> to implement.
>>>>>>
>>>>>> And with that, build one helper that pins before vmap and one 
>>>>>> helper that gets the resv lock.
>>>>>
>>>>> I don't think that this is will work nor is it a good approach.
>>>>>
>>>>> See the ast cursor handling for example. You need to acquire two 
>>>>> BOs here, not just one. And this can't be done cleanly with a 
>>>>> single vmap call.
>>>>
>>>> That seems to be a misunderstanding.
>>>>
>>>> I don't mentioned it explicitly, but there's of course another 
>>>> helper that only vmaps and nothing else. This would be useful for 
>>>> cases like the cursor code. So there would be:
>>>>
>>>>  dma_buf_vmap() - pin + vmap
>>>>  dma_buf_vmap_local() - lock + vmap
>>>>  dma_buf_vmap_locked() - only vmap; caller has set up the BOs
>>>
>>> Well that zoo of helpers will certainly get a NAK from my side.
>>>
>>> See interfaces like this should implement simple functions and not 
>>> hide what's actually needs to be done inside the drivers using this 
>>> interface.
>>
>> If 9 of 10 invocations use the same pattern, why not put that pattern 
>> in a helper? I see nothing wrong with that.
> 
> Because it hides the locking semantics inside the helper. See when you 
> have the lock/unlock inside the driver it is obvious that you need to be 
> careful not to take locks in different orders.
> 
>>> What we could do is to add a pin count to the DMA-buf and then do 
>>> WARN_ON(dma_buf->pin_count || dma_resv_lock_help(dma_buf->resv)) in 
>>> the vmap/vunmap calls.
>>
>> Most of the vmap code is either CMA or SHMEM GEM stuff. They don't 
>> need to pin. It's just baggage to them. The TTM stuff that does need 
>> pinning is the minority.
>>
>>>
>>>>
>>>> I did some conversion of drivers that use vram and shmem. They 
>>>> occasionally update a buffer (ast cursors) or flush a BO from system 
>>>> memory to HW (udl, cirrus, mgag200). In terms of these 3 interfaces: 
>>>> I never needed dma_buf_vmap() because pinning was never really 
>>>> required here. Almost all of the cases were handled by 
>>>> dma_buf_vmap_local(). And the ast cursor code uses the equivalent of 
>>>> dma_buf_vmap_locked().
>>>
>>> Yeah, that is kind of expected. I was already wondering as well why 
>>> we didn't used the reservation lock more extensively.
>>
>> As a side note, I found only 6 trivial implementations of vmap outside 
>> of drivers/gpu/drm. I cannot find a single implementation of pin 
>> there.  What am I missing?
> 
> Amdgpu is the only one currently implementing the new interface. So far 
> we didn't had the time nor the need to correctly move the locking into 
> the calling drivers.
> 
> That's what the whole dynamic DMA-buf patches where all about.

Thanks for the pointer.

Best regards
Thomas

> 
> 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_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20201201/a6a57a39/attachment-0001.sig>