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

From: Thomas Zimmermann <tzimmermann@suse.de>
To: "Christian König" <christian.koenig@amd.com>,
	"Daniel Vetter" <daniel@ffwll.ch>
Cc: Dave Airlie <airlied@redhat.com>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Hans de Goede <hdegoede@redhat.com>
Subject: Re: [PATCH 1/8] drm/gem: Write down some rules for vmap usage
Date: Tue, 1 Dec 2020 13:53:56 +0100	[thread overview]
Message-ID: <49c3560d-08f4-f49d-a55b-18ea87b2c2ad@suse.de> (raw)
In-Reply-To: <974e9258-d50c-c6ef-73e8-e5a762a58aa7@suse.de>

[-- Attachment #1.1.1: Type: text/plain, Size: 4223 bytes --]

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. :)

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
> 

-- 
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.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

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