[v7,10/12] drm/bridge: Implement enable_next_first to alter bridge init order

2835 diff mbox series

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.

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@amarulasolutions.com>
---
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(-)

Hi Jagan

On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> 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@amarulasolutions.com>
> ---
> 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
>

On Wed, Mar 29, 2023 at 05:28:28PM +0100, Dave Stevenson wrote:
> On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> 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.

Yeah, and it looks like that would break the expectation that, in
enable, a bridge can expect its controller to be in HS mode.

I think that was Jagan is trying to do is to work around an issue with
the Allwinner DSI driver:
https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L775

This is working mostly fine since we only have panel support and can
control that, but with bridge support added in the latest patch, then it
probably doesn't work anymore.

The proper way to fix this isn't to put more logic into the framework,
it's to make the DSI driver behave as expected by KMS.

Unfortunately, that controller is not documented, so it's not clear to
me how we can fix it.

IIRC, it's basically a state machine where you would encode the
transitions between one DSI state and the next depending on what your
expectations are.

I think there's two problem with the driver that need to be addressed:

  - First the driver will drop back to LP11 mode to submit commands. I
    don't think it's needed and could even be hurtful to the video
    stream if it was to happen during HS mode:
    https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L877

  - And then, it looks like, in HSD mode, we never get to go to the
    state LPTX is in (LPDT). It would be interesting to test whether
    adding a transition to that state makes it work or not.

Maxime

Hi Maxime

On Wed, 29 Mar 2023 at 17:46, Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Wed, Mar 29, 2023 at 05:28:28PM +0100, Dave Stevenson wrote:
> > On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> 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.
>
> Yeah, and it looks like that would break the expectation that, in
> enable, a bridge can expect its controller to be in HS mode.
>
> I think that was Jagan is trying to do is to work around an issue with
> the Allwinner DSI driver:
> https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L775
>
> This is working mostly fine since we only have panel support and can
> control that, but with bridge support added in the latest patch, then it
> probably doesn't work anymore.
>
> The proper way to fix this isn't to put more logic into the framework,
> it's to make the DSI driver behave as expected by KMS.
>
> Unfortunately, that controller is not documented, so it's not clear to
> me how we can fix it.
>
> IIRC, it's basically a state machine where you would encode the
> transitions between one DSI state and the next depending on what your
> expectations are.
>
> I think there's two problem with the driver that need to be addressed:
>
>   - First the driver will drop back to LP11 mode to submit commands. I
>     don't think it's needed and could even be hurtful to the video
>     stream if it was to happen during HS mode:
>     https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L877
>
>   - And then, it looks like, in HSD mode, we never get to go to the
>     state LPTX is in (LPDT). It would be interesting to test whether
>     adding a transition to that state makes it work or not.

Ooh, not fun.
I'll agree with your assessment - it looks like sunxi driver has
significant limitations on the modes of operation it supports. If
there is no information on sending HS commands, I wonder if it's
possible to note the video state in transfer and stop video, send the
command, and resume video again. Ugly as heck, but possibly the only
real option without documentation. It does raise the question of do
other blocks (eg crtc) need to be stopped as well, or does stopping
the PHY and/or DSI block stop the pixel data getting clocked out.

I can only guess at the meaning of the enum sun6i_dsi_start_inst and
enum sun6i_dsi_inst_id states. LPTX and LPRX are largely obvious, but
HSC(ommand) and HSD(ata) perhaps?
I thought on initial reading that the setup in sun6i_dsi_start made
sense as a sequence of commands, but looking closer at the bitmasking
and shifting I'm not so convinced. Are the DSI_INST_ID_xxx defines
shifts or the bitmask values to or in, as they get used for both.

  Dave

On Wed, Mar 29, 2023 at 10:16 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Wed, Mar 29, 2023 at 05:28:28PM +0100, Dave Stevenson wrote:
> > On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> 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.
>
> Yeah, and it looks like that would break the expectation that, in
> enable, a bridge can expect its controller to be in HS mode.
>
> I think that was Jagan is trying to do is to work around an issue with
> the Allwinner DSI driver:
> https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L775

Correct and I can see it seems to be a classic DSI sequence observed
in dw-mipi-dsi as well - based on Neil's comments.
https://lore.kernel.org/all/9aa3d19d-4378-aaf3-6857-c40be5d252c7@baylibre.com/

In fact, I did follow and initialize the command-mode mode_set which
set low-speed DCS and switch back to video-mode @enable and switch to
HS but could see the same issue as the host cannot accept DCS as
before (I might implement improper sequence, but not sure due to lack
of documentation). But this sequence has issues with calling
post_disable twice even on dw-mipi-dsi.

May be Neill, can comment here?

Thanks,
Jagan.

Hi Jagan

On Thu, 30 Mar 2023 at 07:56, Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> On Wed, Mar 29, 2023 at 10:16 PM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > On Wed, Mar 29, 2023 at 05:28:28PM +0100, Dave Stevenson wrote:
> > > On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> 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.
> >
> > Yeah, and it looks like that would break the expectation that, in
> > enable, a bridge can expect its controller to be in HS mode.
> >
> > I think that was Jagan is trying to do is to work around an issue with
> > the Allwinner DSI driver:
> > https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L775
>
> Correct and I can see it seems to be a classic DSI sequence observed
> in dw-mipi-dsi as well - based on Neil's comments.
> https://lore.kernel.org/all/9aa3d19d-4378-aaf3-6857-c40be5d252c7@baylibre.com/

Neil's comments are from 2021, and his response would appear to be
with regard the PHY power up sequence issues that
pre_enable_prev_first should solve. The DSI host pre_enable can now be
called before the sink's pre_enable, therefore allowing the PHY to be
configured in pre_enable. Hacking the PHY init into mode_set is
therefore not required.

I don't see any restriction in dw-mipi-dsi over when transfer can be
called (as long as it is between pre_enable and post_disable), and it
supports MIPI_DSI_MSG_USE_LPM for requesting the command be sent in
either LP or HS mode.

> In fact, I did follow and initialize the command-mode mode_set which
> set low-speed DCS and switch back to video-mode @enable and switch to
> HS but could see the same issue as the host cannot accept DCS as
> before (I might implement improper sequence, but not sure due to lack
> of documentation). But this sequence has issues with calling
> post_disable twice even on dw-mipi-dsi.

Calling up/down the bridge chain from within other bridge elements is
going to have issues and shouldn't be necessary.

The comment in dw-mipi-dsi post_disable[1]
* TODO Only way found to call panel-bridge post_disable &
* panel unprepare before the dsi "final" disable...
* This needs to be fixed in the drm_bridge framework and the API
* needs to be updated to manage our own call chains...

It has now been fixed up with pre_enable_prev_first.

I seem to recall seeing a patchset for one of the DSI hosts (other
than vc4) that was moving the init from mode_set to pre_enable - I
think it is probably [2] for msm.

Cheers
  Dave

[1] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L862-L867
[2] https://github.com/torvalds/linux/commit/ec7981e6c614254937b37ce0af9eac09901c05c5

> May be Neill, can comment here?
>
> Thanks,
> Jagan.

On 30/03/2023 12:01, Dave Stevenson wrote:
> Hi Jagan
> 
> On Thu, 30 Mar 2023 at 07:56, Jagan Teki <jagan@amarulasolutions.com> wrote:
>>
>> On Wed, Mar 29, 2023 at 10:16 PM Maxime Ripard <maxime@cerno.tech> wrote:
>>>
>>> On Wed, Mar 29, 2023 at 05:28:28PM +0100, Dave Stevenson wrote:
>>>> On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> 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.
>>>
>>> Yeah, and it looks like that would break the expectation that, in
>>> enable, a bridge can expect its controller to be in HS mode.
>>>
>>> I think that was Jagan is trying to do is to work around an issue with
>>> the Allwinner DSI driver:
>>> https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L775
>>
>> Correct and I can see it seems to be a classic DSI sequence observed
>> in dw-mipi-dsi as well - based on Neil's comments.
>> https://lore.kernel.org/all/9aa3d19d-4378-aaf3-6857-c40be5d252c7@baylibre.com/
> 
> Neil's comments are from 2021, and his response would appear to be
> with regard the PHY power up sequence issues that
> pre_enable_prev_first should solve. The DSI host pre_enable can now be
> called before the sink's pre_enable, therefore allowing the PHY to be
> configured in pre_enable. Hacking the PHY init into mode_set is
> therefore not required.

Yes this part is not solved, but is seems the assumption the DSI controller
can switch to HS to LS & then to HS back after a command while in video mode
isn't true in the Allwinner's case. As I understood it's one of the problems.

We're hitting a limit of the DSI controller model in Linux where we cannot
express all the DSI capabilities (Video mode, Command mode, dynamic framerate
switching, DSC, ...) since from the Panel or Bridge PoV we're blind and
we do not know what are the features supported by the DSI controller and
we lack knowledge of any operation mode we must try to achieve.

> 
> I don't see any restriction in dw-mipi-dsi over when transfer can be
> called (as long as it is between pre_enable and post_disable), and it
> supports MIPI_DSI_MSG_USE_LPM for requesting the command be sent in
> either LP or HS mode.
> 
>> In fact, I did follow and initialize the command-mode mode_set which
>> set low-speed DCS and switch back to video-mode @enable and switch to
>> HS but could see the same issue as the host cannot accept DCS as
>> before (I might implement improper sequence, but not sure due to lack
>> of documentation). But this sequence has issues with calling
>> post_disable twice even on dw-mipi-dsi.
> 
> Calling up/down the bridge chain from within other bridge elements is
> going to have issues and shouldn't be necessary.
> 
> The comment in dw-mipi-dsi post_disable[1]
> * TODO Only way found to call panel-bridge post_disable &
> * panel unprepare before the dsi "final" disable...
> * This needs to be fixed in the drm_bridge framework and the API
> * needs to be updated to manage our own call chains...
> 
> It has now been fixed up with pre_enable_prev_first.
> 
> I seem to recall seeing a patchset for one of the DSI hosts (other
> than vc4) that was moving the init from mode_set to pre_enable - I
> think it is probably [2] for msm.
> 
> Cheers
>    Dave
> 
> [1] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L862-L867
> [2] https://github.com/torvalds/linux/commit/ec7981e6c614254937b37ce0af9eac09901c05c5
> 
>> May be Neill, can comment here?
>>
>> Thanks,
>> Jagan.

Hi Dave,

On Thu, Mar 30, 2023 at 3:31 PM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Jagan
>
> On Thu, 30 Mar 2023 at 07:56, Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > On Wed, Mar 29, 2023 at 10:16 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > On Wed, Mar 29, 2023 at 05:28:28PM +0100, Dave Stevenson wrote:
> > > > On Wed, 29 Mar 2023 at 14:19, Jagan Teki <jagan@amarulasolutions.com> 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.
> > >
> > > Yeah, and it looks like that would break the expectation that, in
> > > enable, a bridge can expect its controller to be in HS mode.
> > >
> > > I think that was Jagan is trying to do is to work around an issue with
> > > the Allwinner DSI driver:
> > > https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c#L775
> >
> > Correct and I can see it seems to be a classic DSI sequence observed
> > in dw-mipi-dsi as well - based on Neil's comments.
> > https://lore.kernel.org/all/9aa3d19d-4378-aaf3-6857-c40be5d252c7@baylibre.com/
>
> Neil's comments are from 2021, and his response would appear to be
> with regard the PHY power up sequence issues that
> pre_enable_prev_first should solve. The DSI host pre_enable can now be
> called before the sink's pre_enable, therefore allowing the PHY to be
> configured in pre_enable. Hacking the PHY init into mode_set is
> therefore not required.
>
> I don't see any restriction in dw-mipi-dsi over when transfer can be
> called (as long as it is between pre_enable and post_disable), and it
> supports MIPI_DSI_MSG_USE_LPM for requesting the command be sent in
> either LP or HS mode.
>
> > In fact, I did follow and initialize the command-mode mode_set which
> > set low-speed DCS and switch back to video-mode @enable and switch to
> > HS but could see the same issue as the host cannot accept DCS as
> > before (I might implement improper sequence, but not sure due to lack
> > of documentation). But this sequence has issues with calling
> > post_disable twice even on dw-mipi-dsi.
>
> Calling up/down the bridge chain from within other bridge elements is
> going to have issues and shouldn't be necessary.
>
> The comment in dw-mipi-dsi post_disable[1]
> * TODO Only way found to call panel-bridge post_disable &
> * panel unprepare before the dsi "final" disable...
> * This needs to be fixed in the drm_bridge framework and the API
> * needs to be updated to manage our own call chains...
>
> It has now been fixed up with pre_enable_prev_first.

This seems not fixed in dw-mipi-dsi and it often still requires the
forth and back switch of HS mode even if pre_enable_prev_first.

Thanks,
Jagan.