[PATCH v2 1/2] drm/gem-fb-helper: Cleanup docs

[PATCH v2 1/2] drm/gem-fb-helper: Cleanup docs

[Noralf Trønnes]

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
  *
+ * 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.
  *
--
2.7.4

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

[PATCH v2 2/2] drm/gem-fb-helper: Use debug message on gem lookup failure

[Noralf Trønnes]

GEM lookup failure can easily be triggered by userspace so make
it a debug message, not an error message.

Also remove unnecessary inner parentheses and fix alphabetical
struct declaration order.

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_gem_framebuffer_helper.c | 4 ++--
 include/drm/drm_gem_framebuffer_helper.h     | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
index e2ca002..9cf6566 100644
--- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
+++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
@@ -157,7 +157,7 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
 
 		objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]);
 		if (!objs[i]) {
-			DRM_DEV_ERROR(dev->dev, "Failed to lookup GEM\n");
+			DRM_DEBUG_KMS("Failed to lookup GEM object\n");
 			ret = -ENOENT;
 			goto err_gem_object_put;
 		}
@@ -235,7 +235,7 @@ int drm_gem_fb_prepare_fb(struct drm_plane *plane,
 	struct dma_buf *dma_buf;
 	struct dma_fence *fence;
 
-	if ((plane->state->fb == state->fb) || !state->fb)
+	if (plane->state->fb == state->fb || !state->fb)
 		return 0;
 
 	dma_buf = drm_gem_fb_get_obj(state->fb, 0)->dma_buf;
diff --git a/include/drm/drm_gem_framebuffer_helper.h b/include/drm/drm_gem_framebuffer_helper.h
index db9cfa0..5ca7cdc 100644
--- a/include/drm/drm_gem_framebuffer_helper.h
+++ b/include/drm/drm_gem_framebuffer_helper.h
@@ -2,8 +2,8 @@
 #define __DRM_GEM_FB_HELPER_H__
 
 struct drm_device;
-struct drm_file;
 struct drm_fb_helper_surface_size;
+struct drm_file;
 struct drm_framebuffer;
 struct drm_framebuffer_funcs;
 struct drm_gem_object;
-- 
2.7.4

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

Re: [PATCH v2 1/2] drm/gem-fb-helper: Cleanup docs

[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 :-)

>   * @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


-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v2 2/2] drm/gem-fb-helper: Use debug message on gem lookup failure

[Laurent Pinchart]

Hi Noralf,

Thank you for the patch.

On Monday, 11 September 2017 19:37:45 EEST Noralf Trønnes wrote:
> GEM lookup failure can easily be triggered by userspace so make
> it a debug message, not an error message.
> 
> Also remove unnecessary inner parentheses and fix alphabetical
> struct declaration order.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/drm_gem_framebuffer_helper.c | 4 ++--
>  include/drm/drm_gem_framebuffer_helper.h     | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index e2ca002..9cf6566
> 100644
> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> @@ -157,7 +157,7 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev,
> struct drm_file *file,
> 
>  		objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]);
>  		if (!objs[i]) {
> -			DRM_DEV_ERROR(dev->dev, "Failed to lookup GEM\n");
> +			DRM_DEBUG_KMS("Failed to lookup GEM object\n");
>  			ret = -ENOENT;
>  			goto err_gem_object_put;
>  		}
> @@ -235,7 +235,7 @@ int drm_gem_fb_prepare_fb(struct drm_plane *plane,
>  	struct dma_buf *dma_buf;
>  	struct dma_fence *fence;
> 
> -	if ((plane->state->fb == state->fb) || !state->fb)
> +	if (plane->state->fb == state->fb || !state->fb)
>  		return 0;
> 
>  	dma_buf = drm_gem_fb_get_obj(state->fb, 0)->dma_buf;
> diff --git a/include/drm/drm_gem_framebuffer_helper.h
> b/include/drm/drm_gem_framebuffer_helper.h index db9cfa0..5ca7cdc 100644
> --- a/include/drm/drm_gem_framebuffer_helper.h
> +++ b/include/drm/drm_gem_framebuffer_helper.h
> @@ -2,8 +2,8 @@
>  #define __DRM_GEM_FB_HELPER_H__
> 
>  struct drm_device;
> -struct drm_file;
>  struct drm_fb_helper_surface_size;
> +struct drm_file;
>  struct drm_framebuffer;
>  struct drm_framebuffer_funcs;
>  struct drm_gem_object;


-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v2 1/2] drm/gem-fb-helper: Cleanup docs

[Noralf Trønnes]

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
>

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

Re: [PATCH v2 1/2] drm/gem-fb-helper: Cleanup docs

[Laurent Pinchart]

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 :-)

> 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.

> 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

Re: [PATCH v2 2/2] drm/gem-fb-helper: Use debug message on gem lookup failure

[Noralf Trønnes]

Den 13.09.2017 04.44, skrev Laurent Pinchart:
> Hi Noralf,
>
> Thank you for the patch.
>
> On Monday, 11 September 2017 19:37:45 EEST Noralf Trønnes wrote:
>> GEM lookup failure can easily be triggered by userspace so make
>> it a debug message, not an error message.
>>
>> Also remove unnecessary inner parentheses and fix alphabetical
>> struct declaration order.
>>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks, applied to drm-misc.

Noralf.

>> ---
>>   drivers/gpu/drm/drm_gem_framebuffer_helper.c | 4 ++--
>>   include/drm/drm_gem_framebuffer_helper.h     | 2 +-
>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index e2ca002..9cf6566
>> 100644
>> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> @@ -157,7 +157,7 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev,
>> struct drm_file *file,
>>
>>   		objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]);
>>   		if (!objs[i]) {
>> -			DRM_DEV_ERROR(dev->dev, "Failed to lookup GEM\n");
>> +			DRM_DEBUG_KMS("Failed to lookup GEM object\n");
>>   			ret = -ENOENT;
>>   			goto err_gem_object_put;
>>   		}
>> @@ -235,7 +235,7 @@ int drm_gem_fb_prepare_fb(struct drm_plane *plane,
>>   	struct dma_buf *dma_buf;
>>   	struct dma_fence *fence;
>>
>> -	if ((plane->state->fb == state->fb) || !state->fb)
>> +	if (plane->state->fb == state->fb || !state->fb)
>>   		return 0;
>>
>>   	dma_buf = drm_gem_fb_get_obj(state->fb, 0)->dma_buf;
>> diff --git a/include/drm/drm_gem_framebuffer_helper.h
>> b/include/drm/drm_gem_framebuffer_helper.h index db9cfa0..5ca7cdc 100644
>> --- a/include/drm/drm_gem_framebuffer_helper.h
>> +++ b/include/drm/drm_gem_framebuffer_helper.h
>> @@ -2,8 +2,8 @@
>>   #define __DRM_GEM_FB_HELPER_H__
>>
>>   struct drm_device;
>> -struct drm_file;
>>   struct drm_fb_helper_surface_size;
>> +struct drm_file;
>>   struct drm_framebuffer;
>>   struct drm_framebuffer_funcs;
>>   struct drm_gem_object;
>

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

Re: [PATCH v2 1/2] drm/gem-fb-helper: Cleanup docs

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel