[PATCH] drm/fb-helper: improve kerneldoc

[PATCH] drm/fb-helper: improve kerneldoc

[Daniel Vetter]

Now that the fbdev helper interface for drivers is trimmed down,
update the kerneldoc for all the remaining exported functions.

I've tried to beat the DocBook a bit by reordering the function
references a bit into a more sensible ordering. But that didn't work
out at all. Hence just extend the in-code DOC: section a bit.

Also remove the LOCKING: sections - especially for the setup functions
they're totally bogus. But that's not a documentation problem, but
simply an artifact of the current rather hazardous locking around drm
init and even more so around fbdev setup ...

v2: Some further improvements:
- Also add documentation for drm_fb_helper_single_add_all_connectors,
  Dave Airlie didn't want me to kill this one from the fb helper
  interface.
- Update docs for drm_fb_helper_fill_var/fix - they should be used
  from the driver's ->fb_probe callback to setup the fbdev info
  structure.
- Clarify what the ->fb_probe callback should all do - it needs to
  setup both the fbdev info and allocate the drm framebuffer used as
  backing storage.
- Add basic documentaation for the drm_fb_helper_funcs driver callback
  vfunc.

v3: Implement clarifications Lauren Pinchart suggested in his review.

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/DocBook/drm.tmpl  |    1 +
 drivers/gpu/drm/drm_fb_helper.c |  148 +++++++++++++++++++++++++++++++++++----
 include/drm/drm_fb_helper.h     |   12 ++++
 3 files changed, 146 insertions(+), 15 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 6c11d77..14ad829 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -2139,6 +2139,7 @@ void intel_crt_init(struct drm_device *dev)
       <title>fbdev Helper Functions Reference</title>
 !Pdrivers/gpu/drm/drm_fb_helper.c fbdev helpers
 !Edrivers/gpu/drm/drm_fb_helper.c
+!Iinclude/drm/drm_fb_helper.h
     </sect2>
     <sect2>
       <title>Display Port Helper Functions Reference</title>
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 5a92470..b4cbef3 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -52,9 +52,36 @@ static LIST_HEAD(kernel_fb_helper_list);
  * mode setting driver. They can be used mostly independantely from the crtc
  * helper functions used by many drivers to implement the kernel mode setting
  * interfaces.
+ *
+ * Initialization is done as a three-step process with drm_fb_helper_init(),
+ * drm_fb_helper_single_add_all_connectors() and drm_fb_helper_initial_config().
+ * Drivers with fancier requirements than the default beheviour can override the
+ * second step with their own code.  Teardown is done with drm_fb_helper_fini().
+ *
+ * At runtime drivers should restore the fbdev console by calling
+ * drm_fb_helper_restore_fbdev_mode() from their ->lastclose callback. They
+ * should also notify the fb helper code from updates to the output
+ * configuration by calling drm_fb_helper_hotplug_event(). For easier
+ * integration with the output polling code in drm_crtc_helper.c the modeset
+ * code proves a ->output_poll_changed callback.
+ *
+ * All other functions exported by the fb helper library can be used to
+ * implement the fbdev driver interface by the driver.
  */
 
-/* simple single crtc case helper function */
+/**
+ * drm_fb_helper_single_add_all_connectors() - add all connectors to fbdev
+ * 					       emulation helper
+ * @fb_helper: fbdev initialized with drm_fb_helper_init
+ *
+ * This functions adds all the available connectors for use with the given
+ * fb_helper. This is a separate step to allow drivers to freely assign
+ * connectors to the fbdev, e.g. if some are reserved for special purposes or
+ * not adequate to be used for the fbcon.
+ *
+ * Since this is part of the initial setup before the fbdev is published, no
+ * locking is required.
+ */
 int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
@@ -163,6 +190,10 @@ static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc)
 	crtc->funcs->gamma_set(crtc, r_base, g_base, b_base, 0, crtc->gamma_size);
 }
 
+/**
+ * drm_fb_helper_debug_enter - implementation for ->fb_debug_enter
+ * @info: fbdev registered by the helper
+ */
 int drm_fb_helper_debug_enter(struct fb_info *info)
 {
 	struct drm_fb_helper *helper = info->par;
@@ -208,6 +239,10 @@ static struct drm_framebuffer *drm_mode_config_fb(struct drm_crtc *crtc)
 	return NULL;
 }
 
+/**
+ * drm_fb_helper_debug_leave - implementation for ->fb_debug_leave
+ * @info: fbdev registered by the helper
+ */
 int drm_fb_helper_debug_leave(struct fb_info *info)
 {
 	struct drm_fb_helper *helper = info->par;
@@ -243,9 +278,9 @@ EXPORT_SYMBOL(drm_fb_helper_debug_leave);
  * drm_fb_helper_restore_fbdev_mode - restore fbdev configuration
  * @fb_helper: fbcon to restore
  *
- * This should be called from driver's drm->lastclose callback when implementing
- * an fbcon on top of kms using this helper. This ensures that the user isn't
- * greeted with a black screen when e.g. X dies.
+ * This should be called from driver's drm ->lastclose callback
+ * when implementing an fbcon on top of kms using this helper. This ensures that
+ * the user isn't greeted with a black screen when e.g. X dies.
  */
 bool drm_fb_helper_restore_fbdev_mode(struct drm_fb_helper *fb_helper)
 {
@@ -378,6 +413,11 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
 	drm_modeset_unlock_all(dev);
 }
 
+/**
+ * drm_fb_helper_blank - implementation for ->fb_blank
+ * @blank: desired blanking state
+ * @info: fbdev registered by the helper
+ */
 int drm_fb_helper_blank(int blank, struct fb_info *info)
 {
 	switch (blank) {
@@ -421,6 +461,24 @@ static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
 	kfree(helper->crtc_info);
 }
 
+/**
+ * drm_fb_helper_init - initialize a drm_fb_helper structure
+ * @dev: drm device
+ * @fb_helper: driver-allocated fbdev helper structure to initialize
+ * @crtc_count: maximum number of crtcs to support in this fbdev emulation
+ * @max_conn_count: max connector count
+ *
+ * This allocates the structures for the fbdev helper with the given limits.
+ * Note that this won't yet touch the hardware (through the driver interfaces)
+ * nor register the fbdev. This is only done in drm_fb_helper_initial_config()
+ * to allow driver writes more control over the exact init sequence.
+ *
+ * Drivers must set fb_helper->funcs before calling
+ * drm_fb_helper_initial_config().
+ *
+ * RETURNS:
+ * Zero if everything went ok, nonzero otherwise.
+ */
 int drm_fb_helper_init(struct drm_device *dev,
 		       struct drm_fb_helper *fb_helper,
 		       int crtc_count, int max_conn_count)
@@ -549,6 +607,11 @@ static int setcolreg(struct drm_crtc *crtc, u16 red, u16 green,
 	return 0;
 }
 
+/**
+ * drm_fb_helper_setcmap - implementation for ->fb_setcmap
+ * @cmap: cmap to set
+ * @info: fbdev registered by the helper
+ */
 int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
 {
 	struct drm_fb_helper *fb_helper = info->par;
@@ -588,6 +651,11 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
 }
 EXPORT_SYMBOL(drm_fb_helper_setcmap);
 
+/**
+ * drm_fb_helper_check_var - implementation for ->fb_check_var
+ * @var: screeninfo to check
+ * @info: fbdev registered by the helper
+ */
 int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 			    struct fb_info *info)
 {
@@ -680,7 +748,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 }
 EXPORT_SYMBOL(drm_fb_helper_check_var);
 
-/* this will let fbcon do the mode init */
+/**
+ * drm_fb_helper_set_par - implementation for ->fb_set_par
+ * @info: fbdev registered by the helper
+ *
+ * This will let fbcon do the mode init and is called at initialization time by
+ * the fbdev core when registering the driver, and later on through the hotplug
+ * callback.
+ */
 int drm_fb_helper_set_par(struct fb_info *info)
 {
 	struct drm_fb_helper *fb_helper = info->par;
@@ -712,6 +787,11 @@ int drm_fb_helper_set_par(struct fb_info *info)
 }
 EXPORT_SYMBOL(drm_fb_helper_set_par);
 
+/**
+ * drm_fb_helper_pan_display - implementation for ->fb_pan_display
+ * @var: updated screen information
+ * @info: fbdev registered by the helper
+ */
 int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
 			      struct fb_info *info)
 {
@@ -750,8 +830,9 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
 EXPORT_SYMBOL(drm_fb_helper_pan_display);
 
 /*
- * Allocates the backing storage through the ->fb_probe callback and then
- * registers the fbdev and sets up the panic notifier.
+ * Allocates the backing storage and sets up the fbdev info structure through
+ * the ->fb_probe callback and then registers the fbdev and sets up the panic
+ * notifier.
  */
 static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 					 int preferred_bpp)
@@ -873,6 +954,19 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 	return 0;
 }
 
+/**
+ * drm_fb_helper_fill_fix - initializes fixed fbdev information
+ * @info: fbdev registered by the helper
+ * @pitch: desired pitch
+ * @depth: desired depth
+ *
+ * Helper to fill in the fixed fbdev information useful for a non-accelerated
+ * fbdev emulations. Drivers which support acceleration methods which impose
+ * additional constraints need to set up their own limits.
+ *
+ * Drivers should call this (or their equivalent setup code) from their
+ * ->fb_probe callback.
+ */
 void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
 			    uint32_t depth)
 {
@@ -893,6 +987,20 @@ void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
 }
 EXPORT_SYMBOL(drm_fb_helper_fill_fix);
 
+/**
+ * drm_fb_helper_fill_var - initalizes variable fbdev information
+ * @info: fbdev instance to set up
+ * @fb_helper: fb helper instance to use as template
+ * @fb_width: desired fb width
+ * @fb_height: desired fb height
+ *
+ * Sets up the variable fbdev metainformation from the given fb helper instance
+ * and the drm framebuffer allocated in fb_helper->fb.
+ *
+ * Drivers should call this (or their equivalent setup code) from their
+ * ->fb_probe callback after having allocated the fbdev backing
+ * storage framebuffer.
+ */
 void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helper,
 			    uint32_t fb_width, uint32_t fb_height)
 {
@@ -1355,18 +1463,23 @@ out:
 }
 
 /**
- * drm_helper_initial_config - setup a sane initial connector configuration
+ * drm_fb_helper_initial_config - setup a sane initial connector configuration
  * @fb_helper: fb_helper device struct
  * @bpp_sel: bpp value to use for the framebuffer configuration
  *
- * LOCKING:
- * Called at init time by the driver to set up the @fb_helper initial
- * configuration, must take the mode config lock.
- *
  * Scans the CRTCs and connectors and tries to put together an initial setup.
  * At the moment, this is a cloned configuration across all heads with
  * a new framebuffer object as the backing store.
  *
+ * Note that this also registers the fbdev and so allows userspace to call into
+ * the driver through the fbdev interfaces.
+ *
+ * This function will call down into the ->fb_probe callback to let
+ * the driver allocate and initialize the fbdev info structure and the drm
+ * framebuffer used to back the fbdev. drm_fb_helper_fill_var() and
+ * drm_fb_helper_fill_fix() are provided as helpers to setup simple default
+ * values for the fbdev info structure.
+ *
  * RETURNS:
  * Zero if everything went ok, nonzero otherwise.
  */
@@ -1397,12 +1510,17 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
  *                               probing all the outputs attached to the fb
  * @fb_helper: the drm_fb_helper
  *
- * LOCKING:
- * Called at runtime, must take mode config lock.
- *
  * Scan the connectors attached to the fb_helper and try to put together a
  * setup after *notification of a change in output configuration.
  *
+ * Called at runtime, takes the mode config locks to be able to check/change the
+ * modeset configuration. Must be run from process context (which usually means
+ * either the output polling work or a work item launched from the driver's
+ * hotplug interrupt).
+ *
+ * Note that the driver must ensure that this is only called _after_ the fb has
+ * been fully set up, i.e. after the call to drm_fb_helper_initial_config.
+ *
  * RETURNS:
  * 0 on success and a non-zero error code otherwise.
  */
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 973402d..a60311c 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -48,6 +48,18 @@ struct drm_fb_helper_surface_size {
 	u32 surface_depth;
 };
 
+/**
+ * struct drm_fb_helper_funcs - driver callbacks for the fbdev emulation library
+ * @gamma_set: - Set the given gamma lut register on the given crtc.
+ * @gamma_get: - Read the given gamma lut register on the given crtc, used to
+ * 		 safe the current lut when force-restoring the fbdev for e.g.
+ * 		 kdbg.
+ * @fb_probe: - Driver callback to allocate and initialize the fbdev info
+ * 		structure. Futhermore it also needs to allocate the drm
+ * 		framebuffer used to back the fbdev.
+ *
+ * Driver callbacks used by the fbdev emulation helper library.
+ */
 struct drm_fb_helper_funcs {
 	void (*gamma_set)(struct drm_crtc *crtc, u16 red, u16 green,
 			  u16 blue, int regno);
-- 
1.7.10.4

Re: [PATCH] drm/fb-helper: improve kerneldoc

[Laurent Pinchart]

Hi Daniel,

Thanks for the patch. Just one last small comment.

On Tuesday 05 February 2013 22:43:48 Daniel Vetter wrote:
> Now that the fbdev helper interface for drivers is trimmed down,
> update the kerneldoc for all the remaining exported functions.
> 
> I've tried to beat the DocBook a bit by reordering the function
> references a bit into a more sensible ordering. But that didn't work
> out at all. Hence just extend the in-code DOC: section a bit.
> 
> Also remove the LOCKING: sections - especially for the setup functions
> they're totally bogus. But that's not a documentation problem, but
> simply an artifact of the current rather hazardous locking around drm
> init and even more so around fbdev setup ...
> 
> v2: Some further improvements:
> - Also add documentation for drm_fb_helper_single_add_all_connectors,
>   Dave Airlie didn't want me to kill this one from the fb helper
>   interface.
> - Update docs for drm_fb_helper_fill_var/fix - they should be used
>   from the driver's ->fb_probe callback to setup the fbdev info
>   structure.
> - Clarify what the ->fb_probe callback should all do - it needs to
>   setup both the fbdev info and allocate the drm framebuffer used as
>   backing storage.
> - Add basic documentaation for the drm_fb_helper_funcs driver callback
>   vfunc.
> 
> v3: Implement clarifications Lauren Pinchart suggested in his review.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  Documentation/DocBook/drm.tmpl  |    1 +
>  drivers/gpu/drm/drm_fb_helper.c |  148 ++++++++++++++++++++++++++++++++----
>  include/drm/drm_fb_helper.h     |   12 ++++
>  3 files changed, 146 insertions(+), 15 deletions(-)

[snip]

> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 973402d..a60311c 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -48,6 +48,18 @@ struct drm_fb_helper_surface_size {
>  	u32 surface_depth;
>  };
> 
> +/**
> + * struct drm_fb_helper_funcs - driver callbacks for the fbdev emulation
> library
> + * @gamma_set: - Set the given gamma lut register on the given crtc.
> + * @gamma_get: - Read the given gamma lut register on the given crtc, used
> + * 		 to safe the current lut when force-restoring the fbdev for e.g.
> + * 		 kdbg.

s/safe/save/

With this changed,

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

> + * @fb_probe: - Driver callback to allocate and initialize the fbdev info
> + * 		structure. Futhermore it also needs to allocate the drm
> + * 		framebuffer used to back the fbdev.
> + *
> + * Driver callbacks used by the fbdev emulation helper library.
> + */
>  struct drm_fb_helper_funcs {
>  	void (*gamma_set)(struct drm_crtc *crtc, u16 red, u16 green,
>  			  u16 blue, int regno);

-- 
Regards,

Laurent Pinchart

[PATCH 15/16] drm/fb-helper: improve kerneldoc

[Daniel Vetter]

Now that the fbdev helper interface for drivers is trimmed down,
update the kerneldoc for all the remaining exported functions.

I've tried to beat the DocBook a bit by reordering the function
references a bit into a more sensible ordering. But that didn't work
out at all. Hence just extend the in-code DOC: section a bit.

Also remove the LOCKING: sections - especially for the setup functions
they're totally bogus. But that's not a documentation problem, but
simply an artifact of the current rather hazardous locking around drm
init and even more so around fbdev setup ...

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_fb_helper.c |   97 +++++++++++++++++++++++++++++++++++----
 1 file changed, 88 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index edbfcde..b774101 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -52,6 +52,17 @@ static LIST_HEAD(kernel_fb_helper_list);
  * mode setting driver. They can be used mostly independantely from the crtc
  * helper functions used by many drivers to implement the kernel mode setting
  * interfaces.
+ *
+ * Initialization is done as a two-step process with drm_fb_helper_init() and
+ * drm_fb_helper_initial_config(), teardown is done with drm_fb_helper_fini().
+ *
+ * At runtime drivers should restore the fbdev console by calling
+ * drm_fb_helper_restore_fbdev_mode() from their ->lastclose callback. They can
+ * also notify the fb helper code from updates to the output configuration by
+ * calling drm_fb_helper_hotplug_event().
+ *
+ * All other functions exported by the fb helper library can be used to
+ * implement the fbdev driver interface by the driver.
  */
 
 /* simple single crtc case helper function */
@@ -162,6 +173,10 @@ static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc)
 	crtc->funcs->gamma_set(crtc, r_base, g_base, b_base, 0, crtc->gamma_size);
 }
 
+/**
+ * drm_fb_helper_debug_enter - implementation for ->fb_debug_enter
+ * @info: fbdev registered by the helper.
+ */
 int drm_fb_helper_debug_enter(struct fb_info *info)
 {
 	struct drm_fb_helper *helper = info->par;
@@ -207,6 +222,10 @@ static struct drm_framebuffer *drm_mode_config_fb(struct drm_crtc *crtc)
 	return NULL;
 }
 
+/**
+ * drm_fb_helper_debug_leave - implementation for ->fb_debug_leave
+ * @info: fbdev registered by the helper.
+ */
 int drm_fb_helper_debug_leave(struct fb_info *info)
 {
 	struct drm_fb_helper *helper = info->par;
@@ -377,6 +396,11 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
 	drm_modeset_unlock_all(dev);
 }
 
+/**
+ * drm_fb_helper_blank - implementation for ->fb_blank
+ * @blank: desired blanking state
+ * @info: fbdev registered by the helper.
+ */
 int drm_fb_helper_blank(int blank, struct fb_info *info)
 {
 	switch (blank) {
@@ -420,6 +444,24 @@ static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
 	kfree(helper->crtc_info);
 }
 
+/**
+ * drm_fb_helper_init - initialize a drm_fb_helper structure
+ * @dev: drm device
+ * @fb_helper: driver-allocated fbdev helper structure to initialize
+ * @crtc_count: crtc count
+ * @max_conn_count: max connector count
+ *
+ * This allocates the structures for the fbdev helper with the given limits.
+ * Note that this won't yet touch the hw (through the driver interfaces) nor
+ * register the fbdev. This is only done in drm_fb_helper_initial_config() to
+ * allow driver writes more control over the exact init sequence.
+ *
+ * Drivers must also set fb_helper->funcs before calling
+ * drm_fb_helper_initial_config().
+ *
+ * RETURNS:
+ * Zero if everything went ok, nonzero otherwise.
+ */
 int drm_fb_helper_init(struct drm_device *dev,
 		       struct drm_fb_helper *fb_helper,
 		       int crtc_count, int max_conn_count)
@@ -552,6 +594,11 @@ static int setcolreg(struct drm_crtc *crtc, u16 red, u16 green,
 	return 0;
 }
 
+/**
+ * drm_fb_helper_setcmap - implementation for ->fb_setcmap
+ * @cmap: cmap to set
+ * @info: fbdev registered by the helper.
+ */
 int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
 {
 	struct drm_fb_helper *fb_helper = info->par;
@@ -591,6 +638,10 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
 }
 EXPORT_SYMBOL(drm_fb_helper_setcmap);
 
+/**
+ * drm_fb_helper_check_var - implementation for ->fb_check_var
+ * @info: fbdev registered by the helper.
+ */
 int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 			    struct fb_info *info)
 {
@@ -683,7 +734,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 }
 EXPORT_SYMBOL(drm_fb_helper_check_var);
 
-/* this will let fbcon do the mode init */
+/**
+ * drm_fb_helper_set_par - implementation for ->fb_set_par
+ * @info: fbdev registered by the helper.
+ *
+ * This will let fbcon do the mode init and is called at initialization time by
+ * the fbdev core when registering the driver, and later on through the hotplug
+ * callback.
+ */
 int drm_fb_helper_set_par(struct fb_info *info)
 {
 	struct drm_fb_helper *fb_helper = info->par;
@@ -715,6 +773,11 @@ int drm_fb_helper_set_par(struct fb_info *info)
 }
 EXPORT_SYMBOL(drm_fb_helper_set_par);
 
+/**
+ * drm_fb_helper_pan_display - implementation for ->fb_pan_display
+ * @var: updated screen information
+ * @info: fbdev registered by the helper.
+ */
 int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
 			      struct fb_info *info)
 {
@@ -876,6 +939,12 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 	return 0;
 }
 
+/**
+ * drm_fb_helper_fill_fix - implementation for ->fb_fill_fix
+ * @info: fbdev registered by the helper.
+ * @pitch: desired pitch
+ * @depth: desired depth
+ */
 void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
 			    uint32_t depth)
 {
@@ -896,6 +965,12 @@ void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
 }
 EXPORT_SYMBOL(drm_fb_helper_fill_fix);
 
+/**
+ * drm_fb_helper_fill_var - implementation for ->fb_fill_var
+ * @info: fbdev registered by the helper.
+ * @fb_width: desired fb width
+ * @fb_height: desired fb height
+ */
 void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helper,
 			    uint32_t fb_width, uint32_t fb_height)
 {
@@ -1358,18 +1433,17 @@ out:
 }
 
 /**
- * drm_helper_initial_config - setup a sane initial connector configuration
+ * drm_fb_helper_initial_config - setup a sane initial connector configuration
  * @fb_helper: fb_helper device struct
  * @bpp_sel: bpp value to use for the framebuffer configuration
  *
- * LOCKING:
- * Called at init time by the driver to set up the @fb_helper initial
- * configuration, must take the mode config lock.
- *
  * Scans the CRTCs and connectors and tries to put together an initial setup.
  * At the moment, this is a cloned configuration across all heads with
  * a new framebuffer object as the backing store.
  *
+ * Note that this also registers the fbdev and so allows userspace to call into
+ * the driver through the fbdev interfaces.
+ *
  * RETURNS:
  * Zero if everything went ok, nonzero otherwise.
  */
@@ -1400,12 +1474,17 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
  *                               probing all the outputs attached to the fb
  * @fb_helper: the drm_fb_helper
  *
- * LOCKING:
- * Called at runtime, must take mode config lock.
- *
  * Scan the connectors attached to the fb_helper and try to put together a
  * setup after *notification of a change in output configuration.
  *
+ * Called at runtime, takes the mode config locks to be able to check/change the
+ * modeset configuration. Must be run from process context (which usually means
+ * either the output polling work or a work item launch from the driver's
+ * hotplug interrupt).
+ *
+ * Note that the driver must ensure that this is only called _after_ the fb has
+ * been fully set up, i.e. after the call to drm_fb_helper_initial_config.
+ *
  * RETURNS:
  * 0 on success and a non-zero error code otherwise.
  */
-- 
1.7.10.4

[PATCH] drm/fb-helper: improve kerneldoc

[Daniel Vetter]

Now that the fbdev helper interface for drivers is trimmed down,
update the kerneldoc for all the remaining exported functions.

I've tried to beat the DocBook a bit by reordering the function
references a bit into a more sensible ordering. But that didn't work
out at all. Hence just extend the in-code DOC: section a bit.

Also remove the LOCKING: sections - especially for the setup functions
they're totally bogus. But that's not a documentation problem, but
simply an artifact of the current rather hazardous locking around drm
init and even more so around fbdev setup ...

v2: Some further improvements:
- Also add documentation for drm_fb_helper_single_add_all_connectors,
  Dave Airlie didn't want me to kill this one from the fb helper
  interface.
- Update docs for drm_fb_helper_fill_var/fix - they should be used
  from the driver's ->fb_probe callback to setup the fbdev info
  structure.
- Clarify what the ->fb_probe callback should all do - it needs to
  setup both the fbdev info and allocate the drm framebuffer used as
  backing storage.
- Add basic documentaation for the drm_fb_helper_funcs driver callback
  vfunc.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

moar kerneldoc
---
 Documentation/DocBook/drm.tmpl  |    1 +
 drivers/gpu/drm/drm_fb_helper.c |  146 +++++++++++++++++++++++++++++++++++----
 include/drm/drm_fb_helper.h     |   11 +++
 3 files changed, 143 insertions(+), 15 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 6c11d77..14ad829 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -2139,6 +2139,7 @@ void intel_crt_init(struct drm_device *dev)
       <title>fbdev Helper Functions Reference</title>
 !Pdrivers/gpu/drm/drm_fb_helper.c fbdev helpers
 !Edrivers/gpu/drm/drm_fb_helper.c
+!Iinclude/drm/drm_fb_helper.h
     </sect2>
     <sect2>
       <title>Display Port Helper Functions Reference</title>
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 5a92470..a7538cc 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -52,9 +52,34 @@ static LIST_HEAD(kernel_fb_helper_list);
  * mode setting driver. They can be used mostly independantely from the crtc
  * helper functions used by many drivers to implement the kernel mode setting
  * interfaces.
+ *
+ * Initialization is done as a three-step process with drm_fb_helper_init(),
+ * drm_fb_helper_single_add_all_connectors() and drm_fb_helper_initial_config().
+ * Drivers with fancier requirements than the default beheviour can override the
+ * second step with their own code.  Teardown is done with drm_fb_helper_fini().
+ *
+ * At runtime drivers should restore the fbdev console by calling
+ * drm_fb_helper_restore_fbdev_mode() from their ->lastclose callback. They can
+ * also notify the fb helper code from updates to the output configuration by
+ * calling drm_fb_helper_hotplug_event().
+ *
+ * All other functions exported by the fb helper library can be used to
+ * implement the fbdev driver interface by the driver.
  */
 
-/* simple single crtc case helper function */
+/**
+ * drm_fb_helper_single_add_all_connectors() - add all connectors to fbdev
+ * 					       emulation helper
+ * @fb_helper: fbdev initialized with drm_fb_helper_init
+ *
+ * This functions adds all the available connectors for use with the given
+ * fb_helper. This is a separate step to allow drivers to freely assign
+ * connectors to the fbdev, e.g. if some are reserved for special purposes or
+ * not adequate to be used for the fbcon.
+ *
+ * Since this is part of the initial setup before the fbdev is published, no
+ * locking is required.
+ */
 int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
@@ -163,6 +188,10 @@ static void drm_fb_helper_restore_lut_atomic(struct drm_crtc *crtc)
 	crtc->funcs->gamma_set(crtc, r_base, g_base, b_base, 0, crtc->gamma_size);
 }
 
+/**
+ * drm_fb_helper_debug_enter - implementation for ->fb_debug_enter
+ * @info: fbdev registered by the helper.
+ */
 int drm_fb_helper_debug_enter(struct fb_info *info)
 {
 	struct drm_fb_helper *helper = info->par;
@@ -208,6 +237,10 @@ static struct drm_framebuffer *drm_mode_config_fb(struct drm_crtc *crtc)
 	return NULL;
 }
 
+/**
+ * drm_fb_helper_debug_leave - implementation for ->fb_debug_leave
+ * @info: fbdev registered by the helper.
+ */
 int drm_fb_helper_debug_leave(struct fb_info *info)
 {
 	struct drm_fb_helper *helper = info->par;
@@ -243,9 +276,9 @@ EXPORT_SYMBOL(drm_fb_helper_debug_leave);
  * drm_fb_helper_restore_fbdev_mode - restore fbdev configuration
  * @fb_helper: fbcon to restore
  *
- * This should be called from driver's drm->lastclose callback when implementing
- * an fbcon on top of kms using this helper. This ensures that the user isn't
- * greeted with a black screen when e.g. X dies.
+ * This should be called from driver's drm <code>->lastclose</code> callback
+ * when implementing an fbcon on top of kms using this helper. This ensures that
+ * the user isn't greeted with a black screen when e.g. X dies.
  */
 bool drm_fb_helper_restore_fbdev_mode(struct drm_fb_helper *fb_helper)
 {
@@ -378,6 +411,11 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
 	drm_modeset_unlock_all(dev);
 }
 
+/**
+ * drm_fb_helper_blank - implementation for ->fb_blank
+ * @blank: desired blanking state
+ * @info: fbdev registered by the helper.
+ */
 int drm_fb_helper_blank(int blank, struct fb_info *info)
 {
 	switch (blank) {
@@ -421,6 +459,24 @@ static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
 	kfree(helper->crtc_info);
 }
 
+/**
+ * drm_fb_helper_init - initialize a drm_fb_helper structure
+ * @dev: drm device
+ * @fb_helper: driver-allocated fbdev helper structure to initialize
+ * @crtc_count: crtc count
+ * @max_conn_count: max connector count
+ *
+ * This allocates the structures for the fbdev helper with the given limits.
+ * Note that this won't yet touch the hw (through the driver interfaces) nor
+ * register the fbdev. This is only done in drm_fb_helper_initial_config() to
+ * allow driver writes more control over the exact init sequence.
+ *
+ * Drivers must also set fb_helper->funcs before calling
+ * drm_fb_helper_initial_config().
+ *
+ * RETURNS:
+ * Zero if everything went ok, nonzero otherwise.
+ */
 int drm_fb_helper_init(struct drm_device *dev,
 		       struct drm_fb_helper *fb_helper,
 		       int crtc_count, int max_conn_count)
@@ -549,6 +605,11 @@ static int setcolreg(struct drm_crtc *crtc, u16 red, u16 green,
 	return 0;
 }
 
+/**
+ * drm_fb_helper_setcmap - implementation for ->fb_setcmap
+ * @cmap: cmap to set
+ * @info: fbdev registered by the helper.
+ */
 int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
 {
 	struct drm_fb_helper *fb_helper = info->par;
@@ -588,6 +649,11 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
 }
 EXPORT_SYMBOL(drm_fb_helper_setcmap);
 
+/**
+ * drm_fb_helper_check_var - implementation for ->fb_check_var
+ * @var: screeninfo to check
+ * @info: fbdev registered by the helper.
+ */
 int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 			    struct fb_info *info)
 {
@@ -680,7 +746,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 }
 EXPORT_SYMBOL(drm_fb_helper_check_var);
 
-/* this will let fbcon do the mode init */
+/**
+ * drm_fb_helper_set_par - implementation for ->fb_set_par
+ * @info: fbdev registered by the helper.
+ *
+ * This will let fbcon do the mode init and is called at initialization time by
+ * the fbdev core when registering the driver, and later on through the hotplug
+ * callback.
+ */
 int drm_fb_helper_set_par(struct fb_info *info)
 {
 	struct drm_fb_helper *fb_helper = info->par;
@@ -712,6 +785,11 @@ int drm_fb_helper_set_par(struct fb_info *info)
 }
 EXPORT_SYMBOL(drm_fb_helper_set_par);
 
+/**
+ * drm_fb_helper_pan_display - implementation for ->fb_pan_display
+ * @var: updated screen information
+ * @info: fbdev registered by the helper.
+ */
 int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
 			      struct fb_info *info)
 {
@@ -750,8 +828,9 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
 EXPORT_SYMBOL(drm_fb_helper_pan_display);
 
 /*
- * Allocates the backing storage through the ->fb_probe callback and then
- * registers the fbdev and sets up the panic notifier.
+ * Allocates the backing storage and sets up the fbdev info structure through
+ * the ->fb_probe callback and then registers the fbdev and sets up the panic
+ * notifier.
  */
 static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 					 int preferred_bpp)
@@ -873,6 +952,19 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 	return 0;
 }
 
+/**
+ * drm_fb_helper_fill_fix - initializes fixed fbdev information
+ * @info: fbdev registered by the helper.
+ * @pitch: desired pitch
+ * @depth: desired depth
+ *
+ * Helper to fill in the fixed fbdev information useful for a non-accelerated
+ * fbdev emulations. Drivers which support acceleration methods which impose
+ * additional constraints need to set up their own limits.
+ *
+ * Drivers should call this (or their equivalent setup code) from their
+ * <code>->fb_probe</code> callback.
+ */
 void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
 			    uint32_t depth)
 {
@@ -893,6 +985,20 @@ void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
 }
 EXPORT_SYMBOL(drm_fb_helper_fill_fix);
 
+/**
+ * drm_fb_helper_fill_var - initalizes variable fbdev information
+ * @info: fbdev instance to set up
+ * @fb_helper: fb helper instance to use as template
+ * @fb_width: desired fb width
+ * @fb_height: desired fb height
+ *
+ * Sets up the variable fbdev metainformation from the given fb helper instance
+ * and the drm framebuffer allocated in <code>fb_helper->fb</code>.
+ *
+ * Drivers should call this (or their equivalent setup code) from their
+ * <code>->fb_probe</code> callback after having allocated the fbdev backing
+ * storage framebuffer.
+ */
 void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper *fb_helper,
 			    uint32_t fb_width, uint32_t fb_height)
 {
@@ -1355,18 +1461,23 @@ out:
 }
 
 /**
- * drm_helper_initial_config - setup a sane initial connector configuration
+ * drm_fb_helper_initial_config - setup a sane initial connector configuration
  * @fb_helper: fb_helper device struct
  * @bpp_sel: bpp value to use for the framebuffer configuration
  *
- * LOCKING:
- * Called at init time by the driver to set up the @fb_helper initial
- * configuration, must take the mode config lock.
- *
  * Scans the CRTCs and connectors and tries to put together an initial setup.
  * At the moment, this is a cloned configuration across all heads with
  * a new framebuffer object as the backing store.
  *
+ * Note that this also registers the fbdev and so allows userspace to call into
+ * the driver through the fbdev interfaces.
+ *
+ * This function will call down into the <code>->fb_probe</code> callback to let
+ * the driver allocate and initialize the fbdev info structure and the drm
+ * framebuffer used to back the fbdev. drm_fb_helper_fill_var() and
+ * drm_fb_helper_fill_fix() are provided as helpers to setup simple default
+ * values for the fbdev info structure.
+ *
  * RETURNS:
  * Zero if everything went ok, nonzero otherwise.
  */
@@ -1397,12 +1508,17 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
  *                               probing all the outputs attached to the fb
  * @fb_helper: the drm_fb_helper
  *
- * LOCKING:
- * Called at runtime, must take mode config lock.
- *
  * Scan the connectors attached to the fb_helper and try to put together a
  * setup after *notification of a change in output configuration.
  *
+ * Called at runtime, takes the mode config locks to be able to check/change the
+ * modeset configuration. Must be run from process context (which usually means
+ * either the output polling work or a work item launch from the driver's
+ * hotplug interrupt).
+ *
+ * Note that the driver must ensure that this is only called _after_ the fb has
+ * been fully set up, i.e. after the call to drm_fb_helper_initial_config.
+ *
  * RETURNS:
  * 0 on success and a non-zero error code otherwise.
  */
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 973402d..3c30297 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -48,6 +48,17 @@ struct drm_fb_helper_surface_size {
 	u32 surface_depth;
 };
 
+/**
+ * struct drm_fb_helper_funcs - driver callbacks for the fbdev emulation library
+ * @gamma_set: - Set the given lut register on the given crtc.
+ * @gamma_get: - Read the given lut register on the given crtc, used to safe the
+ *		 current lut when force-restoring the fbdev for e.g. kdbg.
+ * @fb_probe: - Driver callback to allocate and initialize the fbdev info
+ * 		structure. Futhermore it also needs to allocate the drm
+ * 		framebuffer used to back the fbdev.
+ *
+ * Driver callbacks used by the fbdev emulation helper library.
+ */
 struct drm_fb_helper_funcs {
 	void (*gamma_set)(struct drm_crtc *crtc, u16 red, u16 green,
 			  u16 blue, int regno);
-- 
1.7.10.4

Re: [PATCH] drm/fb-helper: improve kerneldoc

[Laurent Pinchart]

Hi Daniel,

Thanks for improving the documentation :-)

On Sunday 27 January 2013 18:42:16 Daniel Vetter wrote:
> Now that the fbdev helper interface for drivers is trimmed down,
> update the kerneldoc for all the remaining exported functions.
> 
> I've tried to beat the DocBook a bit by reordering the function
> references a bit into a more sensible ordering. But that didn't work
> out at all. Hence just extend the in-code DOC: section a bit.
> 
> Also remove the LOCKING: sections - especially for the setup functions
> they're totally bogus. But that's not a documentation problem, but
> simply an artifact of the current rather hazardous locking around drm
> init and even more so around fbdev setup ...

Please see below for comments (I've reflowed the text to ease review).

> v2: Some further improvements:
> - Also add documentation for drm_fb_helper_single_add_all_connectors,
>   Dave Airlie didn't want me to kill this one from the fb helper
>   interface.
> - Update docs for drm_fb_helper_fill_var/fix - they should be used
>   from the driver's ->fb_probe callback to setup the fbdev info
>   structure.
> - Clarify what the ->fb_probe callback should all do - it needs to
>   setup both the fbdev info and allocate the drm framebuffer used as
>   backing storage.
> - Add basic documentaation for the drm_fb_helper_funcs driver callback
>   vfunc.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> moar kerneldoc
> ---
>  Documentation/DocBook/drm.tmpl  |    1 +
>  drivers/gpu/drm/drm_fb_helper.c |  146 ++++++++++++++++++++++++++++++++----
>  include/drm/drm_fb_helper.h     |   11 +++
>  3 files changed, 143 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 6c11d77..14ad829 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -2139,6 +2139,7 @@ void intel_crt_init(struct drm_device *dev)
>        <title>fbdev Helper Functions Reference</title>
>  !Pdrivers/gpu/drm/drm_fb_helper.c fbdev helpers
>  !Edrivers/gpu/drm/drm_fb_helper.c
> +!Iinclude/drm/drm_fb_helper.h
>      </sect2>
>      <sect2>
>        <title>Display Port Helper Functions Reference</title>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c
> b/drivers/gpu/drm/drm_fb_helper.c index 5a92470..a7538cc 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -52,9 +52,34 @@ static LIST_HEAD(kernel_fb_helper_list);
>   * mode setting driver. They can be used mostly independantely from the

Now that you have removed one of the dependencies on the crtc helpers in your 
"drm/fb-helper: don't disable everything in initial_config" patch, are there 
others ? If not you can s/mostly //.

>   * crtc helper functions used by many drivers to implement the kernel mode
>   * setting interfaces.
> + *
> + * Initialization is done as a three-step process with
> + * drm_fb_helper_init(), drm_fb_helper_single_add_all_connectors() and
> + * drm_fb_helper_initial_config(). Drivers with fancier requirements than
> + * the default beheviour can override the second step with their own code.
> + * Teardown is done with drm_fb_helper_fini().
> + *
> + * At runtime drivers should restore the fbdev console by calling
> + * drm_fb_helper_restore_fbdev_mode() from their ->lastclose callback. They
> + * can also notify the fb helper code from updates to the output

Is it "can" or "must" ? If "can", in what conditions should they call 
drm_fb_helper_restore_fbdev_mode() and in what conditions shouldn't they ?

> + * configuration by calling drm_fb_helper_hotplug_event().
> + *
> + * All other functions exported by the fb helper library can be used to
> + * implement the fbdev driver interface by the driver.
>   */
> 
> -/* simple single crtc case helper function */
> +/**
> + * drm_fb_helper_single_add_all_connectors() - add all connectors to fbdev
> + * 					       emulation helper
> + * @fb_helper: fbdev initialized with drm_fb_helper_init
> + *
> + * This functions adds all the available connectors for use with the given
> + * fb_helper. This is a separate step to allow drivers to freely assign or
> + * connectors to the fbdev, e.g. if some are reserved for special purposes
> + * not adequate to be used for the fbcon.
> + *
> + * Since this is part of the initial setup before the fbdev is published,
> + * no locking is required.
> + */
>  int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper
> *fb_helper)
> {
>  	struct drm_device *dev = fb_helper->dev;
> @@ -163,6 +188,10 @@ static void drm_fb_helper_restore_lut_atomic(struct
> drm_crtc *crtc) crtc->funcs->gamma_set(crtc, r_base, g_base, b_base, 0,
> crtc->gamma_size); }
> 
> +/**
> + * drm_fb_helper_debug_enter - implementation for ->fb_debug_enter
> + * @info: fbdev registered by the helper.
> + */
>  int drm_fb_helper_debug_enter(struct fb_info *info)
>  {
>  	struct drm_fb_helper *helper = info->par;
> @@ -208,6 +237,10 @@ static struct drm_framebuffer
> *drm_mode_config_fb(struct drm_crtc *crtc) return NULL;
>  }
> 
> +/**
> + * drm_fb_helper_debug_leave - implementation for ->fb_debug_leave
> + * @info: fbdev registered by the helper.
> + */
>  int drm_fb_helper_debug_leave(struct fb_info *info)
>  {
>  	struct drm_fb_helper *helper = info->par;
> @@ -243,9 +276,9 @@ EXPORT_SYMBOL(drm_fb_helper_debug_leave);
>   * drm_fb_helper_restore_fbdev_mode - restore fbdev configuration
>   * @fb_helper: fbcon to restore
>   *
> - * This should be called from driver's drm->lastclose callback when
> - * implementing an fbcon on top of kms using this helper. This ensures that
> - * the user isn't greeted with a black screen when e.g. X dies.
> + * This should be called from driver's drm <code>->lastclose</code>

The resulting HTML is

&lt;code&gt;-&gt;lastclose&lt;/code&gt;

I'm not sure that's what you want :-)

> + * callback when implementing an fbcon on top of kms using this helper.
> + * This ensures that the user isn't greeted with a black screen when e.g.
> + * X dies.
>   */
>  bool drm_fb_helper_restore_fbdev_mode(struct drm_fb_helper *fb_helper)
>  {
> @@ -378,6 +411,11 @@ static void drm_fb_helper_dpms(struct fb_info *info,
> int dpms_mode) drm_modeset_unlock_all(dev);
>  }
> 
> +/**
> + * drm_fb_helper_blank - implementation for ->fb_blank
> + * @blank: desired blanking state
> + * @info: fbdev registered by the helper.

Nitpicking here, shouldn't you either use a full stop at the end of each 
argument, or no full stop at all (this applies to the other functions as well) 
?

> + */
>  int drm_fb_helper_blank(int blank, struct fb_info *info)
>  {
>  	switch (blank) {
> @@ -421,6 +459,24 @@ static void drm_fb_helper_crtc_free(struct
> drm_fb_helper *helper) kfree(helper->crtc_info);
>  }
> 
> +/**
> + * drm_fb_helper_init - initialize a drm_fb_helper structure
> + * @dev: drm device
> + * @fb_helper: driver-allocated fbdev helper structure to initialize
> + * @crtc_count: crtc count
> + * @max_conn_count: max connector count
> + *
> + * This allocates the structures for the fbdev helper with the given
> + * limits. Note that this won't yet touch the hw (through the driver

s/hw/hardware/ ?

It might be a good idea to add a small description of the crtc_count 
parameter.

> + * interfaces) nor register the fbdev. This is only done in
> + * drm_fb_helper_initial_config() to allow driver writes more control over
> + * the exact init sequence.
> + *
> + * Drivers must also set fb_helper->funcs before calling

s/also // ?

> + * drm_fb_helper_initial_config().
> + *
> + * RETURNS:
> + * Zero if everything went ok, nonzero otherwise.
> + */
>  int drm_fb_helper_init(struct drm_device *dev,
>  		       struct drm_fb_helper *fb_helper,
>  		       int crtc_count, int max_conn_count)
> @@ -549,6 +605,11 @@ static int setcolreg(struct drm_crtc *crtc, u16 red,
> u16 green, return 0;
>  }
> 
> +/**
> + * drm_fb_helper_setcmap - implementation for ->fb_setcmap
> + * @cmap: cmap to set
> + * @info: fbdev registered by the helper.
> + */
>  int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
>  {
>  	struct drm_fb_helper *fb_helper = info->par;
> @@ -588,6 +649,11 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct
> fb_info *info) }
>  EXPORT_SYMBOL(drm_fb_helper_setcmap);
> 
> +/**
> + * drm_fb_helper_check_var - implementation for ->fb_check_var
> + * @var: screeninfo to check
> + * @info: fbdev registered by the helper.
> + */
>  int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>  			    struct fb_info *info)
>  {
> @@ -680,7 +746,14 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo
> *var, }
>  EXPORT_SYMBOL(drm_fb_helper_check_var);
> 
> -/* this will let fbcon do the mode init */
> +/**
> + * drm_fb_helper_set_par - implementation for ->fb_set_par
> + * @info: fbdev registered by the helper.
> + *
> + * This will let fbcon do the mode init and is called at initialization
> + * time by the fbdev core when registering the driver, and later on
> + * through the hotplug callback.
> + */
>  int drm_fb_helper_set_par(struct fb_info *info)
>  {
>  	struct drm_fb_helper *fb_helper = info->par;
> @@ -712,6 +785,11 @@ int drm_fb_helper_set_par(struct fb_info *info)
>  }
>  EXPORT_SYMBOL(drm_fb_helper_set_par);
> 
> +/**
> + * drm_fb_helper_pan_display - implementation for ->fb_pan_display
> + * @var: updated screen information
> + * @info: fbdev registered by the helper.
> + */
>  int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
>  			      struct fb_info *info)
>  {
> @@ -750,8 +828,9 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo
> *var, EXPORT_SYMBOL(drm_fb_helper_pan_display);
> 
>  /*
> - * Allocates the backing storage through the ->fb_probe callback and then
> - * registers the fbdev and sets up the panic notifier.
> + * Allocates the backing storage and sets up the fbdev info structure
> + * through the ->fb_probe callback and then registers the fbdev and sets
> + * up the panic notifier.
>   */
>  static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>  					 int preferred_bpp)
> @@ -873,6 +952,19 @@ static int drm_fb_helper_single_fb_probe(struct
> drm_fb_helper *fb_helper, return 0;
>  }
> 
> +/**
> + * drm_fb_helper_fill_fix - initializes fixed fbdev information
> + * @info: fbdev registered by the helper.
> + * @pitch: desired pitch
> + * @depth: desired depth
> + *
> + * Helper to fill in the fixed fbdev information useful for a
> + * non-accelerated fbdev emulations. Drivers which support acceleration
> + * methods which impose additional constraints need to set up their own
> + * limits.
> + *
> + * Drivers should call this (or their equivalent setup code) from their
> + * <code>->fb_probe</code> callback.
> + */
>  void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
>  			    uint32_t depth)
>  {
> @@ -893,6 +985,20 @@ void drm_fb_helper_fill_fix(struct fb_info *info,
> uint32_t pitch, }
>  EXPORT_SYMBOL(drm_fb_helper_fill_fix);
> 
> +/**
> + * drm_fb_helper_fill_var - initalizes variable fbdev information
> + * @info: fbdev instance to set up
> + * @fb_helper: fb helper instance to use as template
> + * @fb_width: desired fb width
> + * @fb_height: desired fb height
> + *
> + * Sets up the variable fbdev metainformation from the given fb helper
> + * instance and the drm framebuffer allocated in
> + * <code>fb_helper->fb</code>.
> + *
> + * Drivers should call this (or their equivalent setup code) from their
> + * <code>->fb_probe</code> callback after having allocated the fbdev
> + * backing storage framebuffer.
> + */
>  void drm_fb_helper_fill_var(struct fb_info *info, struct drm_fb_helper
> *fb_helper, uint32_t fb_width, uint32_t fb_height)
>  {
> @@ -1355,18 +1461,23 @@ out:
>  }
> 
>  /**
> - * drm_helper_initial_config - setup a sane initial connector configuration
> + * drm_fb_helper_initial_config - setup a sane initial connector
> configuration
> * @fb_helper: fb_helper device struct
>   * @bpp_sel: bpp value to use for the framebuffer configuration
>   *
> - * LOCKING:
> - * Called at init time by the driver to set up the @fb_helper initial
> - * configuration, must take the mode config lock.
> - *
>   * Scans the CRTCs and connectors and tries to put together an initial
>   * setup. At the moment, this is a cloned configuration across all heads
>   * with a new framebuffer object as the backing store.
>   *
> + * Note that this also registers the fbdev and so allows userspace to call
> + * into the driver through the fbdev interfaces.
> + *
> + * This function will call down into the <code>->fb_probe</code> callback

<code> is displayed in the resulting HTML here as well.

> + * to let the driver allocate and initialize the fbdev info structure and
> + * the drm framebuffer used to back the fbdev. drm_fb_helper_fill_var()
> + * and drm_fb_helper_fill_fix() are provided as helpers to setup simple
> + * default values for the fbdev info structure.
> + *
>   * RETURNS:
>   * Zero if everything went ok, nonzero otherwise.
>   */
> @@ -1397,12 +1508,17 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
>   *                               probing all the outputs attached to the fb
> * @fb_helper: the drm_fb_helper
>   *
> - * LOCKING:
> - * Called at runtime, must take mode config lock.
> - *
>   * Scan the connectors attached to the fb_helper and try to put together a
>   * setup after *notification of a change in output configuration.
>   *
> + * Called at runtime, takes the mode config locks to be able to
> + * check/change the modeset configuration. Must be run from process
> + * context (which usually means either the output polling work or a work
> + * item launch from the driver's hotplug interrupt).

s/launch/launched/

> + * Note that the driver must ensure that this is only called _after_ the fb
> + * has been fully set up, i.e. after the call to
> + * drm_fb_helper_initial_config.
> + *
>   * RETURNS:
>   * 0 on success and a non-zero error code otherwise.
>   */
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 973402d..3c30297 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -48,6 +48,17 @@ struct drm_fb_helper_surface_size {
>  	u32 surface_depth;
>  };
> 
> +/**
> + * struct drm_fb_helper_funcs - driver callbacks for the fbdev emulation
> library
> + * @gamma_set: - Set the given lut register on the given crtc.
> + * @gamma_get: - Read the given lut register on the given crtc, used to
> safe the

s/lut/gamma lut/ ?

> + *		 current lut when force-restoring the fbdev for e.g. kdbg.
> + * @fb_probe: - Driver callback to allocate and initialize the fbdev info
> + * 		structure. Futhermore it also needs to allocate the drm
> + * 		framebuffer used to back the fbdev.
> + *
> + * Driver callbacks used by the fbdev emulation helper library.
> + */
>  struct drm_fb_helper_funcs {
>  	void (*gamma_set)(struct drm_crtc *crtc, u16 red, u16 green,
>  			  u16 blue, int regno);
-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm/fb-helper: improve kerneldoc

[Daniel Vetter]

On Fri, Feb 01, 2013 at 01:21:29PM +0100, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thanks for improving the documentation :-)
> 
> On Sunday 27 January 2013 18:42:16 Daniel Vetter wrote:
> > Now that the fbdev helper interface for drivers is trimmed down,
> > update the kerneldoc for all the remaining exported functions.
> > 
> > I've tried to beat the DocBook a bit by reordering the function
> > references a bit into a more sensible ordering. But that didn't work
> > out at all. Hence just extend the in-code DOC: section a bit.
> > 
> > Also remove the LOCKING: sections - especially for the setup functions
> > they're totally bogus. But that's not a documentation problem, but
> > simply an artifact of the current rather hazardous locking around drm
> > init and even more so around fbdev setup ...
> 
> Please see below for comments (I've reflowed the text to ease review).
> 
> > v2: Some further improvements:
> > - Also add documentation for drm_fb_helper_single_add_all_connectors,
> >   Dave Airlie didn't want me to kill this one from the fb helper
> >   interface.
> > - Update docs for drm_fb_helper_fill_var/fix - they should be used
> >   from the driver's ->fb_probe callback to setup the fbdev info
> >   structure.
> > - Clarify what the ->fb_probe callback should all do - it needs to
> >   setup both the fbdev info and allocate the drm framebuffer used as
> >   backing storage.
> > - Add basic documentaation for the drm_fb_helper_funcs driver callback
> >   vfunc.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > moar kerneldoc
> > ---
> >  Documentation/DocBook/drm.tmpl  |    1 +
> >  drivers/gpu/drm/drm_fb_helper.c |  146 ++++++++++++++++++++++++++++++++----
> >  include/drm/drm_fb_helper.h     |   11 +++
> >  3 files changed, 143 insertions(+), 15 deletions(-)
> > 
> > diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> > index 6c11d77..14ad829 100644
> > --- a/Documentation/DocBook/drm.tmpl
> > +++ b/Documentation/DocBook/drm.tmpl
> > @@ -2139,6 +2139,7 @@ void intel_crt_init(struct drm_device *dev)
> >        <title>fbdev Helper Functions Reference</title>
> >  !Pdrivers/gpu/drm/drm_fb_helper.c fbdev helpers
> >  !Edrivers/gpu/drm/drm_fb_helper.c
> > +!Iinclude/drm/drm_fb_helper.h
> >      </sect2>
> >      <sect2>
> >        <title>Display Port Helper Functions Reference</title>
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c
> > b/drivers/gpu/drm/drm_fb_helper.c index 5a92470..a7538cc 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -52,9 +52,34 @@ static LIST_HEAD(kernel_fb_helper_list);
> >   * mode setting driver. They can be used mostly independantely from the
> 
> Now that you have removed one of the dependencies on the crtc helpers in your 
> "drm/fb-helper: don't disable everything in initial_config" patch, are there 
> others ? If not you can s/mostly //.

It's getting better, but a few of the driver callbacks used by the fb
helper are still in the crtc helper function tables. And there's the fb
helper private way to safe/restore gamma tables. But at least semantically
there's no dependency any longer after these patches I think.

> 
> >   * crtc helper functions used by many drivers to implement the kernel mode
> >   * setting interfaces.
> > + *
> > + * Initialization is done as a three-step process with
> > + * drm_fb_helper_init(), drm_fb_helper_single_add_all_connectors() and
> > + * drm_fb_helper_initial_config(). Drivers with fancier requirements than
> > + * the default beheviour can override the second step with their own code.
> > + * Teardown is done with drm_fb_helper_fini().
> > + *
> > + * At runtime drivers should restore the fbdev console by calling
> > + * drm_fb_helper_restore_fbdev_mode() from their ->lastclose callback. They
> > + * can also notify the fb helper code from updates to the output
> 
> Is it "can" or "must" ? If "can", in what conditions should they call 
> drm_fb_helper_restore_fbdev_mode() and in what conditions shouldn't they ?

I've figured that hpd support is optional, hence no "must". I've opted for
a should instead. Also added a bit of text to suggest a good place to put
this call.

> 
> > + * configuration by calling drm_fb_helper_hotplug_event().
> > + *
> > + * All other functions exported by the fb helper library can be used to
> > + * implement the fbdev driver interface by the driver.
> >   */
> > 
> > -/* simple single crtc case helper function */
> > +/**
> > + * drm_fb_helper_single_add_all_connectors() - add all connectors to fbdev
> > + * 					       emulation helper
> > + * @fb_helper: fbdev initialized with drm_fb_helper_init
> > + *
> > + * This functions adds all the available connectors for use with the given
> > + * fb_helper. This is a separate step to allow drivers to freely assign or
> > + * connectors to the fbdev, e.g. if some are reserved for special purposes
> > + * not adequate to be used for the fbcon.
> > + *
> > + * Since this is part of the initial setup before the fbdev is published,
> > + * no locking is required.
> > + */
> >  int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper
> > *fb_helper)
> > {
> >  	struct drm_device *dev = fb_helper->dev;
> > @@ -163,6 +188,10 @@ static void drm_fb_helper_restore_lut_atomic(struct
> > drm_crtc *crtc) crtc->funcs->gamma_set(crtc, r_base, g_base, b_base, 0,
> > crtc->gamma_size); }
> > 
> > +/**
> > + * drm_fb_helper_debug_enter - implementation for ->fb_debug_enter
> > + * @info: fbdev registered by the helper.
> > + */
> >  int drm_fb_helper_debug_enter(struct fb_info *info)
> >  {
> >  	struct drm_fb_helper *helper = info->par;
> > @@ -208,6 +237,10 @@ static struct drm_framebuffer
> > *drm_mode_config_fb(struct drm_crtc *crtc) return NULL;
> >  }
> > 
> > +/**
> > + * drm_fb_helper_debug_leave - implementation for ->fb_debug_leave
> > + * @info: fbdev registered by the helper.
> > + */
> >  int drm_fb_helper_debug_leave(struct fb_info *info)
> >  {
> >  	struct drm_fb_helper *helper = info->par;
> > @@ -243,9 +276,9 @@ EXPORT_SYMBOL(drm_fb_helper_debug_leave);
> >   * drm_fb_helper_restore_fbdev_mode - restore fbdev configuration
> >   * @fb_helper: fbcon to restore
> >   *
> > - * This should be called from driver's drm->lastclose callback when
> > - * implementing an fbcon on top of kms using this helper. This ensures that
> > - * the user isn't greeted with a black screen when e.g. X dies.
> > + * This should be called from driver's drm <code>->lastclose</code>
> 
> The resulting HTML is
> 
> &lt;code&gt;-&gt;lastclose&lt;/code&gt;
> 
> I'm not sure that's what you want :-)

Nope, killed them all.

> 
> > + * callback when implementing an fbcon on top of kms using this helper.
> > + * This ensures that the user isn't greeted with a black screen when e.g.
> > + * X dies.
> >   */
> >  bool drm_fb_helper_restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> >  {
> > @@ -378,6 +411,11 @@ static void drm_fb_helper_dpms(struct fb_info *info,
> > int dpms_mode) drm_modeset_unlock_all(dev);
> >  }
> > 
> > +/**
> > + * drm_fb_helper_blank - implementation for ->fb_blank
> > + * @blank: desired blanking state
> > + * @info: fbdev registered by the helper.
> 
> Nitpicking here, shouldn't you either use a full stop at the end of each 
> argument, or no full stop at all (this applies to the other functions as well) 
> ?

Copy&pasta, full stop dropped everywhere.

> 
> > + */
> >  int drm_fb_helper_blank(int blank, struct fb_info *info)
> >  {
> >  	switch (blank) {
> > @@ -421,6 +459,24 @@ static void drm_fb_helper_crtc_free(struct
> > drm_fb_helper *helper) kfree(helper->crtc_info);
> >  }
> > 
> > +/**
> > + * drm_fb_helper_init - initialize a drm_fb_helper structure
> > + * @dev: drm device
> > + * @fb_helper: driver-allocated fbdev helper structure to initialize
> > + * @crtc_count: crtc count
> > + * @max_conn_count: max connector count
> > + *
> > + * This allocates the structures for the fbdev helper with the given
> > + * limits. Note that this won't yet touch the hw (through the driver
> 
> s/hw/hardware/ ?
> 
> It might be a good idea to add a small description of the crtc_count 
> parameter.

Done. All the other corrections below I've also applied.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch