Hi Laurent,
On 19.04.2022 19:37, Laurent Pinchart wrote:
> On Tue, Apr 19, 2022 at 06:16:07PM +0200, Robert Foss wrote:
>> On Tue, 19 Apr 2022 at 11:41, Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx> wrote:
>>> On Tue, Apr 19, 2022 at 2:44 PM Marek Szyprowski
>>> <m.szyprowski@xxxxxxxxxxx> wrote:
>>>> If panel_bridge_attach() happens after DRM device registration, the
>>>> created connector will not be registered by the DRM core anymore. Fix
>>>> this by registering it explicitely in such case.
>>>>
>>>> This fixes the following issue observed on Samsung Exynos4210-based Trats
>>>> board with a DSI panel (the panel driver is registed after the Exynos DRM
>>>> component device is bound):
>>>>
>>>> $ ./modetest -c -Mexynos
>>>> could not get connector 56: No such file or directory
>>>> Segmentation fault
>>>>
>>>> While touching this, move the connector reset() call also under the DRM
>>>> device registered check, because otherwise it is not really needed.
>>>>
>>>> Fixes: 934aef885f9d ("drm: bridge: panel: Reset the connector state pointer")
>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
>>>> ---
>>>> Here is a bit more backgroud on this issue is available in this thread:
>>>> https://lore.kernel.org/all/f0474a95-4ba3-a74f-d7de-ef7aab12bc86@xxxxxxxxxxx/
>>>> ---
>>>> drivers/gpu/drm/bridge/panel.c | 7 +++++--
>>>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
>>>> index ff1c37b2e6e5..0ee563eb2b6f 100644
>>>> --- a/drivers/gpu/drm/bridge/panel.c
>>>> +++ b/drivers/gpu/drm/bridge/panel.c
>>>> @@ -83,8 +83,11 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
>>>> drm_connector_attach_encoder(&panel_bridge->connector,
>>>> bridge->encoder);
>>>>
>>>> - if (connector->funcs->reset)
>>>> - connector->funcs->reset(connector);
>>>> + if (bridge->dev->registered) {
>>>> + if (connector->funcs->reset)
>>>> + connector->funcs->reset(connector);
>>>> + drm_connector_register(connector);
>>>> + }
>>> Reviewed-by: Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx>
>> Fixed typos in commit message.
>>
>> Reviewed-by: Robert Foss <robert.foss@xxxxxxxxxx>
>>
>> Applied to drm-misc-next
> Doesn't this open the door to various race conditions ?
This only patch restores the old behavior of the Exynos DSI driver. IIRC
Andrzej already checked the possible races and said that the late
connector registration is fine.
> Also, what happens if the panel bridge is detached and reattached ? If I
> recall correctly, registering new connectors at runtime is at least
> partly supported for DP MST, but I'm not sure about unregistration.
Well, indeed the panel unregistration is broken now:
# rmmod panel_samsung_s6e8aa0
------------[ cut here ]------------
WARNING: CPU: 1 PID: 1336 at drivers/gpu/drm/drm_connector.c:462
drm_connector_cleanup+0x26c/0x288
Modules linked in: cmac bnep hci_uart btbcm btintel bluetooth s5p_csis
s5p_fimc exynos4_is_common ecdh_generic ecc v4l2_fwnode v4l2_async
s5p_mfc brcmfmac cfg80211 brcmutil panel_samsung_s6e8aa0(-) s5p_jpeg
videobuf2_dma_contig videobuf2_memops v4l2_mem2mem videobuf2_v4l2
videobuf2_common videodev mc
CPU: 1 PID: 1336 Comm: rmmod Not tainted
5.18.0-rc3-next-20220420-00033-g871e1a91a67f #4866
Hardware name: Samsung Exynos (Flattened Device Tree)
unwind_backtrace from show_stack+0x10/0x14
show_stack from dump_stack_lvl+0x58/0x70
dump_stack_lvl from __warn+0xc8/0x218
__warn from warn_slowpath_fmt+0x5c/0xb4
warn_slowpath_fmt from drm_connector_cleanup+0x26c/0x288
drm_connector_cleanup from exynos_dsi_host_detach+0x24/0x74
exynos_dsi_host_detach from s6e8aa0_remove+0xc/0x1c
[panel_samsung_s6e8aa0]
s6e8aa0_remove [panel_samsung_s6e8aa0] from
device_release_driver_internal+0x1a4/0x20c
device_release_driver_internal from driver_detach+0x44/0x80
driver_detach from bus_remove_driver+0x60/0xd8
bus_remove_driver from sys_delete_module+0x138/0x22c
sys_delete_module from ret_fast_syscall+0x0/0x1c
Exception stack(0xf0c01fa8 to 0xf0c01ff0)
1fa0: 005224d8 00000002 00522514 00000800 e2a92e00
e2a92e00
1fc0: 005224d8 00000002 005224d8 00000081 be9daf02 be9dac1c be9dae08
00000001
1fe0: 00520f70 be9dabb4 00503ac4 b6e2a0fc
irq event stamp: 3465
hardirqs last enabled at (3481): [<c015a448>]
finish_task_switch+0x110/0x368
hardirqs last disabled at (3490): [<c019d210>] __up_console_sem+0x3c/0x60
softirqs last enabled at (3460): [<c0101680>] __do_softirq+0x348/0x610
softirqs last disabled at (3423): [<c012ed64>] __irq_exit_rcu+0x144/0x1ec
---[ end trace 0000000000000000 ]---
8<--- cut here ---
This is however just yet another issue caused by the recent Exynos DSI
driver conversion to DRM bridge. In v5.17 it worked fine. I will try to
look into that issue later, this is not related to the $subject.
Subsequent panel registration (after it has been first unregistered)
seems to be broken for ages. It worked fine in v4.19, but then it got
broken somewhere before v4.20. I've added this to my todo list (with low
priority though).
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
