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