From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH 3/7] drm/doc: Add GEM/CMA helpers to kerneldoc Date: Wed, 5 Nov 2014 16:01:26 +0100 Message-ID: <20141105150124.GC24353@ulmo> References: <1415193919-1687-1-git-send-email-thierry.reding@gmail.com> <1415193919-1687-4-git-send-email-thierry.reding@gmail.com> <20141105143440.GO26941@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0797370249==" Return-path: Received: from mail-wi0-f182.google.com (mail-wi0-f182.google.com [209.85.212.182]) by gabe.freedesktop.org (Postfix) with ESMTP id 3B92F6E7A4 for ; Wed, 5 Nov 2014 07:01:30 -0800 (PST) Received: by mail-wi0-f182.google.com with SMTP id d1so2248449wiv.15 for ; Wed, 05 Nov 2014 07:01:29 -0800 (PST) In-Reply-To: <20141105143440.GO26941@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter Cc: Benjamin Gaignard , Daniel Vetter , dri-devel@lists.freedesktop.org, Tomi Valkeinen , Archit Taneja , Laurent Pinchart , Russell King , Dave Airlie List-Id: dri-devel@lists.freedesktop.org --===============0797370249== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="0lnxQi9hkpPO77W3" Content-Disposition: inline --0lnxQi9hkpPO77W3 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 > >=20 > > 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. > >=20 > > Signed-off-by: Thierry Reding >=20 > Bunch of comments inline below. > -Daniel >=20 > > --- > > 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(-) > >=20 > > diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm= =2Etmpl > > 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 > > > > + > > + CMA Helper Functions Reference > > +!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 > > + > > > > =20 > > > > 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 > > #include > > =20 > > -/* > > +/** > > + * 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 mem= ory. > > + * > > + * The DRM GEM/CMA helpers use this allocator as a means to provide bu= ffer > > + * objects that are physically contiguous in memory. This is useful for > > + * display drivers that are unable to map scattered buffers via an IOM= MU. > > + */ > > + > > +/** > > * __drm_gem_cma_create - Create a GEM CMA object without allocating m= emory > > - * @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()-en= coded >=20 > 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. w= rap > > + * This aligns the pitch and size arguments to the minimum required. W= rap > > * this into your own function if you need bigger alignment. >=20 > I think we should augment this slightly with something like >=20 > "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 >=20 > Imo the real value of kerneldoc is the blabla here that describes what > this should be used for and when. E.g. here >=20 > "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). >=20 > Same for all the functions below. Okay, I'll see what I can come up with. Thierry --0lnxQi9hkpPO77W3 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJUWjvEAAoJEN0jrNd/PrOhFrYQAK4nkuESz7SVUN2tiZDx74UU IOm+B/xShojhj0LDhPevYiPEZdmufcx7LEmJP3WX21taW4ghSwR2MJePsfMd0Q1s EZYhpCoA8/jDbcQD0uw07Nd7HBlQTgu6QRHdUDi/SSj98p9MAKPwgIiMvLsEuaBb 0GK2kv8INsmIZyGNc1rCRHHyH/11Qpj6NB50hO44MDdolwhU1ugl8PsLMWqgEm6Q hIzPn1pz08V+7ZG39SZLj9dOh7aHiXm1g7Koy6LyRV40Z67aQ48wG5WAAd1Xx5VO uBmQG98nmuqQ+FVc448V3nAvNM6ccv6ltQV4vwlyHV62BfM4OH0+2DLJrioxmwzQ oK37ms5CpMc6jPhtu+ufD7MIC3JDtUsVVPkvOJg9MdmeZ8gey6R4gtzokz7g6nik WBXuMgax3rUXFcZw/cYIyYjLquPgkywFsUKfZCn+wuZDK3zRTIDKYhUkELW7LP5/ luoUraSHNutRJEUBIwkFDD2KxOQDQrrNVRvJ02YkEtV0Uui+/U+ZXe3dYizFB+tx 8ft5OeT6rX7G3LGVCylFV9rdL9gwOf1wuZB7wTxrwKIgrqfgJA51iaBRXTJRt4Q4 7ebiUWN3iR0hTfN1lRrsswzcJmgZPAnN5kAaieNPbGpHXjUuT3sYDPyf2arv9HAU SiHiUUSN0b6x2FL7Wj+b =aNq8 -----END PGP SIGNATURE----- --0lnxQi9hkpPO77W3-- --===============0797370249== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK --===============0797370249==--