Re: [PATCH v18 15/26] drm/panfrost: Explicitly get and put drm-shmem pages

[Boris Brezillon]

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