| Message ID | 20200309195216.31042-3-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: mxsfb: Add i.MX7 support | expand |
On 2020-03-09 20:51, Laurent Pinchart wrote: > Replace the manual connector implementation based on drm_panel with the > drm_panel_bridge helper. This simplifies the mxsfb driver by removing > connector-related code, and standardizing all pipeline control > operations on bridges. > > A hack is needed to get hold of the connector, as that's our only source > of bus flags and formats for now. As soon as the bridge API provides us > with that information this can be fixed. This seems like a nice simplification. I gave this a go applied on today's drm-misc-next using a Colibri iMX7 (imx7d-colibri-emmc-eval-v3.dts device tree). This device makes use of the simple panel driver. I get this when booting: [ 2.976698] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). [ 2.983526] ------------[ cut here ]------------ [ 2.988180] WARNING: CPU: 0 PID: 1 at drivers/gpu/drm/bridge/panel.c:267 devm_drm_panel_bridge_add+0x25/0x28 [ 2.998059] Modules linked in: [ 3.001159] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.6.0-rc5-yocto-standard-01250-ga4ce5db7c9f1 #29 [ 3.010493] Hardware name: Freescale i.MX7 Dual (Device Tree) [ 3.016281] [<c010f089>] (unwind_backtrace) from [<c010a9bb>] (show_stack+0xb/0xc) [ 3.023887] [<c010a9bb>] (show_stack) from [<c08b33cf>] (dump_stack+0x63/0x70) [ 3.031144] [<c08b33cf>] (dump_stack) from [<c0124385>] (__warn+0x9d/0xb0) [ 3.038047] [<c0124385>] (__warn) from [<c0124403>] (warn_slowpath_fmt+0x6b/0x70) [ 3.045564] [<c0124403>] (warn_slowpath_fmt) from [<c04ebf65>] (devm_drm_panel_bridge_add+0x25/0x28) [ 3.054736] [<c04ebf65>] (devm_drm_panel_bridge_add) from [<c051fe7f>] (mxsfb_probe+0x197/0x2e0) [ 3.063559] [<c051fe7f>] (mxsfb_probe) from [<c0532e91>] (platform_drv_probe+0x2d/0x60) [ 3.071598] [<c0532e91>] (platform_drv_probe) from [<c0531615>] (really_probe+0x1dd/0x330) [ 3.079897] [<c0531615>] (really_probe) from [<c05318b3>] (driver_probe_device+0x4f/0x154) [ 3.088195] [<c05318b3>] (driver_probe_device) from [<c0531b3f>] (device_driver_attach+0x37/0x44) [ 3.097101] [<c0531b3f>] (device_driver_attach) from [<c0531bc7>] (__driver_attach+0x7b/0xec) [ 3.105660] [<c0531bc7>] (__driver_attach) from [<c0530069>] (bus_for_each_dev+0x3d/0x64) [ 3.113870] [<c0530069>] (bus_for_each_dev) from [<c0530b9b>] (bus_add_driver+0xef/0x160) [ 3.122081] [<c0530b9b>] (bus_add_driver) from [<c053238d>] (driver_register+0x35/0x9c) [ 3.130119] [<c053238d>] (driver_register) from [<c0102de9>] (do_one_initcall+0x3d/0x1e4) [ 3.138333] [<c0102de9>] (do_one_initcall) from [<c1100d13>] (kernel_init_freeable+0x1b3/0x1fc) [ 3.147069] [<c1100d13>] (kernel_init_freeable) from [<c08c3f7b>] (kernel_init+0x7/0xd0) [ 3.155194] [<c08c3f7b>] (kernel_init) from [<c01010f9>] (ret_from_fork+0x11/0x38) [ 3.162789] Exception stack(0xec0e3fb0 to 0xec0e3ff8) [ 3.167862] 3fa0: 00000000 00000000 00000000 00000000 [ 3.176071] 3fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 3.184278] 3fe0: 00000000 00000000 00000000 00000000 00000013 00000000 [ 3.191029] ---[ end trace b69e1f44de470959 ]--- [ 3.195671] mxsfb 30730000.lcdif: Cannot connect bridge: -19 Should we maybe use devm_drm_panel_bridge_add_typed? Two more comments below. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpu/drm/mxsfb/Makefile | 2 +- > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 105 ++++++++++++++---------------- > drivers/gpu/drm/mxsfb/mxsfb_drv.h | 5 +- > drivers/gpu/drm/mxsfb/mxsfb_out.c | 99 ---------------------------- > 4 files changed, 53 insertions(+), 158 deletions(-) > delete mode 100644 drivers/gpu/drm/mxsfb/mxsfb_out.c > > diff --git a/drivers/gpu/drm/mxsfb/Makefile b/drivers/gpu/drm/mxsfb/Makefile > index ff6e358088fa..811584e54ad1 100644 > --- a/drivers/gpu/drm/mxsfb/Makefile > +++ b/drivers/gpu/drm/mxsfb/Makefile > @@ -1,3 +1,3 @@ > # SPDX-License-Identifier: GPL-2.0-only > -mxsfb-y := mxsfb_drv.o mxsfb_crtc.o mxsfb_out.o > +mxsfb-y := mxsfb_drv.o mxsfb_crtc.o > obj-$(CONFIG_DRM_MXSFB) += mxsfb.o > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > index 2e6068d96034..cffc70257bd3 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > @@ -29,7 +29,6 @@ > #include <drm/drm_gem_framebuffer_helper.h> > #include <drm/drm_irq.h> > #include <drm/drm_of.h> > -#include <drm/drm_panel.h> > #include <drm/drm_probe_helper.h> > #include <drm/drm_simple_kms_helper.h> > #include <drm/drm_vblank.h> > @@ -100,29 +99,11 @@ static void mxsfb_pipe_enable(struct > drm_simple_display_pipe *pipe, > struct drm_crtc_state *crtc_state, > struct drm_plane_state *plane_state) > { > - struct drm_connector *connector; > struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe); > struct drm_device *drm = pipe->plane.dev; > > - if (!mxsfb->connector) { > - list_for_each_entry(connector, > - &drm->mode_config.connector_list, > - head) > - if (connector->encoder == &mxsfb->pipe.encoder) { > - mxsfb->connector = connector; > - break; > - } > - } > - > - if (!mxsfb->connector) { > - dev_warn(drm->dev, "No connector attached, using default\n"); > - mxsfb->connector = &mxsfb->panel_connector; > - } > - > pm_runtime_get_sync(drm->dev); > - drm_panel_prepare(mxsfb->panel); > mxsfb_crtc_enable(mxsfb); > - drm_panel_enable(mxsfb->panel); > } > > static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) > @@ -132,9 +113,7 @@ static void mxsfb_pipe_disable(struct > drm_simple_display_pipe *pipe) > struct drm_crtc *crtc = &pipe->crtc; > struct drm_pending_vblank_event *event; > > - drm_panel_disable(mxsfb->panel); > mxsfb_crtc_disable(mxsfb); > - drm_panel_unprepare(mxsfb->panel); > pm_runtime_put_sync(drm->dev); > > spin_lock_irq(&drm->event_lock); > @@ -144,9 +123,6 @@ static void mxsfb_pipe_disable(struct > drm_simple_display_pipe *pipe) > drm_crtc_send_vblank_event(crtc, event); > } > spin_unlock_irq(&drm->event_lock); > - > - if (mxsfb->connector != &mxsfb->panel_connector) > - mxsfb->connector = NULL; > } > > static void mxsfb_pipe_update(struct drm_simple_display_pipe *pipe, > @@ -190,6 +166,48 @@ static struct drm_simple_display_pipe_funcs mxsfb_funcs = { > .disable_vblank = mxsfb_pipe_disable_vblank, > }; > > +static int mxsfb_attach_bridge(struct mxsfb_drm_private *mxsfb) > +{ > + struct drm_device *drm = mxsfb->drm; > + struct drm_connector_list_iter iter; > + struct drm_panel *panel; > + struct drm_bridge *bridge; > + int ret; > + > + ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, &panel, > + &bridge); > + if (ret) > + return ret; > + > + if (panel) { > + bridge = devm_drm_panel_bridge_add(drm->dev, panel); Do we need a select DRM_PANEL_BRIDGE in Kconfig maybe? From what I can tell there is no devm_drm_panel_bridge_add() stub if DRM_PANEL_BRIDGE is not selected. > + if (IS_ERR(bridge)) > + return PTR_ERR(bridge); > + } > + > + if (!bridge) > + return -ENODEV; > + > + ret = drm_simple_display_pipe_attach_bridge(&mxsfb->pipe, bridge); > + if (ret) { > + DRM_DEV_ERROR(drm->dev, > + "failed to attach bridge: %d\n", ret); > + return ret; > + } > + > + mxsfb->bridge = bridge; > + > + /* > + * Get hold of the connector. This is a bit of a hack, until the bridge > + * API gives us bus flags and formats. > + */ > + drm_connector_list_iter_begin(drm, &iter); > + mxsfb->connector = drm_connector_list_iter_next(&iter); > + drm_connector_list_iter_end(&iter); Could we not just use drm_panel_bridge_connector() here? -- Stefan > + > + return 0; > +} > + > static int mxsfb_load(struct drm_device *drm, unsigned long flags) > { > struct platform_device *pdev = to_platform_device(drm->dev); > @@ -201,6 +219,7 @@ static int mxsfb_load(struct drm_device *drm, > unsigned long flags) > if (!mxsfb) > return -ENOMEM; > > + mxsfb->drm = drm; > drm->dev_private = mxsfb; > mxsfb->devdata = &mxsfb_devdata[pdev->id_entry->driver_data]; > > @@ -236,41 +255,17 @@ static int mxsfb_load(struct drm_device *drm, > unsigned long flags) > /* Modeset init */ > drm_mode_config_init(drm); > > - ret = mxsfb_create_output(drm); > - if (ret < 0) { > - dev_err(drm->dev, "Failed to create outputs\n"); > - goto err_vblank; > - } > - > ret = drm_simple_display_pipe_init(drm, &mxsfb->pipe, &mxsfb_funcs, > - mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL, > - mxsfb->connector); > + mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL, NULL); > if (ret < 0) { > dev_err(drm->dev, "Cannot setup simple display pipe\n"); > goto err_vblank; > } > > - /* > - * Attach panel only if there is one. > - * If there is no panel attach, it must be a bridge. In this case, we > - * need a reference to its connector for a proper initialization. > - * We will do this check in pipe->enable(), since the connector won't > - * be attached to an encoder until then. > - */ > - > - if (mxsfb->panel) { > - ret = drm_panel_attach(mxsfb->panel, mxsfb->connector); > - if (ret) { > - dev_err(drm->dev, "Cannot connect panel: %d\n", ret); > - goto err_vblank; > - } > - } else if (mxsfb->bridge) { > - ret = drm_simple_display_pipe_attach_bridge(&mxsfb->pipe, > - mxsfb->bridge); > - if (ret) { > - dev_err(drm->dev, "Cannot connect bridge: %d\n", ret); > - goto err_vblank; > - } > + ret = mxsfb_attach_bridge(mxsfb); > + if (ret) { > + dev_err(drm->dev, "Cannot connect bridge: %d\n", ret); > + goto err_vblank; > } > > drm->mode_config.min_width = MXSFB_MIN_XRES; > @@ -288,7 +283,7 @@ static int mxsfb_load(struct drm_device *drm, > unsigned long flags) > > if (ret < 0) { > dev_err(drm->dev, "Failed to install IRQ handler\n"); > - goto err_irq; > + goto err_vblank; > } > > drm_kms_helper_poll_init(drm); > @@ -299,8 +294,6 @@ static int mxsfb_load(struct drm_device *drm, > unsigned long flags) > > return 0; > > -err_irq: > - drm_panel_detach(mxsfb->panel); > err_vblank: > pm_runtime_disable(drm->dev); > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > index 0b65b5194a9c..0e3e5a63bbf9 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > @@ -8,6 +8,8 @@ > #ifndef __MXSFB_DRV_H__ > #define __MXSFB_DRV_H__ > > +struct drm_device; > + > struct mxsfb_devdata { > unsigned int transfer_count; > unsigned int cur_buf; > @@ -26,10 +28,9 @@ struct mxsfb_drm_private { > struct clk *clk_axi; > struct clk *clk_disp_axi; > > + struct drm_device *drm; > struct drm_simple_display_pipe pipe; > - struct drm_connector panel_connector; > struct drm_connector *connector; > - struct drm_panel *panel; > struct drm_bridge *bridge; > }; > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_out.c > b/drivers/gpu/drm/mxsfb/mxsfb_out.c > deleted file mode 100644 > index 9eca1605d11d..000000000000 > --- a/drivers/gpu/drm/mxsfb/mxsfb_out.c > +++ /dev/null > @@ -1,99 +0,0 @@ > -// SPDX-License-Identifier: GPL-2.0-or-later > -/* > - * Copyright (C) 2016 Marek Vasut <marex@denx.de> > - */ > - > -#include <linux/of_graph.h> > - > -#include <drm/drm_atomic.h> > -#include <drm/drm_atomic_helper.h> > -#include <drm/drm_crtc.h> > -#include <drm/drm_fb_cma_helper.h> > -#include <drm/drm_gem_cma_helper.h> > -#include <drm/drm_of.h> > -#include <drm/drm_panel.h> > -#include <drm/drm_plane_helper.h> > -#include <drm/drm_probe_helper.h> > -#include <drm/drm_simple_kms_helper.h> > - > -#include "mxsfb_drv.h" > - > -static struct mxsfb_drm_private * > -drm_connector_to_mxsfb_drm_private(struct drm_connector *connector) > -{ > - return container_of(connector, struct mxsfb_drm_private, > - panel_connector); > -} > - > -static int mxsfb_panel_get_modes(struct drm_connector *connector) > -{ > - struct mxsfb_drm_private *mxsfb = > - drm_connector_to_mxsfb_drm_private(connector); > - > - if (mxsfb->panel) > - return drm_panel_get_modes(mxsfb->panel, connector); > - > - return 0; > -} > - > -static const struct > -drm_connector_helper_funcs mxsfb_panel_connector_helper_funcs = { > - .get_modes = mxsfb_panel_get_modes, > -}; > - > -static enum drm_connector_status > -mxsfb_panel_connector_detect(struct drm_connector *connector, bool force) > -{ > - struct mxsfb_drm_private *mxsfb = > - drm_connector_to_mxsfb_drm_private(connector); > - > - if (mxsfb->panel) > - return connector_status_connected; > - > - return connector_status_disconnected; > -} > - > -static void mxsfb_panel_connector_destroy(struct drm_connector *connector) > -{ > - struct mxsfb_drm_private *mxsfb = > - drm_connector_to_mxsfb_drm_private(connector); > - > - if (mxsfb->panel) > - drm_panel_detach(mxsfb->panel); > - > - drm_connector_unregister(connector); > - drm_connector_cleanup(connector); > -} > - > -static const struct drm_connector_funcs mxsfb_panel_connector_funcs = { > - .detect = mxsfb_panel_connector_detect, > - .fill_modes = drm_helper_probe_single_connector_modes, > - .destroy = mxsfb_panel_connector_destroy, > - .reset = drm_atomic_helper_connector_reset, > - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > -}; > - > -int mxsfb_create_output(struct drm_device *drm) > -{ > - struct mxsfb_drm_private *mxsfb = drm->dev_private; > - int ret; > - > - ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, > - &mxsfb->panel, &mxsfb->bridge); > - if (ret) > - return ret; > - > - if (mxsfb->panel) { > - mxsfb->connector = &mxsfb->panel_connector; > - mxsfb->connector->dpms = DRM_MODE_DPMS_OFF; > - mxsfb->connector->polled = 0; > - drm_connector_helper_add(mxsfb->connector, > - &mxsfb_panel_connector_helper_funcs); > - ret = drm_connector_init(drm, mxsfb->connector, > - &mxsfb_panel_connector_funcs, > - DRM_MODE_CONNECTOR_Unknown); > - } > - > - return ret; > -}
Hi Stefan. On Mon, Mar 23, 2020 at 10:27:21PM +0100, Stefan Agner wrote: > On 2020-03-09 20:51, Laurent Pinchart wrote: > > Replace the manual connector implementation based on drm_panel with the > > drm_panel_bridge helper. This simplifies the mxsfb driver by removing > > connector-related code, and standardizing all pipeline control > > operations on bridges. > > > > A hack is needed to get hold of the connector, as that's our only source > > of bus flags and formats for now. As soon as the bridge API provides us > > with that information this can be fixed. > > This seems like a nice simplification. > > I gave this a go applied on today's drm-misc-next using a Colibri iMX7 > (imx7d-colibri-emmc-eval-v3.dts device tree). This device makes use of > the simple panel driver. I get this when booting: > > [ 2.976698] [drm] Supports vblank timestamp caching Rev 2 > (21.10.2013). > [ 2.983526] ------------[ cut here ]------------ > [ 2.988180] WARNING: CPU: 0 PID: 1 at > drivers/gpu/drm/bridge/panel.c:267 devm_drm_panel_bridge_add+0x25/0x28 ... I think you hit one of the panels that does not yet specify a connector_type. If this is the case, we should fix the definition in panel-simple. Sam
On 2020-03-23 22:38, Sam Ravnborg wrote: > Hi Stefan. > > On Mon, Mar 23, 2020 at 10:27:21PM +0100, Stefan Agner wrote: >> On 2020-03-09 20:51, Laurent Pinchart wrote: >> > Replace the manual connector implementation based on drm_panel with the >> > drm_panel_bridge helper. This simplifies the mxsfb driver by removing >> > connector-related code, and standardizing all pipeline control >> > operations on bridges. >> > >> > A hack is needed to get hold of the connector, as that's our only source >> > of bus flags and formats for now. As soon as the bridge API provides us >> > with that information this can be fixed. >> >> This seems like a nice simplification. >> >> I gave this a go applied on today's drm-misc-next using a Colibri iMX7 >> (imx7d-colibri-emmc-eval-v3.dts device tree). This device makes use of >> the simple panel driver. I get this when booting: >> >> [ 2.976698] [drm] Supports vblank timestamp caching Rev 2 >> (21.10.2013). >> [ 2.983526] ------------[ cut here ]------------ >> [ 2.988180] WARNING: CPU: 0 PID: 1 at >> drivers/gpu/drm/bridge/panel.c:267 devm_drm_panel_bridge_add+0x25/0x28 > > ... > > I think you hit one of the panels that does not yet specify a > connector_type. > > If this is the case, we should fix the definition in panel-simple. Yes, this indeed the case, I used the "logictechno,lt161010-2nhc" compatible string. Will get this fixed! But I think its still worth defining a default in the driver as DPI is what this IP outputs... -- Stefan
Hi Stefan, On Mon, Mar 23, 2020 at 10:27:21PM +0100, Stefan Agner wrote: > On 2020-03-09 20:51, Laurent Pinchart wrote: > > Replace the manual connector implementation based on drm_panel with the > > drm_panel_bridge helper. This simplifies the mxsfb driver by removing > > connector-related code, and standardizing all pipeline control > > operations on bridges. > > > > A hack is needed to get hold of the connector, as that's our only source > > of bus flags and formats for now. As soon as the bridge API provides us > > with that information this can be fixed. > > This seems like a nice simplification. > > I gave this a go applied on today's drm-misc-next using a Colibri iMX7 > (imx7d-colibri-emmc-eval-v3.dts device tree). This device makes use of > the simple panel driver. I get this when booting: > > [ 2.976698] [drm] Supports vblank timestamp caching Rev 2 > (21.10.2013). > [ 2.983526] ------------[ cut here ]------------ > [ 2.988180] WARNING: CPU: 0 PID: 1 at > drivers/gpu/drm/bridge/panel.c:267 devm_drm_panel_bridge_add+0x25/0x28 > [ 2.998059] Modules linked in: > [ 3.001159] CPU: 0 PID: 1 Comm: swapper/0 Not tainted > 5.6.0-rc5-yocto-standard-01250-ga4ce5db7c9f1 #29 > [ 3.010493] Hardware name: Freescale i.MX7 Dual (Device Tree) > [ 3.016281] [<c010f089>] (unwind_backtrace) from [<c010a9bb>] > (show_stack+0xb/0xc) > [ 3.023887] [<c010a9bb>] (show_stack) from [<c08b33cf>] > (dump_stack+0x63/0x70) > [ 3.031144] [<c08b33cf>] (dump_stack) from [<c0124385>] > (__warn+0x9d/0xb0) > [ 3.038047] [<c0124385>] (__warn) from [<c0124403>] > (warn_slowpath_fmt+0x6b/0x70) > [ 3.045564] [<c0124403>] (warn_slowpath_fmt) from [<c04ebf65>] > (devm_drm_panel_bridge_add+0x25/0x28) > [ 3.054736] [<c04ebf65>] (devm_drm_panel_bridge_add) from > [<c051fe7f>] (mxsfb_probe+0x197/0x2e0) > [ 3.063559] [<c051fe7f>] (mxsfb_probe) from [<c0532e91>] > (platform_drv_probe+0x2d/0x60) > [ 3.071598] [<c0532e91>] (platform_drv_probe) from [<c0531615>] > (really_probe+0x1dd/0x330) > [ 3.079897] [<c0531615>] (really_probe) from [<c05318b3>] > (driver_probe_device+0x4f/0x154) > [ 3.088195] [<c05318b3>] (driver_probe_device) from [<c0531b3f>] > (device_driver_attach+0x37/0x44) > [ 3.097101] [<c0531b3f>] (device_driver_attach) from [<c0531bc7>] > (__driver_attach+0x7b/0xec) > [ 3.105660] [<c0531bc7>] (__driver_attach) from [<c0530069>] > (bus_for_each_dev+0x3d/0x64) > [ 3.113870] [<c0530069>] (bus_for_each_dev) from [<c0530b9b>] > (bus_add_driver+0xef/0x160) > [ 3.122081] [<c0530b9b>] (bus_add_driver) from [<c053238d>] > (driver_register+0x35/0x9c) > [ 3.130119] [<c053238d>] (driver_register) from [<c0102de9>] > (do_one_initcall+0x3d/0x1e4) > [ 3.138333] [<c0102de9>] (do_one_initcall) from [<c1100d13>] > (kernel_init_freeable+0x1b3/0x1fc) > [ 3.147069] [<c1100d13>] (kernel_init_freeable) from [<c08c3f7b>] > (kernel_init+0x7/0xd0) > [ 3.155194] [<c08c3f7b>] (kernel_init) from [<c01010f9>] > (ret_from_fork+0x11/0x38) > [ 3.162789] Exception stack(0xec0e3fb0 to 0xec0e3ff8) > [ 3.167862] 3fa0: 00000000 > 00000000 00000000 00000000 > [ 3.176071] 3fc0: 00000000 00000000 00000000 00000000 00000000 > 00000000 00000000 00000000 > [ 3.184278] 3fe0: 00000000 00000000 00000000 00000000 00000013 > 00000000 > [ 3.191029] ---[ end trace b69e1f44de470959 ]--- > [ 3.195671] mxsfb 30730000.lcdif: Cannot connect bridge: -19 > > Should we maybe use devm_drm_panel_bridge_add_typed? As Sam reported, this is caused by the panel not setting connector_type. We could use devm_drm_panel_bridge_add_typed(), even if I like the warning as it uncovers the problematic panels and gets them fixed. What would be your preference ? > Two more comments below. > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > drivers/gpu/drm/mxsfb/Makefile | 2 +- > > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 105 ++++++++++++++---------------- > > drivers/gpu/drm/mxsfb/mxsfb_drv.h | 5 +- > > drivers/gpu/drm/mxsfb/mxsfb_out.c | 99 ---------------------------- > > 4 files changed, 53 insertions(+), 158 deletions(-) > > delete mode 100644 drivers/gpu/drm/mxsfb/mxsfb_out.c > > > > diff --git a/drivers/gpu/drm/mxsfb/Makefile b/drivers/gpu/drm/mxsfb/Makefile > > index ff6e358088fa..811584e54ad1 100644 > > --- a/drivers/gpu/drm/mxsfb/Makefile > > +++ b/drivers/gpu/drm/mxsfb/Makefile > > @@ -1,3 +1,3 @@ > > # SPDX-License-Identifier: GPL-2.0-only > > -mxsfb-y := mxsfb_drv.o mxsfb_crtc.o mxsfb_out.o > > +mxsfb-y := mxsfb_drv.o mxsfb_crtc.o > > obj-$(CONFIG_DRM_MXSFB) += mxsfb.o > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > index 2e6068d96034..cffc70257bd3 100644 > > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > @@ -29,7 +29,6 @@ > > #include <drm/drm_gem_framebuffer_helper.h> > > #include <drm/drm_irq.h> > > #include <drm/drm_of.h> > > -#include <drm/drm_panel.h> > > #include <drm/drm_probe_helper.h> > > #include <drm/drm_simple_kms_helper.h> > > #include <drm/drm_vblank.h> > > @@ -100,29 +99,11 @@ static void mxsfb_pipe_enable(struct > > drm_simple_display_pipe *pipe, > > struct drm_crtc_state *crtc_state, > > struct drm_plane_state *plane_state) > > { > > - struct drm_connector *connector; > > struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe); > > struct drm_device *drm = pipe->plane.dev; > > > > - if (!mxsfb->connector) { > > - list_for_each_entry(connector, > > - &drm->mode_config.connector_list, > > - head) > > - if (connector->encoder == &mxsfb->pipe.encoder) { > > - mxsfb->connector = connector; > > - break; > > - } > > - } > > - > > - if (!mxsfb->connector) { > > - dev_warn(drm->dev, "No connector attached, using default\n"); > > - mxsfb->connector = &mxsfb->panel_connector; > > - } > > - > > pm_runtime_get_sync(drm->dev); > > - drm_panel_prepare(mxsfb->panel); > > mxsfb_crtc_enable(mxsfb); > > - drm_panel_enable(mxsfb->panel); > > } > > > > static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) > > @@ -132,9 +113,7 @@ static void mxsfb_pipe_disable(struct > > drm_simple_display_pipe *pipe) > > struct drm_crtc *crtc = &pipe->crtc; > > struct drm_pending_vblank_event *event; > > > > - drm_panel_disable(mxsfb->panel); > > mxsfb_crtc_disable(mxsfb); > > - drm_panel_unprepare(mxsfb->panel); > > pm_runtime_put_sync(drm->dev); > > > > spin_lock_irq(&drm->event_lock); > > @@ -144,9 +123,6 @@ static void mxsfb_pipe_disable(struct > > drm_simple_display_pipe *pipe) > > drm_crtc_send_vblank_event(crtc, event); > > } > > spin_unlock_irq(&drm->event_lock); > > - > > - if (mxsfb->connector != &mxsfb->panel_connector) > > - mxsfb->connector = NULL; > > } > > > > static void mxsfb_pipe_update(struct drm_simple_display_pipe *pipe, > > @@ -190,6 +166,48 @@ static struct drm_simple_display_pipe_funcs mxsfb_funcs = { > > .disable_vblank = mxsfb_pipe_disable_vblank, > > }; > > > > +static int mxsfb_attach_bridge(struct mxsfb_drm_private *mxsfb) > > +{ > > + struct drm_device *drm = mxsfb->drm; > > + struct drm_connector_list_iter iter; > > + struct drm_panel *panel; > > + struct drm_bridge *bridge; > > + int ret; > > + > > + ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, &panel, > > + &bridge); > > + if (ret) > > + return ret; > > + > > + if (panel) { > > + bridge = devm_drm_panel_bridge_add(drm->dev, panel); > > Do we need a select DRM_PANEL_BRIDGE in Kconfig maybe? From what I can > tell there is no devm_drm_panel_bridge_add() stub if DRM_PANEL_BRIDGE is > not selected. You're right, I'll add it. > > + if (IS_ERR(bridge)) > > + return PTR_ERR(bridge); > > + } > > + > > + if (!bridge) > > + return -ENODEV; > > + > > + ret = drm_simple_display_pipe_attach_bridge(&mxsfb->pipe, bridge); > > + if (ret) { > > + DRM_DEV_ERROR(drm->dev, > > + "failed to attach bridge: %d\n", ret); > > + return ret; > > + } > > + > > + mxsfb->bridge = bridge; > > + > > + /* > > + * Get hold of the connector. This is a bit of a hack, until the bridge > > + * API gives us bus flags and formats. > > + */ > > + drm_connector_list_iter_begin(drm, &iter); > > + mxsfb->connector = drm_connector_list_iter_next(&iter); > > + drm_connector_list_iter_end(&iter); > > Could we not just use drm_panel_bridge_connector() here? There's no guarantee that mxsfb->bridge is a panel bridge, the LCDIF output could be connected to another bridge in which case drm_of_find_panel_or_bridge() will set panel to NULL and bridge to a non-NULL value). > > + > > + return 0; > > +} > > + > > static int mxsfb_load(struct drm_device *drm, unsigned long flags) > > { > > struct platform_device *pdev = to_platform_device(drm->dev); > > @@ -201,6 +219,7 @@ static int mxsfb_load(struct drm_device *drm, > > unsigned long flags) > > if (!mxsfb) > > return -ENOMEM; > > > > + mxsfb->drm = drm; > > drm->dev_private = mxsfb; > > mxsfb->devdata = &mxsfb_devdata[pdev->id_entry->driver_data]; > > > > @@ -236,41 +255,17 @@ static int mxsfb_load(struct drm_device *drm, > > unsigned long flags) > > /* Modeset init */ > > drm_mode_config_init(drm); > > > > - ret = mxsfb_create_output(drm); > > - if (ret < 0) { > > - dev_err(drm->dev, "Failed to create outputs\n"); > > - goto err_vblank; > > - } > > - > > ret = drm_simple_display_pipe_init(drm, &mxsfb->pipe, &mxsfb_funcs, > > - mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL, > > - mxsfb->connector); > > + mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL, NULL); > > if (ret < 0) { > > dev_err(drm->dev, "Cannot setup simple display pipe\n"); > > goto err_vblank; > > } > > > > - /* > > - * Attach panel only if there is one. > > - * If there is no panel attach, it must be a bridge. In this case, we > > - * need a reference to its connector for a proper initialization. > > - * We will do this check in pipe->enable(), since the connector won't > > - * be attached to an encoder until then. > > - */ > > - > > - if (mxsfb->panel) { > > - ret = drm_panel_attach(mxsfb->panel, mxsfb->connector); > > - if (ret) { > > - dev_err(drm->dev, "Cannot connect panel: %d\n", ret); > > - goto err_vblank; > > - } > > - } else if (mxsfb->bridge) { > > - ret = drm_simple_display_pipe_attach_bridge(&mxsfb->pipe, > > - mxsfb->bridge); > > - if (ret) { > > - dev_err(drm->dev, "Cannot connect bridge: %d\n", ret); > > - goto err_vblank; > > - } > > + ret = mxsfb_attach_bridge(mxsfb); > > + if (ret) { > > + dev_err(drm->dev, "Cannot connect bridge: %d\n", ret); > > + goto err_vblank; > > } > > > > drm->mode_config.min_width = MXSFB_MIN_XRES; > > @@ -288,7 +283,7 @@ static int mxsfb_load(struct drm_device *drm, > > unsigned long flags) > > > > if (ret < 0) { > > dev_err(drm->dev, "Failed to install IRQ handler\n"); > > - goto err_irq; > > + goto err_vblank; > > } > > > > drm_kms_helper_poll_init(drm); > > @@ -299,8 +294,6 @@ static int mxsfb_load(struct drm_device *drm, > > unsigned long flags) > > > > return 0; > > > > -err_irq: > > - drm_panel_detach(mxsfb->panel); > > err_vblank: > > pm_runtime_disable(drm->dev); > > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > > b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > > index 0b65b5194a9c..0e3e5a63bbf9 100644 > > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > > @@ -8,6 +8,8 @@ > > #ifndef __MXSFB_DRV_H__ > > #define __MXSFB_DRV_H__ > > > > +struct drm_device; > > + > > struct mxsfb_devdata { > > unsigned int transfer_count; > > unsigned int cur_buf; > > @@ -26,10 +28,9 @@ struct mxsfb_drm_private { > > struct clk *clk_axi; > > struct clk *clk_disp_axi; > > > > + struct drm_device *drm; > > struct drm_simple_display_pipe pipe; > > - struct drm_connector panel_connector; > > struct drm_connector *connector; > > - struct drm_panel *panel; > > struct drm_bridge *bridge; > > }; > > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_out.c > > b/drivers/gpu/drm/mxsfb/mxsfb_out.c > > deleted file mode 100644 > > index 9eca1605d11d..000000000000 > > --- a/drivers/gpu/drm/mxsfb/mxsfb_out.c > > +++ /dev/null > > @@ -1,99 +0,0 @@ > > -// SPDX-License-Identifier: GPL-2.0-or-later > > -/* > > - * Copyright (C) 2016 Marek Vasut <marex@denx.de> > > - */ > > - > > -#include <linux/of_graph.h> > > - > > -#include <drm/drm_atomic.h> > > -#include <drm/drm_atomic_helper.h> > > -#include <drm/drm_crtc.h> > > -#include <drm/drm_fb_cma_helper.h> > > -#include <drm/drm_gem_cma_helper.h> > > -#include <drm/drm_of.h> > > -#include <drm/drm_panel.h> > > -#include <drm/drm_plane_helper.h> > > -#include <drm/drm_probe_helper.h> > > -#include <drm/drm_simple_kms_helper.h> > > - > > -#include "mxsfb_drv.h" > > - > > -static struct mxsfb_drm_private * > > -drm_connector_to_mxsfb_drm_private(struct drm_connector *connector) > > -{ > > - return container_of(connector, struct mxsfb_drm_private, > > - panel_connector); > > -} > > - > > -static int mxsfb_panel_get_modes(struct drm_connector *connector) > > -{ > > - struct mxsfb_drm_private *mxsfb = > > - drm_connector_to_mxsfb_drm_private(connector); > > - > > - if (mxsfb->panel) > > - return drm_panel_get_modes(mxsfb->panel, connector); > > - > > - return 0; > > -} > > - > > -static const struct > > -drm_connector_helper_funcs mxsfb_panel_connector_helper_funcs = { > > - .get_modes = mxsfb_panel_get_modes, > > -}; > > - > > -static enum drm_connector_status > > -mxsfb_panel_connector_detect(struct drm_connector *connector, bool force) > > -{ > > - struct mxsfb_drm_private *mxsfb = > > - drm_connector_to_mxsfb_drm_private(connector); > > - > > - if (mxsfb->panel) > > - return connector_status_connected; > > - > > - return connector_status_disconnected; > > -} > > - > > -static void mxsfb_panel_connector_destroy(struct drm_connector *connector) > > -{ > > - struct mxsfb_drm_private *mxsfb = > > - drm_connector_to_mxsfb_drm_private(connector); > > - > > - if (mxsfb->panel) > > - drm_panel_detach(mxsfb->panel); > > - > > - drm_connector_unregister(connector); > > - drm_connector_cleanup(connector); > > -} > > - > > -static const struct drm_connector_funcs mxsfb_panel_connector_funcs = { > > - .detect = mxsfb_panel_connector_detect, > > - .fill_modes = drm_helper_probe_single_connector_modes, > > - .destroy = mxsfb_panel_connector_destroy, > > - .reset = drm_atomic_helper_connector_reset, > > - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > > - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > > -}; > > - > > -int mxsfb_create_output(struct drm_device *drm) > > -{ > > - struct mxsfb_drm_private *mxsfb = drm->dev_private; > > - int ret; > > - > > - ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, > > - &mxsfb->panel, &mxsfb->bridge); > > - if (ret) > > - return ret; > > - > > - if (mxsfb->panel) { > > - mxsfb->connector = &mxsfb->panel_connector; > > - mxsfb->connector->dpms = DRM_MODE_DPMS_OFF; > > - mxsfb->connector->polled = 0; > > - drm_connector_helper_add(mxsfb->connector, > > - &mxsfb_panel_connector_helper_funcs); > > - ret = drm_connector_init(drm, mxsfb->connector, > > - &mxsfb_panel_connector_funcs, > > - DRM_MODE_CONNECTOR_Unknown); > > - } > > - > > - return ret; > > -}
On Sat, May 30, 2020 at 05:14:21AM +0300, Laurent Pinchart wrote: > Hi Stefan, > > On Mon, Mar 23, 2020 at 10:27:21PM +0100, Stefan Agner wrote: > > On 2020-03-09 20:51, Laurent Pinchart wrote: > > > Replace the manual connector implementation based on drm_panel with the > > > drm_panel_bridge helper. This simplifies the mxsfb driver by removing > > > connector-related code, and standardizing all pipeline control > > > operations on bridges. > > > > > > A hack is needed to get hold of the connector, as that's our only source > > > of bus flags and formats for now. As soon as the bridge API provides us > > > with that information this can be fixed. > > > > This seems like a nice simplification. > > > > I gave this a go applied on today's drm-misc-next using a Colibri iMX7 > > (imx7d-colibri-emmc-eval-v3.dts device tree). This device makes use of > > the simple panel driver. I get this when booting: > > > > [ 2.976698] [drm] Supports vblank timestamp caching Rev 2 > > (21.10.2013). > > [ 2.983526] ------------[ cut here ]------------ > > [ 2.988180] WARNING: CPU: 0 PID: 1 at > > drivers/gpu/drm/bridge/panel.c:267 devm_drm_panel_bridge_add+0x25/0x28 > > [ 2.998059] Modules linked in: > > [ 3.001159] CPU: 0 PID: 1 Comm: swapper/0 Not tainted > > 5.6.0-rc5-yocto-standard-01250-ga4ce5db7c9f1 #29 > > [ 3.010493] Hardware name: Freescale i.MX7 Dual (Device Tree) > > [ 3.016281] [<c010f089>] (unwind_backtrace) from [<c010a9bb>] > > (show_stack+0xb/0xc) > > [ 3.023887] [<c010a9bb>] (show_stack) from [<c08b33cf>] > > (dump_stack+0x63/0x70) > > [ 3.031144] [<c08b33cf>] (dump_stack) from [<c0124385>] > > (__warn+0x9d/0xb0) > > [ 3.038047] [<c0124385>] (__warn) from [<c0124403>] > > (warn_slowpath_fmt+0x6b/0x70) > > [ 3.045564] [<c0124403>] (warn_slowpath_fmt) from [<c04ebf65>] > > (devm_drm_panel_bridge_add+0x25/0x28) > > [ 3.054736] [<c04ebf65>] (devm_drm_panel_bridge_add) from > > [<c051fe7f>] (mxsfb_probe+0x197/0x2e0) > > [ 3.063559] [<c051fe7f>] (mxsfb_probe) from [<c0532e91>] > > (platform_drv_probe+0x2d/0x60) > > [ 3.071598] [<c0532e91>] (platform_drv_probe) from [<c0531615>] > > (really_probe+0x1dd/0x330) > > [ 3.079897] [<c0531615>] (really_probe) from [<c05318b3>] > > (driver_probe_device+0x4f/0x154) > > [ 3.088195] [<c05318b3>] (driver_probe_device) from [<c0531b3f>] > > (device_driver_attach+0x37/0x44) > > [ 3.097101] [<c0531b3f>] (device_driver_attach) from [<c0531bc7>] > > (__driver_attach+0x7b/0xec) > > [ 3.105660] [<c0531bc7>] (__driver_attach) from [<c0530069>] > > (bus_for_each_dev+0x3d/0x64) > > [ 3.113870] [<c0530069>] (bus_for_each_dev) from [<c0530b9b>] > > (bus_add_driver+0xef/0x160) > > [ 3.122081] [<c0530b9b>] (bus_add_driver) from [<c053238d>] > > (driver_register+0x35/0x9c) > > [ 3.130119] [<c053238d>] (driver_register) from [<c0102de9>] > > (do_one_initcall+0x3d/0x1e4) > > [ 3.138333] [<c0102de9>] (do_one_initcall) from [<c1100d13>] > > (kernel_init_freeable+0x1b3/0x1fc) > > [ 3.147069] [<c1100d13>] (kernel_init_freeable) from [<c08c3f7b>] > > (kernel_init+0x7/0xd0) > > [ 3.155194] [<c08c3f7b>] (kernel_init) from [<c01010f9>] > > (ret_from_fork+0x11/0x38) > > [ 3.162789] Exception stack(0xec0e3fb0 to 0xec0e3ff8) > > [ 3.167862] 3fa0: 00000000 > > 00000000 00000000 00000000 > > [ 3.176071] 3fc0: 00000000 00000000 00000000 00000000 00000000 > > 00000000 00000000 00000000 > > [ 3.184278] 3fe0: 00000000 00000000 00000000 00000000 00000013 > > 00000000 > > [ 3.191029] ---[ end trace b69e1f44de470959 ]--- > > [ 3.195671] mxsfb 30730000.lcdif: Cannot connect bridge: -19 > > > > Should we maybe use devm_drm_panel_bridge_add_typed? > > As Sam reported, this is caused by the panel not setting connector_type. > We could use devm_drm_panel_bridge_add_typed(), even if I like the > warning as it uncovers the problematic panels and gets them fixed. What > would be your preference ? Adding warnings everywhere is kinda uncool, at least my experience is that if there's too much you get warning overload and train people to ignore them. Imo either hide the wwarning for now, or if it's not too much work, review all the panel drivers and make sure they set the connector type somewhere. Warnings are kinda ok once you're pretty sure you got them all, and want to make sure newly added drivers get this all right. But not before we've reached that. Cheers, Daniel > > > Two more comments below. > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > drivers/gpu/drm/mxsfb/Makefile | 2 +- > > > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 105 ++++++++++++++---------------- > > > drivers/gpu/drm/mxsfb/mxsfb_drv.h | 5 +- > > > drivers/gpu/drm/mxsfb/mxsfb_out.c | 99 ---------------------------- > > > 4 files changed, 53 insertions(+), 158 deletions(-) > > > delete mode 100644 drivers/gpu/drm/mxsfb/mxsfb_out.c > > > > > > diff --git a/drivers/gpu/drm/mxsfb/Makefile b/drivers/gpu/drm/mxsfb/Makefile > > > index ff6e358088fa..811584e54ad1 100644 > > > --- a/drivers/gpu/drm/mxsfb/Makefile > > > +++ b/drivers/gpu/drm/mxsfb/Makefile > > > @@ -1,3 +1,3 @@ > > > # SPDX-License-Identifier: GPL-2.0-only > > > -mxsfb-y := mxsfb_drv.o mxsfb_crtc.o mxsfb_out.o > > > +mxsfb-y := mxsfb_drv.o mxsfb_crtc.o > > > obj-$(CONFIG_DRM_MXSFB) += mxsfb.o > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > > b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > > index 2e6068d96034..cffc70257bd3 100644 > > > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > > @@ -29,7 +29,6 @@ > > > #include <drm/drm_gem_framebuffer_helper.h> > > > #include <drm/drm_irq.h> > > > #include <drm/drm_of.h> > > > -#include <drm/drm_panel.h> > > > #include <drm/drm_probe_helper.h> > > > #include <drm/drm_simple_kms_helper.h> > > > #include <drm/drm_vblank.h> > > > @@ -100,29 +99,11 @@ static void mxsfb_pipe_enable(struct > > > drm_simple_display_pipe *pipe, > > > struct drm_crtc_state *crtc_state, > > > struct drm_plane_state *plane_state) > > > { > > > - struct drm_connector *connector; > > > struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe); > > > struct drm_device *drm = pipe->plane.dev; > > > > > > - if (!mxsfb->connector) { > > > - list_for_each_entry(connector, > > > - &drm->mode_config.connector_list, > > > - head) > > > - if (connector->encoder == &mxsfb->pipe.encoder) { > > > - mxsfb->connector = connector; > > > - break; > > > - } > > > - } > > > - > > > - if (!mxsfb->connector) { > > > - dev_warn(drm->dev, "No connector attached, using default\n"); > > > - mxsfb->connector = &mxsfb->panel_connector; > > > - } > > > - > > > pm_runtime_get_sync(drm->dev); > > > - drm_panel_prepare(mxsfb->panel); > > > mxsfb_crtc_enable(mxsfb); > > > - drm_panel_enable(mxsfb->panel); > > > } > > > > > > static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) > > > @@ -132,9 +113,7 @@ static void mxsfb_pipe_disable(struct > > > drm_simple_display_pipe *pipe) > > > struct drm_crtc *crtc = &pipe->crtc; > > > struct drm_pending_vblank_event *event; > > > > > > - drm_panel_disable(mxsfb->panel); > > > mxsfb_crtc_disable(mxsfb); > > > - drm_panel_unprepare(mxsfb->panel); > > > pm_runtime_put_sync(drm->dev); > > > > > > spin_lock_irq(&drm->event_lock); > > > @@ -144,9 +123,6 @@ static void mxsfb_pipe_disable(struct > > > drm_simple_display_pipe *pipe) > > > drm_crtc_send_vblank_event(crtc, event); > > > } > > > spin_unlock_irq(&drm->event_lock); > > > - > > > - if (mxsfb->connector != &mxsfb->panel_connector) > > > - mxsfb->connector = NULL; > > > } > > > > > > static void mxsfb_pipe_update(struct drm_simple_display_pipe *pipe, > > > @@ -190,6 +166,48 @@ static struct drm_simple_display_pipe_funcs mxsfb_funcs = { > > > .disable_vblank = mxsfb_pipe_disable_vblank, > > > }; > > > > > > +static int mxsfb_attach_bridge(struct mxsfb_drm_private *mxsfb) > > > +{ > > > + struct drm_device *drm = mxsfb->drm; > > > + struct drm_connector_list_iter iter; > > > + struct drm_panel *panel; > > > + struct drm_bridge *bridge; > > > + int ret; > > > + > > > + ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, &panel, > > > + &bridge); > > > + if (ret) > > > + return ret; > > > + > > > + if (panel) { > > > + bridge = devm_drm_panel_bridge_add(drm->dev, panel); > > > > Do we need a select DRM_PANEL_BRIDGE in Kconfig maybe? From what I can > > tell there is no devm_drm_panel_bridge_add() stub if DRM_PANEL_BRIDGE is > > not selected. > > You're right, I'll add it. > > > > + if (IS_ERR(bridge)) > > > + return PTR_ERR(bridge); > > > + } > > > + > > > + if (!bridge) > > > + return -ENODEV; > > > + > > > + ret = drm_simple_display_pipe_attach_bridge(&mxsfb->pipe, bridge); > > > + if (ret) { > > > + DRM_DEV_ERROR(drm->dev, > > > + "failed to attach bridge: %d\n", ret); > > > + return ret; > > > + } > > > + > > > + mxsfb->bridge = bridge; > > > + > > > + /* > > > + * Get hold of the connector. This is a bit of a hack, until the bridge > > > + * API gives us bus flags and formats. > > > + */ > > > + drm_connector_list_iter_begin(drm, &iter); > > > + mxsfb->connector = drm_connector_list_iter_next(&iter); > > > + drm_connector_list_iter_end(&iter); > > > > Could we not just use drm_panel_bridge_connector() here? > > There's no guarantee that mxsfb->bridge is a panel bridge, the LCDIF > output could be connected to another bridge in which case > drm_of_find_panel_or_bridge() will set panel to NULL and bridge to a > non-NULL value). > > > > + > > > + return 0; > > > +} > > > + > > > static int mxsfb_load(struct drm_device *drm, unsigned long flags) > > > { > > > struct platform_device *pdev = to_platform_device(drm->dev); > > > @@ -201,6 +219,7 @@ static int mxsfb_load(struct drm_device *drm, > > > unsigned long flags) > > > if (!mxsfb) > > > return -ENOMEM; > > > > > > + mxsfb->drm = drm; > > > drm->dev_private = mxsfb; > > > mxsfb->devdata = &mxsfb_devdata[pdev->id_entry->driver_data]; > > > > > > @@ -236,41 +255,17 @@ static int mxsfb_load(struct drm_device *drm, > > > unsigned long flags) > > > /* Modeset init */ > > > drm_mode_config_init(drm); > > > > > > - ret = mxsfb_create_output(drm); > > > - if (ret < 0) { > > > - dev_err(drm->dev, "Failed to create outputs\n"); > > > - goto err_vblank; > > > - } > > > - > > > ret = drm_simple_display_pipe_init(drm, &mxsfb->pipe, &mxsfb_funcs, > > > - mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL, > > > - mxsfb->connector); > > > + mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL, NULL); > > > if (ret < 0) { > > > dev_err(drm->dev, "Cannot setup simple display pipe\n"); > > > goto err_vblank; > > > } > > > > > > - /* > > > - * Attach panel only if there is one. > > > - * If there is no panel attach, it must be a bridge. In this case, we > > > - * need a reference to its connector for a proper initialization. > > > - * We will do this check in pipe->enable(), since the connector won't > > > - * be attached to an encoder until then. > > > - */ > > > - > > > - if (mxsfb->panel) { > > > - ret = drm_panel_attach(mxsfb->panel, mxsfb->connector); > > > - if (ret) { > > > - dev_err(drm->dev, "Cannot connect panel: %d\n", ret); > > > - goto err_vblank; > > > - } > > > - } else if (mxsfb->bridge) { > > > - ret = drm_simple_display_pipe_attach_bridge(&mxsfb->pipe, > > > - mxsfb->bridge); > > > - if (ret) { > > > - dev_err(drm->dev, "Cannot connect bridge: %d\n", ret); > > > - goto err_vblank; > > > - } > > > + ret = mxsfb_attach_bridge(mxsfb); > > > + if (ret) { > > > + dev_err(drm->dev, "Cannot connect bridge: %d\n", ret); > > > + goto err_vblank; > > > } > > > > > > drm->mode_config.min_width = MXSFB_MIN_XRES; > > > @@ -288,7 +283,7 @@ static int mxsfb_load(struct drm_device *drm, > > > unsigned long flags) > > > > > > if (ret < 0) { > > > dev_err(drm->dev, "Failed to install IRQ handler\n"); > > > - goto err_irq; > > > + goto err_vblank; > > > } > > > > > > drm_kms_helper_poll_init(drm); > > > @@ -299,8 +294,6 @@ static int mxsfb_load(struct drm_device *drm, > > > unsigned long flags) > > > > > > return 0; > > > > > > -err_irq: > > > - drm_panel_detach(mxsfb->panel); > > > err_vblank: > > > pm_runtime_disable(drm->dev); > > > > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > > > b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > > > index 0b65b5194a9c..0e3e5a63bbf9 100644 > > > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > > > @@ -8,6 +8,8 @@ > > > #ifndef __MXSFB_DRV_H__ > > > #define __MXSFB_DRV_H__ > > > > > > +struct drm_device; > > > + > > > struct mxsfb_devdata { > > > unsigned int transfer_count; > > > unsigned int cur_buf; > > > @@ -26,10 +28,9 @@ struct mxsfb_drm_private { > > > struct clk *clk_axi; > > > struct clk *clk_disp_axi; > > > > > > + struct drm_device *drm; > > > struct drm_simple_display_pipe pipe; > > > - struct drm_connector panel_connector; > > > struct drm_connector *connector; > > > - struct drm_panel *panel; > > > struct drm_bridge *bridge; > > > }; > > > > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_out.c > > > b/drivers/gpu/drm/mxsfb/mxsfb_out.c > > > deleted file mode 100644 > > > index 9eca1605d11d..000000000000 > > > --- a/drivers/gpu/drm/mxsfb/mxsfb_out.c > > > +++ /dev/null > > > @@ -1,99 +0,0 @@ > > > -// SPDX-License-Identifier: GPL-2.0-or-later > > > -/* > > > - * Copyright (C) 2016 Marek Vasut <marex@denx.de> > > > - */ > > > - > > > -#include <linux/of_graph.h> > > > - > > > -#include <drm/drm_atomic.h> > > > -#include <drm/drm_atomic_helper.h> > > > -#include <drm/drm_crtc.h> > > > -#include <drm/drm_fb_cma_helper.h> > > > -#include <drm/drm_gem_cma_helper.h> > > > -#include <drm/drm_of.h> > > > -#include <drm/drm_panel.h> > > > -#include <drm/drm_plane_helper.h> > > > -#include <drm/drm_probe_helper.h> > > > -#include <drm/drm_simple_kms_helper.h> > > > - > > > -#include "mxsfb_drv.h" > > > - > > > -static struct mxsfb_drm_private * > > > -drm_connector_to_mxsfb_drm_private(struct drm_connector *connector) > > > -{ > > > - return container_of(connector, struct mxsfb_drm_private, > > > - panel_connector); > > > -} > > > - > > > -static int mxsfb_panel_get_modes(struct drm_connector *connector) > > > -{ > > > - struct mxsfb_drm_private *mxsfb = > > > - drm_connector_to_mxsfb_drm_private(connector); > > > - > > > - if (mxsfb->panel) > > > - return drm_panel_get_modes(mxsfb->panel, connector); > > > - > > > - return 0; > > > -} > > > - > > > -static const struct > > > -drm_connector_helper_funcs mxsfb_panel_connector_helper_funcs = { > > > - .get_modes = mxsfb_panel_get_modes, > > > -}; > > > - > > > -static enum drm_connector_status > > > -mxsfb_panel_connector_detect(struct drm_connector *connector, bool force) > > > -{ > > > - struct mxsfb_drm_private *mxsfb = > > > - drm_connector_to_mxsfb_drm_private(connector); > > > - > > > - if (mxsfb->panel) > > > - return connector_status_connected; > > > - > > > - return connector_status_disconnected; > > > -} > > > - > > > -static void mxsfb_panel_connector_destroy(struct drm_connector *connector) > > > -{ > > > - struct mxsfb_drm_private *mxsfb = > > > - drm_connector_to_mxsfb_drm_private(connector); > > > - > > > - if (mxsfb->panel) > > > - drm_panel_detach(mxsfb->panel); > > > - > > > - drm_connector_unregister(connector); > > > - drm_connector_cleanup(connector); > > > -} > > > - > > > -static const struct drm_connector_funcs mxsfb_panel_connector_funcs = { > > > - .detect = mxsfb_panel_connector_detect, > > > - .fill_modes = drm_helper_probe_single_connector_modes, > > > - .destroy = mxsfb_panel_connector_destroy, > > > - .reset = drm_atomic_helper_connector_reset, > > > - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > > > - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > > > -}; > > > - > > > -int mxsfb_create_output(struct drm_device *drm) > > > -{ > > > - struct mxsfb_drm_private *mxsfb = drm->dev_private; > > > - int ret; > > > - > > > - ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, > > > - &mxsfb->panel, &mxsfb->bridge); > > > - if (ret) > > > - return ret; > > > - > > > - if (mxsfb->panel) { > > > - mxsfb->connector = &mxsfb->panel_connector; > > > - mxsfb->connector->dpms = DRM_MODE_DPMS_OFF; > > > - mxsfb->connector->polled = 0; > > > - drm_connector_helper_add(mxsfb->connector, > > > - &mxsfb_panel_connector_helper_funcs); > > > - ret = drm_connector_init(drm, mxsfb->connector, > > > - &mxsfb_panel_connector_funcs, > > > - DRM_MODE_CONNECTOR_Unknown); > > > - } > > > - > > > - return ret; > > > -} > > -- > Regards, > > Laurent Pinchart > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 2020-06-02 15:12, Daniel Vetter wrote: > On Sat, May 30, 2020 at 05:14:21AM +0300, Laurent Pinchart wrote: >> Hi Stefan, >> >> On Mon, Mar 23, 2020 at 10:27:21PM +0100, Stefan Agner wrote: >> > On 2020-03-09 20:51, Laurent Pinchart wrote: >> > > Replace the manual connector implementation based on drm_panel with the >> > > drm_panel_bridge helper. This simplifies the mxsfb driver by removing >> > > connector-related code, and standardizing all pipeline control >> > > operations on bridges. >> > > >> > > A hack is needed to get hold of the connector, as that's our only source >> > > of bus flags and formats for now. As soon as the bridge API provides us >> > > with that information this can be fixed. >> > >> > This seems like a nice simplification. >> > >> > I gave this a go applied on today's drm-misc-next using a Colibri iMX7 >> > (imx7d-colibri-emmc-eval-v3.dts device tree). This device makes use of >> > the simple panel driver. I get this when booting: >> > >> > [ 2.976698] [drm] Supports vblank timestamp caching Rev 2 >> > (21.10.2013). >> > [ 2.983526] ------------[ cut here ]------------ >> > [ 2.988180] WARNING: CPU: 0 PID: 1 at >> > drivers/gpu/drm/bridge/panel.c:267 devm_drm_panel_bridge_add+0x25/0x28 >> > [ 2.998059] Modules linked in: >> > [ 3.001159] CPU: 0 PID: 1 Comm: swapper/0 Not tainted >> > 5.6.0-rc5-yocto-standard-01250-ga4ce5db7c9f1 #29 >> > [ 3.010493] Hardware name: Freescale i.MX7 Dual (Device Tree) >> > [ 3.016281] [<c010f089>] (unwind_backtrace) from [<c010a9bb>] >> > (show_stack+0xb/0xc) >> > [ 3.023887] [<c010a9bb>] (show_stack) from [<c08b33cf>] >> > (dump_stack+0x63/0x70) >> > [ 3.031144] [<c08b33cf>] (dump_stack) from [<c0124385>] >> > (__warn+0x9d/0xb0) >> > [ 3.038047] [<c0124385>] (__warn) from [<c0124403>] >> > (warn_slowpath_fmt+0x6b/0x70) >> > [ 3.045564] [<c0124403>] (warn_slowpath_fmt) from [<c04ebf65>] >> > (devm_drm_panel_bridge_add+0x25/0x28) >> > [ 3.054736] [<c04ebf65>] (devm_drm_panel_bridge_add) from >> > [<c051fe7f>] (mxsfb_probe+0x197/0x2e0) >> > [ 3.063559] [<c051fe7f>] (mxsfb_probe) from [<c0532e91>] >> > (platform_drv_probe+0x2d/0x60) >> > [ 3.071598] [<c0532e91>] (platform_drv_probe) from [<c0531615>] >> > (really_probe+0x1dd/0x330) >> > [ 3.079897] [<c0531615>] (really_probe) from [<c05318b3>] >> > (driver_probe_device+0x4f/0x154) >> > [ 3.088195] [<c05318b3>] (driver_probe_device) from [<c0531b3f>] >> > (device_driver_attach+0x37/0x44) >> > [ 3.097101] [<c0531b3f>] (device_driver_attach) from [<c0531bc7>] >> > (__driver_attach+0x7b/0xec) >> > [ 3.105660] [<c0531bc7>] (__driver_attach) from [<c0530069>] >> > (bus_for_each_dev+0x3d/0x64) >> > [ 3.113870] [<c0530069>] (bus_for_each_dev) from [<c0530b9b>] >> > (bus_add_driver+0xef/0x160) >> > [ 3.122081] [<c0530b9b>] (bus_add_driver) from [<c053238d>] >> > (driver_register+0x35/0x9c) >> > [ 3.130119] [<c053238d>] (driver_register) from [<c0102de9>] >> > (do_one_initcall+0x3d/0x1e4) >> > [ 3.138333] [<c0102de9>] (do_one_initcall) from [<c1100d13>] >> > (kernel_init_freeable+0x1b3/0x1fc) >> > [ 3.147069] [<c1100d13>] (kernel_init_freeable) from [<c08c3f7b>] >> > (kernel_init+0x7/0xd0) >> > [ 3.155194] [<c08c3f7b>] (kernel_init) from [<c01010f9>] >> > (ret_from_fork+0x11/0x38) >> > [ 3.162789] Exception stack(0xec0e3fb0 to 0xec0e3ff8) >> > [ 3.167862] 3fa0: 00000000 >> > 00000000 00000000 00000000 >> > [ 3.176071] 3fc0: 00000000 00000000 00000000 00000000 00000000 >> > 00000000 00000000 00000000 >> > [ 3.184278] 3fe0: 00000000 00000000 00000000 00000000 00000013 >> > 00000000 >> > [ 3.191029] ---[ end trace b69e1f44de470959 ]--- >> > [ 3.195671] mxsfb 30730000.lcdif: Cannot connect bridge: -19 >> > >> > Should we maybe use devm_drm_panel_bridge_add_typed? >> >> As Sam reported, this is caused by the panel not setting connector_type. >> We could use devm_drm_panel_bridge_add_typed(), even if I like the >> warning as it uncovers the problematic panels and gets them fixed. What >> would be your preference ? > > Adding warnings everywhere is kinda uncool, at least my experience is that > if there's too much you get warning overload and train people to ignore > them. > > Imo either hide the wwarning for now, or if it's not too much work, review > all the panel drivers and make sure they set the connector type somewhere. > Warnings are kinda ok once you're pretty sure you got them all, and want > to make sure newly added drivers get this all right. But not before we've > reached that. I am with Daniel on this, too many warnings make users blind of them, so we should save them when really attention is needed. I guess the only question which connector type we should default to. I think DRM_MODE_CONNECTOR_DPI make sense for this IP. -- Stefan > > Cheers, Daniel > >> >> > Two more comments below. >> > >> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> > > --- >> > > drivers/gpu/drm/mxsfb/Makefile | 2 +- >> > > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 105 ++++++++++++++---------------- >> > > drivers/gpu/drm/mxsfb/mxsfb_drv.h | 5 +- >> > > drivers/gpu/drm/mxsfb/mxsfb_out.c | 99 ---------------------------- >> > > 4 files changed, 53 insertions(+), 158 deletions(-) >> > > delete mode 100644 drivers/gpu/drm/mxsfb/mxsfb_out.c >> > > >> > > diff --git a/drivers/gpu/drm/mxsfb/Makefile b/drivers/gpu/drm/mxsfb/Makefile >> > > index ff6e358088fa..811584e54ad1 100644 >> > > --- a/drivers/gpu/drm/mxsfb/Makefile >> > > +++ b/drivers/gpu/drm/mxsfb/Makefile >> > > @@ -1,3 +1,3 @@ >> > > # SPDX-License-Identifier: GPL-2.0-only >> > > -mxsfb-y := mxsfb_drv.o mxsfb_crtc.o mxsfb_out.o >> > > +mxsfb-y := mxsfb_drv.o mxsfb_crtc.o >> > > obj-$(CONFIG_DRM_MXSFB) += mxsfb.o >> > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c >> > > b/drivers/gpu/drm/mxsfb/mxsfb_drv.c >> > > index 2e6068d96034..cffc70257bd3 100644 >> > > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c >> > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c >> > > @@ -29,7 +29,6 @@ >> > > #include <drm/drm_gem_framebuffer_helper.h> >> > > #include <drm/drm_irq.h> >> > > #include <drm/drm_of.h> >> > > -#include <drm/drm_panel.h> >> > > #include <drm/drm_probe_helper.h> >> > > #include <drm/drm_simple_kms_helper.h> >> > > #include <drm/drm_vblank.h> >> > > @@ -100,29 +99,11 @@ static void mxsfb_pipe_enable(struct >> > > drm_simple_display_pipe *pipe, >> > > struct drm_crtc_state *crtc_state, >> > > struct drm_plane_state *plane_state) >> > > { >> > > - struct drm_connector *connector; >> > > struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe); >> > > struct drm_device *drm = pipe->plane.dev; >> > > >> > > - if (!mxsfb->connector) { >> > > - list_for_each_entry(connector, >> > > - &drm->mode_config.connector_list, >> > > - head) >> > > - if (connector->encoder == &mxsfb->pipe.encoder) { >> > > - mxsfb->connector = connector; >> > > - break; >> > > - } >> > > - } >> > > - >> > > - if (!mxsfb->connector) { >> > > - dev_warn(drm->dev, "No connector attached, using default\n"); >> > > - mxsfb->connector = &mxsfb->panel_connector; >> > > - } >> > > - >> > > pm_runtime_get_sync(drm->dev); >> > > - drm_panel_prepare(mxsfb->panel); >> > > mxsfb_crtc_enable(mxsfb); >> > > - drm_panel_enable(mxsfb->panel); >> > > } >> > > >> > > static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) >> > > @@ -132,9 +113,7 @@ static void mxsfb_pipe_disable(struct >> > > drm_simple_display_pipe *pipe) >> > > struct drm_crtc *crtc = &pipe->crtc; >> > > struct drm_pending_vblank_event *event; >> > > >> > > - drm_panel_disable(mxsfb->panel); >> > > mxsfb_crtc_disable(mxsfb); >> > > - drm_panel_unprepare(mxsfb->panel); >> > > pm_runtime_put_sync(drm->dev); >> > > >> > > spin_lock_irq(&drm->event_lock); >> > > @@ -144,9 +123,6 @@ static void mxsfb_pipe_disable(struct >> > > drm_simple_display_pipe *pipe) >> > > drm_crtc_send_vblank_event(crtc, event); >> > > } >> > > spin_unlock_irq(&drm->event_lock); >> > > - >> > > - if (mxsfb->connector != &mxsfb->panel_connector) >> > > - mxsfb->connector = NULL; >> > > } >> > > >> > > static void mxsfb_pipe_update(struct drm_simple_display_pipe *pipe, >> > > @@ -190,6 +166,48 @@ static struct drm_simple_display_pipe_funcs mxsfb_funcs = { >> > > .disable_vblank = mxsfb_pipe_disable_vblank, >> > > }; >> > > >> > > +static int mxsfb_attach_bridge(struct mxsfb_drm_private *mxsfb) >> > > +{ >> > > + struct drm_device *drm = mxsfb->drm; >> > > + struct drm_connector_list_iter iter; >> > > + struct drm_panel *panel; >> > > + struct drm_bridge *bridge; >> > > + int ret; >> > > + >> > > + ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, &panel, >> > > + &bridge); >> > > + if (ret) >> > > + return ret; >> > > + >> > > + if (panel) { >> > > + bridge = devm_drm_panel_bridge_add(drm->dev, panel); >> > >> > Do we need a select DRM_PANEL_BRIDGE in Kconfig maybe? From what I can >> > tell there is no devm_drm_panel_bridge_add() stub if DRM_PANEL_BRIDGE is >> > not selected. >> >> You're right, I'll add it. >> >> > > + if (IS_ERR(bridge)) >> > > + return PTR_ERR(bridge); >> > > + } >> > > + >> > > + if (!bridge) >> > > + return -ENODEV; >> > > + >> > > + ret = drm_simple_display_pipe_attach_bridge(&mxsfb->pipe, bridge); >> > > + if (ret) { >> > > + DRM_DEV_ERROR(drm->dev, >> > > + "failed to attach bridge: %d\n", ret); >> > > + return ret; >> > > + } >> > > + >> > > + mxsfb->bridge = bridge; >> > > + >> > > + /* >> > > + * Get hold of the connector. This is a bit of a hack, until the bridge >> > > + * API gives us bus flags and formats. >> > > + */ >> > > + drm_connector_list_iter_begin(drm, &iter); >> > > + mxsfb->connector = drm_connector_list_iter_next(&iter); >> > > + drm_connector_list_iter_end(&iter); >> > >> > Could we not just use drm_panel_bridge_connector() here? >> >> There's no guarantee that mxsfb->bridge is a panel bridge, the LCDIF >> output could be connected to another bridge in which case >> drm_of_find_panel_or_bridge() will set panel to NULL and bridge to a >> non-NULL value). >> >> > > + >> > > + return 0; >> > > +} >> > > + >> > > static int mxsfb_load(struct drm_device *drm, unsigned long flags) >> > > { >> > > struct platform_device *pdev = to_platform_device(drm->dev); >> > > @@ -201,6 +219,7 @@ static int mxsfb_load(struct drm_device *drm, >> > > unsigned long flags) >> > > if (!mxsfb) >> > > return -ENOMEM; >> > > >> > > + mxsfb->drm = drm; >> > > drm->dev_private = mxsfb; >> > > mxsfb->devdata = &mxsfb_devdata[pdev->id_entry->driver_data]; >> > > >> > > @@ -236,41 +255,17 @@ static int mxsfb_load(struct drm_device *drm, >> > > unsigned long flags) >> > > /* Modeset init */ >> > > drm_mode_config_init(drm); >> > > >> > > - ret = mxsfb_create_output(drm); >> > > - if (ret < 0) { >> > > - dev_err(drm->dev, "Failed to create outputs\n"); >> > > - goto err_vblank; >> > > - } >> > > - >> > > ret = drm_simple_display_pipe_init(drm, &mxsfb->pipe, &mxsfb_funcs, >> > > - mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL, >> > > - mxsfb->connector); >> > > + mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL, NULL); >> > > if (ret < 0) { >> > > dev_err(drm->dev, "Cannot setup simple display pipe\n"); >> > > goto err_vblank; >> > > } >> > > >> > > - /* >> > > - * Attach panel only if there is one. >> > > - * If there is no panel attach, it must be a bridge. In this case, we >> > > - * need a reference to its connector for a proper initialization. >> > > - * We will do this check in pipe->enable(), since the connector won't >> > > - * be attached to an encoder until then. >> > > - */ >> > > - >> > > - if (mxsfb->panel) { >> > > - ret = drm_panel_attach(mxsfb->panel, mxsfb->connector); >> > > - if (ret) { >> > > - dev_err(drm->dev, "Cannot connect panel: %d\n", ret); >> > > - goto err_vblank; >> > > - } >> > > - } else if (mxsfb->bridge) { >> > > - ret = drm_simple_display_pipe_attach_bridge(&mxsfb->pipe, >> > > - mxsfb->bridge); >> > > - if (ret) { >> > > - dev_err(drm->dev, "Cannot connect bridge: %d\n", ret); >> > > - goto err_vblank; >> > > - } >> > > + ret = mxsfb_attach_bridge(mxsfb); >> > > + if (ret) { >> > > + dev_err(drm->dev, "Cannot connect bridge: %d\n", ret); >> > > + goto err_vblank; >> > > } >> > > >> > > drm->mode_config.min_width = MXSFB_MIN_XRES; >> > > @@ -288,7 +283,7 @@ static int mxsfb_load(struct drm_device *drm, >> > > unsigned long flags) >> > > >> > > if (ret < 0) { >> > > dev_err(drm->dev, "Failed to install IRQ handler\n"); >> > > - goto err_irq; >> > > + goto err_vblank; >> > > } >> > > >> > > drm_kms_helper_poll_init(drm); >> > > @@ -299,8 +294,6 @@ static int mxsfb_load(struct drm_device *drm, >> > > unsigned long flags) >> > > >> > > return 0; >> > > >> > > -err_irq: >> > > - drm_panel_detach(mxsfb->panel); >> > > err_vblank: >> > > pm_runtime_disable(drm->dev); >> > > >> > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h >> > > b/drivers/gpu/drm/mxsfb/mxsfb_drv.h >> > > index 0b65b5194a9c..0e3e5a63bbf9 100644 >> > > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h >> > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h >> > > @@ -8,6 +8,8 @@ >> > > #ifndef __MXSFB_DRV_H__ >> > > #define __MXSFB_DRV_H__ >> > > >> > > +struct drm_device; >> > > + >> > > struct mxsfb_devdata { >> > > unsigned int transfer_count; >> > > unsigned int cur_buf; >> > > @@ -26,10 +28,9 @@ struct mxsfb_drm_private { >> > > struct clk *clk_axi; >> > > struct clk *clk_disp_axi; >> > > >> > > + struct drm_device *drm; >> > > struct drm_simple_display_pipe pipe; >> > > - struct drm_connector panel_connector; >> > > struct drm_connector *connector; >> > > - struct drm_panel *panel; >> > > struct drm_bridge *bridge; >> > > }; >> > > >> > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_out.c >> > > b/drivers/gpu/drm/mxsfb/mxsfb_out.c >> > > deleted file mode 100644 >> > > index 9eca1605d11d..000000000000 >> > > --- a/drivers/gpu/drm/mxsfb/mxsfb_out.c >> > > +++ /dev/null >> > > @@ -1,99 +0,0 @@ >> > > -// SPDX-License-Identifier: GPL-2.0-or-later >> > > -/* >> > > - * Copyright (C) 2016 Marek Vasut <marex@denx.de> >> > > - */ >> > > - >> > > -#include <linux/of_graph.h> >> > > - >> > > -#include <drm/drm_atomic.h> >> > > -#include <drm/drm_atomic_helper.h> >> > > -#include <drm/drm_crtc.h> >> > > -#include <drm/drm_fb_cma_helper.h> >> > > -#include <drm/drm_gem_cma_helper.h> >> > > -#include <drm/drm_of.h> >> > > -#include <drm/drm_panel.h> >> > > -#include <drm/drm_plane_helper.h> >> > > -#include <drm/drm_probe_helper.h> >> > > -#include <drm/drm_simple_kms_helper.h> >> > > - >> > > -#include "mxsfb_drv.h" >> > > - >> > > -static struct mxsfb_drm_private * >> > > -drm_connector_to_mxsfb_drm_private(struct drm_connector *connector) >> > > -{ >> > > - return container_of(connector, struct mxsfb_drm_private, >> > > - panel_connector); >> > > -} >> > > - >> > > -static int mxsfb_panel_get_modes(struct drm_connector *connector) >> > > -{ >> > > - struct mxsfb_drm_private *mxsfb = >> > > - drm_connector_to_mxsfb_drm_private(connector); >> > > - >> > > - if (mxsfb->panel) >> > > - return drm_panel_get_modes(mxsfb->panel, connector); >> > > - >> > > - return 0; >> > > -} >> > > - >> > > -static const struct >> > > -drm_connector_helper_funcs mxsfb_panel_connector_helper_funcs = { >> > > - .get_modes = mxsfb_panel_get_modes, >> > > -}; >> > > - >> > > -static enum drm_connector_status >> > > -mxsfb_panel_connector_detect(struct drm_connector *connector, bool force) >> > > -{ >> > > - struct mxsfb_drm_private *mxsfb = >> > > - drm_connector_to_mxsfb_drm_private(connector); >> > > - >> > > - if (mxsfb->panel) >> > > - return connector_status_connected; >> > > - >> > > - return connector_status_disconnected; >> > > -} >> > > - >> > > -static void mxsfb_panel_connector_destroy(struct drm_connector *connector) >> > > -{ >> > > - struct mxsfb_drm_private *mxsfb = >> > > - drm_connector_to_mxsfb_drm_private(connector); >> > > - >> > > - if (mxsfb->panel) >> > > - drm_panel_detach(mxsfb->panel); >> > > - >> > > - drm_connector_unregister(connector); >> > > - drm_connector_cleanup(connector); >> > > -} >> > > - >> > > -static const struct drm_connector_funcs mxsfb_panel_connector_funcs = { >> > > - .detect = mxsfb_panel_connector_detect, >> > > - .fill_modes = drm_helper_probe_single_connector_modes, >> > > - .destroy = mxsfb_panel_connector_destroy, >> > > - .reset = drm_atomic_helper_connector_reset, >> > > - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, >> > > - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, >> > > -}; >> > > - >> > > -int mxsfb_create_output(struct drm_device *drm) >> > > -{ >> > > - struct mxsfb_drm_private *mxsfb = drm->dev_private; >> > > - int ret; >> > > - >> > > - ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, >> > > - &mxsfb->panel, &mxsfb->bridge); >> > > - if (ret) >> > > - return ret; >> > > - >> > > - if (mxsfb->panel) { >> > > - mxsfb->connector = &mxsfb->panel_connector; >> > > - mxsfb->connector->dpms = DRM_MODE_DPMS_OFF; >> > > - mxsfb->connector->polled = 0; >> > > - drm_connector_helper_add(mxsfb->connector, >> > > - &mxsfb_panel_connector_helper_funcs); >> > > - ret = drm_connector_init(drm, mxsfb->connector, >> > > - &mxsfb_panel_connector_funcs, >> > > - DRM_MODE_CONNECTOR_Unknown); >> > > - } >> > > - >> > > - return ret; >> > > -} >> >> -- >> Regards, >> >> Laurent Pinchart >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Daniel, On Tue, Jun 02, 2020 at 03:12:37PM +0200, Daniel Vetter wrote: > On Sat, May 30, 2020 at 05:14:21AM +0300, Laurent Pinchart wrote: > > On Mon, Mar 23, 2020 at 10:27:21PM +0100, Stefan Agner wrote: > > > On 2020-03-09 20:51, Laurent Pinchart wrote: > > > > Replace the manual connector implementation based on drm_panel with the > > > > drm_panel_bridge helper. This simplifies the mxsfb driver by removing > > > > connector-related code, and standardizing all pipeline control > > > > operations on bridges. > > > > > > > > A hack is needed to get hold of the connector, as that's our only source > > > > of bus flags and formats for now. As soon as the bridge API provides us > > > > with that information this can be fixed. > > > > > > This seems like a nice simplification. > > > > > > I gave this a go applied on today's drm-misc-next using a Colibri iMX7 > > > (imx7d-colibri-emmc-eval-v3.dts device tree). This device makes use of > > > the simple panel driver. I get this when booting: > > > > > > [ 2.976698] [drm] Supports vblank timestamp caching Rev 2 > > > (21.10.2013). > > > [ 2.983526] ------------[ cut here ]------------ > > > [ 2.988180] WARNING: CPU: 0 PID: 1 at > > > drivers/gpu/drm/bridge/panel.c:267 devm_drm_panel_bridge_add+0x25/0x28 > > > [ 2.998059] Modules linked in: > > > [ 3.001159] CPU: 0 PID: 1 Comm: swapper/0 Not tainted > > > 5.6.0-rc5-yocto-standard-01250-ga4ce5db7c9f1 #29 > > > [ 3.010493] Hardware name: Freescale i.MX7 Dual (Device Tree) > > > [ 3.016281] [<c010f089>] (unwind_backtrace) from [<c010a9bb>] > > > (show_stack+0xb/0xc) > > > [ 3.023887] [<c010a9bb>] (show_stack) from [<c08b33cf>] > > > (dump_stack+0x63/0x70) > > > [ 3.031144] [<c08b33cf>] (dump_stack) from [<c0124385>] > > > (__warn+0x9d/0xb0) > > > [ 3.038047] [<c0124385>] (__warn) from [<c0124403>] > > > (warn_slowpath_fmt+0x6b/0x70) > > > [ 3.045564] [<c0124403>] (warn_slowpath_fmt) from [<c04ebf65>] > > > (devm_drm_panel_bridge_add+0x25/0x28) > > > [ 3.054736] [<c04ebf65>] (devm_drm_panel_bridge_add) from > > > [<c051fe7f>] (mxsfb_probe+0x197/0x2e0) > > > [ 3.063559] [<c051fe7f>] (mxsfb_probe) from [<c0532e91>] > > > (platform_drv_probe+0x2d/0x60) > > > [ 3.071598] [<c0532e91>] (platform_drv_probe) from [<c0531615>] > > > (really_probe+0x1dd/0x330) > > > [ 3.079897] [<c0531615>] (really_probe) from [<c05318b3>] > > > (driver_probe_device+0x4f/0x154) > > > [ 3.088195] [<c05318b3>] (driver_probe_device) from [<c0531b3f>] > > > (device_driver_attach+0x37/0x44) > > > [ 3.097101] [<c0531b3f>] (device_driver_attach) from [<c0531bc7>] > > > (__driver_attach+0x7b/0xec) > > > [ 3.105660] [<c0531bc7>] (__driver_attach) from [<c0530069>] > > > (bus_for_each_dev+0x3d/0x64) > > > [ 3.113870] [<c0530069>] (bus_for_each_dev) from [<c0530b9b>] > > > (bus_add_driver+0xef/0x160) > > > [ 3.122081] [<c0530b9b>] (bus_add_driver) from [<c053238d>] > > > (driver_register+0x35/0x9c) > > > [ 3.130119] [<c053238d>] (driver_register) from [<c0102de9>] > > > (do_one_initcall+0x3d/0x1e4) > > > [ 3.138333] [<c0102de9>] (do_one_initcall) from [<c1100d13>] > > > (kernel_init_freeable+0x1b3/0x1fc) > > > [ 3.147069] [<c1100d13>] (kernel_init_freeable) from [<c08c3f7b>] > > > (kernel_init+0x7/0xd0) > > > [ 3.155194] [<c08c3f7b>] (kernel_init) from [<c01010f9>] > > > (ret_from_fork+0x11/0x38) > > > [ 3.162789] Exception stack(0xec0e3fb0 to 0xec0e3ff8) > > > [ 3.167862] 3fa0: 00000000 > > > 00000000 00000000 00000000 > > > [ 3.176071] 3fc0: 00000000 00000000 00000000 00000000 00000000 > > > 00000000 00000000 00000000 > > > [ 3.184278] 3fe0: 00000000 00000000 00000000 00000000 00000013 > > > 00000000 > > > [ 3.191029] ---[ end trace b69e1f44de470959 ]--- > > > [ 3.195671] mxsfb 30730000.lcdif: Cannot connect bridge: -19 > > > > > > Should we maybe use devm_drm_panel_bridge_add_typed? > > > > As Sam reported, this is caused by the panel not setting connector_type. > > We could use devm_drm_panel_bridge_add_typed(), even if I like the > > warning as it uncovers the problematic panels and gets them fixed. What > > would be your preference ? > > Adding warnings everywhere is kinda uncool, at least my experience is that > if there's too much you get warning overload and train people to ignore > them. > > Imo either hide the wwarning for now, or if it's not too much work, review > all the panel drivers and make sure they set the connector type somewhere. All panel drivers but panel-simple set the connector type. For panel-simple, the type is stored in the panel_desc panel description, and out of the 123 supported panels, only 48 have a connector type. I've just sent a patch to fix this for the 7 DSI panels, so only 68 panels to go :-) This will require trying to find the corresponding datasheets, so that's a large effort for a single developer. That's why I was hoping a WARN_ON() could help distributing the work :-) I hear your concern though, and I'll set a default in this driver for the time being. > Warnings are kinda ok once you're pretty sure you got them all, and want > to make sure newly added drivers get this all right. But not before we've > reached that. > > > > Two more comments below. > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > drivers/gpu/drm/mxsfb/Makefile | 2 +- > > > > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 105 ++++++++++++++---------------- > > > > drivers/gpu/drm/mxsfb/mxsfb_drv.h | 5 +- > > > > drivers/gpu/drm/mxsfb/mxsfb_out.c | 99 ---------------------------- > > > > 4 files changed, 53 insertions(+), 158 deletions(-) > > > > delete mode 100644 drivers/gpu/drm/mxsfb/mxsfb_out.c > > > > > > > > diff --git a/drivers/gpu/drm/mxsfb/Makefile b/drivers/gpu/drm/mxsfb/Makefile > > > > index ff6e358088fa..811584e54ad1 100644 > > > > --- a/drivers/gpu/drm/mxsfb/Makefile > > > > +++ b/drivers/gpu/drm/mxsfb/Makefile > > > > @@ -1,3 +1,3 @@ > > > > # SPDX-License-Identifier: GPL-2.0-only > > > > -mxsfb-y := mxsfb_drv.o mxsfb_crtc.o mxsfb_out.o > > > > +mxsfb-y := mxsfb_drv.o mxsfb_crtc.o > > > > obj-$(CONFIG_DRM_MXSFB) += mxsfb.o > > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > > > b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > > > index 2e6068d96034..cffc70257bd3 100644 > > > > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > > > @@ -29,7 +29,6 @@ > > > > #include <drm/drm_gem_framebuffer_helper.h> > > > > #include <drm/drm_irq.h> > > > > #include <drm/drm_of.h> > > > > -#include <drm/drm_panel.h> > > > > #include <drm/drm_probe_helper.h> > > > > #include <drm/drm_simple_kms_helper.h> > > > > #include <drm/drm_vblank.h> > > > > @@ -100,29 +99,11 @@ static void mxsfb_pipe_enable(struct > > > > drm_simple_display_pipe *pipe, > > > > struct drm_crtc_state *crtc_state, > > > > struct drm_plane_state *plane_state) > > > > { > > > > - struct drm_connector *connector; > > > > struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe); > > > > struct drm_device *drm = pipe->plane.dev; > > > > > > > > - if (!mxsfb->connector) { > > > > - list_for_each_entry(connector, > > > > - &drm->mode_config.connector_list, > > > > - head) > > > > - if (connector->encoder == &mxsfb->pipe.encoder) { > > > > - mxsfb->connector = connector; > > > > - break; > > > > - } > > > > - } > > > > - > > > > - if (!mxsfb->connector) { > > > > - dev_warn(drm->dev, "No connector attached, using default\n"); > > > > - mxsfb->connector = &mxsfb->panel_connector; > > > > - } > > > > - > > > > pm_runtime_get_sync(drm->dev); > > > > - drm_panel_prepare(mxsfb->panel); > > > > mxsfb_crtc_enable(mxsfb); > > > > - drm_panel_enable(mxsfb->panel); > > > > } > > > > > > > > static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) > > > > @@ -132,9 +113,7 @@ static void mxsfb_pipe_disable(struct > > > > drm_simple_display_pipe *pipe) > > > > struct drm_crtc *crtc = &pipe->crtc; > > > > struct drm_pending_vblank_event *event; > > > > > > > > - drm_panel_disable(mxsfb->panel); > > > > mxsfb_crtc_disable(mxsfb); > > > > - drm_panel_unprepare(mxsfb->panel); > > > > pm_runtime_put_sync(drm->dev); > > > > > > > > spin_lock_irq(&drm->event_lock); > > > > @@ -144,9 +123,6 @@ static void mxsfb_pipe_disable(struct > > > > drm_simple_display_pipe *pipe) > > > > drm_crtc_send_vblank_event(crtc, event); > > > > } > > > > spin_unlock_irq(&drm->event_lock); > > > > - > > > > - if (mxsfb->connector != &mxsfb->panel_connector) > > > > - mxsfb->connector = NULL; > > > > } > > > > > > > > static void mxsfb_pipe_update(struct drm_simple_display_pipe *pipe, > > > > @@ -190,6 +166,48 @@ static struct drm_simple_display_pipe_funcs mxsfb_funcs = { > > > > .disable_vblank = mxsfb_pipe_disable_vblank, > > > > }; > > > > > > > > +static int mxsfb_attach_bridge(struct mxsfb_drm_private *mxsfb) > > > > +{ > > > > + struct drm_device *drm = mxsfb->drm; > > > > + struct drm_connector_list_iter iter; > > > > + struct drm_panel *panel; > > > > + struct drm_bridge *bridge; > > > > + int ret; > > > > + > > > > + ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, &panel, > > > > + &bridge); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + if (panel) { > > > > + bridge = devm_drm_panel_bridge_add(drm->dev, panel); > > > > > > Do we need a select DRM_PANEL_BRIDGE in Kconfig maybe? From what I can > > > tell there is no devm_drm_panel_bridge_add() stub if DRM_PANEL_BRIDGE is > > > not selected. > > > > You're right, I'll add it. > > > > > > + if (IS_ERR(bridge)) > > > > + return PTR_ERR(bridge); > > > > + } > > > > + > > > > + if (!bridge) > > > > + return -ENODEV; > > > > + > > > > + ret = drm_simple_display_pipe_attach_bridge(&mxsfb->pipe, bridge); > > > > + if (ret) { > > > > + DRM_DEV_ERROR(drm->dev, > > > > + "failed to attach bridge: %d\n", ret); > > > > + return ret; > > > > + } > > > > + > > > > + mxsfb->bridge = bridge; > > > > + > > > > + /* > > > > + * Get hold of the connector. This is a bit of a hack, until the bridge > > > > + * API gives us bus flags and formats. > > > > + */ > > > > + drm_connector_list_iter_begin(drm, &iter); > > > > + mxsfb->connector = drm_connector_list_iter_next(&iter); > > > > + drm_connector_list_iter_end(&iter); > > > > > > Could we not just use drm_panel_bridge_connector() here? > > > > There's no guarantee that mxsfb->bridge is a panel bridge, the LCDIF > > output could be connected to another bridge in which case > > drm_of_find_panel_or_bridge() will set panel to NULL and bridge to a > > non-NULL value). > > > > > > + > > > > + return 0; > > > > +} > > > > + > > > > static int mxsfb_load(struct drm_device *drm, unsigned long flags) > > > > { > > > > struct platform_device *pdev = to_platform_device(drm->dev); > > > > @@ -201,6 +219,7 @@ static int mxsfb_load(struct drm_device *drm, > > > > unsigned long flags) > > > > if (!mxsfb) > > > > return -ENOMEM; > > > > > > > > + mxsfb->drm = drm; > > > > drm->dev_private = mxsfb; > > > > mxsfb->devdata = &mxsfb_devdata[pdev->id_entry->driver_data]; > > > > > > > > @@ -236,41 +255,17 @@ static int mxsfb_load(struct drm_device *drm, > > > > unsigned long flags) > > > > /* Modeset init */ > > > > drm_mode_config_init(drm); > > > > > > > > - ret = mxsfb_create_output(drm); > > > > - if (ret < 0) { > > > > - dev_err(drm->dev, "Failed to create outputs\n"); > > > > - goto err_vblank; > > > > - } > > > > - > > > > ret = drm_simple_display_pipe_init(drm, &mxsfb->pipe, &mxsfb_funcs, > > > > - mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL, > > > > - mxsfb->connector); > > > > + mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL, NULL); > > > > if (ret < 0) { > > > > dev_err(drm->dev, "Cannot setup simple display pipe\n"); > > > > goto err_vblank; > > > > } > > > > > > > > - /* > > > > - * Attach panel only if there is one. > > > > - * If there is no panel attach, it must be a bridge. In this case, we > > > > - * need a reference to its connector for a proper initialization. > > > > - * We will do this check in pipe->enable(), since the connector won't > > > > - * be attached to an encoder until then. > > > > - */ > > > > - > > > > - if (mxsfb->panel) { > > > > - ret = drm_panel_attach(mxsfb->panel, mxsfb->connector); > > > > - if (ret) { > > > > - dev_err(drm->dev, "Cannot connect panel: %d\n", ret); > > > > - goto err_vblank; > > > > - } > > > > - } else if (mxsfb->bridge) { > > > > - ret = drm_simple_display_pipe_attach_bridge(&mxsfb->pipe, > > > > - mxsfb->bridge); > > > > - if (ret) { > > > > - dev_err(drm->dev, "Cannot connect bridge: %d\n", ret); > > > > - goto err_vblank; > > > > - } > > > > + ret = mxsfb_attach_bridge(mxsfb); > > > > + if (ret) { > > > > + dev_err(drm->dev, "Cannot connect bridge: %d\n", ret); > > > > + goto err_vblank; > > > > } > > > > > > > > drm->mode_config.min_width = MXSFB_MIN_XRES; > > > > @@ -288,7 +283,7 @@ static int mxsfb_load(struct drm_device *drm, > > > > unsigned long flags) > > > > > > > > if (ret < 0) { > > > > dev_err(drm->dev, "Failed to install IRQ handler\n"); > > > > - goto err_irq; > > > > + goto err_vblank; > > > > } > > > > > > > > drm_kms_helper_poll_init(drm); > > > > @@ -299,8 +294,6 @@ static int mxsfb_load(struct drm_device *drm, > > > > unsigned long flags) > > > > > > > > return 0; > > > > > > > > -err_irq: > > > > - drm_panel_detach(mxsfb->panel); > > > > err_vblank: > > > > pm_runtime_disable(drm->dev); > > > > > > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > > > > b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > > > > index 0b65b5194a9c..0e3e5a63bbf9 100644 > > > > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > > > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > > > > @@ -8,6 +8,8 @@ > > > > #ifndef __MXSFB_DRV_H__ > > > > #define __MXSFB_DRV_H__ > > > > > > > > +struct drm_device; > > > > + > > > > struct mxsfb_devdata { > > > > unsigned int transfer_count; > > > > unsigned int cur_buf; > > > > @@ -26,10 +28,9 @@ struct mxsfb_drm_private { > > > > struct clk *clk_axi; > > > > struct clk *clk_disp_axi; > > > > > > > > + struct drm_device *drm; > > > > struct drm_simple_display_pipe pipe; > > > > - struct drm_connector panel_connector; > > > > struct drm_connector *connector; > > > > - struct drm_panel *panel; > > > > struct drm_bridge *bridge; > > > > }; > > > > > > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_out.c > > > > b/drivers/gpu/drm/mxsfb/mxsfb_out.c > > > > deleted file mode 100644 > > > > index 9eca1605d11d..000000000000 > > > > --- a/drivers/gpu/drm/mxsfb/mxsfb_out.c > > > > +++ /dev/null > > > > @@ -1,99 +0,0 @@ > > > > -// SPDX-License-Identifier: GPL-2.0-or-later > > > > -/* > > > > - * Copyright (C) 2016 Marek Vasut <marex@denx.de> > > > > - */ > > > > - > > > > -#include <linux/of_graph.h> > > > > - > > > > -#include <drm/drm_atomic.h> > > > > -#include <drm/drm_atomic_helper.h> > > > > -#include <drm/drm_crtc.h> > > > > -#include <drm/drm_fb_cma_helper.h> > > > > -#include <drm/drm_gem_cma_helper.h> > > > > -#include <drm/drm_of.h> > > > > -#include <drm/drm_panel.h> > > > > -#include <drm/drm_plane_helper.h> > > > > -#include <drm/drm_probe_helper.h> > > > > -#include <drm/drm_simple_kms_helper.h> > > > > - > > > > -#include "mxsfb_drv.h" > > > > - > > > > -static struct mxsfb_drm_private * > > > > -drm_connector_to_mxsfb_drm_private(struct drm_connector *connector) > > > > -{ > > > > - return container_of(connector, struct mxsfb_drm_private, > > > > - panel_connector); > > > > -} > > > > - > > > > -static int mxsfb_panel_get_modes(struct drm_connector *connector) > > > > -{ > > > > - struct mxsfb_drm_private *mxsfb = > > > > - drm_connector_to_mxsfb_drm_private(connector); > > > > - > > > > - if (mxsfb->panel) > > > > - return drm_panel_get_modes(mxsfb->panel, connector); > > > > - > > > > - return 0; > > > > -} > > > > - > > > > -static const struct > > > > -drm_connector_helper_funcs mxsfb_panel_connector_helper_funcs = { > > > > - .get_modes = mxsfb_panel_get_modes, > > > > -}; > > > > - > > > > -static enum drm_connector_status > > > > -mxsfb_panel_connector_detect(struct drm_connector *connector, bool force) > > > > -{ > > > > - struct mxsfb_drm_private *mxsfb = > > > > - drm_connector_to_mxsfb_drm_private(connector); > > > > - > > > > - if (mxsfb->panel) > > > > - return connector_status_connected; > > > > - > > > > - return connector_status_disconnected; > > > > -} > > > > - > > > > -static void mxsfb_panel_connector_destroy(struct drm_connector *connector) > > > > -{ > > > > - struct mxsfb_drm_private *mxsfb = > > > > - drm_connector_to_mxsfb_drm_private(connector); > > > > - > > > > - if (mxsfb->panel) > > > > - drm_panel_detach(mxsfb->panel); > > > > - > > > > - drm_connector_unregister(connector); > > > > - drm_connector_cleanup(connector); > > > > -} > > > > - > > > > -static const struct drm_connector_funcs mxsfb_panel_connector_funcs = { > > > > - .detect = mxsfb_panel_connector_detect, > > > > - .fill_modes = drm_helper_probe_single_connector_modes, > > > > - .destroy = mxsfb_panel_connector_destroy, > > > > - .reset = drm_atomic_helper_connector_reset, > > > > - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > > > > - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > > > > -}; > > > > - > > > > -int mxsfb_create_output(struct drm_device *drm) > > > > -{ > > > > - struct mxsfb_drm_private *mxsfb = drm->dev_private; > > > > - int ret; > > > > - > > > > - ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, > > > > - &mxsfb->panel, &mxsfb->bridge); > > > > - if (ret) > > > > - return ret; > > > > - > > > > - if (mxsfb->panel) { > > > > - mxsfb->connector = &mxsfb->panel_connector; > > > > - mxsfb->connector->dpms = DRM_MODE_DPMS_OFF; > > > > - mxsfb->connector->polled = 0; > > > > - drm_connector_helper_add(mxsfb->connector, > > > > - &mxsfb_panel_connector_helper_funcs); > > > > - ret = drm_connector_init(drm, mxsfb->connector, > > > > - &mxsfb_panel_connector_funcs, > > > > - DRM_MODE_CONNECTOR_Unknown); > > > > - } > > > > - > > > > - return ret; > > > > -}
Hi Stefan, On Tue, Jun 02, 2020 at 04:34:07PM +0200, Stefan Agner wrote: > On 2020-06-02 15:12, Daniel Vetter wrote: > > On Sat, May 30, 2020 at 05:14:21AM +0300, Laurent Pinchart wrote: > >> On Mon, Mar 23, 2020 at 10:27:21PM +0100, Stefan Agner wrote: > >>> On 2020-03-09 20:51, Laurent Pinchart wrote: > >>>> Replace the manual connector implementation based on drm_panel with the > >>>> drm_panel_bridge helper. This simplifies the mxsfb driver by removing > >>>> connector-related code, and standardizing all pipeline control > >>>> operations on bridges. > >>>> > >>>> A hack is needed to get hold of the connector, as that's our only source > >>>> of bus flags and formats for now. As soon as the bridge API provides us > >>>> with that information this can be fixed. > >>> > >>> This seems like a nice simplification. > >>> > >>> I gave this a go applied on today's drm-misc-next using a Colibri iMX7 > >>> (imx7d-colibri-emmc-eval-v3.dts device tree). This device makes use of > >>> the simple panel driver. I get this when booting: > >>> > >>> [ 2.976698] [drm] Supports vblank timestamp caching Rev 2 > >>> (21.10.2013). > >>> [ 2.983526] ------------[ cut here ]------------ > >>> [ 2.988180] WARNING: CPU: 0 PID: 1 at > >>> drivers/gpu/drm/bridge/panel.c:267 devm_drm_panel_bridge_add+0x25/0x28 > >>> [ 2.998059] Modules linked in: > >>> [ 3.001159] CPU: 0 PID: 1 Comm: swapper/0 Not tainted > >>> 5.6.0-rc5-yocto-standard-01250-ga4ce5db7c9f1 #29 > >>> [ 3.010493] Hardware name: Freescale i.MX7 Dual (Device Tree) > >>> [ 3.016281] [<c010f089>] (unwind_backtrace) from [<c010a9bb>] > >>> (show_stack+0xb/0xc) > >>> [ 3.023887] [<c010a9bb>] (show_stack) from [<c08b33cf>] > >>> (dump_stack+0x63/0x70) > >>> [ 3.031144] [<c08b33cf>] (dump_stack) from [<c0124385>] > >>> (__warn+0x9d/0xb0) > >>> [ 3.038047] [<c0124385>] (__warn) from [<c0124403>] > >>> (warn_slowpath_fmt+0x6b/0x70) > >>> [ 3.045564] [<c0124403>] (warn_slowpath_fmt) from [<c04ebf65>] > >>> (devm_drm_panel_bridge_add+0x25/0x28) > >>> [ 3.054736] [<c04ebf65>] (devm_drm_panel_bridge_add) from > >>> [<c051fe7f>] (mxsfb_probe+0x197/0x2e0) > >>> [ 3.063559] [<c051fe7f>] (mxsfb_probe) from [<c0532e91>] > >>> (platform_drv_probe+0x2d/0x60) > >>> [ 3.071598] [<c0532e91>] (platform_drv_probe) from [<c0531615>] > >>> (really_probe+0x1dd/0x330) > >>> [ 3.079897] [<c0531615>] (really_probe) from [<c05318b3>] > >>> (driver_probe_device+0x4f/0x154) > >>> [ 3.088195] [<c05318b3>] (driver_probe_device) from [<c0531b3f>] > >>> (device_driver_attach+0x37/0x44) > >>> [ 3.097101] [<c0531b3f>] (device_driver_attach) from [<c0531bc7>] > >>> (__driver_attach+0x7b/0xec) > >>> [ 3.105660] [<c0531bc7>] (__driver_attach) from [<c0530069>] > >>> (bus_for_each_dev+0x3d/0x64) > >>> [ 3.113870] [<c0530069>] (bus_for_each_dev) from [<c0530b9b>] > >>> (bus_add_driver+0xef/0x160) > >>> [ 3.122081] [<c0530b9b>] (bus_add_driver) from [<c053238d>] > >>> (driver_register+0x35/0x9c) > >>> [ 3.130119] [<c053238d>] (driver_register) from [<c0102de9>] > >>> (do_one_initcall+0x3d/0x1e4) > >>> [ 3.138333] [<c0102de9>] (do_one_initcall) from [<c1100d13>] > >>> (kernel_init_freeable+0x1b3/0x1fc) > >>> [ 3.147069] [<c1100d13>] (kernel_init_freeable) from [<c08c3f7b>] > >>> (kernel_init+0x7/0xd0) > >>> [ 3.155194] [<c08c3f7b>] (kernel_init) from [<c01010f9>] > >>> (ret_from_fork+0x11/0x38) > >>> [ 3.162789] Exception stack(0xec0e3fb0 to 0xec0e3ff8) > >>> [ 3.167862] 3fa0: 00000000 > >>> 00000000 00000000 00000000 > >>> [ 3.176071] 3fc0: 00000000 00000000 00000000 00000000 00000000 > >>> 00000000 00000000 00000000 > >>> [ 3.184278] 3fe0: 00000000 00000000 00000000 00000000 00000013 > >>> 00000000 > >>> [ 3.191029] ---[ end trace b69e1f44de470959 ]--- > >>> [ 3.195671] mxsfb 30730000.lcdif: Cannot connect bridge: -19 > >>> > >>> Should we maybe use devm_drm_panel_bridge_add_typed? > >> > >> As Sam reported, this is caused by the panel not setting connector_type. > >> We could use devm_drm_panel_bridge_add_typed(), even if I like the > >> warning as it uncovers the problematic panels and gets them fixed. What > >> would be your preference ? > > > > Adding warnings everywhere is kinda uncool, at least my experience is that > > if there's too much you get warning overload and train people to ignore > > them. > > > > Imo either hide the wwarning for now, or if it's not too much work, review > > all the panel drivers and make sure they set the connector type somewhere. > > Warnings are kinda ok once you're pretty sure you got them all, and want > > to make sure newly added drivers get this all right. But not before we've > > reached that. > > I am with Daniel on this, too many warnings make users blind of them, so > we should save them when really attention is needed. > > I guess the only question which connector type we should default to. I > think DRM_MODE_CONNECTOR_DPI make sense for this IP. Yes, that makes sense. I'll fix this in v3, but will wait until you review the remaining patches from v2 before sending v3 out. > >>> Two more comments below. > >>> > >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>>> --- > >>>> drivers/gpu/drm/mxsfb/Makefile | 2 +- > >>>> drivers/gpu/drm/mxsfb/mxsfb_drv.c | 105 ++++++++++++++---------------- > >>>> drivers/gpu/drm/mxsfb/mxsfb_drv.h | 5 +- > >>>> drivers/gpu/drm/mxsfb/mxsfb_out.c | 99 ---------------------------- > >>>> 4 files changed, 53 insertions(+), 158 deletions(-) > >>>> delete mode 100644 drivers/gpu/drm/mxsfb/mxsfb_out.c > >>>> > >>>> diff --git a/drivers/gpu/drm/mxsfb/Makefile b/drivers/gpu/drm/mxsfb/Makefile > >>>> index ff6e358088fa..811584e54ad1 100644 > >>>> --- a/drivers/gpu/drm/mxsfb/Makefile > >>>> +++ b/drivers/gpu/drm/mxsfb/Makefile > >>>> @@ -1,3 +1,3 @@ > >>>> # SPDX-License-Identifier: GPL-2.0-only > >>>> -mxsfb-y := mxsfb_drv.o mxsfb_crtc.o mxsfb_out.o > >>>> +mxsfb-y := mxsfb_drv.o mxsfb_crtc.o > >>>> obj-$(CONFIG_DRM_MXSFB) += mxsfb.o > >>>> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > >>>> b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > >>>> index 2e6068d96034..cffc70257bd3 100644 > >>>> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > >>>> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > >>>> @@ -29,7 +29,6 @@ > >>>> #include <drm/drm_gem_framebuffer_helper.h> > >>>> #include <drm/drm_irq.h> > >>>> #include <drm/drm_of.h> > >>>> -#include <drm/drm_panel.h> > >>>> #include <drm/drm_probe_helper.h> > >>>> #include <drm/drm_simple_kms_helper.h> > >>>> #include <drm/drm_vblank.h> > >>>> @@ -100,29 +99,11 @@ static void mxsfb_pipe_enable(struct > >>>> drm_simple_display_pipe *pipe, > >>>> struct drm_crtc_state *crtc_state, > >>>> struct drm_plane_state *plane_state) > >>>> { > >>>> - struct drm_connector *connector; > >>>> struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe); > >>>> struct drm_device *drm = pipe->plane.dev; > >>>> > >>>> - if (!mxsfb->connector) { > >>>> - list_for_each_entry(connector, > >>>> - &drm->mode_config.connector_list, > >>>> - head) > >>>> - if (connector->encoder == &mxsfb->pipe.encoder) { > >>>> - mxsfb->connector = connector; > >>>> - break; > >>>> - } > >>>> - } > >>>> - > >>>> - if (!mxsfb->connector) { > >>>> - dev_warn(drm->dev, "No connector attached, using default\n"); > >>>> - mxsfb->connector = &mxsfb->panel_connector; > >>>> - } > >>>> - > >>>> pm_runtime_get_sync(drm->dev); > >>>> - drm_panel_prepare(mxsfb->panel); > >>>> mxsfb_crtc_enable(mxsfb); > >>>> - drm_panel_enable(mxsfb->panel); > >>>> } > >>>> > >>>> static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) > >>>> @@ -132,9 +113,7 @@ static void mxsfb_pipe_disable(struct > >>>> drm_simple_display_pipe *pipe) > >>>> struct drm_crtc *crtc = &pipe->crtc; > >>>> struct drm_pending_vblank_event *event; > >>>> > >>>> - drm_panel_disable(mxsfb->panel); > >>>> mxsfb_crtc_disable(mxsfb); > >>>> - drm_panel_unprepare(mxsfb->panel); > >>>> pm_runtime_put_sync(drm->dev); > >>>> > >>>> spin_lock_irq(&drm->event_lock); > >>>> @@ -144,9 +123,6 @@ static void mxsfb_pipe_disable(struct > >>>> drm_simple_display_pipe *pipe) > >>>> drm_crtc_send_vblank_event(crtc, event); > >>>> } > >>>> spin_unlock_irq(&drm->event_lock); > >>>> - > >>>> - if (mxsfb->connector != &mxsfb->panel_connector) > >>>> - mxsfb->connector = NULL; > >>>> } > >>>> > >>>> static void mxsfb_pipe_update(struct drm_simple_display_pipe *pipe, > >>>> @@ -190,6 +166,48 @@ static struct drm_simple_display_pipe_funcs mxsfb_funcs = { > >>>> .disable_vblank = mxsfb_pipe_disable_vblank, > >>>> }; > >>>> > >>>> +static int mxsfb_attach_bridge(struct mxsfb_drm_private *mxsfb) > >>>> +{ > >>>> + struct drm_device *drm = mxsfb->drm; > >>>> + struct drm_connector_list_iter iter; > >>>> + struct drm_panel *panel; > >>>> + struct drm_bridge *bridge; > >>>> + int ret; > >>>> + > >>>> + ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, &panel, > >>>> + &bridge); > >>>> + if (ret) > >>>> + return ret; > >>>> + > >>>> + if (panel) { > >>>> + bridge = devm_drm_panel_bridge_add(drm->dev, panel); > >>> > >>> Do we need a select DRM_PANEL_BRIDGE in Kconfig maybe? From what I can > >>> tell there is no devm_drm_panel_bridge_add() stub if DRM_PANEL_BRIDGE is > >>> not selected. > >> > >> You're right, I'll add it. > >> > >>>> + if (IS_ERR(bridge)) > >>>> + return PTR_ERR(bridge); > >>>> + } > >>>> + > >>>> + if (!bridge) > >>>> + return -ENODEV; > >>>> + > >>>> + ret = drm_simple_display_pipe_attach_bridge(&mxsfb->pipe, bridge); > >>>> + if (ret) { > >>>> + DRM_DEV_ERROR(drm->dev, > >>>> + "failed to attach bridge: %d\n", ret); > >>>> + return ret; > >>>> + } > >>>> + > >>>> + mxsfb->bridge = bridge; > >>>> + > >>>> + /* > >>>> + * Get hold of the connector. This is a bit of a hack, until the bridge > >>>> + * API gives us bus flags and formats. > >>>> + */ > >>>> + drm_connector_list_iter_begin(drm, &iter); > >>>> + mxsfb->connector = drm_connector_list_iter_next(&iter); > >>>> + drm_connector_list_iter_end(&iter); > >>> > >>> Could we not just use drm_panel_bridge_connector() here? > >> > >> There's no guarantee that mxsfb->bridge is a panel bridge, the LCDIF > >> output could be connected to another bridge in which case > >> drm_of_find_panel_or_bridge() will set panel to NULL and bridge to a > >> non-NULL value). > >> > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> static int mxsfb_load(struct drm_device *drm, unsigned long flags) > >>>> { > >>>> struct platform_device *pdev = to_platform_device(drm->dev); > >>>> @@ -201,6 +219,7 @@ static int mxsfb_load(struct drm_device *drm, > >>>> unsigned long flags) > >>>> if (!mxsfb) > >>>> return -ENOMEM; > >>>> > >>>> + mxsfb->drm = drm; > >>>> drm->dev_private = mxsfb; > >>>> mxsfb->devdata = &mxsfb_devdata[pdev->id_entry->driver_data]; > >>>> > >>>> @@ -236,41 +255,17 @@ static int mxsfb_load(struct drm_device *drm, > >>>> unsigned long flags) > >>>> /* Modeset init */ > >>>> drm_mode_config_init(drm); > >>>> > >>>> - ret = mxsfb_create_output(drm); > >>>> - if (ret < 0) { > >>>> - dev_err(drm->dev, "Failed to create outputs\n"); > >>>> - goto err_vblank; > >>>> - } > >>>> - > >>>> ret = drm_simple_display_pipe_init(drm, &mxsfb->pipe, &mxsfb_funcs, > >>>> - mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL, > >>>> - mxsfb->connector); > >>>> + mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL, NULL); > >>>> if (ret < 0) { > >>>> dev_err(drm->dev, "Cannot setup simple display pipe\n"); > >>>> goto err_vblank; > >>>> } > >>>> > >>>> - /* > >>>> - * Attach panel only if there is one. > >>>> - * If there is no panel attach, it must be a bridge. In this case, we > >>>> - * need a reference to its connector for a proper initialization. > >>>> - * We will do this check in pipe->enable(), since the connector won't > >>>> - * be attached to an encoder until then. > >>>> - */ > >>>> - > >>>> - if (mxsfb->panel) { > >>>> - ret = drm_panel_attach(mxsfb->panel, mxsfb->connector); > >>>> - if (ret) { > >>>> - dev_err(drm->dev, "Cannot connect panel: %d\n", ret); > >>>> - goto err_vblank; > >>>> - } > >>>> - } else if (mxsfb->bridge) { > >>>> - ret = drm_simple_display_pipe_attach_bridge(&mxsfb->pipe, > >>>> - mxsfb->bridge); > >>>> - if (ret) { > >>>> - dev_err(drm->dev, "Cannot connect bridge: %d\n", ret); > >>>> - goto err_vblank; > >>>> - } > >>>> + ret = mxsfb_attach_bridge(mxsfb); > >>>> + if (ret) { > >>>> + dev_err(drm->dev, "Cannot connect bridge: %d\n", ret); > >>>> + goto err_vblank; > >>>> } > >>>> > >>>> drm->mode_config.min_width = MXSFB_MIN_XRES; > >>>> @@ -288,7 +283,7 @@ static int mxsfb_load(struct drm_device *drm, > >>>> unsigned long flags) > >>>> > >>>> if (ret < 0) { > >>>> dev_err(drm->dev, "Failed to install IRQ handler\n"); > >>>> - goto err_irq; > >>>> + goto err_vblank; > >>>> } > >>>> > >>>> drm_kms_helper_poll_init(drm); > >>>> @@ -299,8 +294,6 @@ static int mxsfb_load(struct drm_device *drm, > >>>> unsigned long flags) > >>>> > >>>> return 0; > >>>> > >>>> -err_irq: > >>>> - drm_panel_detach(mxsfb->panel); > >>>> err_vblank: > >>>> pm_runtime_disable(drm->dev); > >>>> > >>>> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > >>>> b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > >>>> index 0b65b5194a9c..0e3e5a63bbf9 100644 > >>>> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > >>>> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > >>>> @@ -8,6 +8,8 @@ > >>>> #ifndef __MXSFB_DRV_H__ > >>>> #define __MXSFB_DRV_H__ > >>>> > >>>> +struct drm_device; > >>>> + > >>>> struct mxsfb_devdata { > >>>> unsigned int transfer_count; > >>>> unsigned int cur_buf; > >>>> @@ -26,10 +28,9 @@ struct mxsfb_drm_private { > >>>> struct clk *clk_axi; > >>>> struct clk *clk_disp_axi; > >>>> > >>>> + struct drm_device *drm; > >>>> struct drm_simple_display_pipe pipe; > >>>> - struct drm_connector panel_connector; > >>>> struct drm_connector *connector; > >>>> - struct drm_panel *panel; > >>>> struct drm_bridge *bridge; > >>>> }; > >>>> > >>>> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_out.c > >>>> b/drivers/gpu/drm/mxsfb/mxsfb_out.c > >>>> deleted file mode 100644 > >>>> index 9eca1605d11d..000000000000 > >>>> --- a/drivers/gpu/drm/mxsfb/mxsfb_out.c > >>>> +++ /dev/null > >>>> @@ -1,99 +0,0 @@ > >>>> -// SPDX-License-Identifier: GPL-2.0-or-later > >>>> -/* > >>>> - * Copyright (C) 2016 Marek Vasut <marex@denx.de> > >>>> - */ > >>>> - > >>>> -#include <linux/of_graph.h> > >>>> - > >>>> -#include <drm/drm_atomic.h> > >>>> -#include <drm/drm_atomic_helper.h> > >>>> -#include <drm/drm_crtc.h> > >>>> -#include <drm/drm_fb_cma_helper.h> > >>>> -#include <drm/drm_gem_cma_helper.h> > >>>> -#include <drm/drm_of.h> > >>>> -#include <drm/drm_panel.h> > >>>> -#include <drm/drm_plane_helper.h> > >>>> -#include <drm/drm_probe_helper.h> > >>>> -#include <drm/drm_simple_kms_helper.h> > >>>> - > >>>> -#include "mxsfb_drv.h" > >>>> - > >>>> -static struct mxsfb_drm_private * > >>>> -drm_connector_to_mxsfb_drm_private(struct drm_connector *connector) > >>>> -{ > >>>> - return container_of(connector, struct mxsfb_drm_private, > >>>> - panel_connector); > >>>> -} > >>>> - > >>>> -static int mxsfb_panel_get_modes(struct drm_connector *connector) > >>>> -{ > >>>> - struct mxsfb_drm_private *mxsfb = > >>>> - drm_connector_to_mxsfb_drm_private(connector); > >>>> - > >>>> - if (mxsfb->panel) > >>>> - return drm_panel_get_modes(mxsfb->panel, connector); > >>>> - > >>>> - return 0; > >>>> -} > >>>> - > >>>> -static const struct > >>>> -drm_connector_helper_funcs mxsfb_panel_connector_helper_funcs = { > >>>> - .get_modes = mxsfb_panel_get_modes, > >>>> -}; > >>>> - > >>>> -static enum drm_connector_status > >>>> -mxsfb_panel_connector_detect(struct drm_connector *connector, bool force) > >>>> -{ > >>>> - struct mxsfb_drm_private *mxsfb = > >>>> - drm_connector_to_mxsfb_drm_private(connector); > >>>> - > >>>> - if (mxsfb->panel) > >>>> - return connector_status_connected; > >>>> - > >>>> - return connector_status_disconnected; > >>>> -} > >>>> - > >>>> -static void mxsfb_panel_connector_destroy(struct drm_connector *connector) > >>>> -{ > >>>> - struct mxsfb_drm_private *mxsfb = > >>>> - drm_connector_to_mxsfb_drm_private(connector); > >>>> - > >>>> - if (mxsfb->panel) > >>>> - drm_panel_detach(mxsfb->panel); > >>>> - > >>>> - drm_connector_unregister(connector); > >>>> - drm_connector_cleanup(connector); > >>>> -} > >>>> - > >>>> -static const struct drm_connector_funcs mxsfb_panel_connector_funcs = { > >>>> - .detect = mxsfb_panel_connector_detect, > >>>> - .fill_modes = drm_helper_probe_single_connector_modes, > >>>> - .destroy = mxsfb_panel_connector_destroy, > >>>> - .reset = drm_atomic_helper_connector_reset, > >>>> - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > >>>> - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > >>>> -}; > >>>> - > >>>> -int mxsfb_create_output(struct drm_device *drm) > >>>> -{ > >>>> - struct mxsfb_drm_private *mxsfb = drm->dev_private; > >>>> - int ret; > >>>> - > >>>> - ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, > >>>> - &mxsfb->panel, &mxsfb->bridge); > >>>> - if (ret) > >>>> - return ret; > >>>> - > >>>> - if (mxsfb->panel) { > >>>> - mxsfb->connector = &mxsfb->panel_connector; > >>>> - mxsfb->connector->dpms = DRM_MODE_DPMS_OFF; > >>>> - mxsfb->connector->polled = 0; > >>>> - drm_connector_helper_add(mxsfb->connector, > >>>> - &mxsfb_panel_connector_helper_funcs); > >>>> - ret = drm_connector_init(drm, mxsfb->connector, > >>>> - &mxsfb_panel_connector_funcs, > >>>> - DRM_MODE_CONNECTOR_Unknown); > >>>> - } > >>>> - > >>>> - return ret; > >>>> -}
On Tue, Jun 02, 2020 at 08:13:17PM +0300, Laurent Pinchart wrote: > Hi Daniel, > > On Tue, Jun 02, 2020 at 03:12:37PM +0200, Daniel Vetter wrote: > > On Sat, May 30, 2020 at 05:14:21AM +0300, Laurent Pinchart wrote: > > > On Mon, Mar 23, 2020 at 10:27:21PM +0100, Stefan Agner wrote: > > > > On 2020-03-09 20:51, Laurent Pinchart wrote: > > > > > Replace the manual connector implementation based on drm_panel with the > > > > > drm_panel_bridge helper. This simplifies the mxsfb driver by removing > > > > > connector-related code, and standardizing all pipeline control > > > > > operations on bridges. > > > > > > > > > > A hack is needed to get hold of the connector, as that's our only source > > > > > of bus flags and formats for now. As soon as the bridge API provides us > > > > > with that information this can be fixed. > > > > > > > > This seems like a nice simplification. > > > > > > > > I gave this a go applied on today's drm-misc-next using a Colibri iMX7 > > > > (imx7d-colibri-emmc-eval-v3.dts device tree). This device makes use of > > > > the simple panel driver. I get this when booting: > > > > > > > > [ 2.976698] [drm] Supports vblank timestamp caching Rev 2 > > > > (21.10.2013). > > > > [ 2.983526] ------------[ cut here ]------------ > > > > [ 2.988180] WARNING: CPU: 0 PID: 1 at > > > > drivers/gpu/drm/bridge/panel.c:267 devm_drm_panel_bridge_add+0x25/0x28 > > > > [ 2.998059] Modules linked in: > > > > [ 3.001159] CPU: 0 PID: 1 Comm: swapper/0 Not tainted > > > > 5.6.0-rc5-yocto-standard-01250-ga4ce5db7c9f1 #29 > > > > [ 3.010493] Hardware name: Freescale i.MX7 Dual (Device Tree) > > > > [ 3.016281] [<c010f089>] (unwind_backtrace) from [<c010a9bb>] > > > > (show_stack+0xb/0xc) > > > > [ 3.023887] [<c010a9bb>] (show_stack) from [<c08b33cf>] > > > > (dump_stack+0x63/0x70) > > > > [ 3.031144] [<c08b33cf>] (dump_stack) from [<c0124385>] > > > > (__warn+0x9d/0xb0) > > > > [ 3.038047] [<c0124385>] (__warn) from [<c0124403>] > > > > (warn_slowpath_fmt+0x6b/0x70) > > > > [ 3.045564] [<c0124403>] (warn_slowpath_fmt) from [<c04ebf65>] > > > > (devm_drm_panel_bridge_add+0x25/0x28) > > > > [ 3.054736] [<c04ebf65>] (devm_drm_panel_bridge_add) from > > > > [<c051fe7f>] (mxsfb_probe+0x197/0x2e0) > > > > [ 3.063559] [<c051fe7f>] (mxsfb_probe) from [<c0532e91>] > > > > (platform_drv_probe+0x2d/0x60) > > > > [ 3.071598] [<c0532e91>] (platform_drv_probe) from [<c0531615>] > > > > (really_probe+0x1dd/0x330) > > > > [ 3.079897] [<c0531615>] (really_probe) from [<c05318b3>] > > > > (driver_probe_device+0x4f/0x154) > > > > [ 3.088195] [<c05318b3>] (driver_probe_device) from [<c0531b3f>] > > > > (device_driver_attach+0x37/0x44) > > > > [ 3.097101] [<c0531b3f>] (device_driver_attach) from [<c0531bc7>] > > > > (__driver_attach+0x7b/0xec) > > > > [ 3.105660] [<c0531bc7>] (__driver_attach) from [<c0530069>] > > > > (bus_for_each_dev+0x3d/0x64) > > > > [ 3.113870] [<c0530069>] (bus_for_each_dev) from [<c0530b9b>] > > > > (bus_add_driver+0xef/0x160) > > > > [ 3.122081] [<c0530b9b>] (bus_add_driver) from [<c053238d>] > > > > (driver_register+0x35/0x9c) > > > > [ 3.130119] [<c053238d>] (driver_register) from [<c0102de9>] > > > > (do_one_initcall+0x3d/0x1e4) > > > > [ 3.138333] [<c0102de9>] (do_one_initcall) from [<c1100d13>] > > > > (kernel_init_freeable+0x1b3/0x1fc) > > > > [ 3.147069] [<c1100d13>] (kernel_init_freeable) from [<c08c3f7b>] > > > > (kernel_init+0x7/0xd0) > > > > [ 3.155194] [<c08c3f7b>] (kernel_init) from [<c01010f9>] > > > > (ret_from_fork+0x11/0x38) > > > > [ 3.162789] Exception stack(0xec0e3fb0 to 0xec0e3ff8) > > > > [ 3.167862] 3fa0: 00000000 > > > > 00000000 00000000 00000000 > > > > [ 3.176071] 3fc0: 00000000 00000000 00000000 00000000 00000000 > > > > 00000000 00000000 00000000 > > > > [ 3.184278] 3fe0: 00000000 00000000 00000000 00000000 00000013 > > > > 00000000 > > > > [ 3.191029] ---[ end trace b69e1f44de470959 ]--- > > > > [ 3.195671] mxsfb 30730000.lcdif: Cannot connect bridge: -19 > > > > > > > > Should we maybe use devm_drm_panel_bridge_add_typed? > > > > > > As Sam reported, this is caused by the panel not setting connector_type. > > > We could use devm_drm_panel_bridge_add_typed(), even if I like the > > > warning as it uncovers the problematic panels and gets them fixed. What > > > would be your preference ? > > > > Adding warnings everywhere is kinda uncool, at least my experience is that > > if there's too much you get warning overload and train people to ignore > > them. > > > > Imo either hide the wwarning for now, or if it's not too much work, review > > all the panel drivers and make sure they set the connector type somewhere. > > All panel drivers but panel-simple set the connector type. For > panel-simple, the type is stored in the panel_desc panel description, > and out of the 123 supported panels, only 48 have a connector type. I've > just sent a patch to fix this for the 7 DSI panels, so only 68 panels to > go :-) This will require trying to find the corresponding datasheets, so > that's a large effort for a single developer. That's why I was hoping a > WARN_ON() could help distributing the work :-) I hear your concern > though, and I'll set a default in this driver for the time being. I think some fallback in simple-panel.c with maybe a fixme would be ok for this case. I don't think this has to be perfect, afaik userspace at best just checks "is this a panel", not which specific type. -Daniel > > > Warnings are kinda ok once you're pretty sure you got them all, and want > > to make sure newly added drivers get this all right. But not before we've > > reached that. > > > > > > Two more comments below. > > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > --- > > > > > drivers/gpu/drm/mxsfb/Makefile | 2 +- > > > > > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 105 ++++++++++++++---------------- > > > > > drivers/gpu/drm/mxsfb/mxsfb_drv.h | 5 +- > > > > > drivers/gpu/drm/mxsfb/mxsfb_out.c | 99 ---------------------------- > > > > > 4 files changed, 53 insertions(+), 158 deletions(-) > > > > > delete mode 100644 drivers/gpu/drm/mxsfb/mxsfb_out.c > > > > > > > > > > diff --git a/drivers/gpu/drm/mxsfb/Makefile b/drivers/gpu/drm/mxsfb/Makefile > > > > > index ff6e358088fa..811584e54ad1 100644 > > > > > --- a/drivers/gpu/drm/mxsfb/Makefile > > > > > +++ b/drivers/gpu/drm/mxsfb/Makefile > > > > > @@ -1,3 +1,3 @@ > > > > > # SPDX-License-Identifier: GPL-2.0-only > > > > > -mxsfb-y := mxsfb_drv.o mxsfb_crtc.o mxsfb_out.o > > > > > +mxsfb-y := mxsfb_drv.o mxsfb_crtc.o > > > > > obj-$(CONFIG_DRM_MXSFB) += mxsfb.o > > > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > > > > b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > > > > index 2e6068d96034..cffc70257bd3 100644 > > > > > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > > > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > > > > @@ -29,7 +29,6 @@ > > > > > #include <drm/drm_gem_framebuffer_helper.h> > > > > > #include <drm/drm_irq.h> > > > > > #include <drm/drm_of.h> > > > > > -#include <drm/drm_panel.h> > > > > > #include <drm/drm_probe_helper.h> > > > > > #include <drm/drm_simple_kms_helper.h> > > > > > #include <drm/drm_vblank.h> > > > > > @@ -100,29 +99,11 @@ static void mxsfb_pipe_enable(struct > > > > > drm_simple_display_pipe *pipe, > > > > > struct drm_crtc_state *crtc_state, > > > > > struct drm_plane_state *plane_state) > > > > > { > > > > > - struct drm_connector *connector; > > > > > struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe); > > > > > struct drm_device *drm = pipe->plane.dev; > > > > > > > > > > - if (!mxsfb->connector) { > > > > > - list_for_each_entry(connector, > > > > > - &drm->mode_config.connector_list, > > > > > - head) > > > > > - if (connector->encoder == &mxsfb->pipe.encoder) { > > > > > - mxsfb->connector = connector; > > > > > - break; > > > > > - } > > > > > - } > > > > > - > > > > > - if (!mxsfb->connector) { > > > > > - dev_warn(drm->dev, "No connector attached, using default\n"); > > > > > - mxsfb->connector = &mxsfb->panel_connector; > > > > > - } > > > > > - > > > > > pm_runtime_get_sync(drm->dev); > > > > > - drm_panel_prepare(mxsfb->panel); > > > > > mxsfb_crtc_enable(mxsfb); > > > > > - drm_panel_enable(mxsfb->panel); > > > > > } > > > > > > > > > > static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) > > > > > @@ -132,9 +113,7 @@ static void mxsfb_pipe_disable(struct > > > > > drm_simple_display_pipe *pipe) > > > > > struct drm_crtc *crtc = &pipe->crtc; > > > > > struct drm_pending_vblank_event *event; > > > > > > > > > > - drm_panel_disable(mxsfb->panel); > > > > > mxsfb_crtc_disable(mxsfb); > > > > > - drm_panel_unprepare(mxsfb->panel); > > > > > pm_runtime_put_sync(drm->dev); > > > > > > > > > > spin_lock_irq(&drm->event_lock); > > > > > @@ -144,9 +123,6 @@ static void mxsfb_pipe_disable(struct > > > > > drm_simple_display_pipe *pipe) > > > > > drm_crtc_send_vblank_event(crtc, event); > > > > > } > > > > > spin_unlock_irq(&drm->event_lock); > > > > > - > > > > > - if (mxsfb->connector != &mxsfb->panel_connector) > > > > > - mxsfb->connector = NULL; > > > > > } > > > > > > > > > > static void mxsfb_pipe_update(struct drm_simple_display_pipe *pipe, > > > > > @@ -190,6 +166,48 @@ static struct drm_simple_display_pipe_funcs mxsfb_funcs = { > > > > > .disable_vblank = mxsfb_pipe_disable_vblank, > > > > > }; > > > > > > > > > > +static int mxsfb_attach_bridge(struct mxsfb_drm_private *mxsfb) > > > > > +{ > > > > > + struct drm_device *drm = mxsfb->drm; > > > > > + struct drm_connector_list_iter iter; > > > > > + struct drm_panel *panel; > > > > > + struct drm_bridge *bridge; > > > > > + int ret; > > > > > + > > > > > + ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, &panel, > > > > > + &bridge); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + if (panel) { > > > > > + bridge = devm_drm_panel_bridge_add(drm->dev, panel); > > > > > > > > Do we need a select DRM_PANEL_BRIDGE in Kconfig maybe? From what I can > > > > tell there is no devm_drm_panel_bridge_add() stub if DRM_PANEL_BRIDGE is > > > > not selected. > > > > > > You're right, I'll add it. > > > > > > > > + if (IS_ERR(bridge)) > > > > > + return PTR_ERR(bridge); > > > > > + } > > > > > + > > > > > + if (!bridge) > > > > > + return -ENODEV; > > > > > + > > > > > + ret = drm_simple_display_pipe_attach_bridge(&mxsfb->pipe, bridge); > > > > > + if (ret) { > > > > > + DRM_DEV_ERROR(drm->dev, > > > > > + "failed to attach bridge: %d\n", ret); > > > > > + return ret; > > > > > + } > > > > > + > > > > > + mxsfb->bridge = bridge; > > > > > + > > > > > + /* > > > > > + * Get hold of the connector. This is a bit of a hack, until the bridge > > > > > + * API gives us bus flags and formats. > > > > > + */ > > > > > + drm_connector_list_iter_begin(drm, &iter); > > > > > + mxsfb->connector = drm_connector_list_iter_next(&iter); > > > > > + drm_connector_list_iter_end(&iter); > > > > > > > > Could we not just use drm_panel_bridge_connector() here? > > > > > > There's no guarantee that mxsfb->bridge is a panel bridge, the LCDIF > > > output could be connected to another bridge in which case > > > drm_of_find_panel_or_bridge() will set panel to NULL and bridge to a > > > non-NULL value). > > > > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > static int mxsfb_load(struct drm_device *drm, unsigned long flags) > > > > > { > > > > > struct platform_device *pdev = to_platform_device(drm->dev); > > > > > @@ -201,6 +219,7 @@ static int mxsfb_load(struct drm_device *drm, > > > > > unsigned long flags) > > > > > if (!mxsfb) > > > > > return -ENOMEM; > > > > > > > > > > + mxsfb->drm = drm; > > > > > drm->dev_private = mxsfb; > > > > > mxsfb->devdata = &mxsfb_devdata[pdev->id_entry->driver_data]; > > > > > > > > > > @@ -236,41 +255,17 @@ static int mxsfb_load(struct drm_device *drm, > > > > > unsigned long flags) > > > > > /* Modeset init */ > > > > > drm_mode_config_init(drm); > > > > > > > > > > - ret = mxsfb_create_output(drm); > > > > > - if (ret < 0) { > > > > > - dev_err(drm->dev, "Failed to create outputs\n"); > > > > > - goto err_vblank; > > > > > - } > > > > > - > > > > > ret = drm_simple_display_pipe_init(drm, &mxsfb->pipe, &mxsfb_funcs, > > > > > - mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL, > > > > > - mxsfb->connector); > > > > > + mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL, NULL); > > > > > if (ret < 0) { > > > > > dev_err(drm->dev, "Cannot setup simple display pipe\n"); > > > > > goto err_vblank; > > > > > } > > > > > > > > > > - /* > > > > > - * Attach panel only if there is one. > > > > > - * If there is no panel attach, it must be a bridge. In this case, we > > > > > - * need a reference to its connector for a proper initialization. > > > > > - * We will do this check in pipe->enable(), since the connector won't > > > > > - * be attached to an encoder until then. > > > > > - */ > > > > > - > > > > > - if (mxsfb->panel) { > > > > > - ret = drm_panel_attach(mxsfb->panel, mxsfb->connector); > > > > > - if (ret) { > > > > > - dev_err(drm->dev, "Cannot connect panel: %d\n", ret); > > > > > - goto err_vblank; > > > > > - } > > > > > - } else if (mxsfb->bridge) { > > > > > - ret = drm_simple_display_pipe_attach_bridge(&mxsfb->pipe, > > > > > - mxsfb->bridge); > > > > > - if (ret) { > > > > > - dev_err(drm->dev, "Cannot connect bridge: %d\n", ret); > > > > > - goto err_vblank; > > > > > - } > > > > > + ret = mxsfb_attach_bridge(mxsfb); > > > > > + if (ret) { > > > > > + dev_err(drm->dev, "Cannot connect bridge: %d\n", ret); > > > > > + goto err_vblank; > > > > > } > > > > > > > > > > drm->mode_config.min_width = MXSFB_MIN_XRES; > > > > > @@ -288,7 +283,7 @@ static int mxsfb_load(struct drm_device *drm, > > > > > unsigned long flags) > > > > > > > > > > if (ret < 0) { > > > > > dev_err(drm->dev, "Failed to install IRQ handler\n"); > > > > > - goto err_irq; > > > > > + goto err_vblank; > > > > > } > > > > > > > > > > drm_kms_helper_poll_init(drm); > > > > > @@ -299,8 +294,6 @@ static int mxsfb_load(struct drm_device *drm, > > > > > unsigned long flags) > > > > > > > > > > return 0; > > > > > > > > > > -err_irq: > > > > > - drm_panel_detach(mxsfb->panel); > > > > > err_vblank: > > > > > pm_runtime_disable(drm->dev); > > > > > > > > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > > > > > b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > > > > > index 0b65b5194a9c..0e3e5a63bbf9 100644 > > > > > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > > > > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > > > > > @@ -8,6 +8,8 @@ > > > > > #ifndef __MXSFB_DRV_H__ > > > > > #define __MXSFB_DRV_H__ > > > > > > > > > > +struct drm_device; > > > > > + > > > > > struct mxsfb_devdata { > > > > > unsigned int transfer_count; > > > > > unsigned int cur_buf; > > > > > @@ -26,10 +28,9 @@ struct mxsfb_drm_private { > > > > > struct clk *clk_axi; > > > > > struct clk *clk_disp_axi; > > > > > > > > > > + struct drm_device *drm; > > > > > struct drm_simple_display_pipe pipe; > > > > > - struct drm_connector panel_connector; > > > > > struct drm_connector *connector; > > > > > - struct drm_panel *panel; > > > > > struct drm_bridge *bridge; > > > > > }; > > > > > > > > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_out.c > > > > > b/drivers/gpu/drm/mxsfb/mxsfb_out.c > > > > > deleted file mode 100644 > > > > > index 9eca1605d11d..000000000000 > > > > > --- a/drivers/gpu/drm/mxsfb/mxsfb_out.c > > > > > +++ /dev/null > > > > > @@ -1,99 +0,0 @@ > > > > > -// SPDX-License-Identifier: GPL-2.0-or-later > > > > > -/* > > > > > - * Copyright (C) 2016 Marek Vasut <marex@denx.de> > > > > > - */ > > > > > - > > > > > -#include <linux/of_graph.h> > > > > > - > > > > > -#include <drm/drm_atomic.h> > > > > > -#include <drm/drm_atomic_helper.h> > > > > > -#include <drm/drm_crtc.h> > > > > > -#include <drm/drm_fb_cma_helper.h> > > > > > -#include <drm/drm_gem_cma_helper.h> > > > > > -#include <drm/drm_of.h> > > > > > -#include <drm/drm_panel.h> > > > > > -#include <drm/drm_plane_helper.h> > > > > > -#include <drm/drm_probe_helper.h> > > > > > -#include <drm/drm_simple_kms_helper.h> > > > > > - > > > > > -#include "mxsfb_drv.h" > > > > > - > > > > > -static struct mxsfb_drm_private * > > > > > -drm_connector_to_mxsfb_drm_private(struct drm_connector *connector) > > > > > -{ > > > > > - return container_of(connector, struct mxsfb_drm_private, > > > > > - panel_connector); > > > > > -} > > > > > - > > > > > -static int mxsfb_panel_get_modes(struct drm_connector *connector) > > > > > -{ > > > > > - struct mxsfb_drm_private *mxsfb = > > > > > - drm_connector_to_mxsfb_drm_private(connector); > > > > > - > > > > > - if (mxsfb->panel) > > > > > - return drm_panel_get_modes(mxsfb->panel, connector); > > > > > - > > > > > - return 0; > > > > > -} > > > > > - > > > > > -static const struct > > > > > -drm_connector_helper_funcs mxsfb_panel_connector_helper_funcs = { > > > > > - .get_modes = mxsfb_panel_get_modes, > > > > > -}; > > > > > - > > > > > -static enum drm_connector_status > > > > > -mxsfb_panel_connector_detect(struct drm_connector *connector, bool force) > > > > > -{ > > > > > - struct mxsfb_drm_private *mxsfb = > > > > > - drm_connector_to_mxsfb_drm_private(connector); > > > > > - > > > > > - if (mxsfb->panel) > > > > > - return connector_status_connected; > > > > > - > > > > > - return connector_status_disconnected; > > > > > -} > > > > > - > > > > > -static void mxsfb_panel_connector_destroy(struct drm_connector *connector) > > > > > -{ > > > > > - struct mxsfb_drm_private *mxsfb = > > > > > - drm_connector_to_mxsfb_drm_private(connector); > > > > > - > > > > > - if (mxsfb->panel) > > > > > - drm_panel_detach(mxsfb->panel); > > > > > - > > > > > - drm_connector_unregister(connector); > > > > > - drm_connector_cleanup(connector); > > > > > -} > > > > > - > > > > > -static const struct drm_connector_funcs mxsfb_panel_connector_funcs = { > > > > > - .detect = mxsfb_panel_connector_detect, > > > > > - .fill_modes = drm_helper_probe_single_connector_modes, > > > > > - .destroy = mxsfb_panel_connector_destroy, > > > > > - .reset = drm_atomic_helper_connector_reset, > > > > > - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > > > > > - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > > > > > -}; > > > > > - > > > > > -int mxsfb_create_output(struct drm_device *drm) > > > > > -{ > > > > > - struct mxsfb_drm_private *mxsfb = drm->dev_private; > > > > > - int ret; > > > > > - > > > > > - ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, > > > > > - &mxsfb->panel, &mxsfb->bridge); > > > > > - if (ret) > > > > > - return ret; > > > > > - > > > > > - if (mxsfb->panel) { > > > > > - mxsfb->connector = &mxsfb->panel_connector; > > > > > - mxsfb->connector->dpms = DRM_MODE_DPMS_OFF; > > > > > - mxsfb->connector->polled = 0; > > > > > - drm_connector_helper_add(mxsfb->connector, > > > > > - &mxsfb_panel_connector_helper_funcs); > > > > > - ret = drm_connector_init(drm, mxsfb->connector, > > > > > - &mxsfb_panel_connector_funcs, > > > > > - DRM_MODE_CONNECTOR_Unknown); > > > > > - } > > > > > - > > > > > - return ret; > > > > > -} > > -- > Regards, > > Laurent Pinchart
On Wed, Jun 03, 2020 at 10:16:17AM +0200, Daniel Vetter wrote: > On Tue, Jun 02, 2020 at 08:13:17PM +0300, Laurent Pinchart wrote: > > Hi Daniel, > > > > On Tue, Jun 02, 2020 at 03:12:37PM +0200, Daniel Vetter wrote: > > > On Sat, May 30, 2020 at 05:14:21AM +0300, Laurent Pinchart wrote: > > > > On Mon, Mar 23, 2020 at 10:27:21PM +0100, Stefan Agner wrote: > > > > > On 2020-03-09 20:51, Laurent Pinchart wrote: > > > > > > Replace the manual connector implementation based on drm_panel with the > > > > > > drm_panel_bridge helper. This simplifies the mxsfb driver by removing > > > > > > connector-related code, and standardizing all pipeline control > > > > > > operations on bridges. > > > > > > > > > > > > A hack is needed to get hold of the connector, as that's our only source > > > > > > of bus flags and formats for now. As soon as the bridge API provides us > > > > > > with that information this can be fixed. > > > > > > > > > > This seems like a nice simplification. > > > > > > > > > > I gave this a go applied on today's drm-misc-next using a Colibri iMX7 > > > > > (imx7d-colibri-emmc-eval-v3.dts device tree). This device makes use of > > > > > the simple panel driver. I get this when booting: > > > > > > > > > > [ 2.976698] [drm] Supports vblank timestamp caching Rev 2 > > > > > (21.10.2013). > > > > > [ 2.983526] ------------[ cut here ]------------ > > > > > [ 2.988180] WARNING: CPU: 0 PID: 1 at > > > > > drivers/gpu/drm/bridge/panel.c:267 devm_drm_panel_bridge_add+0x25/0x28 > > > > > [ 2.998059] Modules linked in: > > > > > [ 3.001159] CPU: 0 PID: 1 Comm: swapper/0 Not tainted > > > > > 5.6.0-rc5-yocto-standard-01250-ga4ce5db7c9f1 #29 > > > > > [ 3.010493] Hardware name: Freescale i.MX7 Dual (Device Tree) > > > > > [ 3.016281] [<c010f089>] (unwind_backtrace) from [<c010a9bb>] > > > > > (show_stack+0xb/0xc) > > > > > [ 3.023887] [<c010a9bb>] (show_stack) from [<c08b33cf>] > > > > > (dump_stack+0x63/0x70) > > > > > [ 3.031144] [<c08b33cf>] (dump_stack) from [<c0124385>] > > > > > (__warn+0x9d/0xb0) > > > > > [ 3.038047] [<c0124385>] (__warn) from [<c0124403>] > > > > > (warn_slowpath_fmt+0x6b/0x70) > > > > > [ 3.045564] [<c0124403>] (warn_slowpath_fmt) from [<c04ebf65>] > > > > > (devm_drm_panel_bridge_add+0x25/0x28) > > > > > [ 3.054736] [<c04ebf65>] (devm_drm_panel_bridge_add) from > > > > > [<c051fe7f>] (mxsfb_probe+0x197/0x2e0) > > > > > [ 3.063559] [<c051fe7f>] (mxsfb_probe) from [<c0532e91>] > > > > > (platform_drv_probe+0x2d/0x60) > > > > > [ 3.071598] [<c0532e91>] (platform_drv_probe) from [<c0531615>] > > > > > (really_probe+0x1dd/0x330) > > > > > [ 3.079897] [<c0531615>] (really_probe) from [<c05318b3>] > > > > > (driver_probe_device+0x4f/0x154) > > > > > [ 3.088195] [<c05318b3>] (driver_probe_device) from [<c0531b3f>] > > > > > (device_driver_attach+0x37/0x44) > > > > > [ 3.097101] [<c0531b3f>] (device_driver_attach) from [<c0531bc7>] > > > > > (__driver_attach+0x7b/0xec) > > > > > [ 3.105660] [<c0531bc7>] (__driver_attach) from [<c0530069>] > > > > > (bus_for_each_dev+0x3d/0x64) > > > > > [ 3.113870] [<c0530069>] (bus_for_each_dev) from [<c0530b9b>] > > > > > (bus_add_driver+0xef/0x160) > > > > > [ 3.122081] [<c0530b9b>] (bus_add_driver) from [<c053238d>] > > > > > (driver_register+0x35/0x9c) > > > > > [ 3.130119] [<c053238d>] (driver_register) from [<c0102de9>] > > > > > (do_one_initcall+0x3d/0x1e4) > > > > > [ 3.138333] [<c0102de9>] (do_one_initcall) from [<c1100d13>] > > > > > (kernel_init_freeable+0x1b3/0x1fc) > > > > > [ 3.147069] [<c1100d13>] (kernel_init_freeable) from [<c08c3f7b>] > > > > > (kernel_init+0x7/0xd0) > > > > > [ 3.155194] [<c08c3f7b>] (kernel_init) from [<c01010f9>] > > > > > (ret_from_fork+0x11/0x38) > > > > > [ 3.162789] Exception stack(0xec0e3fb0 to 0xec0e3ff8) > > > > > [ 3.167862] 3fa0: 00000000 > > > > > 00000000 00000000 00000000 > > > > > [ 3.176071] 3fc0: 00000000 00000000 00000000 00000000 00000000 > > > > > 00000000 00000000 00000000 > > > > > [ 3.184278] 3fe0: 00000000 00000000 00000000 00000000 00000013 > > > > > 00000000 > > > > > [ 3.191029] ---[ end trace b69e1f44de470959 ]--- > > > > > [ 3.195671] mxsfb 30730000.lcdif: Cannot connect bridge: -19 > > > > > > > > > > Should we maybe use devm_drm_panel_bridge_add_typed? > > > > > > > > As Sam reported, this is caused by the panel not setting connector_type. > > > > We could use devm_drm_panel_bridge_add_typed(), even if I like the > > > > warning as it uncovers the problematic panels and gets them fixed. What > > > > would be your preference ? > > > > > > Adding warnings everywhere is kinda uncool, at least my experience is that > > > if there's too much you get warning overload and train people to ignore > > > them. > > > > > > Imo either hide the wwarning for now, or if it's not too much work, review > > > all the panel drivers and make sure they set the connector type somewhere. > > > > All panel drivers but panel-simple set the connector type. For > > panel-simple, the type is stored in the panel_desc panel description, > > and out of the 123 supported panels, only 48 have a connector type. I've > > just sent a patch to fix this for the 7 DSI panels, so only 68 panels to > > go :-) This will require trying to find the corresponding datasheets, so > > that's a large effort for a single developer. That's why I was hoping a > > WARN_ON() could help distributing the work :-) I hear your concern > > though, and I'll set a default in this driver for the time being. > > I think some fallback in simple-panel.c with maybe a fixme would be ok for > this case. I don't think this has to be perfect, afaik userspace at best > just checks "is this a panel", not which specific type. I just applied Laurent's patch for the DSI panels in panel-simple. In another patch-set posted as part of a bridge update I used DRM_MODE_CONNECTOR_DPI as the default for all panels in panel-simple that did not have any connector type specified. I think this matches what you suggest above. I will re-post the patch series when time permits. Sam > -Daniel > > > > > > Warnings are kinda ok once you're pretty sure you got them all, and want > > > to make sure newly added drivers get this all right. But not before we've > > > reached that. > > > > > > > > Two more comments below. > > > > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > --- > > > > > > drivers/gpu/drm/mxsfb/Makefile | 2 +- > > > > > > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 105 ++++++++++++++---------------- > > > > > > drivers/gpu/drm/mxsfb/mxsfb_drv.h | 5 +- > > > > > > drivers/gpu/drm/mxsfb/mxsfb_out.c | 99 ---------------------------- > > > > > > 4 files changed, 53 insertions(+), 158 deletions(-) > > > > > > delete mode 100644 drivers/gpu/drm/mxsfb/mxsfb_out.c > > > > > > > > > > > > diff --git a/drivers/gpu/drm/mxsfb/Makefile b/drivers/gpu/drm/mxsfb/Makefile > > > > > > index ff6e358088fa..811584e54ad1 100644 > > > > > > --- a/drivers/gpu/drm/mxsfb/Makefile > > > > > > +++ b/drivers/gpu/drm/mxsfb/Makefile > > > > > > @@ -1,3 +1,3 @@ > > > > > > # SPDX-License-Identifier: GPL-2.0-only > > > > > > -mxsfb-y := mxsfb_drv.o mxsfb_crtc.o mxsfb_out.o > > > > > > +mxsfb-y := mxsfb_drv.o mxsfb_crtc.o > > > > > > obj-$(CONFIG_DRM_MXSFB) += mxsfb.o > > > > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > > > > > b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > > > > > index 2e6068d96034..cffc70257bd3 100644 > > > > > > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > > > > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c > > > > > > @@ -29,7 +29,6 @@ > > > > > > #include <drm/drm_gem_framebuffer_helper.h> > > > > > > #include <drm/drm_irq.h> > > > > > > #include <drm/drm_of.h> > > > > > > -#include <drm/drm_panel.h> > > > > > > #include <drm/drm_probe_helper.h> > > > > > > #include <drm/drm_simple_kms_helper.h> > > > > > > #include <drm/drm_vblank.h> > > > > > > @@ -100,29 +99,11 @@ static void mxsfb_pipe_enable(struct > > > > > > drm_simple_display_pipe *pipe, > > > > > > struct drm_crtc_state *crtc_state, > > > > > > struct drm_plane_state *plane_state) > > > > > > { > > > > > > - struct drm_connector *connector; > > > > > > struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe); > > > > > > struct drm_device *drm = pipe->plane.dev; > > > > > > > > > > > > - if (!mxsfb->connector) { > > > > > > - list_for_each_entry(connector, > > > > > > - &drm->mode_config.connector_list, > > > > > > - head) > > > > > > - if (connector->encoder == &mxsfb->pipe.encoder) { > > > > > > - mxsfb->connector = connector; > > > > > > - break; > > > > > > - } > > > > > > - } > > > > > > - > > > > > > - if (!mxsfb->connector) { > > > > > > - dev_warn(drm->dev, "No connector attached, using default\n"); > > > > > > - mxsfb->connector = &mxsfb->panel_connector; > > > > > > - } > > > > > > - > > > > > > pm_runtime_get_sync(drm->dev); > > > > > > - drm_panel_prepare(mxsfb->panel); > > > > > > mxsfb_crtc_enable(mxsfb); > > > > > > - drm_panel_enable(mxsfb->panel); > > > > > > } > > > > > > > > > > > > static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) > > > > > > @@ -132,9 +113,7 @@ static void mxsfb_pipe_disable(struct > > > > > > drm_simple_display_pipe *pipe) > > > > > > struct drm_crtc *crtc = &pipe->crtc; > > > > > > struct drm_pending_vblank_event *event; > > > > > > > > > > > > - drm_panel_disable(mxsfb->panel); > > > > > > mxsfb_crtc_disable(mxsfb); > > > > > > - drm_panel_unprepare(mxsfb->panel); > > > > > > pm_runtime_put_sync(drm->dev); > > > > > > > > > > > > spin_lock_irq(&drm->event_lock); > > > > > > @@ -144,9 +123,6 @@ static void mxsfb_pipe_disable(struct > > > > > > drm_simple_display_pipe *pipe) > > > > > > drm_crtc_send_vblank_event(crtc, event); > > > > > > } > > > > > > spin_unlock_irq(&drm->event_lock); > > > > > > - > > > > > > - if (mxsfb->connector != &mxsfb->panel_connector) > > > > > > - mxsfb->connector = NULL; > > > > > > } > > > > > > > > > > > > static void mxsfb_pipe_update(struct drm_simple_display_pipe *pipe, > > > > > > @@ -190,6 +166,48 @@ static struct drm_simple_display_pipe_funcs mxsfb_funcs = { > > > > > > .disable_vblank = mxsfb_pipe_disable_vblank, > > > > > > }; > > > > > > > > > > > > +static int mxsfb_attach_bridge(struct mxsfb_drm_private *mxsfb) > > > > > > +{ > > > > > > + struct drm_device *drm = mxsfb->drm; > > > > > > + struct drm_connector_list_iter iter; > > > > > > + struct drm_panel *panel; > > > > > > + struct drm_bridge *bridge; > > > > > > + int ret; > > > > > > + > > > > > > + ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, &panel, > > > > > > + &bridge); > > > > > > + if (ret) > > > > > > + return ret; > > > > > > + > > > > > > + if (panel) { > > > > > > + bridge = devm_drm_panel_bridge_add(drm->dev, panel); > > > > > > > > > > Do we need a select DRM_PANEL_BRIDGE in Kconfig maybe? From what I can > > > > > tell there is no devm_drm_panel_bridge_add() stub if DRM_PANEL_BRIDGE is > > > > > not selected. > > > > > > > > You're right, I'll add it. > > > > > > > > > > + if (IS_ERR(bridge)) > > > > > > + return PTR_ERR(bridge); > > > > > > + } > > > > > > + > > > > > > + if (!bridge) > > > > > > + return -ENODEV; > > > > > > + > > > > > > + ret = drm_simple_display_pipe_attach_bridge(&mxsfb->pipe, bridge); > > > > > > + if (ret) { > > > > > > + DRM_DEV_ERROR(drm->dev, > > > > > > + "failed to attach bridge: %d\n", ret); > > > > > > + return ret; > > > > > > + } > > > > > > + > > > > > > + mxsfb->bridge = bridge; > > > > > > + > > > > > > + /* > > > > > > + * Get hold of the connector. This is a bit of a hack, until the bridge > > > > > > + * API gives us bus flags and formats. > > > > > > + */ > > > > > > + drm_connector_list_iter_begin(drm, &iter); > > > > > > + mxsfb->connector = drm_connector_list_iter_next(&iter); > > > > > > + drm_connector_list_iter_end(&iter); > > > > > > > > > > Could we not just use drm_panel_bridge_connector() here? > > > > > > > > There's no guarantee that mxsfb->bridge is a panel bridge, the LCDIF > > > > output could be connected to another bridge in which case > > > > drm_of_find_panel_or_bridge() will set panel to NULL and bridge to a > > > > non-NULL value). > > > > > > > > > > + > > > > > > + return 0; > > > > > > +} > > > > > > + > > > > > > static int mxsfb_load(struct drm_device *drm, unsigned long flags) > > > > > > { > > > > > > struct platform_device *pdev = to_platform_device(drm->dev); > > > > > > @@ -201,6 +219,7 @@ static int mxsfb_load(struct drm_device *drm, > > > > > > unsigned long flags) > > > > > > if (!mxsfb) > > > > > > return -ENOMEM; > > > > > > > > > > > > + mxsfb->drm = drm; > > > > > > drm->dev_private = mxsfb; > > > > > > mxsfb->devdata = &mxsfb_devdata[pdev->id_entry->driver_data]; > > > > > > > > > > > > @@ -236,41 +255,17 @@ static int mxsfb_load(struct drm_device *drm, > > > > > > unsigned long flags) > > > > > > /* Modeset init */ > > > > > > drm_mode_config_init(drm); > > > > > > > > > > > > - ret = mxsfb_create_output(drm); > > > > > > - if (ret < 0) { > > > > > > - dev_err(drm->dev, "Failed to create outputs\n"); > > > > > > - goto err_vblank; > > > > > > - } > > > > > > - > > > > > > ret = drm_simple_display_pipe_init(drm, &mxsfb->pipe, &mxsfb_funcs, > > > > > > - mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL, > > > > > > - mxsfb->connector); > > > > > > + mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL, NULL); > > > > > > if (ret < 0) { > > > > > > dev_err(drm->dev, "Cannot setup simple display pipe\n"); > > > > > > goto err_vblank; > > > > > > } > > > > > > > > > > > > - /* > > > > > > - * Attach panel only if there is one. > > > > > > - * If there is no panel attach, it must be a bridge. In this case, we > > > > > > - * need a reference to its connector for a proper initialization. > > > > > > - * We will do this check in pipe->enable(), since the connector won't > > > > > > - * be attached to an encoder until then. > > > > > > - */ > > > > > > - > > > > > > - if (mxsfb->panel) { > > > > > > - ret = drm_panel_attach(mxsfb->panel, mxsfb->connector); > > > > > > - if (ret) { > > > > > > - dev_err(drm->dev, "Cannot connect panel: %d\n", ret); > > > > > > - goto err_vblank; > > > > > > - } > > > > > > - } else if (mxsfb->bridge) { > > > > > > - ret = drm_simple_display_pipe_attach_bridge(&mxsfb->pipe, > > > > > > - mxsfb->bridge); > > > > > > - if (ret) { > > > > > > - dev_err(drm->dev, "Cannot connect bridge: %d\n", ret); > > > > > > - goto err_vblank; > > > > > > - } > > > > > > + ret = mxsfb_attach_bridge(mxsfb); > > > > > > + if (ret) { > > > > > > + dev_err(drm->dev, "Cannot connect bridge: %d\n", ret); > > > > > > + goto err_vblank; > > > > > > } > > > > > > > > > > > > drm->mode_config.min_width = MXSFB_MIN_XRES; > > > > > > @@ -288,7 +283,7 @@ static int mxsfb_load(struct drm_device *drm, > > > > > > unsigned long flags) > > > > > > > > > > > > if (ret < 0) { > > > > > > dev_err(drm->dev, "Failed to install IRQ handler\n"); > > > > > > - goto err_irq; > > > > > > + goto err_vblank; > > > > > > } > > > > > > > > > > > > drm_kms_helper_poll_init(drm); > > > > > > @@ -299,8 +294,6 @@ static int mxsfb_load(struct drm_device *drm, > > > > > > unsigned long flags) > > > > > > > > > > > > return 0; > > > > > > > > > > > > -err_irq: > > > > > > - drm_panel_detach(mxsfb->panel); > > > > > > err_vblank: > > > > > > pm_runtime_disable(drm->dev); > > > > > > > > > > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > > > > > > b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > > > > > > index 0b65b5194a9c..0e3e5a63bbf9 100644 > > > > > > --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h > > > > > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h > > > > > > @@ -8,6 +8,8 @@ > > > > > > #ifndef __MXSFB_DRV_H__ > > > > > > #define __MXSFB_DRV_H__ > > > > > > > > > > > > +struct drm_device; > > > > > > + > > > > > > struct mxsfb_devdata { > > > > > > unsigned int transfer_count; > > > > > > unsigned int cur_buf; > > > > > > @@ -26,10 +28,9 @@ struct mxsfb_drm_private { > > > > > > struct clk *clk_axi; > > > > > > struct clk *clk_disp_axi; > > > > > > > > > > > > + struct drm_device *drm; > > > > > > struct drm_simple_display_pipe pipe; > > > > > > - struct drm_connector panel_connector; > > > > > > struct drm_connector *connector; > > > > > > - struct drm_panel *panel; > > > > > > struct drm_bridge *bridge; > > > > > > }; > > > > > > > > > > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_out.c > > > > > > b/drivers/gpu/drm/mxsfb/mxsfb_out.c > > > > > > deleted file mode 100644 > > > > > > index 9eca1605d11d..000000000000 > > > > > > --- a/drivers/gpu/drm/mxsfb/mxsfb_out.c > > > > > > +++ /dev/null > > > > > > @@ -1,99 +0,0 @@ > > > > > > -// SPDX-License-Identifier: GPL-2.0-or-later > > > > > > -/* > > > > > > - * Copyright (C) 2016 Marek Vasut <marex@denx.de> > > > > > > - */ > > > > > > - > > > > > > -#include <linux/of_graph.h> > > > > > > - > > > > > > -#include <drm/drm_atomic.h> > > > > > > -#include <drm/drm_atomic_helper.h> > > > > > > -#include <drm/drm_crtc.h> > > > > > > -#include <drm/drm_fb_cma_helper.h> > > > > > > -#include <drm/drm_gem_cma_helper.h> > > > > > > -#include <drm/drm_of.h> > > > > > > -#include <drm/drm_panel.h> > > > > > > -#include <drm/drm_plane_helper.h> > > > > > > -#include <drm/drm_probe_helper.h> > > > > > > -#include <drm/drm_simple_kms_helper.h> > > > > > > - > > > > > > -#include "mxsfb_drv.h" > > > > > > - > > > > > > -static struct mxsfb_drm_private * > > > > > > -drm_connector_to_mxsfb_drm_private(struct drm_connector *connector) > > > > > > -{ > > > > > > - return container_of(connector, struct mxsfb_drm_private, > > > > > > - panel_connector); > > > > > > -} > > > > > > - > > > > > > -static int mxsfb_panel_get_modes(struct drm_connector *connector) > > > > > > -{ > > > > > > - struct mxsfb_drm_private *mxsfb = > > > > > > - drm_connector_to_mxsfb_drm_private(connector); > > > > > > - > > > > > > - if (mxsfb->panel) > > > > > > - return drm_panel_get_modes(mxsfb->panel, connector); > > > > > > - > > > > > > - return 0; > > > > > > -} > > > > > > - > > > > > > -static const struct > > > > > > -drm_connector_helper_funcs mxsfb_panel_connector_helper_funcs = { > > > > > > - .get_modes = mxsfb_panel_get_modes, > > > > > > -}; > > > > > > - > > > > > > -static enum drm_connector_status > > > > > > -mxsfb_panel_connector_detect(struct drm_connector *connector, bool force) > > > > > > -{ > > > > > > - struct mxsfb_drm_private *mxsfb = > > > > > > - drm_connector_to_mxsfb_drm_private(connector); > > > > > > - > > > > > > - if (mxsfb->panel) > > > > > > - return connector_status_connected; > > > > > > - > > > > > > - return connector_status_disconnected; > > > > > > -} > > > > > > - > > > > > > -static void mxsfb_panel_connector_destroy(struct drm_connector *connector) > > > > > > -{ > > > > > > - struct mxsfb_drm_private *mxsfb = > > > > > > - drm_connector_to_mxsfb_drm_private(connector); > > > > > > - > > > > > > - if (mxsfb->panel) > > > > > > - drm_panel_detach(mxsfb->panel); > > > > > > - > > > > > > - drm_connector_unregister(connector); > > > > > > - drm_connector_cleanup(connector); > > > > > > -} > > > > > > - > > > > > > -static const struct drm_connector_funcs mxsfb_panel_connector_funcs = { > > > > > > - .detect = mxsfb_panel_connector_detect, > > > > > > - .fill_modes = drm_helper_probe_single_connector_modes, > > > > > > - .destroy = mxsfb_panel_connector_destroy, > > > > > > - .reset = drm_atomic_helper_connector_reset, > > > > > > - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > > > > > > - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > > > > > > -}; > > > > > > - > > > > > > -int mxsfb_create_output(struct drm_device *drm) > > > > > > -{ > > > > > > - struct mxsfb_drm_private *mxsfb = drm->dev_private; > > > > > > - int ret; > > > > > > - > > > > > > - ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, > > > > > > - &mxsfb->panel, &mxsfb->bridge); > > > > > > - if (ret) > > > > > > - return ret; > > > > > > - > > > > > > - if (mxsfb->panel) { > > > > > > - mxsfb->connector = &mxsfb->panel_connector; > > > > > > - mxsfb->connector->dpms = DRM_MODE_DPMS_OFF; > > > > > > - mxsfb->connector->polled = 0; > > > > > > - drm_connector_helper_add(mxsfb->connector, > > > > > > - &mxsfb_panel_connector_helper_funcs); > > > > > > - ret = drm_connector_init(drm, mxsfb->connector, > > > > > > - &mxsfb_panel_connector_funcs, > > > > > > - DRM_MODE_CONNECTOR_Unknown); > > > > > > - } > > > > > > - > > > > > > - return ret; > > > > > > -} > > > > -- > > Regards, > > > > Laurent Pinchart > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/mxsfb/Makefile b/drivers/gpu/drm/mxsfb/Makefile index ff6e358088fa..811584e54ad1 100644 --- a/drivers/gpu/drm/mxsfb/Makefile +++ b/drivers/gpu/drm/mxsfb/Makefile @@ -1,3 +1,3 @@ # SPDX-License-Identifier: GPL-2.0-only -mxsfb-y := mxsfb_drv.o mxsfb_crtc.o mxsfb_out.o +mxsfb-y := mxsfb_drv.o mxsfb_crtc.o obj-$(CONFIG_DRM_MXSFB) += mxsfb.o diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c b/drivers/gpu/drm/mxsfb/mxsfb_drv.c index 2e6068d96034..cffc70257bd3 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c @@ -29,7 +29,6 @@ #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_irq.h> #include <drm/drm_of.h> -#include <drm/drm_panel.h> #include <drm/drm_probe_helper.h> #include <drm/drm_simple_kms_helper.h> #include <drm/drm_vblank.h> @@ -100,29 +99,11 @@ static void mxsfb_pipe_enable(struct drm_simple_display_pipe *pipe, struct drm_crtc_state *crtc_state, struct drm_plane_state *plane_state) { - struct drm_connector *connector; struct mxsfb_drm_private *mxsfb = drm_pipe_to_mxsfb_drm_private(pipe); struct drm_device *drm = pipe->plane.dev; - if (!mxsfb->connector) { - list_for_each_entry(connector, - &drm->mode_config.connector_list, - head) - if (connector->encoder == &mxsfb->pipe.encoder) { - mxsfb->connector = connector; - break; - } - } - - if (!mxsfb->connector) { - dev_warn(drm->dev, "No connector attached, using default\n"); - mxsfb->connector = &mxsfb->panel_connector; - } - pm_runtime_get_sync(drm->dev); - drm_panel_prepare(mxsfb->panel); mxsfb_crtc_enable(mxsfb); - drm_panel_enable(mxsfb->panel); } static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) @@ -132,9 +113,7 @@ static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) struct drm_crtc *crtc = &pipe->crtc; struct drm_pending_vblank_event *event; - drm_panel_disable(mxsfb->panel); mxsfb_crtc_disable(mxsfb); - drm_panel_unprepare(mxsfb->panel); pm_runtime_put_sync(drm->dev); spin_lock_irq(&drm->event_lock); @@ -144,9 +123,6 @@ static void mxsfb_pipe_disable(struct drm_simple_display_pipe *pipe) drm_crtc_send_vblank_event(crtc, event); } spin_unlock_irq(&drm->event_lock); - - if (mxsfb->connector != &mxsfb->panel_connector) - mxsfb->connector = NULL; } static void mxsfb_pipe_update(struct drm_simple_display_pipe *pipe, @@ -190,6 +166,48 @@ static struct drm_simple_display_pipe_funcs mxsfb_funcs = { .disable_vblank = mxsfb_pipe_disable_vblank, }; +static int mxsfb_attach_bridge(struct mxsfb_drm_private *mxsfb) +{ + struct drm_device *drm = mxsfb->drm; + struct drm_connector_list_iter iter; + struct drm_panel *panel; + struct drm_bridge *bridge; + int ret; + + ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, &panel, + &bridge); + if (ret) + return ret; + + if (panel) { + bridge = devm_drm_panel_bridge_add(drm->dev, panel); + if (IS_ERR(bridge)) + return PTR_ERR(bridge); + } + + if (!bridge) + return -ENODEV; + + ret = drm_simple_display_pipe_attach_bridge(&mxsfb->pipe, bridge); + if (ret) { + DRM_DEV_ERROR(drm->dev, + "failed to attach bridge: %d\n", ret); + return ret; + } + + mxsfb->bridge = bridge; + + /* + * Get hold of the connector. This is a bit of a hack, until the bridge + * API gives us bus flags and formats. + */ + drm_connector_list_iter_begin(drm, &iter); + mxsfb->connector = drm_connector_list_iter_next(&iter); + drm_connector_list_iter_end(&iter); + + return 0; +} + static int mxsfb_load(struct drm_device *drm, unsigned long flags) { struct platform_device *pdev = to_platform_device(drm->dev); @@ -201,6 +219,7 @@ static int mxsfb_load(struct drm_device *drm, unsigned long flags) if (!mxsfb) return -ENOMEM; + mxsfb->drm = drm; drm->dev_private = mxsfb; mxsfb->devdata = &mxsfb_devdata[pdev->id_entry->driver_data]; @@ -236,41 +255,17 @@ static int mxsfb_load(struct drm_device *drm, unsigned long flags) /* Modeset init */ drm_mode_config_init(drm); - ret = mxsfb_create_output(drm); - if (ret < 0) { - dev_err(drm->dev, "Failed to create outputs\n"); - goto err_vblank; - } - ret = drm_simple_display_pipe_init(drm, &mxsfb->pipe, &mxsfb_funcs, - mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL, - mxsfb->connector); + mxsfb_formats, ARRAY_SIZE(mxsfb_formats), NULL, NULL); if (ret < 0) { dev_err(drm->dev, "Cannot setup simple display pipe\n"); goto err_vblank; } - /* - * Attach panel only if there is one. - * If there is no panel attach, it must be a bridge. In this case, we - * need a reference to its connector for a proper initialization. - * We will do this check in pipe->enable(), since the connector won't - * be attached to an encoder until then. - */ - - if (mxsfb->panel) { - ret = drm_panel_attach(mxsfb->panel, mxsfb->connector); - if (ret) { - dev_err(drm->dev, "Cannot connect panel: %d\n", ret); - goto err_vblank; - } - } else if (mxsfb->bridge) { - ret = drm_simple_display_pipe_attach_bridge(&mxsfb->pipe, - mxsfb->bridge); - if (ret) { - dev_err(drm->dev, "Cannot connect bridge: %d\n", ret); - goto err_vblank; - } + ret = mxsfb_attach_bridge(mxsfb); + if (ret) { + dev_err(drm->dev, "Cannot connect bridge: %d\n", ret); + goto err_vblank; } drm->mode_config.min_width = MXSFB_MIN_XRES; @@ -288,7 +283,7 @@ static int mxsfb_load(struct drm_device *drm, unsigned long flags) if (ret < 0) { dev_err(drm->dev, "Failed to install IRQ handler\n"); - goto err_irq; + goto err_vblank; } drm_kms_helper_poll_init(drm); @@ -299,8 +294,6 @@ static int mxsfb_load(struct drm_device *drm, unsigned long flags) return 0; -err_irq: - drm_panel_detach(mxsfb->panel); err_vblank: pm_runtime_disable(drm->dev); diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.h b/drivers/gpu/drm/mxsfb/mxsfb_drv.h index 0b65b5194a9c..0e3e5a63bbf9 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.h +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.h @@ -8,6 +8,8 @@ #ifndef __MXSFB_DRV_H__ #define __MXSFB_DRV_H__ +struct drm_device; + struct mxsfb_devdata { unsigned int transfer_count; unsigned int cur_buf; @@ -26,10 +28,9 @@ struct mxsfb_drm_private { struct clk *clk_axi; struct clk *clk_disp_axi; + struct drm_device *drm; struct drm_simple_display_pipe pipe; - struct drm_connector panel_connector; struct drm_connector *connector; - struct drm_panel *panel; struct drm_bridge *bridge; }; diff --git a/drivers/gpu/drm/mxsfb/mxsfb_out.c b/drivers/gpu/drm/mxsfb/mxsfb_out.c deleted file mode 100644 index 9eca1605d11d..000000000000 --- a/drivers/gpu/drm/mxsfb/mxsfb_out.c +++ /dev/null @@ -1,99 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-or-later -/* - * Copyright (C) 2016 Marek Vasut <marex@denx.de> - */ - -#include <linux/of_graph.h> - -#include <drm/drm_atomic.h> -#include <drm/drm_atomic_helper.h> -#include <drm/drm_crtc.h> -#include <drm/drm_fb_cma_helper.h> -#include <drm/drm_gem_cma_helper.h> -#include <drm/drm_of.h> -#include <drm/drm_panel.h> -#include <drm/drm_plane_helper.h> -#include <drm/drm_probe_helper.h> -#include <drm/drm_simple_kms_helper.h> - -#include "mxsfb_drv.h" - -static struct mxsfb_drm_private * -drm_connector_to_mxsfb_drm_private(struct drm_connector *connector) -{ - return container_of(connector, struct mxsfb_drm_private, - panel_connector); -} - -static int mxsfb_panel_get_modes(struct drm_connector *connector) -{ - struct mxsfb_drm_private *mxsfb = - drm_connector_to_mxsfb_drm_private(connector); - - if (mxsfb->panel) - return drm_panel_get_modes(mxsfb->panel, connector); - - return 0; -} - -static const struct -drm_connector_helper_funcs mxsfb_panel_connector_helper_funcs = { - .get_modes = mxsfb_panel_get_modes, -}; - -static enum drm_connector_status -mxsfb_panel_connector_detect(struct drm_connector *connector, bool force) -{ - struct mxsfb_drm_private *mxsfb = - drm_connector_to_mxsfb_drm_private(connector); - - if (mxsfb->panel) - return connector_status_connected; - - return connector_status_disconnected; -} - -static void mxsfb_panel_connector_destroy(struct drm_connector *connector) -{ - struct mxsfb_drm_private *mxsfb = - drm_connector_to_mxsfb_drm_private(connector); - - if (mxsfb->panel) - drm_panel_detach(mxsfb->panel); - - drm_connector_unregister(connector); - drm_connector_cleanup(connector); -} - -static const struct drm_connector_funcs mxsfb_panel_connector_funcs = { - .detect = mxsfb_panel_connector_detect, - .fill_modes = drm_helper_probe_single_connector_modes, - .destroy = mxsfb_panel_connector_destroy, - .reset = drm_atomic_helper_connector_reset, - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, -}; - -int mxsfb_create_output(struct drm_device *drm) -{ - struct mxsfb_drm_private *mxsfb = drm->dev_private; - int ret; - - ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, - &mxsfb->panel, &mxsfb->bridge); - if (ret) - return ret; - - if (mxsfb->panel) { - mxsfb->connector = &mxsfb->panel_connector; - mxsfb->connector->dpms = DRM_MODE_DPMS_OFF; - mxsfb->connector->polled = 0; - drm_connector_helper_add(mxsfb->connector, - &mxsfb_panel_connector_helper_funcs); - ret = drm_connector_init(drm, mxsfb->connector, - &mxsfb_panel_connector_funcs, - DRM_MODE_CONNECTOR_Unknown); - } - - return ret; -}
Replace the manual connector implementation based on drm_panel with the drm_panel_bridge helper. This simplifies the mxsfb driver by removing connector-related code, and standardizing all pipeline control operations on bridges. A hack is needed to get hold of the connector, as that's our only source of bus flags and formats for now. As soon as the bridge API provides us with that information this can be fixed. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/gpu/drm/mxsfb/Makefile | 2 +- drivers/gpu/drm/mxsfb/mxsfb_drv.c | 105 ++++++++++++++---------------- drivers/gpu/drm/mxsfb/mxsfb_drv.h | 5 +- drivers/gpu/drm/mxsfb/mxsfb_out.c | 99 ---------------------------- 4 files changed, 53 insertions(+), 158 deletions(-) delete mode 100644 drivers/gpu/drm/mxsfb/mxsfb_out.c