[PATCH 0/3] drm/cma-helper: Clean up public interface

[PATCH 0/3] drm/cma-helper: Clean up public interface

[Thomas Zimmermann]

Convert GEM CMA functions to accept struct drm_gem_cma_object, provide
small wrappers for GEM object callbacks and update all users. Brings
up the interface to the pattern used in SHMEM and VRAM helpers.

Converting all GEM object functions to use drm_gem_cma_object enables
type checking by the C compiler. Previous callers could have passed any
implementation of drm_gem_object to the GEM CMA helpers. It also
removes upcasting in the GEM functions and simplifies the caller side.
No functional changes.

For GEM object callbacks, the CMA helper library now provides a
number of small wrappers that do the necessary upcasting. Again no
functional changes.

Thomas Zimmermann (3):
  drm/cma-helper: Move driver and file ops to the end of header
  drm/cma-helper: Export dedicated wrappers for GEM object functions
  drm/cma-helper: Pass GEM CMA object in public interfaces

 drivers/gpu/drm/drm_gem_cma_helper.c  |  73 +++++-----
 drivers/gpu/drm/rcar-du/rcar_du_kms.c |  10 +-
 drivers/gpu/drm/vc4/vc4_bo.c          |   8 +-
 include/drm/drm_gem_cma_helper.h      | 189 +++++++++++++++++++-------
 4 files changed, 180 insertions(+), 100 deletions(-)


base-commit: 9fccd12cfac1c863fa46d4d17c2d8ac25a44b190
--
2.33.1

[PATCH 1/3] drm/cma-helper: Move driver and file ops to the end of header

[Thomas Zimmermann]

Restructure the header file for CMA helpers by moving declarations
for driver and file operations to the end of the file. No functional
changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 include/drm/drm_gem_cma_helper.h | 114 ++++++++++++++++---------------
 1 file changed, 60 insertions(+), 54 deletions(-)

diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
index cd13508acbc1..e0fb7a0cf03f 100644
--- a/include/drm/drm_gem_cma_helper.h
+++ b/include/drm/drm_gem_cma_helper.h
@@ -32,77 +32,40 @@ struct drm_gem_cma_object {
 #define to_drm_gem_cma_obj(gem_obj) \
 	container_of(gem_obj, struct drm_gem_cma_object, base)
 
-#ifndef CONFIG_MMU
-#define DRM_GEM_CMA_UNMAPPED_AREA_FOPS \
-	.get_unmapped_area	= drm_gem_cma_get_unmapped_area,
-#else
-#define DRM_GEM_CMA_UNMAPPED_AREA_FOPS
-#endif
-
-/**
- * DEFINE_DRM_GEM_CMA_FOPS() - macro to generate file operations for CMA drivers
- * @name: name for the generated structure
- *
- * This macro autogenerates a suitable &struct file_operations for CMA based
- * drivers, which can be assigned to &drm_driver.fops. Note that this structure
- * cannot be shared between drivers, because it contains a reference to the
- * current module using THIS_MODULE.
- *
- * Note that the declaration is already marked as static - if you need a
- * non-static version of this you're probably doing it wrong and will break the
- * THIS_MODULE reference by accident.
- */
-#define DEFINE_DRM_GEM_CMA_FOPS(name) \
-	static const struct file_operations name = {\
-		.owner		= THIS_MODULE,\
-		.open		= drm_open,\
-		.release	= drm_release,\
-		.unlocked_ioctl	= drm_ioctl,\
-		.compat_ioctl	= drm_compat_ioctl,\
-		.poll		= drm_poll,\
-		.read		= drm_read,\
-		.llseek		= noop_llseek,\
-		.mmap		= drm_gem_mmap,\
-		DRM_GEM_CMA_UNMAPPED_AREA_FOPS \
-	}
-
 /* free GEM object */
 void drm_gem_cma_free_object(struct drm_gem_object *gem_obj);
 
-/* create memory region for DRM framebuffer */
-int drm_gem_cma_dumb_create_internal(struct drm_file *file_priv,
-				     struct drm_device *drm,
-				     struct drm_mode_create_dumb *args);
-
-/* create memory region for DRM framebuffer */
-int drm_gem_cma_dumb_create(struct drm_file *file_priv,
-			    struct drm_device *drm,
-			    struct drm_mode_create_dumb *args);
-
 /* allocate physical memory */
 struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
 					      size_t size);
 
 extern const struct vm_operations_struct drm_gem_cma_vm_ops;
 
-#ifndef CONFIG_MMU
-unsigned long drm_gem_cma_get_unmapped_area(struct file *filp,
-					    unsigned long addr,
-					    unsigned long len,
-					    unsigned long pgoff,
-					    unsigned long flags);
-#endif
-
 void drm_gem_cma_print_info(struct drm_printer *p, unsigned int indent,
 			    const struct drm_gem_object *obj);
 
 struct sg_table *drm_gem_cma_get_sg_table(struct drm_gem_object *obj);
+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);
+
+/*
+ * Driver ops
+ */
+
+/* create memory region for DRM framebuffer */
+int drm_gem_cma_dumb_create_internal(struct drm_file *file_priv,
+				     struct drm_device *drm,
+				     struct drm_mode_create_dumb *args);
+
+/* create memory region for DRM framebuffer */
+int drm_gem_cma_dumb_create(struct drm_file *file_priv,
+			    struct drm_device *drm,
+			    struct drm_mode_create_dumb *args);
+
 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_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
@@ -185,4 +148,47 @@ drm_gem_cma_prime_import_sg_table_vmap(struct drm_device *drm,
 				       struct dma_buf_attachment *attach,
 				       struct sg_table *sgt);
 
+/*
+ * File ops
+ */
+
+#ifndef CONFIG_MMU
+unsigned long drm_gem_cma_get_unmapped_area(struct file *filp,
+					    unsigned long addr,
+					    unsigned long len,
+					    unsigned long pgoff,
+					    unsigned long flags);
+#define DRM_GEM_CMA_UNMAPPED_AREA_FOPS \
+	.get_unmapped_area	= drm_gem_cma_get_unmapped_area,
+#else
+#define DRM_GEM_CMA_UNMAPPED_AREA_FOPS
+#endif
+
+/**
+ * DEFINE_DRM_GEM_CMA_FOPS() - macro to generate file operations for CMA drivers
+ * @name: name for the generated structure
+ *
+ * This macro autogenerates a suitable &struct file_operations for CMA based
+ * drivers, which can be assigned to &drm_driver.fops. Note that this structure
+ * cannot be shared between drivers, because it contains a reference to the
+ * current module using THIS_MODULE.
+ *
+ * Note that the declaration is already marked as static - if you need a
+ * non-static version of this you're probably doing it wrong and will break the
+ * THIS_MODULE reference by accident.
+ */
+#define DEFINE_DRM_GEM_CMA_FOPS(name) \
+	static const struct file_operations name = {\
+		.owner		= THIS_MODULE,\
+		.open		= drm_open,\
+		.release	= drm_release,\
+		.unlocked_ioctl	= drm_ioctl,\
+		.compat_ioctl	= drm_compat_ioctl,\
+		.poll		= drm_poll,\
+		.read		= drm_read,\
+		.llseek		= noop_llseek,\
+		.mmap		= drm_gem_mmap,\
+		DRM_GEM_CMA_UNMAPPED_AREA_FOPS \
+	}
+
 #endif /* __DRM_GEM_CMA_HELPER_H__ */
-- 
2.33.1

Re: [PATCH 1/3] drm/cma-helper: Move driver and file ops to the end of header

[Laurent Pinchart]

Hi Thomas,

Thank you for the patch.

On Mon, Nov 15, 2021 at 01:01:46PM +0100, Thomas Zimmermann wrote:
> Restructure the header file for CMA helpers by moving declarations
> for driver and file operations to the end of the file. No functional
> changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

I'm not sure to see what we gain from this, but I don't mind.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  include/drm/drm_gem_cma_helper.h | 114 ++++++++++++++++---------------
>  1 file changed, 60 insertions(+), 54 deletions(-)
> 
> diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
> index cd13508acbc1..e0fb7a0cf03f 100644
> --- a/include/drm/drm_gem_cma_helper.h
> +++ b/include/drm/drm_gem_cma_helper.h
> @@ -32,77 +32,40 @@ struct drm_gem_cma_object {
>  #define to_drm_gem_cma_obj(gem_obj) \
>  	container_of(gem_obj, struct drm_gem_cma_object, base)
>  
> -#ifndef CONFIG_MMU
> -#define DRM_GEM_CMA_UNMAPPED_AREA_FOPS \
> -	.get_unmapped_area	= drm_gem_cma_get_unmapped_area,
> -#else
> -#define DRM_GEM_CMA_UNMAPPED_AREA_FOPS
> -#endif
> -
> -/**
> - * DEFINE_DRM_GEM_CMA_FOPS() - macro to generate file operations for CMA drivers
> - * @name: name for the generated structure
> - *
> - * This macro autogenerates a suitable &struct file_operations for CMA based
> - * drivers, which can be assigned to &drm_driver.fops. Note that this structure
> - * cannot be shared between drivers, because it contains a reference to the
> - * current module using THIS_MODULE.
> - *
> - * Note that the declaration is already marked as static - if you need a
> - * non-static version of this you're probably doing it wrong and will break the
> - * THIS_MODULE reference by accident.
> - */
> -#define DEFINE_DRM_GEM_CMA_FOPS(name) \
> -	static const struct file_operations name = {\
> -		.owner		= THIS_MODULE,\
> -		.open		= drm_open,\
> -		.release	= drm_release,\
> -		.unlocked_ioctl	= drm_ioctl,\
> -		.compat_ioctl	= drm_compat_ioctl,\
> -		.poll		= drm_poll,\
> -		.read		= drm_read,\
> -		.llseek		= noop_llseek,\
> -		.mmap		= drm_gem_mmap,\
> -		DRM_GEM_CMA_UNMAPPED_AREA_FOPS \
> -	}
> -
>  /* free GEM object */
>  void drm_gem_cma_free_object(struct drm_gem_object *gem_obj);
>  
> -/* create memory region for DRM framebuffer */
> -int drm_gem_cma_dumb_create_internal(struct drm_file *file_priv,
> -				     struct drm_device *drm,
> -				     struct drm_mode_create_dumb *args);
> -
> -/* create memory region for DRM framebuffer */
> -int drm_gem_cma_dumb_create(struct drm_file *file_priv,
> -			    struct drm_device *drm,
> -			    struct drm_mode_create_dumb *args);
> -
>  /* allocate physical memory */
>  struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
>  					      size_t size);
>  
>  extern const struct vm_operations_struct drm_gem_cma_vm_ops;
>  
> -#ifndef CONFIG_MMU
> -unsigned long drm_gem_cma_get_unmapped_area(struct file *filp,
> -					    unsigned long addr,
> -					    unsigned long len,
> -					    unsigned long pgoff,
> -					    unsigned long flags);
> -#endif
> -
>  void drm_gem_cma_print_info(struct drm_printer *p, unsigned int indent,
>  			    const struct drm_gem_object *obj);
>  
>  struct sg_table *drm_gem_cma_get_sg_table(struct drm_gem_object *obj);
> +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);
> +
> +/*
> + * Driver ops
> + */
> +
> +/* create memory region for DRM framebuffer */
> +int drm_gem_cma_dumb_create_internal(struct drm_file *file_priv,
> +				     struct drm_device *drm,
> +				     struct drm_mode_create_dumb *args);
> +
> +/* create memory region for DRM framebuffer */
> +int drm_gem_cma_dumb_create(struct drm_file *file_priv,
> +			    struct drm_device *drm,
> +			    struct drm_mode_create_dumb *args);
> +
>  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_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
> @@ -185,4 +148,47 @@ drm_gem_cma_prime_import_sg_table_vmap(struct drm_device *drm,
>  				       struct dma_buf_attachment *attach,
>  				       struct sg_table *sgt);
>  
> +/*
> + * File ops
> + */
> +
> +#ifndef CONFIG_MMU
> +unsigned long drm_gem_cma_get_unmapped_area(struct file *filp,
> +					    unsigned long addr,
> +					    unsigned long len,
> +					    unsigned long pgoff,
> +					    unsigned long flags);
> +#define DRM_GEM_CMA_UNMAPPED_AREA_FOPS \
> +	.get_unmapped_area	= drm_gem_cma_get_unmapped_area,
> +#else
> +#define DRM_GEM_CMA_UNMAPPED_AREA_FOPS
> +#endif
> +
> +/**
> + * DEFINE_DRM_GEM_CMA_FOPS() - macro to generate file operations for CMA drivers
> + * @name: name for the generated structure
> + *
> + * This macro autogenerates a suitable &struct file_operations for CMA based
> + * drivers, which can be assigned to &drm_driver.fops. Note that this structure
> + * cannot be shared between drivers, because it contains a reference to the
> + * current module using THIS_MODULE.
> + *
> + * Note that the declaration is already marked as static - if you need a
> + * non-static version of this you're probably doing it wrong and will break the
> + * THIS_MODULE reference by accident.
> + */
> +#define DEFINE_DRM_GEM_CMA_FOPS(name) \
> +	static const struct file_operations name = {\
> +		.owner		= THIS_MODULE,\
> +		.open		= drm_open,\
> +		.release	= drm_release,\
> +		.unlocked_ioctl	= drm_ioctl,\
> +		.compat_ioctl	= drm_compat_ioctl,\
> +		.poll		= drm_poll,\
> +		.read		= drm_read,\
> +		.llseek		= noop_llseek,\
> +		.mmap		= drm_gem_mmap,\
> +		DRM_GEM_CMA_UNMAPPED_AREA_FOPS \
> +	}
> +
>  #endif /* __DRM_GEM_CMA_HELPER_H__ */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] drm/cma-helper: Move driver and file ops to the end of header

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 6739 bytes --]

Hi Laurent

Am 15.11.21 um 14:40 schrieb Laurent Pinchart:
> Hi Thomas,
> 
> Thank you for the patch.
> 
> On Mon, Nov 15, 2021 at 01:01:46PM +0100, Thomas Zimmermann wrote:
>> Restructure the header file for CMA helpers by moving declarations
>> for driver and file operations to the end of the file. No functional
>> changes.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> I'm not sure to see what we gain from this, but I don't mind.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks. The patch only prepares the file such that the rest of the 
series looks a bit nicer.

Best regards
Thomas

> 
>> ---
>>   include/drm/drm_gem_cma_helper.h | 114 ++++++++++++++++---------------
>>   1 file changed, 60 insertions(+), 54 deletions(-)
>>
>> diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
>> index cd13508acbc1..e0fb7a0cf03f 100644
>> --- a/include/drm/drm_gem_cma_helper.h
>> +++ b/include/drm/drm_gem_cma_helper.h
>> @@ -32,77 +32,40 @@ struct drm_gem_cma_object {
>>   #define to_drm_gem_cma_obj(gem_obj) \
>>   	container_of(gem_obj, struct drm_gem_cma_object, base)
>>   
>> -#ifndef CONFIG_MMU
>> -#define DRM_GEM_CMA_UNMAPPED_AREA_FOPS \
>> -	.get_unmapped_area	= drm_gem_cma_get_unmapped_area,
>> -#else
>> -#define DRM_GEM_CMA_UNMAPPED_AREA_FOPS
>> -#endif
>> -
>> -/**
>> - * DEFINE_DRM_GEM_CMA_FOPS() - macro to generate file operations for CMA drivers
>> - * @name: name for the generated structure
>> - *
>> - * This macro autogenerates a suitable &struct file_operations for CMA based
>> - * drivers, which can be assigned to &drm_driver.fops. Note that this structure
>> - * cannot be shared between drivers, because it contains a reference to the
>> - * current module using THIS_MODULE.
>> - *
>> - * Note that the declaration is already marked as static - if you need a
>> - * non-static version of this you're probably doing it wrong and will break the
>> - * THIS_MODULE reference by accident.
>> - */
>> -#define DEFINE_DRM_GEM_CMA_FOPS(name) \
>> -	static const struct file_operations name = {\
>> -		.owner		= THIS_MODULE,\
>> -		.open		= drm_open,\
>> -		.release	= drm_release,\
>> -		.unlocked_ioctl	= drm_ioctl,\
>> -		.compat_ioctl	= drm_compat_ioctl,\
>> -		.poll		= drm_poll,\
>> -		.read		= drm_read,\
>> -		.llseek		= noop_llseek,\
>> -		.mmap		= drm_gem_mmap,\
>> -		DRM_GEM_CMA_UNMAPPED_AREA_FOPS \
>> -	}
>> -
>>   /* free GEM object */
>>   void drm_gem_cma_free_object(struct drm_gem_object *gem_obj);
>>   
>> -/* create memory region for DRM framebuffer */
>> -int drm_gem_cma_dumb_create_internal(struct drm_file *file_priv,
>> -				     struct drm_device *drm,
>> -				     struct drm_mode_create_dumb *args);
>> -
>> -/* create memory region for DRM framebuffer */
>> -int drm_gem_cma_dumb_create(struct drm_file *file_priv,
>> -			    struct drm_device *drm,
>> -			    struct drm_mode_create_dumb *args);
>> -
>>   /* allocate physical memory */
>>   struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
>>   					      size_t size);
>>   
>>   extern const struct vm_operations_struct drm_gem_cma_vm_ops;
>>   
>> -#ifndef CONFIG_MMU
>> -unsigned long drm_gem_cma_get_unmapped_area(struct file *filp,
>> -					    unsigned long addr,
>> -					    unsigned long len,
>> -					    unsigned long pgoff,
>> -					    unsigned long flags);
>> -#endif
>> -
>>   void drm_gem_cma_print_info(struct drm_printer *p, unsigned int indent,
>>   			    const struct drm_gem_object *obj);
>>   
>>   struct sg_table *drm_gem_cma_get_sg_table(struct drm_gem_object *obj);
>> +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);
>> +
>> +/*
>> + * Driver ops
>> + */
>> +
>> +/* create memory region for DRM framebuffer */
>> +int drm_gem_cma_dumb_create_internal(struct drm_file *file_priv,
>> +				     struct drm_device *drm,
>> +				     struct drm_mode_create_dumb *args);
>> +
>> +/* create memory region for DRM framebuffer */
>> +int drm_gem_cma_dumb_create(struct drm_file *file_priv,
>> +			    struct drm_device *drm,
>> +			    struct drm_mode_create_dumb *args);
>> +
>>   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_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
>> @@ -185,4 +148,47 @@ drm_gem_cma_prime_import_sg_table_vmap(struct drm_device *drm,
>>   				       struct dma_buf_attachment *attach,
>>   				       struct sg_table *sgt);
>>   
>> +/*
>> + * File ops
>> + */
>> +
>> +#ifndef CONFIG_MMU
>> +unsigned long drm_gem_cma_get_unmapped_area(struct file *filp,
>> +					    unsigned long addr,
>> +					    unsigned long len,
>> +					    unsigned long pgoff,
>> +					    unsigned long flags);
>> +#define DRM_GEM_CMA_UNMAPPED_AREA_FOPS \
>> +	.get_unmapped_area	= drm_gem_cma_get_unmapped_area,
>> +#else
>> +#define DRM_GEM_CMA_UNMAPPED_AREA_FOPS
>> +#endif
>> +
>> +/**
>> + * DEFINE_DRM_GEM_CMA_FOPS() - macro to generate file operations for CMA drivers
>> + * @name: name for the generated structure
>> + *
>> + * This macro autogenerates a suitable &struct file_operations for CMA based
>> + * drivers, which can be assigned to &drm_driver.fops. Note that this structure
>> + * cannot be shared between drivers, because it contains a reference to the
>> + * current module using THIS_MODULE.
>> + *
>> + * Note that the declaration is already marked as static - if you need a
>> + * non-static version of this you're probably doing it wrong and will break the
>> + * THIS_MODULE reference by accident.
>> + */
>> +#define DEFINE_DRM_GEM_CMA_FOPS(name) \
>> +	static const struct file_operations name = {\
>> +		.owner		= THIS_MODULE,\
>> +		.open		= drm_open,\
>> +		.release	= drm_release,\
>> +		.unlocked_ioctl	= drm_ioctl,\
>> +		.compat_ioctl	= drm_compat_ioctl,\
>> +		.poll		= drm_poll,\
>> +		.read		= drm_read,\
>> +		.llseek		= noop_llseek,\
>> +		.mmap		= drm_gem_mmap,\
>> +		DRM_GEM_CMA_UNMAPPED_AREA_FOPS \
>> +	}
>> +
>>   #endif /* __DRM_GEM_CMA_HELPER_H__ */
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 1/3] drm/cma-helper: Move driver and file ops to the end of header

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 344 bytes --]

On Mon, Nov 15, 2021 at 01:01:46PM +0100, Thomas Zimmermann wrote:
> Restructure the header file for CMA helpers by moving declarations
> for driver and file operations to the end of the file. No functional
> changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Reviewed-by: Maxime Ripard <maxime@cerno.tech>

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 2/3] drm/cma-helper: Export dedicated wrappers for GEM object functions

[Thomas Zimmermann]

Wrap GEM CMA functions for struct drm_gem_object_funcs and update
all callers. This will allow for an update of the public interfaces
of the GEM CMA helper library.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_cma_helper.c  | 23 ++++----
 drivers/gpu/drm/rcar-du/rcar_du_kms.c | 10 ++--
 drivers/gpu/drm/vc4/vc4_bo.c          |  4 +-
 include/drm/drm_gem_cma_helper.h      | 78 +++++++++++++++++++++++++++
 4 files changed, 94 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
index 09e2cb80de08..27ccb71e3d66 100644
--- a/drivers/gpu/drm/drm_gem_cma_helper.c
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -35,11 +35,11 @@
  */
 
 static const struct drm_gem_object_funcs drm_gem_cma_default_funcs = {
-	.free = drm_gem_cma_free_object,
-	.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,
+	.free = drm_gem_cma_object_free,
+	.print_info = drm_gem_cma_object_print_info,
+	.get_sg_table = drm_gem_cma_object_get_sg_table,
+	.vmap = drm_gem_cma_object_vmap,
+	.mmap = drm_gem_cma_object_mmap,
 	.vm_ops = &drm_gem_cma_vm_ops,
 };
 
@@ -198,8 +198,6 @@ drm_gem_cma_create_with_handle(struct drm_file *file_priv,
  * This function frees the backing memory of the CMA GEM object, cleans up the
  * GEM object state and frees the memory used to store the object itself.
  * If the buffer is imported and the virtual address is set, it is released.
- * Drivers using the CMA helpers should set this as their
- * &drm_gem_object_funcs.free callback.
  */
 void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
 {
@@ -393,9 +391,8 @@ EXPORT_SYMBOL(drm_gem_cma_print_info);
  *     pages for a CMA GEM object
  * @obj: GEM object
  *
- * This function exports a scatter/gather table by
- * calling the standard DMA mapping API. Drivers using the CMA helpers should
- * set this as their &drm_gem_object_funcs.get_sg_table callback.
+ * This function exports a scatter/gather table by calling the standard
+ * DMA mapping API.
  *
  * Returns:
  * A pointer to the scatter/gather table of pinned pages or NULL on failure.
@@ -475,8 +472,7 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_prime_import_sg_table);
  * This function maps a buffer into the kernel's
  * virtual address space. Since the CMA buffers are already mapped into the
  * kernel virtual address space this simply returns the cached virtual
- * address. Drivers using the CMA helpers should set this as their DRM
- * driver's &drm_gem_object_funcs.vmap callback.
+ * address.
  *
  * Returns:
  * 0 on success, or a negative error code otherwise.
@@ -498,8 +494,7 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_vmap);
  *
  * 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.
+ * object instead of using on-demand faulting.
  *
  * Returns:
  * 0 on success or a negative error code on failure.
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index eacb1f17f747..190dbb7f15dd 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -327,11 +327,11 @@ const struct rcar_du_format_info *rcar_du_format_info(u32 fourcc)
  */
 
 static const struct drm_gem_object_funcs rcar_du_gem_funcs = {
-	.free = drm_gem_cma_free_object,
-	.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,
+	.free = drm_gem_cma_object_free,
+	.print_info = drm_gem_cma_object_print_info,
+	.get_sg_table = drm_gem_cma_object_get_sg_table,
+	.vmap = drm_gem_cma_object_vmap,
+	.mmap = drm_gem_cma_object_mmap,
 	.vm_ops = &drm_gem_cma_vm_ops,
 };
 
diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
index fddaeb0b09c1..830756b3159e 100644
--- a/drivers/gpu/drm/vc4/vc4_bo.c
+++ b/drivers/gpu/drm/vc4/vc4_bo.c
@@ -732,8 +732,8 @@ static const struct vm_operations_struct vc4_vm_ops = {
 static const struct drm_gem_object_funcs vc4_gem_object_funcs = {
 	.free = vc4_free_object,
 	.export = vc4_prime_export,
-	.get_sg_table = drm_gem_cma_get_sg_table,
-	.vmap = drm_gem_cma_vmap,
+	.get_sg_table = drm_gem_cma_object_get_sg_table,
+	.vmap = drm_gem_cma_object_vmap,
 	.mmap = vc4_gem_object_mmap,
 	.vm_ops = &vc4_vm_ops,
 };
diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
index e0fb7a0cf03f..56d2f9fdf9ac 100644
--- a/include/drm/drm_gem_cma_helper.h
+++ b/include/drm/drm_gem_cma_helper.h
@@ -48,6 +48,84 @@ struct sg_table *drm_gem_cma_get_sg_table(struct drm_gem_object *obj);
 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);
 
+/*
+ * GEM object functions
+ */
+
+/**
+ * drm_gem_cma_object_free - GEM object function for drm_gem_cma_free_object()
+ * @obj: GEM object to free
+ *
+ * This function wraps drm_gem_cma_free_object(). Drivers that employ the CMA helpers
+ * should use it as their &drm_gem_object_funcs.free handler.
+ */
+static inline void drm_gem_cma_object_free(struct drm_gem_object *obj)
+{
+	drm_gem_cma_free_object(obj);
+}
+
+/**
+ * drm_gem_cma_object_print_info() - Print &drm_gem_cma_object info for debugfs
+ * @p: DRM printer
+ * @indent: Tab indentation level
+ * @obj: GEM object
+ *
+ * This function wraps drm_gem_cma_print_info(). Drivers that employ the CMA helpers
+ * should use this function as their &drm_gem_object_funcs.print_info handler.
+ */
+static inline void drm_gem_cma_object_print_info(struct drm_printer *p, unsigned int indent,
+						 const struct drm_gem_object *obj)
+{
+	drm_gem_cma_print_info(p, indent, obj);
+}
+
+/**
+ * drm_gem_cma_object_get_sg_table - GEM object function for drm_gem_cma_get_sg_table()
+ * @obj: GEM object
+ *
+ * This function wraps drm_gem_cma_get_sg_table(). Drivers that employ the CMA helpers should
+ * use it as their &drm_gem_object_funcs.get_sg_table handler.
+ *
+ * Returns:
+ * A pointer to the scatter/gather table of pinned pages or NULL on failure.
+ */
+static inline struct sg_table *drm_gem_cma_object_get_sg_table(struct drm_gem_object *obj)
+{
+	return drm_gem_cma_get_sg_table(obj);
+}
+
+/*
+ * drm_gem_cma_object_vmap - GEM object function for drm_gem_cma_vmap()
+ * @obj: GEM object
+ * @map: Returns the kernel virtual address of the CMA GEM object's backing store.
+ *
+ * This function wraps drm_gem_cma_vmap(). Drivers that employ the CMA helpers should
+ * use it as their &drm_gem_object_funcs.vmap handler.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+static inline int drm_gem_cma_object_vmap(struct drm_gem_object *obj, struct dma_buf_map *map)
+{
+	return drm_gem_cma_vmap(obj, map);
+}
+
+/**
+ * drm_gem_cma_object_mmap - GEM object function for drm_gem_cma_mmap()
+ * @obj: GEM object
+ * @vma: VMA for the area to be mapped
+ *
+ * This function wraps drm_gem_cma_mmap(). Drivers that employ the cma helpers should
+ * use it as their &drm_gem_object_funcs.mmap handler.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+static inline int drm_gem_cma_object_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
+{
+	return drm_gem_cma_mmap(obj, vma);
+}
+
 /*
  * Driver ops
  */
-- 
2.33.1

Re: [PATCH 2/3] drm/cma-helper: Export dedicated wrappers for GEM object functions

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 364 bytes --]

On Mon, Nov 15, 2021 at 01:01:47PM +0100, Thomas Zimmermann wrote:
> Wrap GEM CMA functions for struct drm_gem_object_funcs and update
> all callers. This will allow for an update of the public interfaces
> of the GEM CMA helper library.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Reviewed-by: Maxime Ripard <maxime@cerno.tech>

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 3/3] drm/cma-helper: Pass GEM CMA object in public interfaces

[Thomas Zimmermann]

Change all GEM CMA object functions that receive a GEM object
of type struct drm_gem_object to expect an object of type
struct drm_gem_cma_object instead.

This change reduces the number of upcasts from struct drm_gem_object
by moving them into callers. The C compiler can now verify that the
GEM CMA functions are called with the correct type.

For consistency, the patch also renames drm_gem_cma_free_object to
drm_gem_cma_free. It further updates documentation for a number of
functions.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_cma_helper.c | 52 +++++++++++++---------------
 drivers/gpu/drm/vc4/vc4_bo.c         |  4 +--
 include/drm/drm_gem_cma_helper.h     | 39 ++++++++++++---------
 3 files changed, 48 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
index 27ccb71e3d66..7d4895de9e0d 100644
--- a/drivers/gpu/drm/drm_gem_cma_helper.c
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -32,6 +32,10 @@
  * The DRM GEM/CMA helpers use this allocator as a means to provide buffer
  * objects that are physically contiguous in memory. This is useful for
  * display drivers that are unable to map scattered buffers via an IOMMU.
+ *
+ * For GEM callback helpers in struct &drm_gem_object functions, see likewise
+ * named functions with an _object_ infix (e.g., drm_gem_cma_object_vmap() wraps
+ * drm_gem_cma_vmap()). These helpers perform the necessary type conversion.
  */
 
 static const struct drm_gem_object_funcs drm_gem_cma_default_funcs = {
@@ -192,16 +196,16 @@ drm_gem_cma_create_with_handle(struct drm_file *file_priv,
 }
 
 /**
- * drm_gem_cma_free_object - free resources associated with a CMA GEM object
- * @gem_obj: GEM object to free
+ * drm_gem_cma_free - free resources associated with a CMA GEM object
+ * @cma_obj: CMA GEM object to free
  *
  * This function frees the backing memory of the CMA GEM object, cleans up the
  * GEM object state and frees the memory used to store the object itself.
  * If the buffer is imported and the virtual address is set, it is released.
  */
-void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
+void drm_gem_cma_free(struct drm_gem_cma_object *cma_obj)
 {
-	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(gem_obj);
+	struct drm_gem_object *gem_obj = &cma_obj->base;
 	struct dma_buf_map map = DMA_BUF_MAP_INIT_VADDR(cma_obj->vaddr);
 
 	if (gem_obj->import_attach) {
@@ -222,7 +226,7 @@ void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
 
 	kfree(cma_obj);
 }
-EXPORT_SYMBOL_GPL(drm_gem_cma_free_object);
+EXPORT_SYMBOL_GPL(drm_gem_cma_free);
 
 /**
  * drm_gem_cma_dumb_create_internal - create a dumb buffer object
@@ -369,18 +373,15 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_get_unmapped_area);
 
 /**
  * drm_gem_cma_print_info() - Print &drm_gem_cma_object info for debugfs
+ * @cma_obj: CMA GEM object
  * @p: DRM printer
  * @indent: Tab indentation level
- * @obj: GEM object
  *
- * This function can be used as the &drm_driver->gem_print_info callback.
- * It prints paddr and vaddr for use in e.g. debugfs output.
+ * This function prints paddr and vaddr for use in e.g. debugfs output.
  */
-void drm_gem_cma_print_info(struct drm_printer *p, unsigned int indent,
-			    const struct drm_gem_object *obj)
+void drm_gem_cma_print_info(const struct drm_gem_cma_object *cma_obj,
+			    struct drm_printer *p, unsigned int indent)
 {
-	const struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
-
 	drm_printf_indent(p, indent, "paddr=%pad\n", &cma_obj->paddr);
 	drm_printf_indent(p, indent, "vaddr=%p\n", cma_obj->vaddr);
 }
@@ -389,7 +390,7 @@ EXPORT_SYMBOL(drm_gem_cma_print_info);
 /**
  * drm_gem_cma_get_sg_table - provide a scatter/gather table of pinned
  *     pages for a CMA GEM object
- * @obj: GEM object
+ * @cma_obj: CMA GEM object
  *
  * This function exports a scatter/gather table by calling the standard
  * DMA mapping API.
@@ -397,9 +398,9 @@ EXPORT_SYMBOL(drm_gem_cma_print_info);
  * Returns:
  * A pointer to the scatter/gather table of pinned pages or NULL on failure.
  */
-struct sg_table *drm_gem_cma_get_sg_table(struct drm_gem_object *obj)
+struct sg_table *drm_gem_cma_get_sg_table(struct drm_gem_cma_object *cma_obj)
 {
-	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
+	struct drm_gem_object *obj = &cma_obj->base;
 	struct sg_table *sgt;
 	int ret;
 
@@ -465,22 +466,19 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_prime_import_sg_table);
 /**
  * drm_gem_cma_vmap - map a CMA GEM object into the kernel's virtual
  *     address space
- * @obj: GEM object
+ * @cma_obj: CMA GEM object
  * @map: Returns the kernel virtual address of the CMA GEM object's backing
  *       store.
  *
- * This function maps a buffer into the kernel's
- * virtual address space. Since the CMA buffers are already mapped into the
- * kernel virtual address space this simply returns the cached virtual
- * address.
+ * This function maps a buffer into the kernel's virtual address space.
+ * Since the CMA buffers are already mapped into the kernel virtual address
+ * space this simply returns the cached virtual address.
  *
  * Returns:
  * 0 on success, or a negative error code otherwise.
  */
-int drm_gem_cma_vmap(struct drm_gem_object *obj, struct dma_buf_map *map)
+int drm_gem_cma_vmap(struct drm_gem_cma_object *cma_obj, struct dma_buf_map *map)
 {
-	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
-
 	dma_buf_map_set_vaddr(map, cma_obj->vaddr);
 
 	return 0;
@@ -489,7 +487,7 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_vmap);
 
 /**
  * drm_gem_cma_mmap - memory-map an exported CMA GEM object
- * @obj: GEM object
+ * @cma_obj: CMA GEM object
  * @vma: VMA for the area to be mapped
  *
  * This function maps a buffer into a userspace process's address space.
@@ -499,9 +497,9 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_vmap);
  * 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)
+int drm_gem_cma_mmap(struct drm_gem_cma_object *cma_obj, struct vm_area_struct *vma)
 {
-	struct drm_gem_cma_object *cma_obj;
+	struct drm_gem_object *obj = &cma_obj->base;
 	int ret;
 
 	/*
@@ -512,8 +510,6 @@ int drm_gem_cma_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
 	vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
 	vma->vm_flags &= ~VM_PFNMAP;
 
-	cma_obj = to_drm_gem_cma_obj(obj);
-
 	if (cma_obj->map_noncoherent) {
 		vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
 
diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
index 830756b3159e..6d1281a343e9 100644
--- a/drivers/gpu/drm/vc4/vc4_bo.c
+++ b/drivers/gpu/drm/vc4/vc4_bo.c
@@ -177,7 +177,7 @@ static void vc4_bo_destroy(struct vc4_bo *bo)
 		bo->validated_shader = NULL;
 	}
 
-	drm_gem_cma_free_object(obj);
+	drm_gem_cma_free(&bo->base);
 }
 
 static void vc4_bo_remove_from_cache(struct vc4_bo *bo)
@@ -720,7 +720,7 @@ static int vc4_gem_object_mmap(struct drm_gem_object *obj, struct vm_area_struct
 		return -EINVAL;
 	}
 
-	return drm_gem_cma_mmap(obj, vma);
+	return drm_gem_cma_mmap(&bo->base, vma);
 }
 
 static const struct vm_operations_struct vc4_vm_ops = {
diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
index 56d2f9fdf9ac..adb507a9dbf0 100644
--- a/include/drm/drm_gem_cma_helper.h
+++ b/include/drm/drm_gem_cma_helper.h
@@ -32,28 +32,23 @@ struct drm_gem_cma_object {
 #define to_drm_gem_cma_obj(gem_obj) \
 	container_of(gem_obj, struct drm_gem_cma_object, base)
 
-/* free GEM object */
-void drm_gem_cma_free_object(struct drm_gem_object *gem_obj);
-
-/* allocate physical memory */
 struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
 					      size_t size);
+void drm_gem_cma_free(struct drm_gem_cma_object *cma_obj);
+void drm_gem_cma_print_info(const struct drm_gem_cma_object *cma_obj,
+			    struct drm_printer *p, unsigned int indent);
+struct sg_table *drm_gem_cma_get_sg_table(struct drm_gem_cma_object *cma_obj);
+int drm_gem_cma_vmap(struct drm_gem_cma_object *cma_obj, struct dma_buf_map *map);
+int drm_gem_cma_mmap(struct drm_gem_cma_object *cma_obj, struct vm_area_struct *vma);
 
 extern const struct vm_operations_struct drm_gem_cma_vm_ops;
 
-void drm_gem_cma_print_info(struct drm_printer *p, unsigned int indent,
-			    const struct drm_gem_object *obj);
-
-struct sg_table *drm_gem_cma_get_sg_table(struct drm_gem_object *obj);
-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);
-
 /*
  * GEM object functions
  */
 
 /**
- * drm_gem_cma_object_free - GEM object function for drm_gem_cma_free_object()
+ * drm_gem_cma_object_free - GEM object function for drm_gem_cma_free()
  * @obj: GEM object to free
  *
  * This function wraps drm_gem_cma_free_object(). Drivers that employ the CMA helpers
@@ -61,7 +56,9 @@ int drm_gem_cma_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma);
  */
 static inline void drm_gem_cma_object_free(struct drm_gem_object *obj)
 {
-	drm_gem_cma_free_object(obj);
+	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
+
+	drm_gem_cma_free(cma_obj);
 }
 
 /**
@@ -76,7 +73,9 @@ static inline void drm_gem_cma_object_free(struct drm_gem_object *obj)
 static inline void drm_gem_cma_object_print_info(struct drm_printer *p, unsigned int indent,
 						 const struct drm_gem_object *obj)
 {
-	drm_gem_cma_print_info(p, indent, obj);
+	const struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
+
+	drm_gem_cma_print_info(cma_obj, p, indent);
 }
 
 /**
@@ -91,7 +90,9 @@ static inline void drm_gem_cma_object_print_info(struct drm_printer *p, unsigned
  */
 static inline struct sg_table *drm_gem_cma_object_get_sg_table(struct drm_gem_object *obj)
 {
-	return drm_gem_cma_get_sg_table(obj);
+	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
+
+	return drm_gem_cma_get_sg_table(cma_obj);
 }
 
 /*
@@ -107,7 +108,9 @@ static inline struct sg_table *drm_gem_cma_object_get_sg_table(struct drm_gem_ob
  */
 static inline int drm_gem_cma_object_vmap(struct drm_gem_object *obj, struct dma_buf_map *map)
 {
-	return drm_gem_cma_vmap(obj, map);
+	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
+
+	return drm_gem_cma_vmap(cma_obj, map);
 }
 
 /**
@@ -123,7 +126,9 @@ static inline int drm_gem_cma_object_vmap(struct drm_gem_object *obj, struct dma
  */
 static inline int drm_gem_cma_object_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
 {
-	return drm_gem_cma_mmap(obj, vma);
+	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
+
+	return drm_gem_cma_mmap(cma_obj, vma);
 }
 
 /*
-- 
2.33.1

Re: [PATCH 3/3] drm/cma-helper: Pass GEM CMA object in public interfaces

[Laurent Pinchart]

Hi Thomas,

Thank you for the patch.

On Mon, Nov 15, 2021 at 01:01:48PM +0100, Thomas Zimmermann wrote:
> Change all GEM CMA object functions that receive a GEM object
> of type struct drm_gem_object to expect an object of type
> struct drm_gem_cma_object instead.
> 
> This change reduces the number of upcasts from struct drm_gem_object
> by moving them into callers. The C compiler can now verify that the
> GEM CMA functions are called with the correct type.
> 
> For consistency, the patch also renames drm_gem_cma_free_object to
> drm_gem_cma_free. It further updates documentation for a number of
> functions.

I'm not convinced to be honest. I won't block this series, but I don't
really see what it brings us.

> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_gem_cma_helper.c | 52 +++++++++++++---------------
>  drivers/gpu/drm/vc4/vc4_bo.c         |  4 +--
>  include/drm/drm_gem_cma_helper.h     | 39 ++++++++++++---------
>  3 files changed, 48 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
> index 27ccb71e3d66..7d4895de9e0d 100644
> --- a/drivers/gpu/drm/drm_gem_cma_helper.c
> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> @@ -32,6 +32,10 @@
>   * The DRM GEM/CMA helpers use this allocator as a means to provide buffer
>   * objects that are physically contiguous in memory. This is useful for
>   * display drivers that are unable to map scattered buffers via an IOMMU.
> + *
> + * For GEM callback helpers in struct &drm_gem_object functions, see likewise
> + * named functions with an _object_ infix (e.g., drm_gem_cma_object_vmap() wraps
> + * drm_gem_cma_vmap()). These helpers perform the necessary type conversion.
>   */
>  
>  static const struct drm_gem_object_funcs drm_gem_cma_default_funcs = {
> @@ -192,16 +196,16 @@ drm_gem_cma_create_with_handle(struct drm_file *file_priv,
>  }
>  
>  /**
> - * drm_gem_cma_free_object - free resources associated with a CMA GEM object
> - * @gem_obj: GEM object to free
> + * drm_gem_cma_free - free resources associated with a CMA GEM object
> + * @cma_obj: CMA GEM object to free
>   *
>   * This function frees the backing memory of the CMA GEM object, cleans up the
>   * GEM object state and frees the memory used to store the object itself.
>   * If the buffer is imported and the virtual address is set, it is released.
>   */
> -void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
> +void drm_gem_cma_free(struct drm_gem_cma_object *cma_obj)
>  {
> -	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(gem_obj);
> +	struct drm_gem_object *gem_obj = &cma_obj->base;
>  	struct dma_buf_map map = DMA_BUF_MAP_INIT_VADDR(cma_obj->vaddr);
>  
>  	if (gem_obj->import_attach) {
> @@ -222,7 +226,7 @@ void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
>  
>  	kfree(cma_obj);
>  }
> -EXPORT_SYMBOL_GPL(drm_gem_cma_free_object);
> +EXPORT_SYMBOL_GPL(drm_gem_cma_free);
>  
>  /**
>   * drm_gem_cma_dumb_create_internal - create a dumb buffer object
> @@ -369,18 +373,15 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_get_unmapped_area);
>  
>  /**
>   * drm_gem_cma_print_info() - Print &drm_gem_cma_object info for debugfs
> + * @cma_obj: CMA GEM object
>   * @p: DRM printer
>   * @indent: Tab indentation level
> - * @obj: GEM object
>   *
> - * This function can be used as the &drm_driver->gem_print_info callback.
> - * It prints paddr and vaddr for use in e.g. debugfs output.
> + * This function prints paddr and vaddr for use in e.g. debugfs output.
>   */
> -void drm_gem_cma_print_info(struct drm_printer *p, unsigned int indent,
> -			    const struct drm_gem_object *obj)
> +void drm_gem_cma_print_info(const struct drm_gem_cma_object *cma_obj,
> +			    struct drm_printer *p, unsigned int indent)
>  {
> -	const struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
> -
>  	drm_printf_indent(p, indent, "paddr=%pad\n", &cma_obj->paddr);
>  	drm_printf_indent(p, indent, "vaddr=%p\n", cma_obj->vaddr);
>  }
> @@ -389,7 +390,7 @@ EXPORT_SYMBOL(drm_gem_cma_print_info);
>  /**
>   * drm_gem_cma_get_sg_table - provide a scatter/gather table of pinned
>   *     pages for a CMA GEM object
> - * @obj: GEM object
> + * @cma_obj: CMA GEM object
>   *
>   * This function exports a scatter/gather table by calling the standard
>   * DMA mapping API.
> @@ -397,9 +398,9 @@ EXPORT_SYMBOL(drm_gem_cma_print_info);
>   * Returns:
>   * A pointer to the scatter/gather table of pinned pages or NULL on failure.
>   */
> -struct sg_table *drm_gem_cma_get_sg_table(struct drm_gem_object *obj)
> +struct sg_table *drm_gem_cma_get_sg_table(struct drm_gem_cma_object *cma_obj)
>  {
> -	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
> +	struct drm_gem_object *obj = &cma_obj->base;
>  	struct sg_table *sgt;
>  	int ret;
>  
> @@ -465,22 +466,19 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_prime_import_sg_table);
>  /**
>   * drm_gem_cma_vmap - map a CMA GEM object into the kernel's virtual
>   *     address space
> - * @obj: GEM object
> + * @cma_obj: CMA GEM object
>   * @map: Returns the kernel virtual address of the CMA GEM object's backing
>   *       store.
>   *
> - * This function maps a buffer into the kernel's
> - * virtual address space. Since the CMA buffers are already mapped into the
> - * kernel virtual address space this simply returns the cached virtual
> - * address.
> + * This function maps a buffer into the kernel's virtual address space.
> + * Since the CMA buffers are already mapped into the kernel virtual address
> + * space this simply returns the cached virtual address.
>   *
>   * Returns:
>   * 0 on success, or a negative error code otherwise.
>   */
> -int drm_gem_cma_vmap(struct drm_gem_object *obj, struct dma_buf_map *map)
> +int drm_gem_cma_vmap(struct drm_gem_cma_object *cma_obj, struct dma_buf_map *map)
>  {
> -	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
> -
>  	dma_buf_map_set_vaddr(map, cma_obj->vaddr);
>  
>  	return 0;
> @@ -489,7 +487,7 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_vmap);
>  
>  /**
>   * drm_gem_cma_mmap - memory-map an exported CMA GEM object
> - * @obj: GEM object
> + * @cma_obj: CMA GEM object
>   * @vma: VMA for the area to be mapped
>   *
>   * This function maps a buffer into a userspace process's address space.
> @@ -499,9 +497,9 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_vmap);
>   * 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)
> +int drm_gem_cma_mmap(struct drm_gem_cma_object *cma_obj, struct vm_area_struct *vma)
>  {
> -	struct drm_gem_cma_object *cma_obj;
> +	struct drm_gem_object *obj = &cma_obj->base;
>  	int ret;
>  
>  	/*
> @@ -512,8 +510,6 @@ int drm_gem_cma_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
>  	vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
>  	vma->vm_flags &= ~VM_PFNMAP;
>  
> -	cma_obj = to_drm_gem_cma_obj(obj);
> -
>  	if (cma_obj->map_noncoherent) {
>  		vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
>  
> diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
> index 830756b3159e..6d1281a343e9 100644
> --- a/drivers/gpu/drm/vc4/vc4_bo.c
> +++ b/drivers/gpu/drm/vc4/vc4_bo.c
> @@ -177,7 +177,7 @@ static void vc4_bo_destroy(struct vc4_bo *bo)
>  		bo->validated_shader = NULL;
>  	}
>  
> -	drm_gem_cma_free_object(obj);
> +	drm_gem_cma_free(&bo->base);
>  }
>  
>  static void vc4_bo_remove_from_cache(struct vc4_bo *bo)
> @@ -720,7 +720,7 @@ static int vc4_gem_object_mmap(struct drm_gem_object *obj, struct vm_area_struct
>  		return -EINVAL;
>  	}
>  
> -	return drm_gem_cma_mmap(obj, vma);
> +	return drm_gem_cma_mmap(&bo->base, vma);
>  }
>  
>  static const struct vm_operations_struct vc4_vm_ops = {
> diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
> index 56d2f9fdf9ac..adb507a9dbf0 100644
> --- a/include/drm/drm_gem_cma_helper.h
> +++ b/include/drm/drm_gem_cma_helper.h
> @@ -32,28 +32,23 @@ struct drm_gem_cma_object {
>  #define to_drm_gem_cma_obj(gem_obj) \
>  	container_of(gem_obj, struct drm_gem_cma_object, base)
>  
> -/* free GEM object */
> -void drm_gem_cma_free_object(struct drm_gem_object *gem_obj);
> -
> -/* allocate physical memory */
>  struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
>  					      size_t size);
> +void drm_gem_cma_free(struct drm_gem_cma_object *cma_obj);
> +void drm_gem_cma_print_info(const struct drm_gem_cma_object *cma_obj,
> +			    struct drm_printer *p, unsigned int indent);
> +struct sg_table *drm_gem_cma_get_sg_table(struct drm_gem_cma_object *cma_obj);
> +int drm_gem_cma_vmap(struct drm_gem_cma_object *cma_obj, struct dma_buf_map *map);
> +int drm_gem_cma_mmap(struct drm_gem_cma_object *cma_obj, struct vm_area_struct *vma);
>  
>  extern const struct vm_operations_struct drm_gem_cma_vm_ops;
>  
> -void drm_gem_cma_print_info(struct drm_printer *p, unsigned int indent,
> -			    const struct drm_gem_object *obj);
> -
> -struct sg_table *drm_gem_cma_get_sg_table(struct drm_gem_object *obj);
> -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);
> -
>  /*
>   * GEM object functions
>   */
>  
>  /**
> - * drm_gem_cma_object_free - GEM object function for drm_gem_cma_free_object()
> + * drm_gem_cma_object_free - GEM object function for drm_gem_cma_free()
>   * @obj: GEM object to free
>   *
>   * This function wraps drm_gem_cma_free_object(). Drivers that employ the CMA helpers
> @@ -61,7 +56,9 @@ int drm_gem_cma_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma);
>   */
>  static inline void drm_gem_cma_object_free(struct drm_gem_object *obj)
>  {
> -	drm_gem_cma_free_object(obj);
> +	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
> +
> +	drm_gem_cma_free(cma_obj);
>  }
>  
>  /**
> @@ -76,7 +73,9 @@ static inline void drm_gem_cma_object_free(struct drm_gem_object *obj)
>  static inline void drm_gem_cma_object_print_info(struct drm_printer *p, unsigned int indent,
>  						 const struct drm_gem_object *obj)
>  {
> -	drm_gem_cma_print_info(p, indent, obj);
> +	const struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
> +
> +	drm_gem_cma_print_info(cma_obj, p, indent);
>  }
>  
>  /**
> @@ -91,7 +90,9 @@ static inline void drm_gem_cma_object_print_info(struct drm_printer *p, unsigned
>   */
>  static inline struct sg_table *drm_gem_cma_object_get_sg_table(struct drm_gem_object *obj)
>  {
> -	return drm_gem_cma_get_sg_table(obj);
> +	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
> +
> +	return drm_gem_cma_get_sg_table(cma_obj);
>  }
>  
>  /*
> @@ -107,7 +108,9 @@ static inline struct sg_table *drm_gem_cma_object_get_sg_table(struct drm_gem_ob
>   */
>  static inline int drm_gem_cma_object_vmap(struct drm_gem_object *obj, struct dma_buf_map *map)
>  {
> -	return drm_gem_cma_vmap(obj, map);
> +	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
> +
> +	return drm_gem_cma_vmap(cma_obj, map);
>  }
>  
>  /**
> @@ -123,7 +126,9 @@ static inline int drm_gem_cma_object_vmap(struct drm_gem_object *obj, struct dma
>   */
>  static inline int drm_gem_cma_object_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
>  {
> -	return drm_gem_cma_mmap(obj, vma);
> +	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
> +
> +	return drm_gem_cma_mmap(cma_obj, vma);
>  }
>  
>  /*

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] drm/cma-helper: Pass GEM CMA object in public interfaces

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 12715 bytes --]

Hi Laurent

Am 15.11.21 um 14:50 schrieb Laurent Pinchart:
> Hi Thomas,
> 
> Thank you for the patch.
> 
> On Mon, Nov 15, 2021 at 01:01:48PM +0100, Thomas Zimmermann wrote:
>> Change all GEM CMA object functions that receive a GEM object
>> of type struct drm_gem_object to expect an object of type
>> struct drm_gem_cma_object instead.
>>
>> This change reduces the number of upcasts from struct drm_gem_object
>> by moving them into callers. The C compiler can now verify that the
>> GEM CMA functions are called with the correct type.
>>
>> For consistency, the patch also renames drm_gem_cma_free_object to
>> drm_gem_cma_free. It further updates documentation for a number of
>> functions.
> 
> I'm not convinced to be honest. I won't block this series, but I don't
> really see what it brings us.

Type checking. It's much harder to accidentaly pass a wrong GEM object 
(SHMEM, VRAM, etc) to the functions now. I know that it's not been a 
super-huge problem so far, but still worth addressing IMHO.

Best regards
Thomas

> 
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/drm_gem_cma_helper.c | 52 +++++++++++++---------------
>>   drivers/gpu/drm/vc4/vc4_bo.c         |  4 +--
>>   include/drm/drm_gem_cma_helper.h     | 39 ++++++++++++---------
>>   3 files changed, 48 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
>> index 27ccb71e3d66..7d4895de9e0d 100644
>> --- a/drivers/gpu/drm/drm_gem_cma_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
>> @@ -32,6 +32,10 @@
>>    * The DRM GEM/CMA helpers use this allocator as a means to provide buffer
>>    * objects that are physically contiguous in memory. This is useful for
>>    * display drivers that are unable to map scattered buffers via an IOMMU.
>> + *
>> + * For GEM callback helpers in struct &drm_gem_object functions, see likewise
>> + * named functions with an _object_ infix (e.g., drm_gem_cma_object_vmap() wraps
>> + * drm_gem_cma_vmap()). These helpers perform the necessary type conversion.
>>    */
>>   
>>   static const struct drm_gem_object_funcs drm_gem_cma_default_funcs = {
>> @@ -192,16 +196,16 @@ drm_gem_cma_create_with_handle(struct drm_file *file_priv,
>>   }
>>   
>>   /**
>> - * drm_gem_cma_free_object - free resources associated with a CMA GEM object
>> - * @gem_obj: GEM object to free
>> + * drm_gem_cma_free - free resources associated with a CMA GEM object
>> + * @cma_obj: CMA GEM object to free
>>    *
>>    * This function frees the backing memory of the CMA GEM object, cleans up the
>>    * GEM object state and frees the memory used to store the object itself.
>>    * If the buffer is imported and the virtual address is set, it is released.
>>    */
>> -void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
>> +void drm_gem_cma_free(struct drm_gem_cma_object *cma_obj)
>>   {
>> -	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(gem_obj);
>> +	struct drm_gem_object *gem_obj = &cma_obj->base;
>>   	struct dma_buf_map map = DMA_BUF_MAP_INIT_VADDR(cma_obj->vaddr);
>>   
>>   	if (gem_obj->import_attach) {
>> @@ -222,7 +226,7 @@ void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
>>   
>>   	kfree(cma_obj);
>>   }
>> -EXPORT_SYMBOL_GPL(drm_gem_cma_free_object);
>> +EXPORT_SYMBOL_GPL(drm_gem_cma_free);
>>   
>>   /**
>>    * drm_gem_cma_dumb_create_internal - create a dumb buffer object
>> @@ -369,18 +373,15 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_get_unmapped_area);
>>   
>>   /**
>>    * drm_gem_cma_print_info() - Print &drm_gem_cma_object info for debugfs
>> + * @cma_obj: CMA GEM object
>>    * @p: DRM printer
>>    * @indent: Tab indentation level
>> - * @obj: GEM object
>>    *
>> - * This function can be used as the &drm_driver->gem_print_info callback.
>> - * It prints paddr and vaddr for use in e.g. debugfs output.
>> + * This function prints paddr and vaddr for use in e.g. debugfs output.
>>    */
>> -void drm_gem_cma_print_info(struct drm_printer *p, unsigned int indent,
>> -			    const struct drm_gem_object *obj)
>> +void drm_gem_cma_print_info(const struct drm_gem_cma_object *cma_obj,
>> +			    struct drm_printer *p, unsigned int indent)
>>   {
>> -	const struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
>> -
>>   	drm_printf_indent(p, indent, "paddr=%pad\n", &cma_obj->paddr);
>>   	drm_printf_indent(p, indent, "vaddr=%p\n", cma_obj->vaddr);
>>   }
>> @@ -389,7 +390,7 @@ EXPORT_SYMBOL(drm_gem_cma_print_info);
>>   /**
>>    * drm_gem_cma_get_sg_table - provide a scatter/gather table of pinned
>>    *     pages for a CMA GEM object
>> - * @obj: GEM object
>> + * @cma_obj: CMA GEM object
>>    *
>>    * This function exports a scatter/gather table by calling the standard
>>    * DMA mapping API.
>> @@ -397,9 +398,9 @@ EXPORT_SYMBOL(drm_gem_cma_print_info);
>>    * Returns:
>>    * A pointer to the scatter/gather table of pinned pages or NULL on failure.
>>    */
>> -struct sg_table *drm_gem_cma_get_sg_table(struct drm_gem_object *obj)
>> +struct sg_table *drm_gem_cma_get_sg_table(struct drm_gem_cma_object *cma_obj)
>>   {
>> -	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
>> +	struct drm_gem_object *obj = &cma_obj->base;
>>   	struct sg_table *sgt;
>>   	int ret;
>>   
>> @@ -465,22 +466,19 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_prime_import_sg_table);
>>   /**
>>    * drm_gem_cma_vmap - map a CMA GEM object into the kernel's virtual
>>    *     address space
>> - * @obj: GEM object
>> + * @cma_obj: CMA GEM object
>>    * @map: Returns the kernel virtual address of the CMA GEM object's backing
>>    *       store.
>>    *
>> - * This function maps a buffer into the kernel's
>> - * virtual address space. Since the CMA buffers are already mapped into the
>> - * kernel virtual address space this simply returns the cached virtual
>> - * address.
>> + * This function maps a buffer into the kernel's virtual address space.
>> + * Since the CMA buffers are already mapped into the kernel virtual address
>> + * space this simply returns the cached virtual address.
>>    *
>>    * Returns:
>>    * 0 on success, or a negative error code otherwise.
>>    */
>> -int drm_gem_cma_vmap(struct drm_gem_object *obj, struct dma_buf_map *map)
>> +int drm_gem_cma_vmap(struct drm_gem_cma_object *cma_obj, struct dma_buf_map *map)
>>   {
>> -	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
>> -
>>   	dma_buf_map_set_vaddr(map, cma_obj->vaddr);
>>   
>>   	return 0;
>> @@ -489,7 +487,7 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_vmap);
>>   
>>   /**
>>    * drm_gem_cma_mmap - memory-map an exported CMA GEM object
>> - * @obj: GEM object
>> + * @cma_obj: CMA GEM object
>>    * @vma: VMA for the area to be mapped
>>    *
>>    * This function maps a buffer into a userspace process's address space.
>> @@ -499,9 +497,9 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_vmap);
>>    * 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)
>> +int drm_gem_cma_mmap(struct drm_gem_cma_object *cma_obj, struct vm_area_struct *vma)
>>   {
>> -	struct drm_gem_cma_object *cma_obj;
>> +	struct drm_gem_object *obj = &cma_obj->base;
>>   	int ret;
>>   
>>   	/*
>> @@ -512,8 +510,6 @@ int drm_gem_cma_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
>>   	vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
>>   	vma->vm_flags &= ~VM_PFNMAP;
>>   
>> -	cma_obj = to_drm_gem_cma_obj(obj);
>> -
>>   	if (cma_obj->map_noncoherent) {
>>   		vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
>>   
>> diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
>> index 830756b3159e..6d1281a343e9 100644
>> --- a/drivers/gpu/drm/vc4/vc4_bo.c
>> +++ b/drivers/gpu/drm/vc4/vc4_bo.c
>> @@ -177,7 +177,7 @@ static void vc4_bo_destroy(struct vc4_bo *bo)
>>   		bo->validated_shader = NULL;
>>   	}
>>   
>> -	drm_gem_cma_free_object(obj);
>> +	drm_gem_cma_free(&bo->base);
>>   }
>>   
>>   static void vc4_bo_remove_from_cache(struct vc4_bo *bo)
>> @@ -720,7 +720,7 @@ static int vc4_gem_object_mmap(struct drm_gem_object *obj, struct vm_area_struct
>>   		return -EINVAL;
>>   	}
>>   
>> -	return drm_gem_cma_mmap(obj, vma);
>> +	return drm_gem_cma_mmap(&bo->base, vma);
>>   }
>>   
>>   static const struct vm_operations_struct vc4_vm_ops = {
>> diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
>> index 56d2f9fdf9ac..adb507a9dbf0 100644
>> --- a/include/drm/drm_gem_cma_helper.h
>> +++ b/include/drm/drm_gem_cma_helper.h
>> @@ -32,28 +32,23 @@ struct drm_gem_cma_object {
>>   #define to_drm_gem_cma_obj(gem_obj) \
>>   	container_of(gem_obj, struct drm_gem_cma_object, base)
>>   
>> -/* free GEM object */
>> -void drm_gem_cma_free_object(struct drm_gem_object *gem_obj);
>> -
>> -/* allocate physical memory */
>>   struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
>>   					      size_t size);
>> +void drm_gem_cma_free(struct drm_gem_cma_object *cma_obj);
>> +void drm_gem_cma_print_info(const struct drm_gem_cma_object *cma_obj,
>> +			    struct drm_printer *p, unsigned int indent);
>> +struct sg_table *drm_gem_cma_get_sg_table(struct drm_gem_cma_object *cma_obj);
>> +int drm_gem_cma_vmap(struct drm_gem_cma_object *cma_obj, struct dma_buf_map *map);
>> +int drm_gem_cma_mmap(struct drm_gem_cma_object *cma_obj, struct vm_area_struct *vma);
>>   
>>   extern const struct vm_operations_struct drm_gem_cma_vm_ops;
>>   
>> -void drm_gem_cma_print_info(struct drm_printer *p, unsigned int indent,
>> -			    const struct drm_gem_object *obj);
>> -
>> -struct sg_table *drm_gem_cma_get_sg_table(struct drm_gem_object *obj);
>> -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);
>> -
>>   /*
>>    * GEM object functions
>>    */
>>   
>>   /**
>> - * drm_gem_cma_object_free - GEM object function for drm_gem_cma_free_object()
>> + * drm_gem_cma_object_free - GEM object function for drm_gem_cma_free()
>>    * @obj: GEM object to free
>>    *
>>    * This function wraps drm_gem_cma_free_object(). Drivers that employ the CMA helpers
>> @@ -61,7 +56,9 @@ int drm_gem_cma_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma);
>>    */
>>   static inline void drm_gem_cma_object_free(struct drm_gem_object *obj)
>>   {
>> -	drm_gem_cma_free_object(obj);
>> +	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
>> +
>> +	drm_gem_cma_free(cma_obj);
>>   }
>>   
>>   /**
>> @@ -76,7 +73,9 @@ static inline void drm_gem_cma_object_free(struct drm_gem_object *obj)
>>   static inline void drm_gem_cma_object_print_info(struct drm_printer *p, unsigned int indent,
>>   						 const struct drm_gem_object *obj)
>>   {
>> -	drm_gem_cma_print_info(p, indent, obj);
>> +	const struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
>> +
>> +	drm_gem_cma_print_info(cma_obj, p, indent);
>>   }
>>   
>>   /**
>> @@ -91,7 +90,9 @@ static inline void drm_gem_cma_object_print_info(struct drm_printer *p, unsigned
>>    */
>>   static inline struct sg_table *drm_gem_cma_object_get_sg_table(struct drm_gem_object *obj)
>>   {
>> -	return drm_gem_cma_get_sg_table(obj);
>> +	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
>> +
>> +	return drm_gem_cma_get_sg_table(cma_obj);
>>   }
>>   
>>   /*
>> @@ -107,7 +108,9 @@ static inline struct sg_table *drm_gem_cma_object_get_sg_table(struct drm_gem_ob
>>    */
>>   static inline int drm_gem_cma_object_vmap(struct drm_gem_object *obj, struct dma_buf_map *map)
>>   {
>> -	return drm_gem_cma_vmap(obj, map);
>> +	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
>> +
>> +	return drm_gem_cma_vmap(cma_obj, map);
>>   }
>>   
>>   /**
>> @@ -123,7 +126,9 @@ static inline int drm_gem_cma_object_vmap(struct drm_gem_object *obj, struct dma
>>    */
>>   static inline int drm_gem_cma_object_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
>>   {
>> -	return drm_gem_cma_mmap(obj, vma);
>> +	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
>> +
>> +	return drm_gem_cma_mmap(cma_obj, vma);
>>   }
>>   
>>   /*
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 3/3] drm/cma-helper: Pass GEM CMA object in public interfaces

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 714 bytes --]

On Mon, Nov 15, 2021 at 01:01:48PM +0100, Thomas Zimmermann wrote:
> Change all GEM CMA object functions that receive a GEM object
> of type struct drm_gem_object to expect an object of type
> struct drm_gem_cma_object instead.
> 
> This change reduces the number of upcasts from struct drm_gem_object
> by moving them into callers. The C compiler can now verify that the
> GEM CMA functions are called with the correct type.
> 
> For consistency, the patch also renames drm_gem_cma_free_object to
> drm_gem_cma_free. It further updates documentation for a number of
> functions.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Reviewed-by: Maxime Ripard <maxime@cerno.tech>

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 0/3] drm/cma-helper: Clean up public interface

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1610 bytes --]

ping. Are there further comments on this patchset?

Am 15.11.21 um 13:01 schrieb Thomas Zimmermann:
> Convert GEM CMA functions to accept struct drm_gem_cma_object, provide
> small wrappers for GEM object callbacks and update all users. Brings
> up the interface to the pattern used in SHMEM and VRAM helpers.
> 
> Converting all GEM object functions to use drm_gem_cma_object enables
> type checking by the C compiler. Previous callers could have passed any
> implementation of drm_gem_object to the GEM CMA helpers. It also
> removes upcasting in the GEM functions and simplifies the caller side.
> No functional changes.
> 
> For GEM object callbacks, the CMA helper library now provides a
> number of small wrappers that do the necessary upcasting. Again no
> functional changes.
> 
> Thomas Zimmermann (3):
>    drm/cma-helper: Move driver and file ops to the end of header
>    drm/cma-helper: Export dedicated wrappers for GEM object functions
>    drm/cma-helper: Pass GEM CMA object in public interfaces
> 
>   drivers/gpu/drm/drm_gem_cma_helper.c  |  73 +++++-----
>   drivers/gpu/drm/rcar-du/rcar_du_kms.c |  10 +-
>   drivers/gpu/drm/vc4/vc4_bo.c          |   8 +-
>   include/drm/drm_gem_cma_helper.h      | 189 +++++++++++++++++++-------
>   4 files changed, 180 insertions(+), 100 deletions(-)
> 
> 
> base-commit: 9fccd12cfac1c863fa46d4d17c2d8ac25a44b190
> --
> 2.33.1
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]