Re: [PATCH 1/5] drm/panfrost: Restructure the GEM object creation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 17/07/2019 19:33, Rob Herring wrote:
> Setting the GPU VA when creating the GEM object doesn't allow for any
> conditional adjustments. In preparation to support adjusting the
> mapping, restructure the GEM object creation to map the GEM object after
> we've created the base shmem object.
> 
> Cc: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx>
> Cc: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> Cc: Robin Murphy <robin.murphy@xxxxxxx>
> Cc: Steven Price <steven.price@xxxxxxx>
> Cc: Alyssa Rosenzweig <alyssa@xxxxxxxxxxxxx>
> Signed-off-by: Rob Herring <robh@xxxxxxxxxx>

Hi Rob,

I couldn't work out what base this series should be applied to, but I've
tried manually applying it against Linus' tree and run a few tests.

PANFROST_BO_NOEXEC works as expected, but PANFROST_BO_HEAP seems to
cause a memory leak.

> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c | 21 +++------
>  drivers/gpu/drm/panfrost/panfrost_gem.c | 58 ++++++++++++++++++++-----
>  drivers/gpu/drm/panfrost/panfrost_gem.h |  5 +++
>  3 files changed, 59 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index cb43ff4ebf4a..d354b92964d5 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -46,29 +46,20 @@ static int panfrost_ioctl_get_param(struct drm_device *ddev, void *data, struct
>  static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,
>  		struct drm_file *file)
>  {
> -	int ret;
> -	struct drm_gem_shmem_object *shmem;
> +	struct panfrost_gem_object *bo;
>  	struct drm_panfrost_create_bo *args = data;
>  
>  	if (!args->size || args->flags || args->pad)
>  		return -EINVAL;
>  
> -	shmem = drm_gem_shmem_create_with_handle(file, dev, args->size,
> -						 &args->handle);
> -	if (IS_ERR(shmem))
> -		return PTR_ERR(shmem);
> -
> -	ret = panfrost_mmu_map(to_panfrost_bo(&shmem->base));
> -	if (ret)
> -		goto err_free;
> +	bo = panfrost_gem_create_with_handle(file, dev, args->size, args->flags,
> +					     &args->handle);
> +	if (IS_ERR(bo))
> +		return PTR_ERR(bo);
>  
> -	args->offset = to_panfrost_bo(&shmem->base)->node.start << PAGE_SHIFT;
> +	args->offset = bo->node.start << PAGE_SHIFT;
>  
>  	return 0;
> -
> -err_free:
> -	drm_gem_handle_delete(file, args->handle);
> -	return ret;
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index 543ab1b81bd5..df70dcf3cb2f 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -23,7 +23,8 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)
>  		panfrost_mmu_unmap(bo);
>  
>  	spin_lock(&pfdev->mm_lock);
> -	drm_mm_remove_node(&bo->node);
> +	if (drm_mm_node_allocated(&bo->node))
> +		drm_mm_remove_node(&bo->node);

I'm not sure this change should be here - it looks more like it was
meant as part of patch 4.

Steve

>  	spin_unlock(&pfdev->mm_lock);
>  
>  	drm_gem_shmem_free_object(obj);
> @@ -50,10 +51,7 @@ static const struct drm_gem_object_funcs panfrost_gem_funcs = {
>   */
>  struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t size)
>  {
> -	int ret;
> -	struct panfrost_device *pfdev = dev->dev_private;
>  	struct panfrost_gem_object *obj;
> -	u64 align;
>  
>  	obj = kzalloc(sizeof(*obj), GFP_KERNEL);
>  	if (!obj)
> @@ -61,20 +59,52 @@ struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t
>  
>  	obj->base.base.funcs = &panfrost_gem_funcs;
>  
> -	size = roundup(size, PAGE_SIZE);
> -	align = size >= SZ_2M ? SZ_2M >> PAGE_SHIFT : 0;
> +	return &obj->base.base;
> +}
> +
> +static int panfrost_gem_map(struct panfrost_device *pfdev, struct panfrost_gem_object *bo)
> +{
> +	int ret;
> +	size_t size = bo->base.base.size;
> +	u64 align = size >= SZ_2M ? SZ_2M >> PAGE_SHIFT : 0;
>  
>  	spin_lock(&pfdev->mm_lock);
> -	ret = drm_mm_insert_node_generic(&pfdev->mm, &obj->node,
> +	ret = drm_mm_insert_node_generic(&pfdev->mm, &bo->node,
>  					 size >> PAGE_SHIFT, align, 0, 0);
>  	spin_unlock(&pfdev->mm_lock);
> +	if (ret)
> +		return ret;
> +
> +	return panfrost_mmu_map(bo);
> +}
> +
> +struct panfrost_gem_object *
> +panfrost_gem_create_with_handle(struct drm_file *file_priv,
> +				 struct drm_device *dev, size_t size,
> +				 u32 flags,
> +				 uint32_t *handle)
> +{
> +	int ret;
> +	struct panfrost_device *pfdev = dev->dev_private;
> +	struct drm_gem_shmem_object *shmem;
> +	struct panfrost_gem_object *bo;
> +
> +	size = roundup(size, PAGE_SIZE);
> +
> +	shmem = drm_gem_shmem_create_with_handle(file_priv, dev, size, handle);
> +	if (IS_ERR(shmem))
> +		return ERR_CAST(shmem);
> +
> +	bo = to_panfrost_bo(&shmem->base);
> +
> +	ret = panfrost_gem_map(pfdev, bo);
>  	if (ret)
>  		goto free_obj;
>  
> -	return &obj->base.base;
> +	return bo;
>  
>  free_obj:
> -	kfree(obj);
> +	drm_gem_handle_delete(file_priv, *handle);
>  	return ERR_PTR(ret);
>  }
>  
> @@ -83,8 +113,10 @@ panfrost_gem_prime_import_sg_table(struct drm_device *dev,
>  				   struct dma_buf_attachment *attach,
>  				   struct sg_table *sgt)
>  {
> +	int ret;
>  	struct drm_gem_object *obj;
>  	struct panfrost_gem_object *pobj;
> +	struct panfrost_device *pfdev = dev->dev_private;
>  
>  	obj = drm_gem_shmem_prime_import_sg_table(dev, attach, sgt);
>  	if (IS_ERR(obj))
> @@ -92,7 +124,13 @@ panfrost_gem_prime_import_sg_table(struct drm_device *dev,
>  
>  	pobj = to_panfrost_bo(obj);
>  
> -	panfrost_mmu_map(pobj);
> +	ret = panfrost_gem_map(pfdev, pobj);
> +	if (ret)
> +		goto free_obj;
>  
>  	return obj;
> +
> +free_obj:
> +	drm_gem_object_put_unlocked(obj);
> +	return ERR_PTR(ret);
>  }
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
> index 6dbcaba020fc..ce065270720b 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
> @@ -22,6 +22,11 @@ struct  panfrost_gem_object *to_panfrost_bo(struct drm_gem_object *obj)
>  
>  struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t size);
>  
> +struct panfrost_gem_object *
> +panfrost_gem_create_with_handle(struct drm_file *file_priv,
> +				struct drm_device *dev, size_t size, u32 flags,
> +				uint32_t *handle);
> +
>  struct drm_gem_object *
>  panfrost_gem_prime_import_sg_table(struct drm_device *dev,
>  				   struct dma_buf_attachment *attach,
> 

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux