Message-ID: <20250407115654.761ddaa6@collabora.com>
Date: Mon, 7 Apr 2025 11:56:54 +0200
From: Boris Brezillon <boris.brezillon@...labora.com>
To: Thomas Zimmermann <tzimmermann@...e.de>
Cc: Dmitry Osipenko <dmitry.osipenko@...labora.com>, David Airlie
 <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, Maarten Lankhorst
 <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>,
 Christian König <christian.koenig@....com>, Gerd Hoffmann
 <kraxel@...hat.com>, Qiang Yu <yuq825@...il.com>, Steven Price
 <steven.price@....com>, Frank Binns <frank.binns@...tec.com>, Matt Coster
 <matt.coster@...tec.com>, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org, kernel@...labora.com
Subject: Re: [PATCH v20 09/10] drm/shmem-helper: Switch
 drm_gem_shmem_vmap/vunmap to use pin/unpin

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

> Hi
> 
> Am 04.04.25 um 16:52 schrieb Boris Brezillon:
> > On Fri, 4 Apr 2025 10:01:50 +0200
> > Thomas Zimmermann <tzimmermann@...e.de> 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.

Powered by blists - more mailing lists