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

2013-02-12 Thread 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.pinch...@ideasonboard.com
 Signed-off-by: Daniel Vetter daniel.vet...@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.pinch...@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

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

2013-02-05 Thread 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.vet...@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)
 titlefbdev 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
 titleDisplay 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

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

2013-02-01 Thread 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.vet...@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)
titlefbdev 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
titleDisplay 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