Re: [PATCH 06/28] drm/bridge: Improve kerneldoc

[Archit Taneja <architt@codeaurora.org>]

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);
>   };
>
>

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum, hosted by The Linux Foundation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx