Hi Jagan
On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> DSI sink devices typically send the MIPI-DCS commands to the DSI host
> via general MIPI_DSI_DCS read and write API.
>
> The classical DSI sequence mentioned that the DSI host receives MIPI-DCS
> commands from the DSI sink first in order to switch HS mode properly.
> Once the DSI host switches to HS mode any MIPI-DCS commands from the
> DSI sink are unfunctional.
That statement contradicts the spec.
The DSI spec section 8.11.1 Transmission Packet Sequences says that
during any BLLP (Blanking or Low Power) period the host can do any of:
- remain in LP-11
- transmit one or more non-video packets from host to peripheral in escape mode
- transmit one or more non-video packets from host to peripheral in
using HS mode
- receive one or more packets from peripheral to host using escape mode
- transmit data on a different virtual channel.
Indeed if the sink doesn't set MIPI_DSI_MODE_LPM /
MIPI_DSI_MSG_USE_LPM, then the expectation is that any data transfer
will be in HS mode.
That makes me confused as to the need for this patch.
Dave
> DSI sink uses the @enable function to send the MIPI-DCS commands. In a
> typical DSI host, sink pipeline the @enable call chain start with the
> DSI host, and then the DSI sink which is the "wrong" order as DSI host
> @enable is called and switched to HS mode before DSI sink @enable.
>
> If the DSI host enables with the @enable_next_first flag then the
> @enable for the DSI sink will be called first before the @enable of
> the DSI host. This alter bridge init order makes sure that the MIPI-DCS
> commands send first and then switch to the HS mode properly by DSI host.
>
> This new flag @enable_next_first that any bridg can set to swap the
> order of @enable (and #disable) for that and the immediately next bridge.
>
> [enable]
> If a bridge sets @enable_next_first, then the @enable for the next bridge
> will be called first before enable of this bridge.
>
> [disable]
> If a bridge sets @enable_next_first, then the @disable for the next bridge
> will be called first before @disable of this bridge to reverse the @enable
> calling direction.
>
> eg:
> - Panel
> - Bridge 1
> - Bridge 2 enable_next_first
> - Bridge 3
> - Bridge 4 enable_next_first
> - Bridge 5 enable_next_first
> - Bridge 6
> - Encoder
>
> Would result in enable's being called as Encoder, Bridge 6, Bridge 3,
> Bridge 4, Bridge 5, Bridge 1, Bridge 2, Panel.
>
> and the result in disable's being called as Panel, Bridge 2, Bridge 1,
> Bridge 5, Bridge 4, Bridge 3, Bridge 6, Encoder.
>
> Signed-off-by: Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx>
> ---
> Changes for v7:
> - new patch
>
> drivers/gpu/drm/drm_bridge.c | 171 ++++++++++++++++++++++++++++++-----
> include/drm/drm_bridge.h | 8 ++
> 2 files changed, 154 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index caf0f341e524..cdc2669b3512 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -577,6 +577,24 @@ void drm_bridge_chain_mode_set(struct drm_bridge *bridge,
> }
> EXPORT_SYMBOL(drm_bridge_chain_mode_set);
>
> +static void drm_atomic_bridge_call_disable(struct drm_bridge *bridge,
> + struct drm_atomic_state *old_state)
> +{
> + if (old_state && bridge->funcs->atomic_disable) {
> + struct drm_bridge_state *old_bridge_state;
> +
> + old_bridge_state =
> + drm_atomic_get_old_bridge_state(old_state,
> + bridge);
> + if (WARN_ON(!old_bridge_state))
> + return;
> +
> + bridge->funcs->atomic_disable(bridge, old_bridge_state);
> + } else if (bridge->funcs->disable) {
> + bridge->funcs->disable(bridge);
> + }
> +}
> +
> /**
> * drm_atomic_bridge_chain_disable - disables all bridges in the encoder chain
> * @bridge: bridge control structure
> @@ -587,33 +605,73 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
> * starting from the last bridge to the first. These are called before calling
> * &drm_encoder_helper_funcs.atomic_disable
> *
> + * If a bridge sets @enable_next_first, then the @disable for the next bridge
> + * will be called first before @disable of this bridge to reverse the @enable
> + * calling direction.
> + *
> + * Example:
> + * Bridge A ---> Bridge B ---> Bridge C ---> Bridge D ---> Bridge E
> + *
> + * With enable_next_first flag enable in Bridge A, C, D then the resulting
> + * @disable order would be,
> + * Bridge C, Bridge D, Bridge E, Bridge A, Bridge B.
> + *
> * Note: the bridge passed should be the one closest to the encoder
> */
> void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
> struct drm_atomic_state *old_state)
> {
> struct drm_encoder *encoder;
> - struct drm_bridge *iter;
> + struct drm_bridge *iter, *next, *limit;
>
> if (!bridge)
> return;
>
> encoder = bridge->encoder;
> +
> list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> - if (iter->funcs->atomic_disable) {
> - struct drm_bridge_state *old_bridge_state;
> -
> - old_bridge_state =
> - drm_atomic_get_old_bridge_state(old_state,
> - iter);
> - if (WARN_ON(!old_bridge_state))
> - return;
> -
> - iter->funcs->atomic_disable(iter, old_bridge_state);
> - } else if (iter->funcs->disable) {
> - iter->funcs->disable(iter);
> + limit = NULL;
> +
> + if (!list_is_first(&iter->chain_node, &encoder->bridge_chain)) {
> + next = list_prev_entry(iter, chain_node);
> +
> + if (next->enable_next_first) {
> + limit = bridge;
> + list_for_each_entry_from_reverse(next,
> + &encoder->bridge_chain,
> + chain_node) {
> + if (next == bridge)
> + break;
> +
> + if (!next->enable_next_first) {
> + /* Found first bridge that does NOT
> + * request next to be enabled first
> + */
> + next = list_next_entry(next, chain_node);
> + limit = next;
> + break;
> + }
> + }
> +
> + list_for_each_entry_from(next, &encoder->bridge_chain, chain_node) {
> + /* Call requested next bridge enable in order */
> + if (next == iter)
> + /* At the first bridge to request next
> + * bridges called first.
> + */
> + break;
> +
> + drm_atomic_bridge_call_disable(next, old_state);
> + }
> + }
> }
>
> + drm_atomic_bridge_call_disable(iter, old_state);
> +
> + if (limit)
> + /* Jump all bridges that we have already disabled */
> + iter = limit;
> +
> if (iter == bridge)
> break;
> }
> @@ -822,6 +880,24 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
> }
> EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);
>
> +static void drm_atomic_bridge_call_enable(struct drm_bridge *bridge,
> + struct drm_atomic_state *old_state)
> +{
> + if (old_state && bridge->funcs->atomic_enable) {
> + struct drm_bridge_state *old_bridge_state;
> +
> + old_bridge_state =
> + drm_atomic_get_old_bridge_state(old_state,
> + bridge);
> + if (WARN_ON(!old_bridge_state))
> + return;
> +
> + bridge->funcs->atomic_enable(bridge, old_bridge_state);
> + } else if (bridge->funcs->enable) {
> + bridge->funcs->enable(bridge);
> + }
> +}
> +
> /**
> * drm_atomic_bridge_chain_enable - enables all bridges in the encoder chain
> * @bridge: bridge control structure
> @@ -832,31 +908,76 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);
> * starting from the first bridge to the last. These are called after completing
> * &drm_encoder_helper_funcs.atomic_enable
> *
> + * If a bridge sets @enable_next_first, then the @enable for the next bridge
> + * will be called first before enable of this bridge.
> + *
> + * Example:
> + * Bridge A ---> Bridge B ---> Bridge C ---> Bridge D ---> Bridge E
> + *
> + * With enable_next_first flag enable in Bridge A, C, D then the resulting
> + * @enable order would be,
> + * Bridge B, Bridge A, Bridge E, Bridge D, Bridge C.
> + *
> * Note: the bridge passed should be the one closest to the encoder
> */
> void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge,
> struct drm_atomic_state *old_state)
> {
> struct drm_encoder *encoder;
> + struct drm_bridge *next, *limit;
>
> if (!bridge)
> return;
>
> encoder = bridge->encoder;
> +
> list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> - if (bridge->funcs->atomic_enable) {
> - struct drm_bridge_state *old_bridge_state;
> -
> - old_bridge_state =
> - drm_atomic_get_old_bridge_state(old_state,
> - bridge);
> - if (WARN_ON(!old_bridge_state))
> - return;
> -
> - bridge->funcs->atomic_enable(bridge, old_bridge_state);
> - } else if (bridge->funcs->enable) {
> - bridge->funcs->enable(bridge);
> + limit = NULL;
> +
> + if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain)) {
> + if (bridge->enable_next_first) {
> + /* next bridge had requested that next
> + * was enabled first, so disabled last
> + */
> + next = list_next_entry(bridge, chain_node);
> + limit = next;
> +
> + list_for_each_entry_from(next, &encoder->bridge_chain,
> + chain_node) {
> + /* Find the next bridge that has NOT requested
> + * next to be enabled first / disabled last
> + */
> + if (!next->enable_next_first) {
> + limit = next;
> + break;
> + }
> +
> + /* Last bridge has requested next to be limit
> + * otherwise the control move to end of chain
> + */
> + if (list_is_last(&next->chain_node,
> + &encoder->bridge_chain)) {
> + limit = next;
> + break;
> + }
> + }
> +
> + /* Call these bridges in reverse order */
> + list_for_each_entry_from_reverse(next, &encoder->bridge_chain,
> + chain_node) {
> + if (next == bridge)
> + break;
> +
> + drm_atomic_bridge_call_enable(next, old_state);
> + }
> + }
> }
> +
> + drm_atomic_bridge_call_enable(bridge, old_state);
> +
> + if (limit)
> + /* Jump all bridges that we have already enabled */
> + bridge = limit;
> }
> }
> EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index a1a31704b917..9879129047e4 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -752,6 +752,14 @@ struct drm_bridge {
> * before the peripheral.
> */
> bool pre_enable_prev_first;
> + /**
> + * @enable_next_first: The bridge requires that the next bridge @enable
> + * function is called first before its @enable, and conversely for
> + * @disable. This is most frequently a requirement for a DSI host to
> + * receive MIPI-DCS commands from DSI sink first in order to switch
> + * HS mode properly.
> + */
> + bool enable_next_first;
> /**
> * @ddc: Associated I2C adapter for DDC access, if any.
> */
> --
> 2.25.1
>
