| Message ID | 1449218769-16577-7-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/04/2015 02:15 PM, Daniel Vetter wrote: > Especially document the assumptions and semantics of the callbacks > carefully. Just a warm-up excercise really. > > v2: Spelling fixes (Eric). > > v3: Consolidate more with existing docs: > > - Remove the overview section explaining the bridge funcs, that's > now all in the drm_bridge_funcs kerneldoc in much more detail. > > - Use & to reference structs so that kerneldoc automatically inserts > hyperlinks. Reviewed-by: Archit Taneja <architt@codeaurora.org> > > Cc: Eric Anholt <eric@anholt.net> > Cc: Archit Taneja <architt@codeaurora.org> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/drm_bridge.c | 69 +++++++++++------------------ > include/drm/drm_crtc.h | 102 ++++++++++++++++++++++++++++++++++++++++--- > 2 files changed, 122 insertions(+), 49 deletions(-) > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index 6b8f7211e543..26dd1cfb6078 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -31,14 +31,14 @@ > /** > * DOC: overview > * > - * drm_bridge represents a device that hangs on to an encoder. These are handy > - * when a regular drm_encoder entity isn't enough to represent the entire > + * struct &drm_bridge represents a device that hangs on to an encoder. These are > + * handy when a regular &drm_encoder entity isn't enough to represent the entire > * encoder chain. > * > - * A bridge is always associated to a single drm_encoder at a time, but can be > + * A bridge is always associated to a single &drm_encoder at a time, but can be > * either connected to it directly, or through an intermediate bridge: > * > - * encoder ---> bridge B ---> bridge A > + * encoder ---> bridge B ---> bridge A > * > * Here, the output of the encoder feeds to bridge B, and that furthers feeds to > * bridge A. > @@ -46,11 +46,16 @@ > * The driver using the bridge is responsible to make the associations between > * the encoder and bridges. Once these links are made, the bridges will > * participate along with encoder functions to perform mode_set/enable/disable > - * through the ops provided in drm_bridge_funcs. > + * through the ops provided in &drm_bridge_funcs. > * > * drm_bridge, like drm_panel, aren't drm_mode_object entities like planes, > - * crtcs, encoders or connectors. They just provide additional hooks to get the > - * desired output at the end of the encoder chain. > + * crtcs, encoders or connectors and hence are not visible to userspace. They > + * just provide additional hooks to get the desired output at the end of the > + * encoder chain. > + * > + * Bridges can also be chained up using the next pointer in struct &drm_bridge. > + * > + * Both legacy CRTC helpers and the new atomic modeset helpers support bridges. > */ > > static DEFINE_MUTEX(bridge_lock); > @@ -122,34 +127,12 @@ EXPORT_SYMBOL(drm_bridge_attach); > /** > * DOC: bridge callbacks > * > - * The drm_bridge_funcs ops are populated by the bridge driver. The drm > - * internals(atomic and crtc helpers) use the helpers defined in drm_bridge.c > - * These helpers call a specific drm_bridge_funcs op for all the bridges > + * The &drm_bridge_funcs ops are populated by the bridge driver. The drm > + * internals (atomic and crtc helpers) use the helpers defined in drm_bridge.c > + * These helpers call a specific &drm_bridge_funcs op for all the bridges > * during encoder configuration. > * > - * When creating a bridge driver, one can implement drm_bridge_funcs op with > - * the help of these rough rules: > - * > - * pre_enable: this contains things needed to be done for the bridge before > - * its clock and timings are enabled by its source. For a bridge, its source > - * is generally the encoder or bridge just before it in the encoder chain. > - * > - * enable: this contains things needed to be done for the bridge once its > - * source is enabled. In other words, enable is called once the source is > - * ready with clock and timing needed by the bridge. > - * > - * disable: this contains things needed to be done for the bridge assuming > - * that its source is still enabled, i.e. clock and timings are still on. > - * > - * post_disable: this contains things needed to be done for the bridge once > - * its source is disabled, i.e. once clocks and timings are off. > - * > - * mode_fixup: this should fixup the given mode for the bridge. It is called > - * after the encoder's mode fixup. mode_fixup can also reject a mode completely > - * if it's unsuitable for the hardware. > - * > - * mode_set: this sets up the mode for the bridge. It assumes that its source > - * (an encoder or a bridge) has set the mode too. > + * For detailed specification of the bridge callbacks see &drm_bridge_funcs. > */ > > /** > @@ -159,7 +142,7 @@ EXPORT_SYMBOL(drm_bridge_attach); > * @mode: desired mode to be set for the bridge > * @adjusted_mode: updated mode that works for this bridge > * > - * Calls 'mode_fixup' drm_bridge_funcs op for all the bridges in the > + * Calls 'mode_fixup' &drm_bridge_funcs op for all the bridges in the > * encoder chain, starting from the first bridge to the last. > * > * Note: the bridge passed should be the one closest to the encoder > @@ -186,11 +169,11 @@ bool drm_bridge_mode_fixup(struct drm_bridge *bridge, > EXPORT_SYMBOL(drm_bridge_mode_fixup); > > /** > - * drm_bridge_disable - calls 'disable' drm_bridge_funcs op for all > + * drm_bridge_disable - calls 'disable' &drm_bridge_funcs op for all > * bridges in the encoder chain. > * @bridge: bridge control structure > * > - * Calls 'disable' drm_bridge_funcs op for all the bridges in the encoder > + * Calls 'disable' &drm_bridge_funcs op for all the bridges in the encoder > * chain, starting from the last bridge to the first. These are called before > * calling the encoder's prepare op. > * > @@ -208,11 +191,11 @@ void drm_bridge_disable(struct drm_bridge *bridge) > EXPORT_SYMBOL(drm_bridge_disable); > > /** > - * drm_bridge_post_disable - calls 'post_disable' drm_bridge_funcs op for > + * drm_bridge_post_disable - calls 'post_disable' &drm_bridge_funcs op for > * all bridges in the encoder chain. > * @bridge: bridge control structure > * > - * Calls 'post_disable' drm_bridge_funcs op for all the bridges in the > + * Calls 'post_disable' &drm_bridge_funcs op for all the bridges in the > * encoder chain, starting from the first bridge to the last. These are called > * after completing the encoder's prepare op. > * > @@ -236,7 +219,7 @@ EXPORT_SYMBOL(drm_bridge_post_disable); > * @mode: desired mode to be set for the bridge > * @adjusted_mode: updated mode that works for this bridge > * > - * Calls 'mode_set' drm_bridge_funcs op for all the bridges in the > + * Calls 'mode_set' &drm_bridge_funcs op for all the bridges in the > * encoder chain, starting from the first bridge to the last. > * > * Note: the bridge passed should be the one closest to the encoder > @@ -256,11 +239,11 @@ void drm_bridge_mode_set(struct drm_bridge *bridge, > EXPORT_SYMBOL(drm_bridge_mode_set); > > /** > - * drm_bridge_pre_enable - calls 'pre_enable' drm_bridge_funcs op for all > + * drm_bridge_pre_enable - calls 'pre_enable' &drm_bridge_funcs op for all > * bridges in the encoder chain. > * @bridge: bridge control structure > * > - * Calls 'pre_enable' drm_bridge_funcs op for all the bridges in the encoder > + * Calls 'pre_enable' &drm_bridge_funcs op for all the bridges in the encoder > * chain, starting from the last bridge to the first. These are called > * before calling the encoder's commit op. > * > @@ -278,11 +261,11 @@ void drm_bridge_pre_enable(struct drm_bridge *bridge) > EXPORT_SYMBOL(drm_bridge_pre_enable); > > /** > - * drm_bridge_enable - calls 'enable' drm_bridge_funcs op for all bridges > + * drm_bridge_enable - calls 'enable' &drm_bridge_funcs op for all bridges > * in the encoder chain. > * @bridge: bridge control structure > * > - * Calls 'enable' drm_bridge_funcs op for all the bridges in the encoder > + * Calls 'enable' &drm_bridge_funcs op for all the bridges in the encoder > * chain, starting from the first bridge to the last. These are called > * after completing the encoder's commit op. > * > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index e8f8e4e9a6a1..ec3632e62b3f 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -883,24 +883,114 @@ struct drm_plane { > /** > * struct drm_bridge_funcs - drm_bridge control functions > * @attach: Called during drm_bridge_attach > - * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge > - * @disable: Called right before encoder prepare, disables the bridge > - * @post_disable: Called right after encoder prepare, for lockstepped disable > - * @mode_set: Set this mode to the bridge > - * @pre_enable: Called right before encoder commit, for lockstepped commit > - * @enable: Called right after encoder commit, enables the bridge > */ > struct drm_bridge_funcs { > int (*attach)(struct drm_bridge *bridge); > + > + /** > + * @mode_fixup: > + * > + * This callback is used to validate and adjust a mode. The paramater > + * mode is the display mode that should be fed to the next element in > + * the display chain, either the final &drm_connector or the next > + * &drm_bridge. The parameter adjusted_mode is the input mode the bridge > + * requires. It can be modified by this callback and does not need to > + * match mode. > + * > + * This is the only hook that allows a bridge to reject a modeset. If > + * this function passes all other callbacks must succeed for this > + * configuration. > + * > + * NOTE: > + * > + * This function is called in the check phase of atomic modesets, which > + * can be aborted for any reason (including on userspace's request to > + * just check whether a configuration would be possible). Drivers MUST > + * NOT touch any persistent state (hardware or software) or data > + * structures except the passed in adjusted_mode parameter. > + * > + * RETURNS: > + * > + * True if an acceptable configuration is possible, false if the modeset > + * operation should be rejected. > + */ > bool (*mode_fixup)(struct drm_bridge *bridge, > const struct drm_display_mode *mode, > struct drm_display_mode *adjusted_mode); > + /** > + * @disable: > + * > + * This callback should disable the bridge. It is called right before > + * the preceding element in the display pipe is disabled. If the > + * preceding element is a bridge this means it's called before that > + * bridge's ->disaable function. If the preceding element is a > + * &drm_encoder it's called right before the encoder's ->disable, > + * ->prepare or ->dpms hook from struct &drm_encoder_helper_funcs. > + * > + * The bridge can assume that the display pipe (i.e. clocks and timing > + * signals) feeding it is still running when this callback is called. > + */ > void (*disable)(struct drm_bridge *bridge); > + > + /** > + * @post_disable: > + * > + * This callback should disable the bridge. It is called right after > + * the preceding element in the display pipe is disabled. If the > + * preceding element is a bridge this means it's called after that > + * bridge's ->post_disaable function. If the preceding element is a > + * &drm_encoder it's called right after the encoder's ->disable, > + * ->prepare or ->dpms hook from struct &drm_encoder_helper_funcs. > + * > + * The bridge must assume that the display pipe (i.e. clocks and timing > + * singals) feeding it is no longer running when this callback is > + * called. > + */ > void (*post_disable)(struct drm_bridge *bridge); > + > + /** > + * @mode_set: > + * > + * This callback should set the given mode on the bridge. It is called > + * after the ->mode_set callback for the preceding element in the > + * display pipeline has been called already. The display pipe (i.e. > + * clocks and timing signals) is off when this function is called. > + */ > void (*mode_set)(struct drm_bridge *bridge, > struct drm_display_mode *mode, > struct drm_display_mode *adjusted_mode); > + /** > + * @pre_enable: > + * > + * This callback should enable the bridge. It is called right before > + * the preceding element in the display pipe is enabled. If the > + * preceding element is a bridge this means it's called before that > + * bridge's ->pre_enable function. If the preceding element is a > + * &drm_encoder it's called right before the encoder's ->enable, > + * ->commit or ->dpms hook from struct &drm_encoder_helper_funcs. > + * > + * The display pipe (i.e. clocks and timing signals) feeding this bridge > + * will not yet be running when this callback is called. The bridge must > + * not enable the display link feeding the next bridge in the chain (if > + * there is one) when this callback is called. > + */ > void (*pre_enable)(struct drm_bridge *bridge); > + > + /** > + * @enable: > + * > + * This callback should enable the bridge. It is called right after > + * the preceding element in the display pipe is enabled. If the > + * preceding element is a bridge this means it's called after that > + * bridge's ->enable function. If the preceding element is a > + * &drm_encoder it's called right after the encoder's ->enable, > + * ->commit or ->dpms hook from struct &drm_encoder_helper_funcs. > + * > + * The bridge can assume that the display pipe (i.e. clocks and timing > + * signals) feeding it is running when this callback is called. This > + * callback must enable the display link feeding the next bridge in the > + * chain if there is one. > + */ > void (*enable)(struct drm_bridge *bridge); > }; > >
On Fri, Dec 04, 2015 at 09:45:47AM +0100, Daniel Vetter wrote: > Especially document the assumptions and semantics of the callbacks > carefully. Just a warm-up excercise really. > > v2: Spelling fixes (Eric). > > v3: Consolidate more with existing docs: > > - Remove the overview section explaining the bridge funcs, that's > now all in the drm_bridge_funcs kerneldoc in much more detail. > > - Use & to reference structs so that kerneldoc automatically inserts > hyperlinks. > > Cc: Eric Anholt <eric@anholt.net> > Cc: Archit Taneja <architt@codeaurora.org> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/drm_bridge.c | 69 +++++++++++------------------ > include/drm/drm_crtc.h | 102 ++++++++++++++++++++++++++++++++++++++++--- > 2 files changed, 122 insertions(+), 49 deletions(-) > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index 6b8f7211e543..26dd1cfb6078 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -31,14 +31,14 @@ > /** > * DOC: overview > * > - * drm_bridge represents a device that hangs on to an encoder. These are handy > - * when a regular drm_encoder entity isn't enough to represent the entire > + * struct &drm_bridge represents a device that hangs on to an encoder. These are > + * handy when a regular &drm_encoder entity isn't enough to represent the entire > * encoder chain. > * > - * A bridge is always associated to a single drm_encoder at a time, but can be > + * A bridge is always associated to a single &drm_encoder at a time, but can be I think I've seen either "associated with" or "attached to", but the above sounds strange to me. Anyway, it's nothing you've introduced in this patch, so might as well stay the way it is. > * either connected to it directly, or through an intermediate bridge: > * > - * encoder ---> bridge B ---> bridge A > + * encoder ---> bridge B ---> bridge A > * > * Here, the output of the encoder feeds to bridge B, and that furthers feeds to > * bridge A. > @@ -46,11 +46,16 @@ > * The driver using the bridge is responsible to make the associations between > * the encoder and bridges. Once these links are made, the bridges will > * participate along with encoder functions to perform mode_set/enable/disable > - * through the ops provided in drm_bridge_funcs. > + * through the ops provided in &drm_bridge_funcs. > * > * drm_bridge, like drm_panel, aren't drm_mode_object entities like planes, > - * crtcs, encoders or connectors. They just provide additional hooks to get the > - * desired output at the end of the encoder chain. > + * crtcs, encoders or connectors and hence are not visible to userspace. They s/crtcs/CRTCs/ > @@ -122,34 +127,12 @@ EXPORT_SYMBOL(drm_bridge_attach); > /** > * DOC: bridge callbacks > * > - * The drm_bridge_funcs ops are populated by the bridge driver. The drm > - * internals(atomic and crtc helpers) use the helpers defined in drm_bridge.c > - * These helpers call a specific drm_bridge_funcs op for all the bridges > + * The &drm_bridge_funcs ops are populated by the bridge driver. The drm s/drm/DRM/ > + * internals (atomic and crtc helpers) use the helpers defined in drm_bridge.c s/crtc/CRTC/ > /** > @@ -159,7 +142,7 @@ EXPORT_SYMBOL(drm_bridge_attach); > * @mode: desired mode to be set for the bridge > * @adjusted_mode: updated mode that works for this bridge > * > - * Calls 'mode_fixup' drm_bridge_funcs op for all the bridges in the > + * Calls 'mode_fixup' &drm_bridge_funcs op for all the bridges in the "->mode_fixup()"? > @@ -186,11 +169,11 @@ bool drm_bridge_mode_fixup(struct drm_bridge *bridge, > EXPORT_SYMBOL(drm_bridge_mode_fixup); > > /** > - * drm_bridge_disable - calls 'disable' drm_bridge_funcs op for all > + * drm_bridge_disable - calls 'disable' &drm_bridge_funcs op for all > * bridges in the encoder chain. > * @bridge: bridge control structure > * > - * Calls 'disable' drm_bridge_funcs op for all the bridges in the encoder > + * Calls 'disable' &drm_bridge_funcs op for all the bridges in the encoder "->disable()"? Perhaps not worth it because there's a bunch of these in this file. > struct drm_bridge_funcs { > int (*attach)(struct drm_bridge *bridge); > + > + /** > + * @mode_fixup: > + * > + * This callback is used to validate and adjust a mode. The paramater > + * mode is the display mode that should be fed to the next element in > + * the display chain, either the final &drm_connector or the next > + * &drm_bridge. The parameter adjusted_mode is the input mode the bridge > + * requires. It can be modified by this callback and does not need to > + * match mode. > + * > + * This is the only hook that allows a bridge to reject a modeset. If > + * this function passes all other callbacks must succeed for this > + * configuration. > + * > + * NOTE: > + * > + * This function is called in the check phase of atomic modesets, which > + * can be aborted for any reason (including on userspace's request to > + * just check whether a configuration would be possible). Drivers MUST > + * NOT touch any persistent state (hardware or software) or data > + * structures except the passed in adjusted_mode parameter. > + * > + * RETURNS: > + * > + * True if an acceptable configuration is possible, false if the modeset > + * operation should be rejected. > + */ > bool (*mode_fixup)(struct drm_bridge *bridge, > const struct drm_display_mode *mode, > struct drm_display_mode *adjusted_mode); > + /** > + * @disable: > + * > + * This callback should disable the bridge. It is called right before > + * the preceding element in the display pipe is disabled. If the > + * preceding element is a bridge this means it's called before that > + * bridge's ->disaable function. If the preceding element is a s/->disaable/->disable()/? > + * &drm_encoder it's called right before the encoder's ->disable, > + * ->prepare or ->dpms hook from struct &drm_encoder_helper_funcs. ->disable(), ->prepare() or ->dpms()? > + * > + * The bridge can assume that the display pipe (i.e. clocks and timing > + * signals) feeding it is still running when this callback is called. > + */ > void (*disable)(struct drm_bridge *bridge); > + > + /** > + * @post_disable: > + * > + * This callback should disable the bridge. It is called right after > + * the preceding element in the display pipe is disabled. If the > + * preceding element is a bridge this means it's called after that > + * bridge's ->post_disaable function. If the preceding element is a > + * &drm_encoder it's called right after the encoder's ->disable, > + * ->prepare or ->dpms hook from struct &drm_encoder_helper_funcs. Same comments as for ->disable(). Perhaps this should also say what the difference is to ->disable()? But maybe that's more suitable for a follow-up patch. > + * > + * The bridge must assume that the display pipe (i.e. clocks and timing > + * singals) feeding it is no longer running when this callback is "signals". I guess this is the difference. Perhaps mention in the above paragraph that ->post_disable() is called after ->disable(), though that much should be obvious. Thierry
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 6b8f7211e543..26dd1cfb6078 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -31,14 +31,14 @@ /** * DOC: overview * - * drm_bridge represents a device that hangs on to an encoder. These are handy - * when a regular drm_encoder entity isn't enough to represent the entire + * struct &drm_bridge represents a device that hangs on to an encoder. These are + * handy when a regular &drm_encoder entity isn't enough to represent the entire * encoder chain. * - * A bridge is always associated to a single drm_encoder at a time, but can be + * A bridge is always associated to a single &drm_encoder at a time, but can be * either connected to it directly, or through an intermediate bridge: * - * encoder ---> bridge B ---> bridge A + * encoder ---> bridge B ---> bridge A * * Here, the output of the encoder feeds to bridge B, and that furthers feeds to * bridge A. @@ -46,11 +46,16 @@ * The driver using the bridge is responsible to make the associations between * the encoder and bridges. Once these links are made, the bridges will * participate along with encoder functions to perform mode_set/enable/disable - * through the ops provided in drm_bridge_funcs. + * through the ops provided in &drm_bridge_funcs. * * drm_bridge, like drm_panel, aren't drm_mode_object entities like planes, - * crtcs, encoders or connectors. They just provide additional hooks to get the - * desired output at the end of the encoder chain. + * crtcs, encoders or connectors and hence are not visible to userspace. They + * just provide additional hooks to get the desired output at the end of the + * encoder chain. + * + * Bridges can also be chained up using the next pointer in struct &drm_bridge. + * + * Both legacy CRTC helpers and the new atomic modeset helpers support bridges. */ static DEFINE_MUTEX(bridge_lock); @@ -122,34 +127,12 @@ EXPORT_SYMBOL(drm_bridge_attach); /** * DOC: bridge callbacks * - * The drm_bridge_funcs ops are populated by the bridge driver. The drm - * internals(atomic and crtc helpers) use the helpers defined in drm_bridge.c - * These helpers call a specific drm_bridge_funcs op for all the bridges + * The &drm_bridge_funcs ops are populated by the bridge driver. The drm + * internals (atomic and crtc helpers) use the helpers defined in drm_bridge.c + * These helpers call a specific &drm_bridge_funcs op for all the bridges * during encoder configuration. * - * When creating a bridge driver, one can implement drm_bridge_funcs op with - * the help of these rough rules: - * - * pre_enable: this contains things needed to be done for the bridge before - * its clock and timings are enabled by its source. For a bridge, its source - * is generally the encoder or bridge just before it in the encoder chain. - * - * enable: this contains things needed to be done for the bridge once its - * source is enabled. In other words, enable is called once the source is - * ready with clock and timing needed by the bridge. - * - * disable: this contains things needed to be done for the bridge assuming - * that its source is still enabled, i.e. clock and timings are still on. - * - * post_disable: this contains things needed to be done for the bridge once - * its source is disabled, i.e. once clocks and timings are off. - * - * mode_fixup: this should fixup the given mode for the bridge. It is called - * after the encoder's mode fixup. mode_fixup can also reject a mode completely - * if it's unsuitable for the hardware. - * - * mode_set: this sets up the mode for the bridge. It assumes that its source - * (an encoder or a bridge) has set the mode too. + * For detailed specification of the bridge callbacks see &drm_bridge_funcs. */ /** @@ -159,7 +142,7 @@ EXPORT_SYMBOL(drm_bridge_attach); * @mode: desired mode to be set for the bridge * @adjusted_mode: updated mode that works for this bridge * - * Calls 'mode_fixup' drm_bridge_funcs op for all the bridges in the + * Calls 'mode_fixup' &drm_bridge_funcs op for all the bridges in the * encoder chain, starting from the first bridge to the last. * * Note: the bridge passed should be the one closest to the encoder @@ -186,11 +169,11 @@ bool drm_bridge_mode_fixup(struct drm_bridge *bridge, EXPORT_SYMBOL(drm_bridge_mode_fixup); /** - * drm_bridge_disable - calls 'disable' drm_bridge_funcs op for all + * drm_bridge_disable - calls 'disable' &drm_bridge_funcs op for all * bridges in the encoder chain. * @bridge: bridge control structure * - * Calls 'disable' drm_bridge_funcs op for all the bridges in the encoder + * Calls 'disable' &drm_bridge_funcs op for all the bridges in the encoder * chain, starting from the last bridge to the first. These are called before * calling the encoder's prepare op. * @@ -208,11 +191,11 @@ void drm_bridge_disable(struct drm_bridge *bridge) EXPORT_SYMBOL(drm_bridge_disable); /** - * drm_bridge_post_disable - calls 'post_disable' drm_bridge_funcs op for + * drm_bridge_post_disable - calls 'post_disable' &drm_bridge_funcs op for * all bridges in the encoder chain. * @bridge: bridge control structure * - * Calls 'post_disable' drm_bridge_funcs op for all the bridges in the + * Calls 'post_disable' &drm_bridge_funcs op for all the bridges in the * encoder chain, starting from the first bridge to the last. These are called * after completing the encoder's prepare op. * @@ -236,7 +219,7 @@ EXPORT_SYMBOL(drm_bridge_post_disable); * @mode: desired mode to be set for the bridge * @adjusted_mode: updated mode that works for this bridge * - * Calls 'mode_set' drm_bridge_funcs op for all the bridges in the + * Calls 'mode_set' &drm_bridge_funcs op for all the bridges in the * encoder chain, starting from the first bridge to the last. * * Note: the bridge passed should be the one closest to the encoder @@ -256,11 +239,11 @@ void drm_bridge_mode_set(struct drm_bridge *bridge, EXPORT_SYMBOL(drm_bridge_mode_set); /** - * drm_bridge_pre_enable - calls 'pre_enable' drm_bridge_funcs op for all + * drm_bridge_pre_enable - calls 'pre_enable' &drm_bridge_funcs op for all * bridges in the encoder chain. * @bridge: bridge control structure * - * Calls 'pre_enable' drm_bridge_funcs op for all the bridges in the encoder + * Calls 'pre_enable' &drm_bridge_funcs op for all the bridges in the encoder * chain, starting from the last bridge to the first. These are called * before calling the encoder's commit op. * @@ -278,11 +261,11 @@ void drm_bridge_pre_enable(struct drm_bridge *bridge) EXPORT_SYMBOL(drm_bridge_pre_enable); /** - * drm_bridge_enable - calls 'enable' drm_bridge_funcs op for all bridges + * drm_bridge_enable - calls 'enable' &drm_bridge_funcs op for all bridges * in the encoder chain. * @bridge: bridge control structure * - * Calls 'enable' drm_bridge_funcs op for all the bridges in the encoder + * Calls 'enable' &drm_bridge_funcs op for all the bridges in the encoder * chain, starting from the first bridge to the last. These are called * after completing the encoder's commit op. * diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index e8f8e4e9a6a1..ec3632e62b3f 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -883,24 +883,114 @@ struct drm_plane { /** * struct drm_bridge_funcs - drm_bridge control functions * @attach: Called during drm_bridge_attach - * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge - * @disable: Called right before encoder prepare, disables the bridge - * @post_disable: Called right after encoder prepare, for lockstepped disable - * @mode_set: Set this mode to the bridge - * @pre_enable: Called right before encoder commit, for lockstepped commit - * @enable: Called right after encoder commit, enables the bridge */ struct drm_bridge_funcs { int (*attach)(struct drm_bridge *bridge); + + /** + * @mode_fixup: + * + * This callback is used to validate and adjust a mode. The paramater + * mode is the display mode that should be fed to the next element in + * the display chain, either the final &drm_connector or the next + * &drm_bridge. The parameter adjusted_mode is the input mode the bridge + * requires. It can be modified by this callback and does not need to + * match mode. + * + * This is the only hook that allows a bridge to reject a modeset. If + * this function passes all other callbacks must succeed for this + * configuration. + * + * NOTE: + * + * This function is called in the check phase of atomic modesets, which + * can be aborted for any reason (including on userspace's request to + * just check whether a configuration would be possible). Drivers MUST + * NOT touch any persistent state (hardware or software) or data + * structures except the passed in adjusted_mode parameter. + * + * RETURNS: + * + * True if an acceptable configuration is possible, false if the modeset + * operation should be rejected. + */ bool (*mode_fixup)(struct drm_bridge *bridge, const struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode); + /** + * @disable: + * + * This callback should disable the bridge. It is called right before + * the preceding element in the display pipe is disabled. If the + * preceding element is a bridge this means it's called before that + * bridge's ->disaable function. If the preceding element is a + * &drm_encoder it's called right before the encoder's ->disable, + * ->prepare or ->dpms hook from struct &drm_encoder_helper_funcs. + * + * The bridge can assume that the display pipe (i.e. clocks and timing + * signals) feeding it is still running when this callback is called. + */ void (*disable)(struct drm_bridge *bridge); + + /** + * @post_disable: + * + * This callback should disable the bridge. It is called right after + * the preceding element in the display pipe is disabled. If the + * preceding element is a bridge this means it's called after that + * bridge's ->post_disaable function. If the preceding element is a + * &drm_encoder it's called right after the encoder's ->disable, + * ->prepare or ->dpms hook from struct &drm_encoder_helper_funcs. + * + * The bridge must assume that the display pipe (i.e. clocks and timing + * singals) feeding it is no longer running when this callback is + * called. + */ void (*post_disable)(struct drm_bridge *bridge); + + /** + * @mode_set: + * + * This callback should set the given mode on the bridge. It is called + * after the ->mode_set callback for the preceding element in the + * display pipeline has been called already. The display pipe (i.e. + * clocks and timing signals) is off when this function is called. + */ void (*mode_set)(struct drm_bridge *bridge, struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode); + /** + * @pre_enable: + * + * This callback should enable the bridge. It is called right before + * the preceding element in the display pipe is enabled. If the + * preceding element is a bridge this means it's called before that + * bridge's ->pre_enable function. If the preceding element is a + * &drm_encoder it's called right before the encoder's ->enable, + * ->commit or ->dpms hook from struct &drm_encoder_helper_funcs. + * + * The display pipe (i.e. clocks and timing signals) feeding this bridge + * will not yet be running when this callback is called. The bridge must + * not enable the display link feeding the next bridge in the chain (if + * there is one) when this callback is called. + */ void (*pre_enable)(struct drm_bridge *bridge); + + /** + * @enable: + * + * This callback should enable the bridge. It is called right after + * the preceding element in the display pipe is enabled. If the + * preceding element is a bridge this means it's called after that + * bridge's ->enable function. If the preceding element is a + * &drm_encoder it's called right after the encoder's ->enable, + * ->commit or ->dpms hook from struct &drm_encoder_helper_funcs. + * + * The bridge can assume that the display pipe (i.e. clocks and timing + * signals) feeding it is running when this callback is called. This + * callback must enable the display link feeding the next bridge in the + * chain if there is one. + */ void (*enable)(struct drm_bridge *bridge); };
Especially document the assumptions and semantics of the callbacks carefully. Just a warm-up excercise really. v2: Spelling fixes (Eric). v3: Consolidate more with existing docs: - Remove the overview section explaining the bridge funcs, that's now all in the drm_bridge_funcs kerneldoc in much more detail. - Use & to reference structs so that kerneldoc automatically inserts hyperlinks. Cc: Eric Anholt <eric@anholt.net> Cc: Archit Taneja <architt@codeaurora.org> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/drm_bridge.c | 69 +++++++++++------------------ include/drm/drm_crtc.h | 102 ++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 122 insertions(+), 49 deletions(-)