Re: [PATCH 1/5] drm/panfrost: Stop using drm_gem_shmem_put_pages()

[Boris Brezillon <boris.brezillon@collabora.com>]

On Mon, 26 Jun 2023 16:20:53 +0300
Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote:

> On 6/26/23 15:02, Boris Brezillon wrote:
> > -err_pages:
> > -	drm_gem_shmem_put_pages(&bo->base);
> >  err_unlock:
> >  	dma_resv_unlock(obj->resv);
> > +
> > +	if (ret && pinned)
> > +		drm_gem_shmem_unpin(&bo->base);  
> 
> The drm_gem_shmem_unpin() was supposed to be used only in conjunction
> with drm_gem_shmem_pin(). I've a pending patch to enable the pin/unpin
> refcounting needed by drm-shmem shrinker, it will prohibit invocation of
> unpin without a previous pin.

That driver is a bit special in that, in the growable BO case
(AKA pin-on-demand), the driver replaces the drm_gem_shmem_pin()
implementation by a custom one (the logic in
panfrost_mmu_map_fault_addr()), but still relies on the
default implementation to release things. We do increment the
pages_use_count manually to make sure the drm_gem_shmem_unpin() is
balanced.

> 
> I'm wondering whether it will be okay to simply remove
> drm_gem_shmem_put_pages() from the Panfrost code, letting pages to be
> kept allocated in a error case. They will be freed once BO is destroyed.
> 

I'm pretty sure the implementation will then complain about unbalanced
pin/unamp (or get_pages/put_pages) if we do that. I guess one option
would be to completely bypass drm_gem_shmem_[un]pin() for growable BOs
and manage the pages separately at the panfrost_gem_object level, but
the original idea was probably to re-use some of the fields/logic we
had in drm_gem_shmem_object and make partial pinning as close as
possible to regular pinning. Another option would be to teach the shmem
about partial pinning, but I'm not sure we want to expose such a
feature.