On Mon, 26 Jun 2023 16:20:53 +0300 Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx> 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.
