| Message ID | 20231029230205.93277-16-dmitry.osipenko@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add generic memory shrinker to VirtIO-GPU and Panfrost DRM drivers | expand |
On Mon, 30 Oct 2023 02:01:54 +0300 Dmitry Osipenko <dmitry.osipenko@collabora.com> 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@collabora.com> > --- > 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); Hm, there was no drm_gem_shmem_get_pages_sgt() call here, why should we add a drm_gem_shmem_get_pages()? What we should do instead is add a drm_gem_shmem_get_pages() for each drm_gem_shmem_get_pages_sgt() we have in the driver (in panfrost_mmu_map()), and add drm_gem_shmem_put_pages() calls where they are missing (panfrost_mmu_unmap()). > + if (err) > + goto err_free; > + } > + > return bo; > + > +err_free: > + drm_gem_shmem_free(&bo->base); > + > + return ERR_PTR(err); > } > > struct drm_gem_object * > diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c > index 770dab1942c2..ac145a98377b 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c > +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c > @@ -504,7 +504,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, > if (IS_ERR(pages[i])) { > ret = PTR_ERR(pages[i]); > pages[i] = NULL; > - goto err_pages; > + goto err_unlock; > } > } > > @@ -512,7 +512,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, > ret = sg_alloc_table_from_pages(sgt, pages + page_offset, > NUM_FAULT_PAGES, 0, SZ_2M, GFP_KERNEL); > if (ret) > - goto err_pages; > + goto err_unlock; Feels like the panfrost_gem_mapping object should hold a ref on the BO pages, not the BO itself, because, ultimately, the user of the BO is the GPU. This matches what I was saying about moving get/put_pages() to panfrost_mmu_map/unmap(): everytime a panfrost_gem_mapping becomes active, to want to take a pages ref, every time it becomes inactive, you should release the pages ref. > > ret = dma_map_sgtable(pfdev->dev, sgt, DMA_BIDIRECTIONAL, 0); > if (ret) > @@ -535,8 +535,6 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, > > err_map: > sg_free_table(sgt); > -err_pages: > - drm_gem_shmem_put_pages_locked(&bo->base); > err_unlock: > dma_resv_unlock(obj->resv); > err_bo:
On 11/10/23 13:53, Boris Brezillon wrote: > Hm, there was no drm_gem_shmem_get_pages_sgt() call here, why should we > add a drm_gem_shmem_get_pages()? What we should do instead is add a > drm_gem_shmem_get_pages() for each drm_gem_shmem_get_pages_sgt() we > have in the driver (in panfrost_mmu_map()), and add > drm_gem_shmem_put_pages() calls where they are missing > (panfrost_mmu_unmap()). > >> + if (err) >> + goto err_free; >> + } >> + >> return bo; >> + >> +err_free: >> + drm_gem_shmem_free(&bo->base); >> + >> + return ERR_PTR(err); >> } >> >> struct drm_gem_object * >> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c >> index 770dab1942c2..ac145a98377b 100644 >> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c >> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c >> @@ -504,7 +504,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, >> if (IS_ERR(pages[i])) { >> ret = PTR_ERR(pages[i]); >> pages[i] = NULL; >> - goto err_pages; >> + goto err_unlock; >> } >> } >> >> @@ -512,7 +512,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, >> ret = sg_alloc_table_from_pages(sgt, pages + page_offset, >> NUM_FAULT_PAGES, 0, SZ_2M, GFP_KERNEL); >> if (ret) >> - goto err_pages; >> + goto err_unlock; > Feels like the panfrost_gem_mapping object should hold a ref on the BO > pages, not the BO itself, because, ultimately, the user of the BO is > the GPU. This matches what I was saying about moving get/put_pages() to > panfrost_mmu_map/unmap(): everytime a panfrost_gem_mapping becomes > active, to want to take a pages ref, every time it becomes inactive, > you should release the pages ref. The panfrost_mmu_unmap() is also used by shrinker when BO is purged. I'm unhappy with how icky it all becomes if unmap is made to put pages. Previously map() was implicitly allocating pages with get_sgt() and then pages were implicitly released by drm_gem_shmem_free(). A non-heap BO is mapped when it's created by Panfrost, hence the actual lifetime of pages is kept unchanged by this patch. The implicit allocation is turned into explicit one, i.e. pages are explicitly allocated before BO is mapped.
On Thu, 23 Nov 2023 01:04:56 +0300 Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > On 11/10/23 13:53, Boris Brezillon wrote: > > Hm, there was no drm_gem_shmem_get_pages_sgt() call here, why should we > > add a drm_gem_shmem_get_pages()? What we should do instead is add a > > drm_gem_shmem_get_pages() for each drm_gem_shmem_get_pages_sgt() we > > have in the driver (in panfrost_mmu_map()), and add > > drm_gem_shmem_put_pages() calls where they are missing > > (panfrost_mmu_unmap()). > > > >> + if (err) > >> + goto err_free; > >> + } > >> + > >> return bo; > >> + > >> +err_free: > >> + drm_gem_shmem_free(&bo->base); > >> + > >> + return ERR_PTR(err); > >> } > >> > >> struct drm_gem_object * > >> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c > >> index 770dab1942c2..ac145a98377b 100644 > >> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c > >> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c > >> @@ -504,7 +504,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, > >> if (IS_ERR(pages[i])) { > >> ret = PTR_ERR(pages[i]); > >> pages[i] = NULL; > >> - goto err_pages; > >> + goto err_unlock; > >> } > >> } > >> > >> @@ -512,7 +512,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, > >> ret = sg_alloc_table_from_pages(sgt, pages + page_offset, > >> NUM_FAULT_PAGES, 0, SZ_2M, GFP_KERNEL); > >> if (ret) > >> - goto err_pages; > >> + goto err_unlock; > > Feels like the panfrost_gem_mapping object should hold a ref on the BO > > pages, not the BO itself, because, ultimately, the user of the BO is > > the GPU. This matches what I was saying about moving get/put_pages() to > > panfrost_mmu_map/unmap(): everytime a panfrost_gem_mapping becomes > > active, to want to take a pages ref, every time it becomes inactive, > > you should release the pages ref. > > The panfrost_mmu_unmap() is also used by shrinker when BO is purged. I'm > unhappy with how icky it all becomes if unmap is made to put pages. Why, that's exactly what's supposed to happen. If you mmu_unmap(), that means you no longer need the pages ref you got. > > Previously map() was implicitly allocating pages with get_sgt() and then > pages were implicitly released by drm_gem_shmem_free(). A non-heap BO is > mapped when it's created by Panfrost, hence the actual lifetime of pages > is kept unchanged by this patch. But the whole point of making it explicit is to control when pages are needed or not, isn't it. The fact we mmu_map() the BO at open time, and keep it mapped until it's not longer referenced is an implementation choice, and I don't think having pages_put() in mmu_unmap() changes that. > The implicit allocation is turned into > explicit one, i.e. pages are explicitly allocated before BO is mapped. >
On 11/23/23 12:05, Boris Brezillon wrote: > On Thu, 23 Nov 2023 01:04:56 +0300 > Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > >> On 11/10/23 13:53, Boris Brezillon wrote: >>> Hm, there was no drm_gem_shmem_get_pages_sgt() call here, why should we >>> add a drm_gem_shmem_get_pages()? What we should do instead is add a >>> drm_gem_shmem_get_pages() for each drm_gem_shmem_get_pages_sgt() we >>> have in the driver (in panfrost_mmu_map()), and add >>> drm_gem_shmem_put_pages() calls where they are missing >>> (panfrost_mmu_unmap()). >>> >>>> + if (err) >>>> + goto err_free; >>>> + } >>>> + >>>> return bo; >>>> + >>>> +err_free: >>>> + drm_gem_shmem_free(&bo->base); >>>> + >>>> + return ERR_PTR(err); >>>> } >>>> >>>> struct drm_gem_object * >>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c >>>> index 770dab1942c2..ac145a98377b 100644 >>>> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c >>>> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c >>>> @@ -504,7 +504,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, >>>> if (IS_ERR(pages[i])) { >>>> ret = PTR_ERR(pages[i]); >>>> pages[i] = NULL; >>>> - goto err_pages; >>>> + goto err_unlock; >>>> } >>>> } >>>> >>>> @@ -512,7 +512,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, >>>> ret = sg_alloc_table_from_pages(sgt, pages + page_offset, >>>> NUM_FAULT_PAGES, 0, SZ_2M, GFP_KERNEL); >>>> if (ret) >>>> - goto err_pages; >>>> + goto err_unlock; >>> Feels like the panfrost_gem_mapping object should hold a ref on the BO >>> pages, not the BO itself, because, ultimately, the user of the BO is >>> the GPU. This matches what I was saying about moving get/put_pages() to >>> panfrost_mmu_map/unmap(): everytime a panfrost_gem_mapping becomes >>> active, to want to take a pages ref, every time it becomes inactive, >>> you should release the pages ref. >> >> The panfrost_mmu_unmap() is also used by shrinker when BO is purged. I'm >> unhappy with how icky it all becomes if unmap is made to put pages. > > Why, that's exactly what's supposed to happen. If you mmu_unmap(), that > means you no longer need the pages ref you got. The drm_gem_shmem_purge() frees the pages. If mmu_unmap() frees pages too, then it becomes odd for drm_gem_shmem_purge() that it needs to free pages that were already freed. >> Previously map() was implicitly allocating pages with get_sgt() and then >> pages were implicitly released by drm_gem_shmem_free(). A non-heap BO is >> mapped when it's created by Panfrost, hence the actual lifetime of pages >> is kept unchanged by this patch. > > But the whole point of making it explicit is to control when pages are > needed or not, isn't it. The fact we mmu_map() the BO at open time, and > keep it mapped until it's not longer referenced is an implementation > choice, and I don't think having pages_put() in mmu_unmap() changes > that. Previously, when the last mmu_unmap() was done, the pages were not released. If you'll make unmap to put pages, then you can't map BO again because pages are released by the last put() of unmap. In order to keep the old pages allocation logic unchanged, the pages must be referenced while BO is alive, not while mapping is alive. Technically, the code can be changed to put pages on unmap. This requires adding special quirk to drm_gem_shmem_purge() and then for Panfrost pages should have the same lifetime as BO, hence why bother? diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index 5ee98b6f0c94..5492610802a1 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -600,7 +600,9 @@ drm_gem_shmem_shrinker_put_pages_locked(struct drm_gem_shmem_object *shmem) if (shmem->evicted) return; - drm_gem_shmem_free_pages(shmem); + if (refcount_read(&shmem->pages_use_count)) + drm_gem_shmem_free_pages(shmem); + drm_vma_node_unmap(&obj->vma_node, dev->anon_inode->i_mapping); } @@ -608,7 +610,8 @@ void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem) { struct drm_gem_object *obj = &shmem->base; - drm_WARN_ON(obj->dev, !drm_gem_shmem_is_purgeable(shmem)); + if (refcount_read(&shmem->pages_use_count)) + drm_WARN_ON(obj->dev, !drm_gem_shmem_is_purgeable(shmem)); drm_gem_shmem_shrinker_put_pages_locked(shmem); drm_gem_free_mmap_offset(obj); diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c index a6128e32f303..499964c43a7b 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gem.c +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c @@ -41,9 +41,6 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj) 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); } @@ -302,12 +299,6 @@ 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); - if (err) - goto err_free; - } - return bo; err_free: diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c index 01cd97011ea5..4ed4ba5df420 100644 --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c @@ -335,9 +335,13 @@ int panfrost_mmu_map(struct panfrost_gem_mapping *mapping) prot |= IOMMU_NOEXEC; if (!obj->import_attach) { - ret = drm_gem_shmem_pin(shmem); + ret = drm_gem_shmem_get_pages(shmem); if (ret) return ret; + + ret = drm_gem_shmem_pin(shmem); + if (ret) + goto put_pages; } sgt = drm_gem_shmem_get_pages_sgt(shmem); @@ -349,9 +353,18 @@ int panfrost_mmu_map(struct panfrost_gem_mapping *mapping) mmu_map_sg(pfdev, mapping->mmu, mapping->mmnode.start << PAGE_SHIFT, prot, sgt); mapping->active = true; + + if (!obj->import_attach) + drm_gem_shmem_unpin(shmem); + + return 0; + unpin: if (!obj->import_attach) drm_gem_shmem_unpin(shmem); +put_pages: + if (!obj->import_attach) + drm_gem_shmem_put_pages(shmem); return ret; } @@ -360,6 +373,7 @@ void panfrost_mmu_unmap(struct panfrost_gem_mapping *mapping) { struct panfrost_gem_object *bo = mapping->obj; struct drm_gem_object *obj = &bo->base.base; + struct drm_gem_shmem_object *shmem = &bo->base; struct panfrost_device *pfdev = to_panfrost_device(obj->dev); struct io_pgtable_ops *ops = mapping->mmu->pgtbl_ops; u64 iova = mapping->mmnode.start << PAGE_SHIFT; @@ -389,6 +403,9 @@ void panfrost_mmu_unmap(struct panfrost_gem_mapping *mapping) panfrost_mmu_flush_range(pfdev, mapping->mmu, mapping->mmnode.start << PAGE_SHIFT, len); mapping->active = false; + + if (!bo->is_heap && !obj->import_attach) + drm_gem_shmem_put_pages_locked(shmem); } static void mmu_tlb_inv_context_s1(void *cookie)
On Thu, 23 Nov 2023 15:24:32 +0300 Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > On 11/23/23 12:05, Boris Brezillon wrote: > > On Thu, 23 Nov 2023 01:04:56 +0300 > > Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > > > >> On 11/10/23 13:53, Boris Brezillon wrote: > >>> Hm, there was no drm_gem_shmem_get_pages_sgt() call here, why > >>> should we add a drm_gem_shmem_get_pages()? What we should do > >>> instead is add a drm_gem_shmem_get_pages() for each > >>> drm_gem_shmem_get_pages_sgt() we have in the driver (in > >>> panfrost_mmu_map()), and add drm_gem_shmem_put_pages() calls > >>> where they are missing (panfrost_mmu_unmap()). > >>> > >>>> + if (err) > >>>> + goto err_free; > >>>> + } > >>>> + > >>>> return bo; > >>>> + > >>>> +err_free: > >>>> + drm_gem_shmem_free(&bo->base); > >>>> + > >>>> + return ERR_PTR(err); > >>>> } > >>>> > >>>> struct drm_gem_object * > >>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c > >>>> b/drivers/gpu/drm/panfrost/panfrost_mmu.c index > >>>> 770dab1942c2..ac145a98377b 100644 --- > >>>> a/drivers/gpu/drm/panfrost/panfrost_mmu.c +++ > >>>> b/drivers/gpu/drm/panfrost/panfrost_mmu.c @@ -504,7 +504,7 @@ > >>>> static int panfrost_mmu_map_fault_addr(struct panfrost_device > >>>> *pfdev, int as, if (IS_ERR(pages[i])) { ret = PTR_ERR(pages[i]); > >>>> pages[i] = NULL; > >>>> - goto err_pages; > >>>> + goto err_unlock; > >>>> } > >>>> } > >>>> > >>>> @@ -512,7 +512,7 @@ static int > >>>> panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int > >>>> as, ret = sg_alloc_table_from_pages(sgt, pages + page_offset, > >>>> NUM_FAULT_PAGES, 0, SZ_2M, GFP_KERNEL); if (ret) > >>>> - goto err_pages; > >>>> + goto err_unlock; > >>> Feels like the panfrost_gem_mapping object should hold a ref on > >>> the BO pages, not the BO itself, because, ultimately, the user of > >>> the BO is the GPU. This matches what I was saying about moving > >>> get/put_pages() to panfrost_mmu_map/unmap(): everytime a > >>> panfrost_gem_mapping becomes active, to want to take a pages ref, > >>> every time it becomes inactive, you should release the pages ref. > >>> > >> > >> The panfrost_mmu_unmap() is also used by shrinker when BO is > >> purged. I'm unhappy with how icky it all becomes if unmap is made > >> to put pages. > > > > Why, that's exactly what's supposed to happen. If you mmu_unmap(), > > that means you no longer need the pages ref you got. > > The drm_gem_shmem_purge() frees the pages. If mmu_unmap() frees pages > too, then it becomes odd for drm_gem_shmem_purge() that it needs to > free pages that were already freed. Hm, I didn't consider the mmu_unmap() call in the eviction path. > > >> Previously map() was implicitly allocating pages with get_sgt() > >> and then pages were implicitly released by drm_gem_shmem_free(). A > >> non-heap BO is mapped when it's created by Panfrost, hence the > >> actual lifetime of pages is kept unchanged by this patch. > > > > But the whole point of making it explicit is to control when pages > > are needed or not, isn't it. The fact we mmu_map() the BO at open > > time, and keep it mapped until it's not longer referenced is an > > implementation choice, and I don't think having pages_put() in > > mmu_unmap() changes that. > > Previously, when the last mmu_unmap() was done, the pages were not > released. > > If you'll make unmap to put pages, then you can't map BO again > because pages are released by the last put() of unmap. Well, you could, if panfrost_gem_mapping_get() was not only returning an existing mapping, but was also creating one when none exist. But you're right, it messes up with the shmem shrinker and also changes the way we are doing things now. > In order to > keep the old pages allocation logic unchanged, the pages must be > referenced while BO is alive, not while mapping is alive. Correct. > > Technically, the code can be changed to put pages on unmap. This > requires adding special quirk to drm_gem_shmem_purge() and then for > Panfrost pages should have the same lifetime as BO, hence why bother? No, we certainly don't want to do that.
On Mon, 30 Oct 2023 02:01:54 +0300 Dmitry Osipenko <dmitry.osipenko@collabora.com> 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@collabora.com> > --- > 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.
On Thu, 23 Nov 2023 15:48:48 +0100 Boris Brezillon <boris.brezillon@collabora.com> wrote: > On Mon, 30 Oct 2023 02:01:54 +0300 > Dmitry Osipenko <dmitry.osipenko@collabora.com> 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@collabora.com> > > --- > > 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
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); + if (err) + goto err_free; + } + return bo; + +err_free: + drm_gem_shmem_free(&bo->base); + + return ERR_PTR(err); } struct drm_gem_object * diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c index 770dab1942c2..ac145a98377b 100644 --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c @@ -504,7 +504,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, if (IS_ERR(pages[i])) { ret = PTR_ERR(pages[i]); pages[i] = NULL; - goto err_pages; + goto err_unlock; } } @@ -512,7 +512,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, ret = sg_alloc_table_from_pages(sgt, pages + page_offset, NUM_FAULT_PAGES, 0, SZ_2M, GFP_KERNEL); if (ret) - goto err_pages; + goto err_unlock; ret = dma_map_sgtable(pfdev->dev, sgt, DMA_BIDIRECTIONAL, 0); if (ret) @@ -535,8 +535,6 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, err_map: sg_free_table(sgt); -err_pages: - drm_gem_shmem_put_pages_locked(&bo->base); err_unlock: dma_resv_unlock(obj->resv); err_bo:
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@collabora.com> --- drivers/gpu/drm/panfrost/panfrost_gem.c | 17 +++++++++++++++++ drivers/gpu/drm/panfrost/panfrost_mmu.c | 6 ++---- 2 files changed, 19 insertions(+), 4 deletions(-)