Hi All
Changes from v1: - New patch to refactor drm_bridge_chain_post_disable and drm_bridge_chain_pre_enable to reuse drm_atomic_bridge_chain_post_disable / drm_atomic_bridge_chain_pre_enable but with a NULL state. - New patch that adds a pre_enable_upstream_first to drm_panel. - changed from an OPS flag to a bool "pre_enable_upstream_first" in drm_bridge. - Followed Andrzej's suggestion of using continue in the main loop to avoid needing 2 additional loops (one forward to find the last bridge wanting upstream first, and the second backwards again). - Actioned Laurent's review comments on docs patch.
Original cover letter:
Hopefully I've cc'ed all those that have bashed this problem around previously, or are otherwise linked to DRM bridges.
There have been numerous discussions around how DSI support is currently broken as it doesn't support initialising the PHY to LP-11 and potentially the clock lane to HS prior to configuring the DSI peripheral. There is no op where the interface is initialised but HS video isn't also being sent. Currently you have: - peripheral pre_enable (host not initialised yet) - host pre_enable - encoder enable - host enable - peripheral enable (video already running)
vc4 and exynos currently implement the DSI host as an encoder, and split the bridge_chain. This fails if you want to switch to being a bridge and/or use atomic calls as the state of all the elements split off are not added by drm_atomic_add_encoder_bridges.
dw-mipi-dsi[1] and now msm[2] use the mode_set hook to initialise the PHY, so the bridge/panel pre_enable can send commands. In their post_disable they then call the downstream bridge/panel post_disable op manually so that shutdown commands can be sent before shutting down the PHY. Nothing handles that fact, so the framework then continues down the bridge chain and calls the post_disable again, so we get unbalanced panel prepare/unprepare calls being reported [3].
There have been patches[4] proposing reversing the entire direction of pre_enable and post_disable, but that risks driving voltage into devices that have yet to be powered up. There have been discussions about adding either a pre_pre_enable, or adding a DSI host_op to initialise the host[5]. Both require significant reworking to all existing drivers in moving initialisation phases. We have patches that look like they may well be addressing race conditions in starting up a DSI peripheral[6].
This patch takes a hybrid of the two: an optional reversing of the order for specific links within the bridge chain within pre_enable and post_disable done within the drm_bridge framework. I'm more than happy to move where the flag exists in structures (currently as DRM_BRIDGE_OP_UPSTREAM_FIRST in drm_bridge_ops, but it isn't an op), but does this solve the problem posed? If not, then can you describe the actual scenario it doesn't cover? A DSI peripheral can set the flag to get the DSI host initialised first, and therefore it has a stable LP-11 state before pre_enable. Likewise the peripheral can still send shutdown commands prior to the DSI host being shut down in post_disable. It also handles the case where there are multiple devices in the chain that all want their upstream bridge enabled first, so should there be a DSI mux between host and peripheral, then it can still get the host to the correct state.
An example tree is at [7] which is drm-misc-next with these patches and then a conversion of vc4_dsi to use the atomic bridge functions (will be upstreamed once we're over this hurdle). It is working happily with the Toshiba TC358762 on a Raspberry Pi 7" panel. The same approach but on our vendor 5.15 tree[8] has also been tested successfully on a TI SN65DSI83 and LVDS panel.
Whilst here, I've also documented the expected behaviour of DSI hosts and peripherals to aid those who come along after.
Thanks Dave
[1] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/bridge/synopsy... [2] https://lists.freedesktop.org/archives/dri-devel/2022-January/337769.html [3] https://lists.freedesktop.org/archives/dri-devel/2021-December/333908.html [4] https://lists.freedesktop.org/archives/dri-devel/2021-October/328476.html [5] https://lists.freedesktop.org/archives/dri-devel/2021-October/325853.html [6] https://lists.freedesktop.org/archives/dri-devel/2022-February/341852.html [7] https://github.com/6by9/linux/tree/drm-misc-next-vc4_dsi [8] https://github.com/6by9/linux/tree/rpi-5.15.y-sn65dsi83
Dave Stevenson (4): drm/bridge: Remove duplication from drm_bridge and drm_atomic_bridge chains drm/bridge: Introduce pre_enable_upstream_first to alter bridge init order drm/panel: Add prepare_upstream_first flag to drm_panel drm/bridge: Document the expected behaviour of DSI host controllers
Documentation/gpu/drm-kms-helpers.rst | 7 ++ drivers/gpu/drm/bridge/panel.c | 3 + drivers/gpu/drm/drm_bridge.c | 181 ++++++++++++++++++++++++---------- include/drm/drm_bridge.h | 8 ++ include/drm/drm_panel.h | 10 ++ 5 files changed, 159 insertions(+), 50 deletions(-)
drm_bridge_chain_pre_enable is a subset of drm_atomic_bridge_chain_pre_enable, and drm_bridge_chain_post_disable is a subset of drm_atomic_bridge_chain_post_disable.
Change drm_bridge_chain_pre_enable and drm_bridge_chain_post_disable to call the atomic versions with a NULL state, and ensure that atomic calls are not made if there is no state.
Signed-off-by: Dave Stevenson dave.stevenson@raspberrypi.com --- drivers/gpu/drm/drm_bridge.c | 30 ++++-------------------------- 1 file changed, 4 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index c96847fc0ebc..198fd471a488 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -527,16 +527,7 @@ EXPORT_SYMBOL(drm_bridge_chain_disable); */ void drm_bridge_chain_post_disable(struct drm_bridge *bridge) { - struct drm_encoder *encoder; - - if (!bridge) - return; - - encoder = bridge->encoder; - list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) { - if (bridge->funcs->post_disable) - bridge->funcs->post_disable(bridge); - } + drm_atomic_bridge_chain_post_disable(bridge, NULL); } EXPORT_SYMBOL(drm_bridge_chain_post_disable);
@@ -582,20 +573,7 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set); */ void drm_bridge_chain_pre_enable(struct drm_bridge *bridge) { - struct drm_encoder *encoder; - struct drm_bridge *iter; - - if (!bridge) - return; - - encoder = bridge->encoder; - list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) { - if (iter->funcs->pre_enable) - iter->funcs->pre_enable(iter); - - if (iter == bridge) - break; - } + drm_atomic_bridge_chain_pre_enable(bridge, NULL); } EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
@@ -690,7 +668,7 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
encoder = bridge->encoder; list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) { - if (bridge->funcs->atomic_post_disable) { + if (old_state && bridge->funcs->atomic_post_disable) { struct drm_bridge_state *old_bridge_state;
old_bridge_state = @@ -732,7 +710,7 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
encoder = bridge->encoder; list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) { - if (iter->funcs->atomic_pre_enable) { + if (old_state && iter->funcs->atomic_pre_enable) { struct drm_bridge_state *old_bridge_state;
old_bridge_state =
On 04/03/2022 18:17, Dave Stevenson wrote:
drm_bridge_chain_pre_enable is a subset of drm_atomic_bridge_chain_pre_enable, and drm_bridge_chain_post_disable is a subset of drm_atomic_bridge_chain_post_disable.
Change drm_bridge_chain_pre_enable and drm_bridge_chain_post_disable to call the atomic versions with a NULL state, and ensure that atomic calls are not made if there is no state.
Signed-off-by: Dave Stevenson dave.stevenson@raspberrypi.com
I think we should update parade-ps8640 to use drm_atomic_bridge_chain_() and drop drm_bridge_chain_* API completely.
drivers/gpu/drm/drm_bridge.c | 30 ++++-------------------------- 1 file changed, 4 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index c96847fc0ebc..198fd471a488 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -527,16 +527,7 @@ EXPORT_SYMBOL(drm_bridge_chain_disable); */ void drm_bridge_chain_post_disable(struct drm_bridge *bridge) {
- struct drm_encoder *encoder;
- if (!bridge)
return;- encoder = bridge->encoder;
- list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
if (bridge->funcs->post_disable)bridge->funcs->post_disable(bridge);- }
- drm_atomic_bridge_chain_post_disable(bridge, NULL); } EXPORT_SYMBOL(drm_bridge_chain_post_disable);
@@ -582,20 +573,7 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set); */ void drm_bridge_chain_pre_enable(struct drm_bridge *bridge) {
- struct drm_encoder *encoder;
- struct drm_bridge *iter;
- if (!bridge)
return;- encoder = bridge->encoder;
- list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
if (iter->funcs->pre_enable)iter->funcs->pre_enable(iter);if (iter == bridge)break;- }
- drm_atomic_bridge_chain_pre_enable(bridge, NULL); } EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
@@ -690,7 +668,7 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
encoder = bridge->encoder; list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
if (bridge->funcs->atomic_post_disable) {
if (old_state && bridge->funcs->atomic_post_disable) { struct drm_bridge_state *old_bridge_state; old_bridge_state =@@ -732,7 +710,7 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
encoder = bridge->encoder; list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
if (iter->funcs->atomic_pre_enable) {
if (old_state && iter->funcs->atomic_pre_enable) { struct drm_bridge_state *old_bridge_state; old_bridge_state =
DSI sink devices typically want the DSI host powered up and configured before they are powered up. pre_enable is the place this would normally happen, but they are called in reverse order from panel/connector towards the encoder, which is the "wrong" order.
Add a new flag pre_enable_upstream_first that any bridge can set to swap the order of pre_enable (and post_disable) for that and the immediately upstream bridge. Should the immediately upstream bridge also set the pre_enable_upstream_first flag, the bridge upstream of that will be called before either of those which requested pre_enable_upstream_first.
eg: - Panel - Bridge 1 - Bridge 2 pre_enable_upstream_first - Bridge 3 - Bridge 4 pre_enable_upstream_first - Bridge 5 pre_enable_upstream_first - Bridge 6 - Encoder Would result in pre_enable's being called as Panel, Bridge 1, Bridge 3, Bridge 2, Bridge 6, Bridge 5, Bridge 4, Encoder.
Signed-off-by: Dave Stevenson dave.stevenson@raspberrypi.com --- drivers/gpu/drm/drm_bridge.c | 116 +++++++++++++++++++++++++++++++++---------- include/drm/drm_bridge.h | 8 +++ 2 files changed, 98 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 198fd471a488..70b513f5ce0d 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -523,6 +523,10 @@ EXPORT_SYMBOL(drm_bridge_chain_disable); * encoder chain, starting from the first bridge to the last. These are called * after completing the encoder's prepare op. * + * If a bridge sets @pre_enable_upstream_first, then the @post_disable for that + * bridge will be called before the previous one to reverse the @pre_enable + * calling direction. + * * Note: the bridge passed should be the one closest to the encoder */ void drm_bridge_chain_post_disable(struct drm_bridge *bridge) @@ -569,6 +573,9 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set); * chain, starting from the last bridge to the first. These are called * before calling the encoder's commit op. * + * If a bridge sets @pre_enable_upstream_first, then the @pre_enable for the + * previous bridge will be called before @pre_enable of this bridge. + * * Note: the bridge passed should be the one closest to the encoder */ void drm_bridge_chain_pre_enable(struct drm_bridge *bridge) @@ -645,6 +652,25 @@ void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge, } EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);
+static void drm_atomic_bridge_call_post_disable(struct drm_bridge *bridge, + struct drm_atomic_state *old_state) +{ + if (old_state && bridge->funcs->atomic_post_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_post_disable(bridge, + old_bridge_state); + } else if (bridge->funcs->post_disable) { + bridge->funcs->post_disable(bridge); + } +} + /** * drm_atomic_bridge_chain_post_disable - cleans up after disabling all bridges * in the encoder chain @@ -655,6 +681,9 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_disable); * &drm_bridge_funcs.post_disable) op for all the bridges in the encoder chain, * starting from the first bridge to the last. These are called after completing * &drm_encoder_helper_funcs.atomic_disable + * If a bridge sets @pre_enable_upstream_first, then the @post_disable for that + * bridge will be called before the previous one to reverse the @pre_enable + * calling direction. * * Note: the bridge passed should be the one closest to the encoder */ @@ -662,30 +691,55 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge, struct drm_atomic_state *old_state) { struct drm_encoder *encoder; + struct drm_bridge *prev, *tmp;
if (!bridge) return;
encoder = bridge->encoder; - list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) { - if (old_state && bridge->funcs->atomic_post_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; + list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) { + if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain) && + list_next_entry(bridge, chain_node)->pre_enable_upstream_first) + /* Skip bridges where the downstream bridge wanted + * pre_enable after / post_disable before the upstream + * bridge. + */ + continue;
- bridge->funcs->atomic_post_disable(bridge, - old_bridge_state); - } else if (bridge->funcs->post_disable) { - bridge->funcs->post_disable(bridge); - } + /* Call this bridge and any skipped ones in reverse order */ + tmp = bridge; + do { + prev = tmp; + drm_atomic_bridge_call_post_disable(tmp, old_state); + if (tmp == list_first_entry(&encoder->bridge_chain, + struct drm_bridge, chain_node)) + tmp = NULL; + else + tmp = list_prev_entry(tmp, chain_node); + } while (tmp && prev->pre_enable_upstream_first); } } EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
+static void drm_atomic_bridge_call_pre_enable(struct drm_bridge *bridge, + struct drm_atomic_state *old_state) +{ + if (old_state && bridge->funcs->atomic_pre_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_pre_enable(bridge, old_bridge_state); + } else if (bridge->funcs->pre_enable) { + bridge->funcs->pre_enable(bridge); + } +} + /** * drm_atomic_bridge_chain_pre_enable - prepares for enabling all bridges in * the encoder chain @@ -697,32 +751,42 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable); * starting from the last bridge to the first. These are called before calling * &drm_encoder_helper_funcs.atomic_enable * + * If a bridge sets @pre_enable_upstream_first, then the pre_enable for the + * upstream bridge will be called before pre_enable of this bridge. + * * Note: the bridge passed should be the one closest to the encoder */ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge, struct drm_atomic_state *old_state) { struct drm_encoder *encoder; - struct drm_bridge *iter; + struct drm_bridge *iter, *tmp;
if (!bridge) return;
encoder = bridge->encoder; - list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) { - if (old_state && iter->funcs->atomic_pre_enable) { - 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; + list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) { + if (iter->pre_enable_upstream_first && iter != bridge) + /* Skip bridges which want the upstream pre_enable + * called before their pre_enable. + */ + continue;
- iter->funcs->atomic_pre_enable(iter, old_bridge_state); - } else if (iter->funcs->pre_enable) { - iter->funcs->pre_enable(iter); - } + tmp = iter; + do { + /* Work forward through the current bridge, and any + * that had been skipped. + */ + drm_atomic_bridge_call_pre_enable(tmp, old_state); + if (tmp == list_last_entry(&encoder->bridge_chain, + struct drm_bridge, + chain_node)) + tmp = NULL; + else + tmp = list_next_entry(tmp, chain_node); + } while (tmp && tmp->pre_enable_upstream_first);
if (iter == bridge) break; diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index f27b4060faa2..cf1fb3ad7054 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -769,6 +769,14 @@ struct drm_bridge { */ bool interlace_allowed; /** + * @pre_enable_upstream_first: The bridge requires that the upstream + * bridge @pre_enable function is called before its @pre_enable, + * and conversely for post_disable. This is most frequently a + * requirement for DSI devices which need the host to be initialised + * before the peripheral. + */ + bool pre_enable_upstream_first; + /** * @ddc: Associated I2C adapter for DDC access, if any. */ struct i2c_adapter *ddc;
Mapping to the drm_bridge flag pre_enable_upstream_first, add a new flag prepare_upstream_first to drm_panel to allow the panel driver to request that the upstream bridge should be pre_enabled before the panel prepare.
Signed-off-by: Dave Stevenson dave.stevenson@raspberrypi.com --- drivers/gpu/drm/bridge/panel.c | 3 +++ include/drm/drm_panel.h | 10 ++++++++++ 2 files changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index 5be057575183..2ea08b3ba326 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -234,6 +234,9 @@ struct drm_bridge *drm_panel_bridge_add_typed(struct drm_panel *panel, panel_bridge->bridge.ops = DRM_BRIDGE_OP_MODES; panel_bridge->bridge.type = connector_type;
+ panel_bridge->bridge.pre_enable_upstream_first = + panel->prepare_upstream_first; + drm_bridge_add(&panel_bridge->bridge);
return &panel_bridge->bridge; diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 1ba2d424a53f..c0f39dfbd071 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -179,6 +179,16 @@ struct drm_panel { * Panel entry in registry. */ struct list_head list; + + /** + * @prepare_upstream_first: + * + * The upstream controller should be prepared first, before the prepare + * for the panel is called. This is largely required for DSI panels + * where the DSI host controller should be initialised to LP-11 before + * the panel is powered up. + */ + bool prepare_upstream_first; };
void drm_panel_init(struct drm_panel *panel, struct device *dev,
On 04/03/2022 18:17, Dave Stevenson wrote:
Mapping to the drm_bridge flag pre_enable_upstream_first, add a new flag prepare_upstream_first to drm_panel to allow the panel driver to request that the upstream bridge should be pre_enabled before the panel prepare.
Signed-off-by: Dave Stevenson dave.stevenson@raspberrypi.com
Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
drivers/gpu/drm/bridge/panel.c | 3 +++ include/drm/drm_panel.h | 10 ++++++++++ 2 files changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index 5be057575183..2ea08b3ba326 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -234,6 +234,9 @@ struct drm_bridge *drm_panel_bridge_add_typed(struct drm_panel *panel, panel_bridge->bridge.ops = DRM_BRIDGE_OP_MODES; panel_bridge->bridge.type = connector_type;
panel_bridge->bridge.pre_enable_upstream_first =
panel->prepare_upstream_first;drm_bridge_add(&panel_bridge->bridge);
return &panel_bridge->bridge;
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 1ba2d424a53f..c0f39dfbd071 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -179,6 +179,16 @@ struct drm_panel { * Panel entry in registry. */ struct list_head list;
/**
* @prepare_upstream_first:** The upstream controller should be prepared first, before the prepare* for the panel is called. This is largely required for DSI panels* where the DSI host controller should be initialised to LP-11 before* the panel is powered up.*/bool prepare_upstream_first; };
void drm_panel_init(struct drm_panel *panel, struct device *dev,
The exact behaviour of DSI host controllers is not specified, therefore define it.
Signed-off-by: Dave Stevenson dave.stevenson@raspberrypi.com Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- Documentation/gpu/drm-kms-helpers.rst | 7 +++++++ drivers/gpu/drm/drm_bridge.c | 39 +++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+)
diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst index c3ce91eecbc1..362afdb867c6 100644 --- a/Documentation/gpu/drm-kms-helpers.rst +++ b/Documentation/gpu/drm-kms-helpers.rst @@ -185,6 +185,13 @@ Bridge Helper Reference .. kernel-doc:: drivers/gpu/drm/drm_bridge.c :export:
+MIPI-DSI bridge operation +------------------------- + +.. kernel-doc:: drivers/gpu/drm/drm_bridge.c + :doc: dsi bridge operations + + Bridge Connector Helper Reference ---------------------------------
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 70b513f5ce0d..32def1be682a 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -152,6 +152,45 @@ * situation when probing. */
+/** + * DOC: dsi bridge operations + * + * DSI host interfaces are expected to be implemented as bridges rather than + * encoders, however there are a few aspects of their operation that need to + * be defined in order to provide a consistent interface. + * + * A DSI host should keep the PHY powered down until the pre_enable operation is + * called. All lanes are in an undefined idle state up to this point, and it + * must not be assumed that it is LP-11. + * pre_enable should initialise the PHY, set the data lanes to LP-11, and the + * clock lane to either LP-11 or HS depending on the mode_flag + * %MIPI_DSI_CLOCK_NON_CONTINUOUS. + * + * Ordinarily the downstream bridge DSI peripheral pre_enable will have been + * called before the DSI host. If the DSI peripheral requires LP-11 and/or + * the clock lane to be in HS mode prior to pre_enable, then it can set the + * &pre_enable_upstream_first flag to request the pre_enable (and + * post_disable) order to be altered to enable the DSI host first. + * + * Either the CRTC being enabled, or the DSI host enable operation should switch + * the host to actively transmitting video on the data lanes. + * + * The reverse also applies. The DSI host disable operation or stopping the CRTC + * should stop transmitting video, and the data lanes should return to the LP-11 + * state. The DSI host &post_disable operation should disable the PHY. + * If the &pre_enable_upstream_first flag is set, then the DSI peripheral's + * bridge &post_disable will be called before the DSI host's post_disable. + * + * Whilst it is valid to call &host_transfer prior to pre_enable or after + * post_disable, the exact state of the lanes is undefined at this point. The + * DSI host should initialise the interface, transmit the data, and then disable + * the interface again. + * + * Ultra Low Power State (ULPS) is not explicitly supported by DRM. If + * implemented, it therefore needs to be handled entirely within the DSI Host + * driver. + */ + static DEFINE_MUTEX(bridge_lock); static LIST_HEAD(bridge_list);
On Fri, 4 Mar 2022 at 15:18, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
Hi All
A gentle ping on this series. Any comments on the approach? Thanks.
Changes from v1:
- New patch to refactor drm_bridge_chain_post_disable and drm_bridge_chain_pre_enable to reuse drm_atomic_bridge_chain_post_disable / drm_atomic_bridge_chain_pre_enable but with a NULL state.
- New patch that adds a pre_enable_upstream_first to drm_panel.
- changed from an OPS flag to a bool "pre_enable_upstream_first" in drm_bridge.
- Followed Andrzej's suggestion of using continue in the main loop to avoid needing 2 additional loops (one forward to find the last bridge wanting upstream first, and the second backwards again).
- Actioned Laurent's review comments on docs patch.
Original cover letter:
Hopefully I've cc'ed all those that have bashed this problem around previously, or are otherwise linked to DRM bridges.
There have been numerous discussions around how DSI support is currently broken as it doesn't support initialising the PHY to LP-11 and potentially the clock lane to HS prior to configuring the DSI peripheral. There is no op where the interface is initialised but HS video isn't also being sent. Currently you have:
- peripheral pre_enable (host not initialised yet)
- host pre_enable
- encoder enable
- host enable
- peripheral enable (video already running)
vc4 and exynos currently implement the DSI host as an encoder, and split the bridge_chain. This fails if you want to switch to being a bridge and/or use atomic calls as the state of all the elements split off are not added by drm_atomic_add_encoder_bridges.
dw-mipi-dsi[1] and now msm[2] use the mode_set hook to initialise the PHY, so the bridge/panel pre_enable can send commands. In their post_disable they then call the downstream bridge/panel post_disable op manually so that shutdown commands can be sent before shutting down the PHY. Nothing handles that fact, so the framework then continues down the bridge chain and calls the post_disable again, so we get unbalanced panel prepare/unprepare calls being reported [3].
There have been patches[4] proposing reversing the entire direction of pre_enable and post_disable, but that risks driving voltage into devices that have yet to be powered up. There have been discussions about adding either a pre_pre_enable, or adding a DSI host_op to initialise the host[5]. Both require significant reworking to all existing drivers in moving initialisation phases. We have patches that look like they may well be addressing race conditions in starting up a DSI peripheral[6].
This patch takes a hybrid of the two: an optional reversing of the order for specific links within the bridge chain within pre_enable and post_disable done within the drm_bridge framework. I'm more than happy to move where the flag exists in structures (currently as DRM_BRIDGE_OP_UPSTREAM_FIRST in drm_bridge_ops, but it isn't an op), but does this solve the problem posed? If not, then can you describe the actual scenario it doesn't cover? A DSI peripheral can set the flag to get the DSI host initialised first, and therefore it has a stable LP-11 state before pre_enable. Likewise the peripheral can still send shutdown commands prior to the DSI host being shut down in post_disable. It also handles the case where there are multiple devices in the chain that all want their upstream bridge enabled first, so should there be a DSI mux between host and peripheral, then it can still get the host to the correct state.
An example tree is at [7] which is drm-misc-next with these patches and then a conversion of vc4_dsi to use the atomic bridge functions (will be upstreamed once we're over this hurdle). It is working happily with the Toshiba TC358762 on a Raspberry Pi 7" panel. The same approach but on our vendor 5.15 tree[8] has also been tested successfully on a TI SN65DSI83 and LVDS panel.
Whilst here, I've also documented the expected behaviour of DSI hosts and peripherals to aid those who come along after.
Thanks Dave
[1] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/bridge/synopsy... [2] https://lists.freedesktop.org/archives/dri-devel/2022-January/337769.html [3] https://lists.freedesktop.org/archives/dri-devel/2021-December/333908.html [4] https://lists.freedesktop.org/archives/dri-devel/2021-October/328476.html [5] https://lists.freedesktop.org/archives/dri-devel/2021-October/325853.html [6] https://lists.freedesktop.org/archives/dri-devel/2022-February/341852.html [7] https://github.com/6by9/linux/tree/drm-misc-next-vc4_dsi [8] https://github.com/6by9/linux/tree/rpi-5.15.y-sn65dsi83
Dave Stevenson (4): drm/bridge: Remove duplication from drm_bridge and drm_atomic_bridge chains drm/bridge: Introduce pre_enable_upstream_first to alter bridge init order drm/panel: Add prepare_upstream_first flag to drm_panel drm/bridge: Document the expected behaviour of DSI host controllers
Documentation/gpu/drm-kms-helpers.rst | 7 ++ drivers/gpu/drm/bridge/panel.c | 3 + drivers/gpu/drm/drm_bridge.c | 181 ++++++++++++++++++++++++---------- include/drm/drm_bridge.h | 8 ++ include/drm/drm_panel.h | 10 ++ 5 files changed, 159 insertions(+), 50 deletions(-)
-- 2.7.4
On Fri, 18 Mar 2022 at 12:25, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
On Fri, 4 Mar 2022 at 15:18, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
Hi All
A gentle ping on this series. Any comments on the approach? Thanks.
I realise the merge window has just closed and therefore folks have been busy, but no responses on this after a month?
Do I give up and submit a patch to document that DSI is broken and no one cares?
Dave
Changes from v1:
- New patch to refactor drm_bridge_chain_post_disable and drm_bridge_chain_pre_enable to reuse drm_atomic_bridge_chain_post_disable / drm_atomic_bridge_chain_pre_enable but with a NULL state.
- New patch that adds a pre_enable_upstream_first to drm_panel.
- changed from an OPS flag to a bool "pre_enable_upstream_first" in drm_bridge.
- Followed Andrzej's suggestion of using continue in the main loop to avoid needing 2 additional loops (one forward to find the last bridge wanting upstream first, and the second backwards again).
- Actioned Laurent's review comments on docs patch.
Original cover letter:
Hopefully I've cc'ed all those that have bashed this problem around previously, or are otherwise linked to DRM bridges.
There have been numerous discussions around how DSI support is currently broken as it doesn't support initialising the PHY to LP-11 and potentially the clock lane to HS prior to configuring the DSI peripheral. There is no op where the interface is initialised but HS video isn't also being sent. Currently you have:
- peripheral pre_enable (host not initialised yet)
- host pre_enable
- encoder enable
- host enable
- peripheral enable (video already running)
vc4 and exynos currently implement the DSI host as an encoder, and split the bridge_chain. This fails if you want to switch to being a bridge and/or use atomic calls as the state of all the elements split off are not added by drm_atomic_add_encoder_bridges.
dw-mipi-dsi[1] and now msm[2] use the mode_set hook to initialise the PHY, so the bridge/panel pre_enable can send commands. In their post_disable they then call the downstream bridge/panel post_disable op manually so that shutdown commands can be sent before shutting down the PHY. Nothing handles that fact, so the framework then continues down the bridge chain and calls the post_disable again, so we get unbalanced panel prepare/unprepare calls being reported [3].
There have been patches[4] proposing reversing the entire direction of pre_enable and post_disable, but that risks driving voltage into devices that have yet to be powered up. There have been discussions about adding either a pre_pre_enable, or adding a DSI host_op to initialise the host[5]. Both require significant reworking to all existing drivers in moving initialisation phases. We have patches that look like they may well be addressing race conditions in starting up a DSI peripheral[6].
This patch takes a hybrid of the two: an optional reversing of the order for specific links within the bridge chain within pre_enable and post_disable done within the drm_bridge framework. I'm more than happy to move where the flag exists in structures (currently as DRM_BRIDGE_OP_UPSTREAM_FIRST in drm_bridge_ops, but it isn't an op), but does this solve the problem posed? If not, then can you describe the actual scenario it doesn't cover? A DSI peripheral can set the flag to get the DSI host initialised first, and therefore it has a stable LP-11 state before pre_enable. Likewise the peripheral can still send shutdown commands prior to the DSI host being shut down in post_disable. It also handles the case where there are multiple devices in the chain that all want their upstream bridge enabled first, so should there be a DSI mux between host and peripheral, then it can still get the host to the correct state.
An example tree is at [7] which is drm-misc-next with these patches and then a conversion of vc4_dsi to use the atomic bridge functions (will be upstreamed once we're over this hurdle). It is working happily with the Toshiba TC358762 on a Raspberry Pi 7" panel. The same approach but on our vendor 5.15 tree[8] has also been tested successfully on a TI SN65DSI83 and LVDS panel.
Whilst here, I've also documented the expected behaviour of DSI hosts and peripherals to aid those who come along after.
Thanks Dave
[1] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/bridge/synopsy... [2] https://lists.freedesktop.org/archives/dri-devel/2022-January/337769.html [3] https://lists.freedesktop.org/archives/dri-devel/2021-December/333908.html [4] https://lists.freedesktop.org/archives/dri-devel/2021-October/328476.html [5] https://lists.freedesktop.org/archives/dri-devel/2021-October/325853.html [6] https://lists.freedesktop.org/archives/dri-devel/2022-February/341852.html [7] https://github.com/6by9/linux/tree/drm-misc-next-vc4_dsi [8] https://github.com/6by9/linux/tree/rpi-5.15.y-sn65dsi83
Dave Stevenson (4): drm/bridge: Remove duplication from drm_bridge and drm_atomic_bridge chains drm/bridge: Introduce pre_enable_upstream_first to alter bridge init order drm/panel: Add prepare_upstream_first flag to drm_panel drm/bridge: Document the expected behaviour of DSI host controllers
Documentation/gpu/drm-kms-helpers.rst | 7 ++ drivers/gpu/drm/bridge/panel.c | 3 + drivers/gpu/drm/drm_bridge.c | 181 ++++++++++++++++++++++++---------- include/drm/drm_bridge.h | 8 ++ include/drm/drm_panel.h | 10 ++ 5 files changed, 159 insertions(+), 50 deletions(-)
-- 2.7.4
Hi Dave,
On 05.04.2022 13:43, Dave Stevenson wrote:
On Fri, 18 Mar 2022 at 12:25, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
On Fri, 4 Mar 2022 at 15:18, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
Hi All
A gentle ping on this series. Any comments on the approach? Thanks.
I realise the merge window has just closed and therefore folks have been busy, but no responses on this after a month?
Do I give up and submit a patch to document that DSI is broken and no one cares?
Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI DSIM bridge' thread, otherwise I would miss it since I'm not involved much in the DRM development.
This resolves most of the issues in the Exynos DSI and its recent conversion to the drm bridge framework. I've added the needed prepare_upstream_first flags to the panels and everything works fine without the bridge chain order hacks.
Feel free to add:
Tested-by: Marek Szyprowski m.szyprowski@samsung.com
The only remaining thing to resolve is the moment of enabling DSI host. The proper sequence is:
1. host power on, 2. device power on, 3. host init, 4. device init, 5. video enable.
#1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so far done in the first host transfer call, which usually happens in panel's prepare, then the #4 happens. Then video enable is done in the enable callbacks.
Jagan wants to move it to the dsi host pre_enable() to let it work with DSI bridges controlled over different interfaces (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.co... ). This however fails on Exynos with DSI panels, because when dsi's pre_enable is called, the dsi device is not yet powered. I've discussed this with Andrzej Hajda and we came to the conclusion that this can be resolved by adding the init() callback to the struct mipi_dsi_host_ops. Then DSI client (next bridge or panel) would call it after powering self on, but before sending any DSI commands in its pre_enable/prepare functions.
I've prepared a prototype of such solution. This approach finally resolved all the initialization issues! The bridge chain finally matches the hardware, no hack are needed, and everything is controlled by the DRM core. This prototype also includes the Jagan's patches, which add IMX support to Samsung DSIM. If one is interested, here is my git repo with all the PoC patches:
https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework
Best regards
On 5/11/22 16:58, Marek Szyprowski wrote:
Hi Dave,
On 05.04.2022 13:43, Dave Stevenson wrote:
On Fri, 18 Mar 2022 at 12:25, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
On Fri, 4 Mar 2022 at 15:18, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
Hi All
A gentle ping on this series. Any comments on the approach? Thanks.
I realise the merge window has just closed and therefore folks have been busy, but no responses on this after a month?
Do I give up and submit a patch to document that DSI is broken and no one cares?
Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI DSIM bridge' thread, otherwise I would miss it since I'm not involved much in the DRM development.
This resolves most of the issues in the Exynos DSI and its recent conversion to the drm bridge framework. I've added the needed prepare_upstream_first flags to the panels and everything works fine without the bridge chain order hacks.
Feel free to add:
Tested-by: Marek Szyprowski m.szyprowski@samsung.com
The only remaining thing to resolve is the moment of enabling DSI host. The proper sequence is:
- host power on, 2. device power on, 3. host init, 4. device init, 5.
video enable.
+CC Raphael, STM32MP1 has similar topic.
#1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so far done in the first host transfer call, which usually happens in panel's prepare, then the #4 happens. Then video enable is done in the enable callbacks.
Jagan wants to move it to the dsi host pre_enable() to let it work with DSI bridges controlled over different interfaces (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.co... ). This however fails on Exynos with DSI panels, because when dsi's pre_enable is called, the dsi device is not yet powered. I've discussed this with Andrzej Hajda and we came to the conclusion that this can be resolved by adding the init() callback to the struct mipi_dsi_host_ops. Then DSI client (next bridge or panel) would call it after powering self on, but before sending any DSI commands in its pre_enable/prepare functions.
I've prepared a prototype of such solution. This approach finally resolved all the initialization issues! The bridge chain finally matches the hardware, no hack are needed, and everything is controlled by the DRM core. This prototype also includes the Jagan's patches, which add IMX support to Samsung DSIM. If one is interested, here is my git repo with all the PoC patches:
https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework
Can you CC me on the iMX DSIM discussion/patches ? It seems I was left out of it all, even though I work with the iMX8M DRM stuff extensively.
On 11.05.2022 17:47, Marek Vasut wrote:
On 5/11/22 16:58, Marek Szyprowski wrote:
Hi Dave,
On 05.04.2022 13:43, Dave Stevenson wrote:
On Fri, 18 Mar 2022 at 12:25, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
On Fri, 4 Mar 2022 at 15:18, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
Hi All
A gentle ping on this series. Any comments on the approach? Thanks.
I realise the merge window has just closed and therefore folks have been busy, but no responses on this after a month?
Do I give up and submit a patch to document that DSI is broken and no one cares?
Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI DSIM bridge' thread, otherwise I would miss it since I'm not involved much in the DRM development.
This resolves most of the issues in the Exynos DSI and its recent conversion to the drm bridge framework. I've added the needed prepare_upstream_first flags to the panels and everything works fine without the bridge chain order hacks.
Feel free to add:
Tested-by: Marek Szyprowski m.szyprowski@samsung.com
The only remaining thing to resolve is the moment of enabling DSI host. The proper sequence is:
- host power on, 2. device power on, 3. host init, 4. device init, 5.
video enable.
+CC Raphael, STM32MP1 has similar topic.
#1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so far done in the first host transfer call, which usually happens in panel's prepare, then the #4 happens. Then video enable is done in the enable callbacks.
Jagan wants to move it to the dsi host pre_enable() to let it work with DSI bridges controlled over different interfaces (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.co...
). This however fails on Exynos with DSI panels, because when dsi's pre_enable is called, the dsi device is not yet powered. I've discussed this with Andrzej Hajda and we came to the conclusion that this can be resolved by adding the init() callback to the struct mipi_dsi_host_ops. Then DSI client (next bridge or panel) would call it after powering self on, but before sending any DSI commands in its pre_enable/prepare functions.
I've prepared a prototype of such solution. This approach finally resolved all the initialization issues! The bridge chain finally matches the hardware, no hack are needed, and everything is controlled by the DRM core. This prototype also includes the Jagan's patches, which add IMX support to Samsung DSIM. If one is interested, here is my git repo with all the PoC patches:
https://protect2.fireeye.com/v1/url?k=fc60b660-9deba379-fc613d2f-74fe485cbfe...
Can you CC me on the iMX DSIM discussion/patches ? It seems I was left out of it all, even though I work with the iMX8M DRM stuff extensively.
https://lore.kernel.org/all/20220504114021.33265-1-jagan@amarulasolutions.co...
Best regards
On Wed, May 11, 2022 at 9:17 PM Marek Vasut marex@denx.de wrote:
On 5/11/22 16:58, Marek Szyprowski wrote:
Hi Dave,
On 05.04.2022 13:43, Dave Stevenson wrote:
On Fri, 18 Mar 2022 at 12:25, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
On Fri, 4 Mar 2022 at 15:18, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
Hi All
A gentle ping on this series. Any comments on the approach? Thanks.
I realise the merge window has just closed and therefore folks have been busy, but no responses on this after a month?
Do I give up and submit a patch to document that DSI is broken and no one cares?
Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI DSIM bridge' thread, otherwise I would miss it since I'm not involved much in the DRM development.
This resolves most of the issues in the Exynos DSI and its recent conversion to the drm bridge framework. I've added the needed prepare_upstream_first flags to the panels and everything works fine without the bridge chain order hacks.
Feel free to add:
Tested-by: Marek Szyprowski m.szyprowski@samsung.com
The only remaining thing to resolve is the moment of enabling DSI host. The proper sequence is:
- host power on, 2. device power on, 3. host init, 4. device init, 5.
video enable.
+CC Raphael, STM32MP1 has similar topic.
#1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so far done in the first host transfer call, which usually happens in panel's prepare, then the #4 happens. Then video enable is done in the enable callbacks.
Jagan wants to move it to the dsi host pre_enable() to let it work with DSI bridges controlled over different interfaces (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.co... ). This however fails on Exynos with DSI panels, because when dsi's pre_enable is called, the dsi device is not yet powered. I've discussed this with Andrzej Hajda and we came to the conclusion that this can be resolved by adding the init() callback to the struct mipi_dsi_host_ops. Then DSI client (next bridge or panel) would call it after powering self on, but before sending any DSI commands in its pre_enable/prepare functions.
I've prepared a prototype of such solution. This approach finally resolved all the initialization issues! The bridge chain finally matches the hardware, no hack are needed, and everything is controlled by the DRM core. This prototype also includes the Jagan's patches, which add IMX support to Samsung DSIM. If one is interested, here is my git repo with all the PoC patches:
https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework
Can you CC me on the iMX DSIM discussion/patches ? It seems I was left out of it all, even though I work with the iMX8M DRM stuff extensively.
Sorry, this is not intentional. I added you and many others in RFC and subsequently, I have added in the next versions whoever responds to the previous. I will do it in the next version DSIM.
Thanks, Jagan.
Hi Marek.
On Wed, 11 May 2022 at 15:58, Marek Szyprowski m.szyprowski@samsung.com wrote:
Hi Dave,
On 05.04.2022 13:43, Dave Stevenson wrote:
On Fri, 18 Mar 2022 at 12:25, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
On Fri, 4 Mar 2022 at 15:18, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
Hi All
A gentle ping on this series. Any comments on the approach? Thanks.
I realise the merge window has just closed and therefore folks have been busy, but no responses on this after a month?
Do I give up and submit a patch to document that DSI is broken and no one cares?
Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI DSIM bridge' thread, otherwise I would miss it since I'm not involved much in the DRM development.
This resolves most of the issues in the Exynos DSI and its recent conversion to the drm bridge framework. I've added the needed prepare_upstream_first flags to the panels and everything works fine without the bridge chain order hacks.
Feel free to add:
Tested-by: Marek Szyprowski m.szyprowski@samsung.com
Thanks for testing it. I was almost at the stage of abandoning the patch set.
The only remaining thing to resolve is the moment of enabling DSI host. The proper sequence is:
- host power on, 2. device power on, 3. host init, 4. device init, 5.
video enable.
#1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so far done in the first host transfer call, which usually happens in panel's prepare, then the #4 happens. Then video enable is done in the enable callbacks.
What's your definition of host power on and host init here? What state are you defining the DSI interface to be in after each operation?
Jagan wants to move it to the dsi host pre_enable() to let it work with DSI bridges controlled over different interfaces (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.co... ).
I think I'm in agreement with Jagan. As documented in patch 4/4: + * A DSI host should keep the PHY powered down until the pre_enable operation is + * called. All lanes are in an undefined idle state up to this point, and it + * must not be assumed that it is LP-11. + * pre_enable should initialise the PHY, set the data lanes to LP-11, and the + * clock lane to either LP-11 or HS depending on the mode_flag + * %MIPI_DSI_CLOCK_NON_CONTINUOUS.
This however fails on Exynos with DSI panels, because when dsi's pre_enable is called, the dsi device is not yet powered.
Sorry, I'm not following what the issue is here? DSI lanes being at LP-11 when the sink isn't powered, so potential for leakage through the device? In which case the device should NOT set pre_enable_upstream first, and the host gets powered up and down with each host_transfer call the device makes in pre_enable.
(Whilst I can't claim that I know of every single DSI device, most datasheets I've encountered have required LP-11 on the lanes before powering up the device).
I've discussed this with Andrzej Hajda and we came to the conclusion that this can be resolved by adding the init() callback to the struct mipi_dsi_host_ops. Then DSI client (next bridge or panel) would call it after powering self on, but before sending any DSI commands in its pre_enable/prepare functions.
You may as well have a mipi_dsi_host_ops call to fully control the DSI host state, and forget about changing the pre_enable/post_disable order. However it involves even more changes to all the panel and bridge drivers.
If you've added an init hook, don't you also need a deinint hook to ensure that the host is restored to the "power on but not inited" state before device power off? Currently it feels unbalanced, but largely as I don't know exactly what you're defining that power on state to be.
Dave
I've prepared a prototype of such solution. This approach finally resolved all the initialization issues! The bridge chain finally matches the hardware, no hack are needed, and everything is controlled by the DRM core. This prototype also includes the Jagan's patches, which add IMX support to Samsung DSIM. If one is interested, here is my git repo with all the PoC patches:
https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework
Hi Dave,
On 11.05.2022 17:47, Dave Stevenson wrote:
On Wed, 11 May 2022 at 15:58, Marek Szyprowski m.szyprowski@samsung.com wrote:
On 05.04.2022 13:43, Dave Stevenson wrote:
On Fri, 18 Mar 2022 at 12:25, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
On Fri, 4 Mar 2022 at 15:18, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
Hi All
A gentle ping on this series. Any comments on the approach? Thanks.
I realise the merge window has just closed and therefore folks have been busy, but no responses on this after a month?
Do I give up and submit a patch to document that DSI is broken and no one cares?
Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI DSIM bridge' thread, otherwise I would miss it since I'm not involved much in the DRM development.
This resolves most of the issues in the Exynos DSI and its recent conversion to the drm bridge framework. I've added the needed prepare_upstream_first flags to the panels and everything works fine without the bridge chain order hacks.
Feel free to add:
Tested-by: Marek Szyprowski m.szyprowski@samsung.com
Thanks for testing it. I was almost at the stage of abandoning the patch set.
The only remaining thing to resolve is the moment of enabling DSI host. The proper sequence is:
- host power on, 2. device power on, 3. host init, 4. device init, 5.
video enable.
#1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so far done in the first host transfer call, which usually happens in panel's prepare, then the #4 happens. Then video enable is done in the enable callbacks.
What's your definition of host power on and host init here? What state are you defining the DSI interface to be in after each operation?
Well, lets start from the point that I'm not a DSI specialist nor I'm not the exynos-dsi author. I just played a bit with the code trying to restore proper driver operation on the various Exynos based boards I have.
By the host/device power on I mean enabling their power regulators. By host init I mean executing the samsung_dsim_init() function, which basically sets the lp-11 state if I understand it right.
Jagan wants to move it to the dsi host pre_enable() to let it work with DSI bridges controlled over different interfaces (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.co... ).
I think I'm in agreement with Jagan. As documented in patch 4/4:
- A DSI host should keep the PHY powered down until the pre_enable
operation is
- called. All lanes are in an undefined idle state up to this point, and it
- must not be assumed that it is LP-11.
- pre_enable should initialise the PHY, set the data lanes to LP-11, and the
- clock lane to either LP-11 or HS depending on the mode_flag
- %MIPI_DSI_CLOCK_NON_CONTINUOUS.
Right, this theory makes sense.
However Exynos DSI for some reasons did the host initialization in the first call of the samsung_dsim_host_transfer(). If I moved the host initialization to pre_enable (before powering the panel on), executing DSI commands failed (timeout). This issue happens on all boards I have access (Trats, Trats2, Arndale, TM2e), so this must be an issue with Exynos DSI host itself not related to particular panel/bridge.
If I call samsung_dsim_init() once again, before issuing the first DSI command, then everything works fine. I've tried to check which part of that function is needed to be executed before transferring the commands, but it turned out that the complete host reset and (re)configuration is necessary. It looks that the initialization will need to be done twice, first time in the pre_enable to satisfy Jagan case, then on the first dsi transfer to make it work with real DSI panels.
Here is a git repo with such change: https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework-v2
This however fails on Exynos with DSI panels, because when dsi's pre_enable is called, the dsi device is not yet powered.
Sorry, I'm not following what the issue is here? DSI lanes being at LP-11 when the sink isn't powered, so potential for leakage through the device?
I also have no idea why sending DSI commands fails if host is initialized without device being powered up first. Maybe powering it later causes some glitches on the lines? However it looks doing the initialization again on the first transfer is enough to fix it.
In which case the device should NOT set pre_enable_upstream first, and the host gets powered up and down with each host_transfer call the device makes in pre_enable.
Doing the initialization on each host_transfer also is not an option, because in such case the panel is not initialized properly. I get no errors, but also there is no valid display on the panel in such case.
(Whilst I can't claim that I know of every single DSI device, most datasheets I've encountered have required LP-11 on the lanes before powering up the device).
I've discussed this with Andrzej Hajda and we came to the conclusion that this can be resolved by adding the init() callback to the struct mipi_dsi_host_ops. Then DSI client (next bridge or panel) would call it after powering self on, but before sending any DSI commands in its pre_enable/prepare functions.
You may as well have a mipi_dsi_host_ops call to fully control the DSI host state, and forget about changing the pre_enable/post_disable order. However it involves even more changes to all the panel and bridge drivers.
True. Although setting prepare_upstream_first/pre_enable_upstream_first flags also requires to revisit all the dsi panels and bridges.
It looks that I've focused too much on finding a single place of the dsi initialization, what resulted in that host_init callback. I can live without it, doing the initialization twice.
If you've added an init hook, don't you also need a deinint hook to ensure that the host is restored to the "power on but not inited" state before device power off? Currently it feels unbalanced, but largely as I don't know exactly what you're defining that power on state to be.
So far I had no use case for that deinit hook.
Best regards
On 18.05.2022 16:05, Marek Szyprowski wrote:
Hi Dave,
On 11.05.2022 17:47, Dave Stevenson wrote:
On Wed, 11 May 2022 at 15:58, Marek Szyprowski m.szyprowski@samsung.com wrote:
On 05.04.2022 13:43, Dave Stevenson wrote:
On Fri, 18 Mar 2022 at 12:25, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
On Fri, 4 Mar 2022 at 15:18, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
Hi All
A gentle ping on this series. Any comments on the approach? Thanks.
I realise the merge window has just closed and therefore folks have been busy, but no responses on this after a month?
Do I give up and submit a patch to document that DSI is broken and no one cares?
Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI DSIM bridge' thread, otherwise I would miss it since I'm not involved much in the DRM development.
This resolves most of the issues in the Exynos DSI and its recent conversion to the drm bridge framework. I've added the needed prepare_upstream_first flags to the panels and everything works fine without the bridge chain order hacks.
Feel free to add:
Tested-by: Marek Szyprowski m.szyprowski@samsung.com
Thanks for testing it. I was almost at the stage of abandoning the patch set.
The only remaining thing to resolve is the moment of enabling DSI host. The proper sequence is:
- host power on, 2. device power on, 3. host init, 4. device init, 5.
video enable.
#1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so far done in the first host transfer call, which usually happens in panel's prepare, then the #4 happens. Then video enable is done in the enable callbacks.
What's your definition of host power on and host init here? What state are you defining the DSI interface to be in after each operation?
Well, lets start from the point that I'm not a DSI specialist nor I'm not the exynos-dsi author. I just played a bit with the code trying to restore proper driver operation on the various Exynos based boards I have.
By the host/device power on I mean enabling their power regulators. By host init I mean executing the samsung_dsim_init() function, which basically sets the lp-11 state if I understand it right.
Jagan wants to move it to the dsi host pre_enable() to let it work with DSI bridges controlled over different interfaces (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.co... ).
I think I'm in agreement with Jagan. As documented in patch 4/4:
- A DSI host should keep the PHY powered down until the pre_enable
operation is
- called. All lanes are in an undefined idle state up to this point, and it
- must not be assumed that it is LP-11.
- pre_enable should initialise the PHY, set the data lanes to LP-11, and the
- clock lane to either LP-11 or HS depending on the mode_flag
- %MIPI_DSI_CLOCK_NON_CONTINUOUS.
Right, this theory makes sense.
However Exynos DSI for some reasons did the host initialization in the first call of the samsung_dsim_host_transfer().
It was a way to fit exynos DSI order of initialization to the frameworks present at the time it was written: exynos_dsi was an drm_encoder, and it was connected to drm_panel (DSI controlled panel). 1. In exynos_dsi_enable host was powered on then drm_panel_prepare was called. 2. drm_panel_prepare called .prepare callback of the panel, which powered on the panel and then requested dsi transfers to initialize panel. 3. 1st dsi transfer performed dsi host init (LP-11), after that all transfers started by panel performed panel initialization. 4. after that control goes back to exynos_dsi_enable. 5. exynos_dsi_enable starts video transfer and notify panel about it via drm_panel_enable. 6. .enable callback of the panel starts displaying frames (after some delay).
This construction allowed to keep proper order of hw initialization: host power on, panel power on, host init, panel init, start video transmission, start display frames. Almost all elements of this sequence are enforced by DSI specification (at least for devices controlled via dsi). The only thing which I am not sure is placement of "panel power on". It does not seem to be a part of DSI spec, but I am not 100% sure. It could be specific to platform (mis)configuration (regulators, level shifters, ...), or just dsi host init sequence tries to do too much. I wonder if dropping checking stop state in exynos_dsi wouldn't solve the mystery [1], or moving it to 1st transfer, maybe with(or w/o) subsequent code. Does IMX have some 'vendor code' of DSI host to compare with?
[1]: https://elixir.bootlin.com/linux/v5.15.1/source/drivers/gpu/drm/exynos/exyno...
Regards Andrzej
If I moved the host initialization to pre_enable (before powering the panel on), executing DSI commands failed (timeout). This issue happens on all boards I have access (Trats, Trats2, Arndale, TM2e), so this must be an issue with Exynos DSI host itself not related to particular panel/bridge.
If I call samsung_dsim_init() once again, before issuing the first DSI command, then everything works fine. I've tried to check which part of that function is needed to be executed before transferring the commands, but it turned out that the complete host reset and (re)configuration is necessary. It looks that the initialization will need to be done twice, first time in the pre_enable to satisfy Jagan case, then on the first dsi transfer to make it work with real DSI panels.
Here is a git repo with such change: https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework-v2
This however fails on Exynos with DSI panels, because when dsi's pre_enable is called, the dsi device is not yet powered.
Sorry, I'm not following what the issue is here? DSI lanes being at LP-11 when the sink isn't powered, so potential for leakage through the device?
I also have no idea why sending DSI commands fails if host is initialized without device being powered up first. Maybe powering it later causes some glitches on the lines? However it looks doing the initialization again on the first transfer is enough to fix it.
In which case the device should NOT set pre_enable_upstream first, and the host gets powered up and down with each host_transfer call the device makes in pre_enable.
Doing the initialization on each host_transfer also is not an option, because in such case the panel is not initialized properly. I get no errors, but also there is no valid display on the panel in such case.
(Whilst I can't claim that I know of every single DSI device, most datasheets I've encountered have required LP-11 on the lanes before powering up the device).
I've discussed this with Andrzej Hajda and we came to the conclusion that this can be resolved by adding the init() callback to the struct mipi_dsi_host_ops. Then DSI client (next bridge or panel) would call it after powering self on, but before sending any DSI commands in its pre_enable/prepare functions.
You may as well have a mipi_dsi_host_ops call to fully control the DSI host state, and forget about changing the pre_enable/post_disable order. However it involves even more changes to all the panel and bridge drivers.
True. Although setting prepare_upstream_first/pre_enable_upstream_first flags also requires to revisit all the dsi panels and bridges.
It looks that I've focused too much on finding a single place of the dsi initialization, what resulted in that host_init callback. I can live without it, doing the initialization twice.
If you've added an init hook, don't you also need a deinint hook to ensure that the host is restored to the "power on but not inited" state before device power off? Currently it feels unbalanced, but largely as I don't know exactly what you're defining that power on state to be.
So far I had no use case for that deinit hook.
Best regards
Hi Marek and Andrzej
On Wed, 18 May 2022 at 23:53, Andrzej Hajda andrzej.hajda@intel.com wrote:
On 18.05.2022 16:05, Marek Szyprowski wrote:
Hi Dave,
On 11.05.2022 17:47, Dave Stevenson wrote:
On Wed, 11 May 2022 at 15:58, Marek Szyprowski m.szyprowski@samsung.com wrote:
On 05.04.2022 13:43, Dave Stevenson wrote:
On Fri, 18 Mar 2022 at 12:25, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
On Fri, 4 Mar 2022 at 15:18, Dave Stevenson dave.stevenson@raspberrypi.com wrote: > Hi All A gentle ping on this series. Any comments on the approach? Thanks.
I realise the merge window has just closed and therefore folks have been busy, but no responses on this after a month?
Do I give up and submit a patch to document that DSI is broken and no one cares?
Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI DSIM bridge' thread, otherwise I would miss it since I'm not involved much in the DRM development.
This resolves most of the issues in the Exynos DSI and its recent conversion to the drm bridge framework. I've added the needed prepare_upstream_first flags to the panels and everything works fine without the bridge chain order hacks.
Feel free to add:
Tested-by: Marek Szyprowski m.szyprowski@samsung.com
Thanks for testing it. I was almost at the stage of abandoning the patch set.
The only remaining thing to resolve is the moment of enabling DSI host. The proper sequence is:
- host power on, 2. device power on, 3. host init, 4. device init, 5.
video enable.
#1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so far done in the first host transfer call, which usually happens in panel's prepare, then the #4 happens. Then video enable is done in the enable callbacks.
What's your definition of host power on and host init here? What state are you defining the DSI interface to be in after each operation?
Well, lets start from the point that I'm not a DSI specialist nor I'm not the exynos-dsi author. I just played a bit with the code trying to restore proper driver operation on the various Exynos based boards I have.
Is there any such thing as a DSI specialist? It seems to have many nasty corners that very few can really claim to fully understand (I don't claim to be an expert in it!).
By the host/device power on I mean enabling their power regulators. By host init I mean executing the samsung_dsim_init() function, which basically sets the lp-11 state if I understand it right.
Jagan wants to move it to the dsi host pre_enable() to let it work with DSI bridges controlled over different interfaces (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.co... ).
I think I'm in agreement with Jagan. As documented in patch 4/4:
- A DSI host should keep the PHY powered down until the pre_enable
operation is
- called. All lanes are in an undefined idle state up to this point, and it
- must not be assumed that it is LP-11.
- pre_enable should initialise the PHY, set the data lanes to LP-11, and the
- clock lane to either LP-11 or HS depending on the mode_flag
- %MIPI_DSI_CLOCK_NON_CONTINUOUS.
Right, this theory makes sense.
However Exynos DSI for some reasons did the host initialization in the first call of the samsung_dsim_host_transfer().
It was a way to fit exynos DSI order of initialization to the frameworks present at the time it was written: exynos_dsi was an drm_encoder, and it was connected to drm_panel (DSI controlled panel).
- In exynos_dsi_enable host was powered on then drm_panel_prepare was
called. 2. drm_panel_prepare called .prepare callback of the panel, which powered on the panel and then requested dsi transfers to initialize panel. 3. 1st dsi transfer performed dsi host init (LP-11), after that all transfers started by panel performed panel initialization. 4. after that control goes back to exynos_dsi_enable. 5. exynos_dsi_enable starts video transfer and notify panel about it via drm_panel_enable. 6. .enable callback of the panel starts displaying frames (after some delay).
This construction allowed to keep proper order of hw initialization: host power on, panel power on, host init, panel init, start video transmission, start display frames. Almost all elements of this sequence are enforced by DSI specification (at least for devices controlled via dsi). The only thing which I am not sure is placement of "panel power on". It does not seem to be a part of DSI spec, but I am not 100% sure. It could be specific to platform (mis)configuration (regulators, level shifters, ...), or just dsi host init sequence tries to do too much. I wonder if dropping checking stop state in exynos_dsi wouldn't solve the mystery [1], or moving it to 1st transfer, maybe with(or w/o) subsequent code.
I was thinking along similar lines when I read Marek's email.
If the panel is powered off then it may well be pulling the lanes down, and so the host can not achieve LP-11. However at the pre_enable stage there is no requirement to achieve LP-11 as there is no data being passed such that bus contention needs to be detected. Moving that test to host_transfer, where we are potentially reading data and there is a definite need to check for contention, would certainly be a useful test. I see that registers DSIM_ESCMODE_REG and DSIM_TIMEOUT_REG that are only set after the lanes are viewed as being in the "right" state, so presumably those are not being written with things reordered. (The return value of exynos_dsi_init_link is not checked, so that won't make any difference). Potentially test the lane state at enable too, although one could argue that detecting contention when sending video is of limited benefit as the display may be able to receive the data even if the host sees issues.
I've had a conversation with my hardware colleagues and they see no reason for concern for having LP-11 driven from the host with the slave powered down.
Checking the MIPI D-PHY spec v1.1 section 6.11 Initialization. State "Slave off", Line Levels "Any", so the slave is meant to handle anything when powered down. Section 4.2 Mandatory Functionality, "All functionality that is specified in this document and which is not explicitly stated in Section 5.5 shall be implemented for all D-PHY configurations." So that is not optional, but does rely on implementers having followed the spec.
Dave
Does IMX have some 'vendor code' of DSI host to compare with?
Regards Andrzej
If I moved the host initialization to pre_enable (before powering the panel on), executing DSI commands failed (timeout). This issue happens on all boards I have access (Trats, Trats2, Arndale, TM2e), so this must be an issue with Exynos DSI host itself not related to particular panel/bridge.
If I call samsung_dsim_init() once again, before issuing the first DSI command, then everything works fine. I've tried to check which part of that function is needed to be executed before transferring the commands, but it turned out that the complete host reset and (re)configuration is necessary. It looks that the initialization will need to be done twice, first time in the pre_enable to satisfy Jagan case, then on the first dsi transfer to make it work with real DSI panels.
Here is a git repo with such change: https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework-v2
This however fails on Exynos with DSI panels, because when dsi's pre_enable is called, the dsi device is not yet powered.
Sorry, I'm not following what the issue is here? DSI lanes being at LP-11 when the sink isn't powered, so potential for leakage through the device?
I also have no idea why sending DSI commands fails if host is initialized without device being powered up first. Maybe powering it later causes some glitches on the lines? However it looks doing the initialization again on the first transfer is enough to fix it.
In which case the device should NOT set pre_enable_upstream first, and the host gets powered up and down with each host_transfer call the device makes in pre_enable.
Doing the initialization on each host_transfer also is not an option, because in such case the panel is not initialized properly. I get no errors, but also there is no valid display on the panel in such case.
(Whilst I can't claim that I know of every single DSI device, most datasheets I've encountered have required LP-11 on the lanes before powering up the device).
I've discussed this with Andrzej Hajda and we came to the conclusion that this can be resolved by adding the init() callback to the struct mipi_dsi_host_ops. Then DSI client (next bridge or panel) would call it after powering self on, but before sending any DSI commands in its pre_enable/prepare functions.
You may as well have a mipi_dsi_host_ops call to fully control the DSI host state, and forget about changing the pre_enable/post_disable order. However it involves even more changes to all the panel and bridge drivers.
True. Although setting prepare_upstream_first/pre_enable_upstream_first flags also requires to revisit all the dsi panels and bridges.
It looks that I've focused too much on finding a single place of the dsi initialization, what resulted in that host_init callback. I can live without it, doing the initialization twice.
If you've added an init hook, don't you also need a deinint hook to ensure that the host is restored to the "power on but not inited" state before device power off? Currently it feels unbalanced, but largely as I don't know exactly what you're defining that power on state to be.
So far I had no use case for that deinit hook.
Best regards
Hi All.
This series has been kicking around since March now. Any chance of a review by any of the bridge maintainers or other DSI maintainers?
Converting Exynos DSI to the bridge framework seems to have issues, but that is a separate thing to this patch set. (It looks to be nearly there, but the check of stop state at init is not required by the DSI spec, and is likely to be tripping things up as it then doesn't set some registers. Contention only needs to be checked for before sending data).
Thanks Dave
On Thu, 19 May 2022 at 13:56, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
Hi Marek and Andrzej
On Wed, 18 May 2022 at 23:53, Andrzej Hajda andrzej.hajda@intel.com wrote:
On 18.05.2022 16:05, Marek Szyprowski wrote:
Hi Dave,
On 11.05.2022 17:47, Dave Stevenson wrote:
On Wed, 11 May 2022 at 15:58, Marek Szyprowski m.szyprowski@samsung.com wrote:
On 05.04.2022 13:43, Dave Stevenson wrote:
On Fri, 18 Mar 2022 at 12:25, Dave Stevenson dave.stevenson@raspberrypi.com wrote: > On Fri, 4 Mar 2022 at 15:18, Dave Stevenson > dave.stevenson@raspberrypi.com wrote: >> Hi All > A gentle ping on this series. Any comments on the approach? > Thanks. I realise the merge window has just closed and therefore folks have been busy, but no responses on this after a month?
Do I give up and submit a patch to document that DSI is broken and no one cares?
Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI DSIM bridge' thread, otherwise I would miss it since I'm not involved much in the DRM development.
This resolves most of the issues in the Exynos DSI and its recent conversion to the drm bridge framework. I've added the needed prepare_upstream_first flags to the panels and everything works fine without the bridge chain order hacks.
Feel free to add:
Tested-by: Marek Szyprowski m.szyprowski@samsung.com
Thanks for testing it. I was almost at the stage of abandoning the patch set.
The only remaining thing to resolve is the moment of enabling DSI host. The proper sequence is:
- host power on, 2. device power on, 3. host init, 4. device init, 5.
video enable.
#1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so far done in the first host transfer call, which usually happens in panel's prepare, then the #4 happens. Then video enable is done in the enable callbacks.
What's your definition of host power on and host init here? What state are you defining the DSI interface to be in after each operation?
Well, lets start from the point that I'm not a DSI specialist nor I'm not the exynos-dsi author. I just played a bit with the code trying to restore proper driver operation on the various Exynos based boards I have.
Is there any such thing as a DSI specialist? It seems to have many nasty corners that very few can really claim to fully understand (I don't claim to be an expert in it!).
By the host/device power on I mean enabling their power regulators. By host init I mean executing the samsung_dsim_init() function, which basically sets the lp-11 state if I understand it right.
Jagan wants to move it to the dsi host pre_enable() to let it work with DSI bridges controlled over different interfaces (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.co... ).
I think I'm in agreement with Jagan. As documented in patch 4/4:
- A DSI host should keep the PHY powered down until the pre_enable
operation is
- called. All lanes are in an undefined idle state up to this point, and it
- must not be assumed that it is LP-11.
- pre_enable should initialise the PHY, set the data lanes to LP-11, and the
- clock lane to either LP-11 or HS depending on the mode_flag
- %MIPI_DSI_CLOCK_NON_CONTINUOUS.
Right, this theory makes sense.
However Exynos DSI for some reasons did the host initialization in the first call of the samsung_dsim_host_transfer().
It was a way to fit exynos DSI order of initialization to the frameworks present at the time it was written: exynos_dsi was an drm_encoder, and it was connected to drm_panel (DSI controlled panel).
- In exynos_dsi_enable host was powered on then drm_panel_prepare was
called. 2. drm_panel_prepare called .prepare callback of the panel, which powered on the panel and then requested dsi transfers to initialize panel. 3. 1st dsi transfer performed dsi host init (LP-11), after that all transfers started by panel performed panel initialization. 4. after that control goes back to exynos_dsi_enable. 5. exynos_dsi_enable starts video transfer and notify panel about it via drm_panel_enable. 6. .enable callback of the panel starts displaying frames (after some delay).
This construction allowed to keep proper order of hw initialization: host power on, panel power on, host init, panel init, start video transmission, start display frames. Almost all elements of this sequence are enforced by DSI specification (at least for devices controlled via dsi). The only thing which I am not sure is placement of "panel power on". It does not seem to be a part of DSI spec, but I am not 100% sure. It could be specific to platform (mis)configuration (regulators, level shifters, ...), or just dsi host init sequence tries to do too much. I wonder if dropping checking stop state in exynos_dsi wouldn't solve the mystery [1], or moving it to 1st transfer, maybe with(or w/o) subsequent code.
I was thinking along similar lines when I read Marek's email.
If the panel is powered off then it may well be pulling the lanes down, and so the host can not achieve LP-11. However at the pre_enable stage there is no requirement to achieve LP-11 as there is no data being passed such that bus contention needs to be detected. Moving that test to host_transfer, where we are potentially reading data and there is a definite need to check for contention, would certainly be a useful test. I see that registers DSIM_ESCMODE_REG and DSIM_TIMEOUT_REG that are only set after the lanes are viewed as being in the "right" state, so presumably those are not being written with things reordered. (The return value of exynos_dsi_init_link is not checked, so that won't make any difference). Potentially test the lane state at enable too, although one could argue that detecting contention when sending video is of limited benefit as the display may be able to receive the data even if the host sees issues.
I've had a conversation with my hardware colleagues and they see no reason for concern for having LP-11 driven from the host with the slave powered down.
Checking the MIPI D-PHY spec v1.1 section 6.11 Initialization. State "Slave off", Line Levels "Any", so the slave is meant to handle anything when powered down. Section 4.2 Mandatory Functionality, "All functionality that is specified in this document and which is not explicitly stated in Section 5.5 shall be implemented for all D-PHY configurations." So that is not optional, but does rely on implementers having followed the spec.
Dave
Does IMX have some 'vendor code' of DSI host to compare with?
Regards Andrzej
If I moved the host initialization to pre_enable (before powering the panel on), executing DSI commands failed (timeout). This issue happens on all boards I have access (Trats, Trats2, Arndale, TM2e), so this must be an issue with Exynos DSI host itself not related to particular panel/bridge.
If I call samsung_dsim_init() once again, before issuing the first DSI command, then everything works fine. I've tried to check which part of that function is needed to be executed before transferring the commands, but it turned out that the complete host reset and (re)configuration is necessary. It looks that the initialization will need to be done twice, first time in the pre_enable to satisfy Jagan case, then on the first dsi transfer to make it work with real DSI panels.
Here is a git repo with such change: https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework-v2
This however fails on Exynos with DSI panels, because when dsi's pre_enable is called, the dsi device is not yet powered.
Sorry, I'm not following what the issue is here? DSI lanes being at LP-11 when the sink isn't powered, so potential for leakage through the device?
I also have no idea why sending DSI commands fails if host is initialized without device being powered up first. Maybe powering it later causes some glitches on the lines? However it looks doing the initialization again on the first transfer is enough to fix it.
In which case the device should NOT set pre_enable_upstream first, and the host gets powered up and down with each host_transfer call the device makes in pre_enable.
Doing the initialization on each host_transfer also is not an option, because in such case the panel is not initialized properly. I get no errors, but also there is no valid display on the panel in such case.
(Whilst I can't claim that I know of every single DSI device, most datasheets I've encountered have required LP-11 on the lanes before powering up the device).
I've discussed this with Andrzej Hajda and we came to the conclusion that this can be resolved by adding the init() callback to the struct mipi_dsi_host_ops. Then DSI client (next bridge or panel) would call it after powering self on, but before sending any DSI commands in its pre_enable/prepare functions.
You may as well have a mipi_dsi_host_ops call to fully control the DSI host state, and forget about changing the pre_enable/post_disable order. However it involves even more changes to all the panel and bridge drivers.
True. Although setting prepare_upstream_first/pre_enable_upstream_first flags also requires to revisit all the dsi panels and bridges.
It looks that I've focused too much on finding a single place of the dsi initialization, what resulted in that host_init callback. I can live without it, doing the initialization twice.
If you've added an init hook, don't you also need a deinint hook to ensure that the host is restored to the "power on but not inited" state before device power off? Currently it feels unbalanced, but largely as I don't know exactly what you're defining that power on state to be.
So far I had no use case for that deinit hook.
Best regards
On Wed, Jun 8, 2022 at 4:19 PM Dave Stevenson dave.stevenson@raspberrypi.com wrote:
Hi All.
This series has been kicking around since March now. Any chance of a review by any of the bridge maintainers or other DSI maintainers?
Converting Exynos DSI to the bridge framework seems to have issues, but that is a separate thing to this patch set.
Correct.
(It looks to be nearly there, but the check of stop state at init is not required by the DSI spec, and is likely to be tripping things up as it then doesn't set some registers. Contention only needs to be checked for before sending data).
I might take a look at this weekend and see if I can give some comments. thanks.
Jagan.
Hi Marek,
On Wed, May 18, 2022 at 7:35 PM Marek Szyprowski m.szyprowski@samsung.com wrote:
Hi Dave,
On 11.05.2022 17:47, Dave Stevenson wrote:
On Wed, 11 May 2022 at 15:58, Marek Szyprowski m.szyprowski@samsung.com wrote:
On 05.04.2022 13:43, Dave Stevenson wrote:
On Fri, 18 Mar 2022 at 12:25, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
On Fri, 4 Mar 2022 at 15:18, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
Hi All
A gentle ping on this series. Any comments on the approach? Thanks.
I realise the merge window has just closed and therefore folks have been busy, but no responses on this after a month?
Do I give up and submit a patch to document that DSI is broken and no one cares?
Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI DSIM bridge' thread, otherwise I would miss it since I'm not involved much in the DRM development.
This resolves most of the issues in the Exynos DSI and its recent conversion to the drm bridge framework. I've added the needed prepare_upstream_first flags to the panels and everything works fine without the bridge chain order hacks.
Feel free to add:
Tested-by: Marek Szyprowski m.szyprowski@samsung.com
Thanks for testing it. I was almost at the stage of abandoning the patch set.
The only remaining thing to resolve is the moment of enabling DSI host. The proper sequence is:
- host power on, 2. device power on, 3. host init, 4. device init, 5.
video enable.
#1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so far done in the first host transfer call, which usually happens in panel's prepare, then the #4 happens. Then video enable is done in the enable callbacks.
What's your definition of host power on and host init here? What state are you defining the DSI interface to be in after each operation?
Well, lets start from the point that I'm not a DSI specialist nor I'm not the exynos-dsi author. I just played a bit with the code trying to restore proper driver operation on the various Exynos based boards I have.
By the host/device power on I mean enabling their power regulators. By host init I mean executing the samsung_dsim_init() function, which basically sets the lp-11 state if I understand it right.
Jagan wants to move it to the dsi host pre_enable() to let it work with DSI bridges controlled over different interfaces (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.co... ).
I think I'm in agreement with Jagan. As documented in patch 4/4:
- A DSI host should keep the PHY powered down until the pre_enable
operation is
- called. All lanes are in an undefined idle state up to this point, and it
- must not be assumed that it is LP-11.
- pre_enable should initialise the PHY, set the data lanes to LP-11, and the
- clock lane to either LP-11 or HS depending on the mode_flag
- %MIPI_DSI_CLOCK_NON_CONTINUOUS.
Right, this theory makes sense.
However Exynos DSI for some reasons did the host initialization in the first call of the samsung_dsim_host_transfer(). If I moved the host initialization to pre_enable (before powering the panel on), executing DSI commands failed (timeout). This issue happens on all boards I have access (Trats, Trats2, Arndale, TM2e), so this must be an issue with Exynos DSI host itself not related to particular panel/bridge.
If I call samsung_dsim_init() once again, before issuing the first DSI command, then everything works fine. I've tried to check which part of that function is needed to be executed before transferring the commands, but it turned out that the complete host reset and (re)configuration is necessary. It looks that the initialization will need to be done twice, first time in the pre_enable to satisfy Jagan case, then on the first dsi transfer to make it work with real DSI panels.
Here is a git repo with such change: https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework-v2
This is worthy check, I will test it on imx8mm and integrate it into the next version - hope this is fine for you?
Jagan.
On Wed, May 11, 2022 at 8:28 PM Marek Szyprowski m.szyprowski@samsung.com wrote:
Hi Dave,
On 05.04.2022 13:43, Dave Stevenson wrote:
On Fri, 18 Mar 2022 at 12:25, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
On Fri, 4 Mar 2022 at 15:18, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
Hi All
A gentle ping on this series. Any comments on the approach? Thanks.
I realise the merge window has just closed and therefore folks have been busy, but no responses on this after a month?
Do I give up and submit a patch to document that DSI is broken and no one cares?
Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI DSIM bridge' thread, otherwise I would miss it since I'm not involved much in the DRM development.
This resolves most of the issues in the Exynos DSI and its recent conversion to the drm bridge framework. I've added the needed prepare_upstream_first flags to the panels and everything works fine without the bridge chain order hacks.
Feel free to add:
Tested-by: Marek Szyprowski m.szyprowski@samsung.com
The only remaining thing to resolve is the moment of enabling DSI host. The proper sequence is:
- host power on, 2. device power on, 3. host init, 4. device init, 5.
video enable.
#1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so far done in the first host transfer call, which usually happens in panel's prepare, then the #4 happens. Then video enable is done in the enable callbacks.
Jagan wants to move it to the dsi host pre_enable() to let it work with DSI bridges controlled over different interfaces (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.co... ). This however fails on Exynos with DSI panels, because when dsi's pre_enable is called, the dsi device is not yet powered. I've discussed this with Andrzej Hajda and we came to the conclusion that this can be resolved by adding the init() callback to the struct mipi_dsi_host_ops. Then DSI client (next bridge or panel) would call it after powering self on, but before sending any DSI commands in its pre_enable/prepare functions.
I've prepared a prototype of such solution. This approach finally resolved all the initialization issues! The bridge chain finally matches the hardware, no hack are needed, and everything is controlled by the DRM core. This prototype also includes the Jagan's patches, which add IMX support to Samsung DSIM. If one is interested, here is my git repo with all the PoC patches:
https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework
This seems a bit confusing to me, how come a Host initialization depends on the downstream bridge call flow?
Thanks, Jagan.
Hi,
Am Mittwoch, dem 11.05.2022 um 16:58 +0200 schrieb Marek Szyprowski:
Hi Dave,
On 05.04.2022 13:43, Dave Stevenson wrote:
On Fri, 18 Mar 2022 at 12:25, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
On Fri, 4 Mar 2022 at 15:18, Dave Stevenson dave.stevenson@raspberrypi.com wrote:
Hi All
A gentle ping on this series. Any comments on the approach? Thanks.
I realise the merge window has just closed and therefore folks have been busy, but no responses on this after a month?
Do I give up and submit a patch to document that DSI is broken and no one cares?
Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI DSIM bridge' thread, otherwise I would miss it since I'm not involved much in the DRM development.
This resolves most of the issues in the Exynos DSI and its recent conversion to the drm bridge framework. I've added the needed prepare_upstream_first flags to the panels and everything works fine without the bridge chain order hacks.
Feel free to add:
Tested-by: Marek Szyprowski m.szyprowski@samsung.com
The only remaining thing to resolve is the moment of enabling DSI host. The proper sequence is:
- host power on, 2. device power on, 3. host init, 4. device init, 5.
video enable.
#1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so far done in the first host transfer call, which usually happens in panel's prepare, then the #4 happens. Then video enable is done in the enable callbacks.
Jagan wants to move it to the dsi host pre_enable() to let it work with DSI bridges controlled over different interfaces (https://lore.kernel.org/all/20220504114021.33265-6-jagan@amarulasolutions.co... ). This however fails on Exynos with DSI panels, because when dsi's pre_enable is called, the dsi device is not yet powered. I've discussed this with Andrzej Hajda and we came to the conclusion that this can be resolved by adding the init() callback to the struct mipi_dsi_host_ops. Then DSI client (next bridge or panel) would call it after powering self on, but before sending any DSI commands in its pre_enable/prepare functions.
I've prepared a prototype of such solution. This approach finally resolved all the initialization issues! The bridge chain finally matches the hardware, no hack are needed, and everything is controlled by the DRM core. This prototype also includes the Jagan's patches, which add IMX support to Samsung DSIM. If one is interested, here is my git repo with all the PoC patches:
https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework
While this needs rework on the bridge chip side, I fear that we need something like that to allow the bridge to control the sequencing of the DSI host init. While most bridges that aren't controlled via the DSI channel might be fine with just initializing the host right before a video signal is driven, there are some that need a different sequencing.
The chip I'm currently looking at is a TC368767, where the datasheet states that the DSI lanes must be in LP-11 before the reset is released. While the datasheet doesn't specify what happens if that sequence is violated, Marek Vasut found that the chip enters a test mode if the lanes are not in LP-11 at that point and I can confirm this observation. Now with the TC358767 being a DSI to (e)DP converter, we need to release the chip from reset pretty early to establish the DP AUX connection to communicate with the display, in order to find out which video modes we can drive. As the chip is controlled via i2c in my case, initializing the DSI host on first DSI command transaction is out and doing so before the bridge pre_enable is way too late.
What I would need for this chip to work properly is an explicit call, like the mipi_dsi_host_init() added in the PoC above, to allow the bridge driver to kick the DSI host initialization before releasing the chip from reset state.
Regards, Lucas
dri-devel@lists.freedesktop.org
-
Andrzej Hajda -
Dave Stevenson -
Dmitry Baryshkov -
Jagan Teki -
Lucas Stach -
Marek Szyprowski -
Marek Vasut