RE: [PATCH v3] drm/bridge: panel: Add a device link between drm device and panel device

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Linus,

On Wednesday, November 15, 2023 6:00 AM, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> On Mon, Aug 7, 2023 at 8:06 AM Liu Ying <victor.liu@xxxxxxx> wrote:
>
> > Add the device link when panel bridge is attached and delete the link
> > when panel bridge is detached.  The drm device is the consumer while
> > the panel device is the supplier.  This makes sure that the drm device
> > suspends eariler and resumes later than the panel device, hence resolves
> > problems where the order is reversed, like the problematic case mentioned
> > in the below link.
> >
> > Link:
> https://lore.k/
> ernel.org%2Flkml%2FCAPDyKFr0XjrU_udKoUKQ_q8RWaUkyqL%2B8fV-
> 7s1CTMqi7u3-
> Rg%40mail.gmail.com%2FT%2F&data=05%7C01%7Cvictor.liu%40nxp.com%7
> Cd007e6260de84d50c92b08dbe55d10e5%7C686ea1d3bc2b4c6fa92cd99c5c3
> 01635%7C0%7C0%7C638355960110773397%7CUnknown%7CTWFpbGZsb3d8
> eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3
> D%7C3000%7C%7C%7C&sdata=eXtezc2MPeFy1uo09iqHlYJq3iA6S%2BfxSre5y
> s7xrhc%3D&reserved=0
> > Suggested-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> > Signed-off-by: Liu Ying <victor.liu@xxxxxxx>
> > ---
> > v2->v3:
> > * Improve commit message s/swapped/reversed/.
>
> This patch causes a regression in the Ux500 MCDE
> drivers/gpu/drm/mcde/* driver with the nt35510 panel
> drivers/gpu/drm/panel/panel-novatek-nt35510.c
> my dmesg looks like this:
>
> [    1.678680] mcde a0350000.mcde: MCDE clk rate 199680000 Hz
> [    1.684448] mcde a0350000.mcde: found MCDE HW revision 3.0 (dev 8,
> metal fix 0)
> [    1.692840] mcde-dsi a0351000.dsi: HW revision 0x02327457
> [    1.699310] mcde-dsi a0351000.dsi: attached DSI device with 2 lanes
> [    1.705627] mcde-dsi a0351000.dsi: format 00000000, 24bpp
> [    1.711059] mcde-dsi a0351000.dsi: mode flags: 00000400
> [    1.716400] mcde-dsi a0351000.dsi: registered DSI host
> [    1.722473] mcde-dsi a0352000.dsi: HW revision 0x02327457
> [    1.727874] mcde-dsi a0352000.dsi: registered DSI host
> [    1.733734] mcde-dsi a0353000.dsi: HW revision 0x02327457
> [    1.739135] mcde-dsi a0353000.dsi: registered DSI host
> [    1.814971] mcde-dsi a0351000.dsi: connected to panel
> [    1.820037] mcde-dsi a0351000.dsi: initialized MCDE DSI bridge
> [    1.825958] mcde a0350000.mcde: bound a0351000.dsi (ops
> mcde_dsi_component_ops)
> [    1.833312] mcde-dsi a0352000.dsi: unused DSI interface
> [    1.838531] mcde a0350000.mcde: bound a0352000.dsi (ops
> mcde_dsi_component_ops)
> [    1.845886] mcde-dsi a0353000.dsi: unused DSI interface
> [    1.851135] mcde a0350000.mcde: bound a0353000.dsi (ops
> mcde_dsi_component_ops)
> [    1.858917] [drm:panel_bridge_attach] *ERROR* Failed to add device
> link between a0350000.mcde and a0351000.dsi.0

Sorry for the breakage and a bit late response(I'm a bit busy with internal
things).

I think device_link_add() fails because a0351000.dsi.0 already depends
on a0350000.mcde.  Can you confirm that device_link_add() returns NULL
right after it calls device_is_dependent()?

Does this patch fix the issue?

--------------------------------------8<---------------------------------------------------
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index e48823a4f1ed..d44de301a312 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -23,6 +23,7 @@ struct panel_bridge {
        struct drm_panel *panel;
        struct device_link *link;
        u32 connector_type;
+       bool is_independent;
 };

 static inline struct panel_bridge *
@@ -67,12 +68,17 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
        struct drm_device *drm_dev = bridge->dev;
        int ret;

-       panel_bridge->link = device_link_add(drm_dev->dev, panel->dev,
-                                            DL_FLAG_STATELESS);
-       if (!panel_bridge->link) {
-               DRM_ERROR("Failed to add device link between %s and %s\n",
-                         dev_name(drm_dev->dev), dev_name(panel->dev));
-               return -EINVAL;
+       panel_bridge->is_independent = !device_is_dependent(drm_dev->dev,
+                                                           panel->dev);
+
+       if (panel_bridge->is_independent) {
+               panel_bridge->link = device_link_add(drm_dev->dev, panel->dev,
+                                                    DL_FLAG_STATELESS);
+               if (!panel_bridge->link) {
+                       DRM_ERROR("Failed to add device link between %s and %s\n",
+                                 dev_name(drm_dev->dev), dev_name(panel->dev));
+                       return -EINVAL;
+               }
        }

        if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
@@ -92,7 +98,8 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
                                 panel_bridge->connector_type);
        if (ret) {
                DRM_ERROR("Failed to initialize connector\n");
-               device_link_del(panel_bridge->link);
+               if (panel_bridge->is_independent)
+                       device_link_del(panel_bridge->link);
                return ret;
        }

@@ -115,7 +122,8 @@ static void panel_bridge_detach(struct drm_bridge *bridge)
        struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
        struct drm_connector *connector = &panel_bridge->connector;

-       device_link_del(panel_bridge->link);
+       if (panel_bridge->is_independent)
+               device_link_del(panel_bridge->link);

        /*
         * Cleanup the connector if we know it was initialized.
--------------------------------------8<---------------------------------------------------

> [    1.869171] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
> /soc/mcde@a0350000/dsi@a0351000/panel to encoder None-34: -22
> [    1.880920] [drm:drm_bridge_attach] *ERROR* failed to attach bridge
> /soc/mcde@a0350000/dsi@a0351000 to encoder None-34: -22
> [    1.892120] mcde a0350000.mcde: failed to attach display output bridge
> [    1.898773] mcde a0350000.mcde: adev bind failed: -22
> [    1.903991] mcde a0350000.mcde: failed to add component master
> [    1.909912] mcde: probe of a0350000.mcde failed with error -22
> [    1.916656] ------------[ cut here ]------------
> [    1.921295] WARNING: CPU: 1 PID: 1 at drivers/regulator/core.c:2996
> _regulator_disable+0x130/0x190
> [    1.930297] unbalanced disables for AUX6
> [    1.934265] Modules linked in:
> [    1.937347] CPU: 1 PID: 1 Comm: swapper/0 Not tainted
> 6.6.0-08649-g7d461b291e65 #3
> [    1.944915] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree
> Support)
> [    1.951873]  unwind_backtrace from show_stack+0x10/0x14
> [    1.957122]  show_stack from dump_stack_lvl+0x40/0x4c
> [    1.962188]  dump_stack_lvl from __warn+0x84/0xc8
> [    1.966918]  __warn from warn_slowpath_fmt+0x124/0x190
> [    1.972076]  warn_slowpath_fmt from _regulator_disable+0x130/0x190
> [    1.978271]  _regulator_disable from regulator_bulk_disable+0x5c/0x100
> [    1.984802]  regulator_bulk_disable from nt35510_remove+0x1c/0x58
> [    1.990905]  nt35510_remove from mipi_dsi_drv_remove+0x18/0x20
> [    1.996765]  mipi_dsi_drv_remove from
> device_release_driver_internal+0x184/0x1f8
> [    2.004180]  device_release_driver_internal from
> bus_remove_device+0xc0/0xe4
> [    2.011230]  bus_remove_device from device_del+0x14c/0x464
> [    2.016723]  device_del from device_unregister+0xc/0x20
> [    2.021972]  device_unregister from mipi_dsi_remove_device_fn+0x34/0x3c
> [    2.028594]  mipi_dsi_remove_device_fn from
> device_for_each_child+0x64/0xa4
> [    2.035583]  device_for_each_child from
> mipi_dsi_host_unregister+0x24/0x50
> [    2.042480]  mipi_dsi_host_unregister from platform_remove+0x20/0x5c
> [    2.048858]  platform_remove from
> device_release_driver_internal+0x184/0x1f8
> [    2.055908]  device_release_driver_internal from
> bus_remove_device+0xc0/0xe4
> [    2.062957]  bus_remove_device from device_del+0x14c/0x464
> [    2.068450]  device_del from platform_device_del.part.0+0x10/0x74
> [    2.074554]  platform_device_del.part.0 from
> platform_device_unregister+0x18/0x24
> [    2.082061]  platform_device_unregister from
> of_platform_device_destroy+0x9c/0xac
> [    2.089569]  of_platform_device_destroy from
> device_for_each_child_reverse+0x78/0xbc
> [    2.097320]  device_for_each_child_reverse from
> devm_of_platform_populate_release+0x34/0x48
> [    2.105682]  devm_of_platform_populate_release from
> devres_release_all+0x94/0xf8
> [    2.113098]  devres_release_all from device_unbind_cleanup+0xc/0x60
> [    2.119384]  device_unbind_cleanup from really_probe+0x1a0/0x2d8
> [    2.125396]  really_probe from __driver_probe_device+0x84/0xd4
> [    2.131225]  __driver_probe_device from driver_probe_device+0x30/0x104
> [    2.137756]  driver_probe_device from __driver_attach+0x90/0x178
> [    2.143768]  __driver_attach from bus_for_each_dev+0x7c/0xcc
> [    2.149444]  bus_for_each_dev from bus_add_driver+0xcc/0x1cc
> [    2.155120]  bus_add_driver from driver_register+0x7c/0x114
> [    2.160705]  driver_register from do_one_initcall+0x5c/0x190
> [    2.166381]  do_one_initcall from kernel_init_freeable+0x1f8/0x250
> [    2.172576]  kernel_init_freeable from kernel_init+0x18/0x12c
> [    2.178344]  kernel_init from ret_from_fork+0x14/0x28
> [    2.183410] Exception stack(0xf08c5fb0 to 0xf08c5ff8)
> [    2.188446] 5fa0:                                     00000000
> 00000000 00000000 00000000
> [    2.196624] 5fc0: 00000000 00000000 00000000 00000000 00000000
> 00000000 00000000 00000000
> [    2.204803] 5fe0: 00000000 00000000 00000000 00000000 00000013
> 00000000
> [    2.211486] ---[ end trace 0000000000000000 ]---
> [    2.216125] Failed to disable vddi: -EIO
> [    2.220062] panel-novatek-nt35510 a0351000.dsi.0: Failed to power off
>
> Reverting the patch solves the problem.
>
> See device tree at e.g.:
> arch/arm/boot/dts/st/ste-ux500-samsung-skomer.dts
>
> The usual problems with patches like this is that our DSI panel
> is attached in the DT without any graph:
>
>                 mcde@a0350000 {
>                         status = "okay";
>                         pinctrl-names = "default";
>                         pinctrl-0 = <&dsi_default_mode>;
>
>                         dsi@a0351000 {
>                                 panel {
>                                         /* NT35510-based Hydis HVA40WV1 */
>                                         compatible = "hydis,hva40wv1",
> "novatek,nt35510";
>                                         reg = <0>;
>                                         /* v_lcd_3v0 2.3-4.8V */
>                                         vdd-supply = <&ab8500_ldo_aux4_reg>;
>                                         /* v_lcd_1v8 1.65-3.3V */
>                                         vddi-supply = <&ab8500_ldo_aux6_reg>;
>                                         /* GPIO 139 */
>                                         reset-gpios = <&gpio4 11
> GPIO_ACTIVE_LOW>;
>                                         pinctrl-names = "default";
>                                         pinctrl-0 = <&display_default_mode>;
>                                         backlight = <&ktd253>;
>                                 };
>                         };
>                 };
>
> The DSI bridge is inside the display controller (MCDE) and the panel is
> right inside the DSI bridge.

This indicates that the panel device already depends on the mcde device.

Regards,
Liu Ying

>
> Suggestions welcome, I'm clueless why this happens.
>
> Yours,
> Linus Walleij




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux