Re: [PATCH v18 14/26] drm/lima: Explicitly get and put drm-shmem pages

[Boris Brezillon <boris.brezillon@collabora.com>]

On Mon, 30 Oct 2023 02:01:53 +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. 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@collabora.com>
> ---
>  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 *