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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- 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 --]