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@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. 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:
pgpaBo3X52hJ7.pgp
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
