[PATCH v2 10/16] drm/exynos: implement a drm bridge

Mon Sep 14 21:19:01 UTC 2020

Hi Marek, Michael,

On 14.09.2020 22:01, Michael Tretter wrote:
> Hi,
>
> On Mon, 14 Sep 2020 14:31:19 +0200, Marek Szyprowski wrote:
>> On 14.09.2020 10:29, Marek Szyprowski wrote:
>>> On 11.09.2020 15:54, Michael Tretter wrote:
>>>> Make the exynos_dsi driver a full drm bridge that can be found and used
>>>> from other drivers.
>>>>
>>>> Other drivers can only attach to the bridge, if a mipi dsi device
>>>> already attached to the bridge. This allows to defer the probe of the
>>>> display pipe until the downstream bridges are available, too.
>>>>
>>>> Signed-off-by: Michael Tretter <m.tretter at pengutronix.de>
>>> This one (and the whole series applied) still fails on Exynos boards:
>>>
>>> [drm] Exynos DRM: using 11c00000.fimd device for DMA mapping operations
>>> exynos-drm exynos-drm: bound 11c00000.fimd (ops fimd_component_ops)
>>> OF: graph: no port node found in /soc/dsi at 11c80000
>>> 8<--- cut here ---
>>> Unable to handle kernel NULL pointer dereference at virtual address 00000084
>>> pgd = (ptrval)
>>> [00000084] *pgd=00000000
>>> Internal error: Oops: 5 [#1] PREEMPT SMP ARM
>>> Modules linked in:
>>> CPU: 1 PID: 1 Comm: swapper/0 Not tainted
>>> 5.9.0-rc4-next-20200911-00010-g417dc70d70ec #1608
>>> Hardware name: Samsung Exynos (Flattened Device Tree)
>>> PC is at drm_bridge_attach+0x18/0x164
>>> LR is at exynos_dsi_bind+0x88/0xa8
>>> pc : [<c0628c08>]    lr : [<c064d560>]    psr: 20000013
>>> sp : ef0dfca8  ip : 00000002  fp : c13190e0
>>> r10: 00000000  r9 : ee46d580  r8 : c13190e0
>>> r7 : ee438800  r6 : 00000018  r5 : ef253810  r4 : ef39e840
>>> r3 : 00000000  r2 : 00000018  r1 : ef39e888  r0 : ef39e840
>>> Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
>>> Control: 10c5387d  Table: 4000404a  DAC: 00000051
>>> Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
>>> Stack: (0xef0dfca8 to 0xef0e0000)
>>> ...
>>> [<c0628c08>] (drm_bridge_attach) from [<c064d560>]
>>> (exynos_dsi_bind+0x88/0xa8)
>>> [<c064d560>] (exynos_dsi_bind) from [<c066a800>]
>>> (component_bind_all+0xfc/0x290)
>>> [<c066a800>] (component_bind_all) from [<c0649dc0>]
>>> (exynos_drm_bind+0xe4/0x19c)
>>> [<c0649dc0>] (exynos_drm_bind) from [<c066ad74>]
>>> (try_to_bring_up_master+0x1e4/0x2c4)
>>> [<c066ad74>] (try_to_bring_up_master) from [<c066b2b4>]
>>> (component_master_add_with_match+0xd4/0x108)
>>> [<c066b2b4>] (component_master_add_with_match) from [<c0649ae8>]
>>> (exynos_drm_platform_probe+0xe4/0x110)
>>> [<c0649ae8>] (exynos_drm_platform_probe) from [<c0674e6c>]
>>> (platform_drv_probe+0x6c/0xa4)
>>> [<c0674e6c>] (platform_drv_probe) from [<c067242c>]
>>> (really_probe+0x200/0x4fc)
>>> [<c067242c>] (really_probe) from [<c06728f0>]
>>> (driver_probe_device+0x78/0x1fc)
>>> [<c06728f0>] (driver_probe_device) from [<c0672cd8>]
>>> (device_driver_attach+0x58/0x60)
>>> [<c0672cd8>] (device_driver_attach) from [<c0672dbc>]
>>> (__driver_attach+0xdc/0x174)
>>> [<c0672dbc>] (__driver_attach) from [<c06701b4>]
>>> (bus_for_each_dev+0x68/0xb4)
>>> [<c06701b4>] (bus_for_each_dev) from [<c06714e8>]
>>> (bus_add_driver+0x158/0x214)
>>> [<c06714e8>] (bus_add_driver) from [<c0673c1c>] (driver_register+0x78/0x110)
>>> [<c0673c1c>] (driver_register) from [<c0649ca8>]
>>> (exynos_drm_init+0xe4/0x118)
>>> [<c0649ca8>] (exynos_drm_init) from [<c0102484>]
>>> (do_one_initcall+0x8c/0x42c)
>>> [<c0102484>] (do_one_initcall) from [<c11011c0>]
>>> (kernel_init_freeable+0x190/0x1dc)
>>> [<c11011c0>] (kernel_init_freeable) from [<c0af7880>]
>>> (kernel_init+0x8/0x118)
>>> [<c0af7880>] (kernel_init) from [<c0100114>] (ret_from_fork+0x14/0x20)
>>> Exception stack(0xef0dffb0 to 0xef0dfff8)
>>> ...
>>> ---[ end trace ee27f313f9ed9da1 ]---
>>>
>>> # arm-linux-gnueabi-addr2line -e vmlinux c0628c08
>>> drivers/gpu/drm/drm_bridge.c:184 (discriminator 1)
>>>
>>> I will try to debug it a bit more today.
>> The above crash has been caused by lack of in_bridge initialization to
>> NULL in exynos_dsi_bind() in this patch. However, fixing it reveals
>> another issue:
>>
>> [drm] Exynos DRM: using 11c00000.fimd device for DMA mapping operations
>> exynos-drm exynos-drm: bound 11c00000.fimd (ops fimd_component_ops)
>> OF: graph: no port node found in /soc/dsi at 11c80000
>> 8<--- cut here ---
>> Unable to handle kernel NULL pointer dereference at virtual address 00000280
>> pgd = (ptrval)
>> [00000280] *pgd=00000000
>> Internal error: Oops: 5 [#1] PREEMPT SMP ARM
>> Modules linked in:
>> CPU: 0 PID: 1 Comm: swapper/0 Not tainted
>> 5.9.0-rc4-next-20200911-00010-g417dc70d70ec-dirty #1613
>> Hardware name: Samsung Exynos (Flattened Device Tree)
>> PC is at __mutex_lock+0x54/0xb18
>> LR is at lock_is_held_type+0x80/0x138
>> pc : [<c0afc920>]    lr : [<c0af63e8>]    psr: 60000013
>> sp : ef0dfd30  ip : 33937b74  fp : c13193c8
>> r10: c1208eec  r9 : 00000000  r8 : ee45f808
>> r7 : c19561a4  r6 : 00000000  r5 : 00000000  r4 : 0000024c
>> r3 : 00000000  r2 : 00204140  r1 : c124f13c  r0 : 00000000
>> Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
>> Control: 10c5387d  Table: 4000404a  DAC: 00000051
>> Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
>> Stack: (0xef0dfd30 to 0xef0e0000)
>> ...
>> [<c0afc920>] (__mutex_lock) from [<c0afd400>] (mutex_lock_nested+0x1c/0x24)
>> [<c0afd400>] (mutex_lock_nested) from [<c064d4b8>]
>> (__exynos_dsi_host_attach+0x20/0x6c)
>> [<c064d4b8>] (__exynos_dsi_host_attach) from [<c064d914>]
>> (exynos_dsi_host_attach+0x70/0x194)
>> [<c064d914>] (exynos_dsi_host_attach) from [<c0656b64>]
>> (s6e8aa0_probe+0x1b0/0x218)
>> [<c0656b64>] (s6e8aa0_probe) from [<c0672530>] (really_probe+0x200/0x4fc)
>> [<c0672530>] (really_probe) from [<c06729f4>]
>> (driver_probe_device+0x78/0x1fc)
>> [<c06729f4>] (driver_probe_device) from [<c0672ddc>]
>> (device_driver_attach+0x58/0x60)
>> [<c0672ddc>] (device_driver_attach) from [<c0672ec0>]
>> (__driver_attach+0xdc/0x174)
>> [<c0672ec0>] (__driver_attach) from [<c06702b8>]
>> (bus_for_each_dev+0x68/0xb4)
>> [<c06702b8>] (bus_for_each_dev) from [<c06715ec>]
>> (bus_add_driver+0x158/0x214)
>> [<c06715ec>] (bus_add_driver) from [<c0673d20>] (driver_register+0x78/0x110)
>> [<c0673d20>] (driver_register) from [<c0102484>]
>> (do_one_initcall+0x8c/0x42c)
>> [<c0102484>] (do_one_initcall) from [<c11011c0>]
>> (kernel_init_freeable+0x190/0x1dc)
>> [<c11011c0>] (kernel_init_freeable) from [<c0af7988>]
>> (kernel_init+0x8/0x118)
>> [<c0af7988>] (kernel_init) from [<c0100114>] (ret_from_fork+0x14/0x20)
>> Exception stack(0xef0dffb0 to 0xef0dfff8)
>> ...
>> ---[ end trace c06e996ec2e8234d ]---
>>
>> This means that dsi->encoder.dev is not initialized in
>> __exynos_dsi_host_attach().
>>
>> This happens, because drm_bridge_attach() in exynos_dsi_bind() returned
>> earlier -517 (deferred probe), what causes cleanup of encoder and
>> release of all drm resources.
>>
>> Then however, the panel tries to register itself and
>> exynos_dsi_host_attach() tries to access the released encoder (which is
>> zeroed in drm_encoder_release) and rest of resources, what causes failure.
>>
>> It looks that something is missing. Maybe mipi host has to be registered
>> later, when bridge is ready? I have no idea how it is handled before
>> this patch. Andrzej, could you comment it a bit?
> I intentionally changed the order, because if another bridge follows in the
> pipeline, the probe of the drm driver has to be deferred until some bridge
> provides a connector. The next bridge registers itself via the host_attach
> function and the deferral is ensured via the bind for the bind/unbind API or
> the bridge_attach function otherwise.
>
> On the other hand, the bridge does not have an encoder until the mipi device
> has been attached.
>
> As a solution, the exynos dsi driver must initialize the encoder in
> exynos_dsi_probe instead of in exynos_dsi_bind and access the encoder via
> exynos_dsi instead of the bridge.
>
> Can you try to move everything except samsung_dsim_bind from exynos_dsi_bind
> to exynos_dsi_probe (respectively for unbind) and report if it fixes the
> crash.

The original behaviour is that encoder (exynos_dsi) is registered 
regardless of sink presence (initially panel, later also bridge) - it 
avoids multiple issues with deferred probe, device driver bind/unbind 
and module load/unload. Appearance or disappearance of sink is reported 
to host nicely via DSI attach/detach callbacks - and it is reflected in 
drm world as change state of the connector.

Registering DSI host in bind and unregistering in unbind assures that if 
mipi_dsi device is attached/detached the drm device is always present - 
it makes device/driver binding race free and allows to avoid additional 
locking.

Moving DSI host registration to probe changes everything, for sure it 
breaks the nice feature of DSI attach/detach callbacks and apparently 
can cause different issues depending on device bind order.

I will try to look at the patches tomorrow and maybe I can find more 
constructive comments :)

Regards

Andrzej

>
> Michael
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://protect2.fireeye.com/v1/url?k=4f0be936-129547ac-4f0a6279-0cc47a336fae-e9aecfc5418740e8&q=1&e=1d4b0871-5b85-47f3-9506-79c768643aee&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel