| Message ID | 20200726203324.3722593-5-sam@ravnborg.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: support chained bridges + panel updates | expand |
Hi Sam, Thank you for the patch. On Sun, Jul 26, 2020 at 10:33:13PM +0200, Sam Ravnborg wrote: > Prepare the tc358764 bridge driver for use in a chained setup by > replacing direct use of drm_panel with drm_panel_bridge support. > > The bridge panel will use the connector type reported by the panel, > where the connector for this driver hardcodes DRM_MODE_CONNECTOR_LVDS. > > The tc358764 did not any additional info the the connector so the Did you mean s/did not any/did not add any/ ? s/the the/to the/ > connector creation is passed to the bridge panel driver. > > v3: > - Merge with patch to make connector creation optional to avoid > creating two connectors (Laurent) > - Pass connector creation to bridge panel, as this bridge driver > did not add any extra info to the connector. > - Set bridge.type to DRM_MODE_CONNECTOR_LVDS. > > v2: > - Use PTR_ERR_OR_ZERO() (kbuild test robot) > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: kbuild test robot <lkp@intel.com> > Cc: Andrzej Hajda <a.hajda@samsung.com> > Cc: Neil Armstrong <narmstrong@baylibre.com> > Cc: Jonas Karlman <jonas@kwiboo.se> > Cc: Jernej Skrabec <jernej.skrabec@siol.net> > --- > drivers/gpu/drm/bridge/tc358764.c | 107 +++++------------------------- > 1 file changed, 16 insertions(+), 91 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c > index a277739fab58..fdde4cfdc724 100644 > --- a/drivers/gpu/drm/bridge/tc358764.c > +++ b/drivers/gpu/drm/bridge/tc358764.c > @@ -153,10 +153,9 @@ static const char * const tc358764_supplies[] = { > struct tc358764 { > struct device *dev; > struct drm_bridge bridge; > - struct drm_connector connector; > struct regulator_bulk_data supplies[ARRAY_SIZE(tc358764_supplies)]; > struct gpio_desc *gpio_reset; > - struct drm_panel *panel; > + struct drm_bridge *panel_bridge; > int error; > }; > > @@ -210,12 +209,6 @@ static inline struct tc358764 *bridge_to_tc358764(struct drm_bridge *bridge) > return container_of(bridge, struct tc358764, bridge); > } > > -static inline > -struct tc358764 *connector_to_tc358764(struct drm_connector *connector) > -{ > - return container_of(connector, struct tc358764, connector); > -} > - > static int tc358764_init(struct tc358764 *ctx) > { > u32 v = 0; > @@ -278,43 +271,11 @@ static void tc358764_reset(struct tc358764 *ctx) > usleep_range(1000, 2000); > } > > -static int tc358764_get_modes(struct drm_connector *connector) > -{ > - struct tc358764 *ctx = connector_to_tc358764(connector); > - > - return drm_panel_get_modes(ctx->panel, connector); > -} > - > -static const > -struct drm_connector_helper_funcs tc358764_connector_helper_funcs = { > - .get_modes = tc358764_get_modes, > -}; > - > -static const struct drm_connector_funcs tc358764_connector_funcs = { > - .fill_modes = drm_helper_probe_single_connector_modes, > - .destroy = drm_connector_cleanup, > - .reset = drm_atomic_helper_connector_reset, > - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > -}; > - > -static void tc358764_disable(struct drm_bridge *bridge) > -{ > - struct tc358764 *ctx = bridge_to_tc358764(bridge); > - int ret = drm_panel_disable(bridge_to_tc358764(bridge)->panel); > - > - if (ret < 0) > - dev_err(ctx->dev, "error disabling panel (%d)\n", ret); > -} > - > static void tc358764_post_disable(struct drm_bridge *bridge) > { > struct tc358764 *ctx = bridge_to_tc358764(bridge); > int ret; > > - ret = drm_panel_unprepare(ctx->panel); > - if (ret < 0) > - dev_err(ctx->dev, "error unpreparing panel (%d)\n", ret); > tc358764_reset(ctx); > usleep_range(10000, 15000); > ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies); > @@ -335,70 +296,28 @@ static void tc358764_pre_enable(struct drm_bridge *bridge) > ret = tc358764_init(ctx); > if (ret < 0) > dev_err(ctx->dev, "error initializing bridge (%d)\n", ret); > - ret = drm_panel_prepare(ctx->panel); > - if (ret < 0) > - dev_err(ctx->dev, "error preparing panel (%d)\n", ret); > -} > - > -static void tc358764_enable(struct drm_bridge *bridge) > -{ > - struct tc358764 *ctx = bridge_to_tc358764(bridge); > - int ret = drm_panel_enable(ctx->panel); > - > - if (ret < 0) > - dev_err(ctx->dev, "error enabling panel (%d)\n", ret); > } > > static int tc358764_attach(struct drm_bridge *bridge, > enum drm_bridge_attach_flags flags) > -{ > - struct tc358764 *ctx = bridge_to_tc358764(bridge); > - struct drm_device *drm = bridge->dev; > - int ret; > - > - if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { > - DRM_ERROR("Fix bridge driver to make connector optional!"); > - return -EINVAL; > - } > - > - ctx->connector.polled = DRM_CONNECTOR_POLL_HPD; > - ret = drm_connector_init(drm, &ctx->connector, > - &tc358764_connector_funcs, > - DRM_MODE_CONNECTOR_LVDS); > - if (ret) { > - DRM_ERROR("Failed to initialize connector\n"); > - return ret; > - } > - > - drm_connector_helper_add(&ctx->connector, > - &tc358764_connector_helper_funcs); > - drm_connector_attach_encoder(&ctx->connector, bridge->encoder); > - drm_panel_attach(ctx->panel, &ctx->connector); > - ctx->connector.funcs->reset(&ctx->connector); > - > - return 0; > -} > - > -static void tc358764_detach(struct drm_bridge *bridge) > { > struct tc358764 *ctx = bridge_to_tc358764(bridge); > > - drm_panel_detach(ctx->panel); > - ctx->panel = NULL; > + return drm_bridge_attach(bridge->encoder, ctx->panel_bridge, > + bridge, flags); > } > > static const struct drm_bridge_funcs tc358764_bridge_funcs = { > - .disable = tc358764_disable, > .post_disable = tc358764_post_disable, > - .enable = tc358764_enable, > .pre_enable = tc358764_pre_enable, > .attach = tc358764_attach, > - .detach = tc358764_detach, > }; > > static int tc358764_parse_dt(struct tc358764 *ctx) > { > + struct drm_bridge *panel_bridge; > struct device *dev = ctx->dev; > + struct drm_panel *panel; > int ret; > > ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); > @@ -407,12 +326,17 @@ static int tc358764_parse_dt(struct tc358764 *ctx) > return PTR_ERR(ctx->gpio_reset); > } > > - ret = drm_of_find_panel_or_bridge(ctx->dev->of_node, 1, 0, &ctx->panel, > - NULL); > - if (ret && ret != -EPROBE_DEFER) > - dev_err(dev, "cannot find panel (%d)\n", ret); > + ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL); > + if (ret) > + return ret; > > - return ret; > + panel_bridge = devm_drm_panel_bridge_add(dev, panel); > + You can drop this blank line. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + if (IS_ERR(panel_bridge)) > + return PTR_ERR(panel_bridge); > + > + ctx->panel_bridge = panel_bridge; > + return 0; > } > > static int tc358764_configure_regulators(struct tc358764 *ctx) > @@ -458,6 +382,7 @@ static int tc358764_probe(struct mipi_dsi_device *dsi) > return ret; > > ctx->bridge.funcs = &tc358764_bridge_funcs; > + ctx->bridge.type = DRM_MODE_CONNECTOR_LVDS; > ctx->bridge.of_node = dev->of_node; > > drm_bridge_add(&ctx->bridge);
Hi Sam, On 26.07.2020 22:33, Sam Ravnborg wrote: > Prepare the tc358764 bridge driver for use in a chained setup by > replacing direct use of drm_panel with drm_panel_bridge support. > > The bridge panel will use the connector type reported by the panel, > where the connector for this driver hardcodes DRM_MODE_CONNECTOR_LVDS. > > The tc358764 did not any additional info the the connector so the > connector creation is passed to the bridge panel driver. > > v3: > - Merge with patch to make connector creation optional to avoid > creating two connectors (Laurent) > - Pass connector creation to bridge panel, as this bridge driver > did not add any extra info to the connector. > - Set bridge.type to DRM_MODE_CONNECTOR_LVDS. > > v2: > - Use PTR_ERR_OR_ZERO() (kbuild test robot) > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: kbuild test robot <lkp@intel.com> > Cc: Andrzej Hajda <a.hajda@samsung.com> > Cc: Neil Armstrong <narmstrong@baylibre.com> > Cc: Jonas Karlman <jonas@kwiboo.se> > Cc: Jernej Skrabec <jernej.skrabec@siol.net> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I've noticed that this patch has been merged recently to linux-next. Sadly it causes regression on Samsung Exynos5250-based Arndale board. It can be observed by the following warning during boot: ------------[ cut here ]------------ WARNING: CPU: 1 PID: 1 at drivers/gpu/drm/drm_atomic_state_helper.c:494 drm_atomic_helper_connector_duplicate_state+0x60/0x68 Modules linked in: CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.8.0-rc2-00501-g1644127f83bc #1526 Hardware name: Samsung Exynos (Flattened Device Tree) [<c011184c>] (unwind_backtrace) from [<c010d250>] (show_stack+0x10/0x14) [<c010d250>] (show_stack) from [<c0517ce4>] (dump_stack+0xbc/0xe8) [<c0517ce4>] (dump_stack) from [<c01270a8>] (__warn+0xf0/0x108) [<c01270a8>] (__warn) from [<c0127170>] (warn_slowpath_fmt+0xb0/0xb8) [<c0127170>] (warn_slowpath_fmt) from [<c05e81f0>] (drm_atomic_helper_connector_duplicate_state+0x60/0x68) [<c05e81f0>] (drm_atomic_helper_connector_duplicate_state) from [<c06014b8>] (drm_atomic_get_connector_state+0xfc/0x184) [<c06014b8>] (drm_atomic_get_connector_state) from [<c0602238>] (__drm_atomic_helper_set_config+0x2a0/0x368) [<c0602238>] (__drm_atomic_helper_set_config) from [<c06183b8>] (drm_client_modeset_commit_atomic+0x180/0x284) [<c06183b8>] (drm_client_modeset_commit_atomic) from [<c061859c>] (drm_client_modeset_commit_locked+0x64/0x1cc) [<c061859c>] (drm_client_modeset_commit_locked) from [<c0618728>] (drm_client_modeset_commit+0x24/0x40) [<c0618728>] (drm_client_modeset_commit) from [<c05eb6b4>] (drm_fb_helper_restore_fbdev_mode_unlocked+0x50/0x94) [<c05eb6b4>] (drm_fb_helper_restore_fbdev_mode_unlocked) from [<c05eb728>] (drm_fb_helper_set_par+0x30/0x5c) [<c05eb728>] (drm_fb_helper_set_par) from [<c055dedc>] (fbcon_init+0x5c8/0x65c) [<c055dedc>] (fbcon_init) from [<c05a8530>] (visual_init+0xc0/0x108) [<c05a8530>] (visual_init) from [<c05aaca4>] (do_bind_con_driver+0x180/0x39c) [<c05aaca4>] (do_bind_con_driver) from [<c05ab244>] (do_take_over_console+0x140/0x1cc) [<c05ab244>] (do_take_over_console) from [<c055ac04>] (do_fbcon_takeover+0x84/0xe0) [<c055ac04>] (do_fbcon_takeover) from [<c0553820>] (register_framebuffer+0x1cc/0x2dc) [<c0553820>] (register_framebuffer) from [<c05eb19c>] (__drm_fb_helper_initial_config_and_unlock+0x3f0/0x5e8) [<c05eb19c>] (__drm_fb_helper_initial_config_and_unlock) from [<c05d941c>] (drm_kms_helper_hotplug_event+0x24/0x30) [<c05d941c>] (drm_kms_helper_hotplug_event) from [<c0628f74>] (exynos_dsi_host_attach+0x184/0x2d8) [<c0628f74>] (exynos_dsi_host_attach) from [<c0634120>] (tc358764_probe+0x13c/0x1ac) [<c0634120>] (tc358764_probe) from [<c064cce4>] (really_probe+0x200/0x48c) [<c064cce4>] (really_probe) from [<c064d0d8>] (driver_probe_device+0x78/0x1fc) [<c064d0d8>] (driver_probe_device) from [<c064d4c0>] (device_driver_attach+0x58/0x60) [<c064d4c0>] (device_driver_attach) from [<c064d5a4>] (__driver_attach+0xdc/0x174) [<c064d5a4>] (__driver_attach) from [<c064aaf0>] (bus_for_each_dev+0x68/0xb4) [<c064aaf0>] (bus_for_each_dev) from [<c064be24>] (bus_add_driver+0x158/0x214) [<c064be24>] (bus_add_driver) from [<c064e478>] (driver_register+0x78/0x110) [<c064e478>] (driver_register) from [<c0102378>] (do_one_initcall+0x8c/0x424) [<c0102378>] (do_one_initcall) from [<c1001158>] (kernel_init_freeable+0x190/0x204) [<c1001158>] (kernel_init_freeable) from [<c0ab835c>] (kernel_init+0x8/0x118) [<c0ab835c>] (kernel_init) from [<c0100114>] (ret_from_fork+0x14/0x20) Exception stack(0xee8ddfb0 to 0xee8ddff8) dfa0: 00000000 00000000 00000000 00000000 dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 dfe0: 00000000 00000000 00000000 00000000 00000013 00000000 irq event stamp: 171647 hardirqs last enabled at (171653): [<c019ec00>] vprintk_emit+0x2ac/0x2ec hardirqs last disabled at (171658): [<c019eab8>] vprintk_emit+0x164/0x2ec softirqs last enabled at (171486): [<c010174c>] __do_softirq+0x50c/0x608 softirqs last disabled at (171473): [<c0130340>] irq_exit+0x168/0x16c ---[ end trace 33117a16f066466a ]--- Then calling modetest end with segmentation fault. I'm not able to check currently if there is anything on the display because of having only remote access to the board. If this is important I will try to ask someone to help checking at the board's display at the office. Best regards
Hi Marek. On Thu, Aug 27, 2020 at 01:39:06PM +0200, Marek Szyprowski wrote: > Hi Sam, > > On 26.07.2020 22:33, Sam Ravnborg wrote: > > Prepare the tc358764 bridge driver for use in a chained setup by > > replacing direct use of drm_panel with drm_panel_bridge support. > > > > The bridge panel will use the connector type reported by the panel, > > where the connector for this driver hardcodes DRM_MODE_CONNECTOR_LVDS. > > > > The tc358764 did not any additional info the the connector so the > > connector creation is passed to the bridge panel driver. > > > > v3: > > - Merge with patch to make connector creation optional to avoid > > creating two connectors (Laurent) > > - Pass connector creation to bridge panel, as this bridge driver > > did not add any extra info to the connector. > > - Set bridge.type to DRM_MODE_CONNECTOR_LVDS. > > > > v2: > > - Use PTR_ERR_OR_ZERO() (kbuild test robot) > > > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Cc: kbuild test robot <lkp@intel.com> > > Cc: Andrzej Hajda <a.hajda@samsung.com> > > Cc: Neil Armstrong <narmstrong@baylibre.com> > > Cc: Jonas Karlman <jonas@kwiboo.se> > > Cc: Jernej Skrabec <jernej.skrabec@siol.net> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > I've noticed that this patch has been merged recently to linux-next. > Sadly it causes regression on Samsung Exynos5250-based Arndale board. Thanks for reporting this! I did not find time to focus on this bug this weekend. It is on my todo list for the coming weekend. Anything you could do to narrow down this a bit to help finding the root cause? Ideas: - Trying to find out what part of the connector that cuases troubles - Posting the full kernel boot log, to help identifying something. Bonus if we get a working and non-working log - so we can compare. - Migrate exonys to the new model That would not fix the bug, so that would be a natural step 2 - Identify the exact code-patch in the exonys driver that is used. drm_bridge_attach() is called in several places - And likely much more that I just forgot Any help would be appreciated - I did not find the culprint from first glance. I may still be obvious but I just failed to spot it. Sam > > It can be observed by the following warning during boot: > > ------------[ cut here ]------------ > WARNING: CPU: 1 PID: 1 at drivers/gpu/drm/drm_atomic_state_helper.c:494 > drm_atomic_helper_connector_duplicate_state+0x60/0x68 > Modules linked in: > CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.8.0-rc2-00501-g1644127f83bc > #1526 > Hardware name: Samsung Exynos (Flattened Device Tree) > [<c011184c>] (unwind_backtrace) from [<c010d250>] (show_stack+0x10/0x14) > [<c010d250>] (show_stack) from [<c0517ce4>] (dump_stack+0xbc/0xe8) > [<c0517ce4>] (dump_stack) from [<c01270a8>] (__warn+0xf0/0x108) > [<c01270a8>] (__warn) from [<c0127170>] (warn_slowpath_fmt+0xb0/0xb8) > [<c0127170>] (warn_slowpath_fmt) from [<c05e81f0>] > (drm_atomic_helper_connector_duplicate_state+0x60/0x68) > [<c05e81f0>] (drm_atomic_helper_connector_duplicate_state) from > [<c06014b8>] (drm_atomic_get_connector_state+0xfc/0x184) > [<c06014b8>] (drm_atomic_get_connector_state) from [<c0602238>] > (__drm_atomic_helper_set_config+0x2a0/0x368) > [<c0602238>] (__drm_atomic_helper_set_config) from [<c06183b8>] > (drm_client_modeset_commit_atomic+0x180/0x284) > [<c06183b8>] (drm_client_modeset_commit_atomic) from [<c061859c>] > (drm_client_modeset_commit_locked+0x64/0x1cc) > [<c061859c>] (drm_client_modeset_commit_locked) from [<c0618728>] > (drm_client_modeset_commit+0x24/0x40) > [<c0618728>] (drm_client_modeset_commit) from [<c05eb6b4>] > (drm_fb_helper_restore_fbdev_mode_unlocked+0x50/0x94) > [<c05eb6b4>] (drm_fb_helper_restore_fbdev_mode_unlocked) from > [<c05eb728>] (drm_fb_helper_set_par+0x30/0x5c) > [<c05eb728>] (drm_fb_helper_set_par) from [<c055dedc>] > (fbcon_init+0x5c8/0x65c) > [<c055dedc>] (fbcon_init) from [<c05a8530>] (visual_init+0xc0/0x108) > [<c05a8530>] (visual_init) from [<c05aaca4>] > (do_bind_con_driver+0x180/0x39c) > [<c05aaca4>] (do_bind_con_driver) from [<c05ab244>] > (do_take_over_console+0x140/0x1cc) > [<c05ab244>] (do_take_over_console) from [<c055ac04>] > (do_fbcon_takeover+0x84/0xe0) > [<c055ac04>] (do_fbcon_takeover) from [<c0553820>] > (register_framebuffer+0x1cc/0x2dc) > [<c0553820>] (register_framebuffer) from [<c05eb19c>] > (__drm_fb_helper_initial_config_and_unlock+0x3f0/0x5e8) > [<c05eb19c>] (__drm_fb_helper_initial_config_and_unlock) from > [<c05d941c>] (drm_kms_helper_hotplug_event+0x24/0x30) > [<c05d941c>] (drm_kms_helper_hotplug_event) from [<c0628f74>] > (exynos_dsi_host_attach+0x184/0x2d8) > [<c0628f74>] (exynos_dsi_host_attach) from [<c0634120>] > (tc358764_probe+0x13c/0x1ac) > [<c0634120>] (tc358764_probe) from [<c064cce4>] (really_probe+0x200/0x48c) > [<c064cce4>] (really_probe) from [<c064d0d8>] > (driver_probe_device+0x78/0x1fc) > [<c064d0d8>] (driver_probe_device) from [<c064d4c0>] > (device_driver_attach+0x58/0x60) > [<c064d4c0>] (device_driver_attach) from [<c064d5a4>] > (__driver_attach+0xdc/0x174) > [<c064d5a4>] (__driver_attach) from [<c064aaf0>] > (bus_for_each_dev+0x68/0xb4) > [<c064aaf0>] (bus_for_each_dev) from [<c064be24>] > (bus_add_driver+0x158/0x214) > [<c064be24>] (bus_add_driver) from [<c064e478>] (driver_register+0x78/0x110) > [<c064e478>] (driver_register) from [<c0102378>] > (do_one_initcall+0x8c/0x424) > [<c0102378>] (do_one_initcall) from [<c1001158>] > (kernel_init_freeable+0x190/0x204) > [<c1001158>] (kernel_init_freeable) from [<c0ab835c>] > (kernel_init+0x8/0x118) > [<c0ab835c>] (kernel_init) from [<c0100114>] (ret_from_fork+0x14/0x20) > Exception stack(0xee8ddfb0 to 0xee8ddff8) > dfa0: 00000000 00000000 00000000 > 00000000 > dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 > 00000000 > dfe0: 00000000 00000000 00000000 00000000 00000013 00000000 > irq event stamp: 171647 > hardirqs last enabled at (171653): [<c019ec00>] vprintk_emit+0x2ac/0x2ec > hardirqs last disabled at (171658): [<c019eab8>] vprintk_emit+0x164/0x2ec > softirqs last enabled at (171486): [<c010174c>] __do_softirq+0x50c/0x608 > softirqs last disabled at (171473): [<c0130340>] irq_exit+0x168/0x16c > ---[ end trace 33117a16f066466a ]--- > > Then calling modetest end with segmentation fault. I'm not able to check > currently if there is anything on the display because of having only > remote access to the board. If this is important I will try to ask > someone to help checking at the board's display at the office. > > Best regards > -- > Marek Szyprowski, PhD > Samsung R&D Institute Poland
Hi Sam, On 30.08.2020 22:42, Sam Ravnborg wrote: > Hi Marek. > > On Thu, Aug 27, 2020 at 01:39:06PM +0200, Marek Szyprowski wrote: >> Hi Sam, >> >> On 26.07.2020 22:33, Sam Ravnborg wrote: >>> Prepare the tc358764 bridge driver for use in a chained setup by >>> replacing direct use of drm_panel with drm_panel_bridge support. >>> >>> The bridge panel will use the connector type reported by the panel, >>> where the connector for this driver hardcodes DRM_MODE_CONNECTOR_LVDS. >>> >>> The tc358764 did not any additional info the the connector so the >>> connector creation is passed to the bridge panel driver. >>> >>> v3: >>> - Merge with patch to make connector creation optional to avoid >>> creating two connectors (Laurent) >>> - Pass connector creation to bridge panel, as this bridge driver >>> did not add any extra info to the connector. >>> - Set bridge.type to DRM_MODE_CONNECTOR_LVDS. >>> >>> v2: >>> - Use PTR_ERR_OR_ZERO() (kbuild test robot) >>> >>> Signed-off-by: Sam Ravnborg <sam@ravnborg.org> >>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> Cc: kbuild test robot <lkp@intel.com> >>> Cc: Andrzej Hajda <a.hajda@samsung.com> >>> Cc: Neil Armstrong <narmstrong@baylibre.com> >>> Cc: Jonas Karlman <jonas@kwiboo.se> >>> Cc: Jernej Skrabec <jernej.skrabec@siol.net> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> I've noticed that this patch has been merged recently to linux-next. >> Sadly it causes regression on Samsung Exynos5250-based Arndale board. > Thanks for reporting this! > > I did not find time to focus on this bug this weekend. It is on my todo > list for the coming weekend. > > Anything you could do to narrow down this a bit to help finding the root > cause? > > Ideas: > - Trying to find out what part of the connector that cuases troubles > - Posting the full kernel boot log, to help identifying something. > Bonus if we get a working and non-working log - so we can compare. > - Migrate exonys to the new model > That would not fix the bug, so that would be a natural step 2 > - Identify the exact code-patch in the exonys driver that is used. > drm_bridge_attach() is called in several places > - And likely much more that I just forgot dmesg WARN suggest there is no connector registered and it can be true. I guess drm_panel_bridge does not support dynamic connector registration, so if the bridge appears after drm dev is created it will not be properly registered. I will try look closer at the patch but I guess the above can be the main reason. Regards Andrzej > > Any help would be appreciated - I did not find the culprint from first > glance. I may still be obvious but I just failed to spot it. > > Sam > >> It can be observed by the following warning during boot: >> >> ------------[ cut here ]------------ >> WARNING: CPU: 1 PID: 1 at drivers/gpu/drm/drm_atomic_state_helper.c:494 >> drm_atomic_helper_connector_duplicate_state+0x60/0x68 >> Modules linked in: >> CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.8.0-rc2-00501-g1644127f83bc >> #1526 >> Hardware name: Samsung Exynos (Flattened Device Tree) >> [<c011184c>] (unwind_backtrace) from [<c010d250>] (show_stack+0x10/0x14) >> [<c010d250>] (show_stack) from [<c0517ce4>] (dump_stack+0xbc/0xe8) >> [<c0517ce4>] (dump_stack) from [<c01270a8>] (__warn+0xf0/0x108) >> [<c01270a8>] (__warn) from [<c0127170>] (warn_slowpath_fmt+0xb0/0xb8) >> [<c0127170>] (warn_slowpath_fmt) from [<c05e81f0>] >> (drm_atomic_helper_connector_duplicate_state+0x60/0x68) >> [<c05e81f0>] (drm_atomic_helper_connector_duplicate_state) from >> [<c06014b8>] (drm_atomic_get_connector_state+0xfc/0x184) >> [<c06014b8>] (drm_atomic_get_connector_state) from [<c0602238>] >> (__drm_atomic_helper_set_config+0x2a0/0x368) >> [<c0602238>] (__drm_atomic_helper_set_config) from [<c06183b8>] >> (drm_client_modeset_commit_atomic+0x180/0x284) >> [<c06183b8>] (drm_client_modeset_commit_atomic) from [<c061859c>] >> (drm_client_modeset_commit_locked+0x64/0x1cc) >> [<c061859c>] (drm_client_modeset_commit_locked) from [<c0618728>] >> (drm_client_modeset_commit+0x24/0x40) >> [<c0618728>] (drm_client_modeset_commit) from [<c05eb6b4>] >> (drm_fb_helper_restore_fbdev_mode_unlocked+0x50/0x94) >> [<c05eb6b4>] (drm_fb_helper_restore_fbdev_mode_unlocked) from >> [<c05eb728>] (drm_fb_helper_set_par+0x30/0x5c) >> [<c05eb728>] (drm_fb_helper_set_par) from [<c055dedc>] >> (fbcon_init+0x5c8/0x65c) >> [<c055dedc>] (fbcon_init) from [<c05a8530>] (visual_init+0xc0/0x108) >> [<c05a8530>] (visual_init) from [<c05aaca4>] >> (do_bind_con_driver+0x180/0x39c) >> [<c05aaca4>] (do_bind_con_driver) from [<c05ab244>] >> (do_take_over_console+0x140/0x1cc) >> [<c05ab244>] (do_take_over_console) from [<c055ac04>] >> (do_fbcon_takeover+0x84/0xe0) >> [<c055ac04>] (do_fbcon_takeover) from [<c0553820>] >> (register_framebuffer+0x1cc/0x2dc) >> [<c0553820>] (register_framebuffer) from [<c05eb19c>] >> (__drm_fb_helper_initial_config_and_unlock+0x3f0/0x5e8) >> [<c05eb19c>] (__drm_fb_helper_initial_config_and_unlock) from >> [<c05d941c>] (drm_kms_helper_hotplug_event+0x24/0x30) >> [<c05d941c>] (drm_kms_helper_hotplug_event) from [<c0628f74>] >> (exynos_dsi_host_attach+0x184/0x2d8) >> [<c0628f74>] (exynos_dsi_host_attach) from [<c0634120>] >> (tc358764_probe+0x13c/0x1ac) >> [<c0634120>] (tc358764_probe) from [<c064cce4>] (really_probe+0x200/0x48c) >> [<c064cce4>] (really_probe) from [<c064d0d8>] >> (driver_probe_device+0x78/0x1fc) >> [<c064d0d8>] (driver_probe_device) from [<c064d4c0>] >> (device_driver_attach+0x58/0x60) >> [<c064d4c0>] (device_driver_attach) from [<c064d5a4>] >> (__driver_attach+0xdc/0x174) >> [<c064d5a4>] (__driver_attach) from [<c064aaf0>] >> (bus_for_each_dev+0x68/0xb4) >> [<c064aaf0>] (bus_for_each_dev) from [<c064be24>] >> (bus_add_driver+0x158/0x214) >> [<c064be24>] (bus_add_driver) from [<c064e478>] (driver_register+0x78/0x110) >> [<c064e478>] (driver_register) from [<c0102378>] >> (do_one_initcall+0x8c/0x424) >> [<c0102378>] (do_one_initcall) from [<c1001158>] >> (kernel_init_freeable+0x190/0x204) >> [<c1001158>] (kernel_init_freeable) from [<c0ab835c>] >> (kernel_init+0x8/0x118) >> [<c0ab835c>] (kernel_init) from [<c0100114>] (ret_from_fork+0x14/0x20) >> Exception stack(0xee8ddfb0 to 0xee8ddff8) >> dfa0: 00000000 00000000 00000000 >> 00000000 >> dfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 >> 00000000 >> dfe0: 00000000 00000000 00000000 00000000 00000013 00000000 >> irq event stamp: 171647 >> hardirqs last enabled at (171653): [<c019ec00>] vprintk_emit+0x2ac/0x2ec >> hardirqs last disabled at (171658): [<c019eab8>] vprintk_emit+0x164/0x2ec >> softirqs last enabled at (171486): [<c010174c>] __do_softirq+0x50c/0x608 >> softirqs last disabled at (171473): [<c0130340>] irq_exit+0x168/0x16c >> ---[ end trace 33117a16f066466a ]--- >> >> Then calling modetest end with segmentation fault. I'm not able to check >> currently if there is anything on the display because of having only >> remote access to the board. If this is important I will try to ask >> someone to help checking at the board's display at the office. >> >> Best regards >> -- >> Marek Szyprowski, PhD >> Samsung R&D Institute Poland > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://protect2.fireeye.com/url?k=4965500c-14b1ec64-4964db43-0cc47a3356b2-f40ef8e76ffb9eeb&q=1&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel >
On 26.07.2020 22:33, Sam Ravnborg wrote: > Prepare the tc358764 bridge driver for use in a chained setup by > replacing direct use of drm_panel with drm_panel_bridge support. > > The bridge panel will use the connector type reported by the panel, > where the connector for this driver hardcodes DRM_MODE_CONNECTOR_LVDS. > > The tc358764 did not any additional info the the connector so the > connector creation is passed to the bridge panel driver. > > v3: > - Merge with patch to make connector creation optional to avoid > creating two connectors (Laurent) > - Pass connector creation to bridge panel, as this bridge driver > did not add any extra info to the connector. > - Set bridge.type to DRM_MODE_CONNECTOR_LVDS. > > v2: > - Use PTR_ERR_OR_ZERO() (kbuild test robot) > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: kbuild test robot <lkp@intel.com> > Cc: Andrzej Hajda <a.hajda@samsung.com> > Cc: Neil Armstrong <narmstrong@baylibre.com> > Cc: Jonas Karlman <jonas@kwiboo.se> > Cc: Jernej Skrabec <jernej.skrabec@siol.net> > --- > drivers/gpu/drm/bridge/tc358764.c | 107 +++++------------------------- > 1 file changed, 16 insertions(+), 91 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c > index a277739fab58..fdde4cfdc724 100644 > --- a/drivers/gpu/drm/bridge/tc358764.c > +++ b/drivers/gpu/drm/bridge/tc358764.c > @@ -153,10 +153,9 @@ static const char * const tc358764_supplies[] = { > struct tc358764 { > struct device *dev; > struct drm_bridge bridge; > - struct drm_connector connector; > struct regulator_bulk_data supplies[ARRAY_SIZE(tc358764_supplies)]; > struct gpio_desc *gpio_reset; > - struct drm_panel *panel; > + struct drm_bridge *panel_bridge; > int error; > }; > > @@ -210,12 +209,6 @@ static inline struct tc358764 *bridge_to_tc358764(struct drm_bridge *bridge) > return container_of(bridge, struct tc358764, bridge); > } > > -static inline > -struct tc358764 *connector_to_tc358764(struct drm_connector *connector) > -{ > - return container_of(connector, struct tc358764, connector); > -} > - > static int tc358764_init(struct tc358764 *ctx) > { > u32 v = 0; > @@ -278,43 +271,11 @@ static void tc358764_reset(struct tc358764 *ctx) > usleep_range(1000, 2000); > } > > -static int tc358764_get_modes(struct drm_connector *connector) > -{ > - struct tc358764 *ctx = connector_to_tc358764(connector); > - > - return drm_panel_get_modes(ctx->panel, connector); > -} > - > -static const > -struct drm_connector_helper_funcs tc358764_connector_helper_funcs = { > - .get_modes = tc358764_get_modes, > -}; > - > -static const struct drm_connector_funcs tc358764_connector_funcs = { > - .fill_modes = drm_helper_probe_single_connector_modes, > - .destroy = drm_connector_cleanup, > - .reset = drm_atomic_helper_connector_reset, > - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > -}; > - > -static void tc358764_disable(struct drm_bridge *bridge) > -{ > - struct tc358764 *ctx = bridge_to_tc358764(bridge); > - int ret = drm_panel_disable(bridge_to_tc358764(bridge)->panel); > - > - if (ret < 0) > - dev_err(ctx->dev, "error disabling panel (%d)\n", ret); > -} > - > static void tc358764_post_disable(struct drm_bridge *bridge) > { > struct tc358764 *ctx = bridge_to_tc358764(bridge); > int ret; > > - ret = drm_panel_unprepare(ctx->panel); > - if (ret < 0) > - dev_err(ctx->dev, "error unpreparing panel (%d)\n", ret); Using this bridge_panel thing you reverse order of hw initialization/de-initialization, this is incorrect. For example: - panel_unprepare should be called before tc35* turn off, - panel_prepare should be called after tc35* on. This is why I avoid the whole "bridge chaining" - it enforces ridiculous order of initialization. > tc358764_reset(ctx); > usleep_range(10000, 15000); > ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies); > @@ -335,70 +296,28 @@ static void tc358764_pre_enable(struct drm_bridge *bridge) > ret = tc358764_init(ctx); > if (ret < 0) > dev_err(ctx->dev, "error initializing bridge (%d)\n", ret); > - ret = drm_panel_prepare(ctx->panel); > - if (ret < 0) > - dev_err(ctx->dev, "error preparing panel (%d)\n", ret); > -} > - > -static void tc358764_enable(struct drm_bridge *bridge) > -{ > - struct tc358764 *ctx = bridge_to_tc358764(bridge); > - int ret = drm_panel_enable(ctx->panel); > - > - if (ret < 0) > - dev_err(ctx->dev, "error enabling panel (%d)\n", ret); > } > > static int tc358764_attach(struct drm_bridge *bridge, > enum drm_bridge_attach_flags flags) > -{ > - struct tc358764 *ctx = bridge_to_tc358764(bridge); > - struct drm_device *drm = bridge->dev; > - int ret; > - > - if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { > - DRM_ERROR("Fix bridge driver to make connector optional!"); > - return -EINVAL; > - } > - > - ctx->connector.polled = DRM_CONNECTOR_POLL_HPD; > - ret = drm_connector_init(drm, &ctx->connector, > - &tc358764_connector_funcs, > - DRM_MODE_CONNECTOR_LVDS); > - if (ret) { > - DRM_ERROR("Failed to initialize connector\n"); > - return ret; > - } > - > - drm_connector_helper_add(&ctx->connector, > - &tc358764_connector_helper_funcs); > - drm_connector_attach_encoder(&ctx->connector, bridge->encoder); > - drm_panel_attach(ctx->panel, &ctx->connector); > - ctx->connector.funcs->reset(&ctx->connector); I guess lack of calling .reset here is direct cause of WARN reported by Marek. Summarizing my findings: 1. drm_panel_bridge does not fit to this scenario - it relays on 'bridge chaining" which has crazy assumption that order of hw initalization in the display chain follows the same fixed order of calls for all hw. 2. tc35* bridge allocates/deallocates connector dynamically - to safely handle drivers load/unload, and to avoid multiple deferred probe issues , drm_panel_bridge does not support it. This and previous patch violates both points. Regards Andrzej > - > - return 0; > -} > - > -static void tc358764_detach(struct drm_bridge *bridge) > { > struct tc358764 *ctx = bridge_to_tc358764(bridge); > > - drm_panel_detach(ctx->panel); > - ctx->panel = NULL; > + return drm_bridge_attach(bridge->encoder, ctx->panel_bridge, > + bridge, flags); > } > > static const struct drm_bridge_funcs tc358764_bridge_funcs = { > - .disable = tc358764_disable, > .post_disable = tc358764_post_disable, > - .enable = tc358764_enable, > .pre_enable = tc358764_pre_enable, > .attach = tc358764_attach, > - .detach = tc358764_detach, > }; > > static int tc358764_parse_dt(struct tc358764 *ctx) > { > + struct drm_bridge *panel_bridge; > struct device *dev = ctx->dev; > + struct drm_panel *panel; > int ret; > > ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); > @@ -407,12 +326,17 @@ static int tc358764_parse_dt(struct tc358764 *ctx) > return PTR_ERR(ctx->gpio_reset); > } > > - ret = drm_of_find_panel_or_bridge(ctx->dev->of_node, 1, 0, &ctx->panel, > - NULL); > - if (ret && ret != -EPROBE_DEFER) > - dev_err(dev, "cannot find panel (%d)\n", ret); > + ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL); > + if (ret) > + return ret; > > - return ret; > + panel_bridge = devm_drm_panel_bridge_add(dev, panel); > + > + if (IS_ERR(panel_bridge)) > + return PTR_ERR(panel_bridge); > + > + ctx->panel_bridge = panel_bridge; > + return 0; > } > > static int tc358764_configure_regulators(struct tc358764 *ctx) > @@ -458,6 +382,7 @@ static int tc358764_probe(struct mipi_dsi_device *dsi) > return ret; > > ctx->bridge.funcs = &tc358764_bridge_funcs; > + ctx->bridge.type = DRM_MODE_CONNECTOR_LVDS; > ctx->bridge.of_node = dev->of_node; > > drm_bridge_add(&ctx->bridge);
Hi Andrzej, On Thu, Sep 03, 2020 at 11:40:58AM +0200, Andrzej Hajda wrote: > On 26.07.2020 22:33, Sam Ravnborg wrote: > > Prepare the tc358764 bridge driver for use in a chained setup by > > replacing direct use of drm_panel with drm_panel_bridge support. > > > > The bridge panel will use the connector type reported by the panel, > > where the connector for this driver hardcodes DRM_MODE_CONNECTOR_LVDS. > > > > The tc358764 did not any additional info the the connector so the > > connector creation is passed to the bridge panel driver. > > > > v3: > > - Merge with patch to make connector creation optional to avoid > > creating two connectors (Laurent) > > - Pass connector creation to bridge panel, as this bridge driver > > did not add any extra info to the connector. > > - Set bridge.type to DRM_MODE_CONNECTOR_LVDS. > > > > v2: > > - Use PTR_ERR_OR_ZERO() (kbuild test robot) > > > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Cc: kbuild test robot <lkp@intel.com> > > Cc: Andrzej Hajda <a.hajda@samsung.com> > > Cc: Neil Armstrong <narmstrong@baylibre.com> > > Cc: Jonas Karlman <jonas@kwiboo.se> > > Cc: Jernej Skrabec <jernej.skrabec@siol.net> > > --- > > drivers/gpu/drm/bridge/tc358764.c | 107 +++++------------------------- > > 1 file changed, 16 insertions(+), 91 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c > > index a277739fab58..fdde4cfdc724 100644 > > --- a/drivers/gpu/drm/bridge/tc358764.c > > +++ b/drivers/gpu/drm/bridge/tc358764.c > > @@ -153,10 +153,9 @@ static const char * const tc358764_supplies[] = { > > struct tc358764 { > > struct device *dev; > > struct drm_bridge bridge; > > - struct drm_connector connector; > > struct regulator_bulk_data supplies[ARRAY_SIZE(tc358764_supplies)]; > > struct gpio_desc *gpio_reset; > > - struct drm_panel *panel; > > + struct drm_bridge *panel_bridge; > > int error; > > }; > > > > @@ -210,12 +209,6 @@ static inline struct tc358764 *bridge_to_tc358764(struct drm_bridge *bridge) > > return container_of(bridge, struct tc358764, bridge); > > } > > > > -static inline > > -struct tc358764 *connector_to_tc358764(struct drm_connector *connector) > > -{ > > - return container_of(connector, struct tc358764, connector); > > -} > > - > > static int tc358764_init(struct tc358764 *ctx) > > { > > u32 v = 0; > > @@ -278,43 +271,11 @@ static void tc358764_reset(struct tc358764 *ctx) > > usleep_range(1000, 2000); > > } > > > > -static int tc358764_get_modes(struct drm_connector *connector) > > -{ > > - struct tc358764 *ctx = connector_to_tc358764(connector); > > - > > - return drm_panel_get_modes(ctx->panel, connector); > > -} > > - > > -static const > > -struct drm_connector_helper_funcs tc358764_connector_helper_funcs = { > > - .get_modes = tc358764_get_modes, > > -}; > > - > > -static const struct drm_connector_funcs tc358764_connector_funcs = { > > - .fill_modes = drm_helper_probe_single_connector_modes, > > - .destroy = drm_connector_cleanup, > > - .reset = drm_atomic_helper_connector_reset, > > - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > > - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > > -}; > > - > > -static void tc358764_disable(struct drm_bridge *bridge) > > -{ > > - struct tc358764 *ctx = bridge_to_tc358764(bridge); > > - int ret = drm_panel_disable(bridge_to_tc358764(bridge)->panel); > > - > > - if (ret < 0) > > - dev_err(ctx->dev, "error disabling panel (%d)\n", ret); > > -} > > - > > static void tc358764_post_disable(struct drm_bridge *bridge) > > { > > struct tc358764 *ctx = bridge_to_tc358764(bridge); > > int ret; > > > > - ret = drm_panel_unprepare(ctx->panel); > > - if (ret < 0) > > - dev_err(ctx->dev, "error unpreparing panel (%d)\n", ret); > > > Using this bridge_panel thing you reverse order of hw > initialization/de-initialization, this is incorrect. > > For example: > > - panel_unprepare should be called before tc35* turn off, > > - panel_prepare should be called after tc35* on. > > This is why I avoid the whole "bridge chaining" - it enforces ridiculous > order of initialization. > > > > tc358764_reset(ctx); > > usleep_range(10000, 15000); > > ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies); > > @@ -335,70 +296,28 @@ static void tc358764_pre_enable(struct drm_bridge *bridge) > > ret = tc358764_init(ctx); > > if (ret < 0) > > dev_err(ctx->dev, "error initializing bridge (%d)\n", ret); > > - ret = drm_panel_prepare(ctx->panel); > > - if (ret < 0) > > - dev_err(ctx->dev, "error preparing panel (%d)\n", ret); > > -} > > - > > -static void tc358764_enable(struct drm_bridge *bridge) > > -{ > > - struct tc358764 *ctx = bridge_to_tc358764(bridge); > > - int ret = drm_panel_enable(ctx->panel); > > - > > - if (ret < 0) > > - dev_err(ctx->dev, "error enabling panel (%d)\n", ret); > > } > > > > static int tc358764_attach(struct drm_bridge *bridge, > > enum drm_bridge_attach_flags flags) > > -{ > > - struct tc358764 *ctx = bridge_to_tc358764(bridge); > > - struct drm_device *drm = bridge->dev; > > - int ret; > > - > > - if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { > > - DRM_ERROR("Fix bridge driver to make connector optional!"); > > - return -EINVAL; > > - } > > - > > - ctx->connector.polled = DRM_CONNECTOR_POLL_HPD; > > - ret = drm_connector_init(drm, &ctx->connector, > > - &tc358764_connector_funcs, > > - DRM_MODE_CONNECTOR_LVDS); > > - if (ret) { > > - DRM_ERROR("Failed to initialize connector\n"); > > - return ret; > > - } > > - > > - drm_connector_helper_add(&ctx->connector, > > - &tc358764_connector_helper_funcs); > > - drm_connector_attach_encoder(&ctx->connector, bridge->encoder); > > - drm_panel_attach(ctx->panel, &ctx->connector); > > - ctx->connector.funcs->reset(&ctx->connector); > > > I guess lack of calling .reset here is direct cause of WARN reported by > Marek. > > > Summarizing my findings: > > 1. drm_panel_bridge does not fit to this scenario - it relays on 'bridge > chaining" which has crazy assumption that order of hw initalization in > the display chain follows the same fixed order of calls for all hw. This would need to be addressed in the bridge core. I don't want to go back to manual chaining of operations, that opens the door to creating incompatibilities between bridges and display controllers. The pre/post enable/disable operations probably need to be better defined, and if a sink requires a smaller granularity, then new operations need to be added. > 2. tc35* bridge allocates/deallocates connector dynamically - to safely > handle drivers load/unload, and to avoid multiple deferred probe issues > , drm_panel_bridge does not support it. > > This and previous patch violates both points. > > > - > > - return 0; > > -} > > - > > -static void tc358764_detach(struct drm_bridge *bridge) > > { > > struct tc358764 *ctx = bridge_to_tc358764(bridge); > > > > - drm_panel_detach(ctx->panel); > > - ctx->panel = NULL; > > + return drm_bridge_attach(bridge->encoder, ctx->panel_bridge, > > + bridge, flags); > > } > > > > static const struct drm_bridge_funcs tc358764_bridge_funcs = { > > - .disable = tc358764_disable, > > .post_disable = tc358764_post_disable, > > - .enable = tc358764_enable, > > .pre_enable = tc358764_pre_enable, > > .attach = tc358764_attach, > > - .detach = tc358764_detach, > > }; > > > > static int tc358764_parse_dt(struct tc358764 *ctx) > > { > > + struct drm_bridge *panel_bridge; > > struct device *dev = ctx->dev; > > + struct drm_panel *panel; > > int ret; > > > > ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); > > @@ -407,12 +326,17 @@ static int tc358764_parse_dt(struct tc358764 *ctx) > > return PTR_ERR(ctx->gpio_reset); > > } > > > > - ret = drm_of_find_panel_or_bridge(ctx->dev->of_node, 1, 0, &ctx->panel, > > - NULL); > > - if (ret && ret != -EPROBE_DEFER) > > - dev_err(dev, "cannot find panel (%d)\n", ret); > > + ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL); > > + if (ret) > > + return ret; > > > > - return ret; > > + panel_bridge = devm_drm_panel_bridge_add(dev, panel); > > + > > + if (IS_ERR(panel_bridge)) > > + return PTR_ERR(panel_bridge); > > + > > + ctx->panel_bridge = panel_bridge; > > + return 0; > > } > > > > static int tc358764_configure_regulators(struct tc358764 *ctx) > > @@ -458,6 +382,7 @@ static int tc358764_probe(struct mipi_dsi_device *dsi) > > return ret; > > > > ctx->bridge.funcs = &tc358764_bridge_funcs; > > + ctx->bridge.type = DRM_MODE_CONNECTOR_LVDS; > > ctx->bridge.of_node = dev->of_node; > > > > drm_bridge_add(&ctx->bridge);
Hi Laurent, On 03.09.2020 11:59, Laurent Pinchart wrote: > Hi Andrzej, > > On Thu, Sep 03, 2020 at 11:40:58AM +0200, Andrzej Hajda wrote: >> On 26.07.2020 22:33, Sam Ravnborg wrote: >>> Prepare the tc358764 bridge driver for use in a chained setup by >>> replacing direct use of drm_panel with drm_panel_bridge support. >>> >>> The bridge panel will use the connector type reported by the panel, >>> where the connector for this driver hardcodes DRM_MODE_CONNECTOR_LVDS. >>> >>> The tc358764 did not any additional info the the connector so the >>> connector creation is passed to the bridge panel driver. >>> >>> v3: >>> - Merge with patch to make connector creation optional to avoid >>> creating two connectors (Laurent) >>> - Pass connector creation to bridge panel, as this bridge driver >>> did not add any extra info to the connector. >>> - Set bridge.type to DRM_MODE_CONNECTOR_LVDS. >>> >>> v2: >>> - Use PTR_ERR_OR_ZERO() (kbuild test robot) >>> >>> Signed-off-by: Sam Ravnborg <sam@ravnborg.org> >>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> Cc: kbuild test robot <lkp@intel.com> >>> Cc: Andrzej Hajda <a.hajda@samsung.com> >>> Cc: Neil Armstrong <narmstrong@baylibre.com> >>> Cc: Jonas Karlman <jonas@kwiboo.se> >>> Cc: Jernej Skrabec <jernej.skrabec@siol.net> >>> --- >>> drivers/gpu/drm/bridge/tc358764.c | 107 +++++------------------------- >>> 1 file changed, 16 insertions(+), 91 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c >>> index a277739fab58..fdde4cfdc724 100644 >>> --- a/drivers/gpu/drm/bridge/tc358764.c >>> +++ b/drivers/gpu/drm/bridge/tc358764.c >>> @@ -153,10 +153,9 @@ static const char * const tc358764_supplies[] = { >>> struct tc358764 { >>> struct device *dev; >>> struct drm_bridge bridge; >>> - struct drm_connector connector; >>> struct regulator_bulk_data supplies[ARRAY_SIZE(tc358764_supplies)]; >>> struct gpio_desc *gpio_reset; >>> - struct drm_panel *panel; >>> + struct drm_bridge *panel_bridge; >>> int error; >>> }; >>> >>> @@ -210,12 +209,6 @@ static inline struct tc358764 *bridge_to_tc358764(struct drm_bridge *bridge) >>> return container_of(bridge, struct tc358764, bridge); >>> } >>> >>> -static inline >>> -struct tc358764 *connector_to_tc358764(struct drm_connector *connector) >>> -{ >>> - return container_of(connector, struct tc358764, connector); >>> -} >>> - >>> static int tc358764_init(struct tc358764 *ctx) >>> { >>> u32 v = 0; >>> @@ -278,43 +271,11 @@ static void tc358764_reset(struct tc358764 *ctx) >>> usleep_range(1000, 2000); >>> } >>> >>> -static int tc358764_get_modes(struct drm_connector *connector) >>> -{ >>> - struct tc358764 *ctx = connector_to_tc358764(connector); >>> - >>> - return drm_panel_get_modes(ctx->panel, connector); >>> -} >>> - >>> -static const >>> -struct drm_connector_helper_funcs tc358764_connector_helper_funcs = { >>> - .get_modes = tc358764_get_modes, >>> -}; >>> - >>> -static const struct drm_connector_funcs tc358764_connector_funcs = { >>> - .fill_modes = drm_helper_probe_single_connector_modes, >>> - .destroy = drm_connector_cleanup, >>> - .reset = drm_atomic_helper_connector_reset, >>> - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, >>> - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, >>> -}; >>> - >>> -static void tc358764_disable(struct drm_bridge *bridge) >>> -{ >>> - struct tc358764 *ctx = bridge_to_tc358764(bridge); >>> - int ret = drm_panel_disable(bridge_to_tc358764(bridge)->panel); >>> - >>> - if (ret < 0) >>> - dev_err(ctx->dev, "error disabling panel (%d)\n", ret); >>> -} >>> - >>> static void tc358764_post_disable(struct drm_bridge *bridge) >>> { >>> struct tc358764 *ctx = bridge_to_tc358764(bridge); >>> int ret; >>> >>> - ret = drm_panel_unprepare(ctx->panel); >>> - if (ret < 0) >>> - dev_err(ctx->dev, "error unpreparing panel (%d)\n", ret); >> >> Using this bridge_panel thing you reverse order of hw >> initialization/de-initialization, this is incorrect. >> >> For example: >> >> - panel_unprepare should be called before tc35* turn off, >> >> - panel_prepare should be called after tc35* on. >> >> This is why I avoid the whole "bridge chaining" - it enforces ridiculous >> order of initialization. >> >> >>> tc358764_reset(ctx); >>> usleep_range(10000, 15000); >>> ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies); >>> @@ -335,70 +296,28 @@ static void tc358764_pre_enable(struct drm_bridge *bridge) >>> ret = tc358764_init(ctx); >>> if (ret < 0) >>> dev_err(ctx->dev, "error initializing bridge (%d)\n", ret); >>> - ret = drm_panel_prepare(ctx->panel); >>> - if (ret < 0) >>> - dev_err(ctx->dev, "error preparing panel (%d)\n", ret); >>> -} >>> - >>> -static void tc358764_enable(struct drm_bridge *bridge) >>> -{ >>> - struct tc358764 *ctx = bridge_to_tc358764(bridge); >>> - int ret = drm_panel_enable(ctx->panel); >>> - >>> - if (ret < 0) >>> - dev_err(ctx->dev, "error enabling panel (%d)\n", ret); >>> } >>> >>> static int tc358764_attach(struct drm_bridge *bridge, >>> enum drm_bridge_attach_flags flags) >>> -{ >>> - struct tc358764 *ctx = bridge_to_tc358764(bridge); >>> - struct drm_device *drm = bridge->dev; >>> - int ret; >>> - >>> - if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { >>> - DRM_ERROR("Fix bridge driver to make connector optional!"); >>> - return -EINVAL; >>> - } >>> - >>> - ctx->connector.polled = DRM_CONNECTOR_POLL_HPD; >>> - ret = drm_connector_init(drm, &ctx->connector, >>> - &tc358764_connector_funcs, >>> - DRM_MODE_CONNECTOR_LVDS); >>> - if (ret) { >>> - DRM_ERROR("Failed to initialize connector\n"); >>> - return ret; >>> - } >>> - >>> - drm_connector_helper_add(&ctx->connector, >>> - &tc358764_connector_helper_funcs); >>> - drm_connector_attach_encoder(&ctx->connector, bridge->encoder); >>> - drm_panel_attach(ctx->panel, &ctx->connector); >>> - ctx->connector.funcs->reset(&ctx->connector); >> >> I guess lack of calling .reset here is direct cause of WARN reported by >> Marek. >> >> >> Summarizing my findings: >> >> 1. drm_panel_bridge does not fit to this scenario - it relays on 'bridge >> chaining" which has crazy assumption that order of hw initalization in >> the display chain follows the same fixed order of calls for all hw. > This would need to be addressed in the bridge core. I don't want to go > back to manual chaining of operations, that opens the door to creating > incompatibilities between bridges and display controllers. The pre/post > enable/disable operations probably need to be better defined, and if a > sink requires a smaller granularity, then new operations need to be > added. Bigger granularity of operations is source of incompatibilities. We have already multiple issues with only two operations - pre_enable, post_enable, developers are confused what put where, especially if they can test bridge driver only with only fixed chain determined by the platform they are working on. Adding new operations will make things worse. On the other hand explicit calling ops of downstream device has following advantages: - we see explicitly how the bridge and its sink is initialized, it is even easier to compare it with docs, - in case the stream is split or duplicated to two or more sinks, and/or eventually joined then later, it is much easier and straightforward to program it with explicit ops. With bridge chaining it is impossible without workarounds - all the cases with dual DSI etc. Regards Andrzej >> 2. tc35* bridge allocates/deallocates connector dynamically - to safely >> handle drivers load/unload, and to avoid multiple deferred probe issues >> , drm_panel_bridge does not support it. >> >> This and previous patch violates both points. >> >>> - >>> - return 0; >>> -} >>> - >>> -static void tc358764_detach(struct drm_bridge *bridge) >>> { >>> struct tc358764 *ctx = bridge_to_tc358764(bridge); >>> >>> - drm_panel_detach(ctx->panel); >>> - ctx->panel = NULL; >>> + return drm_bridge_attach(bridge->encoder, ctx->panel_bridge, >>> + bridge, flags); >>> } >>> >>> static const struct drm_bridge_funcs tc358764_bridge_funcs = { >>> - .disable = tc358764_disable, >>> .post_disable = tc358764_post_disable, >>> - .enable = tc358764_enable, >>> .pre_enable = tc358764_pre_enable, >>> .attach = tc358764_attach, >>> - .detach = tc358764_detach, >>> }; >>> >>> static int tc358764_parse_dt(struct tc358764 *ctx) >>> { >>> + struct drm_bridge *panel_bridge; >>> struct device *dev = ctx->dev; >>> + struct drm_panel *panel; >>> int ret; >>> >>> ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); >>> @@ -407,12 +326,17 @@ static int tc358764_parse_dt(struct tc358764 *ctx) >>> return PTR_ERR(ctx->gpio_reset); >>> } >>> >>> - ret = drm_of_find_panel_or_bridge(ctx->dev->of_node, 1, 0, &ctx->panel, >>> - NULL); >>> - if (ret && ret != -EPROBE_DEFER) >>> - dev_err(dev, "cannot find panel (%d)\n", ret); >>> + ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL); >>> + if (ret) >>> + return ret; >>> >>> - return ret; >>> + panel_bridge = devm_drm_panel_bridge_add(dev, panel); >>> + >>> + if (IS_ERR(panel_bridge)) >>> + return PTR_ERR(panel_bridge); >>> + >>> + ctx->panel_bridge = panel_bridge; >>> + return 0; >>> } >>> >>> static int tc358764_configure_regulators(struct tc358764 *ctx) >>> @@ -458,6 +382,7 @@ static int tc358764_probe(struct mipi_dsi_device *dsi) >>> return ret; >>> >>> ctx->bridge.funcs = &tc358764_bridge_funcs; >>> + ctx->bridge.type = DRM_MODE_CONNECTOR_LVDS; >>> ctx->bridge.of_node = dev->of_node; >>> >>> drm_bridge_add(&ctx->bridge);
diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c index a277739fab58..fdde4cfdc724 100644 --- a/drivers/gpu/drm/bridge/tc358764.c +++ b/drivers/gpu/drm/bridge/tc358764.c @@ -153,10 +153,9 @@ static const char * const tc358764_supplies[] = { struct tc358764 { struct device *dev; struct drm_bridge bridge; - struct drm_connector connector; struct regulator_bulk_data supplies[ARRAY_SIZE(tc358764_supplies)]; struct gpio_desc *gpio_reset; - struct drm_panel *panel; + struct drm_bridge *panel_bridge; int error; }; @@ -210,12 +209,6 @@ static inline struct tc358764 *bridge_to_tc358764(struct drm_bridge *bridge) return container_of(bridge, struct tc358764, bridge); } -static inline -struct tc358764 *connector_to_tc358764(struct drm_connector *connector) -{ - return container_of(connector, struct tc358764, connector); -} - static int tc358764_init(struct tc358764 *ctx) { u32 v = 0; @@ -278,43 +271,11 @@ static void tc358764_reset(struct tc358764 *ctx) usleep_range(1000, 2000); } -static int tc358764_get_modes(struct drm_connector *connector) -{ - struct tc358764 *ctx = connector_to_tc358764(connector); - - return drm_panel_get_modes(ctx->panel, connector); -} - -static const -struct drm_connector_helper_funcs tc358764_connector_helper_funcs = { - .get_modes = tc358764_get_modes, -}; - -static const struct drm_connector_funcs tc358764_connector_funcs = { - .fill_modes = drm_helper_probe_single_connector_modes, - .destroy = drm_connector_cleanup, - .reset = drm_atomic_helper_connector_reset, - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, -}; - -static void tc358764_disable(struct drm_bridge *bridge) -{ - struct tc358764 *ctx = bridge_to_tc358764(bridge); - int ret = drm_panel_disable(bridge_to_tc358764(bridge)->panel); - - if (ret < 0) - dev_err(ctx->dev, "error disabling panel (%d)\n", ret); -} - static void tc358764_post_disable(struct drm_bridge *bridge) { struct tc358764 *ctx = bridge_to_tc358764(bridge); int ret; - ret = drm_panel_unprepare(ctx->panel); - if (ret < 0) - dev_err(ctx->dev, "error unpreparing panel (%d)\n", ret); tc358764_reset(ctx); usleep_range(10000, 15000); ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies); @@ -335,70 +296,28 @@ static void tc358764_pre_enable(struct drm_bridge *bridge) ret = tc358764_init(ctx); if (ret < 0) dev_err(ctx->dev, "error initializing bridge (%d)\n", ret); - ret = drm_panel_prepare(ctx->panel); - if (ret < 0) - dev_err(ctx->dev, "error preparing panel (%d)\n", ret); -} - -static void tc358764_enable(struct drm_bridge *bridge) -{ - struct tc358764 *ctx = bridge_to_tc358764(bridge); - int ret = drm_panel_enable(ctx->panel); - - if (ret < 0) - dev_err(ctx->dev, "error enabling panel (%d)\n", ret); } static int tc358764_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) -{ - struct tc358764 *ctx = bridge_to_tc358764(bridge); - struct drm_device *drm = bridge->dev; - int ret; - - if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { - DRM_ERROR("Fix bridge driver to make connector optional!"); - return -EINVAL; - } - - ctx->connector.polled = DRM_CONNECTOR_POLL_HPD; - ret = drm_connector_init(drm, &ctx->connector, - &tc358764_connector_funcs, - DRM_MODE_CONNECTOR_LVDS); - if (ret) { - DRM_ERROR("Failed to initialize connector\n"); - return ret; - } - - drm_connector_helper_add(&ctx->connector, - &tc358764_connector_helper_funcs); - drm_connector_attach_encoder(&ctx->connector, bridge->encoder); - drm_panel_attach(ctx->panel, &ctx->connector); - ctx->connector.funcs->reset(&ctx->connector); - - return 0; -} - -static void tc358764_detach(struct drm_bridge *bridge) { struct tc358764 *ctx = bridge_to_tc358764(bridge); - drm_panel_detach(ctx->panel); - ctx->panel = NULL; + return drm_bridge_attach(bridge->encoder, ctx->panel_bridge, + bridge, flags); } static const struct drm_bridge_funcs tc358764_bridge_funcs = { - .disable = tc358764_disable, .post_disable = tc358764_post_disable, - .enable = tc358764_enable, .pre_enable = tc358764_pre_enable, .attach = tc358764_attach, - .detach = tc358764_detach, }; static int tc358764_parse_dt(struct tc358764 *ctx) { + struct drm_bridge *panel_bridge; struct device *dev = ctx->dev; + struct drm_panel *panel; int ret; ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); @@ -407,12 +326,17 @@ static int tc358764_parse_dt(struct tc358764 *ctx) return PTR_ERR(ctx->gpio_reset); } - ret = drm_of_find_panel_or_bridge(ctx->dev->of_node, 1, 0, &ctx->panel, - NULL); - if (ret && ret != -EPROBE_DEFER) - dev_err(dev, "cannot find panel (%d)\n", ret); + ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL); + if (ret) + return ret; - return ret; + panel_bridge = devm_drm_panel_bridge_add(dev, panel); + + if (IS_ERR(panel_bridge)) + return PTR_ERR(panel_bridge); + + ctx->panel_bridge = panel_bridge; + return 0; } static int tc358764_configure_regulators(struct tc358764 *ctx) @@ -458,6 +382,7 @@ static int tc358764_probe(struct mipi_dsi_device *dsi) return ret; ctx->bridge.funcs = &tc358764_bridge_funcs; + ctx->bridge.type = DRM_MODE_CONNECTOR_LVDS; ctx->bridge.of_node = dev->of_node; drm_bridge_add(&ctx->bridge);
Prepare the tc358764 bridge driver for use in a chained setup by replacing direct use of drm_panel with drm_panel_bridge support. The bridge panel will use the connector type reported by the panel, where the connector for this driver hardcodes DRM_MODE_CONNECTOR_LVDS. The tc358764 did not any additional info the the connector so the connector creation is passed to the bridge panel driver. v3: - Merge with patch to make connector creation optional to avoid creating two connectors (Laurent) - Pass connector creation to bridge panel, as this bridge driver did not add any extra info to the connector. - Set bridge.type to DRM_MODE_CONNECTOR_LVDS. v2: - Use PTR_ERR_OR_ZERO() (kbuild test robot) Signed-off-by: Sam Ravnborg <sam@ravnborg.org> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: kbuild test robot <lkp@intel.com> Cc: Andrzej Hajda <a.hajda@samsung.com> Cc: Neil Armstrong <narmstrong@baylibre.com> Cc: Jonas Karlman <jonas@kwiboo.se> Cc: Jernej Skrabec <jernej.skrabec@siol.net> --- drivers/gpu/drm/bridge/tc358764.c | 107 +++++------------------------- 1 file changed, 16 insertions(+), 91 deletions(-)