Re: [PATCH v4 04/11] drm/bridge: Make the bridge chain a double-linked list

[Marek Szyprowski <m.szyprowski@samsung.com>]

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@collabora.com> wrote:
>>> On Tue, 24 Dec 2019 10:44:22 +0100
>>> Boris Brezillon <boris.brezillon@collabora.com> wrote:
>>>> On Tue, 24 Dec 2019 10:16:49 +0100
>>>> Andrzej Hajda <a.hajda@samsung.com> 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@samsung.com> wrote:
>>>>>>>> On 16.12.2019 15:55, Boris Brezillon wrote:
>>>>>>>>> On Mon, 16 Dec 2019 14:54:25 +0100
>>>>>>>>> Marek Szyprowski <m.szyprowski@samsung.com> 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@collabora.com>
>>>>>>>>>>> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
>>>>>>>>>>> Reviewed-by: Laurent Pinchart 
>>>>>>>>>>> <laurent.pinchart@ideasonboard.com>
>>>>>>>>>> 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