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

Thu Feb 4 16:05:09 UTC 2021

On Thu, Feb 04, 2021 at 11:56:32AM +0100, Michael Tretter wrote:
> On Thu, 04 Feb 2021 11:17:49 +0100, Daniel Vetter wrote:
> > On Wed, Feb 3, 2021 at 9:32 PM Michael Tretter <m.tretter at pengutronix.de> wrote:
> > >
> > > On Mon, 01 Feb 2021 17:33:14 +0100, Michael Tretter wrote:
> > > > On Tue, 15 Sep 2020 21:40:40 +0200, Andrzej Hajda wrote:
> > > > > W dniu 14.09.2020 o 23:19, Andrzej Hajda pisze:
> > > > > > On 14.09.2020 22:01, Michael Tretter wrote:
> > > > > >> 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 :)
> > > > >
> > > > >
> > > > > As I said yesterday, exynos_dsi driver uses dsi host attach/detach
> > > > > callbacks to control appearance/disappearance of downstream device. It
> > > > > allows to:
> > > > >
> > > > > 1. Safely bind/unbind different device drivers at any time and at any
> > > > > order, without killing exynos_drm and/or crashing system.
> > > > >
> > > > > 2. Avoid issues with late drm init - on some platforms exynos_drm device
> > > > > appeared too late, due to deferred probe, and resulted in black screen
> > > > > in userspace.
> > > > >
> > > > >
> > > > > Now if we want to convert exynos_dsi to drm_bridge I see following options:
> > > > >
> > > > > A. Forgot about callbacks and make the exynos_drm to defer probing until
> > > > > exynos_dsi bridge is available, probably it will cause later exynos_drm
> > > > > appearance, thus probably black screen on some targets. So for sure it
> > > > > will be suboptimal. Making it bridge unbind safe would be another
> > > > > problem, but most developers do not care about it so why should we? :)
> > > > >
> > > > > B. Try to mimic current behaviour - exynos_dsi register bridge ASAP,
> > > > > even if downstream devices are not yet attached, on attach/detach notify
> > > > > drm about it via connector status change, for this dsi_host registration
> > > > > should be performed from drm_bridge attach, I guess.
> > > > >
> > > > >
> > > > > Option A is more standard, but is unsafe and causes other issues.
> > > > >
> > > > > Option B keeps current behaviour.
> > > >
> > > > Maybe we can have both, but I am not sure, if I am missing something:
> > > >
> > > > I still prefer option A for the samsung-dsim driver, because it is more
> > > > standard, simpler and avoids issues with encoders, connectors or handling
> > > > hotplug.
> > > >
> > > > The idea is to use two bridges in the exynos-dsi driver: One bridge in the
> > > > samsung-dsim driver which implements option A and defers probing of the drm
> > > > driver until the next bridge is attached. And a second bridge in the
> > > > exynos_dsi that attaches to the first bridge (thus, allowing the exynos_drm
> > > > device to appear) and implements the hotplug handling for notifying drm via
> > > > connector status change.
> > > >
> > > > The driver for the i.MX8M would use the samsung-dsim bridge without an
> > > > additional bridge.
> > > >
> > > > This allows the samsung-dsim driver to expose the standard behavior while the
> > > > exynos_dsi may stick to the existing behavior for the exynos_drm driver.
> > > >
> > > > I hope this makes sense and does not sound too crazy. It might be difficult to
> > > > get the probing and mipi host/device registration correct, but I will try, if
> > > > this can work.
> > >
> > > Adding two bridges for being able to support hotplugging adds many special
> > > cases to the bridge driver and still requires more custom API to correctly add
> > > the second bridge. I don't think that this a viable path to go.
> > 
> > Just jumping in here: You cannot hotplug/hotremove anything from a
> > drm_device after drm_dev_register has been called, except
> > drm_connector. I didn't dig into details here so not sure whether you
> > want to late-bind your bridge after drm_dev_register is called or not,
> > so might just be fyi and not relevant to the discussion.
> 
> Thanks. AFAIC that is exactly what is currently implemented in the exynos_drm
> driver (i.e. Option B)
> 
> exynos_dsi_bind configures the encoder and registers a DSI host. Afterwards,
> exynos_drm_bind (as component_master_ops) calls drm_dev_register. Later, a DSI
> device might attach to the DSI host and call exynos_dsi_host_attach. In
> exynos_dsi_host_attach, the driver finds the drm_bridge for the DSI device and
> attaches this bridge to the encoder _after_ drm_dev_register has been called.
> This is invalid behavior, right?

Definitely not supported, I don't think we have the right locks in place
to make sure this works.

Now if your _only_ adding a drm_bridge (and not an encoder or anything
like that), and you are adding the drm_connector correctly (like a
hotplugged DP MST sink), then that would at least work from a uapi pov.
Because drm_bridge isn't exposed as an uapi object.

But yeah, as-is, don't :-)

The solution here is a bunch of EPROBE_DEFER handling until all your
bridges are loaded, with or without the assistance of component.c
framework. Only then call drm_dev_register.
-Daniel

> 
> Michael
> 
> > -Daniel
> > 
> > >
> > > This leaves us with:
> > >
> > > Option A) Standard drm_bridge behavior, which is currently implemented, but
> > > incompatible with the currently expected behavior of exynos_drm.
> > >
> > > Option B) Creating the drm device without all bridges being attached, which
> > > would work with the exynos_drm driver, but breaks for the standard drm_bridge
> > > behavior, especially, if the encoder/connector is created at the beginning of
> > > the pipeline and passed downwards when the bridges are attached.
> > >
> > > Option C) Extracting only low level register accesses into shared code, adding
> > > a custom interface and implementing the drm_bridge handling in the platform
> > > specific code.
> > >
> > > None of the options really convinces me.
> > >
> > > Michael
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch