On 24.12.2019 10:44, Boris Brezillon wrote: > On Tue, 24 Dec 2019 10:16:49 +0100 > Andrzej Hajda <a.hajda@xxxxxxxxxxx> wrote: > >> On 23.12.2019 10:55, Marek Szyprowski wrote: >>> Hi Boris, >>> >>> On 16.12.2019 16:25, Boris Brezillon wrote: >>>> On Mon, 16 Dec 2019 16:02:36 +0100 >>>> Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> wrote: >>>>> Hi Boris, >>>>> >>>>> On 16.12.2019 15:55, Boris Brezillon wrote: >>>>>> On Mon, 16 Dec 2019 14:54:25 +0100 >>>>>> Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> wrote: >>>>>>> On 03.12.2019 15:15, Boris Brezillon wrote: >>>>>>>> So that each element in the chain can easily access its predecessor. >>>>>>>> This will be needed to support bus format negotiation between elements >>>>>>>> of the bridge chain. >>>>>>>> >>>>>>>> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> >>>>>>>> Reviewed-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx> >>>>>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> >>>>>>> I've noticed that this patch got merged to linux-next as commit >>>>>>> 05193dc38197021894b17239fafbd2eb1afe5a45. Sadly it breaks booting of >>>>>>> Samsung Exynos5250-based Arndale board. Booting stops after following >>>>>>> messages: >>>>>>> >>>>>>> [drm] Exynos DRM: using 14400000.fimd device for DMA mapping operations >>>>>>> exynos-drm exynos-drm: bound 14400000.fimd (ops fimd_component_ops) >>>>>>> exynos-drm exynos-drm: bound 14450000.mixer (ops mixer_component_ops) >>>>>>> exynos-drm exynos-drm: bound 14500000.dsi (ops exynos_dsi_component_ops) >>>>>>> exynos-drm exynos-drm: bound 14530000.hdmi (ops hdmi_component_ops) >>>>>>> [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). >>>>>>> [drm] No driver support for vblank timestamp query. >>>>>>> [drm] Cannot find any crtc or sizes >>>>>>> [drm] Cannot find any crtc or sizes >>>>>>> [drm] Initialized exynos 1.1.0 20180330 for exynos-drm on minor 0 >>>>>>> >>>>>>> I will try to debug this and provide more information soon. >>>>>>> >>>>>> Can you try with this diff applied? >>>>> This patch doesn't change anything. >>>> Okay. Can you do a list_for_each_entry() on both encoder->bridge_chain >>>> and dsi->bridge_chain (dump bridge pointers in a pr_info()) before and >>>> after the list_splice_init() call? >>> encoder->bridge_chain contains only one element. dsi->drive_chain is empty. >>> >>> Replacing that list_splice() with INIT_LIST_HEAD(&encoder->bridge_chain) >>> fixed the boot issue. > If INIT_LIST_HEAD() worked, I don't understand why replacing the > list_splice() call by a list_splice_init() (which doing a list_splice() > + INIT_LIST_HEAD()) didn't fix the problem. Are you sure the > list_splice_init() version doesn't work? > >>> It looks that this is related with the way the >>> Exynos DSI handles bridges (in bridge and out brige?). Maybe Andrzej >>> will give a bit more detailed comment and spread some light on this. >> >> Hi Marek, Boris, >> >> >> I have not followed latest patches due to high work load, my bad. Marek >> thanks from pointing >> >> About ExynosDSI bridge handling: >> >> The order of calling encoder, bridge (and consequently panel) ops >> enforced by DRM core (bridge->pre_enable, encoder->enable, >> bridge->enable) does not fit to ExynosDSI hardware initialization >> sequence, if I remember correctly it does not fit to whole MIPI DSI >> standard (I think similar situation is with eDP). As a result DSI >> drivers must use some ugly workarounds, rely on HW properly coping with >> incorrect sequences, or, as in case of ExynosDSI driver, just avoid >> using encoder->bridge chaining and call bridge ops by itself when suitable. > Yes, that's definitely hack-ish, and I proposed 2 solutions to address > that in previous versions of this patchset, unfortunately I didn't get > any feedback so I went for the less invasive option (keep the hack but > adapt it to the double-linked list changes), which still lead to > regressions :-/. > > Just a reminder of my 2 proposals: > > 1/ implement the bridge_ops->pre_enable/post_disable() hooks so you can > split your enable/disable logic in 2 parts and make sure things are > ready when the panel/next bridge tries to send DSI commands If it means 'convert exynos_dsi to bridge' I do not think it will help - - pre_enable op will be still called after pre_enable op of downstream bridge - and this is the main reason why exynos_dsi do not use encoder bridge chain - it needs to perform some operations BEFORE (pre)enabling downstream devices. > 2/ move everything that's needed to send DSI commands out of the > ->enable() path (maybe in runtime PM resume/suspend hooks) so you > can call that in the DSI transfer path too It looks like a solution for DSI protocol, where control bus is shared with data bus, but the problem is more general - we have source and sink connected with some local bus, which has some negotiation/enable/disable protocol/requirements. And drm_core/bridge framework enforces us to fit every such protocol to 'drm_bridge protocol' with few opses called in fixed order, without clearly defined purpose of each ops. That does not sound generic and results in multiple issues: - different drivers uses different opses to perform the same thing, - different drivers assumes different things about their sinks/sources in their opses, - more complicated sequences does not fit at all to this model. All this results in incompatibilities between drivers which become visible with devices used in different configurations/platforms. Regards Andrzej > > As pointed out by Laurent, #1 doesn't work because some panel drivers > send DSI commands in their ->prepare() hook, and ->pre_enable() methods > are called in reverse order, meaning that the DRM panel bridge driver > would try to issue DSI commands before the DSI host controllers is ready > to send them. I still thing #2 is a good option. > >> So proper patch converting to double-linked list should not try to >> splice ExynosDSI private bridge list with with encoder's, encoder's list >> should be always empty, as Marek suggested. > That's exactly what I wanted to do: make the encoder's list empty after > attach() and restore it to its initial state before unregistering > the bridge, except I forgot that list_splice() doesn't call > INIT_LIST_HEAD(). It's still not clear to me why replacing the > list_splice() call by a list_splice_init() didn't work. > Also note that calling INIT_LIST_HEAD() only works if you have one > bridge in the chain, so if we go for that option we need a comment > explaining the limitations of this approach. >
