[PATCH] drm: bridge: panel: Register connector if DRM device is already registered

[PATCH] drm: bridge: panel: Register connector if DRM device is already registered

[Marek Szyprowski]

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@samsung.com>
---
Here is a bit more backgroud on this issue is available in this thread:
https://lore.kernel.org/all/f0474a95-4ba3-a74f-d7de-ef7aab12bc86@samsung.com/
---
 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);
+	}
 
 	return 0;
 }
-- 
2.17.1

Re: [PATCH] drm: bridge: panel: Register connector if DRM device is already registered

[Jagan Teki]

On Tue, Apr 19, 2022 at 2:44 PM Marek Szyprowski
<m.szyprowski@samsung.com> 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@samsung.com>
> ---
> Here is a bit more backgroud on this issue is available in this thread:
> https://lore.kernel.org/all/f0474a95-4ba3-a74f-d7de-ef7aab12bc86@samsung.com/
> ---
>  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@amarulasolutions.com>

Re: [PATCH] drm: bridge: panel: Register connector if DRM device is already registered

[Robert Foss]

On Tue, 19 Apr 2022 at 11:41, Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> On Tue, Apr 19, 2022 at 2:44 PM Marek Szyprowski
> <m.szyprowski@samsung.com> 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@samsung.com>
> > ---
> > Here is a bit more backgroud on this issue is available in this thread:
> > https://lore.kernel.org/all/f0474a95-4ba3-a74f-d7de-ef7aab12bc86@samsung.com/
> > ---
> >  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@amarulasolutions.com>

Fixed typos in commit message.

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Applied to drm-misc-next

Re: [PATCH] drm: bridge: panel: Register connector if DRM device is already registered

[Laurent Pinchart]

On Tue, Apr 19, 2022 at 06:16:07PM +0200, Robert Foss wrote:
> On Tue, 19 Apr 2022 at 11:41, Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > On Tue, Apr 19, 2022 at 2:44 PM Marek Szyprowski
> > <m.szyprowski@samsung.com> 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@samsung.com>
> > > ---
> > > Here is a bit more backgroud on this issue is available in this thread:
> > > https://lore.kernel.org/all/f0474a95-4ba3-a74f-d7de-ef7aab12bc86@samsung.com/
> > > ---
> > >  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@amarulasolutions.com>
> 
> Fixed typos in commit message.
> 
> Reviewed-by: Robert Foss <robert.foss@linaro.org>
> 
> Applied to drm-misc-next

Doesn't this open the door to various race conditions ?

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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm: bridge: panel: Register connector if DRM device is already registered

[Jagan Teki]

On Tue, Apr 19, 2022 at 11:08 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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@amarulasolutions.com> wrote:
> > >
> > > On Tue, Apr 19, 2022 at 2:44 PM Marek Szyprowski
> > > <m.szyprowski@samsung.com> 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@samsung.com>
> > > > ---
> > > > Here is a bit more backgroud on this issue is available in this thread:
> > > > https://lore.kernel.org/all/f0474a95-4ba3-a74f-d7de-ef7aab12bc86@samsung.com/
> > > > ---
> > > >  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@amarulasolutions.com>
> >
> > Fixed typos in commit message.
> >
> > Reviewed-by: Robert Foss <robert.foss@linaro.org>
> >
> > Applied to drm-misc-next
>
> Doesn't this open the door to various race conditions ?
>
> Also, what happens if the panel bridge is detached and reattached ? If I

I believe the dev->registered is the check for those already registered.

Jagan.

Re: [PATCH] drm: bridge: panel: Register connector if DRM device is already registered

[Marek Szyprowski]

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@amarulasolutions.com> wrote:
>>> On Tue, Apr 19, 2022 at 2:44 PM Marek Szyprowski
>>> <m.szyprowski@samsung.com> 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@samsung.com>
>>>> ---
>>>> Here is a bit more backgroud on this issue is available in this thread:
>>>> https://lore.kernel.org/all/f0474a95-4ba3-a74f-d7de-ef7aab12bc86@samsung.com/
>>>> ---
>>>>   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@amarulasolutions.com>
>> Fixed typos in commit message.
>>
>> Reviewed-by: Robert Foss <robert.foss@linaro.org>
>>
>> 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