Re: [PATCH v4 04/11] drm/bridge: Make the bridge chain a double-linked list
Hi Boris, On Fri, Dec 27, 2019 at 01:21:31PM +0100, Boris Brezillon wrote: > On Fri, 27 Dec 2019 12:51:54 +0200 Laurent Pinchart wrote: > > On Fri, Dec 27, 2019 at 10:42:25AM +0100, Andrzej Hajda wrote: > > > On 24.12.2019 10:44, Boris Brezillon wrote: > > > > On Tue, 24 Dec 2019 10:16:49 +0100 Andrzej Hajda 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 wrote: > > > > 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 1440.fimd device for DMA mapping > > > >>> operations > > > >>> exynos-drm exynos-drm: bound 1440.fimd (ops > > > >>> fimd_component_ops) > > > >>> exynos-drm exynos-drm: bound 1445.mixer (ops > > > >>> mixer_component_ops) > > > >>> exynos-drm exynos-drm: bound 1450.dsi (ops > > > >>> exynos_dsi_component_ops) > > > >>> exynos-drm exynos-drm: bound 1453.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(>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
Re: [PATCH v4 04/11] drm/bridge: Make the bridge chain a double-linked list
On Tue, 24 Dec 2019 12:31:11 +0100 Sam Ravnborg wrote: > Hi Boris. > > > 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. > > Jitao Shi suggested to extend panels so we had a sequence of: > > prepare_power() <= new callback, >here one should NOT be allowed to send >DSI commands > prepare() > enable() > ># ># panel is now ready to show your favourite christmas movie ># > > disable() > unprepare() > unprepare_power() <= new callback > > > Would this help implement what you suggest above? > Relevant panels would then have to be updated - but this > is doable. I didn't look at Jitao's proposal but it looks like it's addressing a similar issue on the DSI slave/device side: the device probably needs to be powered before the host can interact with it through the DSI+DPHY bus. I'm not entirely sure why we'd need another hook to do that since we already have the ->prepare() one. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 04/11] drm/bridge: Make the bridge chain a double-linked list
On Fri, 27 Dec 2019 12:51:54 +0200 Laurent Pinchart wrote: > Hi Andrzej, > > On Fri, Dec 27, 2019 at 10:42:25AM +0100, Andrzej Hajda wrote: > > On 24.12.2019 10:44, Boris Brezillon wrote: > > > On Tue, 24 Dec 2019 10:16:49 +0100 Andrzej Hajda 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 wrote: > > > 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 1440.fimd device for DMA mapping > > >>> operations > > >>> exynos-drm exynos-drm: bound 1440.fimd (ops fimd_component_ops) > > >>> exynos-drm exynos-drm: bound 1445.mixer (ops > > >>> mixer_component_ops) > > >>> exynos-drm exynos-drm: bound 1450.dsi (ops > > >>> exynos_dsi_component_ops) > > >>> exynos-drm exynos-drm: bound 1453.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(>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. Yep, I
Re: [PATCH v4 04/11] drm/bridge: Make the bridge chain a double-linked list
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 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: >> On 16.12.2019 16:25, Boris Brezillon wrote: >>> On Mon, 16 Dec 2019 16:02:36 +0100 >>> Marek Szyprowski wrote: 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 1440.fimd device for DMA mapping >> operations >> exynos-drm exynos-drm: bound 1440.fimd (ops >> fimd_component_ops) >> exynos-drm exynos-drm: bound 1445.mixer (ops >> mixer_component_ops) >> exynos-drm exynos-drm: bound 1450.dsi (ops >> exynos_dsi_component_ops) >> exynos-drm exynos-drm: bound 1453.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(>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
Re: [PATCH v4 04/11] drm/bridge: Make the bridge chain a double-linked list
Hi Andrzej, On Fri, Dec 27, 2019 at 10:42:25AM +0100, Andrzej Hajda wrote: > On 24.12.2019 10:44, Boris Brezillon wrote: > > On Tue, 24 Dec 2019 10:16:49 +0100 Andrzej Hajda 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 wrote: > > 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 1440.fimd device for DMA mapping > >>> operations > >>> exynos-drm exynos-drm: bound 1440.fimd (ops fimd_component_ops) > >>> exynos-drm exynos-drm: bound 1445.mixer (ops mixer_component_ops) > >>> exynos-drm exynos-drm: bound 1450.dsi (ops > >>> exynos_dsi_component_ops) > >>> exynos-drm exynos-drm: bound 1453.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(>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,
Re: [PATCH v4 04/11] drm/bridge: Make the bridge chain a double-linked list
Hi Boris, On 24.12.2019 11:03, Boris Brezillon wrote: > 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: > On 16.12.2019 16:25, Boris Brezillon wrote: >> On Mon, 16 Dec 2019 16:02:36 +0100 >> Marek Szyprowski wrote: >>> 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 1440.fimd device for DMA mapping > operations > exynos-drm exynos-drm: bound 1440.fimd (ops fimd_component_ops) > exynos-drm exynos-drm: bound 1445.mixer (ops mixer_component_ops) > exynos-drm exynos-drm: bound 1450.dsi (ops > exynos_dsi_component_ops) > exynos-drm exynos-drm: bound 1453.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(>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
Re: [PATCH v4 04/11] drm/bridge: Make the bridge chain a double-linked list
On 24.12.2019 10:44, 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 1440.fimd device for DMA mapping operations >>> exynos-drm exynos-drm: bound 1440.fimd (ops fimd_component_ops) >>> exynos-drm exynos-drm: bound 1445.mixer (ops mixer_component_ops) >>> exynos-drm exynos-drm: bound 1450.dsi (ops exynos_dsi_component_ops) >>> exynos-drm exynos-drm: bound 1453.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(>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
Re: [PATCH v4 04/11] drm/bridge: Make the bridge chain a double-linked list
Hi Sam, On Tue, Dec 24, 2019 at 12:31:11PM +0100, Sam Ravnborg wrote: > > 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. > > Jitao Shi suggested to extend panels so we had a sequence of: > > prepare_power() <= new callback, >here one should NOT be allowed to send >DSI commands > prepare() > enable() > ># ># panel is now ready to show your favourite christmas movie ># > > disable() > unprepare() > unprepare_power() <= new callback Please note that you will need corresponding bridge operations in order to implement the DRM panel bridge. > Would this help implement what you suggest above? > Relevant panels would then have to be updated - but this > is doable. The fundamental issue is that the enable/disable sequence requirements are inherently driven by sinks, while the bridge API goes from source to sink. A DSI panel could for instance require the following enable sequence. 0. DSI bus disabled (LP-00 state) 1. Power up the panel 2. Enable the DSI bus (LP-11 state) 3. Configure the panel (through DCS LP mode writes, SPI, GPIOs, ...) 4. Transition the source to HS mode, sending sync packets, but no data 5. Perform additional configuration for the panel 6. Enable transmission of video from the source For a given bus type there are thus well-specified states for a source (in this case disabled, enabled in LP mode, in HS mode, sending sync packets, sending video packets). As our API operates from source to sink, fine-grained control of bridges is difficult. We can of course always make bridge and panel operations more fine-grained, but that's more of a ad-hoc solution that may lead to abuse. We would at the very least document very explicitly what each operation would be allowed to do for every bus type. -- Regards, Laurent Pinchart ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 04/11] drm/bridge: Make the bridge chain a double-linked list
Hi Boris. > 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. Jitao Shi suggested to extend panels so we had a sequence of: prepare_power() <= new callback, here one should NOT be allowed to send DSI commands prepare() enable() # # panel is now ready to show your favourite christmas movie # disable() unprepare() unprepare_power() <= new callback Would this help implement what you suggest above? Relevant panels would then have to be updated - but this is doable. Sam ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 04/11] drm/bridge: Make the bridge chain a double-linked list
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 1440.fimd device for DMA mapping > > > > operations > > > > exynos-drm exynos-drm: bound 1440.fimd (ops fimd_component_ops) > > > > exynos-drm exynos-drm: bound 1445.mixer (ops > > > > mixer_component_ops) > > > > exynos-drm exynos-drm: bound 1450.dsi (ops > > > > exynos_dsi_component_ops) > > > > exynos-drm exynos-drm: bound 1453.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(>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
Re: [PATCH v4 04/11] drm/bridge: Make the bridge chain a double-linked list
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 1440.fimd device for DMA mapping > > > operations > > > exynos-drm exynos-drm: bound 1440.fimd (ops fimd_component_ops) > > > exynos-drm exynos-drm: bound 1445.mixer (ops mixer_component_ops) > > > exynos-drm exynos-drm: bound 1450.dsi (ops > > > exynos_dsi_component_ops) > > > exynos-drm exynos-drm: bound 1453.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(>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
Re: [PATCH v4 04/11] drm/bridge: Make the bridge chain a double-linked list
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 1440.fimd device for DMA mapping operations > > exynos-drm exynos-drm: bound 1440.fimd (ops fimd_component_ops) > > exynos-drm exynos-drm: bound 1445.mixer (ops mixer_component_ops) > > exynos-drm exynos-drm: bound 1450.dsi (ops exynos_dsi_component_ops) > > exynos-drm exynos-drm: bound 1453.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(>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
Re: [PATCH v4 04/11] drm/bridge: Make the bridge chain a double-linked list
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 1440.fimd device for DMA mapping operations
> exynos-drm exynos-drm: bound 1440.fimd (ops fimd_component_ops)
> exynos-drm exynos-drm: bound 1445.mixer (ops mixer_component_ops)
> exynos-drm exynos-drm: bound 1450.dsi (ops exynos_dsi_component_ops)
> exynos-drm exynos-drm: bound 1453.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(>bridge_chain)
> fixed the boot issue. 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.
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.
Regards
Andrzej
>
> I can send a formal patch fixing this if You want.
>
--->8---
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 3955f84dc893..118ecedc7621 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -1523,7 +1523,7 @@ static int exynos_dsi_host_attach(struct
mipi_dsi_host *host,
if (out_bridge) {
drm_bridge_attach(encoder, out_bridge, NULL);
dsi->out_bridge = out_bridge;
- list_splice(>bridge_chain, >bridge_chain);
+ list_splice_init(>bridge_chain,
>bridge_chain);
} else {
int ret = exynos_dsi_create_connector(encoder);
diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 6c5b80ad6154..e1378d48210f 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -1613,7 +1613,7 @@ static int vc4_dsi_bind(struct device *dev, struct
device *master, void *data)
* from our driver, since we need to sequence them within the
* encoder's enable/disable paths.
*/
- list_splice(>encoder->bridge_chain, >bridge_chain);
+ list_splice_init(>encoder->bridge_chain, >bridge_chain);
if (dsi->port == 0)
vc4_debugfs_add_regset32(drm, "dsi0_regs", >regset);
@@ -1639,7 +1639,7 @@ static void vc4_dsi_unbind(struct device *dev,
struct device *master,
* Restore the bridge_chain so the bridge detach procedure can
happen
* normally.
*/
- list_splice(>bridge_chain,
Re: [PATCH v4 04/11] drm/bridge: Make the bridge chain a double-linked list
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 1440.fimd device for DMA mapping operations
exynos-drm exynos-drm: bound 1440.fimd (ops fimd_component_ops)
exynos-drm exynos-drm: bound 1445.mixer (ops mixer_component_ops)
exynos-drm exynos-drm: bound 1450.dsi (ops exynos_dsi_component_ops)
exynos-drm exynos-drm: bound 1453.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(>bridge_chain)
fixed the boot issue. 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.
I can send a formal patch fixing this if You want.
>>> --->8---
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> index 3955f84dc893..118ecedc7621 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> @@ -1523,7 +1523,7 @@ static int exynos_dsi_host_attach(struct
>>> mipi_dsi_host *host,
>>> if (out_bridge) {
>>> drm_bridge_attach(encoder, out_bridge, NULL);
>>> dsi->out_bridge = out_bridge;
>>> - list_splice(>bridge_chain, >bridge_chain);
>>> + list_splice_init(>bridge_chain,
>>> >bridge_chain);
>>> } else {
>>> int ret = exynos_dsi_create_connector(encoder);
>>>
>>> diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
>>> index 6c5b80ad6154..e1378d48210f 100644
>>> --- a/drivers/gpu/drm/vc4/vc4_dsi.c
>>> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
>>> @@ -1613,7 +1613,7 @@ static int vc4_dsi_bind(struct device *dev, struct
>>> device *master, void *data)
>>>* from our driver, since we need to sequence them within the
>>>* encoder's enable/disable paths.
>>>*/
>>> - list_splice(>encoder->bridge_chain, >bridge_chain);
>>> + list_splice_init(>encoder->bridge_chain, >bridge_chain);
>>>
>>> if (dsi->port == 0)
>>> vc4_debugfs_add_regset32(drm, "dsi0_regs", >regset);
>>> @@ -1639,7 +1639,7 @@ static void vc4_dsi_unbind(struct device *dev, struct
>>> device *master,
>>>* Restore the bridge_chain so the bridge detach procedure can
>>> happen
>>>* normally.
>>>*/
>>> - list_splice(>bridge_chain, >encoder->bridge_chain);
>>> + list_splice_init(>bridge_chain, >encoder->bridge_chain);
>>> vc4_dsi_encoder_destroy(dsi->encoder);
>>>
>>> if (dsi->port == 1)
>>>
>>>
Best regards
--
Marek Szyprowski, PhD
Samsung R Institute Poland
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 04/11] drm/bridge: Make the bridge chain a double-linked list
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 1440.fimd device for DMA mapping operations
> >> exynos-drm exynos-drm: bound 1440.fimd (ops fimd_component_ops)
> >> exynos-drm exynos-drm: bound 1445.mixer (ops mixer_component_ops)
> >> exynos-drm exynos-drm: bound 1450.dsi (ops exynos_dsi_component_ops)
> >> exynos-drm exynos-drm: bound 1453.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?
>
> > --->8---
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > index 3955f84dc893..118ecedc7621 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> > @@ -1523,7 +1523,7 @@ static int exynos_dsi_host_attach(struct
> > mipi_dsi_host *host,
> > if (out_bridge) {
> > drm_bridge_attach(encoder, out_bridge, NULL);
> > dsi->out_bridge = out_bridge;
> > - list_splice(>bridge_chain, >bridge_chain);
> > + list_splice_init(>bridge_chain,
> > >bridge_chain);
> > } else {
> > int ret = exynos_dsi_create_connector(encoder);
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> > index 6c5b80ad6154..e1378d48210f 100644
> > --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> > +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> > @@ -1613,7 +1613,7 @@ static int vc4_dsi_bind(struct device *dev, struct
> > device *master, void *data)
> > * from our driver, since we need to sequence them within the
> > * encoder's enable/disable paths.
> > */
> > - list_splice(>encoder->bridge_chain, >bridge_chain);
> > + list_splice_init(>encoder->bridge_chain, >bridge_chain);
> >
> > if (dsi->port == 0)
> > vc4_debugfs_add_regset32(drm, "dsi0_regs", >regset);
> > @@ -1639,7 +1639,7 @@ static void vc4_dsi_unbind(struct device *dev, struct
> > device *master,
> > * Restore the bridge_chain so the bridge detach procedure can
> > happen
> > * normally.
> > */
> > - list_splice(>bridge_chain, >encoder->bridge_chain);
> > + list_splice_init(>bridge_chain, >encoder->bridge_chain);
> > vc4_dsi_encoder_destroy(dsi->encoder);
> >
> > if (dsi->port == 1)
> >
> >
> Best regards
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 04/11] drm/bridge: Make the bridge chain a double-linked list
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 1440.fimd device for DMA mapping operations
>> exynos-drm exynos-drm: bound 1440.fimd (ops fimd_component_ops)
>> exynos-drm exynos-drm: bound 1445.mixer (ops mixer_component_ops)
>> exynos-drm exynos-drm: bound 1450.dsi (ops exynos_dsi_component_ops)
>> exynos-drm exynos-drm: bound 1453.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.
> --->8---
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index 3955f84dc893..118ecedc7621 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -1523,7 +1523,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host
> *host,
> if (out_bridge) {
> drm_bridge_attach(encoder, out_bridge, NULL);
> dsi->out_bridge = out_bridge;
> - list_splice(>bridge_chain, >bridge_chain);
> + list_splice_init(>bridge_chain, >bridge_chain);
> } else {
> int ret = exynos_dsi_create_connector(encoder);
>
> diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> index 6c5b80ad6154..e1378d48210f 100644
> --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> @@ -1613,7 +1613,7 @@ static int vc4_dsi_bind(struct device *dev, struct
> device *master, void *data)
> * from our driver, since we need to sequence them within the
> * encoder's enable/disable paths.
> */
> - list_splice(>encoder->bridge_chain, >bridge_chain);
> + list_splice_init(>encoder->bridge_chain, >bridge_chain);
>
> if (dsi->port == 0)
> vc4_debugfs_add_regset32(drm, "dsi0_regs", >regset);
> @@ -1639,7 +1639,7 @@ static void vc4_dsi_unbind(struct device *dev, struct
> device *master,
> * Restore the bridge_chain so the bridge detach procedure can happen
> * normally.
> */
> - list_splice(>bridge_chain, >encoder->bridge_chain);
> + list_splice_init(>bridge_chain, >encoder->bridge_chain);
> vc4_dsi_encoder_destroy(dsi->encoder);
>
> if (dsi->port == 1)
>
>
Best regards
--
Marek Szyprowski, PhD
Samsung R Institute Poland
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 04/11] drm/bridge: Make the bridge chain a double-linked list
Hello Marek,
On Mon, 16 Dec 2019 14:54:25 +0100
Marek Szyprowski wrote:
> Hi All,
>
> 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 1440.fimd device for DMA mapping operations
> exynos-drm exynos-drm: bound 1440.fimd (ops fimd_component_ops)
> exynos-drm exynos-drm: bound 1445.mixer (ops mixer_component_ops)
> exynos-drm exynos-drm: bound 1450.dsi (ops exynos_dsi_component_ops)
> exynos-drm exynos-drm: bound 1453.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?
--->8---
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 3955f84dc893..118ecedc7621 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -1523,7 +1523,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host
*host,
if (out_bridge) {
drm_bridge_attach(encoder, out_bridge, NULL);
dsi->out_bridge = out_bridge;
- list_splice(>bridge_chain, >bridge_chain);
+ list_splice_init(>bridge_chain, >bridge_chain);
} else {
int ret = exynos_dsi_create_connector(encoder);
diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 6c5b80ad6154..e1378d48210f 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -1613,7 +1613,7 @@ static int vc4_dsi_bind(struct device *dev, struct device
*master, void *data)
* from our driver, since we need to sequence them within the
* encoder's enable/disable paths.
*/
- list_splice(>encoder->bridge_chain, >bridge_chain);
+ list_splice_init(>encoder->bridge_chain, >bridge_chain);
if (dsi->port == 0)
vc4_debugfs_add_regset32(drm, "dsi0_regs", >regset);
@@ -1639,7 +1639,7 @@ static void vc4_dsi_unbind(struct device *dev, struct
device *master,
* Restore the bridge_chain so the bridge detach procedure can happen
* normally.
*/
- list_splice(>bridge_chain, >encoder->bridge_chain);
+ list_splice_init(>bridge_chain, >encoder->bridge_chain);
vc4_dsi_encoder_destroy(dsi->encoder);
if (dsi->port == 1)
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 04/11] drm/bridge: Make the bridge chain a double-linked list
Hi All,
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 1440.fimd device for DMA mapping operations
exynos-drm exynos-drm: bound 1440.fimd (ops fimd_component_ops)
exynos-drm exynos-drm: bound 1445.mixer (ops mixer_component_ops)
exynos-drm exynos-drm: bound 1450.dsi (ops exynos_dsi_component_ops)
exynos-drm exynos-drm: bound 1453.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.
> ---
> Changes in v4:
> * Simplify the drm_bridge_attach() logic
> * Fix list iteration bugs
> * Patch VC4 and Exynos DSI drivers to match core changes
> * Add R-bs
>
> Changes in v3:
> * None
>
> Changes in v2:
> * Adjust things to the "dummy encoder bridge" change (patch 2 in this
>series)
> ---
> drivers/gpu/drm/drm_bridge.c| 171 +++-
> drivers/gpu/drm/drm_encoder.c | 16 +--
> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 5 +-
> drivers/gpu/drm/vc4/vc4_dsi.c | 10 +-
> include/drm/drm_bridge.h| 12 +-
> include/drm/drm_encoder.h | 7 +-
> 6 files changed, 143 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 54c874493c57..b6517b4fa3d1 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -55,7 +55,7 @@
>* just provide additional hooks to get the desired output at the end of the
>* encoder chain.
>*
> - * Bridges can also be chained up using the _bridge.next pointer.
> + * Bridges can also be chained up using the _bridge.chain_node field.
>*
>* Both legacy CRTC helpers and the new atomic modeset helpers support
> bridges.
>*/
> @@ -128,20 +128,21 @@ int drm_bridge_attach(struct drm_encoder *encoder,
> struct drm_bridge *bridge,
> bridge->dev = encoder->dev;
> bridge->encoder = encoder;
>
> + if (previous)
> + list_add(>chain_node, >chain_node);
> + else
> + list_add(>chain_node, >bridge_chain);
> +
> if (bridge->funcs->attach) {
> ret = bridge->funcs->attach(bridge);
> if (ret < 0) {
> + list_del(>chain_node);
> bridge->dev = NULL;
> bridge->encoder = NULL;
> return ret;
> }
> }
>
> - if (previous)
> - previous->next = bridge;
> - else
> - encoder->bridge = bridge;
> -
> return 0;
> }
> EXPORT_SYMBOL(drm_bridge_attach);
> @@ -157,6 +158,7 @@ void drm_bridge_detach(struct drm_bridge *bridge)
> if (bridge->funcs->detach)
> bridge->funcs->detach(bridge);
>
> + list_del(>chain_node);
> bridge->dev = NULL;
> }
>
> @@ -190,18 +192,21 @@ bool drm_bridge_chain_mode_fixup(struct drm_bridge
> *bridge,
>const struct drm_display_mode *mode,
>struct drm_display_mode *adjusted_mode)
> {
> - bool ret = true;
> + struct drm_encoder *encoder;
>
> if (!bridge)
> return true;
>
> - if (bridge->funcs->mode_fixup)
> - ret = bridge->funcs->mode_fixup(bridge, mode, adjusted_mode);
> + encoder = bridge->encoder;
> + list_for_each_entry_from(bridge, >bridge_chain, chain_node) {
> + if (!bridge->funcs->mode_fixup)
> + continue;
>
> - ret = ret && drm_bridge_chain_mode_fixup(bridge->next, mode,
> - adjusted_mode);
> + if (!bridge->funcs->mode_fixup(bridge, mode, adjusted_mode))
> + return false;
> + }
>
> - return ret;
> + return true;
> }
> EXPORT_SYMBOL(drm_bridge_chain_mode_fixup);
>
> @@ -224,18 +229,24 @@ enum drm_mode_status
> drm_bridge_chain_mode_valid(struct drm_bridge *bridge,
> const struct drm_display_mode *mode)
> {
> - enum drm_mode_status ret = MODE_OK;
> + struct drm_encoder *encoder;
>
> if (!bridge)
> - return ret;
> + return MODE_OK;
>
> - if (bridge->funcs->mode_valid)
> - ret =
[PATCH v4 04/11] drm/bridge: Make the bridge chain a double-linked list
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
---
Changes in v4:
* Simplify the drm_bridge_attach() logic
* Fix list iteration bugs
* Patch VC4 and Exynos DSI drivers to match core changes
* Add R-bs
Changes in v3:
* None
Changes in v2:
* Adjust things to the "dummy encoder bridge" change (patch 2 in this
series)
---
drivers/gpu/drm/drm_bridge.c| 171 +++-
drivers/gpu/drm/drm_encoder.c | 16 +--
drivers/gpu/drm/exynos/exynos_drm_dsi.c | 5 +-
drivers/gpu/drm/vc4/vc4_dsi.c | 10 +-
include/drm/drm_bridge.h| 12 +-
include/drm/drm_encoder.h | 7 +-
6 files changed, 143 insertions(+), 78 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 54c874493c57..b6517b4fa3d1 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -55,7 +55,7 @@
* just provide additional hooks to get the desired output at the end of the
* encoder chain.
*
- * Bridges can also be chained up using the _bridge.next pointer.
+ * Bridges can also be chained up using the _bridge.chain_node field.
*
* Both legacy CRTC helpers and the new atomic modeset helpers support bridges.
*/
@@ -128,20 +128,21 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct
drm_bridge *bridge,
bridge->dev = encoder->dev;
bridge->encoder = encoder;
+ if (previous)
+ list_add(>chain_node, >chain_node);
+ else
+ list_add(>chain_node, >bridge_chain);
+
if (bridge->funcs->attach) {
ret = bridge->funcs->attach(bridge);
if (ret < 0) {
+ list_del(>chain_node);
bridge->dev = NULL;
bridge->encoder = NULL;
return ret;
}
}
- if (previous)
- previous->next = bridge;
- else
- encoder->bridge = bridge;
-
return 0;
}
EXPORT_SYMBOL(drm_bridge_attach);
@@ -157,6 +158,7 @@ void drm_bridge_detach(struct drm_bridge *bridge)
if (bridge->funcs->detach)
bridge->funcs->detach(bridge);
+ list_del(>chain_node);
bridge->dev = NULL;
}
@@ -190,18 +192,21 @@ bool drm_bridge_chain_mode_fixup(struct drm_bridge
*bridge,
const struct drm_display_mode *mode,
struct drm_display_mode *adjusted_mode)
{
- bool ret = true;
+ struct drm_encoder *encoder;
if (!bridge)
return true;
- if (bridge->funcs->mode_fixup)
- ret = bridge->funcs->mode_fixup(bridge, mode, adjusted_mode);
+ encoder = bridge->encoder;
+ list_for_each_entry_from(bridge, >bridge_chain, chain_node) {
+ if (!bridge->funcs->mode_fixup)
+ continue;
- ret = ret && drm_bridge_chain_mode_fixup(bridge->next, mode,
-adjusted_mode);
+ if (!bridge->funcs->mode_fixup(bridge, mode, adjusted_mode))
+ return false;
+ }
- return ret;
+ return true;
}
EXPORT_SYMBOL(drm_bridge_chain_mode_fixup);
@@ -224,18 +229,24 @@ enum drm_mode_status
drm_bridge_chain_mode_valid(struct drm_bridge *bridge,
const struct drm_display_mode *mode)
{
- enum drm_mode_status ret = MODE_OK;
+ struct drm_encoder *encoder;
if (!bridge)
- return ret;
+ return MODE_OK;
- if (bridge->funcs->mode_valid)
- ret = bridge->funcs->mode_valid(bridge, mode);
+ encoder = bridge->encoder;
+ list_for_each_entry_from(bridge, >bridge_chain, chain_node) {
+ enum drm_mode_status ret;
+
+ if (!bridge->funcs->mode_valid)
+ continue;
- if (ret != MODE_OK)
- return ret;
+ ret = bridge->funcs->mode_valid(bridge, mode);
+ if (ret != MODE_OK)
+ return ret;
+ }
- return drm_bridge_chain_mode_valid(bridge->next, mode);
+ return MODE_OK;
}
EXPORT_SYMBOL(drm_bridge_chain_mode_valid);
@@ -251,13 +262,20 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_valid);
*/
void drm_bridge_chain_disable(struct drm_bridge *bridge)
{
+ struct drm_encoder *encoder;
+ struct drm_bridge *iter;
+
if (!bridge)
return;
- drm_bridge_chain_disable(bridge->next);
+ encoder = bridge->encoder;
+ list_for_each_entry_reverse(iter, >bridge_chain, chain_node) {
+ if (iter->funcs->disable)
+ iter->funcs->disable(iter);
- if
