| Message ID | 20230329131929.1328612-1-jagan@amarulasolutions.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: sun4i: Convert Allwinner DSI to bridge | expand |
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.
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. */
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(-)