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

["Christian König" <christian.koenig@amd.com>]

Am 01.12.20 um 13:53 schrieb Thomas Zimmermann:
> Hi
>
> Am 01.12.20 um 13:51 schrieb 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.
>
> That was not a snarky comment, although it might sound like one. I 
> found the series in my inbox. :)

It wasn't recognized as such. And just to be clear your work here is 
extremely welcomed.

Regards,
Christian.

>
> Best regards
> Thomas
>
>>
>> Best regards
>> Thomas
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Best regards
>>>> Thomas
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel