Re: [PATCH 3/7] drm/doc: Add GEM/CMA helpers to kerneldoc

[Thierry Reding <thierry.reding@gmail.com>]

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

On Wed, Nov 05, 2014 at 03:34:40PM +0100, Daniel Vetter wrote:
> On Wed, Nov 05, 2014 at 02:25:15PM +0100, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > 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@nvidia.com>
> 
> 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.

I've been following the style described in the kernel-doc nano-HOWTO,
which says that:

	The return value, if any, should be described in a dedicated
	section named "Return".

There are other things in that document that we don't follow in DRM, so
I wonder if we should just consider it as guidelines rather than actual
rules (they aren't enforced anyway) or perhaps make a pass over existing
kerneldoc and convert it to the rules described in that document.

That document is the only reference for the kerneldoc syntax (that I
know of), so I had always thought that we should be following it.

> > -/*
> > - * 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."

Good idea.

> Similar for dumb_mmap_offset below.

I don't see any way how drm_gem_cma_dumb_map_offset() could be
parameterized. It certainly looks generic enough not to need any
device-specific quirks.

> > -/*
> > - * 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."

I've taken the easy route and simply copied your example.

> So requires grepping each function and hunting for the usual pattern (and
> noticing tons of crazy stuff, usually).
> 
> Same for all the functions below.

Okay, I'll see what I can come up with.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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