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:
Hi Boris,
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:
Hi Boris,
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. 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.