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@xxxxxxxx>
> >
> > 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
>
> <code>->lastclose</code>
>
> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
