Re: [PATCH v18 15/26] drm/panfrost: Explicitly get and put drm-shmem pages
On Thu, 23 Nov 2023 15:48:48 +0100
Boris Brezillon wrote:
> On Mon, 30 Oct 2023 02:01:54 +0300
> Dmitry Osipenko 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
> > ---
> > 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(>base);
> > }
> >
> > + if (!bo->is_heap && !obj->import_attach)
> > + drm_gem_shmem_put_pages(>base);
> > +
> > drm_gem_shmem_free(>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
Re: [PATCH v18 15/26] drm/panfrost: Explicitly get and put drm-shmem pages
On Mon, 30 Oct 2023 02:01:54 +0300
Dmitry Osipenko 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
> ---
> 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(>base);
> }
>
> + if (!bo->is_heap && !obj->import_attach)
> + drm_gem_shmem_put_pages(>base);
> +
> drm_gem_shmem_free(>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.
Re: [PATCH v18 15/26] drm/panfrost: Explicitly get and put drm-shmem pages
On Thu, 23 Nov 2023 15:24:32 +0300
Dmitry Osipenko wrote:
> On 11/23/23 12:05, Boris Brezillon wrote:
> > On Thu, 23 Nov 2023 01:04:56 +0300
> > Dmitry Osipenko 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(>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.
Re: [PATCH v18 15/26] drm/panfrost: Explicitly get and put drm-shmem pages
On 11/23/23 12:05, Boris Brezillon wrote:
> On Thu, 23 Nov 2023 01:04:56 +0300
> Dmitry Osipenko 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(>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(>pages_use_count))
+ drm_gem_shmem_free_pages(shmem);
+
drm_vma_node_unmap(>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 = >base;
- drm_WARN_ON(obj->dev, !drm_gem_shmem_is_purgeable(shmem));
+ if (refcount_read(>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)
Re: [PATCH v18 15/26] drm/panfrost: Explicitly get and put drm-shmem pages
On Thu, 23 Nov 2023 01:04:56 +0300
Dmitry Osipenko 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(>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.
>
Re: [PATCH v18 15/26] drm/panfrost: Explicitly get and put drm-shmem pages
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(>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.
--
Best regards,
Dmitry
Re: [PATCH v18 15/26] drm/panfrost: Explicitly get and put drm-shmem pages
On Mon, 30 Oct 2023 02:01:54 +0300
Dmitry Osipenko 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
> ---
> 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(>base);
> }
>
> + if (!bo->is_heap && !obj->import_attach)
> + drm_gem_shmem_put_pages(>base);
> +
> drm_gem_shmem_free(>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(>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(>base);
> err_unlock:
> dma_resv_unlock(obj->resv);
> err_bo:
