| Message ID | 20230807061115.3244501-1-victor.liu@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] drm/bridge: panel: Add a device link between drm device and panel device | expand |
On Mon, 7 Aug 2023 at 08:06, Liu Ying <victor.liu@nxp.com> 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.kernel.org/lkml/CAPDyKFr0XjrU_udKoUKQ_q8RWaUkyqL+8fV-7s1CTMqi7u3-Rg@mail.gmail.com/T/ > Suggested-by: Ulf Hansson <ulf.hansson@linaro.org> > Signed-off-by: Liu Ying <victor.liu@nxp.com> Looks good to me! Just a minor question though, don't we need to manage runtime PM too - or this is solely for system wide suspend/resume? Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > v2->v3: > * Improve commit message s/swapped/reversed/. > > v1->v2: > * Fix bailout for panel_bridge_attach() in case device_link_add() fails. > > drivers/gpu/drm/bridge/panel.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c > index 9316384b4474..a6587d233505 100644 > --- a/drivers/gpu/drm/bridge/panel.c > +++ b/drivers/gpu/drm/bridge/panel.c > @@ -4,6 +4,8 @@ > * Copyright (C) 2017 Broadcom > */ > > +#include <linux/device.h> > + > #include <drm/drm_atomic_helper.h> > #include <drm/drm_bridge.h> > #include <drm/drm_connector.h> > @@ -19,6 +21,7 @@ struct panel_bridge { > struct drm_bridge bridge; > struct drm_connector connector; > struct drm_panel *panel; > + struct device_link *link; > u32 connector_type; > }; > > @@ -60,6 +63,8 @@ static int panel_bridge_attach(struct drm_bridge *bridge, > { > struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > struct drm_connector *connector = &panel_bridge->connector; > + struct drm_panel *panel = panel_bridge->panel; > + struct drm_device *drm_dev = bridge->dev; > int ret; > > if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) > @@ -70,6 +75,14 @@ static int panel_bridge_attach(struct drm_bridge *bridge, > return -ENODEV; > } > > + 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; > + } > + > drm_connector_helper_add(connector, > &panel_bridge_connector_helper_funcs); > > @@ -78,6 +91,7 @@ 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); > return ret; > } > > @@ -100,6 +114,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); > + > /* > * Cleanup the connector if we know it was initialized. > * > -- > 2.37.1 >
On Wednesday, August 9, 2023 9:54 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Mon, 7 Aug 2023 at 08:06, Liu Ying <victor.liu@nxp.com> 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 > Cb498937c20c94ab9148908db98e02662%7C686ea1d3bc2b4c6fa92cd99c5c30 > 1635%7C0%7C0%7C638271860697989733%7CUnknown%7CTWFpbGZsb3d8e > yJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D > %7C3000%7C%7C%7C&sdata=iGMdYWbOeyVxzy9T9THCNh%2Ff%2BbKFLP0tI > m%2BowL7h5Og%3D&reserved=0 > > Suggested-by: Ulf Hansson <ulf.hansson@linaro.org> > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > > Looks good to me! Just a minor question though, don't we need to > manage runtime PM too - or this is solely for system wide > suspend/resume? I think this is solely for system wide suspend/resume. AFAICS, there is no any particular need to manage runtime PM. > > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Thank you for your review. Regards, Liu Ying > > Kind regards > Uffe > > > --- > > v2->v3: > > * Improve commit message s/swapped/reversed/. > > > > v1->v2: > > * Fix bailout for panel_bridge_attach() in case device_link_add() fails. > > > > drivers/gpu/drm/bridge/panel.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/drivers/gpu/drm/bridge/panel.c > b/drivers/gpu/drm/bridge/panel.c > > index 9316384b4474..a6587d233505 100644 > > --- a/drivers/gpu/drm/bridge/panel.c > > +++ b/drivers/gpu/drm/bridge/panel.c > > @@ -4,6 +4,8 @@ > > * Copyright (C) 2017 Broadcom > > */ > > > > +#include <linux/device.h> > > + > > #include <drm/drm_atomic_helper.h> > > #include <drm/drm_bridge.h> > > #include <drm/drm_connector.h> > > @@ -19,6 +21,7 @@ struct panel_bridge { > > struct drm_bridge bridge; > > struct drm_connector connector; > > struct drm_panel *panel; > > + struct device_link *link; > > u32 connector_type; > > }; > > > > @@ -60,6 +63,8 @@ static int panel_bridge_attach(struct drm_bridge > *bridge, > > { > > struct panel_bridge *panel_bridge = > drm_bridge_to_panel_bridge(bridge); > > struct drm_connector *connector = &panel_bridge->connector; > > + struct drm_panel *panel = panel_bridge->panel; > > + struct drm_device *drm_dev = bridge->dev; > > int ret; > > > > if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) > > @@ -70,6 +75,14 @@ static int panel_bridge_attach(struct drm_bridge > *bridge, > > return -ENODEV; > > } > > > > + 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; > > + } > > + > > drm_connector_helper_add(connector, > > &panel_bridge_connector_helper_funcs); > > > > @@ -78,6 +91,7 @@ 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); > > return ret; > > } > > > > @@ -100,6 +114,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); > > + > > /* > > * Cleanup the connector if we know it was initialized. > > * > > -- > > 2.37.1 > >
Hi, On Mon, 07 Aug 2023 14:11:15 +0800, Liu Ying 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. > > [...] Thanks, Applied to https://anongit.freedesktop.org/git/drm/drm-misc.git (drm-misc-next) [1/1] drm/bridge: panel: Add a device link between drm device and panel device https://cgit.freedesktop.org/drm/drm-misc/commit/?id=199cf07ebd2b0d41185ac79b895547d45610b681
On Mon, Aug 7, 2023 at 8:06 AM Liu Ying <victor.liu@nxp.com> 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.kernel.org/lkml/CAPDyKFr0XjrU_udKoUKQ_q8RWaUkyqL+8fV-7s1CTMqi7u3-Rg@mail.gmail.com/T/ > Suggested-by: Ulf Hansson <ulf.hansson@linaro.org> > Signed-off-by: Liu Ying <victor.liu@nxp.com> > --- > 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 [ 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. Suggestions welcome, I'm clueless why this happens. Yours, Linus Walleij
Hi Linus, On Wednesday, November 15, 2023 6:00 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, Aug 7, 2023 at 8:06 AM Liu Ying <victor.liu@nxp.com> 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@linaro.org> > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > > --- > > 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
Hi Ying, On Mon, Nov 20, 2023 at 11:08 AM Ying Liu <victor.liu@nxp.com> wrote: [Me] > > > 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: (...) > 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? Yep it works! You missed one device_link_del() instance on the errorpath. Tested-by: Linus Walleij <linus.walleij@linaro.org> Can you send it as a proper patch? Yours, Linus Walleij
On Wednesday, November 22, 2023 9:59 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > Hi Ying, Hi Linus, > > On Mon, Nov 20, 2023 at 11:08 AM Ying Liu <victor.liu@nxp.com> wrote: > > [Me] > > > > 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: > (...) > > 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? > > Yep it works! > > You missed one device_link_del() instance on the errorpath. Will add it. > > Tested-by: Linus Walleij <linus.walleij@linaro.org> Thanks for the test. > > Can you send it as a proper patch? Will do. Regards, Liu Ying
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index 9316384b4474..a6587d233505 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -4,6 +4,8 @@ * Copyright (C) 2017 Broadcom */ +#include <linux/device.h> + #include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> #include <drm/drm_connector.h> @@ -19,6 +21,7 @@ struct panel_bridge { struct drm_bridge bridge; struct drm_connector connector; struct drm_panel *panel; + struct device_link *link; u32 connector_type; }; @@ -60,6 +63,8 @@ static int panel_bridge_attach(struct drm_bridge *bridge, { struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); struct drm_connector *connector = &panel_bridge->connector; + struct drm_panel *panel = panel_bridge->panel; + struct drm_device *drm_dev = bridge->dev; int ret; if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) @@ -70,6 +75,14 @@ static int panel_bridge_attach(struct drm_bridge *bridge, return -ENODEV; } + 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; + } + drm_connector_helper_add(connector, &panel_bridge_connector_helper_funcs); @@ -78,6 +91,7 @@ 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); return ret; } @@ -100,6 +114,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); + /* * Cleanup the connector if we know it was initialized. *
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.kernel.org/lkml/CAPDyKFr0XjrU_udKoUKQ_q8RWaUkyqL+8fV-7s1CTMqi7u3-Rg@mail.gmail.com/T/ Suggested-by: Ulf Hansson <ulf.hansson@linaro.org> Signed-off-by: Liu Ying <victor.liu@nxp.com> --- v2->v3: * Improve commit message s/swapped/reversed/. v1->v2: * Fix bailout for panel_bridge_attach() in case device_link_add() fails. drivers/gpu/drm/bridge/panel.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)