| Message ID | cover.1646406653.git.dave.stevenson@raspberrypi.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | DSI host and peripheral initialisation ordering | expand |
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/synopsys/dw-mipi-dsi.c#L940 > [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/synopsys/dw-mipi-dsi.c#L940 > > [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.com/ ). 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: > > 1. 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.com/ > ). 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.
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: > > 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. 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.com/ > ). 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
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: >> >> 1. 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.com/ >> >> ). 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-74fe485cbfec-6741e6fb26af486e&q=1&e=07baacdb-540b-46fa-be0f-f534635150ec&u=https%3A%2F%2Fgithub.com%2Fmszyprow%2Flinux%2Ftree%2Fv5.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. https://lore.kernel.org/all/20220504114021.33265-1-jagan@amarulasolutions.com/ Best regards
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: > > 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.com/ > ). 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.
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: > > > > 1. 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.com/ > > ). 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 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: >> >> 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. > 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.com/ >> ). > 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: >>> >>> 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. >> 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.com/ >>> ). >> 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/exynos_drm_dsi.c#L860 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: > >>> > >>> 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. > >> 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.com/ > >>> ). > >> 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. 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? > > [1]: > https://elixir.bootlin.com/linux/v5.15.1/source/drivers/gpu/drm/exynos/exynos_drm_dsi.c#L860 > > 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, 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: > >> > >> 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. > > 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.com/ > >> ). > > 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.
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: > > >>> > > >>> 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. > > >> 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.com/ > > >>> ). > > >> 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. > > 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? > > > > [1]: > > https://elixir.bootlin.com/linux/v5.15.1/source/drivers/gpu/drm/exynos/exynos_drm_dsi.c#L860 > > > > 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, 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: > > 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.com/ > ). 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
Am 10.06.22 um 09:52 schrieb Lucas Stach: > 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: >> >> 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.com/ >> ). 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. The SN65DSI84 also has this requirement according to the datasheet. > 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. So to sum up on the missing parts: 1. This series needs more reviews, but is generally agreed on. 2. Jagan will integrate Marek's mipi_dsi_host_init() PoC when respinning his series "drm: bridge: Add Samsung MIPI DSIM bridge". 3. Bridge drivers need to be adjusted to call mipi_dsi_host_init() if required. Did I get anything wrong here, or is there some point that I'm missing? Jagan, do you have any plan to pick up the threads? Thanks Frieder
Hi Frieder, On Wed, Jul 6, 2022 at 12:39 PM Frieder Schrempf <frieder.schrempf@kontron.de> wrote: > > Am 10.06.22 um 09:52 schrieb Lucas Stach: > > 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: > >> > >> 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.com/ > >> ). 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. > > The SN65DSI84 also has this requirement according to the datasheet. > > > 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. > > So to sum up on the missing parts: > > 1. This series needs more reviews, but is generally agreed on. > 2. Jagan will integrate Marek's mipi_dsi_host_init() PoC when respinning > his series "drm: bridge: Add Samsung MIPI DSIM bridge". > 3. Bridge drivers need to be adjusted to call mipi_dsi_host_init() if > required. > > Did I get anything wrong here, or is there some point that I'm missing? > Jagan, do you have any plan to pick up the threads? I'm working on for next version series to pick a few changes from Marek's comment on the DSIM series. Not sure about mipi_dsi_host_init() but I will comment on it based on my new series of tests. Jagan.
Hi Frieder. Apologies Lucas - I missed your response. On Wed, 6 Jul 2022 at 08:09, Frieder Schrempf <frieder.schrempf@kontron.de> wrote: > > Am 10.06.22 um 09:52 schrieb Lucas Stach: > > 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: > >> > >> 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.com/ > >> ). 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. > > The SN65DSI84 also has this requirement according to the datasheet. SN65DSI8[3|4|5] need LP-11 before the chip comes out of reset, but that only happens as part of enabling the bridge chain. This patch set allows it to request the DSI host to be initialised first in order to meet that constraint, which is common with many DSI sink devices. Lucas' comment with TC368767 is that it is doing other things for display negotiation over the AUX channel prior to enabling the video path, and that is needing the DSI interface to be enabled and in LP-11 much earlier (and potentially clock lane in HS if not using an external clock). > > 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. > > So to sum up on the missing parts: > > 1. This series needs more reviews, but is generally agreed on. > 2. Jagan will integrate Marek's mipi_dsi_host_init() PoC when respinning > his series "drm: bridge: Add Samsung MIPI DSIM bridge". I'm still not clear as to whether Marek's mipi_dsi_host_init is needed in the majority of cases. Exynos appeared to be trying to check for no contention as part of the initialisation to LP-11, and that isn't necessary. No one has come back on that one. Adding a mipi_dsi_host_init would potentially solve the additional issue for TC358767. However it leaves a number of open questions. The first I can think of is that there are no defined mechanisms for specifying the link frequency prior to having a video mode set. Yes struct mipi_dsi_device has hs_rate, but that is defined as the maximum HS rate that the device supports, and therefore open to variation in the implementation. Exynos has various vendor specific DT parameters to configure link frequencies, which ought to be standardised if that is the preferred approach. > 3. Bridge drivers need to be adjusted to call mipi_dsi_host_init() if > required. Which hopefully is only the weirder devices such as TC358767. SN65DSI8[3|4|5] do not need this, but they will need pre_enable_upstream_first if/when the enable logic is shifted from atomic_enable to atomic_pre_enable. Cheers. Dave > Did I get anything wrong here, or is there some point that I'm missing? > Jagan, do you have any plan to pick up the threads? > > Thanks > Frieder
Hi Dave, Am 06.07.22 um 12:27 schrieb Dave Stevenson: > Hi Frieder. > > Apologies Lucas - I missed your response. > > On Wed, 6 Jul 2022 at 08:09, Frieder Schrempf > <frieder.schrempf@kontron.de> wrote: >> >> Am 10.06.22 um 09:52 schrieb Lucas Stach: >>> 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: >>>> >>>> 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://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20220504114021.33265-6-jagan%40amarulasolutions.com%2F&data=05%7C01%7Cfrieder.schrempf%40kontron.de%7Ca82478efb8434ac07dfb08da5f3a1b75%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637927000416444951%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=vgnJ8L7fhloUV83p%2Be6ziUKHbL9apqcLWpDvMUcOOoY%3D&reserved=0 >>>> ). 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://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmszyprow%2Flinux%2Ftree%2Fv5.18-next-20220511-dsi-rework&data=05%7C01%7Cfrieder.schrempf%40kontron.de%7Ca82478efb8434ac07dfb08da5f3a1b75%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637927000416444951%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=ka2wikbVhc4RxFGARjTh0ixGKiaNHloW9dVkejA5kEg%3D&reserved=0 >>> >>> 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. >> >> The SN65DSI84 also has this requirement according to the datasheet. > > SN65DSI8[3|4|5] need LP-11 before the chip comes out of reset, but > that only happens as part of enabling the bridge chain. This patch set > allows it to request the DSI host to be initialised first in order to > meet that constraint, which is common with many DSI sink devices. > > Lucas' comment with TC368767 is that it is doing other things for > display negotiation over the AUX channel prior to enabling the video > path, and that is needing the DSI interface to be enabled and in LP-11 > much earlier (and potentially clock lane in HS if not using an > external clock). Ok, got it know. I see this is a separate issue that is specific to devices that need the link for video mode negotiation early. Thanks for the explanation! > >>> 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. >> >> So to sum up on the missing parts: >> >> 1. This series needs more reviews, but is generally agreed on. >> 2. Jagan will integrate Marek's mipi_dsi_host_init() PoC when respinning >> his series "drm: bridge: Add Samsung MIPI DSIM bridge". > > I'm still not clear as to whether Marek's mipi_dsi_host_init is needed > in the majority of cases. > Exynos appeared to be trying to check for no contention as part of the > initialisation to LP-11, and that isn't necessary. No one has come > back on that one. > > Adding a mipi_dsi_host_init would potentially solve the additional > issue for TC358767. > However it leaves a number of open questions. The first I can think of > is that there are no defined mechanisms for specifying the link > frequency prior to having a video mode set. Yes struct mipi_dsi_device > has hs_rate, but that is defined as the maximum HS rate that the > device supports, and therefore open to variation in the > implementation. Exynos has various vendor specific DT parameters to > configure link frequencies, which ought to be standardised if that is > the preferred approach. Having standardized DT properties to configure the early link parameters could be a possible solution. > >> 3. Bridge drivers need to be adjusted to call mipi_dsi_host_init() if >> required. > > Which hopefully is only the weirder devices such as TC358767. > SN65DSI8[3|4|5] do not need this, but they will need > pre_enable_upstream_first if/when the enable logic is shifted from > atomic_enable to atomic_pre_enable. Got it, thanks for clarifying! Thanks Frieder
Hi Dave, a long overdue reply on this series. On Fri, Mar 04, 2022 at 03:17:55PM +0000, Dave Stevenson wrote: > 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. A typically chain looks like this: CRTC => Encoder => Bridge A => Bridge B We have in DRM bridges established what is the "next" bridge - indicated with the direction of the arrows in the drawing. This set of patches introduces the concept of "upstream" bridges. pre_enable_prev_bridge_first would be easier to understand as it uses the current terminology. I get that "upstream" is used in the DSI specification - but we are dealing with bridges that happens to support DSI and more, and mixing the two terminologies is not good. Note: Upstream is also used in a bridge doc section - here it should most likely be updated too. The current approach set a flag that magically makes the core do something else. Have you considered a much more explicit approach? A few helpers like: drm_bridge_pre_enable_prev_bridge() drm_bridge_enable_prev_bridge() drm_bridge_disable_prev_bridge() drm_bridge_post_disable_prev_bridge() And then update the core so the relevant function is only called once for a bridge. Then the need for DSI lanes in LP-11 can be archived by a call to drm_bridge_pre_enable_prev_bridge() This is more explicit than a flag that triggers some magic behaviour. It may even see uses we have not realised yet. It is late here - so maybe the above is not a good idea tomorrow - but right now I like the simplicity of it. Other than the above I read that a mipi_dsi_host_init() is planned, which is also explicit and simple - good. Have we seen a new revision of some of these? Chances are high that I have missed it then. Sam
Hi Sam On Mon, 18 Jul 2022 at 21:52, Sam Ravnborg <sam@ravnborg.org> wrote: > > Hi Dave, > > a long overdue reply on this series. > > On Fri, Mar 04, 2022 at 03:17:55PM +0000, Dave Stevenson wrote: > > 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. > > A typically chain looks like this: > > CRTC => Encoder => Bridge A => Bridge B > > We have in DRM bridges established what is the "next" bridge - indicated > with the direction of the arrows in the drawing. > > This set of patches introduces the concept of "upstream" bridges. > > pre_enable_prev_bridge_first would be easier to understand as it uses > the current terminology. > I get that "upstream" is used in the DSI specification - but we are > dealing with bridges that happens to support DSI and more, and mixing > the two terminologies is not good. > > Note: Upstream is also used in a bridge doc section - here it should > most likely be updated too. Sure, I have no issues with switching to prev/next from upstream/downstream. To the outsider it can be confusing - in pre_enable and disable, the next bridge to be called is the previous one. At least it is documented. > The current approach set a flag that magically makes the core do something > else. Have you considered a much more explicit approach? > > A few helpers like: > > drm_bridge_pre_enable_prev_bridge() > drm_bridge_enable_prev_bridge() > drm_bridge_disable_prev_bridge() > drm_bridge_post_disable_prev_bridge() No point in drm_bridge_enable_prev_bridge() and drm_bridge_post_disable_prev_bridge() as the call order down the chain will mean that they have already been called. drm_bridge_enable_next_bridge() and drm_bridge_post_disable_next_bridge() possibly. > And then update the core so the relevant function is only called once > for a bridge. > Then the need for DSI lanes in LP-11 can be archived by a call to > > drm_bridge_pre_enable_prev_bridge() Unfortunately it gets ugly with post_disable. The DSI host controller post_disable will have been called before the DSI peripheral's post_disable, and there are conditions where the peripheral needs to send DSI commands in post_disable (eg panel-asus-z00t-tm5p5-n35596 [1]). Changing all DSI hosts to call drm_bridge_post_disable_next_bridge feels like the wrong thing to do. There are currently hacks in dw-mipi-dsi that do call the next panel/bridge post_disable [2] and it would be nice to get rid of them. Currently the calls aren't tracked for state, so you end up with post_disable being called twice, and panels having to track state (eg jdi_lt070me050000 [3]). [1] tm5p5_nt35596_unprepare() calls tm5p5_nt35596_off() https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c#L107 [2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L889 [3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c#L44 > This is more explicit than a flag that triggers some magic behaviour. > It may even see uses we have not realised yet. Personally it feels like more boilerplate in almost all DSI drivers, and generally I see a push to remove boilerplate. > It is late here - so maybe the above is not a good idea tomorrow - but > right now I like the simplicity of it. > > Other than the above I read that a mipi_dsi_host_init() is planned, > which is also explicit and simple - good. It's been raised, but the justification for most use cases hasn't been made. The Exynos conversion looks to be doing the wrong thing in checking state, and that's why it is currently needing it. Again it's also more boilerplate. TC358767 is an odd one as it wants the DSI interface enabled very early in order to have a clock for the DP aux channel well before video is running. I had a thought on that, but It looks like I haven't hit send on a reply to Lucas on that one - too many distractions. > Have we seen a new revision of some of these? > Chances are high that I have missed it then. No, still on V2. Other than Dmitry's comment over updating parade-ps8640 and dropping drm_bridge_chain_*, no real comments had been made. Dave
Hi Lucas On Fri, 10 Jun 2022 at 08:52, Lucas Stach <l.stach@pengutronix.de> wrote: > > 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: > > > > 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.com/ > > ). 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. This is going off on a slight tangent from the original patch set, but a thought has just come to mind for this use case. For this sort of bridge device where you want to power up early (either just for LP-11 state, or for HS on the clock lane), is power up at mipi_dsi_attach, and power down at mipi_dsi_detach sufficient? We have mode_flags in struct mipi_dsi_device passed in mipi_dsi_attach, so an extra flag in there (eg MIPI_DSI_EARLY_POWER_ON) would be very easy and be a simple way to signal an alternate DSI host behaviour. It still leaves the configuration of link frequency as an open question, but potentially solves the sequencing issue. Just a thought. Perhaps dsi_attach() is still too late in the process. Dave
Hi Dave, On 19/07/2022 16:45, Dave Stevenson wrote: > Hi Sam > > On Mon, 18 Jul 2022 at 21:52, Sam Ravnborg <sam@ravnborg.org> wrote: >> >> Hi Dave, >> >> a long overdue reply on this series. >> >> On Fri, Mar 04, 2022 at 03:17:55PM +0000, Dave Stevenson wrote: >>> 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. >> >> A typically chain looks like this: >> >> CRTC => Encoder => Bridge A => Bridge B >> >> We have in DRM bridges established what is the "next" bridge - indicated >> with the direction of the arrows in the drawing. >> >> This set of patches introduces the concept of "upstream" bridges. >> >> pre_enable_prev_bridge_first would be easier to understand as it uses >> the current terminology. >> I get that "upstream" is used in the DSI specification - but we are >> dealing with bridges that happens to support DSI and more, and mixing >> the two terminologies is not good. >> >> Note: Upstream is also used in a bridge doc section - here it should >> most likely be updated too. > > Sure, I have no issues with switching to prev/next from upstream/downstream. > To the outsider it can be confusing - in pre_enable and disable, the > next bridge to be called is the previous one. At least it is > documented. > >> The current approach set a flag that magically makes the core do something >> else. Have you considered a much more explicit approach? >> >> A few helpers like: >> >> drm_bridge_pre_enable_prev_bridge() >> drm_bridge_enable_prev_bridge() >> drm_bridge_disable_prev_bridge() >> drm_bridge_post_disable_prev_bridge() > > No point in drm_bridge_enable_prev_bridge() and > drm_bridge_post_disable_prev_bridge() as the call order down the chain > will mean that they have already been called. > drm_bridge_enable_next_bridge() and > drm_bridge_post_disable_next_bridge() possibly. > >> And then update the core so the relevant function is only called once >> for a bridge. >> Then the need for DSI lanes in LP-11 can be archived by a call to >> >> drm_bridge_pre_enable_prev_bridge() > > Unfortunately it gets ugly with post_disable. > The DSI host controller post_disable will have been called before the > DSI peripheral's post_disable, and there are conditions where the > peripheral needs to send DSI commands in post_disable (eg > panel-asus-z00t-tm5p5-n35596 [1]). Changing all DSI hosts to call > drm_bridge_post_disable_next_bridge feels like the wrong thing to do. > There are currently hacks in dw-mipi-dsi that do call the next > panel/bridge post_disable [2] and it would be nice to get rid of them. > Currently the calls aren't tracked for state, so you end up with > post_disable being called twice, and panels having to track state (eg > jdi_lt070me050000 [3]). > > [1] tm5p5_nt35596_unprepare() calls tm5p5_nt35596_off() > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c#L107 > [2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L889 > [3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c#L44 > >> This is more explicit than a flag that triggers some magic behaviour. >> It may even see uses we have not realised yet. > > Personally it feels like more boilerplate in almost all DSI drivers, > and generally I see a push to remove boilerplate. > >> It is late here - so maybe the above is not a good idea tomorrow - but >> right now I like the simplicity of it. >> >> Other than the above I read that a mipi_dsi_host_init() is planned, >> which is also explicit and simple - good. > > It's been raised, but the justification for most use cases hasn't been > made. The Exynos conversion looks to be doing the wrong thing in > checking state, and that's why it is currently needing it. > Again it's also more boilerplate. > > TC358767 is an odd one as it wants the DSI interface enabled very > early in order to have a clock for the DP aux channel well before > video is running. I had a thought on that, but It looks like I haven't > hit send on a reply to Lucas on that one - too many distractions. > >> Have we seen a new revision of some of these? >> Chances are high that I have missed it then. > > No, still on V2. Other than Dmitry's comment over updating > parade-ps8640 and dropping drm_bridge_chain_*, no real comments had > been made. It's been a while now. Do you still plan to pursue this patchset? [personal notice: I'd prefer something less strange, e.g. an explicit calls to mipi_dsi_host, but as this patchset seems to fix the issues, I'm fine with it]. > > Dave
Hi Dmitry On Sun, 13 Nov 2022 at 13:06, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > Hi Dave, > > On 19/07/2022 16:45, Dave Stevenson wrote: > > Hi Sam > > > > On Mon, 18 Jul 2022 at 21:52, Sam Ravnborg <sam@ravnborg.org> wrote: > >> > >> Hi Dave, > >> > >> a long overdue reply on this series. > >> > >> On Fri, Mar 04, 2022 at 03:17:55PM +0000, Dave Stevenson wrote: > >>> 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. > >> > >> A typically chain looks like this: > >> > >> CRTC => Encoder => Bridge A => Bridge B > >> > >> We have in DRM bridges established what is the "next" bridge - indicated > >> with the direction of the arrows in the drawing. > >> > >> This set of patches introduces the concept of "upstream" bridges. > >> > >> pre_enable_prev_bridge_first would be easier to understand as it uses > >> the current terminology. > >> I get that "upstream" is used in the DSI specification - but we are > >> dealing with bridges that happens to support DSI and more, and mixing > >> the two terminologies is not good. > >> > >> Note: Upstream is also used in a bridge doc section - here it should > >> most likely be updated too. > > > > Sure, I have no issues with switching to prev/next from upstream/downstream. > > To the outsider it can be confusing - in pre_enable and disable, the > > next bridge to be called is the previous one. At least it is > > documented. > > > >> The current approach set a flag that magically makes the core do something > >> else. Have you considered a much more explicit approach? > >> > >> A few helpers like: > >> > >> drm_bridge_pre_enable_prev_bridge() > >> drm_bridge_enable_prev_bridge() > >> drm_bridge_disable_prev_bridge() > >> drm_bridge_post_disable_prev_bridge() > > > > No point in drm_bridge_enable_prev_bridge() and > > drm_bridge_post_disable_prev_bridge() as the call order down the chain > > will mean that they have already been called. > > drm_bridge_enable_next_bridge() and > > drm_bridge_post_disable_next_bridge() possibly. > > > >> And then update the core so the relevant function is only called once > >> for a bridge. > >> Then the need for DSI lanes in LP-11 can be archived by a call to > >> > >> drm_bridge_pre_enable_prev_bridge() > > > > Unfortunately it gets ugly with post_disable. > > The DSI host controller post_disable will have been called before the > > DSI peripheral's post_disable, and there are conditions where the > > peripheral needs to send DSI commands in post_disable (eg > > panel-asus-z00t-tm5p5-n35596 [1]). Changing all DSI hosts to call > > drm_bridge_post_disable_next_bridge feels like the wrong thing to do. > > There are currently hacks in dw-mipi-dsi that do call the next > > panel/bridge post_disable [2] and it would be nice to get rid of them. > > Currently the calls aren't tracked for state, so you end up with > > post_disable being called twice, and panels having to track state (eg > > jdi_lt070me050000 [3]). > > > > [1] tm5p5_nt35596_unprepare() calls tm5p5_nt35596_off() > > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c#L107 > > [2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L889 > > [3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c#L44 > > > >> This is more explicit than a flag that triggers some magic behaviour. > >> It may even see uses we have not realised yet. > > > > Personally it feels like more boilerplate in almost all DSI drivers, > > and generally I see a push to remove boilerplate. > > > >> It is late here - so maybe the above is not a good idea tomorrow - but > >> right now I like the simplicity of it. > >> > >> Other than the above I read that a mipi_dsi_host_init() is planned, > >> which is also explicit and simple - good. > > > > It's been raised, but the justification for most use cases hasn't been > > made. The Exynos conversion looks to be doing the wrong thing in > > checking state, and that's why it is currently needing it. > > Again it's also more boilerplate. > > > > TC358767 is an odd one as it wants the DSI interface enabled very > > early in order to have a clock for the DP aux channel well before > > video is running. I had a thought on that, but It looks like I haven't > > hit send on a reply to Lucas on that one - too many distractions. > > > >> Have we seen a new revision of some of these? > >> Chances are high that I have missed it then. > > > > No, still on V2. Other than Dmitry's comment over updating > > parade-ps8640 and dropping drm_bridge_chain_*, no real comments had > > been made. > > It's been a while now. Do you still plan to pursue this patchset? If there was anything that could actually be worked on, then I'm happy to respin it, but if the approach is generally being rejected then I don't want to waste the effort. I'm not totally clear who the maintainers are that the final arbiters and need to sign off on this. drm_bridge.c falls to Maarten, Maxime, and Thomas for "DRM DRIVERS AND MISC GPU PATCHES" drm_panel.c falls to Thierry and Sam for "DRM PANEL DRIVERS", and then Maarten, Maxime, and Thomas. Only Sam has responded publicly. I have had discussions with Maxime, but it's not directly his area of knowledge. Looking at the patch series: Patch 1: Your comment "update parade-ps8640 to use drm_atomic_bridge_chain_". It looks like patchset [1] by Sam does this, but the patchset went wrong and is missing patches 8-11 and therefore hasn't been merged. Patch 2: Comment from Jagan that it's like an old patch. It has similarities, but isn't the same. Patch 3: R-b by you (thank you), but concerns from Jagan which I still don't understand. Without clarification on the issue and whether my suggested alternative place for the hook solves the issue, IMHO it's not worth respinning. Patch 4: R-b Laurent. This cover note got totally subverted with Exynos issues. Sam did request use of prev / next instead of upstream / downstream, which can be done and perhaps warrants a respin now. > [personal notice: I'd prefer something less strange, e.g. an explicit > calls to mipi_dsi_host, but as this patchset seems to fix the issues, > I'm fine with it]. That can fix the power up sequence, but how do you propose telling the DSI controller NOT to power down in post_disable before the DSI peripheral post_disable has potentially sent DCS commands - i.e. the case you were discussing on Friday in [2]. If a panel/bridge driver doesn't call mipi_dsi_host_init then the expectation must be that it will be called by the DSI controller's pre_enable, and deinit from post_disable. Likewise init & deinit would be called if host_transfer is used when the host isn't initialised. If the panel/bridge driver explicitly calls mipi_dsi_host_init, then does that mandate that it must also call mipi_dsi_host_deinint. The controller post_disable is then effectively a no-op. This can be covered in documentation, but also leaves the potential for strange behaviour if the requirement is not followed, and I can't think of a nice place to drop a WARN to flag the issue in the driver. TBH The lack of interest in solving the issues almost makes me want to just document the total brokenness of it and throw in the towel. Seeing as we as Raspberry Pi run a vendor kernel, we can run with downstream patches until those who care finally make a decision for mainline. I'd prefer to solve it properly, but it requires some engagement from the community. I'll do a respin now second guessing Jagan's comment, and then give it a month before giving up. Cheers Dave [1] https://patchwork.freedesktop.org/series/106422/#rev5 [2] https://lists.freedesktop.org/archives/dri-devel/2022-November/379693.html > > > > Dave > > -- > With best wishes > Dmitry >
On 15/11/2022 17:14, Dave Stevenson wrote: > Hi Dmitry > > On Sun, 13 Nov 2022 at 13:06, Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: >> >> Hi Dave, >> >> On 19/07/2022 16:45, Dave Stevenson wrote: >>> Hi Sam >>> >>> On Mon, 18 Jul 2022 at 21:52, Sam Ravnborg <sam@ravnborg.org> wrote: >>>> >>>> Hi Dave, >>>> >>>> a long overdue reply on this series. >>>> >>>> On Fri, Mar 04, 2022 at 03:17:55PM +0000, Dave Stevenson wrote: >>>>> 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. >>>> >>>> A typically chain looks like this: >>>> >>>> CRTC => Encoder => Bridge A => Bridge B >>>> >>>> We have in DRM bridges established what is the "next" bridge - indicated >>>> with the direction of the arrows in the drawing. >>>> >>>> This set of patches introduces the concept of "upstream" bridges. >>>> >>>> pre_enable_prev_bridge_first would be easier to understand as it uses >>>> the current terminology. >>>> I get that "upstream" is used in the DSI specification - but we are >>>> dealing with bridges that happens to support DSI and more, and mixing >>>> the two terminologies is not good. >>>> >>>> Note: Upstream is also used in a bridge doc section - here it should >>>> most likely be updated too. >>> >>> Sure, I have no issues with switching to prev/next from upstream/downstream. >>> To the outsider it can be confusing - in pre_enable and disable, the >>> next bridge to be called is the previous one. At least it is >>> documented. >>> >>>> The current approach set a flag that magically makes the core do something >>>> else. Have you considered a much more explicit approach? >>>> >>>> A few helpers like: >>>> >>>> drm_bridge_pre_enable_prev_bridge() >>>> drm_bridge_enable_prev_bridge() >>>> drm_bridge_disable_prev_bridge() >>>> drm_bridge_post_disable_prev_bridge() >>> >>> No point in drm_bridge_enable_prev_bridge() and >>> drm_bridge_post_disable_prev_bridge() as the call order down the chain >>> will mean that they have already been called. >>> drm_bridge_enable_next_bridge() and >>> drm_bridge_post_disable_next_bridge() possibly. >>> >>>> And then update the core so the relevant function is only called once >>>> for a bridge. >>>> Then the need for DSI lanes in LP-11 can be archived by a call to >>>> >>>> drm_bridge_pre_enable_prev_bridge() >>> >>> Unfortunately it gets ugly with post_disable. >>> The DSI host controller post_disable will have been called before the >>> DSI peripheral's post_disable, and there are conditions where the >>> peripheral needs to send DSI commands in post_disable (eg >>> panel-asus-z00t-tm5p5-n35596 [1]). Changing all DSI hosts to call >>> drm_bridge_post_disable_next_bridge feels like the wrong thing to do. >>> There are currently hacks in dw-mipi-dsi that do call the next >>> panel/bridge post_disable [2] and it would be nice to get rid of them. >>> Currently the calls aren't tracked for state, so you end up with >>> post_disable being called twice, and panels having to track state (eg >>> jdi_lt070me050000 [3]). >>> >>> [1] tm5p5_nt35596_unprepare() calls tm5p5_nt35596_off() >>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c#L107 >>> [2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L889 >>> [3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c#L44 >>> >>>> This is more explicit than a flag that triggers some magic behaviour. >>>> It may even see uses we have not realised yet. >>> >>> Personally it feels like more boilerplate in almost all DSI drivers, >>> and generally I see a push to remove boilerplate. >>> >>>> It is late here - so maybe the above is not a good idea tomorrow - but >>>> right now I like the simplicity of it. >>>> >>>> Other than the above I read that a mipi_dsi_host_init() is planned, >>>> which is also explicit and simple - good. >>> >>> It's been raised, but the justification for most use cases hasn't been >>> made. The Exynos conversion looks to be doing the wrong thing in >>> checking state, and that's why it is currently needing it. >>> Again it's also more boilerplate. >>> >>> TC358767 is an odd one as it wants the DSI interface enabled very >>> early in order to have a clock for the DP aux channel well before >>> video is running. I had a thought on that, but It looks like I haven't >>> hit send on a reply to Lucas on that one - too many distractions. >>> >>>> Have we seen a new revision of some of these? >>>> Chances are high that I have missed it then. >>> >>> No, still on V2. Other than Dmitry's comment over updating >>> parade-ps8640 and dropping drm_bridge_chain_*, no real comments had >>> been made. >> >> It's been a while now. Do you still plan to pursue this patchset? > > If there was anything that could actually be worked on, then I'm happy > to respin it, but if the approach is generally being rejected then I > don't want to waste the effort. > > I'm not totally clear who the maintainers are that the final arbiters > and need to sign off on this. > drm_bridge.c falls to Maarten, Maxime, and Thomas for "DRM DRIVERS AND > MISC GPU PATCHES" > drm_panel.c falls to Thierry and Sam for "DRM PANEL DRIVERS", and then > Maarten, Maxime, and Thomas. > Only Sam has responded publicly. I have had discussions with Maxime, > but it's not directly his area of knowledge. > > Looking at the patch series: > Patch 1: Your comment "update parade-ps8640 to use > drm_atomic_bridge_chain_". It looks like patchset [1] by Sam does > this, but the patchset went wrong and is missing patches 8-11 and > therefore hasn't been merged. > Patch 2: Comment from Jagan that it's like an old patch. It has > similarities, but isn't the same. > Patch 3: R-b by you (thank you), but concerns from Jagan which I still > don't understand. Without clarification on the issue and whether my > suggested alternative place for the hook solves the issue, IMHO it's > not worth respinning. > Patch 4: R-b Laurent. > > This cover note got totally subverted with Exynos issues. > Sam did request use of prev / next instead of upstream / downstream, > which can be done and perhaps warrants a respin now. > >> [personal notice: I'd prefer something less strange, e.g. an explicit >> calls to mipi_dsi_host, but as this patchset seems to fix the issues, >> I'm fine with it]. > > That can fix the power up sequence, but how do you propose telling the > DSI controller NOT to power down in post_disable before the DSI > peripheral post_disable has potentially sent DCS commands - i.e. the > case you were discussing on Friday in [2]. I thought that the same 'call the parent beforehand' switch applied to the deinit paths, didn't it? > If a panel/bridge driver doesn't call mipi_dsi_host_init then the > expectation must be that it will be called by the DSI controller's > pre_enable, and deinit from post_disable. Likewise init & deinit would > be called if host_transfer is used when the host isn't initialised. > > If the panel/bridge driver explicitly calls mipi_dsi_host_init, then > does that mandate that it must also call mipi_dsi_host_deinint. The > controller post_disable is then effectively a no-op. This can be > covered in documentation, but also leaves the potential for strange > behaviour if the requirement is not followed, and I can't think of a > nice place to drop a WARN to flag the issue in the driver. > > > TBH The lack of interest in solving the issues almost makes me want to > just document the total brokenness of it and throw in the towel. > Seeing as we as Raspberry Pi run a vendor kernel, we can run with > downstream patches until those who care finally make a decision for > mainline. I'd prefer to solve it properly, but it requires some > engagement from the community. I see. I can probably try spinning a patchset doing explicit mipi_dsi calls. Let's see if it gains more attention. It seems something is broken with respect to reviewing of core drm patches touching strange areas. My patchset improving drm_bridge_connector HPD also didn't gain a lot of responses. > I'll do a respin now second guessing Jagan's comment, and then give it > a month before giving up > > Cheers > Dave > > [1] https://patchwork.freedesktop.org/series/106422/#rev5 > [2] https://lists.freedesktop.org/archives/dri-devel/2022-November/379693.html > > >>> >>> Dave >> >> -- >> With best wishes >> Dmitry >>
Hi Dmitry On Tue, 15 Nov 2022 at 14:21, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On 15/11/2022 17:14, Dave Stevenson wrote: > > Hi Dmitry > > > > On Sun, 13 Nov 2022 at 13:06, Dmitry Baryshkov > > <dmitry.baryshkov@linaro.org> wrote: > >> > >> Hi Dave, > >> > >> On 19/07/2022 16:45, Dave Stevenson wrote: > >>> Hi Sam > >>> > >>> On Mon, 18 Jul 2022 at 21:52, Sam Ravnborg <sam@ravnborg.org> wrote: > >>>> > >>>> Hi Dave, > >>>> > >>>> a long overdue reply on this series. > >>>> > >>>> On Fri, Mar 04, 2022 at 03:17:55PM +0000, Dave Stevenson wrote: > >>>>> 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. > >>>> > >>>> A typically chain looks like this: > >>>> > >>>> CRTC => Encoder => Bridge A => Bridge B > >>>> > >>>> We have in DRM bridges established what is the "next" bridge - indicated > >>>> with the direction of the arrows in the drawing. > >>>> > >>>> This set of patches introduces the concept of "upstream" bridges. > >>>> > >>>> pre_enable_prev_bridge_first would be easier to understand as it uses > >>>> the current terminology. > >>>> I get that "upstream" is used in the DSI specification - but we are > >>>> dealing with bridges that happens to support DSI and more, and mixing > >>>> the two terminologies is not good. > >>>> > >>>> Note: Upstream is also used in a bridge doc section - here it should > >>>> most likely be updated too. > >>> > >>> Sure, I have no issues with switching to prev/next from upstream/downstream. > >>> To the outsider it can be confusing - in pre_enable and disable, the > >>> next bridge to be called is the previous one. At least it is > >>> documented. > >>> > >>>> The current approach set a flag that magically makes the core do something > >>>> else. Have you considered a much more explicit approach? > >>>> > >>>> A few helpers like: > >>>> > >>>> drm_bridge_pre_enable_prev_bridge() > >>>> drm_bridge_enable_prev_bridge() > >>>> drm_bridge_disable_prev_bridge() > >>>> drm_bridge_post_disable_prev_bridge() > >>> > >>> No point in drm_bridge_enable_prev_bridge() and > >>> drm_bridge_post_disable_prev_bridge() as the call order down the chain > >>> will mean that they have already been called. > >>> drm_bridge_enable_next_bridge() and > >>> drm_bridge_post_disable_next_bridge() possibly. > >>> > >>>> And then update the core so the relevant function is only called once > >>>> for a bridge. > >>>> Then the need for DSI lanes in LP-11 can be archived by a call to > >>>> > >>>> drm_bridge_pre_enable_prev_bridge() > >>> > >>> Unfortunately it gets ugly with post_disable. > >>> The DSI host controller post_disable will have been called before the > >>> DSI peripheral's post_disable, and there are conditions where the > >>> peripheral needs to send DSI commands in post_disable (eg > >>> panel-asus-z00t-tm5p5-n35596 [1]). Changing all DSI hosts to call > >>> drm_bridge_post_disable_next_bridge feels like the wrong thing to do. > >>> There are currently hacks in dw-mipi-dsi that do call the next > >>> panel/bridge post_disable [2] and it would be nice to get rid of them. > >>> Currently the calls aren't tracked for state, so you end up with > >>> post_disable being called twice, and panels having to track state (eg > >>> jdi_lt070me050000 [3]). > >>> > >>> [1] tm5p5_nt35596_unprepare() calls tm5p5_nt35596_off() > >>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c#L107 > >>> [2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L889 > >>> [3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c#L44 > >>> > >>>> This is more explicit than a flag that triggers some magic behaviour. > >>>> It may even see uses we have not realised yet. > >>> > >>> Personally it feels like more boilerplate in almost all DSI drivers, > >>> and generally I see a push to remove boilerplate. > >>> > >>>> It is late here - so maybe the above is not a good idea tomorrow - but > >>>> right now I like the simplicity of it. > >>>> > >>>> Other than the above I read that a mipi_dsi_host_init() is planned, > >>>> which is also explicit and simple - good. > >>> > >>> It's been raised, but the justification for most use cases hasn't been > >>> made. The Exynos conversion looks to be doing the wrong thing in > >>> checking state, and that's why it is currently needing it. > >>> Again it's also more boilerplate. > >>> > >>> TC358767 is an odd one as it wants the DSI interface enabled very > >>> early in order to have a clock for the DP aux channel well before > >>> video is running. I had a thought on that, but It looks like I haven't > >>> hit send on a reply to Lucas on that one - too many distractions. > >>> > >>>> Have we seen a new revision of some of these? > >>>> Chances are high that I have missed it then. > >>> > >>> No, still on V2. Other than Dmitry's comment over updating > >>> parade-ps8640 and dropping drm_bridge_chain_*, no real comments had > >>> been made. > >> > >> It's been a while now. Do you still plan to pursue this patchset? > > > > If there was anything that could actually be worked on, then I'm happy > > to respin it, but if the approach is generally being rejected then I > > don't want to waste the effort. > > > > I'm not totally clear who the maintainers are that the final arbiters > > and need to sign off on this. > > drm_bridge.c falls to Maarten, Maxime, and Thomas for "DRM DRIVERS AND > > MISC GPU PATCHES" > > drm_panel.c falls to Thierry and Sam for "DRM PANEL DRIVERS", and then > > Maarten, Maxime, and Thomas. > > Only Sam has responded publicly. I have had discussions with Maxime, > > but it's not directly his area of knowledge. > > > > Looking at the patch series: > > Patch 1: Your comment "update parade-ps8640 to use > > drm_atomic_bridge_chain_". It looks like patchset [1] by Sam does > > this, but the patchset went wrong and is missing patches 8-11 and > > therefore hasn't been merged. > > Patch 2: Comment from Jagan that it's like an old patch. It has > > similarities, but isn't the same. > > Patch 3: R-b by you (thank you), but concerns from Jagan which I still > > don't understand. Without clarification on the issue and whether my > > suggested alternative place for the hook solves the issue, IMHO it's > > not worth respinning. > > Patch 4: R-b Laurent. > > > > This cover note got totally subverted with Exynos issues. > > Sam did request use of prev / next instead of upstream / downstream, > > which can be done and perhaps warrants a respin now. > > > >> [personal notice: I'd prefer something less strange, e.g. an explicit > >> calls to mipi_dsi_host, but as this patchset seems to fix the issues, > >> I'm fine with it]. > > > > That can fix the power up sequence, but how do you propose telling the > > DSI controller NOT to power down in post_disable before the DSI > > peripheral post_disable has potentially sent DCS commands - i.e. the > > case you were discussing on Friday in [2]. > > I thought that the same 'call the parent beforehand' switch applied to > the deinit paths, didn't it? My proposed flag does indeed swap the order of post_disable as well. Perhaps I was misunderstanding your personal preference. I was taking it as an explicit mipi_dsi_host call to initialise the DSI link, which then also needs an explicit mipi_dsi_host call to tear it down as well. In that situation there is a need to rework the bridge chain post_disable to allow for the panel post_disable to send DCS commands before the DSI host is disabled. > > If a panel/bridge driver doesn't call mipi_dsi_host_init then the > > expectation must be that it will be called by the DSI controller's > > pre_enable, and deinit from post_disable. Likewise init & deinit would > > be called if host_transfer is used when the host isn't initialised. > > > > If the panel/bridge driver explicitly calls mipi_dsi_host_init, then > > does that mandate that it must also call mipi_dsi_host_deinint. The > > controller post_disable is then effectively a no-op. This can be > > covered in documentation, but also leaves the potential for strange > > behaviour if the requirement is not followed, and I can't think of a > > nice place to drop a WARN to flag the issue in the driver. > > > > > > TBH The lack of interest in solving the issues almost makes me want to > > just document the total brokenness of it and throw in the towel. > > Seeing as we as Raspberry Pi run a vendor kernel, we can run with > > downstream patches until those who care finally make a decision for > > mainline. I'd prefer to solve it properly, but it requires some > > engagement from the community. > > I see. I can probably try spinning a patchset doing explicit mipi_dsi > calls. Let's see if it gains more attention. Is it better to have 2 competing patchsets floating around, or try and get responses on one first? I'll try and get an updated set out today. Whilst the main use case now is DSI devices that want the bus powered up, is it only restricted to DSI or are there potentially other links that have a similar requirement? > It seems something is broken with respect to reviewing of core drm > patches touching strange areas. My patchset improving > drm_bridge_connector HPD also didn't gain a lot of responses. I do appreciate that folks are busy, but there does seem to be a tendency of "it might not be the optimum solution, however I haven't time to think it through so let's just ignore the problem". Particularly when it is purely an in-kernel API with no impact on userspace, I do find that quite frustrating. Dave > > I'll do a respin now second guessing Jagan's comment, and then give it > > a month before giving up > > > > Cheers > > Dave > > > > [1] https://patchwork.freedesktop.org/series/106422/#rev5 > > [2] https://lists.freedesktop.org/archives/dri-devel/2022-November/379693.html > > > > > >>> > >>> Dave > >> > >> -- > >> With best wishes > >> Dmitry > >> > > -- > With best wishes > Dmitry >
On 15/11/2022 17:38, Dave Stevenson wrote: > Hi Dmitry > > On Tue, 15 Nov 2022 at 14:21, Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: >> >> On 15/11/2022 17:14, Dave Stevenson wrote: >>> Hi Dmitry >>> >>> On Sun, 13 Nov 2022 at 13:06, Dmitry Baryshkov >>> <dmitry.baryshkov@linaro.org> wrote: >>>> >>>> Hi Dave, >>>> >>>> On 19/07/2022 16:45, Dave Stevenson wrote: >>>>> Hi Sam >>>>> >>>>> On Mon, 18 Jul 2022 at 21:52, Sam Ravnborg <sam@ravnborg.org> wrote: >>>>>> >>>>>> Hi Dave, >>>>>> >>>>>> a long overdue reply on this series. >>>>>> >>>>>> On Fri, Mar 04, 2022 at 03:17:55PM +0000, Dave Stevenson wrote: >>>>>>> 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. >>>>>> >>>>>> A typically chain looks like this: >>>>>> >>>>>> CRTC => Encoder => Bridge A => Bridge B >>>>>> >>>>>> We have in DRM bridges established what is the "next" bridge - indicated >>>>>> with the direction of the arrows in the drawing. >>>>>> >>>>>> This set of patches introduces the concept of "upstream" bridges. >>>>>> >>>>>> pre_enable_prev_bridge_first would be easier to understand as it uses >>>>>> the current terminology. >>>>>> I get that "upstream" is used in the DSI specification - but we are >>>>>> dealing with bridges that happens to support DSI and more, and mixing >>>>>> the two terminologies is not good. >>>>>> >>>>>> Note: Upstream is also used in a bridge doc section - here it should >>>>>> most likely be updated too. >>>>> >>>>> Sure, I have no issues with switching to prev/next from upstream/downstream. >>>>> To the outsider it can be confusing - in pre_enable and disable, the >>>>> next bridge to be called is the previous one. At least it is >>>>> documented. >>>>> >>>>>> The current approach set a flag that magically makes the core do something >>>>>> else. Have you considered a much more explicit approach? >>>>>> >>>>>> A few helpers like: >>>>>> >>>>>> drm_bridge_pre_enable_prev_bridge() >>>>>> drm_bridge_enable_prev_bridge() >>>>>> drm_bridge_disable_prev_bridge() >>>>>> drm_bridge_post_disable_prev_bridge() >>>>> >>>>> No point in drm_bridge_enable_prev_bridge() and >>>>> drm_bridge_post_disable_prev_bridge() as the call order down the chain >>>>> will mean that they have already been called. >>>>> drm_bridge_enable_next_bridge() and >>>>> drm_bridge_post_disable_next_bridge() possibly. >>>>> >>>>>> And then update the core so the relevant function is only called once >>>>>> for a bridge. >>>>>> Then the need for DSI lanes in LP-11 can be archived by a call to >>>>>> >>>>>> drm_bridge_pre_enable_prev_bridge() >>>>> >>>>> Unfortunately it gets ugly with post_disable. >>>>> The DSI host controller post_disable will have been called before the >>>>> DSI peripheral's post_disable, and there are conditions where the >>>>> peripheral needs to send DSI commands in post_disable (eg >>>>> panel-asus-z00t-tm5p5-n35596 [1]). Changing all DSI hosts to call >>>>> drm_bridge_post_disable_next_bridge feels like the wrong thing to do. >>>>> There are currently hacks in dw-mipi-dsi that do call the next >>>>> panel/bridge post_disable [2] and it would be nice to get rid of them. >>>>> Currently the calls aren't tracked for state, so you end up with >>>>> post_disable being called twice, and panels having to track state (eg >>>>> jdi_lt070me050000 [3]). >>>>> >>>>> [1] tm5p5_nt35596_unprepare() calls tm5p5_nt35596_off() >>>>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c#L107 >>>>> [2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L889 >>>>> [3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c#L44 >>>>> >>>>>> This is more explicit than a flag that triggers some magic behaviour. >>>>>> It may even see uses we have not realised yet. >>>>> >>>>> Personally it feels like more boilerplate in almost all DSI drivers, >>>>> and generally I see a push to remove boilerplate. >>>>> >>>>>> It is late here - so maybe the above is not a good idea tomorrow - but >>>>>> right now I like the simplicity of it. >>>>>> >>>>>> Other than the above I read that a mipi_dsi_host_init() is planned, >>>>>> which is also explicit and simple - good. >>>>> >>>>> It's been raised, but the justification for most use cases hasn't been >>>>> made. The Exynos conversion looks to be doing the wrong thing in >>>>> checking state, and that's why it is currently needing it. >>>>> Again it's also more boilerplate. >>>>> >>>>> TC358767 is an odd one as it wants the DSI interface enabled very >>>>> early in order to have a clock for the DP aux channel well before >>>>> video is running. I had a thought on that, but It looks like I haven't >>>>> hit send on a reply to Lucas on that one - too many distractions. >>>>> >>>>>> Have we seen a new revision of some of these? >>>>>> Chances are high that I have missed it then. >>>>> >>>>> No, still on V2. Other than Dmitry's comment over updating >>>>> parade-ps8640 and dropping drm_bridge_chain_*, no real comments had >>>>> been made. >>>> >>>> It's been a while now. Do you still plan to pursue this patchset? >>> >>> If there was anything that could actually be worked on, then I'm happy >>> to respin it, but if the approach is generally being rejected then I >>> don't want to waste the effort. >>> >>> I'm not totally clear who the maintainers are that the final arbiters >>> and need to sign off on this. >>> drm_bridge.c falls to Maarten, Maxime, and Thomas for "DRM DRIVERS AND >>> MISC GPU PATCHES" >>> drm_panel.c falls to Thierry and Sam for "DRM PANEL DRIVERS", and then >>> Maarten, Maxime, and Thomas. >>> Only Sam has responded publicly. I have had discussions with Maxime, >>> but it's not directly his area of knowledge. >>> >>> Looking at the patch series: >>> Patch 1: Your comment "update parade-ps8640 to use >>> drm_atomic_bridge_chain_". It looks like patchset [1] by Sam does >>> this, but the patchset went wrong and is missing patches 8-11 and >>> therefore hasn't been merged. >>> Patch 2: Comment from Jagan that it's like an old patch. It has >>> similarities, but isn't the same. >>> Patch 3: R-b by you (thank you), but concerns from Jagan which I still >>> don't understand. Without clarification on the issue and whether my >>> suggested alternative place for the hook solves the issue, IMHO it's >>> not worth respinning. >>> Patch 4: R-b Laurent. >>> >>> This cover note got totally subverted with Exynos issues. >>> Sam did request use of prev / next instead of upstream / downstream, >>> which can be done and perhaps warrants a respin now. >>> >>>> [personal notice: I'd prefer something less strange, e.g. an explicit >>>> calls to mipi_dsi_host, but as this patchset seems to fix the issues, >>>> I'm fine with it]. >>> >>> That can fix the power up sequence, but how do you propose telling the >>> DSI controller NOT to power down in post_disable before the DSI >>> peripheral post_disable has potentially sent DCS commands - i.e. the >>> case you were discussing on Friday in [2]. >> >> I thought that the same 'call the parent beforehand' switch applied to >> the deinit paths, didn't it? > > My proposed flag does indeed swap the order of post_disable as well. > > Perhaps I was misunderstanding your personal preference. > I was taking it as an explicit mipi_dsi_host call to initialise the > DSI link, which then also needs an explicit mipi_dsi_host call to tear > it down as well. In that situation there is a need to rework the > bridge chain post_disable to allow for the panel post_disable to send > DCS commands before the DSI host is disabled. > >>> If a panel/bridge driver doesn't call mipi_dsi_host_init then the >>> expectation must be that it will be called by the DSI controller's >>> pre_enable, and deinit from post_disable. Likewise init & deinit would >>> be called if host_transfer is used when the host isn't initialised. >>> >>> If the panel/bridge driver explicitly calls mipi_dsi_host_init, then >>> does that mandate that it must also call mipi_dsi_host_deinint. The >>> controller post_disable is then effectively a no-op. This can be >>> covered in documentation, but also leaves the potential for strange >>> behaviour if the requirement is not followed, and I can't think of a >>> nice place to drop a WARN to flag the issue in the driver. >>> >>> >>> TBH The lack of interest in solving the issues almost makes me want to >>> just document the total brokenness of it and throw in the towel. >>> Seeing as we as Raspberry Pi run a vendor kernel, we can run with >>> downstream patches until those who care finally make a decision for >>> mainline. I'd prefer to solve it properly, but it requires some >>> engagement from the community. >> >> I see. I can probably try spinning a patchset doing explicit mipi_dsi >> calls. Let's see if it gains more attention. > > Is it better to have 2 competing patchsets floating around, or try and > get responses on one first? I'll try and get an updated set out today. I'm a bit in a tough position here. I can't say that I like this approach, but it seems to fix all the issues that we have with DSI hosts, so it's better than the current state. > Whilst the main use case now is DSI devices that want the bus powered > up, is it only restricted to DSI or are there potentially other links > that have a similar requirement? eDP has even more strange requirements: several panels have strict requirement to power it off completely before being able to power it up again (see all the patches from Douglas Anderson to get AUX bus to work properly). And powering up a bus requires waiting for the HPD pin. In the eDP case powering up the bus was solved using autosuspend on the panel and on the host sides. However I don't remember whether eDP has anything comparable to DSI's LP state > >> It seems something is broken with respect to reviewing of core drm >> patches touching strange areas. My patchset improving >> drm_bridge_connector HPD also didn't gain a lot of responses. > > I do appreciate that folks are busy, but there does seem to be a > tendency of "it might not be the optimum solution, however I haven't > time to think it through so let's just ignore the problem". > Particularly when it is purely an in-kernel API with no impact on > userspace, I do find that quite frustrating. Fully agree. > > Dave > >>> I'll do a respin now second guessing Jagan's comment, and then give it >>> a month before giving up >>> >>> Cheers >>> Dave >>> >>> [1] https://patchwork.freedesktop.org/series/106422/#rev5 >>> [2] https://lists.freedesktop.org/archives/dri-devel/2022-November/379693.html
On Thu, Nov 17, 2022 at 03:24:07PM +0200, Dmitry Baryshkov wrote: > On 15/11/2022 17:38, Dave Stevenson wrote: > > Hi Dmitry > > > > On Tue, 15 Nov 2022 at 14:21, Dmitry Baryshkov > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > On 15/11/2022 17:14, Dave Stevenson wrote: > > > > Hi Dmitry > > > > > > > > On Sun, 13 Nov 2022 at 13:06, Dmitry Baryshkov > > > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > > > > > Hi Dave, > > > > > > > > > > On 19/07/2022 16:45, Dave Stevenson wrote: > > > > > > Hi Sam > > > > > > > > > > > > On Mon, 18 Jul 2022 at 21:52, Sam Ravnborg <sam@ravnborg.org> wrote: > > > > > > > > > > > > > > Hi Dave, > > > > > > > > > > > > > > a long overdue reply on this series. > > > > > > > > > > > > > > On Fri, Mar 04, 2022 at 03:17:55PM +0000, Dave Stevenson wrote: > > > > > > > > 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. > > > > > > > > > > > > > > A typically chain looks like this: > > > > > > > > > > > > > > CRTC => Encoder => Bridge A => Bridge B > > > > > > > > > > > > > > We have in DRM bridges established what is the "next" bridge - indicated > > > > > > > with the direction of the arrows in the drawing. > > > > > > > > > > > > > > This set of patches introduces the concept of "upstream" bridges. > > > > > > > > > > > > > > pre_enable_prev_bridge_first would be easier to understand as it uses > > > > > > > the current terminology. > > > > > > > I get that "upstream" is used in the DSI specification - but we are > > > > > > > dealing with bridges that happens to support DSI and more, and mixing > > > > > > > the two terminologies is not good. > > > > > > > > > > > > > > Note: Upstream is also used in a bridge doc section - here it should > > > > > > > most likely be updated too. > > > > > > > > > > > > Sure, I have no issues with switching to prev/next from upstream/downstream. > > > > > > To the outsider it can be confusing - in pre_enable and disable, the > > > > > > next bridge to be called is the previous one. At least it is > > > > > > documented. > > > > > > > > > > > > > The current approach set a flag that magically makes the core do something > > > > > > > else. Have you considered a much more explicit approach? > > > > > > > > > > > > > > A few helpers like: > > > > > > > > > > > > > > drm_bridge_pre_enable_prev_bridge() > > > > > > > drm_bridge_enable_prev_bridge() > > > > > > > drm_bridge_disable_prev_bridge() > > > > > > > drm_bridge_post_disable_prev_bridge() > > > > > > > > > > > > No point in drm_bridge_enable_prev_bridge() and > > > > > > drm_bridge_post_disable_prev_bridge() as the call order down the chain > > > > > > will mean that they have already been called. > > > > > > drm_bridge_enable_next_bridge() and > > > > > > drm_bridge_post_disable_next_bridge() possibly. > > > > > > > > > > > > > And then update the core so the relevant function is only called once > > > > > > > for a bridge. > > > > > > > Then the need for DSI lanes in LP-11 can be archived by a call to > > > > > > > > > > > > > > drm_bridge_pre_enable_prev_bridge() > > > > > > > > > > > > Unfortunately it gets ugly with post_disable. > > > > > > The DSI host controller post_disable will have been called before the > > > > > > DSI peripheral's post_disable, and there are conditions where the > > > > > > peripheral needs to send DSI commands in post_disable (eg > > > > > > panel-asus-z00t-tm5p5-n35596 [1]). Changing all DSI hosts to call > > > > > > drm_bridge_post_disable_next_bridge feels like the wrong thing to do. > > > > > > There are currently hacks in dw-mipi-dsi that do call the next > > > > > > panel/bridge post_disable [2] and it would be nice to get rid of them. > > > > > > Currently the calls aren't tracked for state, so you end up with > > > > > > post_disable being called twice, and panels having to track state (eg > > > > > > jdi_lt070me050000 [3]). > > > > > > > > > > > > [1] tm5p5_nt35596_unprepare() calls tm5p5_nt35596_off() > > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c#L107 > > > > > > [2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L889 > > > > > > [3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c#L44 > > > > > > > > > > > > > This is more explicit than a flag that triggers some magic behaviour. > > > > > > > It may even see uses we have not realised yet. > > > > > > > > > > > > Personally it feels like more boilerplate in almost all DSI drivers, > > > > > > and generally I see a push to remove boilerplate. > > > > > > > > > > > > > It is late here - so maybe the above is not a good idea tomorrow - but > > > > > > > right now I like the simplicity of it. > > > > > > > > > > > > > > Other than the above I read that a mipi_dsi_host_init() is planned, > > > > > > > which is also explicit and simple - good. > > > > > > > > > > > > It's been raised, but the justification for most use cases hasn't been > > > > > > made. The Exynos conversion looks to be doing the wrong thing in > > > > > > checking state, and that's why it is currently needing it. > > > > > > Again it's also more boilerplate. > > > > > > > > > > > > TC358767 is an odd one as it wants the DSI interface enabled very > > > > > > early in order to have a clock for the DP aux channel well before > > > > > > video is running. I had a thought on that, but It looks like I haven't > > > > > > hit send on a reply to Lucas on that one - too many distractions. > > > > > > > > > > > > > Have we seen a new revision of some of these? > > > > > > > Chances are high that I have missed it then. > > > > > > > > > > > > No, still on V2. Other than Dmitry's comment over updating > > > > > > parade-ps8640 and dropping drm_bridge_chain_*, no real comments had > > > > > > been made. > > > > > > > > > > It's been a while now. Do you still plan to pursue this patchset? > > > > > > > > If there was anything that could actually be worked on, then I'm happy > > > > to respin it, but if the approach is generally being rejected then I > > > > don't want to waste the effort. > > > > > > > > I'm not totally clear who the maintainers are that the final arbiters > > > > and need to sign off on this. > > > > drm_bridge.c falls to Maarten, Maxime, and Thomas for "DRM DRIVERS AND > > > > MISC GPU PATCHES" > > > > drm_panel.c falls to Thierry and Sam for "DRM PANEL DRIVERS", and then > > > > Maarten, Maxime, and Thomas. > > > > Only Sam has responded publicly. I have had discussions with Maxime, > > > > but it's not directly his area of knowledge. > > > > > > > > Looking at the patch series: > > > > Patch 1: Your comment "update parade-ps8640 to use > > > > drm_atomic_bridge_chain_". It looks like patchset [1] by Sam does > > > > this, but the patchset went wrong and is missing patches 8-11 and > > > > therefore hasn't been merged. > > > > Patch 2: Comment from Jagan that it's like an old patch. It has > > > > similarities, but isn't the same. > > > > Patch 3: R-b by you (thank you), but concerns from Jagan which I still > > > > don't understand. Without clarification on the issue and whether my > > > > suggested alternative place for the hook solves the issue, IMHO it's > > > > not worth respinning. > > > > Patch 4: R-b Laurent. > > > > > > > > This cover note got totally subverted with Exynos issues. > > > > Sam did request use of prev / next instead of upstream / downstream, > > > > which can be done and perhaps warrants a respin now. > > > > > > > > > [personal notice: I'd prefer something less strange, e.g. an explicit > > > > > calls to mipi_dsi_host, but as this patchset seems to fix the issues, > > > > > I'm fine with it]. > > > > > > > > That can fix the power up sequence, but how do you propose telling the > > > > DSI controller NOT to power down in post_disable before the DSI > > > > peripheral post_disable has potentially sent DCS commands - i.e. the > > > > case you were discussing on Friday in [2]. > > > > > > I thought that the same 'call the parent beforehand' switch applied to > > > the deinit paths, didn't it? > > > > My proposed flag does indeed swap the order of post_disable as well. > > > > Perhaps I was misunderstanding your personal preference. > > I was taking it as an explicit mipi_dsi_host call to initialise the > > DSI link, which then also needs an explicit mipi_dsi_host call to tear > > it down as well. In that situation there is a need to rework the > > bridge chain post_disable to allow for the panel post_disable to send > > DCS commands before the DSI host is disabled. > > > > > > If a panel/bridge driver doesn't call mipi_dsi_host_init then the > > > > expectation must be that it will be called by the DSI controller's > > > > pre_enable, and deinit from post_disable. Likewise init & deinit would > > > > be called if host_transfer is used when the host isn't initialised. > > > > > > > > If the panel/bridge driver explicitly calls mipi_dsi_host_init, then > > > > does that mandate that it must also call mipi_dsi_host_deinint. The > > > > controller post_disable is then effectively a no-op. This can be > > > > covered in documentation, but also leaves the potential for strange > > > > behaviour if the requirement is not followed, and I can't think of a > > > > nice place to drop a WARN to flag the issue in the driver. > > > > > > > > > > > > TBH The lack of interest in solving the issues almost makes me want to > > > > just document the total brokenness of it and throw in the towel. > > > > Seeing as we as Raspberry Pi run a vendor kernel, we can run with > > > > downstream patches until those who care finally make a decision for > > > > mainline. I'd prefer to solve it properly, but it requires some > > > > engagement from the community. > > > > > > I see. I can probably try spinning a patchset doing explicit mipi_dsi > > > calls. Let's see if it gains more attention. > > > > Is it better to have 2 competing patchsets floating around, or try and > > get responses on one first? I'll try and get an updated set out today. > > I'm a bit in a tough position here. I can't say that I like this approach, > but it seems to fix all the issues that we have with DSI hosts, so it's > better than the current state. I'd say the bridge support in general is under-maintained. Historically, Boris and Laurent did most of the architecture work, but are either completely drowning under patches or have moved on. I can't really speak for Thomas and Maarten, but I don't really have such a good knowledge about the bridge infrastructure and haven't been very involved. I have the same impression from Maarten and Thomas though. Which means that it's pretty much a blindspot for us :) I'm sorry if it's taking a while, but I'd say that if you two have a good comprehension of the issue (and I know Dave has), if you can reach a reasonable solution for both of you, and if there is proper documentation for the new work, I'd consider this a net improvement. And as far as I know from that discussion, we're pretty much there already. So yeah, go ahead with a new version and we'll merge it. Maxime
On 17/11/2022 17:34, Maxime Ripard wrote: > On Thu, Nov 17, 2022 at 03:24:07PM +0200, Dmitry Baryshkov wrote: >> On 15/11/2022 17:38, Dave Stevenson wrote: >>> Hi Dmitry >>> >>> On Tue, 15 Nov 2022 at 14:21, Dmitry Baryshkov >>> <dmitry.baryshkov@linaro.org> wrote: >>>> >>>> On 15/11/2022 17:14, Dave Stevenson wrote: >>>>> Hi Dmitry >>>>> >>>>> On Sun, 13 Nov 2022 at 13:06, Dmitry Baryshkov >>>>> <dmitry.baryshkov@linaro.org> wrote: >>>>>> >>>>>> Hi Dave, >>>>>> >>>>>> On 19/07/2022 16:45, Dave Stevenson wrote: >>>>>>> Hi Sam >>>>>>> >>>>>>> On Mon, 18 Jul 2022 at 21:52, Sam Ravnborg <sam@ravnborg.org> wrote: >>>>>>>> >>>>>>>> Hi Dave, >>>>>>>> >>>>>>>> a long overdue reply on this series. >>>>>>>> >>>>>>>> On Fri, Mar 04, 2022 at 03:17:55PM +0000, Dave Stevenson wrote: >>>>>>>>> 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. >>>>>>>> >>>>>>>> A typically chain looks like this: >>>>>>>> >>>>>>>> CRTC => Encoder => Bridge A => Bridge B >>>>>>>> >>>>>>>> We have in DRM bridges established what is the "next" bridge - indicated >>>>>>>> with the direction of the arrows in the drawing. >>>>>>>> >>>>>>>> This set of patches introduces the concept of "upstream" bridges. >>>>>>>> >>>>>>>> pre_enable_prev_bridge_first would be easier to understand as it uses >>>>>>>> the current terminology. >>>>>>>> I get that "upstream" is used in the DSI specification - but we are >>>>>>>> dealing with bridges that happens to support DSI and more, and mixing >>>>>>>> the two terminologies is not good. >>>>>>>> >>>>>>>> Note: Upstream is also used in a bridge doc section - here it should >>>>>>>> most likely be updated too. >>>>>>> >>>>>>> Sure, I have no issues with switching to prev/next from upstream/downstream. >>>>>>> To the outsider it can be confusing - in pre_enable and disable, the >>>>>>> next bridge to be called is the previous one. At least it is >>>>>>> documented. >>>>>>> >>>>>>>> The current approach set a flag that magically makes the core do something >>>>>>>> else. Have you considered a much more explicit approach? >>>>>>>> >>>>>>>> A few helpers like: >>>>>>>> >>>>>>>> drm_bridge_pre_enable_prev_bridge() >>>>>>>> drm_bridge_enable_prev_bridge() >>>>>>>> drm_bridge_disable_prev_bridge() >>>>>>>> drm_bridge_post_disable_prev_bridge() >>>>>>> >>>>>>> No point in drm_bridge_enable_prev_bridge() and >>>>>>> drm_bridge_post_disable_prev_bridge() as the call order down the chain >>>>>>> will mean that they have already been called. >>>>>>> drm_bridge_enable_next_bridge() and >>>>>>> drm_bridge_post_disable_next_bridge() possibly. >>>>>>> >>>>>>>> And then update the core so the relevant function is only called once >>>>>>>> for a bridge. >>>>>>>> Then the need for DSI lanes in LP-11 can be archived by a call to >>>>>>>> >>>>>>>> drm_bridge_pre_enable_prev_bridge() >>>>>>> >>>>>>> Unfortunately it gets ugly with post_disable. >>>>>>> The DSI host controller post_disable will have been called before the >>>>>>> DSI peripheral's post_disable, and there are conditions where the >>>>>>> peripheral needs to send DSI commands in post_disable (eg >>>>>>> panel-asus-z00t-tm5p5-n35596 [1]). Changing all DSI hosts to call >>>>>>> drm_bridge_post_disable_next_bridge feels like the wrong thing to do. >>>>>>> There are currently hacks in dw-mipi-dsi that do call the next >>>>>>> panel/bridge post_disable [2] and it would be nice to get rid of them. >>>>>>> Currently the calls aren't tracked for state, so you end up with >>>>>>> post_disable being called twice, and panels having to track state (eg >>>>>>> jdi_lt070me050000 [3]). >>>>>>> >>>>>>> [1] tm5p5_nt35596_unprepare() calls tm5p5_nt35596_off() >>>>>>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c#L107 >>>>>>> [2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L889 >>>>>>> [3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c#L44 >>>>>>> >>>>>>>> This is more explicit than a flag that triggers some magic behaviour. >>>>>>>> It may even see uses we have not realised yet. >>>>>>> >>>>>>> Personally it feels like more boilerplate in almost all DSI drivers, >>>>>>> and generally I see a push to remove boilerplate. >>>>>>> >>>>>>>> It is late here - so maybe the above is not a good idea tomorrow - but >>>>>>>> right now I like the simplicity of it. >>>>>>>> >>>>>>>> Other than the above I read that a mipi_dsi_host_init() is planned, >>>>>>>> which is also explicit and simple - good. >>>>>>> >>>>>>> It's been raised, but the justification for most use cases hasn't been >>>>>>> made. The Exynos conversion looks to be doing the wrong thing in >>>>>>> checking state, and that's why it is currently needing it. >>>>>>> Again it's also more boilerplate. >>>>>>> >>>>>>> TC358767 is an odd one as it wants the DSI interface enabled very >>>>>>> early in order to have a clock for the DP aux channel well before >>>>>>> video is running. I had a thought on that, but It looks like I haven't >>>>>>> hit send on a reply to Lucas on that one - too many distractions. >>>>>>> >>>>>>>> Have we seen a new revision of some of these? >>>>>>>> Chances are high that I have missed it then. >>>>>>> >>>>>>> No, still on V2. Other than Dmitry's comment over updating >>>>>>> parade-ps8640 and dropping drm_bridge_chain_*, no real comments had >>>>>>> been made. >>>>>> >>>>>> It's been a while now. Do you still plan to pursue this patchset? >>>>> >>>>> If there was anything that could actually be worked on, then I'm happy >>>>> to respin it, but if the approach is generally being rejected then I >>>>> don't want to waste the effort. >>>>> >>>>> I'm not totally clear who the maintainers are that the final arbiters >>>>> and need to sign off on this. >>>>> drm_bridge.c falls to Maarten, Maxime, and Thomas for "DRM DRIVERS AND >>>>> MISC GPU PATCHES" >>>>> drm_panel.c falls to Thierry and Sam for "DRM PANEL DRIVERS", and then >>>>> Maarten, Maxime, and Thomas. >>>>> Only Sam has responded publicly. I have had discussions with Maxime, >>>>> but it's not directly his area of knowledge. >>>>> >>>>> Looking at the patch series: >>>>> Patch 1: Your comment "update parade-ps8640 to use >>>>> drm_atomic_bridge_chain_". It looks like patchset [1] by Sam does >>>>> this, but the patchset went wrong and is missing patches 8-11 and >>>>> therefore hasn't been merged. >>>>> Patch 2: Comment from Jagan that it's like an old patch. It has >>>>> similarities, but isn't the same. >>>>> Patch 3: R-b by you (thank you), but concerns from Jagan which I still >>>>> don't understand. Without clarification on the issue and whether my >>>>> suggested alternative place for the hook solves the issue, IMHO it's >>>>> not worth respinning. >>>>> Patch 4: R-b Laurent. >>>>> >>>>> This cover note got totally subverted with Exynos issues. >>>>> Sam did request use of prev / next instead of upstream / downstream, >>>>> which can be done and perhaps warrants a respin now. >>>>> >>>>>> [personal notice: I'd prefer something less strange, e.g. an explicit >>>>>> calls to mipi_dsi_host, but as this patchset seems to fix the issues, >>>>>> I'm fine with it]. >>>>> >>>>> That can fix the power up sequence, but how do you propose telling the >>>>> DSI controller NOT to power down in post_disable before the DSI >>>>> peripheral post_disable has potentially sent DCS commands - i.e. the >>>>> case you were discussing on Friday in [2]. >>>> >>>> I thought that the same 'call the parent beforehand' switch applied to >>>> the deinit paths, didn't it? >>> >>> My proposed flag does indeed swap the order of post_disable as well. >>> >>> Perhaps I was misunderstanding your personal preference. >>> I was taking it as an explicit mipi_dsi_host call to initialise the >>> DSI link, which then also needs an explicit mipi_dsi_host call to tear >>> it down as well. In that situation there is a need to rework the >>> bridge chain post_disable to allow for the panel post_disable to send >>> DCS commands before the DSI host is disabled. >>> >>>>> If a panel/bridge driver doesn't call mipi_dsi_host_init then the >>>>> expectation must be that it will be called by the DSI controller's >>>>> pre_enable, and deinit from post_disable. Likewise init & deinit would >>>>> be called if host_transfer is used when the host isn't initialised. >>>>> >>>>> If the panel/bridge driver explicitly calls mipi_dsi_host_init, then >>>>> does that mandate that it must also call mipi_dsi_host_deinint. The >>>>> controller post_disable is then effectively a no-op. This can be >>>>> covered in documentation, but also leaves the potential for strange >>>>> behaviour if the requirement is not followed, and I can't think of a >>>>> nice place to drop a WARN to flag the issue in the driver. >>>>> >>>>> >>>>> TBH The lack of interest in solving the issues almost makes me want to >>>>> just document the total brokenness of it and throw in the towel. >>>>> Seeing as we as Raspberry Pi run a vendor kernel, we can run with >>>>> downstream patches until those who care finally make a decision for >>>>> mainline. I'd prefer to solve it properly, but it requires some >>>>> engagement from the community. >>>> >>>> I see. I can probably try spinning a patchset doing explicit mipi_dsi >>>> calls. Let's see if it gains more attention. >>> >>> Is it better to have 2 competing patchsets floating around, or try and >>> get responses on one first? I'll try and get an updated set out today. >> >> I'm a bit in a tough position here. I can't say that I like this approach, >> but it seems to fix all the issues that we have with DSI hosts, so it's >> better than the current state. > > I'd say the bridge support in general is under-maintained. Historically, > Boris and Laurent did most of the architecture work, but are either > completely drowning under patches or have moved on. > > I can't really speak for Thomas and Maarten, but I don't really have > such a good knowledge about the bridge infrastructure and haven't been > very involved. I have the same impression from Maarten and Thomas > though. > > Which means that it's pretty much a blindspot for us :) > > I'm sorry if it's taking a while, but I'd say that if you two have a > good comprehension of the issue (and I know Dave has), if you can reach > a reasonable solution for both of you, and if there is proper > documentation for the new work, I'd consider this a net improvement. > > And as far as I know from that discussion, we're pretty much there > already. So yeah, go ahead with a new version and we'll merge it. Ack, then I'll hold on my proposal to let Dave's version to be merged.
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/synopsys/dw-mipi-dsi.c#L940 [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(-)