On Thu, 23 Nov 2023 15:48:48 +0100 Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote: > On Mon, 30 Oct 2023 02:01:54 +0300 > Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx> wrote: > > > To simplify the drm-shmem refcnt handling, we're moving away from > > the implicit get_pages() that is used by get_pages_sgt(). From now on > > drivers will have to pin pages while they use sgt. Panfrost's shrinker > > doesn't support swapping out BOs, hence pages are pinned and sgt is valid > > as long as pages' use-count > 0. > > > > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx> > > --- > > drivers/gpu/drm/panfrost/panfrost_gem.c | 17 +++++++++++++++++ > > drivers/gpu/drm/panfrost/panfrost_mmu.c | 6 ++---- > > 2 files changed, 19 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c > > index 6b77d8cebcb2..bb9d43cf7c3c 100644 > > --- a/drivers/gpu/drm/panfrost/panfrost_gem.c > > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c > > @@ -47,8 +47,13 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj) > > } > > } > > kvfree(bo->sgts); > > + > > + drm_gem_shmem_put_pages(&bo->base); > > } > > > > + if (!bo->is_heap && !obj->import_attach) > > + drm_gem_shmem_put_pages(&bo->base); > > + > > drm_gem_shmem_free(&bo->base); > > } > > > > @@ -269,6 +274,7 @@ panfrost_gem_create(struct drm_device *dev, size_t size, u32 flags) > > { > > struct drm_gem_shmem_object *shmem; > > struct panfrost_gem_object *bo; > > + int err; > > > > /* Round up heap allocations to 2MB to keep fault handling simple */ > > if (flags & PANFROST_BO_HEAP) > > @@ -282,7 +288,18 @@ panfrost_gem_create(struct drm_device *dev, size_t size, u32 flags) > > bo->noexec = !!(flags & PANFROST_BO_NOEXEC); > > bo->is_heap = !!(flags & PANFROST_BO_HEAP); > > > > + if (!bo->is_heap) { > > + err = drm_gem_shmem_get_pages(shmem); > > I really hate the fact we request pages here while we call > panfrost_mmu_map() in panfrost_gem_open(), because ultimately, pages > are requested for the MMU mapping. Also hate the quirk we have in shmem > to call free_pages() instead of put_pages_locked() when the BO refcount > dropped to zero, and I was hoping we could get rid of it at some point > by teaching drivers to request pages when they actually need it instead > of tying pages lifetime to the GEM object lifetime. > > Maybe what we should do instead is move the get/put_pages() in > panfrost_mmu_map/unmap() (as I suggested), but have a special mapping > panfrost_mmu_evict/restore() helpers that kill/restore the MMU mappings > without releasing/acquiring the pages ref. Okay, so I played with your branch and did what I suggested here ^. The end result is available here [1]. I also split this patch in two: - A fix for the error path in panfrost_mmu_map_fault_addr() [2] - The explicit get/put_pages() stuff with pages ref owned by the panfrost_gem_mapping object [3] [1]https://gitlab.freedesktop.org/bbrezillon/linux/-/commits/virtio-gpu-shrinker-v18 [2]https://gitlab.freedesktop.org/bbrezillon/linux/-/commit/9d499e971fdae4d6e52f8871ca27c24b2a2c43d6 [3]https://gitlab.freedesktop.org/bbrezillon/linux/-/commit/ba3de65bf1cf0ca95710e743ec85ca67ff1aa223