On Wed, Nov 05, 2014 at 02:25:15PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@xxxxxxxxxx>
>
> Most of the functions already have the beginnings of kerneldoc comments
> but are using the wrong opening marker. Use the correct opening marker
> and flesh out the comments so that they can be integrated with the DRM
> DocBook document.
>
> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
Bunch of comments inline below.
-Daniel
> ---
> Documentation/DocBook/drm.tmpl | 6 ++
> drivers/gpu/drm/drm_gem_cma_helper.c | 155 ++++++++++++++++++++++++++---------
> include/drm/drm_gem_cma_helper.h | 25 ++++--
> 3 files changed, 139 insertions(+), 47 deletions(-)
>
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 18025496a736..666a210af121 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -963,6 +963,12 @@ struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev,
> !Edrivers/gpu/drm/drm_mm.c
> !Iinclude/drm/drm_mm.h
> </sect2>
> + <sect2>
> + <title>CMA Helper Functions Reference</title>
> +!Pdrivers/gpu/drm/drm_gem_cma_helper.c cma helpers
> +!Edrivers/gpu/drm/drm_gem_cma_helper.c
> +!Iinclude/drm/drm_gem_cma_helper.h
> + </sect2>
> </sect1>
>
> <!-- Internals: mode setting -->
> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
> index 0316310e2cc4..55306be1f8f7 100644
> --- a/drivers/gpu/drm/drm_gem_cma_helper.c
> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> @@ -29,18 +29,30 @@
> #include <drm/drm_gem_cma_helper.h>
> #include <drm/drm_vma_manager.h>
>
> -/*
> +/**
> + * DOC: cma helpers
> + *
> + * The Contiguous Memory Allocator reserves a pool of memory at early boot
> + * that is used to service requests for large blocks of contiguous memory.
> + *
> + * 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.
> + */
> +
> +/**
> * __drm_gem_cma_create - Create a GEM CMA object without allocating memory
> - * @drm: The drm device
> - * @size: The GEM object size
> + * @drm: DRM device
> + * @size: size of the object to allocate
> *
> - * This function creates and initializes a GEM CMA object of the given size, but
> - * doesn't allocate any memory to back the object.
> + * This function creates and initializes a GEM CMA object of the given size,
> + * but doesn't allocate any memory to back the object.
> *
> - * Return a struct drm_gem_cma_object* on success or ERR_PTR values on failure.
> + * Return: A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded
Same bikeshed about "Returns:\n" as with the panel kerneldoc patch.
> + * negative error code on failure.
> */
> static struct drm_gem_cma_object *
> -__drm_gem_cma_create(struct drm_device *drm, unsigned int size)
> +__drm_gem_cma_create(struct drm_device *drm, size_t size)
> {
> struct drm_gem_cma_object *cma_obj;
> struct drm_gem_object *gem_obj;
> @@ -69,14 +81,16 @@ error:
> return ERR_PTR(ret);
> }
>
> -/*
> +/**
> * drm_gem_cma_create - allocate an object with the given size
> + * @drm: DRM device
> + * @size: size of the object to allocate
> *
> - * returns a struct drm_gem_cma_object* on success or ERR_PTR values
> - * on failure.
> + * Return: A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded
> + * negative error code on failure.
> */
> struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
> - unsigned int size)
> + size_t size)
> {
> struct drm_gem_cma_object *cma_obj;
> int ret;
> @@ -104,17 +118,21 @@ error:
> }
> EXPORT_SYMBOL_GPL(drm_gem_cma_create);
>
> -/*
> - * drm_gem_cma_create_with_handle - allocate an object with the given
> - * size and create a gem handle on it
> +/**
> + * drm_gem_cma_create_with_handle - allocate an object with the given size and
> + * return a GEM handle to it
> + * @file_priv: DRM file-private structure to register the handle for
> + * @drm: DRM device
> + * @size: size of the object to allocate
> + * @handle: return location for the GEM handle
> *
> - * returns a struct drm_gem_cma_object* on success or ERR_PTR values
> - * on failure.
> + * Return: A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded
> + * negative error code on failure.
> */
> -static struct drm_gem_cma_object *drm_gem_cma_create_with_handle(
> - struct drm_file *file_priv,
> - struct drm_device *drm, unsigned int size,
> - unsigned int *handle)
> +static struct drm_gem_cma_object *
> +drm_gem_cma_create_with_handle(struct drm_file *file_priv,
> + struct drm_device *drm, size_t size,
> + uint32_t *handle)
> {
> struct drm_gem_cma_object *cma_obj;
> struct drm_gem_object *gem_obj;
> @@ -145,9 +163,9 @@ err_handle_create:
> return ERR_PTR(ret);
> }
>
> -/*
> - * drm_gem_cma_free_object - (struct drm_driver)->gem_free_object callback
> - * function
> +/**
> + * drm_gem_cma_free_object - free resources associated with a CMA GEM object
> + * @gem_obj: GEM object to free
> */
> void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
> {
> @@ -170,15 +188,20 @@ void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
> }
> EXPORT_SYMBOL_GPL(drm_gem_cma_free_object);
>
> -/*
> - * drm_gem_cma_dumb_create - (struct drm_driver)->dumb_create callback
> - * function
> +/**
> + * drm_gem_cma_dumb_create - create a dumb buffer object
> + * @file_priv: DRM file-private structure to create the dumb buffer for
> + * @drm: DRM device
> + * @args: IOCTL data
> *
> - * This aligns the pitch and size arguments to the minimum required. wrap
> + * This aligns the pitch and size arguments to the minimum required. Wrap
> * this into your own function if you need bigger alignment.
I think we should augment this slightly with something like
"Drivers without special needs can directly use this as their
->dumb_create callback."
Similar for dumb_mmap_offset below.
> + *
> + * Return: 0 on success or a negative error code on failure.
> */
> int drm_gem_cma_dumb_create(struct drm_file *file_priv,
> - struct drm_device *dev, struct drm_mode_create_dumb *args)
> + struct drm_device *drm,
> + struct drm_mode_create_dumb *args)
> {
> struct drm_gem_cma_object *cma_obj;
> int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> @@ -189,18 +212,25 @@ int drm_gem_cma_dumb_create(struct drm_file *file_priv,
> if (args->size < args->pitch * args->height)
> args->size = args->pitch * args->height;
>
> - cma_obj = drm_gem_cma_create_with_handle(file_priv, dev,
> - args->size, &args->handle);
> + cma_obj = drm_gem_cma_create_with_handle(file_priv, drm, args->size,
> + &args->handle);
> return PTR_ERR_OR_ZERO(cma_obj);
> }
> EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_create);
>
> -/*
> - * drm_gem_cma_dumb_map_offset - (struct drm_driver)->dumb_map_offset callback
> - * function
> +/**
> + * drm_gem_cma_dumb_map_offset - return the fake mmap offset for a CMA GEM
> + * object
> + * @file_priv: DRM file-private structure containing the GEM object
> + * @drm: DRM device
> + * @handle: GEM object handle
> + * @offset: return location for the fake mmap offset
> + *
> + * Return: 0 on success or a negative error code on failure.
> */
> int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv,
> - struct drm_device *drm, uint32_t handle, uint64_t *offset)
> + struct drm_device *drm, u32 handle,
> + u64 *offset)
> {
> struct drm_gem_object *gem_obj;
>
> @@ -208,7 +238,7 @@ int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv,
>
> gem_obj = drm_gem_object_lookup(drm, file_priv, handle);
> if (!gem_obj) {
> - dev_err(drm->dev, "failed to lookup gem object\n");
> + dev_err(drm->dev, "failed to lookup GEM object\n");
> mutex_unlock(&drm->struct_mutex);
> return -EINVAL;
> }
> @@ -251,8 +281,12 @@ static int drm_gem_cma_mmap_obj(struct drm_gem_cma_object *cma_obj,
> return ret;
> }
>
> -/*
> - * drm_gem_cma_mmap - (struct file_operation)->mmap callback function
> +/**
> + * drm_gem_cma_mmap - memory-map a CMA GEM object
> + * @filp: file object
> + * @vma: VMA for the area to be mapped
Imo the real value of kerneldoc is the blabla here that describes what
this should be used for and when. E.g. here
"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-demand
faulting. Drivers which employ the cma helpers should use this function as
their ->mmap handler for the DRM device file's file_operation structure."
So requires grepping each function and hunting for the usual pattern (and
noticing tons of crazy stuff, usually).
Same for all the functions below.
> + *
> + * Return: 0 on success or a negative error code on failure.
> */
> int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
> {
> @@ -272,7 +306,13 @@ int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
> EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
>
> #ifdef CONFIG_DEBUG_FS
> -void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj, struct seq_file *m)
> +/**
> + * drm_gem_cma_describe - describe a CMA GEM object for debugfs
> + * @cma_obj: CMA GEM object
> + * @m: debugfs file handle
> + */
> +void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj,
> + struct seq_file *m)
> {
> struct drm_gem_object *obj = &cma_obj->base;
> struct drm_device *dev = obj->dev;
> @@ -291,7 +331,14 @@ void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj, struct seq_file *m
> EXPORT_SYMBOL_GPL(drm_gem_cma_describe);
> #endif
>
> -/* low-level interface prime helpers */
> +/**
> + * drm_gem_cma_prime_get_sg_table - provide a scatter/gather table of pinned
> + * pages for a CMA GEM object
> + * @obj: GEM object
> + *
> + * Return: A pointer to the scatter/gather table of pinned pages or NULL on
> + * failure.
> + */
> struct sg_table *drm_gem_cma_prime_get_sg_table(struct drm_gem_object *obj)
> {
> struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
> @@ -315,6 +362,16 @@ out:
> }
> EXPORT_SYMBOL_GPL(drm_gem_cma_prime_get_sg_table);
>
> +/**
> + * drm_gem_cma_prime_import_sg_table - produce a CMA GEM object from another
> + * driver's scatter/gather table of pinned pages
> + * @dev: device to import into
> + * @attach: DMA-BUF attachment
> + * @sgt: scatter/gather table of pinned pages
> + *
> + * Return: A pointer to a newly created GEM object or an ERR_PTR-encoded
> + * negative error code on failure.
> + */
> struct drm_gem_object *
> drm_gem_cma_prime_import_sg_table(struct drm_device *dev,
> struct dma_buf_attachment *attach,
> @@ -339,6 +396,13 @@ 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
> + *
> + * Return: 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)
> {
> @@ -357,6 +421,13 @@ int drm_gem_cma_prime_mmap(struct drm_gem_object *obj,
> }
> EXPORT_SYMBOL_GPL(drm_gem_cma_prime_mmap);
>
> +/**
> + * drm_gem_cma_prime_vmap - map a CMA GEM object into the kernel's virtual
> + * address space
> + * @obj: GEM object
> + *
> + * Return: The kernel virtual address of the CMA GEM object's backing store.
> + */
> void *drm_gem_cma_prime_vmap(struct drm_gem_object *obj)
> {
> struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
> @@ -365,6 +436,12 @@ void *drm_gem_cma_prime_vmap(struct drm_gem_object *obj)
> }
> EXPORT_SYMBOL_GPL(drm_gem_cma_prime_vmap);
>
> +/**
> + * drm_gem_cma_prime_vunmap - unmap a CMA GEM object from the kernel's virtual
> + * address space
> + * @obj: GEM object
> + * @vaddr: kernel virtual address where the CMA GEM object was mapped
> + */
> void drm_gem_cma_prime_vunmap(struct drm_gem_object *obj, void *vaddr)
> {
> /* Nothing to do */
> diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
> index 2ff35f3de9c5..873d4eb7f125 100644
> --- a/include/drm/drm_gem_cma_helper.h
> +++ b/include/drm/drm_gem_cma_helper.h
> @@ -4,6 +4,13 @@
> #include <drm/drmP.h>
> #include <drm/drm_gem.h>
>
> +/**
> + * struct drm_gem_cma_object - GEM object backed by CMA memory allocations
> + * @base: base GEM object
> + * @paddr: physical address of the backing memory
> + * @sgt: scatter/gather table for imported PRIME buffers
> + * @vaddr: kernel virtual address of the backing memory
> + */
> struct drm_gem_cma_object {
> struct drm_gem_object base;
> dma_addr_t paddr;
> @@ -19,23 +26,25 @@ to_drm_gem_cma_obj(struct drm_gem_object *gem_obj)
> return container_of(gem_obj, struct drm_gem_cma_object, base);
> }
>
> -/* free gem object. */
> +/* free GEM object */
> void drm_gem_cma_free_object(struct drm_gem_object *gem_obj);
>
> -/* create memory region for drm framebuffer. */
> +/* 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_device *drm,
> + struct drm_mode_create_dumb *args);
>
> -/* map memory region for drm framebuffer to user space. */
> +/* map memory region for DRM framebuffer to user space */
> int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv,
> - struct drm_device *drm, uint32_t handle, uint64_t *offset);
> + struct drm_device *drm, u32 handle,
> + u64 *offset);
>
> -/* set vm_flags and we can change the vm attribute to other one at here. */
> +/* 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. */
> +/* allocate physical memory */
> struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
> - unsigned int size);
> + size_t size);
>
> extern const struct vm_operations_struct drm_gem_cma_vm_ops;
>
> --
> 2.1.3
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
