Hi All, On 27.12.2019 11:25, Marek Szyprowski wrote: > On 24.12.2019 11:03, Boris Brezillon wrote: >> On Tue, 24 Dec 2019 10:49:36 +0100 >> Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote: >>> On Tue, 24 Dec 2019 10:44:22 +0100 >>> Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> 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: >>>>>> On 16.12.2019 16:25, Boris Brezillon wrote: >>>>>>> On Mon, 16 Dec 2019 16:02:36 +0100 >>>>>>> Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> wrote: >>>>>>>> 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 >>>> 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 >>>> >>>> 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. >>> Okay, I think I figured it out: drm_bridge_chain_xx() helpers use >>> encoder->bridge_chain as their list head, and you'll never hit the >>> 'elem >>> is list head' condition since we moved all elems from >>> encoder->bridge_chain to exynos_dsi->bridge_chain. The only way this >>> can work is if we stop using the helpers and implement our own list >>> iterators. >> Just to make it clear, calling INIT_LIST_HEAD(encoder->bridge_chain) >> doesn't really fix the bug, it just prevents the hang (infinite loop) >> and turn all drm_bridge_chain_xx() calls into NOPs. > > Right, I've just checked it and indeed the display chain outputs > nothing after such 'fix'. To get it finally working I've replaced > drm_bridge_chain_*() operations for exynos_dsi 'out_bridge' by a > direct calls. I will submit a patch in a few minutes. I hope that such > workaround can be used now to fix the regression until a better > solution is agreed. I've posted a 'quick & working' fix for Exynos DRM DSI driver: https://lkml.org/lkml/2019/12/27/121 Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland
