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 at codeaurora.org> > > Cc: Eric Anholt <eric at anholt.net> > Cc: Archit Taneja <architt at codeaurora.org> > Signed-off-by: Daniel Vetter <daniel.vetter at 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); > }; > > -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation