| Message ID | 1351777518-11212-2-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Nov 1, 2012 at 2:45 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > - Add the missing doc for drm_helper_move_panel_connectors_to_head. > - Fixup any outdated stuff in existing sections. I've only looked at > those kerneldoc headers that actually resulted in a complaint from > the kerneldoc parser tool. > > v2: > - Actually include the docbook snippet in the right patch. > - Fix spelling fail. > > v3: It's now called drm_crtc_helper_set_mode, spotted by Laurent > Pinchart. > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > Documentation/DocBook/drm.tmpl | 4 +++ > drivers/gpu/drm/drm_crtc_helper.c | 66 +++++++++++++++++++++++++-------------- > 2 files changed, 46 insertions(+), 24 deletions(-) > > diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl > index 270bc12..c2b31b9 100644 > --- a/Documentation/DocBook/drm.tmpl > +++ b/Documentation/DocBook/drm.tmpl > @@ -2106,6 +2106,10 @@ void intel_crt_init(struct drm_device *dev) > </listitem> > </itemizedlist> > </sect2> > + <sect2> > + <title>Modeset Helper Functions Reference</title> > +!Edrivers/gpu/drm/drm_crtc_helper.c > + </sect2> > </sect1> > > <!-- Internals: vertical blanking --> > diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c > index 7105168..2a7a886 100644 > --- a/drivers/gpu/drm/drm_crtc_helper.c > +++ b/drivers/gpu/drm/drm_crtc_helper.c > @@ -39,6 +39,17 @@ > #include <drm/drm_fb_helper.h> > #include <drm/drm_edid.h> > > +/** > + * drm_helper_move_panel_connectors_to_head() - move panels to the front in the > + * connector list > + * @dev: drm device to operate on > + * > + * Some userspace presumes that the first connected connector is the main > + * display, where it's supposed to display e.g. the login screen. For > + * laptops, this should be the main panel. Use this function to sort all > + * (eDP/LVDS) panels to the front of the connector list, instead of > + * painstakingly trying to initialize them in the right order. > + */ > void drm_helper_move_panel_connectors_to_head(struct drm_device *dev) > { > struct drm_connector *connector, *tmp; > @@ -82,22 +93,21 @@ static void drm_mode_validate_flag(struct drm_connector *connector, > > /** > * drm_helper_probe_single_connector_modes - get complete set of display modes > - * @dev: DRM device > + * @connector: connector to probe > * @maxX: max width for modes > * @maxY: max height for modes > * > * LOCKING: > * Caller must hold mode config lock. > * > - * Based on @dev's mode_config layout, scan all the connectors and try to detect > - * modes on them. Modes will first be added to the connector's probed_modes > - * list, then culled (based on validity and the @maxX, @maxY parameters) and > - * put into the normal modes list. > - * > - * Intended to be used either at bootup time or when major configuration > - * changes have occurred. > + * Based on the helper callbacks implemented by @connector try to detect all > + * valid modes. Modes will first be added to the connector's probed_modes list, > + * then culled (based on validity and the @maxX, @maxY parameters) and put into > + * the normal modes list. > * > - * FIXME: take into account monitor limits Should this really be removed? I'm guess it has been fixed or is not really needed anymore but just making sure it shouldn't live somewhere else. Cheers, Jakob.
On Thu, 1 Nov 2012 15:49:16 +0100, Jakob Bornecrantz <wallbraker@gmail.com> wrote: > On Thu, Nov 1, 2012 at 2:45 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > - Add the missing doc for drm_helper_move_panel_connectors_to_head. > > - Fixup any outdated stuff in existing sections. I've only looked at > > those kerneldoc headers that actually resulted in a complaint from > > the kerneldoc parser tool. > > > > v2: > > - Actually include the docbook snippet in the right patch. > > - Fix spelling fail. > > > > v3: It's now called drm_crtc_helper_set_mode, spotted by Laurent > > Pinchart. > > > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > --- > > Documentation/DocBook/drm.tmpl | 4 +++ > > drivers/gpu/drm/drm_crtc_helper.c | 66 +++++++++++++++++++++++++-------------- > > 2 files changed, 46 insertions(+), 24 deletions(-) > > > > diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl > > index 270bc12..c2b31b9 100644 > > --- a/Documentation/DocBook/drm.tmpl > > +++ b/Documentation/DocBook/drm.tmpl > > @@ -2106,6 +2106,10 @@ void intel_crt_init(struct drm_device *dev) > > </listitem> > > </itemizedlist> > > </sect2> > > + <sect2> > > + <title>Modeset Helper Functions Reference</title> > > +!Edrivers/gpu/drm/drm_crtc_helper.c > > + </sect2> > > </sect1> > > > > <!-- Internals: vertical blanking --> > > diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c > > index 7105168..2a7a886 100644 > > --- a/drivers/gpu/drm/drm_crtc_helper.c > > +++ b/drivers/gpu/drm/drm_crtc_helper.c > > @@ -39,6 +39,17 @@ > > #include <drm/drm_fb_helper.h> > > #include <drm/drm_edid.h> > > > > +/** > > + * drm_helper_move_panel_connectors_to_head() - move panels to the front in the > > + * connector list > > + * @dev: drm device to operate on > > + * > > + * Some userspace presumes that the first connected connector is the main > > + * display, where it's supposed to display e.g. the login screen. For > > + * laptops, this should be the main panel. Use this function to sort all > > + * (eDP/LVDS) panels to the front of the connector list, instead of > > + * painstakingly trying to initialize them in the right order. > > + */ > > void drm_helper_move_panel_connectors_to_head(struct drm_device *dev) > > { > > struct drm_connector *connector, *tmp; > > @@ -82,22 +93,21 @@ static void drm_mode_validate_flag(struct drm_connector *connector, > > > > /** > > * drm_helper_probe_single_connector_modes - get complete set of display modes > > - * @dev: DRM device > > + * @connector: connector to probe > > * @maxX: max width for modes > > * @maxY: max height for modes > > * > > * LOCKING: > > * Caller must hold mode config lock. > > * > > - * Based on @dev's mode_config layout, scan all the connectors and try to detect > > - * modes on them. Modes will first be added to the connector's probed_modes > > - * list, then culled (based on validity and the @maxX, @maxY parameters) and > > - * put into the normal modes list. > > - * > > - * Intended to be used either at bootup time or when major configuration > > - * changes have occurred. > > + * Based on the helper callbacks implemented by @connector try to detect all > > + * valid modes. Modes will first be added to the connector's probed_modes list, > > + * then culled (based on validity and the @maxX, @maxY parameters) and put into > > + * the normal modes list. > > * > > - * FIXME: take into account monitor limits > > Should this really be removed? I'm guess it has been fixed or is not > really needed anymore but just making sure it shouldn't live somewhere > else. Yes, the modes from an EDID are validated against the limits given by the EDID, which is as much information as we have. Higher levels can filter the modes based on alternate sources, ofc. Granted removing a FIXME should be a separate commit giving good reason why it can be considered fixed. (i.e. treat it like the bug it warns about of its own right.) -Chris
On Thu, Nov 01, 2012 at 03:06:44PM +0000, Chris Wilson wrote: > On Thu, 1 Nov 2012 15:49:16 +0100, Jakob Bornecrantz <wallbraker@gmail.com> wrote: > > On Thu, Nov 1, 2012 at 2:45 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > - Add the missing doc for drm_helper_move_panel_connectors_to_head. > > > - Fixup any outdated stuff in existing sections. I've only looked at > > > those kerneldoc headers that actually resulted in a complaint from > > > the kerneldoc parser tool. > > > > > > v2: > > > - Actually include the docbook snippet in the right patch. > > > - Fix spelling fail. > > > > > > v3: It's now called drm_crtc_helper_set_mode, spotted by Laurent > > > Pinchart. > > > > > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > --- > > > Documentation/DocBook/drm.tmpl | 4 +++ > > > drivers/gpu/drm/drm_crtc_helper.c | 66 +++++++++++++++++++++++++-------------- > > > 2 files changed, 46 insertions(+), 24 deletions(-) > > > > > > diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl > > > index 270bc12..c2b31b9 100644 > > > --- a/Documentation/DocBook/drm.tmpl > > > +++ b/Documentation/DocBook/drm.tmpl > > > @@ -2106,6 +2106,10 @@ void intel_crt_init(struct drm_device *dev) > > > </listitem> > > > </itemizedlist> > > > </sect2> > > > + <sect2> > > > + <title>Modeset Helper Functions Reference</title> > > > +!Edrivers/gpu/drm/drm_crtc_helper.c > > > + </sect2> > > > </sect1> > > > > > > <!-- Internals: vertical blanking --> > > > diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c > > > index 7105168..2a7a886 100644 > > > --- a/drivers/gpu/drm/drm_crtc_helper.c > > > +++ b/drivers/gpu/drm/drm_crtc_helper.c > > > @@ -39,6 +39,17 @@ > > > #include <drm/drm_fb_helper.h> > > > #include <drm/drm_edid.h> > > > > > > +/** > > > + * drm_helper_move_panel_connectors_to_head() - move panels to the front in the > > > + * connector list > > > + * @dev: drm device to operate on > > > + * > > > + * Some userspace presumes that the first connected connector is the main > > > + * display, where it's supposed to display e.g. the login screen. For > > > + * laptops, this should be the main panel. Use this function to sort all > > > + * (eDP/LVDS) panels to the front of the connector list, instead of > > > + * painstakingly trying to initialize them in the right order. > > > + */ > > > void drm_helper_move_panel_connectors_to_head(struct drm_device *dev) > > > { > > > struct drm_connector *connector, *tmp; > > > @@ -82,22 +93,21 @@ static void drm_mode_validate_flag(struct drm_connector *connector, > > > > > > /** > > > * drm_helper_probe_single_connector_modes - get complete set of display modes > > > - * @dev: DRM device > > > + * @connector: connector to probe > > > * @maxX: max width for modes > > > * @maxY: max height for modes > > > * > > > * LOCKING: > > > * Caller must hold mode config lock. > > > * > > > - * Based on @dev's mode_config layout, scan all the connectors and try to detect > > > - * modes on them. Modes will first be added to the connector's probed_modes > > > - * list, then culled (based on validity and the @maxX, @maxY parameters) and > > > - * put into the normal modes list. > > > - * > > > - * Intended to be used either at bootup time or when major configuration > > > - * changes have occurred. > > > + * Based on the helper callbacks implemented by @connector try to detect all > > > + * valid modes. Modes will first be added to the connector's probed_modes list, > > > + * then culled (based on validity and the @maxX, @maxY parameters) and put into > > > + * the normal modes list. > > > * > > > - * FIXME: take into account monitor limits > > > > Should this really be removed? I'm guess it has been fixed or is not > > really needed anymore but just making sure it shouldn't live somewhere > > else. > > Yes, the modes from an EDID are validated against the limits given by the > EDID, which is as much information as we have. Higher levels can filter > the modes based on alternate sources, ofc. > > Granted removing a FIXME should be a separate commit giving good reason > why it can be considered fixed. (i.e. treat it like the bug it warns > about of its own right.) We now call down into the connectors ->mode_valid callback. I've figured that's about as much checking the helper function here should concern itself with. Now ofc, most drivers don't bother to trouble themselves with dotclock limits, but imo that's not a FIXME for the helper code. And no, adding another common check is imo not the solution, since doing the full mode checking requires the global configuration anyway ... I guess I get bad marks for failing to mention that in the commit message ;-) Cheers, Daniel
diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl index 270bc12..c2b31b9 100644 --- a/Documentation/DocBook/drm.tmpl +++ b/Documentation/DocBook/drm.tmpl @@ -2106,6 +2106,10 @@ void intel_crt_init(struct drm_device *dev) </listitem> </itemizedlist> </sect2> + <sect2> + <title>Modeset Helper Functions Reference</title> +!Edrivers/gpu/drm/drm_crtc_helper.c + </sect2> </sect1> <!-- Internals: vertical blanking --> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 7105168..2a7a886 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -39,6 +39,17 @@ #include <drm/drm_fb_helper.h> #include <drm/drm_edid.h> +/** + * drm_helper_move_panel_connectors_to_head() - move panels to the front in the + * connector list + * @dev: drm device to operate on + * + * Some userspace presumes that the first connected connector is the main + * display, where it's supposed to display e.g. the login screen. For + * laptops, this should be the main panel. Use this function to sort all + * (eDP/LVDS) panels to the front of the connector list, instead of + * painstakingly trying to initialize them in the right order. + */ void drm_helper_move_panel_connectors_to_head(struct drm_device *dev) { struct drm_connector *connector, *tmp; @@ -82,22 +93,21 @@ static void drm_mode_validate_flag(struct drm_connector *connector, /** * drm_helper_probe_single_connector_modes - get complete set of display modes - * @dev: DRM device + * @connector: connector to probe * @maxX: max width for modes * @maxY: max height for modes * * LOCKING: * Caller must hold mode config lock. * - * Based on @dev's mode_config layout, scan all the connectors and try to detect - * modes on them. Modes will first be added to the connector's probed_modes - * list, then culled (based on validity and the @maxX, @maxY parameters) and - * put into the normal modes list. - * - * Intended to be used either at bootup time or when major configuration - * changes have occurred. + * Based on the helper callbacks implemented by @connector try to detect all + * valid modes. Modes will first be added to the connector's probed_modes list, + * then culled (based on validity and the @maxX, @maxY parameters) and put into + * the normal modes list. * - * FIXME: take into account monitor limits + * Intended to be use as a generic implementation of the ->probe() @connector + * callback for drivers that use the crtc helpers for output mode filtering and + * detection. * * RETURNS: * Number of modes found on @connector. @@ -343,17 +353,24 @@ drm_crtc_prepare_encoders(struct drm_device *dev) } /** - * drm_crtc_set_mode - set a mode + * drm_crtc_helper_set_mode - internal helper to set a mode * @crtc: CRTC to program * @mode: mode to use * @x: width of mode * @y: height of mode + * @old_fb: old framebuffer, for cleanup * * LOCKING: * Caller must hold mode config lock. * * Try to set @mode on @crtc. Give @crtc and its associated connectors a chance - * to fixup or reject the mode prior to trying to set it. + * to fixup or reject the mode prior to trying to set it. This is an internal + * helper that drivers could e.g. use to update properties that require the + * entire output pipe to be disabled and re-enabled in a new configuration. For + * example for changing whether audio is enabled on a hdmi link or for changing + * panel fitter or dither attributes. It is also called by the + * drm_crtc_helper_set_config() helper function to drive the mode setting + * sequence. * * RETURNS: * True if the mode was set successfully, or false otherwise. @@ -509,20 +526,19 @@ drm_crtc_helper_disable(struct drm_crtc *crtc) /** * drm_crtc_helper_set_config - set a new config from userspace - * @crtc: CRTC to setup - * @crtc_info: user provided configuration - * @new_mode: new mode to set - * @connector_set: set of connectors for the new config - * @fb: new framebuffer + * @set: mode set configuration * * LOCKING: * Caller must hold mode config lock. * - * Setup a new configuration, provided by the user in @crtc_info, and enable - * it. + * Setup a new configuration, provided by the upper layers (either an ioctl call + * from userspace or internally e.g. from the fbdev suppport code) in @set, and + * enable it. This is the main helper functions for drivers that implement + * kernel mode setting with the crtc helper functions and the assorted + * ->prepare(), ->modeset() and ->commit() helper callbacks. * * RETURNS: - * Zero. (FIXME) + * Returns 0 on success, -ERRNO on failure. */ int drm_crtc_helper_set_config(struct drm_mode_set *set) { @@ -818,12 +834,14 @@ static int drm_helper_choose_crtc_dpms(struct drm_crtc *crtc) } /** - * drm_helper_connector_dpms - * @connector affected connector - * @mode DPMS mode + * drm_helper_connector_dpms() - connector dpms helper implementation + * @connector: affected connector + * @mode: DPMS mode * - * Calls the low-level connector DPMS function, then - * calls appropriate encoder and crtc DPMS functions as well + * This is the main helper function provided by the crtc helper framework for + * implementing the DPMS connector attribute. It computes the new desired DPMS + * state for all encoders and crtcs in the output mesh and calls the ->dpms() + * callback provided by the driver appropriately. */ void drm_helper_connector_dpms(struct drm_connector *connector, int mode) {