Re: [PATCH 2/2] drm/cma-helper: Implement mmap as GEM CMA object functions

* Re: [PATCH 2/2] drm/cma-helper: Implement mmap as GEM CMA object functions
       [not found] ` <20201123115646.11004-3-tzimmermann@suse.de>
@ 2021-01-14 12:51   ` Kieran Bingham
  2021-01-14 13:26     ` Thomas Zimmermann
  0 siblings, 1 reply; 7+ messages in thread
From: Kieran Bingham @ 2021-01-14 12:51 UTC (permalink / raw)
  To: Thomas Zimmermann, maarten.lankhorst, mripard, airlied, daniel,
	eric, Linux-Renesas, Laurent Pinchart
  Cc: dri-devel

Hi Thomas,

On 23/11/2020 11:56, Thomas Zimmermann wrote:
> The new GEM object function drm_gem_cma_mmap() sets the VMA flags
> and offset as in the old implementation and immediately maps in the
> buffer's memory pages.
> 
> Changing CMA helpers to use the GEM object function allows for the
> removal of the special implementations for mmap and gem_prime_mmap
> callbacks. The regular functions drm_gem_mmap() and drm_gem_prime_mmap()
> are now used.

I've encountered a memory leak regression in our Renesas R-Car DU tests,
and git bisection has led me to this patch (as commit f5ca8eb6f9).

Running the tests sequentially, while grepping /proc/meminfo for Cma, it
is evident that CMA memory is not released, until exhausted and the
allocations fail (seen in [0]) shown by the error report:

>     self.fbs.append(pykms.DumbFramebuffer(self.card, mode.hdisplay, mode.vdisplay, "XR24"))
> ValueError: DRM_IOCTL_MODE_CREATE_DUMB failed: Cannot allocate memory

Failing tests at f5ca8eb6f9 can be seen at [0], while the tests pass
successfully [1] on the commit previous to that (bc2532ab7c2):

Reverting f5ca8eb6f9 also produces a successful pass [2]

 [0] https://paste.ubuntu.com/p/VjPGPgswxR/ # Failed at f5ca8eb6f9
 [1] https://paste.ubuntu.com/p/78RRp2WpNR/ # Success at bc2532ab7c2
 [2] https://paste.ubuntu.com/p/qJKjZZN2pt/ # Success with revert

I don't believe we handle mmap specially in the RCar-DU driver, so I
wonder if this issue has hit anyone else as well?

Any ideas of a repair without a revert ? Or do we just need to submit a
revert?

I've yet to fully understand the implications of the patch below.

Thanks
--
Regards

Kieran

> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_file.c           |   3 +-
>  drivers/gpu/drm/drm_gem_cma_helper.c | 121 +++++++++------------------
>  drivers/gpu/drm/pl111/pl111_drv.c    |   2 +-
>  drivers/gpu/drm/vc4/vc4_bo.c         |   2 +-
>  include/drm/drm_gem_cma_helper.h     |  10 +--
>  5 files changed, 44 insertions(+), 94 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index b50380fa80ce..80886d50d0f1 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -113,8 +113,7 @@ bool drm_dev_needs_global_mutex(struct drm_device *dev)
>   * The memory mapping implementation will vary depending on how the driver
>   * manages memory. Legacy drivers will use the deprecated drm_legacy_mmap()
>   * function, modern drivers should use one of the provided memory-manager
> - * specific implementations. For GEM-based drivers this is drm_gem_mmap(), and
> - * for drivers which use the CMA GEM helpers it's drm_gem_cma_mmap().
> + * specific implementations. For GEM-based drivers this is drm_gem_mmap().
>   *
>   * No other file operations are supported by the DRM userspace API. Overall the
>   * following is an example &file_operations structure::
> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
> index 6a4ef335ebc9..7942cf05cd93 100644
> --- a/drivers/gpu/drm/drm_gem_cma_helper.c
> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> @@ -38,6 +38,7 @@ static const struct drm_gem_object_funcs drm_gem_cma_default_funcs = {
>  	.print_info = drm_gem_cma_print_info,
>  	.get_sg_table = drm_gem_cma_get_sg_table,
>  	.vmap = drm_gem_cma_vmap,
> +	.mmap = drm_gem_cma_mmap,
>  	.vm_ops = &drm_gem_cma_vm_ops,
>  };
>  
> @@ -277,62 +278,6 @@ const struct vm_operations_struct drm_gem_cma_vm_ops = {
>  };
>  EXPORT_SYMBOL_GPL(drm_gem_cma_vm_ops);
>  
> -static int drm_gem_cma_mmap_obj(struct drm_gem_cma_object *cma_obj,
> -				struct vm_area_struct *vma)
> -{
> -	int ret;
> -
> -	/*
> -	 * Clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
> -	 * vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want to map
> -	 * the whole buffer.
> -	 */
> -	vma->vm_flags &= ~VM_PFNMAP;
> -	vma->vm_pgoff = 0;
> -
> -	ret = dma_mmap_wc(cma_obj->base.dev->dev, vma, cma_obj->vaddr,
> -			  cma_obj->paddr, vma->vm_end - vma->vm_start);
> -	if (ret)
> -		drm_gem_vm_close(vma);
> -
> -	return ret;
> -}
> -
> -/**
> - * drm_gem_cma_mmap - memory-map a CMA GEM object
> - * @filp: file object
> - * @vma: VMA for the area to be mapped
> - *
> - * This function implements an augmented version of the GEM DRM file mmap
> - * operation for CMA objects: In addition to the usual GEM VMA setup it
> - * immediately faults in the entire object instead of using on-demaind
> - * faulting. Drivers which employ the CMA helpers should use this function
> - * as their ->mmap() handler in the DRM device file's file_operations
> - * structure.
> - *
> - * Instead of directly referencing this function, drivers should use the
> - * DEFINE_DRM_GEM_CMA_FOPS().macro.
> - *
> - * Returns:
> - * 0 on success or a negative error code on failure.
> - */
> -int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
> -{
> -	struct drm_gem_cma_object *cma_obj;
> -	struct drm_gem_object *gem_obj;
> -	int ret;
> -
> -	ret = drm_gem_mmap(filp, vma);
> -	if (ret)
> -		return ret;
> -
> -	gem_obj = vma->vm_private_data;
> -	cma_obj = to_drm_gem_cma_obj(gem_obj);
> -
> -	return drm_gem_cma_mmap_obj(cma_obj, vma);
> -}
> -EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
> -
>  #ifndef CONFIG_MMU
>  /**
>   * drm_gem_cma_get_unmapped_area - propose address for mapping in noMMU cases
> @@ -500,33 +445,6 @@ drm_gem_cma_prime_import_sg_table(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_cma_prime_import_sg_table);
>  
> -/**
> - * drm_gem_cma_prime_mmap - memory-map an exported CMA GEM object
> - * @obj: GEM object
> - * @vma: VMA for the area to be mapped
> - *
> - * This function maps a buffer imported via DRM PRIME into a userspace
> - * process's address space. Drivers that use the CMA helpers should set this
> - * as their &drm_driver.gem_prime_mmap callback.
> - *
> - * Returns:
> - * 0 on success or a negative error code on failure.
> - */
> -int drm_gem_cma_prime_mmap(struct drm_gem_object *obj,
> -			   struct vm_area_struct *vma)
> -{
> -	struct drm_gem_cma_object *cma_obj;
> -	int ret;
> -
> -	ret = drm_gem_mmap_obj(obj, obj->size, vma);
> -	if (ret < 0)
> -		return ret;
> -
> -	cma_obj = to_drm_gem_cma_obj(obj);
> -	return drm_gem_cma_mmap_obj(cma_obj, vma);
> -}
> -EXPORT_SYMBOL_GPL(drm_gem_cma_prime_mmap);
> -
>  /**
>   * drm_gem_cma_vmap - map a CMA GEM object into the kernel's virtual
>   *     address space
> @@ -553,6 +471,43 @@ int drm_gem_cma_vmap(struct drm_gem_object *obj, struct dma_buf_map *map)
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_cma_vmap);
>  
> +/**
> + * drm_gem_cma_mmap - memory-map an exported CMA GEM object
> + * @obj: GEM object
> + * @vma: VMA for the area to be mapped
> + *
> + * This function maps a buffer into a userspace process's address space.
> + * In addition to the usual GEM VMA setup it immediately faults in the entire
> + * object instead of using on-demand faulting. Drivers that use the CMA
> + * helpers should set this as their &drm_gem_object_funcs.mmap callback.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +int drm_gem_cma_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
> +{
> +	struct drm_gem_cma_object *cma_obj;
> +	int ret;
> +
> +	/*
> +	 * Clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
> +	 * vm_pgoff (used as a fake buffer offset by DRM) to 0 as we want to map
> +	 * the whole buffer.
> +	 */
> +	vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
> +	vma->vm_flags &= ~VM_PFNMAP;
> +
> +	cma_obj = to_drm_gem_cma_obj(obj);
> +
> +	ret = dma_mmap_wc(cma_obj->base.dev->dev, vma, cma_obj->vaddr,
> +			  cma_obj->paddr, vma->vm_end - vma->vm_start);
> +	if (ret)
> +		drm_gem_vm_close(vma);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
> +
>  /**
>   * drm_gem_cma_prime_import_sg_table_vmap - PRIME import another driver's
>   *	scatter/gather table and get the virtual address of the buffer
> diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
> index 40e6708fbbe2..e4dcaef6c143 100644
> --- a/drivers/gpu/drm/pl111/pl111_drv.c
> +++ b/drivers/gpu/drm/pl111/pl111_drv.c
> @@ -228,7 +228,7 @@ static const struct drm_driver pl111_drm_driver = {
>  	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>  	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>  	.gem_prime_import_sg_table = pl111_gem_import_sg_table,
> -	.gem_prime_mmap = drm_gem_cma_prime_mmap,
> +	.gem_prime_mmap = drm_gem_prime_mmap,
>  
>  #if defined(CONFIG_DEBUG_FS)
>  	.debugfs_init = pl111_debugfs_init,
> diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
> index 813e6cb3f9af..dc316cb79e00 100644
> --- a/drivers/gpu/drm/vc4/vc4_bo.c
> +++ b/drivers/gpu/drm/vc4/vc4_bo.c
> @@ -782,7 +782,7 @@ int vc4_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
>  		return -EINVAL;
>  	}
>  
> -	return drm_gem_cma_prime_mmap(obj, vma);
> +	return drm_gem_prime_mmap(obj, vma);
>  }
>  
>  int vc4_prime_vmap(struct drm_gem_object *obj, struct dma_buf_map *map)
> diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
> index 4680275ab339..0a9711caa3e8 100644
> --- a/include/drm/drm_gem_cma_helper.h
> +++ b/include/drm/drm_gem_cma_helper.h
> @@ -59,7 +59,7 @@ struct drm_gem_cma_object {
>  		.poll		= drm_poll,\
>  		.read		= drm_read,\
>  		.llseek		= noop_llseek,\
> -		.mmap		= drm_gem_cma_mmap,\
> +		.mmap		= drm_gem_mmap,\
>  		DRM_GEM_CMA_UNMAPPED_AREA_FOPS \
>  	}
>  
> @@ -76,9 +76,6 @@ int drm_gem_cma_dumb_create(struct drm_file *file_priv,
>  			    struct drm_device *drm,
>  			    struct drm_mode_create_dumb *args);
>  
> -/* set vm_flags and we can change the VM attribute to other one at here */
> -int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma);
> -
>  /* allocate physical memory */
>  struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
>  					      size_t size);
> @@ -101,9 +98,8 @@ struct drm_gem_object *
>  drm_gem_cma_prime_import_sg_table(struct drm_device *dev,
>  				  struct dma_buf_attachment *attach,
>  				  struct sg_table *sgt);
> -int drm_gem_cma_prime_mmap(struct drm_gem_object *obj,
> -			   struct vm_area_struct *vma);
>  int drm_gem_cma_vmap(struct drm_gem_object *obj, struct dma_buf_map *map);
> +int drm_gem_cma_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma);
>  
>  /**
>   * DRM_GEM_CMA_DRIVER_OPS_WITH_DUMB_CREATE - CMA GEM driver operations
> @@ -123,7 +119,7 @@ int drm_gem_cma_vmap(struct drm_gem_object *obj, struct dma_buf_map *map);
>  	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd, \
>  	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle, \
>  	.gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table, \
> -	.gem_prime_mmap		= drm_gem_cma_prime_mmap
> +	.gem_prime_mmap		= drm_gem_prime_mmap
>  
>  /**
>   * DRM_GEM_CMA_DRIVER_OPS - CMA GEM driver operations
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread