Re: [PATCH v19 18/30] drm/panfrost: Explicitly get and put drm-shmem pages
On 26/01/2024 09:39, Boris Brezillon wrote:
> On Thu, 25 Jan 2024 16:47:24 +
> Steven Price wrote:
>
>> On 05/01/2024 18:46, 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.
>>>
>>> In Panfrost, panfrost_gem_mapping, which is the object representing a
>>> GPU mapping of a BO, owns a pages ref. This guarantees that any BO being
>>> mapped GPU side has its pages retained till the mapping is destroyed.
>>>
>>> Since pages are no longer guaranteed to stay pinned for the BO lifetime,
>>> and MADVISE(DONT_NEED) flagging remains after the GEM handle has been
>>> destroyed, we need to add an extra 'is_purgeable' check in
>>> panfrost_gem_purge(), to make sure we're not trying to purge a BO that
>>> already had its pages released.
>>>
>>> Signed-off-by: Dmitry Osipenko
>>
>> Reviewed-by: Steven Price
>>
>> Although I don't like the condition in panfrost_gem_mapping_release()
>> for drm_gem_shmem_put_pages() and assigning NULL to bo->sgts - it feels
>> very fragile. See below.
>>
>>> ---
>>> drivers/gpu/drm/panfrost/panfrost_gem.c | 63 ++-
>>> .../gpu/drm/panfrost/panfrost_gem_shrinker.c | 6 ++
>>> 2 files changed, 52 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c
>>> b/drivers/gpu/drm/panfrost/panfrost_gem.c
>>> index f268bd5c2884..7edfc12f7c1f 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
>>> @@ -35,20 +35,6 @@ static void panfrost_gem_free_object(struct
>>> drm_gem_object *obj)
>>> */
>>> WARN_ON_ONCE(!list_empty(>mappings.list));
>>>
>>> - if (bo->sgts) {
>>> - int i;
>>> - int n_sgt = bo->base.base.size / SZ_2M;
>>> -
>>> - for (i = 0; i < n_sgt; i++) {
>>> - if (bo->sgts[i].sgl) {
>>> - dma_unmap_sgtable(pfdev->dev, >sgts[i],
>>> - DMA_BIDIRECTIONAL, 0);
>>> - sg_free_table(>sgts[i]);
>>> - }
>>> - }
>>> - kvfree(bo->sgts);
>>> - }
>>> -
>>> drm_gem_shmem_free(>base);
>>> }
>>>
>>> @@ -85,11 +71,40 @@ panfrost_gem_teardown_mapping(struct
>>> panfrost_gem_mapping *mapping)
>>>
>>> static void panfrost_gem_mapping_release(struct kref *kref)
>>> {
>>> - struct panfrost_gem_mapping *mapping;
>>> -
>>> - mapping = container_of(kref, struct panfrost_gem_mapping, refcount);
>>> + struct panfrost_gem_mapping *mapping =
>>> + container_of(kref, struct panfrost_gem_mapping, refcount);
>>> + struct panfrost_gem_object *bo = mapping->obj;
>>> + struct panfrost_device *pfdev = bo->base.base.dev->dev_private;
>>>
>>> panfrost_gem_teardown_mapping(mapping);
>>> +
>>> + /* On heap BOs, release the sgts created in the fault handler path. */
>>> + if (bo->sgts) {
>>> + int i, n_sgt = bo->base.base.size / SZ_2M;
>>> +
>>> + for (i = 0; i < n_sgt; i++) {
>>> + if (bo->sgts[i].sgl) {
>>> + dma_unmap_sgtable(pfdev->dev, >sgts[i],
>>> + DMA_BIDIRECTIONAL, 0);
>>> + sg_free_table(>sgts[i]);
>>> + }
>>> + }
>>> + kvfree(bo->sgts);
>>> + }
>>> +
>>> + /* Pages ref is owned by the panfrost_gem_mapping object. We must
>>> +* release our pages ref (if any), before releasing the object
>>> +* ref.
>>> +* Non-heap BOs acquired the pages at panfrost_gem_mapping creation
>>> +* time, and heap BOs may have acquired pages if the fault handler
>>> +* was called, in which case bo->sgts should be non-NULL.
>>> +*/
>>> + if (!bo->base.base.import_attach && (!bo->is_heap || bo->sgts) &&
>>> + bo->base.madv >= 0) {
>>> + drm_gem_shmem_put_pages(>base);
>>> + bo->sgts = NULL;
>>
>> The assignment of NULL here really ought to be unconditional - it isn't
>> a valid pointer because of the kvfree() above.
>
> Fair enough. How about we drop the '|| bo->sgts' and add an
> drm_gem_shmem_put_pages() to the above if (bo->sgts) block, where we'll
> also assign bo->sgts to NULL?
Yes that would be good.
>>
>> I also feel that the big condition above suggests there's a need for a
>> better state machine to keep track of what's going on.
>
> I'm planning to extend drm_gem_shmem to support the alloc-on-fault use
> case that all Mali GPUs seem to rely on (lima, panfrost and soon
> panthor would use those helpers). The idea is to:
>
> - make the allocation non-blocking, so we can kill the blocking
> allocation in the dma signalling path (basically what
Re: [PATCH v19 18/30] drm/panfrost: Explicitly get and put drm-shmem pages
On Thu, 25 Jan 2024 16:47:24 +
Steven Price wrote:
> On 05/01/2024 18:46, 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.
> >
> > In Panfrost, panfrost_gem_mapping, which is the object representing a
> > GPU mapping of a BO, owns a pages ref. This guarantees that any BO being
> > mapped GPU side has its pages retained till the mapping is destroyed.
> >
> > Since pages are no longer guaranteed to stay pinned for the BO lifetime,
> > and MADVISE(DONT_NEED) flagging remains after the GEM handle has been
> > destroyed, we need to add an extra 'is_purgeable' check in
> > panfrost_gem_purge(), to make sure we're not trying to purge a BO that
> > already had its pages released.
> >
> > Signed-off-by: Dmitry Osipenko
>
> Reviewed-by: Steven Price
>
> Although I don't like the condition in panfrost_gem_mapping_release()
> for drm_gem_shmem_put_pages() and assigning NULL to bo->sgts - it feels
> very fragile. See below.
>
> > ---
> > drivers/gpu/drm/panfrost/panfrost_gem.c | 63 ++-
> > .../gpu/drm/panfrost/panfrost_gem_shrinker.c | 6 ++
> > 2 files changed, 52 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c
> > b/drivers/gpu/drm/panfrost/panfrost_gem.c
> > index f268bd5c2884..7edfc12f7c1f 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> > @@ -35,20 +35,6 @@ static void panfrost_gem_free_object(struct
> > drm_gem_object *obj)
> > */
> > WARN_ON_ONCE(!list_empty(>mappings.list));
> >
> > - if (bo->sgts) {
> > - int i;
> > - int n_sgt = bo->base.base.size / SZ_2M;
> > -
> > - for (i = 0; i < n_sgt; i++) {
> > - if (bo->sgts[i].sgl) {
> > - dma_unmap_sgtable(pfdev->dev, >sgts[i],
> > - DMA_BIDIRECTIONAL, 0);
> > - sg_free_table(>sgts[i]);
> > - }
> > - }
> > - kvfree(bo->sgts);
> > - }
> > -
> > drm_gem_shmem_free(>base);
> > }
> >
> > @@ -85,11 +71,40 @@ panfrost_gem_teardown_mapping(struct
> > panfrost_gem_mapping *mapping)
> >
> > static void panfrost_gem_mapping_release(struct kref *kref)
> > {
> > - struct panfrost_gem_mapping *mapping;
> > -
> > - mapping = container_of(kref, struct panfrost_gem_mapping, refcount);
> > + struct panfrost_gem_mapping *mapping =
> > + container_of(kref, struct panfrost_gem_mapping, refcount);
> > + struct panfrost_gem_object *bo = mapping->obj;
> > + struct panfrost_device *pfdev = bo->base.base.dev->dev_private;
> >
> > panfrost_gem_teardown_mapping(mapping);
> > +
> > + /* On heap BOs, release the sgts created in the fault handler path. */
> > + if (bo->sgts) {
> > + int i, n_sgt = bo->base.base.size / SZ_2M;
> > +
> > + for (i = 0; i < n_sgt; i++) {
> > + if (bo->sgts[i].sgl) {
> > + dma_unmap_sgtable(pfdev->dev, >sgts[i],
> > + DMA_BIDIRECTIONAL, 0);
> > + sg_free_table(>sgts[i]);
> > + }
> > + }
> > + kvfree(bo->sgts);
> > + }
> > +
> > + /* Pages ref is owned by the panfrost_gem_mapping object. We must
> > +* release our pages ref (if any), before releasing the object
> > +* ref.
> > +* Non-heap BOs acquired the pages at panfrost_gem_mapping creation
> > +* time, and heap BOs may have acquired pages if the fault handler
> > +* was called, in which case bo->sgts should be non-NULL.
> > +*/
> > + if (!bo->base.base.import_attach && (!bo->is_heap || bo->sgts) &&
> > + bo->base.madv >= 0) {
> > + drm_gem_shmem_put_pages(>base);
> > + bo->sgts = NULL;
>
> The assignment of NULL here really ought to be unconditional - it isn't
> a valid pointer because of the kvfree() above.
Fair enough. How about we drop the '|| bo->sgts' and add an
drm_gem_shmem_put_pages() to the above if (bo->sgts) block, where we'll
also assign bo->sgts to NULL?
>
> I also feel that the big condition above suggests there's a need for a
> better state machine to keep track of what's going on.
I'm planning to extend drm_gem_shmem to support the alloc-on-fault use
case that all Mali GPUs seem to rely on (lima, panfrost and soon
panthor would use those helpers). The idea is to:
- make the allocation non-blocking, so we can kill the blocking
allocation in the dma signalling path (basically what intel does)
- allow dynamic extension of the pages array using an xarray instead of
a plain array
Re: [PATCH v19 18/30] drm/panfrost: Explicitly get and put drm-shmem pages
On 05/01/2024 18:46, 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.
>
> In Panfrost, panfrost_gem_mapping, which is the object representing a
> GPU mapping of a BO, owns a pages ref. This guarantees that any BO being
> mapped GPU side has its pages retained till the mapping is destroyed.
>
> Since pages are no longer guaranteed to stay pinned for the BO lifetime,
> and MADVISE(DONT_NEED) flagging remains after the GEM handle has been
> destroyed, we need to add an extra 'is_purgeable' check in
> panfrost_gem_purge(), to make sure we're not trying to purge a BO that
> already had its pages released.
>
> Signed-off-by: Dmitry Osipenko
Reviewed-by: Steven Price
Although I don't like the condition in panfrost_gem_mapping_release()
for drm_gem_shmem_put_pages() and assigning NULL to bo->sgts - it feels
very fragile. See below.
> ---
> drivers/gpu/drm/panfrost/panfrost_gem.c | 63 ++-
> .../gpu/drm/panfrost/panfrost_gem_shrinker.c | 6 ++
> 2 files changed, 52 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c
> b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index f268bd5c2884..7edfc12f7c1f 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -35,20 +35,6 @@ static void panfrost_gem_free_object(struct drm_gem_object
> *obj)
>*/
> WARN_ON_ONCE(!list_empty(>mappings.list));
>
> - if (bo->sgts) {
> - int i;
> - int n_sgt = bo->base.base.size / SZ_2M;
> -
> - for (i = 0; i < n_sgt; i++) {
> - if (bo->sgts[i].sgl) {
> - dma_unmap_sgtable(pfdev->dev, >sgts[i],
> - DMA_BIDIRECTIONAL, 0);
> - sg_free_table(>sgts[i]);
> - }
> - }
> - kvfree(bo->sgts);
> - }
> -
> drm_gem_shmem_free(>base);
> }
>
> @@ -85,11 +71,40 @@ panfrost_gem_teardown_mapping(struct panfrost_gem_mapping
> *mapping)
>
> static void panfrost_gem_mapping_release(struct kref *kref)
> {
> - struct panfrost_gem_mapping *mapping;
> -
> - mapping = container_of(kref, struct panfrost_gem_mapping, refcount);
> + struct panfrost_gem_mapping *mapping =
> + container_of(kref, struct panfrost_gem_mapping, refcount);
> + struct panfrost_gem_object *bo = mapping->obj;
> + struct panfrost_device *pfdev = bo->base.base.dev->dev_private;
>
> panfrost_gem_teardown_mapping(mapping);
> +
> + /* On heap BOs, release the sgts created in the fault handler path. */
> + if (bo->sgts) {
> + int i, n_sgt = bo->base.base.size / SZ_2M;
> +
> + for (i = 0; i < n_sgt; i++) {
> + if (bo->sgts[i].sgl) {
> + dma_unmap_sgtable(pfdev->dev, >sgts[i],
> + DMA_BIDIRECTIONAL, 0);
> + sg_free_table(>sgts[i]);
> + }
> + }
> + kvfree(bo->sgts);
> + }
> +
> + /* Pages ref is owned by the panfrost_gem_mapping object. We must
> + * release our pages ref (if any), before releasing the object
> + * ref.
> + * Non-heap BOs acquired the pages at panfrost_gem_mapping creation
> + * time, and heap BOs may have acquired pages if the fault handler
> + * was called, in which case bo->sgts should be non-NULL.
> + */
> + if (!bo->base.base.import_attach && (!bo->is_heap || bo->sgts) &&
> + bo->base.madv >= 0) {
> + drm_gem_shmem_put_pages(>base);
> + bo->sgts = NULL;
The assignment of NULL here really ought to be unconditional - it isn't
a valid pointer because of the kvfree() above.
I also feel that the big condition above suggests there's a need for a
better state machine to keep track of what's going on.
But having said that I do think this series as a whole is an
improvement, it's nice to get the shrinker code generic. And sadly I
don't have an immediate idea for cleaning this up, hence my R-b.
Steve
> + }
> +
> drm_gem_object_put(>obj->base.base);
> panfrost_mmu_ctx_put(mapping->mmu);
> kfree(mapping);
> @@ -125,6 +140,20 @@ int panfrost_gem_open(struct drm_gem_object *obj, struct
> drm_file *file_priv)
> if (!mapping)
> return -ENOMEM;
>
> + if (!bo->is_heap && !bo->base.base.import_attach) {
> + /* Pages ref is owned by the panfrost_gem_mapping object.
> + * For non-heap BOs, we request pages at mapping creation
> + * time, such that the
Re: [PATCH v19 18/30] drm/panfrost: Explicitly get and put drm-shmem pages
On Fri, 5 Jan 2024 21:46:12 +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.
>
> In Panfrost, panfrost_gem_mapping, which is the object representing a
> GPU mapping of a BO, owns a pages ref. This guarantees that any BO being
> mapped GPU side has its pages retained till the mapping is destroyed.
>
> Since pages are no longer guaranteed to stay pinned for the BO lifetime,
> and MADVISE(DONT_NEED) flagging remains after the GEM handle has been
> destroyed, we need to add an extra 'is_purgeable' check in
> panfrost_gem_purge(), to make sure we're not trying to purge a BO that
> already had its pages released.
>
> Signed-off-by: Dmitry Osipenko
Reviewed-by: Boris Brezillon
But I'd like to have Steve's review as well on that one.
> ---
> drivers/gpu/drm/panfrost/panfrost_gem.c | 63 ++-
> .../gpu/drm/panfrost/panfrost_gem_shrinker.c | 6 ++
> 2 files changed, 52 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c
> b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index f268bd5c2884..7edfc12f7c1f 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -35,20 +35,6 @@ static void panfrost_gem_free_object(struct drm_gem_object
> *obj)
>*/
> WARN_ON_ONCE(!list_empty(>mappings.list));
>
> - if (bo->sgts) {
> - int i;
> - int n_sgt = bo->base.base.size / SZ_2M;
> -
> - for (i = 0; i < n_sgt; i++) {
> - if (bo->sgts[i].sgl) {
> - dma_unmap_sgtable(pfdev->dev, >sgts[i],
> - DMA_BIDIRECTIONAL, 0);
> - sg_free_table(>sgts[i]);
> - }
> - }
> - kvfree(bo->sgts);
> - }
> -
> drm_gem_shmem_free(>base);
> }
>
> @@ -85,11 +71,40 @@ panfrost_gem_teardown_mapping(struct panfrost_gem_mapping
> *mapping)
>
> static void panfrost_gem_mapping_release(struct kref *kref)
> {
> - struct panfrost_gem_mapping *mapping;
> -
> - mapping = container_of(kref, struct panfrost_gem_mapping, refcount);
> + struct panfrost_gem_mapping *mapping =
> + container_of(kref, struct panfrost_gem_mapping, refcount);
> + struct panfrost_gem_object *bo = mapping->obj;
> + struct panfrost_device *pfdev = bo->base.base.dev->dev_private;
>
> panfrost_gem_teardown_mapping(mapping);
> +
> + /* On heap BOs, release the sgts created in the fault handler path. */
> + if (bo->sgts) {
> + int i, n_sgt = bo->base.base.size / SZ_2M;
> +
> + for (i = 0; i < n_sgt; i++) {
> + if (bo->sgts[i].sgl) {
> + dma_unmap_sgtable(pfdev->dev, >sgts[i],
> + DMA_BIDIRECTIONAL, 0);
> + sg_free_table(>sgts[i]);
> + }
> + }
> + kvfree(bo->sgts);
> + }
> +
> + /* Pages ref is owned by the panfrost_gem_mapping object. We must
> + * release our pages ref (if any), before releasing the object
> + * ref.
> + * Non-heap BOs acquired the pages at panfrost_gem_mapping creation
> + * time, and heap BOs may have acquired pages if the fault handler
> + * was called, in which case bo->sgts should be non-NULL.
> + */
> + if (!bo->base.base.import_attach && (!bo->is_heap || bo->sgts) &&
> + bo->base.madv >= 0) {
> + drm_gem_shmem_put_pages(>base);
> + bo->sgts = NULL;
> + }
> +
> drm_gem_object_put(>obj->base.base);
> panfrost_mmu_ctx_put(mapping->mmu);
> kfree(mapping);
> @@ -125,6 +140,20 @@ int panfrost_gem_open(struct drm_gem_object *obj, struct
> drm_file *file_priv)
> if (!mapping)
> return -ENOMEM;
>
> + if (!bo->is_heap && !bo->base.base.import_attach) {
> + /* Pages ref is owned by the panfrost_gem_mapping object.
> + * For non-heap BOs, we request pages at mapping creation
> + * time, such that the panfrost_mmu_map() call, further down in
> + * this function, is guaranteed to have pages_use_count > 0
> + * when drm_gem_shmem_get_pages_sgt() is called.
> + */
> + ret = drm_gem_shmem_get_pages(>base);
> + if (ret) {
> + kfree(mapping);
> + return ret;
> + }
> + }
> +
> INIT_LIST_HEAD(>node);
> kref_init(>refcount);
> drm_gem_object_get(obj);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
>
[PATCH v19 18/30] drm/panfrost: Explicitly get and put drm-shmem pages
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.
In Panfrost, panfrost_gem_mapping, which is the object representing a
GPU mapping of a BO, owns a pages ref. This guarantees that any BO being
mapped GPU side has its pages retained till the mapping is destroyed.
Since pages are no longer guaranteed to stay pinned for the BO lifetime,
and MADVISE(DONT_NEED) flagging remains after the GEM handle has been
destroyed, we need to add an extra 'is_purgeable' check in
panfrost_gem_purge(), to make sure we're not trying to purge a BO that
already had its pages released.
Signed-off-by: Dmitry Osipenko
---
drivers/gpu/drm/panfrost/panfrost_gem.c | 63 ++-
.../gpu/drm/panfrost/panfrost_gem_shrinker.c | 6 ++
2 files changed, 52 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c
b/drivers/gpu/drm/panfrost/panfrost_gem.c
index f268bd5c2884..7edfc12f7c1f 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -35,20 +35,6 @@ static void panfrost_gem_free_object(struct drm_gem_object
*obj)
*/
WARN_ON_ONCE(!list_empty(>mappings.list));
- if (bo->sgts) {
- int i;
- int n_sgt = bo->base.base.size / SZ_2M;
-
- for (i = 0; i < n_sgt; i++) {
- if (bo->sgts[i].sgl) {
- dma_unmap_sgtable(pfdev->dev, >sgts[i],
- DMA_BIDIRECTIONAL, 0);
- sg_free_table(>sgts[i]);
- }
- }
- kvfree(bo->sgts);
- }
-
drm_gem_shmem_free(>base);
}
@@ -85,11 +71,40 @@ panfrost_gem_teardown_mapping(struct panfrost_gem_mapping
*mapping)
static void panfrost_gem_mapping_release(struct kref *kref)
{
- struct panfrost_gem_mapping *mapping;
-
- mapping = container_of(kref, struct panfrost_gem_mapping, refcount);
+ struct panfrost_gem_mapping *mapping =
+ container_of(kref, struct panfrost_gem_mapping, refcount);
+ struct panfrost_gem_object *bo = mapping->obj;
+ struct panfrost_device *pfdev = bo->base.base.dev->dev_private;
panfrost_gem_teardown_mapping(mapping);
+
+ /* On heap BOs, release the sgts created in the fault handler path. */
+ if (bo->sgts) {
+ int i, n_sgt = bo->base.base.size / SZ_2M;
+
+ for (i = 0; i < n_sgt; i++) {
+ if (bo->sgts[i].sgl) {
+ dma_unmap_sgtable(pfdev->dev, >sgts[i],
+ DMA_BIDIRECTIONAL, 0);
+ sg_free_table(>sgts[i]);
+ }
+ }
+ kvfree(bo->sgts);
+ }
+
+ /* Pages ref is owned by the panfrost_gem_mapping object. We must
+* release our pages ref (if any), before releasing the object
+* ref.
+* Non-heap BOs acquired the pages at panfrost_gem_mapping creation
+* time, and heap BOs may have acquired pages if the fault handler
+* was called, in which case bo->sgts should be non-NULL.
+*/
+ if (!bo->base.base.import_attach && (!bo->is_heap || bo->sgts) &&
+ bo->base.madv >= 0) {
+ drm_gem_shmem_put_pages(>base);
+ bo->sgts = NULL;
+ }
+
drm_gem_object_put(>obj->base.base);
panfrost_mmu_ctx_put(mapping->mmu);
kfree(mapping);
@@ -125,6 +140,20 @@ int panfrost_gem_open(struct drm_gem_object *obj, struct
drm_file *file_priv)
if (!mapping)
return -ENOMEM;
+ if (!bo->is_heap && !bo->base.base.import_attach) {
+ /* Pages ref is owned by the panfrost_gem_mapping object.
+* For non-heap BOs, we request pages at mapping creation
+* time, such that the panfrost_mmu_map() call, further down in
+* this function, is guaranteed to have pages_use_count > 0
+* when drm_gem_shmem_get_pages_sgt() is called.
+*/
+ ret = drm_gem_shmem_get_pages(>base);
+ if (ret) {
+ kfree(mapping);
+ return ret;
+ }
+ }
+
INIT_LIST_HEAD(>node);
kref_init(>refcount);
drm_gem_object_get(obj);
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
index 02b60ea1433a..d4fb0854cf2f 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
@@ -50,6 +50,12 @@ static bool
