Re: [PATCH v20 09/10] drm/shmem-helper: Switch drm_gem_shmem_vmap/vunmap to use pin/unpin

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 03.04.25 um 10:50 schrieb Boris Brezillon:
> On Thu, 3 Apr 2025 09:20:00 +0200
> Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
>> Hi
>>
>> Am 02.04.25 um 15:21 schrieb Boris Brezillon:
>>> On Wed, 2 Apr 2025 15:58:55 +0300
>>> Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote:
>>>   
>>>> On 4/2/25 15:47, Thomas Zimmermann wrote:
>>>>> Hi
>>>>>
>>>>> Am 22.03.25 um 22:26 schrieb Dmitry Osipenko:
>>>>>> The vmapped pages shall be pinned in memory and previously get/
>>>>>> put_pages()
>>>>>> were implicitly hard-pinning/unpinning the pages. This will no longer be
>>>>>> the case with addition of memory shrinker because pages_use_count > 0
>>>>>> won't
>>>>>> determine anymore whether pages are hard-pinned (they will be soft-
>>>>>> pinned),
>>>>>> while the new pages_pin_count will do the hard-pinning. Switch the
>>>>>> vmap/vunmap() to use pin/unpin() functions in a preparation of addition
>>>>>> of the memory shrinker support to drm-shmem.
>>>>> I've meanwhile rediscovered this patch and I'm sure this is not correct.
>>>>> Vmap should not pin AFAIK. It is possible to vmap if the buffer has been
>>>>> pinned, but that's not automatic.  For other vmaps it is necessary to
>>>>> hold the reservation lock to prevent the buffer from moving.
>>> Hm, is this problematic though? If you want to vmap() inside a section
>>> that's protected by the resv lock, you can
>>>
>>> - drm_gem_shmem_vmap_locked()
>>> - do whatever you need to do with the vaddr,
>>> - drm_gem_shmem_vunmap_locked()
>>>
>>> and the {pin,page_use}_count will be back to their original values.
>>> Those are just ref counters, and I doubt the overhead of
>>> incrementing/decrementing them makes a difference compared to the heavy
>>> page-allocation/vmap operations...
>> I once tried to add pin as part of vmap, so that pages stay in place.
>> Christian was very clear about not doing this. I found this made a lot
>> of sense: vmap means "make the memory available to the CPU". The memory
>> location doesn't matter much here. Pin means something like "make the
>> memory available to the GPU". But which GPU depends on the caller: calls
>> via GEM refer to the local GPU, calls via dma-buf usually refer to the
>> importer's GPU. That GPU uncertainty makes pin problematic already.
> Okay, so it looks more like a naming issue then. The intent here is to

It's certainly possible to see this as a problem naming.

> make sure the page array doesn't disappear while we have a kernel
> mapping active (address returned by vmap()). The reason we went from
> pages_count to pages_use_count+pin_count is because we have two kind of
> references in drm_gem_shmem:
>
> - weak references (tracked with pages_use_count). Those are
>    usually held by GPU VMs, and they are weak in the sense they
>    shouldn't prevent the shrinker to reclaim them if the GPU VM is idle.
>    The other user of weak references is userspace mappings of GEM
>    objects (mmap()), because then we can repopulate those with our fault
>    handler.
> - hard references (tracked with pin_count) which are used to prevent
>    the shrinker from even considering the GEM as reclaimable. And clearly
>    kernel mappings fall in that case, because otherwise we could reclaim
>    pages that might be dereferenced by the CPU later on. It's also used
>    to implement drm_gem_pin because it's the same mechanism really,
>    hence the name

Yeah, this should be rename IMHO. Pin is a TTM operation that fixes 
buffers in certain locations. Drivers do this internally. It has nothing 
to do with gem-shmem.

There's also a pin operation in GEM BOs' drm_gem_object_funcs, but it is 
only called for PRIME-exported buffers and not for general use. For 
gem-shmem, the callback would be implemented on top of the hard references.

And there's also a pin in dma_buf_ops. The term 'pin' is somewhat 
overloaded already.

>
>> In your case, vmap an pin both intent to hold the shmem pages in memory.
>> They might be build on top of the same implementation, but one should
>> not be implemented with the other because of their different meanings.
> But that's not what we do, is it? Sure, in drm_gem_shmem_vmap_locked(),
> we call drm_gem_shmem_pin_locked(), but that's an internal function to
> make sure the pages are allocated and stay around until
> drm_gem_shmem_vunmap_locked() is called.
>
> I guess we could rename pin_count into hard_refcount or
> page_residency_count or xxx_count, and change the pin/unpin_locked()
> function names accordingly, but that's just a naming detail, it doesn't
> force you to call drm_gem_pin() to vmap() your GEM, it's something we
> do internally.

Such a rename would be much appreciated. page_residency_count seems 
appropriate.

>
>> More generally speaking, I've meanwhile come to the conclusion that pin
>> should not even exist in the GEM interface. It's an internal operation
>> of TTM and reveals too much about what happens within the
>> implementation. Instead GEM should be free to move buffers around.
> Well, yes and no. There are situations where you simply can't move
> things around if there are active users, and vmap() is one of those
> AFAICT.

Sure. What I mean here is that pin/unpin is something of an 
implementation detail. IMHO the pin/unpin callbacks in 
drm_gem_object_funcs should get different names, such as pin_exported. 
They are not for general use.

>   
>> Dma-buf importers should only tell exporters to make buffers available
>> to them, but not how to do this. AFAIK that's what dma-buf's
>> attach/detach is for.
> And that's what they do, no? attach() tells the exporter to give the
> importer a way to access those buffers, and given the exporter has no
> clue about when/how the exporter will access those, there's no other way
> but to pin the pages. Am I missing something here?

Yeah, that's what they do.

Best regards
Thomas


-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)