On Mon, 30 Oct 2023 02:01:54 +0300
Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> ---
> 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:
