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

[Daniel Vetter <daniel@ffwll.ch>]

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