| Message ID | 1505147865-18194-1-git-send-email-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Noralf, Thank you for the patch. On Monday, 11 September 2017 19:37:44 EEST Noralf Trønnes wrote: > Make the docs read a little better. > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > > Changes: > Addressed Daniel's comments. > > drivers/gpu/drm/drm_gem_framebuffer_helper.c | 25 ++++++++++++++----------- > 1 file changed, 14 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c > b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index d54a083..e2ca002 > 100644 > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c > @@ -27,18 +27,21 @@ > * DOC: overview > * > * This library provides helpers for drivers that don't subclass > - * &drm_framebuffer and and use &drm_gem_object for their backing storage. > + * &drm_framebuffer and use &drm_gem_object for their backing storage. > * > * Drivers without additional needs to validate framebuffers can simply use > - * drm_gem_fb_create() and everything is wired up automatically. But all - > * parts can be used individually. > + * drm_gem_fb_create() and everything is wired up automatically. Other > drivers + * can use all parts independently. > */ > > /** > * drm_gem_fb_get_obj() - Get GEM object for framebuffer > - * @fb: The framebuffer > + * @fb: framebuffer > * @plane: Which plane Nitpicking, you should capitalize all entries or none. > * > + * No additional reference is taken beyond the one that the &drm_frambuffer > + * already holds. > + * > * Returns the GEM object for given framebuffer. > */ > struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb, > @@ -82,7 +85,7 @@ drm_gem_fb_alloc(struct drm_device *dev, > > /** > * drm_gem_fb_destroy - Free GEM backed framebuffer > - * @fb: DRM framebuffer > + * @fb: framebuffer > * > * Frees a GEM backed framebuffer with its backing buffer(s) and the > structure > * itself. Drivers can use this as their &drm_framebuffer_funcs->destroy > @@ -102,8 +105,8 @@ > EXPORT_SYMBOL(drm_gem_fb_destroy); > > /** > * drm_gem_fb_create_handle - Create handle for GEM backed framebuffer > - * @fb: DRM framebuffer > - * @file: drm file > + * @fb: framebuffer > + * @file: DRM file I wonder why framebuffer doesn't need a DRM while file does :-) > * @handle: handle created > * > * Drivers can use this as their &drm_framebuffer_funcs->create_handle > @@ -124,7 +127,7 @@ EXPORT_SYMBOL(drm_gem_fb_create_handle); > * &drm_mode_config_funcs.fb_create > * callback > * @dev: DRM device > - * @file: drm file for the ioctl call > + * @file: DRM file for the ioctl call > * @mode_cmd: metadata from the userspace fb creation request > * @funcs: vtable to be used for the new framebuffer object > * > @@ -194,7 +197,7 @@ static const struct drm_framebuffer_funcs > drm_gem_fb_funcs = { /** > * drm_gem_fb_create() - &drm_mode_config_funcs.fb_create callback function > * @dev: DRM device > - * @file: drm file for the ioctl call > + * @file: DRM file for the ioctl call > * @mode_cmd: metadata from the userspace fb creation request > * > * If your hardware has special alignment or pitch requirements these > should be @@ -214,11 +217,11 @@ EXPORT_SYMBOL_GPL(drm_gem_fb_create); > /** > * drm_gem_fb_prepare_fb() - Prepare gem framebuffer > * @plane: Which plane > - * @state: Plane state attach fence to > + * @state: Plane state the fence will be attached to > * > * This can be used as the &drm_plane_helper_funcs.prepare_fb hook. > * > - * This function checks if the plane FB has an dma-buf attached, extracts > + * This function checks if the plane FB has a dma-buf attached, extracts > * the exclusive fence and attaches it to plane state for the atomic helper > * to wait on. > * > -- > 2.7.4
Hi Laurent, Den 13.09.2017 04.44, skrev Laurent Pinchart: > Hi Noralf, > > Thank you for the patch. > > On Monday, 11 September 2017 19:37:44 EEST Noralf Trønnes wrote: >> Make the docs read a little better. >> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> --- >> >> Changes: >> Addressed Daniel's comments. >> >> drivers/gpu/drm/drm_gem_framebuffer_helper.c | 25 ++++++++++++++----------- >> 1 file changed, 14 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c >> b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index d54a083..e2ca002 >> 100644 >> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c >> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c >> @@ -27,18 +27,21 @@ >> * DOC: overview >> * >> * This library provides helpers for drivers that don't subclass >> - * &drm_framebuffer and and use &drm_gem_object for their backing storage. >> + * &drm_framebuffer and use &drm_gem_object for their backing storage. >> * >> * Drivers without additional needs to validate framebuffers can simply use >> - * drm_gem_fb_create() and everything is wired up automatically. But all - >> * parts can be used individually. >> + * drm_gem_fb_create() and everything is wired up automatically. Other >> drivers + * can use all parts independently. >> */ >> >> /** >> * drm_gem_fb_get_obj() - Get GEM object for framebuffer >> - * @fb: The framebuffer >> + * @fb: framebuffer >> * @plane: Which plane > Nitpicking, you should capitalize all entries or none. > >> * >> + * No additional reference is taken beyond the one that the &drm_frambuffer >> + * already holds. >> + * >> * Returns the GEM object for given framebuffer. >> */ >> struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb, >> @@ -82,7 +85,7 @@ drm_gem_fb_alloc(struct drm_device *dev, >> >> /** >> * drm_gem_fb_destroy - Free GEM backed framebuffer >> - * @fb: DRM framebuffer >> + * @fb: framebuffer >> * >> * Frees a GEM backed framebuffer with its backing buffer(s) and the >> structure >> * itself. Drivers can use this as their &drm_framebuffer_funcs->destroy >> @@ -102,8 +105,8 @@ >> EXPORT_SYMBOL(drm_gem_fb_destroy); >> >> /** >> * drm_gem_fb_create_handle - Create handle for GEM backed framebuffer >> - * @fb: DRM framebuffer >> - * @file: drm file >> + * @fb: framebuffer >> + * @file: DRM file > I wonder why framebuffer doesn't need a DRM while file does :-) Yes this is haphazard. I think my problem is that I haven't been able to pick up a consistent "signal" from the DRM subsystem when it comes to how documentation should be written. Code patterns are fairly consistent and looks much the same including variable names, but documentation is more or less all over the place. So I guess I need to come to grips with this, since this isn't the last time I have to write documentation. I have to make myself some rules that I can look at next time when all of this is forgotten. Should description entries be Capitalized? My gut feeling is that most DRM docs don't do that, but when humans write for humans we do capatalize the start of sentences. So I guess that's the natural thing to do. Should DRM objects start with DRM in the argument doc entries? 'DRM device' is a given since the kernel has many types of devices, but should we write 'DRM framebuffer' or 'Framebuffer'? How descriptive should the descriptions be? Let's take this example: /** * drm_gem_fb_prepare_fb() - Prepare gem framebuffer * @plane: Which plane * @state: Plane state the fence will be attached to * * This can be used as the &drm_plane_helper_funcs.prepare_fb hook. * * This function checks if the plane FB has an dma-buf attached, extracts * the exclusive fence and attaches it to plane state for the atomic helper * to wait on. Both the @state description and the body says that the fence will be attached to the plane state. Would it be better to just say: * @state: Plane state Another way to ask this is: Should the documentation be terse or should it be repeating itself? Then we have (copied from the cma library): * @plane: Which plane Which is probably short for: The plane which we are operating/acting on. More possibilities: * @plane: DRM plane * @plane: Plane * @plane: The plane for which a framebuffer is prepared for scanout The text for the last one is also available when clicking on the &drm_plane_helper_funcs.prepare_fb link, so it's repeating something that is one click away. I always get comments on my documentation, so it's clearly something I need to find a way to improve. Noralf. >> * @handle: handle created >> * >> * Drivers can use this as their &drm_framebuffer_funcs->create_handle >> @@ -124,7 +127,7 @@ EXPORT_SYMBOL(drm_gem_fb_create_handle); >> * &drm_mode_config_funcs.fb_create >> * callback >> * @dev: DRM device >> - * @file: drm file for the ioctl call >> + * @file: DRM file for the ioctl call >> * @mode_cmd: metadata from the userspace fb creation request >> * @funcs: vtable to be used for the new framebuffer object >> * >> @@ -194,7 +197,7 @@ static const struct drm_framebuffer_funcs >> drm_gem_fb_funcs = { /** >> * drm_gem_fb_create() - &drm_mode_config_funcs.fb_create callback function >> * @dev: DRM device >> - * @file: drm file for the ioctl call >> + * @file: DRM file for the ioctl call >> * @mode_cmd: metadata from the userspace fb creation request >> * >> * If your hardware has special alignment or pitch requirements these >> should be @@ -214,11 +217,11 @@ EXPORT_SYMBOL_GPL(drm_gem_fb_create); >> /** >> * drm_gem_fb_prepare_fb() - Prepare gem framebuffer >> * @plane: Which plane >> - * @state: Plane state attach fence to >> + * @state: Plane state the fence will be attached to >> * >> * This can be used as the &drm_plane_helper_funcs.prepare_fb hook. >> * >> - * This function checks if the plane FB has an dma-buf attached, extracts >> + * This function checks if the plane FB has a dma-buf attached, extracts >> * the exclusive fence and attaches it to plane state for the atomic helper >> * to wait on. >> * >> -- >> 2.7.4 >
On Fri, Sep 15, 2017 at 12:45:14AM +0300, Laurent Pinchart wrote: > Hi Noralf, > > On Wednesday, 13 September 2017 16:41:49 EEST Noralf Trønnes wrote: > > Den 13.09.2017 04.44, skrev Laurent Pinchart: > > > On Monday, 11 September 2017 19:37:44 EEST Noralf Trønnes wrote: > > >> Make the docs read a little better. > > >> > > >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > > >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > >> --- > > >> > > >> Changes: > > >> Addressed Daniel's comments. > > >> > > >> drivers/gpu/drm/drm_gem_framebuffer_helper.c | 25 +++++++++++++---------- > > >> > > >> 1 file changed, 14 insertions(+), 11 deletions(-) > > >> > > >> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c > > >> b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index d54a083..e2ca002 > > >> 100644 > > >> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c > > >> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c > > [snip] > > > >> /** > > >> * drm_gem_fb_create_handle - Create handle for GEM backed framebuffer > > >> - * @fb: DRM framebuffer > > >> - * @file: drm file > > >> + * @fb: framebuffer > > >> + * @file: DRM file > > > > > > I wonder why framebuffer doesn't need a DRM while file does :-) > > > > Yes this is haphazard. > > I think my problem is that I haven't been able to pick up a consistent > > "signal" from the DRM subsystem when it comes to how documentation > > should be written. Code patterns are fairly consistent and looks much > > the same including variable names, but documentation is more or less > > all over the place. > > That doesn't surprise me too much, as documentation in DRM is pretty recent, > and we never agreed to a documentation style. > > > So I guess I need to come to grips with this, since this isn't the last > > time I have to write documentation. I have to make myself some rules > > that I can look at next time when all of this is forgotten. > > Or, as the first person to address this problem, you could set your own rules > that everybody else should then follow :-) Better is to actually switch all cases over. Which yes is a lot more work, but otherwise it won't converge. Since at least me personally I tend to just copy the patterns from the same file ... > > Should description entries be Capitalized? > > My gut feeling is that most DRM docs don't do that, but when humans > > write for humans we do capatalize the start of sentences. So I guess > > that's the natural thing to do. > > > > Should DRM objects start with DRM in the argument doc entries? > > 'DRM device' is a given since the kernel has many types of devices, but > > should we write 'DRM framebuffer' or 'Framebuffer'? > > I would only mention DRM when the description could otherwise be > misinterpreted, as in DRM device. > > > How descriptive should the descriptions be? > > Let's take this example: > > > > /** > > * drm_gem_fb_prepare_fb() - Prepare gem framebuffer > > * @plane: Which plane > > * @state: Plane state the fence will be attached to > > * > > * This can be used as the &drm_plane_helper_funcs.prepare_fb hook. > > * > > * This function checks if the plane FB has an dma-buf attached, extracts > > * the exclusive fence and attaches it to plane state for the atomic helper > > * to wait on. > > > > Both the @state description and the body says that the fence will be > > attached to the plane state. Would it be better to just say: > > > > * @state: Plane state > > > > Another way to ask this is: > > Should the documentation be terse or should it be repeating itself? > > > > Then we have (copied from the cma library): > > > > * @plane: Which plane > > > > Which is probably short for: The plane which we are operating/acting on. > > > > More possibilities: > > > > * @plane: DRM plane > > * @plane: Plane > > * @plane: The plane for which a framebuffer is prepared for scanout > > > > The text for the last one is also available when clicking on the > > &drm_plane_helper_funcs.prepare_fb link, so it's repeating something > > that is one click away. > > Writing kerneldoc just for the sake of it is mostly pointless. We should write > documentation to make it as useful and easy to consume as possible. Keeping > that in mind, > > * @plane: Plane > > isn't very useful. It's clear from the function argument name (and type) that > it is a pointer to a plane. I would thus advocate for more detailed parameter > descriptions. There's unfortunately a certain amount of boilerplate in some of the argument docs. It happens, and as long as the main text explains it all, I think it's ok to have terse doc like this. If we put nothing, kernel-doc will complain (and those warnings are kinda nice to spot outdated docs). Thanks, Daniel > > I always get comments on my documentation, so it's clearly something I > > need to find a way to improve. > > I don't think it's specific to your documentation. You're doing a great job, > for which I'm personally grateful. The fact that we haven't defined a > documentation style in a similar way as we have defined a coding style is an > issue of the DRM/KMS community. > > > >> * @handle: handle created > > >> * > > >> * Drivers can use this as their &drm_framebuffer_funcs->create_handle > > [snip] > > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index d54a083..e2ca002 100644 --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -27,18 +27,21 @@ * DOC: overview * * This library provides helpers for drivers that don't subclass - * &drm_framebuffer and and use &drm_gem_object for their backing storage. + * &drm_framebuffer and use &drm_gem_object for their backing storage. * * Drivers without additional needs to validate framebuffers can simply use - * drm_gem_fb_create() and everything is wired up automatically. But all - * parts can be used individually. + * drm_gem_fb_create() and everything is wired up automatically. Other drivers + * can use all parts independently. */ /** * drm_gem_fb_get_obj() - Get GEM object for framebuffer - * @fb: The framebuffer + * @fb: framebuffer * @plane: Which plane * + * No additional reference is taken beyond the one that the &drm_frambuffer + * already holds. + * * Returns the GEM object for given framebuffer. */ struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb, @@ -82,7 +85,7 @@ drm_gem_fb_alloc(struct drm_device *dev, /** * drm_gem_fb_destroy - Free GEM backed framebuffer - * @fb: DRM framebuffer + * @fb: framebuffer * * Frees a GEM backed framebuffer with its backing buffer(s) and the structure * itself. Drivers can use this as their &drm_framebuffer_funcs->destroy @@ -102,8 +105,8 @@ EXPORT_SYMBOL(drm_gem_fb_destroy); /** * drm_gem_fb_create_handle - Create handle for GEM backed framebuffer - * @fb: DRM framebuffer - * @file: drm file + * @fb: framebuffer + * @file: DRM file * @handle: handle created * * Drivers can use this as their &drm_framebuffer_funcs->create_handle @@ -124,7 +127,7 @@ EXPORT_SYMBOL(drm_gem_fb_create_handle); * &drm_mode_config_funcs.fb_create * callback * @dev: DRM device - * @file: drm file for the ioctl call + * @file: DRM file for the ioctl call * @mode_cmd: metadata from the userspace fb creation request * @funcs: vtable to be used for the new framebuffer object * @@ -194,7 +197,7 @@ static const struct drm_framebuffer_funcs drm_gem_fb_funcs = { /** * drm_gem_fb_create() - &drm_mode_config_funcs.fb_create callback function * @dev: DRM device - * @file: drm file for the ioctl call + * @file: DRM file for the ioctl call * @mode_cmd: metadata from the userspace fb creation request * * If your hardware has special alignment or pitch requirements these should be @@ -214,11 +217,11 @@ EXPORT_SYMBOL_GPL(drm_gem_fb_create); /** * drm_gem_fb_prepare_fb() - Prepare gem framebuffer * @plane: Which plane - * @state: Plane state attach fence to + * @state: Plane state the fence will be attached to * * This can be used as the &drm_plane_helper_funcs.prepare_fb hook. * - * This function checks if the plane FB has an dma-buf attached, extracts + * This function checks if the plane FB has a dma-buf attached, extracts * the exclusive fence and attaches it to plane state for the atomic helper * to wait on. *