Re: [PATCH v7 09/11] drm/mediatek: dp: Add support for embedded DisplayPort aux-bus

[Michael Walle <mwalle@kernel.org>]

Hi Nicolas,

>> > For the eDP case we can support using aux-bus on MediaTek DP: this
>> > gives us the possibility to declare our panel as generic "panel-edp"
>> > which will automatically configure the timings and available modes
>> > via the EDID that we read from it.
>> >
>> > To do this, move the panel parsing at the end of the probe function
>> > so that the hardware is initialized beforehand and also initialize
>> > the DPTX AUX block and power both on as, when we populate the
>> > aux-bus, the panel driver will trigger an EDID read to perform
>> > panel detection.
>> >
>> > Last but not least, since now the AUX transfers can happen in the
>> > separated aux-bus, it was necessary to add an exclusion for the
>> > cable_plugged_in check in `mtk_dp_aux_transfer()` and the easiest
>> > way to do this is to simply ignore checking that when the bridge
>> > type is eDP.
>> 
>> This patch breaks my board based on the MT8195 which only has one
>> DisplayPort output port. I suspect it might also break the 
>> mt8195-cherry
>> board.
> 
> Do you mean that your board does not have an internal display, only the 
> one
> output port? If so, why are you enabling the nodes for the internal 
> display path
> in your board specific DT?

Well, that depends. The board actually has an eDP socket, but because we
are an OEM, there might be no display connected to it. (And I haven't
tried it yet). It should probably go into an own device tree or overlay
if it is used. I agree. But it looked like it was auto-detectable (it
even has a HDP pin on the eDP socket, not sure about its use case..)

But the real reason I've enabled it was because I'll get an kernel
oops otherwise. I thought it might be some quirk that you'll need both,
because eDP will register even if theres no display - as you've
mentioned below.

Here's the splat:
[    3.237064] mediatek-drm mediatek-drm.10.auto: bound 
1c110000.vpp-merge (ops mtk_disp_merge_component_ops)
[    3.238274] mediatek-drm mediatek-drm.8.auto: Not creating crtc 0 
because component 8 is disabled or missing
[    3.239504] mediatek-drm mediatek-drm.8.auto: Not creating crtc 0 
because component 9 is disabled or missing
[    3.240741] Unable to handle kernel NULL pointer dereference at 
virtual address 00000000000004a0
[    3.241841] Mem abort info:
[    3.242192]   ESR = 0x0000000096000004
[    3.242662]   EC = 0x25: DABT (current EL), IL = 32 bits
[    3.243328]   SET = 0, FnV = 0
[    3.243710]   EA = 0, S1PTW = 0
[    3.244104]   FSC = 0x04: level 0 translation fault
[    3.244717] Data abort info:
[    3.245078]   ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
[    3.245765]   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
[    3.246398]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[    3.247063] [00000000000004a0] user address but active_mm is swapper
[    3.247860] Internal error: Oops: 0000000096000004 [#1] SMP
[    3.248559] Modules linked in:
[    3.248945] CPU: 4 PID: 11 Comm: kworker/u16:0 Not tainted 
6.5.0-rc7-next-20230821+ #2225
[    3.249970] Hardware name: Kontron 3.5"-SBC-i1200 (DT)
[    3.250614] Workqueue: events_unbound deferred_probe_work_func
[    3.251347] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS 
BTYPE=--)
[    3.252220] pc : mtk_drm_crtc_dma_dev_get+0x8/0x18
[    3.252824] lr : mtk_drm_bind+0x458/0x558
[    3.253326] sp : ffff800082b23a20
[    3.253741] x29: ffff800082b23a20 x28: ffff000002c78880 x27: 
ffff8000816466d0
[    3.254635] x26: ffff000002c6f010 x25: 0000000000000003 x24: 
0000000000000000
[    3.255529] x23: ffff000002c78880 x22: 0000000000000002 x21: 
0000000000000000
[    3.256423] x20: ffff000006516800 x19: ffff000002c78880 x18: 
ffffffffffffffff
[    3.257317] x17: 6f63206573756163 x16: 6562203020637472 x15: 
6320676e69746165
[    3.258211] x14: 726320746f4e203a x13: 676e697373696d20 x12: 
726f2064656c6261
[    3.259106] x11: 7369642073692039 x10: ffff80008275c0c0 x9 : 
ffff80008091ebf8
[    3.260000] x8 : 00000000ffffefff x7 : ffff80008275c0c0 x6 : 
80000000fffff000
[    3.260895] x5 : 000000000000bff4 x4 : 0000000000000000 x3 : 
ffff000006516ae0
[    3.261789] x2 : ffff000006516ae0 x1 : 0000000000000000 x0 : 
0000000000000000
[    3.262684] Call trace:
[    3.262991]  mtk_drm_crtc_dma_dev_get+0x8/0x18
[    3.263549]  try_to_bring_up_aggregate_device+0x16c/0x1e0
[    3.264227]  __component_add+0xac/0x180
[    3.264708]  component_add+0x1c/0x30
[    3.265158]  mtk_disp_rdma_probe+0x17c/0x270
[    3.265695]  platform_probe+0x70/0xd0
[    3.266155]  really_probe+0x150/0x2c0
[    3.266615]  __driver_probe_device+0x80/0x140
[    3.267162]  driver_probe_device+0x44/0x170
[    3.267687]  __device_attach_driver+0xc0/0x148
[    3.268245]  bus_for_each_drv+0x88/0xf0
[    3.268727]  __device_attach+0xa4/0x198
[    3.269208]  device_initial_probe+0x1c/0x30
[    3.269732]  bus_probe_device+0xb4/0xc0
[    3.270214]  deferred_probe_work_func+0x90/0xd0
[    3.270783]  process_one_work+0x144/0x3a0
[    3.271289]  worker_thread+0x2ac/0x4b8
[    3.271761]  kthread+0xec/0xf8
[    3.272145]  ret_from_fork+0x10/0x20
[    3.272597] Code: 814f7858 ffff8000 aa1e03e9 d503201f (f9425000)
[    3.273361] ---[ end trace 0000000000000000 ]---

Here's the complete bootlog:
https://pastebin.com/raw/SekMYBj4

Any help is where to look is appreciated.

>> While the mediatek-dpi driver finds the DP port:
>> [    3.131645] mediatek-dpi 1c113000.dp-intf: Found bridge node: 
>> /soc/dp-tx@1c600000
>> 
>> The probing of the eDP is deferred:
>> [   13.289009] platform 1c015000.dp-intf: deferred probe pending
>> 
>> So I don't know why, but to make dp_intf1 work, it seems that dp_intf0
>> must be probed successfully. After this patch, the edp (which is
>> connected to the dp_intf1) probe will return with an -ENODEV and
>> the previous call to devm_drm_bridge_add() will be rolled back.
> 
> The MediaTek DRM driver uses the component framework, so it waits for 
> all of its
> components to register until it binds them all (which includes both 
> intf0 and
> intf1, unless they're disabled on the DT).

Oh with "I don't know why, but to make dp_intf1 work.." I meant the 
splat
above. I figured that dpi won't probe if a dependency is still deferred 
;)

-michael

> It's true that before this patch no panel being found for edp-tx 
> wouldn't
> prevent it to probe, but it really should.
> 
> Thanks,
> Nícolas
> 
>> 
>> Before this patch, bridge_add() was called in any case (in the
>> error case with next_bridge = NULL) and the mediatek-dpi probed
>> like that:
>> 
>> [    3.121011] mediatek-dpi 1c015000.dp-intf: Found bridge node: 
>> /soc/edp-tx@1c500000
>> [    3.122111] mediatek-dpi 1c113000.dp-intf: Found bridge node: 
>> /soc/dp-tx@1c600000
>> 
>> Eventually resulting in a framebuffer device:
>> [    4.451081] mediatek-drm mediatek-drm.8.auto: [drm] fb0: 
>> mediatekdrmfb frame buffer device
>> 
>> 
>> NB, somehow this series broke the initial display output. I always 
>> have
>> to replug the DisplayPort to get some output. I'll dig deeper into 
>> that
>> later.
>> 
>> ..
>> 
>> > @@ -2519,21 +2553,14 @@ static int mtk_dp_probe(struct platform_device *pdev)
>> >  		return dev_err_probe(dev, mtk_dp->irq,
>> >  				     "failed to request dp irq resource\n");
>> >
>> > -	mtk_dp->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
>> > -	if (IS_ERR(mtk_dp->next_bridge) &&
>> > -	    PTR_ERR(mtk_dp->next_bridge) == -ENODEV)
>> > -		mtk_dp->next_bridge = NULL;
>> 
>> In my case, this branch was taken.
>> 
>> -michael
>> 
>> > -	else if (IS_ERR(mtk_dp->next_bridge))
>> > -		return dev_err_probe(dev, PTR_ERR(mtk_dp->next_bridge),
>> > -				     "Failed to get bridge\n");
>> > -
>> >  	ret = mtk_dp_dt_parse(mtk_dp, pdev);
>> >  	if (ret)
>> >  		return dev_err_probe(dev, ret, "Failed to parse dt\n");
>> >
>> > -	drm_dp_aux_init(&mtk_dp->aux);
>> >  	mtk_dp->aux.name = "aux_mtk_dp";
>> > +	mtk_dp->aux.dev = dev;
>> >  	mtk_dp->aux.transfer = mtk_dp_aux_transfer;
>> > +	drm_dp_aux_init(&mtk_dp->aux);
>> >
>> >  	spin_lock_init(&mtk_dp->irq_thread_lock);
>> >