On Mon, 30 Oct 2023 02:01:53 +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. Lima driver doesn't
> have shrinker, 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/lima/lima_gem.c | 18 ++++++++++++++++--
> drivers/gpu/drm/lima/lima_gem.h | 1 +
> 2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/lima/lima_gem.c b/drivers/gpu/drm/lima/lima_gem.c
> index 988e74f67465..d255f5775dac 100644
> --- a/drivers/gpu/drm/lima/lima_gem.c
> +++ b/drivers/gpu/drm/lima/lima_gem.c
> @@ -46,6 +46,7 @@ int lima_heap_alloc(struct lima_bo *bo, struct lima_vm *vm)
> return -ENOMEM;
> }
>
> + bo->put_pages = true;
> bo->base.pages = pages;
> refcount_set(&bo->base.pages_use_count, 1);
>
> @@ -115,6 +116,7 @@ int lima_gem_create_handle(struct drm_device *dev, struct drm_file *file,
> return PTR_ERR(shmem);
>
> obj = &shmem->base;
> + bo = to_lima_bo(obj);
>
> /* Mali Utgard GPU can only support 32bit address space */
> mask = mapping_gfp_mask(obj->filp->f_mapping);
> @@ -123,13 +125,19 @@ int lima_gem_create_handle(struct drm_device *dev, struct drm_file *file,
> mapping_set_gfp_mask(obj->filp->f_mapping, mask);
>
> if (is_heap) {
> - bo = to_lima_bo(obj);
> err = lima_heap_alloc(bo, NULL);
> if (err)
> goto out;
> } else {
> - struct sg_table *sgt = drm_gem_shmem_get_pages_sgt(shmem);
> + struct sg_table *sgt;
> +
> + err = drm_gem_shmem_get_pages(shmem);
> + if (err)
> + goto out;
> +
> + bo->put_pages = true;
>
> + sgt = drm_gem_shmem_get_pages_sgt(shmem);
> if (IS_ERR(sgt)) {
> err = PTR_ERR(sgt);
> goto out;
Pretty sure we don't need this put_pages flag. We can either check
ba->base.base.pages or refcount_read(&bo->base.pages_use_count). Or,
even better, if it's just used in the error path of the same function,
simply have a dedicated error path for that case:
drm_gem_object_put(obj);
return 0;
err_put_pages:
if (!is_heap)
drm_gem_shmem_put_pages(shmem);
err_put_bo:
drm_gem_object_put(obj);
return err;
}
> @@ -139,6 +147,9 @@ int lima_gem_create_handle(struct drm_device *dev, struct drm_file *file,
> err = drm_gem_handle_create(file, obj, handle);
>
> out:
> + if (err && bo->put_pages)
> + drm_gem_shmem_put_pages(shmem);
> +
> /* drop reference from allocate - handle holds it now */
> drm_gem_object_put(obj);
>
> @@ -152,6 +163,9 @@ static void lima_gem_free_object(struct drm_gem_object *obj)
> if (!list_empty(&bo->va))
> dev_err(obj->dev->dev, "lima gem free bo still has va\n");
>
> + if (bo->put_pages)
> + drm_gem_shmem_put_pages(&bo->base);
This one can be replaced by
if (!is_heap || bo->base.base.pages)
drm_gem_shmem_put_pages(&bo->base);
> +
> drm_gem_shmem_free(&bo->base);
> }
>
> diff --git a/drivers/gpu/drm/lima/lima_gem.h b/drivers/gpu/drm/lima/lima_gem.h
> index ccea06142f4b..dc5a6d465c80 100644
> --- a/drivers/gpu/drm/lima/lima_gem.h
> +++ b/drivers/gpu/drm/lima/lima_gem.h
> @@ -16,6 +16,7 @@ struct lima_bo {
> struct list_head va;
>
> size_t heap_size;
> + bool put_pages;
> };
>
> static inline struct lima_bo *
