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

[Boris Brezillon]

On Fri, 4 Apr 2025 16:58:20 +0200
Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:

> Hi
> 
> Am 04.04.25 um 16:52 schrieb Boris Brezillon:
> > On Fri, 4 Apr 2025 10:01:50 +0200
> > Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
> >  
> >>>> 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.  
> > On a second thought, I think I prefer
> > 'unevictable_count/inc_unevictable()/dec_unevictable()'. But looking at  
> 
> Ok
> 
> > the gem-vram changes you just posted, it looks like gem-shmem is not the
> > only one to use the term 'pin' for this page pinning thing, so if we go
> > and plan for a rename, I'd rather make it DRM-wide than gem-shmem being
> > the outlier yet again :-).  
> 
> Which one do you mean?
> 
> - The code in gem-vram does pinning in the TTM sense of the term.

Sorry, but I still don't see why pinning should be a TTM only thing. If
I read the doc of drm_gem_vram_pin():

"
Pinning a buffer object ensures that it is not evicted from a memory
region. A pinned buffer object has to be unpinned before it can be
pinned to another region. If the pl_flag argument is 0, the buffer is
pinned at its current location (video RAM or system memory).
"

And this pinning is not so different from the pinning we have in
gem-shmem: making buffer object memory unevictable/unmovable.

> 
> - From the client code, the pinning got removed.
> 
> - The GEM pin/unpin callbacks are still there, but the long-term plan is 
> to go to dma-buf pin callbacks AFAICT.
> 
> - Renaming the dma-buf pin/unpin would be a kernel-wide change. Not 
> likely to happen.

Again, I'm not sure why we'd want to do that anyway. Just because the
TTM pinning semantics might be slightly different from the
GEM/dma-buf ones doesn't mean the pinning term should be
prohibited outside the TTM scope. The concept of pinning is pretty
generic and applies to system memory too AFAICT.