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

[Andrzej Hajda]

On 24.12.2019 10:44, Boris Brezillon wrote:
> On Tue, 24 Dec 2019 10:16:49 +0100
> Andrzej Hajda <a.hajda at samsung.com> 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 at samsung.com> 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 at 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 at collabora.com>
>>>>>>>> Reviewed-by: Neil Armstrong <narmstrong at baylibre.com>
>>>>>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart at 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


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.
>