From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_2 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9136BC2D0D2 for ; Tue, 24 Dec 2019 10:03:15 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 6711A20722 for ; Tue, 24 Dec 2019 10:03:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726091AbfLXKDO (ORCPT ); Tue, 24 Dec 2019 05:03:14 -0500 Received: from bhuna.collabora.co.uk ([46.235.227.227]:42950 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726076AbfLXKDO (ORCPT ); Tue, 24 Dec 2019 05:03:14 -0500 Received: from localhost (unknown [IPv6:2a01:e0a:2c:6930:5cf4:84a1:2763:fe0d]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) (Authenticated sender: bbrezillon) by bhuna.collabora.co.uk (Postfix) with ESMTPSA id BF61928FA57; Tue, 24 Dec 2019 10:03:10 +0000 (GMT) Date: Tue, 24 Dec 2019 11:03:07 +0100 From: Boris Brezillon To: Andrzej Hajda Cc: Mark Rutland , Nikita Yushchenko , Jernej Skrabec , Jonas Karlman , Andrey Smirnov , Neil Armstrong , Seung-Woo Kim , Kyungmin Park , dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org, Rob Herring , Thierry Reding , Laurent Pinchart , kernel@collabora.com, Sam Ravnborg , Chris Healy , Marek Szyprowski Subject: Re: [PATCH v4 04/11] drm/bridge: Make the bridge chain a double-linked list Message-ID: <20191224110307.00ca841d@collabora.com> In-Reply-To: <20191224104936.6a7c4977@collabora.com> References: <20191203141515.3597631-1-boris.brezillon@collabora.com> <20191203141515.3597631-5-boris.brezillon@collabora.com> <4e901ab9-07d4-4238-7322-c7c5a3959513@samsung.com> <20191216155551.083dcbaf@collabora.com> <75a06e2a-4587-ee16-0f5d-af75fbe89793@samsung.com> <20191216162542.261c821c@collabora.com> <60f03d50-7c0f-c3d0-920f-0625c08b2171@samsung.com> <1010f5fc-0672-643c-4410-e053a928cb66@samsung.com> <20191224104422.25dbf980@collabora.com> <20191224104936.6a7c4977@collabora.com> Organization: Collabora X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: devicetree-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On Tue, 24 Dec 2019 10:49:36 +0100 Boris Brezillon wrote: > On Tue, 24 Dec 2019 10:44:22 +0100 > Boris Brezillon wrote: > > > On Tue, 24 Dec 2019 10:16:49 +0100 > > Andrzej Hajda 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 wrote: > > > >>> Hi Boris, > > > >>> > > > >>> On 16.12.2019 15:55, Boris Brezillon wrote: > > > >>>> On Mon, 16 Dec 2019 14:54:25 +0100 > > > >>>> Marek Szyprowski 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 > > > >>>>>> Reviewed-by: Neil Armstrong > > > >>>>>> Reviewed-by: Laurent Pinchart > > > >>>>> 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. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_2 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 80E5AC2D0CF for ; Tue, 24 Dec 2019 10:03:14 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 5D818206B7 for ; Tue, 24 Dec 2019 10:03:14 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5D818206B7 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=collabora.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 820D56E321; Tue, 24 Dec 2019 10:03:13 +0000 (UTC) Received: from bhuna.collabora.co.uk (bhuna.collabora.co.uk [IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3]) by gabe.freedesktop.org (Postfix) with ESMTPS id 750646E321 for ; Tue, 24 Dec 2019 10:03:12 +0000 (UTC) Received: from localhost (unknown [IPv6:2a01:e0a:2c:6930:5cf4:84a1:2763:fe0d]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) (Authenticated sender: bbrezillon) by bhuna.collabora.co.uk (Postfix) with ESMTPSA id BF61928FA57; Tue, 24 Dec 2019 10:03:10 +0000 (GMT) Date: Tue, 24 Dec 2019 11:03:07 +0100 From: Boris Brezillon To: Andrzej Hajda Subject: Re: [PATCH v4 04/11] drm/bridge: Make the bridge chain a double-linked list Message-ID: <20191224110307.00ca841d@collabora.com> In-Reply-To: <20191224104936.6a7c4977@collabora.com> References: <20191203141515.3597631-1-boris.brezillon@collabora.com> <20191203141515.3597631-5-boris.brezillon@collabora.com> <4e901ab9-07d4-4238-7322-c7c5a3959513@samsung.com> <20191216155551.083dcbaf@collabora.com> <75a06e2a-4587-ee16-0f5d-af75fbe89793@samsung.com> <20191216162542.261c821c@collabora.com> <60f03d50-7c0f-c3d0-920f-0625c08b2171@samsung.com> <1010f5fc-0672-643c-4410-e053a928cb66@samsung.com> <20191224104422.25dbf980@collabora.com> <20191224104936.6a7c4977@collabora.com> Organization: Collabora X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; x86_64-redhat-linux-gnu) MIME-Version: 1.0 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , Nikita Yushchenko , Jernej Skrabec , Neil Armstrong , Andrey Smirnov , Jonas Karlman , Seung-Woo Kim , dri-devel@lists.freedesktop.org, Kyungmin Park , Rob Herring , Thierry Reding , Laurent Pinchart , kernel@collabora.com, Sam Ravnborg , Chris Healy , devicetree@vger.kernel.org, Marek Szyprowski Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Tue, 24 Dec 2019 10:49:36 +0100 Boris Brezillon wrote: > On Tue, 24 Dec 2019 10:44:22 +0100 > Boris Brezillon wrote: > > > On Tue, 24 Dec 2019 10:16:49 +0100 > > Andrzej Hajda 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 wrote: > > > >>> Hi Boris, > > > >>> > > > >>> On 16.12.2019 15:55, Boris Brezillon wrote: > > > >>>> On Mon, 16 Dec 2019 14:54:25 +0100 > > > >>>> Marek Szyprowski 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 > > > >>>>>> Reviewed-by: Neil Armstrong > > > >>>>>> Reviewed-by: Laurent Pinchart > > > >>>>> 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. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel