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
