| Message ID | 20170427163601.7313-1-eric@anholt.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Anholt <eric@anholt.net> writes: > Many DRM drivers have common code to make a stub connector > implementation that wraps a drm_panel. By wrapping the panel in a DRM > bridge, all of the connector code (including calls during encoder > enable/disable) goes away. > > Signed-off-by: Eric Anholt <eric@anholt.net> > +/** > + * drm_panel_bridge_add - Creates a drm_bridge and drm_connector that > + * just call the appropriate functions from drm_panel. > + * > + * @dev: The struct device of the panel device. This is used for > + * registering the drm_bridge. > + * @panel: The drm_panel being wrapped. Must be non-NULL. > + * @connector_type: The DRM_MODE_CONNECTOR_* for the connector to be > + * created. > + * > + * For drivers converting from directly using drm_panel: The expected > + * usage pattern is that during either encoder module probe or DSI > + * host attach, a drm_panel will be looked up through > + * drm_of_find_panel_or_bridge(). drm_panel_bridge_add() is used to > + * wrap that panel in the new bridge, and the result can then be > + * passed to drm_bridge_attach(). The drm_panel_prepare() and related > + * functions can be dropped from the encoder driver (they're now > + * called by the KMS helpers before calling into the encoder), along > + * with connector creation. > + */ > +struct drm_bridge *drm_panel_bridge_add(struct device *dev, > + struct drm_panel *panel, > + u32 connector_type) > +{ > + struct panel_bridge *panel_bridge = > + devm_kzalloc(dev, sizeof(*panel_bridge), GFP_KERNEL); > + int ret; > + > + if (!dev || !panel) > + return ERR_PTR(EINVAL); > + > + panel_bridge->dev = dev; > + panel_bridge->connector_type = connector_type; > + panel_bridge->panel = panel; > + > + panel_bridge->bridge.funcs = &panel_bridge_bridge_funcs; > + panel_bridge->bridge.of_node = dev->of_node; > + > + ret = drm_bridge_add(&panel_bridge->bridge); > + if (ret) > + return ERR_PTR(ret); > + > + return &panel_bridge->bridge; > +} > +EXPORT_SYMBOL(drm_panel_bridge_add); > + > +void drm_panel_bridge_remove(struct device *dev, struct drm_bridge *bridge) > +{ > + drm_bridge_remove(bridge); > + devm_kfree(dev, bridge); > +} > +EXPORT_SYMBOL(drm_panel_bridge_remove); Working with this interface in another driver, I want to drop the "struct device" argument, since we can get the struct device from panel->dev. It keeps the user of the driver from needing to keep track of the panel/device it needs to hand to drm_panel_bridge_remove().
Hi, On 04/27/2017 10:06 PM, Eric Anholt wrote: > Many DRM drivers have common code to make a stub connector > implementation that wraps a drm_panel. By wrapping the panel in a DRM > bridge, all of the connector code (including calls during encoder > enable/disable) goes away. > > Signed-off-by: Eric Anholt <eric@anholt.net> > --- > Documentation/gpu/drm-kms-helpers.rst | 3 + > drivers/gpu/drm/bridge/Kconfig | 11 +- > drivers/gpu/drm/bridge/Makefile | 1 + > drivers/gpu/drm/bridge/lvds-encoder.c | 158 ++++------------------------ > drivers/gpu/drm/bridge/panel.c | 188 ++++++++++++++++++++++++++++++++++ > include/drm/drm_bridge.h | 8 ++ > 6 files changed, 228 insertions(+), 141 deletions(-) > create mode 100644 drivers/gpu/drm/bridge/panel.c > > diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst > index c075aadd7078..60eb3b41702b 100644 > --- a/Documentation/gpu/drm-kms-helpers.rst > +++ b/Documentation/gpu/drm-kms-helpers.rst > @@ -143,6 +143,9 @@ Bridge Helper Reference > .. kernel-doc:: drivers/gpu/drm/drm_bridge.c > :export: > > +.. kernel-doc:: drivers/gpu/drm/bridge/panel.c > + :export: > + > .. _drm_panel_helper: > > Panel Helper Reference > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index f6968d3b4b41..c4daca38743c 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -4,6 +4,14 @@ config DRM_BRIDGE > help > Bridge registration and lookup framework. > > +config DRM_PANEL_BRIDGE > + def_bool y > + depends on DRM_BRIDGE > + select DRM_KMS_HELPER > + select DRM_PANEL > + help > + DRM bridge wrapper of DRM panels > + > menu "Display Interface Bridges" > depends on DRM && DRM_BRIDGE > > @@ -27,8 +35,7 @@ config DRM_DUMB_VGA_DAC > config DRM_LVDS_ENCODER > tristate "Transparent parallel to LVDS encoder support" > depends on OF > - select DRM_KMS_HELPER > - select DRM_PANEL > + select DRM_PANEL_BRIDGE > help > Support for transparent parallel to LVDS encoders that don't require > any configuration. > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > index 3fe2226ee2f2..40a43750ad55 100644 > --- a/drivers/gpu/drm/bridge/Makefile > +++ b/drivers/gpu/drm/bridge/Makefile > @@ -5,6 +5,7 @@ obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o > obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o > obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o > obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o > +obj-$(CONFIG_DRM_PANEL_BRIDGE) += panel.o > obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o > obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o > obj-$(CONFIG_DRM_SII902X) += sii902x.o > diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c > index f1f67a279426..04e1504c4d8f 100644 > --- a/drivers/gpu/drm/bridge/lvds-encoder.c > +++ b/drivers/gpu/drm/bridge/lvds-encoder.c > @@ -8,144 +8,18 @@ > */ > > #include <drm/drmP.h> > -#include <drm/drm_atomic_helper.h> > -#include <drm/drm_connector.h> > -#include <drm/drm_crtc_helper.h> > -#include <drm/drm_encoder.h> > -#include <drm/drm_modeset_helper_vtables.h> > +#include <drm/drm_bridge.h> > #include <drm/drm_panel.h> > > #include <linux/of_graph.h> > > -struct lvds_encoder { > - struct device *dev; > - > - struct drm_bridge bridge; > - struct drm_connector connector; > - struct drm_panel *panel; > -}; > - > -static inline struct lvds_encoder * > -drm_bridge_to_lvds_encoder(struct drm_bridge *bridge) > -{ > - return container_of(bridge, struct lvds_encoder, bridge); > -} > - > -static inline struct lvds_encoder * > -drm_connector_to_lvds_encoder(struct drm_connector *connector) > -{ > - return container_of(connector, struct lvds_encoder, connector); > -} > - > -static int lvds_connector_get_modes(struct drm_connector *connector) > -{ > - struct lvds_encoder *lvds = drm_connector_to_lvds_encoder(connector); > - > - return drm_panel_get_modes(lvds->panel); > -} > - > -static const struct drm_connector_helper_funcs lvds_connector_helper_funcs = { > - .get_modes = lvds_connector_get_modes, > -}; > - > -static const struct drm_connector_funcs lvds_connector_funcs = { > - .dpms = drm_atomic_helper_connector_dpms, > - .reset = drm_atomic_helper_connector_reset, > - .fill_modes = drm_helper_probe_single_connector_modes, > - .destroy = drm_connector_cleanup, > - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > -}; > - > -static int lvds_encoder_attach(struct drm_bridge *bridge) > -{ > - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); > - struct drm_connector *connector = &lvds->connector; > - int ret; > - > - if (!bridge->encoder) { > - DRM_ERROR("Missing encoder\n"); > - return -ENODEV; > - } > - > - drm_connector_helper_add(connector, &lvds_connector_helper_funcs); > - > - ret = drm_connector_init(bridge->dev, connector, &lvds_connector_funcs, > - DRM_MODE_CONNECTOR_LVDS); > - if (ret) { > - DRM_ERROR("Failed to initialize connector\n"); > - return ret; > - } > - > - drm_mode_connector_attach_encoder(&lvds->connector, bridge->encoder); > - > - ret = drm_panel_attach(lvds->panel, &lvds->connector); > - if (ret < 0) > - return ret; > - > - return 0; > -} > - > -static void lvds_encoder_detach(struct drm_bridge *bridge) > -{ > - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); > - > - drm_panel_detach(lvds->panel); > -} > - > -static void lvds_encoder_pre_enable(struct drm_bridge *bridge) > -{ > - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); > - > - drm_panel_prepare(lvds->panel); > -} > - > -static void lvds_encoder_enable(struct drm_bridge *bridge) > -{ > - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); > - > - drm_panel_enable(lvds->panel); > -} > - > -static void lvds_encoder_disable(struct drm_bridge *bridge) > -{ > - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); > - > - drm_panel_disable(lvds->panel); > -} > - > -static void lvds_encoder_post_disable(struct drm_bridge *bridge) > -{ > - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); > - > - drm_panel_unprepare(lvds->panel); > -} > - > -static const struct drm_bridge_funcs lvds_encoder_bridge_funcs = { > - .attach = lvds_encoder_attach, > - .detach = lvds_encoder_detach, > - .pre_enable = lvds_encoder_pre_enable, > - .enable = lvds_encoder_enable, > - .disable = lvds_encoder_disable, > - .post_disable = lvds_encoder_post_disable, > -}; > - > static int lvds_encoder_probe(struct platform_device *pdev) > { > - struct lvds_encoder *lvds; > struct device_node *port; > struct device_node *endpoint; > - struct device_node *panel; > - > - lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL); > - if (!lvds) > - return -ENOMEM; > - > - lvds->dev = &pdev->dev; > - platform_set_drvdata(pdev, lvds); > - > - lvds->bridge.funcs = &lvds_encoder_bridge_funcs; > - lvds->bridge.of_node = pdev->dev.of_node; > + struct device_node *panel_node; > + struct drm_panel *panel; > + struct drm_bridge *bridge; > > /* Locate the panel DT node. */ > port = of_graph_get_port_by_id(pdev->dev.of_node, 1); > @@ -161,29 +35,35 @@ static int lvds_encoder_probe(struct platform_device *pdev) > return -ENXIO; > } > > - panel = of_graph_get_remote_port_parent(endpoint); > + panel_node = of_graph_get_remote_port_parent(endpoint); > of_node_put(endpoint); > - if (!panel) { > + if (!panel_node) { > dev_dbg(&pdev->dev, "no remote endpoint for port 1\n"); > return -ENXIO; > } > > - lvds->panel = of_drm_find_panel(panel); > - of_node_put(panel); > - if (!lvds->panel) { > + panel = of_drm_find_panel(panel_node); > + of_node_put(panel_node); > + if (!panel) { > dev_dbg(&pdev->dev, "panel not found, deferring probe\n"); > return -EPROBE_DEFER; > } > > - /* Register the bridge. */ > - return drm_bridge_add(&lvds->bridge); > + bridge = drm_panel_bridge_add(&pdev->dev, panel, > + DRM_MODE_CONNECTOR_LVDS); > + if (IS_ERR(bridge)) > + return PTR_ERR(bridge); > + > + platform_set_drvdata(pdev, bridge); > + > + return 0; > } > > static int lvds_encoder_remove(struct platform_device *pdev) > { > - struct lvds_encoder *encoder = platform_get_drvdata(pdev); > + struct drm_bridge *bridge = platform_get_drvdata(pdev); > > - drm_bridge_remove(&encoder->bridge); > + drm_bridge_remove(bridge); > > return 0; > } > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c > new file mode 100644 > index 000000000000..2081245455c6 > --- /dev/null > +++ b/drivers/gpu/drm/bridge/panel.c > @@ -0,0 +1,188 @@ > +/* > + * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + * Copyright (C) 2017 Broadcom > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation; either version 2 of > + * the License, or (at your option) any later version. > + */ > + > +#include <drm/drmP.h> > +#include <drm/drm_panel.h> > +#include <drm/drm_atomic_helper.h> > +#include <drm/drm_connector.h> > +#include <drm/drm_crtc_helper.h> > +#include <drm/drm_encoder.h> > +#include <drm/drm_modeset_helper_vtables.h> > +#include <drm/drm_panel.h> > + > +struct panel_bridge { > + struct drm_bridge bridge; > + struct device *dev; > + struct drm_connector connector; > + struct drm_panel *panel; > + u32 connector_type; > +}; > + > +static inline struct panel_bridge * > +drm_bridge_to_panel_bridge(struct drm_bridge *bridge) > +{ > + return container_of(bridge, struct panel_bridge, bridge); > +} > + > +static inline struct panel_bridge * > +drm_connector_to_panel_bridge(struct drm_connector *connector) > +{ > + return container_of(connector, struct panel_bridge, connector); > +} > + > +static int panel_bridge_connector_get_modes(struct drm_connector *connector) > +{ > + struct panel_bridge *panel_bridge = > + drm_connector_to_panel_bridge(connector); > + > + return drm_panel_get_modes(panel_bridge->panel); > +} > + > +static const struct drm_connector_helper_funcs panel_bridge_connector_helper_funcs = { > + .get_modes = panel_bridge_connector_get_modes, > +}; > + > +static const struct drm_connector_funcs panel_bridge_connector_funcs = { > + .dpms = drm_atomic_helper_connector_dpms, > + .reset = drm_atomic_helper_connector_reset, > + .fill_modes = drm_helper_probe_single_connector_modes, > + .destroy = drm_connector_cleanup, > + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > +}; > + > +static int panel_bridge_attach(struct drm_bridge *bridge) > +{ > + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > + struct drm_connector *connector = &panel_bridge->connector; > + int ret; > + > + if (!bridge->encoder) { > + DRM_ERROR("Missing encoder\n"); > + return -ENODEV; > + } > + > + drm_connector_helper_add(connector, > + &panel_bridge_connector_helper_funcs); > + > + ret = drm_connector_init(bridge->dev, connector, > + &panel_bridge_connector_funcs, > + panel_bridge->connector_type); > + if (ret) { > + DRM_ERROR("Failed to initialize connector\n"); > + return ret; > + } > + > + drm_mode_connector_attach_encoder(&panel_bridge->connector, > + bridge->encoder); > + > + ret = drm_panel_attach(panel_bridge->panel, &panel_bridge->connector); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static void panel_bridge_detach(struct drm_bridge *bridge) > +{ > + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > + > + drm_panel_detach(panel_bridge->panel); > +} > + > +static void panel_bridge_pre_enable(struct drm_bridge *bridge) > +{ > + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > + > + drm_panel_prepare(panel_bridge->panel); > +} > + > +static void panel_bridge_enable(struct drm_bridge *bridge) > +{ > + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > + > + drm_panel_enable(panel_bridge->panel); > +} > + > +static void panel_bridge_disable(struct drm_bridge *bridge) > +{ > + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > + > + drm_panel_disable(panel_bridge->panel); > +} > + > +static void panel_bridge_post_disable(struct drm_bridge *bridge) > +{ > + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > + > + drm_panel_unprepare(panel_bridge->panel); > +} > + > +static const struct drm_bridge_funcs panel_bridge_bridge_funcs = { > + .attach = panel_bridge_attach, > + .detach = panel_bridge_detach, > + .pre_enable = panel_bridge_pre_enable, > + .enable = panel_bridge_enable, > + .disable = panel_bridge_disable, > + .post_disable = panel_bridge_post_disable, > +}; > + > +/** > + * drm_panel_bridge_add - Creates a drm_bridge and drm_connector that > + * just call the appropriate functions from drm_panel. > + * > + * @dev: The struct device of the panel device. This is used for > + * registering the drm_bridge. > + * @panel: The drm_panel being wrapped. Must be non-NULL. > + * @connector_type: The DRM_MODE_CONNECTOR_* for the connector to be > + * created. > + * > + * For drivers converting from directly using drm_panel: The expected > + * usage pattern is that during either encoder module probe or DSI > + * host attach, a drm_panel will be looked up through > + * drm_of_find_panel_or_bridge(). drm_panel_bridge_add() is used to > + * wrap that panel in the new bridge, and the result can then be > + * passed to drm_bridge_attach(). The drm_panel_prepare() and related > + * functions can be dropped from the encoder driver (they're now > + * called by the KMS helpers before calling into the encoder), along > + * with connector creation. +panel/bridge reviewers. This does make things much cleaner, but it seems a bit strange to create a drm_bridge when there isn't really a HW bridge in the display chain (i.e, when the DSI encoder is directly connected to a DSI panel). There are kms drivers that use drm_panel, but don't have simple stub connectors that wrap around a drm_panel. They have more complicated connector ops, and may call drm_panel_prepare() and related functions a bit differently. We won't be able to use drm_panel_bridge for those drivers. For msm, we check whether the DSI encoder is connected directly to a panel or an external bridge. If it's connected to an external bridge, we skip the creation of the stub connector, and rely on the external bridge driver to create the connector: http://lxr.free-electrons.com/source/drivers/gpu/drm/msm/dsi/dsi.c#L227 The msm solution isn't very neat, but it avoids the need to create another bridge to glue things together. Archit > + */ > +struct drm_bridge *drm_panel_bridge_add(struct device *dev, > + struct drm_panel *panel, > + u32 connector_type) > +{ > + struct panel_bridge *panel_bridge = > + devm_kzalloc(dev, sizeof(*panel_bridge), GFP_KERNEL); > + int ret;tegra_output_connector_detect, > + > + if (!dev || !panel) > + return ERR_PTR(EINVAL); > + > + panel_bridge->dev = dev; > + panel_bridge->connector_type = connector_type; > + panel_bridge->panel = panel; > + > + panel_bridge->bridge.funcs = &panel_bridge_bridge_funcs; > + panel_bridge->bridge.of_node = dev->of_node; > + > + ret = drm_bridge_add(&panel_bridge->bridge); > + if (ret) > + return ERR_PTR(ret); > + > + return &panel_bridge->bridge; > +} > +EXPORT_SYMBOL(drm_panel_bridge_add); > + > +void drm_panel_bridge_remove(struct device *dev, struct drm_bridge *bridge) > +{ > + drm_bridge_remove(bridge); > + devm_kfree(dev, bridge); > +} > +EXPORT_SYMBOL(drm_panel_bridge_remove); > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > index fdd82fcbf168..e08563ee0546 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -29,6 +29,7 @@ > #include <drm/drm_modes.h> > > struct drm_bridge; > +struct drm_panel; > > /** > * struct drm_bridge_funcs - drm_bridge control functions > @@ -221,4 +222,11 @@ void drm_bridge_mode_set(struct drm_bridge *bridge, > void drm_bridge_pre_enable(struct drm_bridge *bridge); > void drm_bridge_enable(struct drm_bridge *bridge); > > +#ifdef CONFIG_DRM_PANEL > +struct drm_bridge *drm_panel_bridge_add(struct device *dev, > + struct drm_panel *panel, > + u32 connector_type); > +void drm_panel_bridge_remove(struct device *dev, struct drm_bridge *bridge); > +#endif > + > #endif >
Hi Archit, On Wednesday 03 May 2017 14:53:00 Archit Taneja wrote: > On 04/27/2017 10:06 PM, Eric Anholt wrote: > > Many DRM drivers have common code to make a stub connector > > implementation that wraps a drm_panel. By wrapping the panel in a DRM > > bridge, all of the connector code (including calls during encoder > > enable/disable) goes away. > > > > Signed-off-by: Eric Anholt <eric@anholt.net> > > --- > > > > Documentation/gpu/drm-kms-helpers.rst | 3 + > > drivers/gpu/drm/bridge/Kconfig | 11 +- > > drivers/gpu/drm/bridge/Makefile | 1 + > > drivers/gpu/drm/bridge/lvds-encoder.c | 158 ++++------------------------ > > drivers/gpu/drm/bridge/panel.c | 188 +++++++++++++++++++++++++++++ > > include/drm/drm_bridge.h | 8 ++ > > 6 files changed, 228 insertions(+), 141 deletions(-) > > create mode 100644 drivers/gpu/drm/bridge/panel.c > > > > diff --git a/Documentation/gpu/drm-kms-helpers.rst > > b/Documentation/gpu/drm-kms-helpers.rst index c075aadd7078..60eb3b41702b > > 100644 > > --- a/Documentation/gpu/drm-kms-helpers.rst > > +++ b/Documentation/gpu/drm-kms-helpers.rst > > @@ -143,6 +143,9 @@ Bridge Helper Reference > > .. kernel-doc:: drivers/gpu/drm/drm_bridge.c > > :export: > > > > +.. kernel-doc:: drivers/gpu/drm/bridge/panel.c > > + :export: > > + > > .. _drm_panel_helper: > > Panel Helper Reference > > > > diff --git a/drivers/gpu/drm/bridge/Kconfig > > b/drivers/gpu/drm/bridge/Kconfig index f6968d3b4b41..c4daca38743c 100644 > > --- a/drivers/gpu/drm/bridge/Kconfig > > +++ b/drivers/gpu/drm/bridge/Kconfig > > @@ -4,6 +4,14 @@ config DRM_BRIDGE > > help > > Bridge registration and lookup framework. > > > > +config DRM_PANEL_BRIDGE > > + def_bool y > > + depends on DRM_BRIDGE > > + select DRM_KMS_HELPER > > + select DRM_PANEL > > + help > > + DRM bridge wrapper of DRM panels > > + > > menu "Display Interface Bridges" > > depends on DRM && DRM_BRIDGE > > > > @@ -27,8 +35,7 @@ config DRM_DUMB_VGA_DAC > > config DRM_LVDS_ENCODER > > tristate "Transparent parallel to LVDS encoder support" > > depends on OF > > - select DRM_KMS_HELPER > > - select DRM_PANEL > > + select DRM_PANEL_BRIDGE > > help > > Support for transparent parallel to LVDS encoders that don't require > > any configuration. > > diff --git a/drivers/gpu/drm/bridge/Makefile > > b/drivers/gpu/drm/bridge/Makefile index 3fe2226ee2f2..40a43750ad55 100644 > > --- a/drivers/gpu/drm/bridge/Makefile > > +++ b/drivers/gpu/drm/bridge/Makefile > > @@ -5,6 +5,7 @@ obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o > > obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o > > obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += > > megachips-stdpxxxx-ge-b850v3-fw.o obj-$(CONFIG_DRM_NXP_PTN3460) += > > nxp-ptn3460.o > > +obj-$(CONFIG_DRM_PANEL_BRIDGE) += panel.o > > obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o > > obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o > > obj-$(CONFIG_DRM_SII902X) += sii902x.o > > diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c > > b/drivers/gpu/drm/bridge/lvds-encoder.c index f1f67a279426..04e1504c4d8f > > 100644 > > --- a/drivers/gpu/drm/bridge/lvds-encoder.c > > +++ b/drivers/gpu/drm/bridge/lvds-encoder.c > > @@ -8,144 +8,18 @@ > > */ > > > > #include <drm/drmP.h> > > -#include <drm/drm_atomic_helper.h> > > -#include <drm/drm_connector.h> > > -#include <drm/drm_crtc_helper.h> > > -#include <drm/drm_encoder.h> > > -#include <drm/drm_modeset_helper_vtables.h> > > +#include <drm/drm_bridge.h> > > #include <drm/drm_panel.h> > > > > #include <linux/of_graph.h> > > > > -struct lvds_encoder { > > - struct device *dev; > > - > > - struct drm_bridge bridge; > > - struct drm_connector connector; > > - struct drm_panel *panel; > > -}; > > - > > -static inline struct lvds_encoder * > > -drm_bridge_to_lvds_encoder(struct drm_bridge *bridge) > > -{ > > - return container_of(bridge, struct lvds_encoder, bridge); > > -} > > - > > -static inline struct lvds_encoder * > > -drm_connector_to_lvds_encoder(struct drm_connector *connector) > > -{ > > - return container_of(connector, struct lvds_encoder, connector); > > -} > > - > > -static int lvds_connector_get_modes(struct drm_connector *connector) > > -{ > > - struct lvds_encoder *lvds = drm_connector_to_lvds_encoder(connector); > > - > > - return drm_panel_get_modes(lvds->panel); > > -} > > - > > -static const struct drm_connector_helper_funcs > > lvds_connector_helper_funcs = { - .get_modes = lvds_connector_get_modes, > > -}; > > - > > -static const struct drm_connector_funcs lvds_connector_funcs = { > > - .dpms = drm_atomic_helper_connector_dpms, > > - .reset = drm_atomic_helper_connector_reset, > > - .fill_modes = drm_helper_probe_single_connector_modes, > > - .destroy = drm_connector_cleanup, > > - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > > - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > > -}; > > - > > -static int lvds_encoder_attach(struct drm_bridge *bridge) > > -{ > > - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); > > - struct drm_connector *connector = &lvds->connector; > > - int ret; > > - > > - if (!bridge->encoder) { > > - DRM_ERROR("Missing encoder\n"); > > - return -ENODEV; > > - } > > - > > - drm_connector_helper_add(connector, &lvds_connector_helper_funcs); > > - > > - ret = drm_connector_init(bridge->dev, connector, &lvds_connector_funcs, > > - DRM_MODE_CONNECTOR_LVDS); > > - if (ret) { > > - DRM_ERROR("Failed to initialize connector\n"); > > - return ret; > > - } > > - > > - drm_mode_connector_attach_encoder(&lvds->connector, bridge->encoder); > > - > > - ret = drm_panel_attach(lvds->panel, &lvds->connector); > > - if (ret < 0) > > - return ret; > > - > > - return 0; > > -} > > - > > -static void lvds_encoder_detach(struct drm_bridge *bridge) > > -{ > > - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); > > - > > - drm_panel_detach(lvds->panel); > > -} > > - > > -static void lvds_encoder_pre_enable(struct drm_bridge *bridge) > > -{ > > - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); > > - > > - drm_panel_prepare(lvds->panel); > > -} > > - > > -static void lvds_encoder_enable(struct drm_bridge *bridge) > > -{ > > - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); > > - > > - drm_panel_enable(lvds->panel); > > -} > > - > > -static void lvds_encoder_disable(struct drm_bridge *bridge) > > -{ > > - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); > > - > > - drm_panel_disable(lvds->panel); > > -} > > - > > -static void lvds_encoder_post_disable(struct drm_bridge *bridge) > > -{ > > - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); > > - > > - drm_panel_unprepare(lvds->panel); > > -} > > - > > -static const struct drm_bridge_funcs lvds_encoder_bridge_funcs = { > > - .attach = lvds_encoder_attach, > > - .detach = lvds_encoder_detach, > > - .pre_enable = lvds_encoder_pre_enable, > > - .enable = lvds_encoder_enable, > > - .disable = lvds_encoder_disable, > > - .post_disable = lvds_encoder_post_disable, > > -}; > > - > > > > static int lvds_encoder_probe(struct platform_device *pdev) > > { > > > > - struct lvds_encoder *lvds; > > > > struct device_node *port; > > struct device_node *endpoint; > > > > - struct device_node *panel; > > - > > - lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL); > > - if (!lvds) > > - return -ENOMEM; > > - > > - lvds->dev = &pdev->dev; > > - platform_set_drvdata(pdev, lvds); > > - > > - lvds->bridge.funcs = &lvds_encoder_bridge_funcs; > > - lvds->bridge.of_node = pdev->dev.of_node; > > + struct device_node *panel_node; > > + struct drm_panel *panel; > > + struct drm_bridge *bridge; > > > > /* Locate the panel DT node. */ > > port = of_graph_get_port_by_id(pdev->dev.of_node, 1); > > > > @@ -161,29 +35,35 @@ static int lvds_encoder_probe(struct platform_device > > *pdev)> > > return -ENXIO; > > > > } > > > > - panel = of_graph_get_remote_port_parent(endpoint); > > + panel_node = of_graph_get_remote_port_parent(endpoint); > > > > of_node_put(endpoint); > > > > - if (!panel) { > > + if (!panel_node) { > > > > dev_dbg(&pdev->dev, "no remote endpoint for port 1\n"); > > return -ENXIO; > > > > } > > > > - lvds->panel = of_drm_find_panel(panel); > > - of_node_put(panel); > > - if (!lvds->panel) { > > + panel = of_drm_find_panel(panel_node); > > + of_node_put(panel_node); > > + if (!panel) { > > > > dev_dbg(&pdev->dev, "panel not found, deferring probe\n"); > > return -EPROBE_DEFER; > > > > } > > > > - /* Register the bridge. */ > > - return drm_bridge_add(&lvds->bridge); > > + bridge = drm_panel_bridge_add(&pdev->dev, panel, > > + DRM_MODE_CONNECTOR_LVDS); > > + if (IS_ERR(bridge)) > > + return PTR_ERR(bridge); > > + > > + platform_set_drvdata(pdev, bridge); > > + > > + return 0; > > > > } > > > > static int lvds_encoder_remove(struct platform_device *pdev) > > { > > > > - struct lvds_encoder *encoder = platform_get_drvdata(pdev); > > + struct drm_bridge *bridge = platform_get_drvdata(pdev); > > > > - drm_bridge_remove(&encoder->bridge); > > + drm_bridge_remove(bridge); > > > > return 0; > > > > } > > > > diff --git a/drivers/gpu/drm/bridge/panel.c > > b/drivers/gpu/drm/bridge/panel.c new file mode 100644 > > index 000000000000..2081245455c6 > > --- /dev/null > > +++ b/drivers/gpu/drm/bridge/panel.c > > @@ -0,0 +1,188 @@ > > +/* > > + * Copyright (C) 2016 Laurent Pinchart > > <laurent.pinchart@ideasonboard.com> > > + * Copyright (C) 2017 Broadcom > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License as > > + * published by the Free Software Foundation; either version 2 of > > + * the License, or (at your option) any later version. > > + */ > > + > > +#include <drm/drmP.h> > > +#include <drm/drm_panel.h> > > +#include <drm/drm_atomic_helper.h> > > +#include <drm/drm_connector.h> > > +#include <drm/drm_crtc_helper.h> > > +#include <drm/drm_encoder.h> > > +#include <drm/drm_modeset_helper_vtables.h> > > +#include <drm/drm_panel.h> > > + > > +struct panel_bridge { > > + struct drm_bridge bridge; > > + struct device *dev; > > + struct drm_connector connector; > > + struct drm_panel *panel; > > + u32 connector_type; > > +}; > > + > > +static inline struct panel_bridge * > > +drm_bridge_to_panel_bridge(struct drm_bridge *bridge) > > +{ > > + return container_of(bridge, struct panel_bridge, bridge); > > +} > > + > > +static inline struct panel_bridge * > > +drm_connector_to_panel_bridge(struct drm_connector *connector) > > +{ > > + return container_of(connector, struct panel_bridge, connector); > > +} > > + > > +static int panel_bridge_connector_get_modes(struct drm_connector > > *connector) +{ > > + struct panel_bridge *panel_bridge = > > + drm_connector_to_panel_bridge(connector); > > + > > + return drm_panel_get_modes(panel_bridge->panel); > > +} > > + > > +static const struct drm_connector_helper_funcs > > panel_bridge_connector_helper_funcs = { + .get_modes = > > panel_bridge_connector_get_modes, > > +}; > > + > > +static const struct drm_connector_funcs panel_bridge_connector_funcs = { > > + .dpms = drm_atomic_helper_connector_dpms, > > + .reset = drm_atomic_helper_connector_reset, > > + .fill_modes = drm_helper_probe_single_connector_modes, > > + .destroy = drm_connector_cleanup, > > + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > > + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > > +}; > > + > > +static int panel_bridge_attach(struct drm_bridge *bridge) > > +{ > > + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > > + struct drm_connector *connector = &panel_bridge->connector; > > + int ret; > > + > > + if (!bridge->encoder) { > > + DRM_ERROR("Missing encoder\n"); > > + return -ENODEV; > > + } > > + > > + drm_connector_helper_add(connector, > > + &panel_bridge_connector_helper_funcs); > > + > > + ret = drm_connector_init(bridge->dev, connector, > > + &panel_bridge_connector_funcs, > > + panel_bridge->connector_type); > > + if (ret) { > > + DRM_ERROR("Failed to initialize connector\n"); > > + return ret; > > + } > > + > > + drm_mode_connector_attach_encoder(&panel_bridge->connector, > > + bridge->encoder); > > + > > + ret = drm_panel_attach(panel_bridge->panel, &panel_bridge->connector); > > + if (ret < 0) > > + return ret; > > + > > + return 0; > > +} > > + > > +static void panel_bridge_detach(struct drm_bridge *bridge) > > +{ > > + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > > + > > + drm_panel_detach(panel_bridge->panel); > > +} > > + > > +static void panel_bridge_pre_enable(struct drm_bridge *bridge) > > +{ > > + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > > + > > + drm_panel_prepare(panel_bridge->panel); > > +} > > + > > +static void panel_bridge_enable(struct drm_bridge *bridge) > > +{ > > + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > > + > > + drm_panel_enable(panel_bridge->panel); > > +} > > + > > +static void panel_bridge_disable(struct drm_bridge *bridge) > > +{ > > + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > > + > > + drm_panel_disable(panel_bridge->panel); > > +} > > + > > +static void panel_bridge_post_disable(struct drm_bridge *bridge) > > +{ > > + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > > + > > + drm_panel_unprepare(panel_bridge->panel); > > +} > > + > > +static const struct drm_bridge_funcs panel_bridge_bridge_funcs = { > > + .attach = panel_bridge_attach, > > + .detach = panel_bridge_detach, > > + .pre_enable = panel_bridge_pre_enable, > > + .enable = panel_bridge_enable, > > + .disable = panel_bridge_disable, > > + .post_disable = panel_bridge_post_disable, > > +}; > > + > > +/** > > + * drm_panel_bridge_add - Creates a drm_bridge and drm_connector that > > + * just call the appropriate functions from drm_panel. > > + * > > + * @dev: The struct device of the panel device. This is used for > > + * registering the drm_bridge. > > + * @panel: The drm_panel being wrapped. Must be non-NULL. > > + * @connector_type: The DRM_MODE_CONNECTOR_* for the connector to be > > + * created. > > + * > > + * For drivers converting from directly using drm_panel: The expected > > + * usage pattern is that during either encoder module probe or DSI > > + * host attach, a drm_panel will be looked up through > > + * drm_of_find_panel_or_bridge(). drm_panel_bridge_add() is used to > > + * wrap that panel in the new bridge, and the result can then be > > + * passed to drm_bridge_attach(). The drm_panel_prepare() and related > > + * functions can be dropped from the encoder driver (they're now > > + * called by the KMS helpers before calling into the encoder), along > > + * with connector creation. > > +panel/bridge reviewers. Thank you for that. > This does make things much cleaner, but it seems a bit strange to create > a drm_bridge when there isn't really a HW bridge in the display chain (i.e, > when the DSI encoder is directly connected to a DSI panel). I agree with you. I don't think this is a good idea, and what I'd like to see is a refactoring of the panel and bridge ops that would make them more alike, allowing to handle bridges and panels through a common interface. Another reason to go in that direction is that, if we create an object that can expose both encoder-related and connector-related operations (bridges being geared towards the former and panel the latter at the moment), then we'll be able to handle connector creation in a much easier better way. As I've explained before, bridges should not create connectors, as they have no idea whether they're chained to another bridge or output directly to a connector. > There are kms drivers that use drm_panel, but don't have simple stub > connectors that wrap around a drm_panel. They have more complicated > connector ops, and may call drm_panel_prepare() and related functions a bit > differently. We won't be able to use drm_panel_bridge for those drivers. > > For msm, we check whether the DSI encoder is connected directly to a panel > or an external bridge. If it's connected to an external bridge, we skip the > creation of the stub connector, and rely on the external bridge driver to > create the connector: > > http://lxr.free-electrons.com/source/drivers/gpu/drm/msm/dsi/dsi.c#L227 > > The msm solution isn't very neat, but it avoids the need to create another > bridge to glue things together. > > Archit > > > + */ > > +struct drm_bridge *drm_panel_bridge_add(struct device *dev, > > + struct drm_panel *panel, > > + u32 connector_type) > > +{ > > + struct panel_bridge *panel_bridge = > > + devm_kzalloc(dev, sizeof(*panel_bridge), GFP_KERNEL); > > + int ret;tegra_output_connector_detect, > > + > > + if (!dev || !panel) > > + return ERR_PTR(EINVAL); > > + > > + panel_bridge->dev = dev; > > + panel_bridge->connector_type = connector_type; > > + panel_bridge->panel = panel; > > + > > + panel_bridge->bridge.funcs = &panel_bridge_bridge_funcs; > > + panel_bridge->bridge.of_node = dev->of_node; > > + > > + ret = drm_bridge_add(&panel_bridge->bridge); > > + if (ret) > > + return ERR_PTR(ret); > > + > > + return &panel_bridge->bridge; > > +} > > +EXPORT_SYMBOL(drm_panel_bridge_add); > > + > > +void drm_panel_bridge_remove(struct device *dev, struct drm_bridge > > *bridge) +{ > > + drm_bridge_remove(bridge); > > + devm_kfree(dev, bridge); > > +} > > +EXPORT_SYMBOL(drm_panel_bridge_remove); > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > > index fdd82fcbf168..e08563ee0546 100644 > > --- a/include/drm/drm_bridge.h > > +++ b/include/drm/drm_bridge.h > > @@ -29,6 +29,7 @@ > > #include <drm/drm_modes.h> > > > > struct drm_bridge; > > +struct drm_panel; > > > > /** > > * struct drm_bridge_funcs - drm_bridge control functions > > @@ -221,4 +222,11 @@ void drm_bridge_mode_set(struct drm_bridge *bridge, > > void drm_bridge_pre_enable(struct drm_bridge *bridge); > > void drm_bridge_enable(struct drm_bridge *bridge); > > > > +#ifdef CONFIG_DRM_PANEL > > +struct drm_bridge *drm_panel_bridge_add(struct device *dev, > > + struct drm_panel *panel, > > + u32 connector_type); > > +void drm_panel_bridge_remove(struct device *dev, struct drm_bridge > > *bridge); > > +#endif > > + > > #endif
On Wed, May 03, 2017 at 02:53:00PM +0530, Archit Taneja wrote: > +panel/bridge reviewers. > > This does make things much cleaner, but it seems a bit strange to create > a drm_bridge when there isn't really a HW bridge in the display chain (i.e, > when the DSI encoder is directly connected to a DSI panel). > > There are kms drivers that use drm_panel, but don't have simple stub connectors > that wrap around a drm_panel. They have more complicated connector ops, and > may call drm_panel_prepare() and related functions a bit differently. We won't > be able to use drm_panel_bridge for those drivers. > > For msm, we check whether the DSI encoder is connected directly to a panel > or an external bridge. If it's connected to an external bridge, we skip the > creation of the stub connector, and rely on the external bridge driver to > create the connector: > > http://lxr.free-electrons.com/source/drivers/gpu/drm/msm/dsi/dsi.c#L227 > > The msm solution isn't very neat, but it avoids the need to create another > bridge to glue things together. Since I suggested this, yes I like it. And I think just unconditionally creating the panel bridge is probably even simpler, after all bridges are supposed to be chainable. I guess there's always going to be drivers where we need special handling, but I'm kinda hoping that for most cases simply plugging in a panel bridge is all that's need to glue drm_panel support into a driver. The simple pipe helpers do support bridges, and part of the goal there very much was to make it easy to glue in panel drivers. -Daniel
Hi Daniel, On Wednesday 03 May 2017 11:32:17 Daniel Vetter wrote: > On Wed, May 03, 2017 at 02:53:00PM +0530, Archit Taneja wrote: > > +panel/bridge reviewers. > > > > This does make things much cleaner, but it seems a bit strange to create > > a drm_bridge when there isn't really a HW bridge in the display chain > > (i.e, when the DSI encoder is directly connected to a DSI panel). > > > > There are kms drivers that use drm_panel, but don't have simple stub > > connectors that wrap around a drm_panel. They have more complicated > > connector ops, and may call drm_panel_prepare() and related functions a > > bit differently. We won't be able to use drm_panel_bridge for those > > drivers. > > > > For msm, we check whether the DSI encoder is connected directly to a panel > > or an external bridge. If it's connected to an external bridge, we skip > > the creation of the stub connector, and rely on the external bridge driver > > to create the connector: > > > > http://lxr.free-electrons.com/source/drivers/gpu/drm/msm/dsi/dsi.c#L227 > > > > The msm solution isn't very neat, but it avoids the need to create another > > bridge to glue things together. > > Since I suggested this, yes I like it. And I think just unconditionally > creating the panel bridge is probably even simpler, after all bridges are > supposed to be chainable. I guess there's always going to be drivers where > we need special handling, but I'm kinda hoping that for most cases simply > plugging in a panel bridge is all that's need to glue drm_panel support > into a driver. The simple pipe helpers do support bridges, and part of the > goal there very much was to make it easy to glue in panel drivers. As I've just explained in another reply, I don't see the point in doing this when we can instead refactor the bridge and panel operations to expose a common base object that will then be easy to handle in core code. This isn't just for panels, as connectors should have DT nodes, it makes sense to instantiate an object for them that can be handled by the DRM core, without having to push connector handling in all bridge drivers.
On Thu, Apr 27, 2017 at 09:36:00AM -0700, Eric Anholt wrote: > Many DRM drivers have common code to make a stub connector > implementation that wraps a drm_panel. By wrapping the panel in a DRM > bridge, all of the connector code (including calls during encoder > enable/disable) goes away. > > Signed-off-by: Eric Anholt <eric@anholt.net> > --- > Documentation/gpu/drm-kms-helpers.rst | 3 + > drivers/gpu/drm/bridge/Kconfig | 11 +- > drivers/gpu/drm/bridge/Makefile | 1 + > drivers/gpu/drm/bridge/lvds-encoder.c | 158 ++++------------------------ > drivers/gpu/drm/bridge/panel.c | 188 ++++++++++++++++++++++++++++++++++ > include/drm/drm_bridge.h | 8 ++ > 6 files changed, 228 insertions(+), 141 deletions(-) > create mode 100644 drivers/gpu/drm/bridge/panel.c > > diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst > index c075aadd7078..60eb3b41702b 100644 > --- a/Documentation/gpu/drm-kms-helpers.rst > +++ b/Documentation/gpu/drm-kms-helpers.rst > @@ -143,6 +143,9 @@ Bridge Helper Reference > .. kernel-doc:: drivers/gpu/drm/drm_bridge.c > :export: > I'd do a separate heading for this one (even if it's just two functions) to make it stick a bit more out. "Panel Bridge helper" or similar. And please add kerneldoc for the remove function (and maybe cross-reference them for ocd consistency). Besides this nits, I very much like this. And I think this addresses Laurent's concern of hard-coding the DT for his lvds bridge too much. Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> Feel free to upgrade to an r-b with the kerneldoc polish applied (but pls make sure you have acks from Laurent and Archit before pushing). -Daniel > +.. kernel-doc:: drivers/gpu/drm/bridge/panel.c > + :export: > + > .. _drm_panel_helper: > > Panel Helper Reference > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index f6968d3b4b41..c4daca38743c 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -4,6 +4,14 @@ config DRM_BRIDGE > help > Bridge registration and lookup framework. > > +config DRM_PANEL_BRIDGE > + def_bool y > + depends on DRM_BRIDGE > + select DRM_KMS_HELPER > + select DRM_PANEL > + help > + DRM bridge wrapper of DRM panels > + > menu "Display Interface Bridges" > depends on DRM && DRM_BRIDGE > > @@ -27,8 +35,7 @@ config DRM_DUMB_VGA_DAC > config DRM_LVDS_ENCODER > tristate "Transparent parallel to LVDS encoder support" > depends on OF > - select DRM_KMS_HELPER > - select DRM_PANEL > + select DRM_PANEL_BRIDGE > help > Support for transparent parallel to LVDS encoders that don't require > any configuration. > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > index 3fe2226ee2f2..40a43750ad55 100644 > --- a/drivers/gpu/drm/bridge/Makefile > +++ b/drivers/gpu/drm/bridge/Makefile > @@ -5,6 +5,7 @@ obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o > obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o > obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o > obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o > +obj-$(CONFIG_DRM_PANEL_BRIDGE) += panel.o > obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o > obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o > obj-$(CONFIG_DRM_SII902X) += sii902x.o > diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c > index f1f67a279426..04e1504c4d8f 100644 > --- a/drivers/gpu/drm/bridge/lvds-encoder.c > +++ b/drivers/gpu/drm/bridge/lvds-encoder.c > @@ -8,144 +8,18 @@ > */ > > #include <drm/drmP.h> > -#include <drm/drm_atomic_helper.h> > -#include <drm/drm_connector.h> > -#include <drm/drm_crtc_helper.h> > -#include <drm/drm_encoder.h> > -#include <drm/drm_modeset_helper_vtables.h> > +#include <drm/drm_bridge.h> > #include <drm/drm_panel.h> > > #include <linux/of_graph.h> > > -struct lvds_encoder { > - struct device *dev; > - > - struct drm_bridge bridge; > - struct drm_connector connector; > - struct drm_panel *panel; > -}; > - > -static inline struct lvds_encoder * > -drm_bridge_to_lvds_encoder(struct drm_bridge *bridge) > -{ > - return container_of(bridge, struct lvds_encoder, bridge); > -} > - > -static inline struct lvds_encoder * > -drm_connector_to_lvds_encoder(struct drm_connector *connector) > -{ > - return container_of(connector, struct lvds_encoder, connector); > -} > - > -static int lvds_connector_get_modes(struct drm_connector *connector) > -{ > - struct lvds_encoder *lvds = drm_connector_to_lvds_encoder(connector); > - > - return drm_panel_get_modes(lvds->panel); > -} > - > -static const struct drm_connector_helper_funcs lvds_connector_helper_funcs = { > - .get_modes = lvds_connector_get_modes, > -}; > - > -static const struct drm_connector_funcs lvds_connector_funcs = { > - .dpms = drm_atomic_helper_connector_dpms, > - .reset = drm_atomic_helper_connector_reset, > - .fill_modes = drm_helper_probe_single_connector_modes, > - .destroy = drm_connector_cleanup, > - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > -}; > - > -static int lvds_encoder_attach(struct drm_bridge *bridge) > -{ > - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); > - struct drm_connector *connector = &lvds->connector; > - int ret; > - > - if (!bridge->encoder) { > - DRM_ERROR("Missing encoder\n"); > - return -ENODEV; > - } > - > - drm_connector_helper_add(connector, &lvds_connector_helper_funcs); > - > - ret = drm_connector_init(bridge->dev, connector, &lvds_connector_funcs, > - DRM_MODE_CONNECTOR_LVDS); > - if (ret) { > - DRM_ERROR("Failed to initialize connector\n"); > - return ret; > - } > - > - drm_mode_connector_attach_encoder(&lvds->connector, bridge->encoder); > - > - ret = drm_panel_attach(lvds->panel, &lvds->connector); > - if (ret < 0) > - return ret; > - > - return 0; > -} > - > -static void lvds_encoder_detach(struct drm_bridge *bridge) > -{ > - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); > - > - drm_panel_detach(lvds->panel); > -} > - > -static void lvds_encoder_pre_enable(struct drm_bridge *bridge) > -{ > - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); > - > - drm_panel_prepare(lvds->panel); > -} > - > -static void lvds_encoder_enable(struct drm_bridge *bridge) > -{ > - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); > - > - drm_panel_enable(lvds->panel); > -} > - > -static void lvds_encoder_disable(struct drm_bridge *bridge) > -{ > - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); > - > - drm_panel_disable(lvds->panel); > -} > - > -static void lvds_encoder_post_disable(struct drm_bridge *bridge) > -{ > - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); > - > - drm_panel_unprepare(lvds->panel); > -} > - > -static const struct drm_bridge_funcs lvds_encoder_bridge_funcs = { > - .attach = lvds_encoder_attach, > - .detach = lvds_encoder_detach, > - .pre_enable = lvds_encoder_pre_enable, > - .enable = lvds_encoder_enable, > - .disable = lvds_encoder_disable, > - .post_disable = lvds_encoder_post_disable, > -}; > - > static int lvds_encoder_probe(struct platform_device *pdev) > { > - struct lvds_encoder *lvds; > struct device_node *port; > struct device_node *endpoint; > - struct device_node *panel; > - > - lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL); > - if (!lvds) > - return -ENOMEM; > - > - lvds->dev = &pdev->dev; > - platform_set_drvdata(pdev, lvds); > - > - lvds->bridge.funcs = &lvds_encoder_bridge_funcs; > - lvds->bridge.of_node = pdev->dev.of_node; > + struct device_node *panel_node; > + struct drm_panel *panel; > + struct drm_bridge *bridge; > > /* Locate the panel DT node. */ > port = of_graph_get_port_by_id(pdev->dev.of_node, 1); > @@ -161,29 +35,35 @@ static int lvds_encoder_probe(struct platform_device *pdev) > return -ENXIO; > } > > - panel = of_graph_get_remote_port_parent(endpoint); > + panel_node = of_graph_get_remote_port_parent(endpoint); > of_node_put(endpoint); > - if (!panel) { > + if (!panel_node) { > dev_dbg(&pdev->dev, "no remote endpoint for port 1\n"); > return -ENXIO; > } > > - lvds->panel = of_drm_find_panel(panel); > - of_node_put(panel); > - if (!lvds->panel) { > + panel = of_drm_find_panel(panel_node); > + of_node_put(panel_node); > + if (!panel) { > dev_dbg(&pdev->dev, "panel not found, deferring probe\n"); > return -EPROBE_DEFER; > } > > - /* Register the bridge. */ > - return drm_bridge_add(&lvds->bridge); > + bridge = drm_panel_bridge_add(&pdev->dev, panel, > + DRM_MODE_CONNECTOR_LVDS); > + if (IS_ERR(bridge)) > + return PTR_ERR(bridge); > + > + platform_set_drvdata(pdev, bridge); > + > + return 0; > } > > static int lvds_encoder_remove(struct platform_device *pdev) > { > - struct lvds_encoder *encoder = platform_get_drvdata(pdev); > + struct drm_bridge *bridge = platform_get_drvdata(pdev); > > - drm_bridge_remove(&encoder->bridge); > + drm_bridge_remove(bridge); > > return 0; > } > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c > new file mode 100644 > index 000000000000..2081245455c6 > --- /dev/null > +++ b/drivers/gpu/drm/bridge/panel.c > @@ -0,0 +1,188 @@ > +/* > + * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + * Copyright (C) 2017 Broadcom > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation; either version 2 of > + * the License, or (at your option) any later version. > + */ > + > +#include <drm/drmP.h> > +#include <drm/drm_panel.h> > +#include <drm/drm_atomic_helper.h> > +#include <drm/drm_connector.h> > +#include <drm/drm_crtc_helper.h> > +#include <drm/drm_encoder.h> > +#include <drm/drm_modeset_helper_vtables.h> > +#include <drm/drm_panel.h> > + > +struct panel_bridge { > + struct drm_bridge bridge; > + struct device *dev; > + struct drm_connector connector; > + struct drm_panel *panel; > + u32 connector_type; > +}; > + > +static inline struct panel_bridge * > +drm_bridge_to_panel_bridge(struct drm_bridge *bridge) > +{ > + return container_of(bridge, struct panel_bridge, bridge); > +} > + > +static inline struct panel_bridge * > +drm_connector_to_panel_bridge(struct drm_connector *connector) > +{ > + return container_of(connector, struct panel_bridge, connector); > +} > + > +static int panel_bridge_connector_get_modes(struct drm_connector *connector) > +{ > + struct panel_bridge *panel_bridge = > + drm_connector_to_panel_bridge(connector); > + > + return drm_panel_get_modes(panel_bridge->panel); > +} > + > +static const struct drm_connector_helper_funcs panel_bridge_connector_helper_funcs = { > + .get_modes = panel_bridge_connector_get_modes, > +}; > + > +static const struct drm_connector_funcs panel_bridge_connector_funcs = { > + .dpms = drm_atomic_helper_connector_dpms, > + .reset = drm_atomic_helper_connector_reset, > + .fill_modes = drm_helper_probe_single_connector_modes, > + .destroy = drm_connector_cleanup, > + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > +}; > + > +static int panel_bridge_attach(struct drm_bridge *bridge) > +{ > + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > + struct drm_connector *connector = &panel_bridge->connector; > + int ret; > + > + if (!bridge->encoder) { > + DRM_ERROR("Missing encoder\n"); > + return -ENODEV; > + } > + > + drm_connector_helper_add(connector, > + &panel_bridge_connector_helper_funcs); > + > + ret = drm_connector_init(bridge->dev, connector, > + &panel_bridge_connector_funcs, > + panel_bridge->connector_type); > + if (ret) { > + DRM_ERROR("Failed to initialize connector\n"); > + return ret; > + } > + > + drm_mode_connector_attach_encoder(&panel_bridge->connector, > + bridge->encoder); > + > + ret = drm_panel_attach(panel_bridge->panel, &panel_bridge->connector); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static void panel_bridge_detach(struct drm_bridge *bridge) > +{ > + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > + > + drm_panel_detach(panel_bridge->panel); > +} > + > +static void panel_bridge_pre_enable(struct drm_bridge *bridge) > +{ > + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > + > + drm_panel_prepare(panel_bridge->panel); > +} > + > +static void panel_bridge_enable(struct drm_bridge *bridge) > +{ > + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > + > + drm_panel_enable(panel_bridge->panel); > +} > + > +static void panel_bridge_disable(struct drm_bridge *bridge) > +{ > + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > + > + drm_panel_disable(panel_bridge->panel); > +} > + > +static void panel_bridge_post_disable(struct drm_bridge *bridge) > +{ > + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > + > + drm_panel_unprepare(panel_bridge->panel); > +} > + > +static const struct drm_bridge_funcs panel_bridge_bridge_funcs = { > + .attach = panel_bridge_attach, > + .detach = panel_bridge_detach, > + .pre_enable = panel_bridge_pre_enable, > + .enable = panel_bridge_enable, > + .disable = panel_bridge_disable, > + .post_disable = panel_bridge_post_disable, > +}; > + > +/** > + * drm_panel_bridge_add - Creates a drm_bridge and drm_connector that > + * just call the appropriate functions from drm_panel. > + * > + * @dev: The struct device of the panel device. This is used for > + * registering the drm_bridge. > + * @panel: The drm_panel being wrapped. Must be non-NULL. > + * @connector_type: The DRM_MODE_CONNECTOR_* for the connector to be > + * created. > + * > + * For drivers converting from directly using drm_panel: The expected > + * usage pattern is that during either encoder module probe or DSI > + * host attach, a drm_panel will be looked up through > + * drm_of_find_panel_or_bridge(). drm_panel_bridge_add() is used to > + * wrap that panel in the new bridge, and the result can then be > + * passed to drm_bridge_attach(). The drm_panel_prepare() and related > + * functions can be dropped from the encoder driver (they're now > + * called by the KMS helpers before calling into the encoder), along > + * with connector creation. > + */ > +struct drm_bridge *drm_panel_bridge_add(struct device *dev, > + struct drm_panel *panel, > + u32 connector_type) > +{ > + struct panel_bridge *panel_bridge = > + devm_kzalloc(dev, sizeof(*panel_bridge), GFP_KERNEL); > + int ret; > + > + if (!dev || !panel) > + return ERR_PTR(EINVAL); > + > + panel_bridge->dev = dev; > + panel_bridge->connector_type = connector_type; > + panel_bridge->panel = panel; > + > + panel_bridge->bridge.funcs = &panel_bridge_bridge_funcs; > + panel_bridge->bridge.of_node = dev->of_node; > + > + ret = drm_bridge_add(&panel_bridge->bridge); > + if (ret) > + return ERR_PTR(ret); > + > + return &panel_bridge->bridge; > +} > +EXPORT_SYMBOL(drm_panel_bridge_add); > + > +void drm_panel_bridge_remove(struct device *dev, struct drm_bridge *bridge) > +{ > + drm_bridge_remove(bridge); > + devm_kfree(dev, bridge); > +} > +EXPORT_SYMBOL(drm_panel_bridge_remove); > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > index fdd82fcbf168..e08563ee0546 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -29,6 +29,7 @@ > #include <drm/drm_modes.h> > > struct drm_bridge; > +struct drm_panel; > > /** > * struct drm_bridge_funcs - drm_bridge control functions > @@ -221,4 +222,11 @@ void drm_bridge_mode_set(struct drm_bridge *bridge, > void drm_bridge_pre_enable(struct drm_bridge *bridge); > void drm_bridge_enable(struct drm_bridge *bridge); > > +#ifdef CONFIG_DRM_PANEL > +struct drm_bridge *drm_panel_bridge_add(struct device *dev, > + struct drm_panel *panel, > + u32 connector_type); > +void drm_panel_bridge_remove(struct device *dev, struct drm_bridge *bridge); > +#endif > + > #endif > -- > 2.11.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, May 03, 2017 at 12:36:06PM +0300, Laurent Pinchart wrote: > Hi Daniel, > > On Wednesday 03 May 2017 11:32:17 Daniel Vetter wrote: > > On Wed, May 03, 2017 at 02:53:00PM +0530, Archit Taneja wrote: > > > +panel/bridge reviewers. > > > > > > This does make things much cleaner, but it seems a bit strange to create > > > a drm_bridge when there isn't really a HW bridge in the display chain > > > (i.e, when the DSI encoder is directly connected to a DSI panel). > > > > > > There are kms drivers that use drm_panel, but don't have simple stub > > > connectors that wrap around a drm_panel. They have more complicated > > > connector ops, and may call drm_panel_prepare() and related functions a > > > bit differently. We won't be able to use drm_panel_bridge for those > > > drivers. > > > > > > For msm, we check whether the DSI encoder is connected directly to a panel > > > or an external bridge. If it's connected to an external bridge, we skip > > > the creation of the stub connector, and rely on the external bridge driver > > > to create the connector: > > > > > > http://lxr.free-electrons.com/source/drivers/gpu/drm/msm/dsi/dsi.c#L227 > > > > > > The msm solution isn't very neat, but it avoids the need to create another > > > bridge to glue things together. > > > > Since I suggested this, yes I like it. And I think just unconditionally > > creating the panel bridge is probably even simpler, after all bridges are > > supposed to be chainable. I guess there's always going to be drivers where > > we need special handling, but I'm kinda hoping that for most cases simply > > plugging in a panel bridge is all that's need to glue drm_panel support > > into a driver. The simple pipe helpers do support bridges, and part of the > > goal there very much was to make it easy to glue in panel drivers. > > As I've just explained in another reply, I don't see the point in doing this > when we can instead refactor the bridge and panel operations to expose a > common base object that will then be easy to handle in core code. This isn't > just for panels, as connectors should have DT nodes, it makes sense to > instantiate an object for them that can be handled by the DRM core, without > having to push connector handling in all bridge drivers. Imo you're aiming too high. We have 21 bridge drivers and 11 panel drivers. Asking someone to refactor them all (plus all the callers and everything) means it won't happen. At least I personally will definitely not block a contribution on this happening, that's a totally outsized demand. What we could do instead is slowly merge these two worlds together, and this here is definitely a step into that direction. Let's please not throw out useful improvements by insisting that we only merge perfect code. We already did merge both drm_panel and drm_bridge (plus a few more earlier attempts), clearly we're not only merging perfect code :-) Or you go ahead and deliver that refactoring, that's another option ofc ... Cheers, Daniel
Hi Daniel, On Wednesday 03 May 2017 16:28:56 Daniel Vetter wrote: > On Wed, May 03, 2017 at 12:36:06PM +0300, Laurent Pinchart wrote: > > On Wednesday 03 May 2017 11:32:17 Daniel Vetter wrote: > >> On Wed, May 03, 2017 at 02:53:00PM +0530, Archit Taneja wrote: > >>> +panel/bridge reviewers. > >>> > >>> This does make things much cleaner, but it seems a bit strange to > >>> create a drm_bridge when there isn't really a HW bridge in the display > >>> chain (i.e, when the DSI encoder is directly connected to a DSI panel). > >>> > >>> There are kms drivers that use drm_panel, but don't have simple stub > >>> connectors that wrap around a drm_panel. They have more complicated > >>> connector ops, and may call drm_panel_prepare() and related functions > >>> a bit differently. We won't be able to use drm_panel_bridge for those > >>> drivers. > >>> > >>> For msm, we check whether the DSI encoder is connected directly to a > >>> panel or an external bridge. If it's connected to an external bridge, > >>> we skip the creation of the stub connector, and rely on the external > >>> bridge driver to create the connector: > >>> > >>> http://lxr.free-electrons.com/source/drivers/gpu/drm/msm/dsi/dsi.c#L22 > >>> 7 > >>> > >>> The msm solution isn't very neat, but it avoids the need to create > >>> another bridge to glue things together. > >> > >> Since I suggested this, yes I like it. And I think just unconditionally > >> creating the panel bridge is probably even simpler, after all bridges > >> are supposed to be chainable. I guess there's always going to be drivers > >> where we need special handling, but I'm kinda hoping that for most cases > >> simply plugging in a panel bridge is all that's need to glue drm_panel > >> support into a driver. The simple pipe helpers do support bridges, and > >> part of the goal there very much was to make it easy to glue in panel > >> drivers. > > > > As I've just explained in another reply, I don't see the point in doing > > this when we can instead refactor the bridge and panel operations to > > expose a common base object that will then be easy to handle in core > > code. This isn't just for panels, as connectors should have DT nodes, it > > makes sense to instantiate an object for them that can be handled by the > > DRM core, without having to push connector handling in all bridge > > drivers. > > Imo you're aiming too high. We have 21 bridge drivers and 11 panel > drivers. Asking someone to refactor them all (plus all the callers and > everything) means it won't happen. At least I personally will definitely > not block a contribution on this happening, that's a totally outsized > demand. I think you're aiming too low. When the atomic update API was introduced I could have told you that converting all drivers was an impossible task ;-) Jokes aside, I believe it might be possible to implement something simple. I'm flexible about the naming, so instead of creating a new base structure and refactor drm_bridge and drm_panel to embed it, we could as a first step use drm_bridge as that base structure. We would only need to embed a drm_bridge instance in drm_panel and add a few connector-related operations to the bridge ops structure. As existing bridge drivers wouldn't need to provide those new ops, they wouldn't need to be touched. > What we could do instead is slowly merge these two worlds together, and > this here is definitely a step into that direction. Let's please not throw > out useful improvements by insisting that we only merge perfect code. We > already did merge both drm_panel and drm_bridge (plus a few more earlier > attempts), clearly we're not only merging perfect code :-) > > Or you go ahead and deliver that refactoring, that's another option ofc > ... It's on my to-do list for the near future actually, in order to convert the omapdrm-specific bridge and panel drivers into standard DRM drivers. I'd like to get a general agreement on the direction I'd like to take before converting everything though, so I'd appreciate your feedback on the thoughts above.
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > Hi Daniel, > > On Wednesday 03 May 2017 16:28:56 Daniel Vetter wrote: >> On Wed, May 03, 2017 at 12:36:06PM +0300, Laurent Pinchart wrote: >> > On Wednesday 03 May 2017 11:32:17 Daniel Vetter wrote: >> >> On Wed, May 03, 2017 at 02:53:00PM +0530, Archit Taneja wrote: >> >>> +panel/bridge reviewers. >> >>> >> >>> This does make things much cleaner, but it seems a bit strange to >> >>> create a drm_bridge when there isn't really a HW bridge in the display >> >>> chain (i.e, when the DSI encoder is directly connected to a DSI panel). >> >>> >> >>> There are kms drivers that use drm_panel, but don't have simple stub >> >>> connectors that wrap around a drm_panel. They have more complicated >> >>> connector ops, and may call drm_panel_prepare() and related functions >> >>> a bit differently. We won't be able to use drm_panel_bridge for those >> >>> drivers. >> >>> >> >>> For msm, we check whether the DSI encoder is connected directly to a >> >>> panel or an external bridge. If it's connected to an external bridge, >> >>> we skip the creation of the stub connector, and rely on the external >> >>> bridge driver to create the connector: >> >>> >> >>> http://lxr.free-electrons.com/source/drivers/gpu/drm/msm/dsi/dsi.c#L22 >> >>> 7 >> >>> >> >>> The msm solution isn't very neat, but it avoids the need to create >> >>> another bridge to glue things together. >> >> >> >> Since I suggested this, yes I like it. And I think just unconditionally >> >> creating the panel bridge is probably even simpler, after all bridges >> >> are supposed to be chainable. I guess there's always going to be drivers >> >> where we need special handling, but I'm kinda hoping that for most cases >> >> simply plugging in a panel bridge is all that's need to glue drm_panel >> >> support into a driver. The simple pipe helpers do support bridges, and >> >> part of the goal there very much was to make it easy to glue in panel >> >> drivers. >> > >> > As I've just explained in another reply, I don't see the point in doing >> > this when we can instead refactor the bridge and panel operations to >> > expose a common base object that will then be easy to handle in core >> > code. This isn't just for panels, as connectors should have DT nodes, it >> > makes sense to instantiate an object for them that can be handled by the >> > DRM core, without having to push connector handling in all bridge >> > drivers. >> >> Imo you're aiming too high. We have 21 bridge drivers and 11 panel >> drivers. Asking someone to refactor them all (plus all the callers and >> everything) means it won't happen. At least I personally will definitely >> not block a contribution on this happening, that's a totally outsized >> demand. > > I think you're aiming too low. When the atomic update API was introduced I > could have told you that converting all drivers was an impossible task ;-) > > Jokes aside, I believe it might be possible to implement something simple. I'm > flexible about the naming, so instead of creating a new base structure and > refactor drm_bridge and drm_panel to embed it, we could as a first step use > drm_bridge as that base structure. We would only need to embed a drm_bridge > instance in drm_panel and add a few connector-related operations to the bridge > ops structure. As existing bridge drivers wouldn't need to provide those new > ops, they wouldn't need to be touched. I think drm_bridge itself should be the base structure, as it is today. It's got the entrypoints we want, connected in at the right level in the modeset chain. You could call it the "drm_display_chain_link" or something, but "bridge" is short and I don't think renaming is worth the trouble. This helper just makes it easy to make one of these display chain links for the endpoint of your display chain when it's a drm_panel. One of the followons I'd like to see after this is to make a helper that drivers get to use that does the "grab a bridge? no? grab a panel? Ok? wrap it in a bridge" pattern. Then the drm_panel/drm_connector are an implementation detail of whatever is at the end of the chain, and not something that the middle of the chain needs to know about. The alternative to that that I see would be to have panel drivers call this code to advertise themselves through the list of bridges.
Archit Taneja <architt@codeaurora.org> writes: > Hi, > > On 04/27/2017 10:06 PM, Eric Anholt wrote: >> Many DRM drivers have common code to make a stub connector >> implementation that wraps a drm_panel. By wrapping the panel in a DRM >> bridge, all of the connector code (including calls during encoder >> enable/disable) goes away. >> >> Signed-off-by: Eric Anholt <eric@anholt.net> >> --- >> Documentation/gpu/drm-kms-helpers.rst | 3 + >> drivers/gpu/drm/bridge/Kconfig | 11 +- >> drivers/gpu/drm/bridge/Makefile | 1 + >> drivers/gpu/drm/bridge/lvds-encoder.c | 158 ++++------------------------ >> drivers/gpu/drm/bridge/panel.c | 188 ++++++++++++++++++++++++++++++++++ >> include/drm/drm_bridge.h | 8 ++ >> 6 files changed, 228 insertions(+), 141 deletions(-) >> create mode 100644 drivers/gpu/drm/bridge/panel.c >> >> diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst >> index c075aadd7078..60eb3b41702b 100644 >> --- a/Documentation/gpu/drm-kms-helpers.rst >> +++ b/Documentation/gpu/drm-kms-helpers.rst >> @@ -143,6 +143,9 @@ Bridge Helper Reference >> .. kernel-doc:: drivers/gpu/drm/drm_bridge.c >> :export: >> >> +.. kernel-doc:: drivers/gpu/drm/bridge/panel.c >> + :export: >> + >> .. _drm_panel_helper: >> >> Panel Helper Reference >> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig >> index f6968d3b4b41..c4daca38743c 100644 >> --- a/drivers/gpu/drm/bridge/Kconfig >> +++ b/drivers/gpu/drm/bridge/Kconfig >> @@ -4,6 +4,14 @@ config DRM_BRIDGE >> help >> Bridge registration and lookup framework. >> >> +config DRM_PANEL_BRIDGE >> + def_bool y >> + depends on DRM_BRIDGE >> + select DRM_KMS_HELPER >> + select DRM_PANEL >> + help >> + DRM bridge wrapper of DRM panels >> + >> menu "Display Interface Bridges" >> depends on DRM && DRM_BRIDGE >> >> @@ -27,8 +35,7 @@ config DRM_DUMB_VGA_DAC >> config DRM_LVDS_ENCODER >> tristate "Transparent parallel to LVDS encoder support" >> depends on OF >> - select DRM_KMS_HELPER >> - select DRM_PANEL >> + select DRM_PANEL_BRIDGE >> help >> Support for transparent parallel to LVDS encoders that don't require >> any configuration. >> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile >> index 3fe2226ee2f2..40a43750ad55 100644 >> --- a/drivers/gpu/drm/bridge/Makefile >> +++ b/drivers/gpu/drm/bridge/Makefile >> @@ -5,6 +5,7 @@ obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o >> obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o >> obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o >> obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o >> +obj-$(CONFIG_DRM_PANEL_BRIDGE) += panel.o >> obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o >> obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o >> obj-$(CONFIG_DRM_SII902X) += sii902x.o >> diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c >> index f1f67a279426..04e1504c4d8f 100644 >> --- a/drivers/gpu/drm/bridge/lvds-encoder.c >> +++ b/drivers/gpu/drm/bridge/lvds-encoder.c >> @@ -8,144 +8,18 @@ >> */ >> >> #include <drm/drmP.h> >> -#include <drm/drm_atomic_helper.h> >> -#include <drm/drm_connector.h> >> -#include <drm/drm_crtc_helper.h> >> -#include <drm/drm_encoder.h> >> -#include <drm/drm_modeset_helper_vtables.h> >> +#include <drm/drm_bridge.h> >> #include <drm/drm_panel.h> >> >> #include <linux/of_graph.h> >> >> -struct lvds_encoder { >> - struct device *dev; >> - >> - struct drm_bridge bridge; >> - struct drm_connector connector; >> - struct drm_panel *panel; >> -}; >> - >> -static inline struct lvds_encoder * >> -drm_bridge_to_lvds_encoder(struct drm_bridge *bridge) >> -{ >> - return container_of(bridge, struct lvds_encoder, bridge); >> -} >> - >> -static inline struct lvds_encoder * >> -drm_connector_to_lvds_encoder(struct drm_connector *connector) >> -{ >> - return container_of(connector, struct lvds_encoder, connector); >> -} >> - >> -static int lvds_connector_get_modes(struct drm_connector *connector) >> -{ >> - struct lvds_encoder *lvds = drm_connector_to_lvds_encoder(connector); >> - >> - return drm_panel_get_modes(lvds->panel); >> -} >> - >> -static const struct drm_connector_helper_funcs lvds_connector_helper_funcs = { >> - .get_modes = lvds_connector_get_modes, >> -}; >> - >> -static const struct drm_connector_funcs lvds_connector_funcs = { >> - .dpms = drm_atomic_helper_connector_dpms, >> - .reset = drm_atomic_helper_connector_reset, >> - .fill_modes = drm_helper_probe_single_connector_modes, >> - .destroy = drm_connector_cleanup, >> - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, >> - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, >> -}; >> - >> -static int lvds_encoder_attach(struct drm_bridge *bridge) >> -{ >> - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); >> - struct drm_connector *connector = &lvds->connector; >> - int ret; >> - >> - if (!bridge->encoder) { >> - DRM_ERROR("Missing encoder\n"); >> - return -ENODEV; >> - } >> - >> - drm_connector_helper_add(connector, &lvds_connector_helper_funcs); >> - >> - ret = drm_connector_init(bridge->dev, connector, &lvds_connector_funcs, >> - DRM_MODE_CONNECTOR_LVDS); >> - if (ret) { >> - DRM_ERROR("Failed to initialize connector\n"); >> - return ret; >> - } >> - >> - drm_mode_connector_attach_encoder(&lvds->connector, bridge->encoder); >> - >> - ret = drm_panel_attach(lvds->panel, &lvds->connector); >> - if (ret < 0) >> - return ret; >> - >> - return 0; >> -} >> - >> -static void lvds_encoder_detach(struct drm_bridge *bridge) >> -{ >> - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); >> - >> - drm_panel_detach(lvds->panel); >> -} >> - >> -static void lvds_encoder_pre_enable(struct drm_bridge *bridge) >> -{ >> - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); >> - >> - drm_panel_prepare(lvds->panel); >> -} >> - >> -static void lvds_encoder_enable(struct drm_bridge *bridge) >> -{ >> - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); >> - >> - drm_panel_enable(lvds->panel); >> -} >> - >> -static void lvds_encoder_disable(struct drm_bridge *bridge) >> -{ >> - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); >> - >> - drm_panel_disable(lvds->panel); >> -} >> - >> -static void lvds_encoder_post_disable(struct drm_bridge *bridge) >> -{ >> - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); >> - >> - drm_panel_unprepare(lvds->panel); >> -} >> - >> -static const struct drm_bridge_funcs lvds_encoder_bridge_funcs = { >> - .attach = lvds_encoder_attach, >> - .detach = lvds_encoder_detach, >> - .pre_enable = lvds_encoder_pre_enable, >> - .enable = lvds_encoder_enable, >> - .disable = lvds_encoder_disable, >> - .post_disable = lvds_encoder_post_disable, >> -}; >> - >> static int lvds_encoder_probe(struct platform_device *pdev) >> { >> - struct lvds_encoder *lvds; >> struct device_node *port; >> struct device_node *endpoint; >> - struct device_node *panel; >> - >> - lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL); >> - if (!lvds) >> - return -ENOMEM; >> - >> - lvds->dev = &pdev->dev; >> - platform_set_drvdata(pdev, lvds); >> - >> - lvds->bridge.funcs = &lvds_encoder_bridge_funcs; >> - lvds->bridge.of_node = pdev->dev.of_node; >> + struct device_node *panel_node; >> + struct drm_panel *panel; >> + struct drm_bridge *bridge; >> >> /* Locate the panel DT node. */ >> port = of_graph_get_port_by_id(pdev->dev.of_node, 1); >> @@ -161,29 +35,35 @@ static int lvds_encoder_probe(struct platform_device *pdev) >> return -ENXIO; >> } >> >> - panel = of_graph_get_remote_port_parent(endpoint); >> + panel_node = of_graph_get_remote_port_parent(endpoint); >> of_node_put(endpoint); >> - if (!panel) { >> + if (!panel_node) { >> dev_dbg(&pdev->dev, "no remote endpoint for port 1\n"); >> return -ENXIO; >> } >> >> - lvds->panel = of_drm_find_panel(panel); >> - of_node_put(panel); >> - if (!lvds->panel) { >> + panel = of_drm_find_panel(panel_node); >> + of_node_put(panel_node); >> + if (!panel) { >> dev_dbg(&pdev->dev, "panel not found, deferring probe\n"); >> return -EPROBE_DEFER; >> } >> >> - /* Register the bridge. */ >> - return drm_bridge_add(&lvds->bridge); >> + bridge = drm_panel_bridge_add(&pdev->dev, panel, >> + DRM_MODE_CONNECTOR_LVDS); >> + if (IS_ERR(bridge)) >> + return PTR_ERR(bridge); >> + >> + platform_set_drvdata(pdev, bridge); >> + >> + return 0; >> } >> >> static int lvds_encoder_remove(struct platform_device *pdev) >> { >> - struct lvds_encoder *encoder = platform_get_drvdata(pdev); >> + struct drm_bridge *bridge = platform_get_drvdata(pdev); >> >> - drm_bridge_remove(&encoder->bridge); >> + drm_bridge_remove(bridge); >> >> return 0; >> } >> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c >> new file mode 100644 >> index 000000000000..2081245455c6 >> --- /dev/null >> +++ b/drivers/gpu/drm/bridge/panel.c >> @@ -0,0 +1,188 @@ >> +/* >> + * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> + * Copyright (C) 2017 Broadcom >> + * >> + * This program is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU General Public License as >> + * published by the Free Software Foundation; either version 2 of >> + * the License, or (at your option) any later version. >> + */ >> + >> +#include <drm/drmP.h> >> +#include <drm/drm_panel.h> >> +#include <drm/drm_atomic_helper.h> >> +#include <drm/drm_connector.h> >> +#include <drm/drm_crtc_helper.h> >> +#include <drm/drm_encoder.h> >> +#include <drm/drm_modeset_helper_vtables.h> >> +#include <drm/drm_panel.h> >> + >> +struct panel_bridge { >> + struct drm_bridge bridge; >> + struct device *dev; >> + struct drm_connector connector; >> + struct drm_panel *panel; >> + u32 connector_type; >> +}; >> + >> +static inline struct panel_bridge * >> +drm_bridge_to_panel_bridge(struct drm_bridge *bridge) >> +{ >> + return container_of(bridge, struct panel_bridge, bridge); >> +} >> + >> +static inline struct panel_bridge * >> +drm_connector_to_panel_bridge(struct drm_connector *connector) >> +{ >> + return container_of(connector, struct panel_bridge, connector); >> +} >> + >> +static int panel_bridge_connector_get_modes(struct drm_connector *connector) >> +{ >> + struct panel_bridge *panel_bridge = >> + drm_connector_to_panel_bridge(connector); >> + >> + return drm_panel_get_modes(panel_bridge->panel); >> +} >> + >> +static const struct drm_connector_helper_funcs panel_bridge_connector_helper_funcs = { >> + .get_modes = panel_bridge_connector_get_modes, >> +}; >> + >> +static const struct drm_connector_funcs panel_bridge_connector_funcs = { >> + .dpms = drm_atomic_helper_connector_dpms, >> + .reset = drm_atomic_helper_connector_reset, >> + .fill_modes = drm_helper_probe_single_connector_modes, >> + .destroy = drm_connector_cleanup, >> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, >> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, >> +}; >> + >> +static int panel_bridge_attach(struct drm_bridge *bridge) >> +{ >> + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); >> + struct drm_connector *connector = &panel_bridge->connector; >> + int ret; >> + >> + if (!bridge->encoder) { >> + DRM_ERROR("Missing encoder\n"); >> + return -ENODEV; >> + } >> + >> + drm_connector_helper_add(connector, >> + &panel_bridge_connector_helper_funcs); >> + >> + ret = drm_connector_init(bridge->dev, connector, >> + &panel_bridge_connector_funcs, >> + panel_bridge->connector_type); >> + if (ret) { >> + DRM_ERROR("Failed to initialize connector\n"); >> + return ret; >> + } >> + >> + drm_mode_connector_attach_encoder(&panel_bridge->connector, >> + bridge->encoder); >> + >> + ret = drm_panel_attach(panel_bridge->panel, &panel_bridge->connector); >> + if (ret < 0) >> + return ret; >> + >> + return 0; >> +} >> + >> +static void panel_bridge_detach(struct drm_bridge *bridge) >> +{ >> + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); >> + >> + drm_panel_detach(panel_bridge->panel); >> +} >> + >> +static void panel_bridge_pre_enable(struct drm_bridge *bridge) >> +{ >> + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); >> + >> + drm_panel_prepare(panel_bridge->panel); >> +} >> + >> +static void panel_bridge_enable(struct drm_bridge *bridge) >> +{ >> + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); >> + >> + drm_panel_enable(panel_bridge->panel); >> +} >> + >> +static void panel_bridge_disable(struct drm_bridge *bridge) >> +{ >> + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); >> + >> + drm_panel_disable(panel_bridge->panel); >> +} >> + >> +static void panel_bridge_post_disable(struct drm_bridge *bridge) >> +{ >> + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); >> + >> + drm_panel_unprepare(panel_bridge->panel); >> +} >> + >> +static const struct drm_bridge_funcs panel_bridge_bridge_funcs = { >> + .attach = panel_bridge_attach, >> + .detach = panel_bridge_detach, >> + .pre_enable = panel_bridge_pre_enable, >> + .enable = panel_bridge_enable, >> + .disable = panel_bridge_disable, >> + .post_disable = panel_bridge_post_disable, >> +}; >> + >> +/** >> + * drm_panel_bridge_add - Creates a drm_bridge and drm_connector that >> + * just call the appropriate functions from drm_panel. >> + * >> + * @dev: The struct device of the panel device. This is used for >> + * registering the drm_bridge. >> + * @panel: The drm_panel being wrapped. Must be non-NULL. >> + * @connector_type: The DRM_MODE_CONNECTOR_* for the connector to be >> + * created. >> + * >> + * For drivers converting from directly using drm_panel: The expected >> + * usage pattern is that during either encoder module probe or DSI >> + * host attach, a drm_panel will be looked up through >> + * drm_of_find_panel_or_bridge(). drm_panel_bridge_add() is used to >> + * wrap that panel in the new bridge, and the result can then be >> + * passed to drm_bridge_attach(). The drm_panel_prepare() and related >> + * functions can be dropped from the encoder driver (they're now >> + * called by the KMS helpers before calling into the encoder), along >> + * with connector creation. > > +panel/bridge reviewers. > > This does make things much cleaner, but it seems a bit strange to create > a drm_bridge when there isn't really a HW bridge in the display chain (i.e, > when the DSI encoder is directly connected to a DSI panel). > > There are kms drivers that use drm_panel, but don't have simple stub connectors > that wrap around a drm_panel. They have more complicated connector ops, and > may call drm_panel_prepare() and related functions a bit differently. We won't > be able to use drm_panel_bridge for those drivers. Can you point to examples? The only thing on my mind that would be missing if drivers converted to bridges only is having your encoder be able to filter the mode list for timing limits. That seems like something you'd want any link in the display chain to be able to do, so we may want to extend drm_bridge for handling that. Maybe you're thinking of something else, though. > For msm, we check whether the DSI encoder is connected directly to a panel > or an external bridge. If it's connected to an external bridge, we skip the > creation of the stub connector, and rely on the external bridge driver to > create the connector: > > http://lxr.free-electrons.com/source/drivers/gpu/drm/msm/dsi/dsi.c#L227 > > The msm solution isn't very neat, but it avoids the need to create another > bridge to glue things together. Yeah, that's basically the code I was going to have to replicate, which is why I wrote a generic helper instead. I think allocating a drm_bridge (which isn't visible to userspace) is a small price to pay for cutting 100 lines of boilerplate from a driver.
On Wed, May 3, 2017 at 6:17 PM, Eric Anholt <eric@anholt.net> wrote: > Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > >> Hi Daniel, >> >> On Wednesday 03 May 2017 16:28:56 Daniel Vetter wrote: >>> On Wed, May 03, 2017 at 12:36:06PM +0300, Laurent Pinchart wrote: >>> > On Wednesday 03 May 2017 11:32:17 Daniel Vetter wrote: >>> >> On Wed, May 03, 2017 at 02:53:00PM +0530, Archit Taneja wrote: >>> >>> +panel/bridge reviewers. >>> >>> >>> >>> This does make things much cleaner, but it seems a bit strange to >>> >>> create a drm_bridge when there isn't really a HW bridge in the display >>> >>> chain (i.e, when the DSI encoder is directly connected to a DSI panel). >>> >>> >>> >>> There are kms drivers that use drm_panel, but don't have simple stub >>> >>> connectors that wrap around a drm_panel. They have more complicated >>> >>> connector ops, and may call drm_panel_prepare() and related functions >>> >>> a bit differently. We won't be able to use drm_panel_bridge for those >>> >>> drivers. >>> >>> >>> >>> For msm, we check whether the DSI encoder is connected directly to a >>> >>> panel or an external bridge. If it's connected to an external bridge, >>> >>> we skip the creation of the stub connector, and rely on the external >>> >>> bridge driver to create the connector: >>> >>> >>> >>> http://lxr.free-electrons.com/source/drivers/gpu/drm/msm/dsi/dsi.c#L22 >>> >>> 7 >>> >>> >>> >>> The msm solution isn't very neat, but it avoids the need to create >>> >>> another bridge to glue things together. >>> >> >>> >> Since I suggested this, yes I like it. And I think just unconditionally >>> >> creating the panel bridge is probably even simpler, after all bridges >>> >> are supposed to be chainable. I guess there's always going to be drivers >>> >> where we need special handling, but I'm kinda hoping that for most cases >>> >> simply plugging in a panel bridge is all that's need to glue drm_panel >>> >> support into a driver. The simple pipe helpers do support bridges, and >>> >> part of the goal there very much was to make it easy to glue in panel >>> >> drivers. >>> > >>> > As I've just explained in another reply, I don't see the point in doing >>> > this when we can instead refactor the bridge and panel operations to >>> > expose a common base object that will then be easy to handle in core >>> > code. This isn't just for panels, as connectors should have DT nodes, it >>> > makes sense to instantiate an object for them that can be handled by the >>> > DRM core, without having to push connector handling in all bridge >>> > drivers. >>> >>> Imo you're aiming too high. We have 21 bridge drivers and 11 panel >>> drivers. Asking someone to refactor them all (plus all the callers and >>> everything) means it won't happen. At least I personally will definitely >>> not block a contribution on this happening, that's a totally outsized >>> demand. >> >> I think you're aiming too low. When the atomic update API was introduced I >> could have told you that converting all drivers was an impossible task ;-) We still have non-atomic drivers. We will have non-atomic drivers for a _very_ long time. Heck, we still have non-kms drivers in drm. And especially with atomic we've started out with some helpers to glue the new world into the old one, not by refactoring existing interfaces. That's somewhat starting to happen now, 2 years and 20+ drivers later. You've just proven my point I think :-) >> Jokes aside, I believe it might be possible to implement something simple. I'm >> flexible about the naming, so instead of creating a new base structure and >> refactor drm_bridge and drm_panel to embed it, we could as a first step use >> drm_bridge as that base structure. We would only need to embed a drm_bridge >> instance in drm_panel and add a few connector-related operations to the bridge >> ops structure. As existing bridge drivers wouldn't need to provide those new >> ops, they wouldn't need to be touched. > > I think drm_bridge itself should be the base structure, as it is today. > It's got the entrypoints we want, connected in at the right level in the > modeset chain. You could call it the "drm_display_chain_link" or > something, but "bridge" is short and I don't think renaming is worth the > trouble. This helper just makes it easy to make one of these display > chain links for the endpoint of your display chain when it's a > drm_panel. > > One of the followons I'd like to see after this is to make a helper that > drivers get to use that does the "grab a bridge? no? grab a panel? > Ok? wrap it in a bridge" pattern. Then the drm_panel/drm_connector are > an implementation detail of whatever is at the end of the chain, and not > something that the middle of the chain needs to know about. > > The alternative to that that I see would be to have panel drivers call > this code to advertise themselves through the list of bridges. Yeah, I do believe that Eric's series here is actual the right (first, among a lot more) steps into the direction of unifying panels and bridges. Atm, if you want to have this shiny new unified world you get to deal with n bridges, m panels and bunch of callers for each. With Eric's work here we can get the n:m out of this refactoring problem by making panels looks like bridges for everyone outside at least. And then, once that part is done (we might, or might not need to clarify something in the bridge interface), we can then push that bridge interface down, or at least embed it into drm_panel and unify them. But at that point it's only a 1:m issue (hopefully at least). Asking for the entire refactoring in one go will just increase the odds we'll get it wrong imo. -Daniel
On 05/03/2017 10:00 PM, Eric Anholt wrote: > Archit Taneja <architt@codeaurora.org> writes: > >> Hi, >> >> On 04/27/2017 10:06 PM, Eric Anholt wrote: >>> Many DRM drivers have common code to make a stub connector >>> implementation that wraps a drm_panel. By wrapping the panel in a DRM >>> bridge, all of the connector code (including calls during encoder >>> enable/disable) goes away. >>> >>> Signed-off-by: Eric Anholt <eric@anholt.net> >>> --- >>> Documentation/gpu/drm-kms-helpers.rst | 3 + >>> drivers/gpu/drm/bridge/Kconfig | 11 +- >>> drivers/gpu/drm/bridge/Makefile | 1 + >>> drivers/gpu/drm/bridge/lvds-encoder.c | 158 ++++------------------------ >>> drivers/gpu/drm/bridge/panel.c | 188 ++++++++++++++++++++++++++++++++++ >>> include/drm/drm_bridge.h | 8 ++ >>> 6 files changed, 228 insertions(+), 141 deletions(-) >>> create mode 100644 drivers/gpu/drm/bridge/panel.c >>> >>> diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst >>> index c075aadd7078..60eb3b41702b 100644 >>> --- a/Documentation/gpu/drm-kms-helpers.rst >>> +++ b/Documentation/gpu/drm-kms-helpers.rst >>> @@ -143,6 +143,9 @@ Bridge Helper Reference >>> .. kernel-doc:: drivers/gpu/drm/drm_bridge.c >>> :export: >>> >>> +.. kernel-doc:: drivers/gpu/drm/bridge/panel.c >>> + :export: >>> + >>> .. _drm_panel_helper: >>> >>> Panel Helper Reference >>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig >>> index f6968d3b4b41..c4daca38743c 100644 >>> --- a/drivers/gpu/drm/bridge/Kconfig >>> +++ b/drivers/gpu/drm/bridge/Kconfig >>> @@ -4,6 +4,14 @@ config DRM_BRIDGE >>> help >>> Bridge registration and lookup framework. >>> >>> +config DRM_PANEL_BRIDGE >>> + def_bool y >>> + depends on DRM_BRIDGE >>> + select DRM_KMS_HELPER >>> + select DRM_PANEL >>> + help >>> + DRM bridge wrapper of DRM panels >>> + >>> menu "Display Interface Bridges" >>> depends on DRM && DRM_BRIDGE >>> >>> @@ -27,8 +35,7 @@ config DRM_DUMB_VGA_DAC >>> config DRM_LVDS_ENCODER >>> tristate "Transparent parallel to LVDS encoder support" >>> depends on OF >>> - select DRM_KMS_HELPER >>> - select DRM_PANEL >>> + select DRM_PANEL_BRIDGE >>> help >>> Support for transparent parallel to LVDS encoders that don't require >>> any configuration. >>> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile >>> index 3fe2226ee2f2..40a43750ad55 100644 >>> --- a/drivers/gpu/drm/bridge/Makefile >>> +++ b/drivers/gpu/drm/bridge/Makefile >>> @@ -5,6 +5,7 @@ obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o >>> obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o >>> obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o >>> obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o >>> +obj-$(CONFIG_DRM_PANEL_BRIDGE) += panel.o >>> obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o >>> obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o >>> obj-$(CONFIG_DRM_SII902X) += sii902x.o >>> diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c >>> index f1f67a279426..04e1504c4d8f 100644 >>> --- a/drivers/gpu/drm/bridge/lvds-encoder.c >>> +++ b/drivers/gpu/drm/bridge/lvds-encoder.c >>> @@ -8,144 +8,18 @@ >>> */ >>> >>> #include <drm/drmP.h> >>> -#include <drm/drm_atomic_helper.h> >>> -#include <drm/drm_connector.h> >>> -#include <drm/drm_crtc_helper.h> >>> -#include <drm/drm_encoder.h> >>> -#include <drm/drm_modeset_helper_vtables.h> >>> +#include <drm/drm_bridge.h> >>> #include <drm/drm_panel.h> >>> >>> #include <linux/of_graph.h> >>> >>> -struct lvds_encoder { >>> - struct device *dev; >>> - >>> - struct drm_bridge bridge; >>> - struct drm_connector connector; >>> - struct drm_panel *panel; >>> -}; >>> - >>> -static inline struct lvds_encoder * >>> -drm_bridge_to_lvds_encoder(struct drm_bridge *bridge) >>> -{ >>> - return container_of(bridge, struct lvds_encoder, bridge); >>> -} >>> - >>> -static inline struct lvds_encoder * >>> -drm_connector_to_lvds_encoder(struct drm_connector *connector) >>> -{ >>> - return container_of(connector, struct lvds_encoder, connector); >>> -} >>> - >>> -static int lvds_connector_get_modes(struct drm_connector *connector) >>> -{ >>> - struct lvds_encoder *lvds = drm_connector_to_lvds_encoder(connector); >>> - >>> - return drm_panel_get_modes(lvds->panel); >>> -} >>> - >>> -static const struct drm_connector_helper_funcs lvds_connector_helper_funcs = { >>> - .get_modes = lvds_connector_get_modes, >>> -}; >>> - >>> -static const struct drm_connector_funcs lvds_connector_funcs = { >>> - .dpms = drm_atomic_helper_connector_dpms, >>> - .reset = drm_atomic_helper_connector_reset, >>> - .fill_modes = drm_helper_probe_single_connector_modes, >>> - .destroy = drm_connector_cleanup, >>> - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, >>> - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, >>> -}; >>> - >>> -static int lvds_encoder_attach(struct drm_bridge *bridge) >>> -{ >>> - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); >>> - struct drm_connector *connector = &lvds->connector; >>> - int ret; >>> - >>> - if (!bridge->encoder) { >>> - DRM_ERROR("Missing encoder\n"); >>> - return -ENODEV; >>> - } >>> - >>> - drm_connector_helper_add(connector, &lvds_connector_helper_funcs); >>> - >>> - ret = drm_connector_init(bridge->dev, connector, &lvds_connector_funcs, >>> - DRM_MODE_CONNECTOR_LVDS); >>> - if (ret) { >>> - DRM_ERROR("Failed to initialize connector\n"); >>> - return ret; >>> - } >>> - >>> - drm_mode_connector_attach_encoder(&lvds->connector, bridge->encoder); >>> - >>> - ret = drm_panel_attach(lvds->panel, &lvds->connector); >>> - if (ret < 0) >>> - return ret; >>> - >>> - return 0; >>> -} >>> - >>> -static void lvds_encoder_detach(struct drm_bridge *bridge) >>> -{ >>> - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); >>> - >>> - drm_panel_detach(lvds->panel); >>> -} >>> - >>> -static void lvds_encoder_pre_enable(struct drm_bridge *bridge) >>> -{ >>> - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); >>> - >>> - drm_panel_prepare(lvds->panel); >>> -} >>> - >>> -static void lvds_encoder_enable(struct drm_bridge *bridge) >>> -{ >>> - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); >>> - >>> - drm_panel_enable(lvds->panel); >>> -} >>> - >>> -static void lvds_encoder_disable(struct drm_bridge *bridge) >>> -{ >>> - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); >>> - >>> - drm_panel_disable(lvds->panel); >>> -} >>> - >>> -static void lvds_encoder_post_disable(struct drm_bridge *bridge) >>> -{ >>> - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); >>> - >>> - drm_panel_unprepare(lvds->panel); >>> -} >>> - >>> -static const struct drm_bridge_funcs lvds_encoder_bridge_funcs = { >>> - .attach = lvds_encoder_attach, >>> - .detach = lvds_encoder_detach, >>> - .pre_enable = lvds_encoder_pre_enable, >>> - .enable = lvds_encoder_enable, >>> - .disable = lvds_encoder_disable, >>> - .post_disable = lvds_encoder_post_disable, >>> -}; >>> - >>> static int lvds_encoder_probe(struct platform_device *pdev) >>> { >>> - struct lvds_encoder *lvds; >>> struct device_node *port; >>> struct device_node *endpoint; >>> - struct device_node *panel; >>> - >>> - lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL); >>> - if (!lvds) >>> - return -ENOMEM; >>> - >>> - lvds->dev = &pdev->dev; >>> - platform_set_drvdata(pdev, lvds); >>> - >>> - lvds->bridge.funcs = &lvds_encoder_bridge_funcs; >>> - lvds->bridge.of_node = pdev->dev.of_node; >>> + struct device_node *panel_node; >>> + struct drm_panel *panel; >>> + struct drm_bridge *bridge; >>> >>> /* Locate the panel DT node. */ >>> port = of_graph_get_port_by_id(pdev->dev.of_node, 1); >>> @@ -161,29 +35,35 @@ static int lvds_encoder_probe(struct platform_device *pdev) >>> return -ENXIO; >>> } >>> >>> - panel = of_graph_get_remote_port_parent(endpoint); >>> + panel_node = of_graph_get_remote_port_parent(endpoint); >>> of_node_put(endpoint); >>> - if (!panel) { >>> + if (!panel_node) { >>> dev_dbg(&pdev->dev, "no remote endpoint for port 1\n"); >>> return -ENXIO; >>> } >>> >>> - lvds->panel = of_drm_find_panel(panel); >>> - of_node_put(panel); >>> - if (!lvds->panel) { >>> + panel = of_drm_find_panel(panel_node); >>> + of_node_put(panel_node); >>> + if (!panel) { >>> dev_dbg(&pdev->dev, "panel not found, deferring probe\n"); >>> return -EPROBE_DEFER; >>> } >>> >>> - /* Register the bridge. */ >>> - return drm_bridge_add(&lvds->bridge); >>> + bridge = drm_panel_bridge_add(&pdev->dev, panel, >>> + DRM_MODE_CONNECTOR_LVDS); >>> + if (IS_ERR(bridge)) >>> + return PTR_ERR(bridge); >>> + >>> + platform_set_drvdata(pdev, bridge); >>> + >>> + return 0; >>> } >>> >>> static int lvds_encoder_remove(struct platform_device *pdev) >>> { >>> - struct lvds_encoder *encoder = platform_get_drvdata(pdev); >>> + struct drm_bridge *bridge = platform_get_drvdata(pdev); >>> >>> - drm_bridge_remove(&encoder->bridge); >>> + drm_bridge_remove(bridge); >>> >>> return 0; >>> } >>> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c >>> new file mode 100644 >>> index 000000000000..2081245455c6 >>> --- /dev/null >>> +++ b/drivers/gpu/drm/bridge/panel.c >>> @@ -0,0 +1,188 @@ >>> +/* >>> + * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> + * Copyright (C) 2017 Broadcom >>> + * >>> + * This program is free software; you can redistribute it and/or >>> + * modify it under the terms of the GNU General Public License as >>> + * published by the Free Software Foundation; either version 2 of >>> + * the License, or (at your option) any later version. >>> + */ >>> + >>> +#include <drm/drmP.h> >>> +#include <drm/drm_panel.h> >>> +#include <drm/drm_atomic_helper.h> >>> +#include <drm/drm_connector.h> >>> +#include <drm/drm_crtc_helper.h> >>> +#include <drm/drm_encoder.h> >>> +#include <drm/drm_modeset_helper_vtables.h> >>> +#include <drm/drm_panel.h> >>> + >>> +struct panel_bridge { >>> + struct drm_bridge bridge; >>> + struct device *dev; >>> + struct drm_connector connector; >>> + struct drm_panel *panel; >>> + u32 connector_type; >>> +}; >>> + >>> +static inline struct panel_bridge * >>> +drm_bridge_to_panel_bridge(struct drm_bridge *bridge) >>> +{ >>> + return container_of(bridge, struct panel_bridge, bridge); >>> +} >>> + >>> +static inline struct panel_bridge * >>> +drm_connector_to_panel_bridge(struct drm_connector *connector) >>> +{ >>> + return container_of(connector, struct panel_bridge, connector); >>> +} >>> + >>> +static int panel_bridge_connector_get_modes(struct drm_connector *connector) >>> +{ >>> + struct panel_bridge *panel_bridge = >>> + drm_connector_to_panel_bridge(connector); >>> + >>> + return drm_panel_get_modes(panel_bridge->panel); >>> +} >>> + >>> +static const struct drm_connector_helper_funcs panel_bridge_connector_helper_funcs = { >>> + .get_modes = panel_bridge_connector_get_modes, >>> +}; >>> + >>> +static const struct drm_connector_funcs panel_bridge_connector_funcs = { >>> + .dpms = drm_atomic_helper_connector_dpms, >>> + .reset = drm_atomic_helper_connector_reset, >>> + .fill_modes = drm_helper_probe_single_connector_modes, >>> + .destroy = drm_connector_cleanup, >>> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, >>> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, >>> +}; >>> + >>> +static int panel_bridge_attach(struct drm_bridge *bridge) >>> +{ >>> + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); >>> + struct drm_connector *connector = &panel_bridge->connector; >>> + int ret; >>> + >>> + if (!bridge->encoder) { >>> + DRM_ERROR("Missing encoder\n"); >>> + return -ENODEV; >>> + } >>> + >>> + drm_connector_helper_add(connector, >>> + &panel_bridge_connector_helper_funcs); >>> + >>> + ret = drm_connector_init(bridge->dev, connector, >>> + &panel_bridge_connector_funcs, >>> + panel_bridge->connector_type); >>> + if (ret) { >>> + DRM_ERROR("Failed to initialize connector\n"); >>> + return ret; >>> + } >>> + >>> + drm_mode_connector_attach_encoder(&panel_bridge->connector, >>> + bridge->encoder); >>> + >>> + ret = drm_panel_attach(panel_bridge->panel, &panel_bridge->connector); >>> + if (ret < 0) >>> + return ret; >>> + >>> + return 0; >>> +} >>> + >>> +static void panel_bridge_detach(struct drm_bridge *bridge) >>> +{ >>> + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); >>> + >>> + drm_panel_detach(panel_bridge->panel); >>> +} >>> + >>> +static void panel_bridge_pre_enable(struct drm_bridge *bridge) >>> +{ >>> + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); >>> + >>> + drm_panel_prepare(panel_bridge->panel); >>> +} >>> + >>> +static void panel_bridge_enable(struct drm_bridge *bridge) >>> +{ >>> + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); >>> + >>> + drm_panel_enable(panel_bridge->panel); >>> +} >>> + >>> +static void panel_bridge_disable(struct drm_bridge *bridge) >>> +{ >>> + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); >>> + >>> + drm_panel_disable(panel_bridge->panel); >>> +} >>> + >>> +static void panel_bridge_post_disable(struct drm_bridge *bridge) >>> +{ >>> + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); >>> + >>> + drm_panel_unprepare(panel_bridge->panel); >>> +} >>> + >>> +static const struct drm_bridge_funcs panel_bridge_bridge_funcs = { >>> + .attach = panel_bridge_attach, >>> + .detach = panel_bridge_detach, >>> + .pre_enable = panel_bridge_pre_enable, >>> + .enable = panel_bridge_enable, >>> + .disable = panel_bridge_disable, >>> + .post_disable = panel_bridge_post_disable, >>> +}; >>> + >>> +/** >>> + * drm_panel_bridge_add - Creates a drm_bridge and drm_connector that >>> + * just call the appropriate functions from drm_panel. >>> + * >>> + * @dev: The struct device of the panel device. This is used for >>> + * registering the drm_bridge. >>> + * @panel: The drm_panel being wrapped. Must be non-NULL. >>> + * @connector_type: The DRM_MODE_CONNECTOR_* for the connector to be >>> + * created. >>> + * >>> + * For drivers converting from directly using drm_panel: The expected >>> + * usage pattern is that during either encoder module probe or DSI >>> + * host attach, a drm_panel will be looked up through >>> + * drm_of_find_panel_or_bridge(). drm_panel_bridge_add() is used to >>> + * wrap that panel in the new bridge, and the result can then be >>> + * passed to drm_bridge_attach(). The drm_panel_prepare() and related >>> + * functions can be dropped from the encoder driver (they're now >>> + * called by the KMS helpers before calling into the encoder), along >>> + * with connector creation. >> >> +panel/bridge reviewers. >> >> This does make things much cleaner, but it seems a bit strange to create >> a drm_bridge when there isn't really a HW bridge in the display chain (i.e, >> when the DSI encoder is directly connected to a DSI panel). >> >> There are kms drivers that use drm_panel, but don't have simple stub connectors >> that wrap around a drm_panel. They have more complicated connector ops, and >> may call drm_panel_prepare() and related functions a bit differently. We won't >> be able to use drm_panel_bridge for those drivers. > > Can you point to examples? > > The only thing on my mind that would be missing if drivers converted to > bridges only is having your encoder be able to filter the mode list for > timing limits. That seems like something you'd want any link in the > display chain to be able to do, so we may want to extend drm_bridge for > handling that. Yeah, that's one of the reasons, and probably the most important one since the majority of the kms drivers use the connector's mode_valid() to check their encoder/crtc related mode constraints. Some drivers subclass the connector atomic state, this would require their own versions of connector ops to duplicate, copy, destroy their atomic state, which would be hard to do if connector creation was moved outside of the kms driver. I see at least the tegra DSI driver do that. It's another question whether that stuff should belong to the connector state or elsewhere. The msm DSI driver has special connector ops that does some tricks to ensure a dual DSI link panel is configured properly. In this case, there are 2 connectors created, but only one drm_panel. We also need to configure some msm components to work in dual DSI mode, this is done as callbacks from the connector ops. That being said, I think we could move some of this from the kms driver to drm_panel_bridge() as a special case. About the usage of drm_panel_prepare() and other funcs, the majority of the users mostly do: encoder_enable_helper_func() { drm_panel_prepare(); /* do stuff to actually enable the encoder */ drm_panel_enable(); } The above usage can be replaced by creating a drm_panel_bridge(), but other types of usage would mess up the sequence a bit. For example, rockchip's dw-mipi-dsi seems to something like do: dw_mipi_dsi_encoder_enable() { /* some DSI encoder preparation, configure DSI in command mode */ drm_panel_prepare(); /* configure DSI in video mode */ drm_panel_enable(); /* some more DSI configuration */ } It's possible that we can adjust these non-complying cases without breaking functionality, but it would need some testing. > > Maybe you're thinking of something else, though. > >> For msm, we check whether the DSI encoder is connected directly to a panel >> or an external bridge. If it's connected to an external bridge, we skip the >> creation of the stub connector, and rely on the external bridge driver to >> create the connector: >> >> http://lxr.free-electrons.com/source/drivers/gpu/drm/msm/dsi/dsi.c#L227 >> >> The msm solution isn't very neat, but it avoids the need to create another >> bridge to glue things together. > > Yeah, that's basically the code I was going to have to replicate, which > is why I wrote a generic helper instead. I think allocating a > drm_bridge (which isn't visible to userspace) is a small price to pay > for cutting 100 lines of boilerplate from a driver. That sounds fair enough, I guess. Thanks, Archit
On Thu, May 04, 2017 at 07:44:08AM +0200, Daniel Vetter wrote: > On Wed, May 3, 2017 at 6:17 PM, Eric Anholt <eric@anholt.net> wrote: > > Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > > > >> Hi Daniel, > >> > >> On Wednesday 03 May 2017 16:28:56 Daniel Vetter wrote: > >>> On Wed, May 03, 2017 at 12:36:06PM +0300, Laurent Pinchart wrote: > >>> > On Wednesday 03 May 2017 11:32:17 Daniel Vetter wrote: > >>> >> On Wed, May 03, 2017 at 02:53:00PM +0530, Archit Taneja wrote: > >>> >>> +panel/bridge reviewers. > >>> >>> > >>> >>> This does make things much cleaner, but it seems a bit strange to > >>> >>> create a drm_bridge when there isn't really a HW bridge in the display > >>> >>> chain (i.e, when the DSI encoder is directly connected to a DSI panel). > >>> >>> > >>> >>> There are kms drivers that use drm_panel, but don't have simple stub > >>> >>> connectors that wrap around a drm_panel. They have more complicated > >>> >>> connector ops, and may call drm_panel_prepare() and related functions > >>> >>> a bit differently. We won't be able to use drm_panel_bridge for those > >>> >>> drivers. > >>> >>> > >>> >>> For msm, we check whether the DSI encoder is connected directly to a > >>> >>> panel or an external bridge. If it's connected to an external bridge, > >>> >>> we skip the creation of the stub connector, and rely on the external > >>> >>> bridge driver to create the connector: > >>> >>> > >>> >>> http://lxr.free-electrons.com/source/drivers/gpu/drm/msm/dsi/dsi.c#L22 > >>> >>> 7 > >>> >>> > >>> >>> The msm solution isn't very neat, but it avoids the need to create > >>> >>> another bridge to glue things together. > >>> >> > >>> >> Since I suggested this, yes I like it. And I think just unconditionally > >>> >> creating the panel bridge is probably even simpler, after all bridges > >>> >> are supposed to be chainable. I guess there's always going to be drivers > >>> >> where we need special handling, but I'm kinda hoping that for most cases > >>> >> simply plugging in a panel bridge is all that's need to glue drm_panel > >>> >> support into a driver. The simple pipe helpers do support bridges, and > >>> >> part of the goal there very much was to make it easy to glue in panel > >>> >> drivers. > >>> > > >>> > As I've just explained in another reply, I don't see the point in doing > >>> > this when we can instead refactor the bridge and panel operations to > >>> > expose a common base object that will then be easy to handle in core > >>> > code. This isn't just for panels, as connectors should have DT nodes, it > >>> > makes sense to instantiate an object for them that can be handled by the > >>> > DRM core, without having to push connector handling in all bridge > >>> > drivers. > >>> > >>> Imo you're aiming too high. We have 21 bridge drivers and 11 panel > >>> drivers. Asking someone to refactor them all (plus all the callers and > >>> everything) means it won't happen. At least I personally will definitely > >>> not block a contribution on this happening, that's a totally outsized > >>> demand. > >> > >> I think you're aiming too low. When the atomic update API was introduced I > >> could have told you that converting all drivers was an impossible task ;-) > > We still have non-atomic drivers. We will have non-atomic drivers for > a _very_ long time. Heck, we still have non-kms drivers in drm. And > especially with atomic we've started out with some helpers to glue the > new world into the old one, not by refactoring existing interfaces. > That's somewhat starting to happen now, 2 years and 20+ drivers later. > You've just proven my point I think :-) > > >> Jokes aside, I believe it might be possible to implement something simple. I'm > >> flexible about the naming, so instead of creating a new base structure and > >> refactor drm_bridge and drm_panel to embed it, we could as a first step use > >> drm_bridge as that base structure. We would only need to embed a drm_bridge > >> instance in drm_panel and add a few connector-related operations to the bridge > >> ops structure. As existing bridge drivers wouldn't need to provide those new > >> ops, they wouldn't need to be touched. > > > > I think drm_bridge itself should be the base structure, as it is today. > > It's got the entrypoints we want, connected in at the right level in the > > modeset chain. You could call it the "drm_display_chain_link" or > > something, but "bridge" is short and I don't think renaming is worth the > > trouble. This helper just makes it easy to make one of these display > > chain links for the endpoint of your display chain when it's a > > drm_panel. > > > > One of the followons I'd like to see after this is to make a helper that > > drivers get to use that does the "grab a bridge? no? grab a panel? > > Ok? wrap it in a bridge" pattern. Then the drm_panel/drm_connector are > > an implementation detail of whatever is at the end of the chain, and not > > something that the middle of the chain needs to know about. > > > > The alternative to that that I see would be to have panel drivers call > > this code to advertise themselves through the list of bridges. > > Yeah, I do believe that Eric's series here is actual the right (first, > among a lot more) steps into the direction of unifying panels and > bridges. Atm, if you want to have this shiny new unified world you get > to deal with n bridges, m panels and bunch of callers for each. With > Eric's work here we can get the n:m out of this refactoring problem by > making panels looks like bridges for everyone outside at least. And > then, once that part is done (we might, or might not need to clarify > something in the bridge interface), we can then push that bridge > interface down, or at least embed it into drm_panel and unify them. > But at that point it's only a 1:m issue (hopefully at least). I'm not sure I understand what the goal here is, or what you think we'd gain. Bridges and panels are fundamentally different things. One sits between an encoder and a connector, the other sits behind the connector. The only reason the interfaces have some resemblance is because they are fairly simple things that just need a two step initialization and two step deinitialization. Why would you want to unify them? I think that complicates things because you'd have to special case depending on whether the "bridge" is behind an encoder or behind a connector. That said, I think this helper makes sense for cases where the standard handling is used and where we otherwise would be using a tightly coupled encoder/connector pair. Thierry
On 04.05.2017 14:35, Thierry Reding wrote: > On Thu, May 04, 2017 at 07:44:08AM +0200, Daniel Vetter wrote: >> On Wed, May 3, 2017 at 6:17 PM, Eric Anholt <eric@anholt.net> wrote: >>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: >>> >>>> Hi Daniel, >>>> >>>> On Wednesday 03 May 2017 16:28:56 Daniel Vetter wrote: >>>>> On Wed, May 03, 2017 at 12:36:06PM +0300, Laurent Pinchart wrote: >>>>>> On Wednesday 03 May 2017 11:32:17 Daniel Vetter wrote: >>>>>>> On Wed, May 03, 2017 at 02:53:00PM +0530, Archit Taneja wrote: >>>>>>>> +panel/bridge reviewers. >>>>>>>> >>>>>>>> This does make things much cleaner, but it seems a bit strange to >>>>>>>> create a drm_bridge when there isn't really a HW bridge in the display >>>>>>>> chain (i.e, when the DSI encoder is directly connected to a DSI panel). >>>>>>>> >>>>>>>> There are kms drivers that use drm_panel, but don't have simple stub >>>>>>>> connectors that wrap around a drm_panel. They have more complicated >>>>>>>> connector ops, and may call drm_panel_prepare() and related functions >>>>>>>> a bit differently. We won't be able to use drm_panel_bridge for those >>>>>>>> drivers. >>>>>>>> >>>>>>>> For msm, we check whether the DSI encoder is connected directly to a >>>>>>>> panel or an external bridge. If it's connected to an external bridge, >>>>>>>> we skip the creation of the stub connector, and rely on the external >>>>>>>> bridge driver to create the connector: >>>>>>>> >>>>>>>> http://lxr.free-electrons.com/source/drivers/gpu/drm/msm/dsi/dsi.c#L22 >>>>>>>> 7 >>>>>>>> >>>>>>>> The msm solution isn't very neat, but it avoids the need to create >>>>>>>> another bridge to glue things together. >>>>>>> Since I suggested this, yes I like it. And I think just unconditionally >>>>>>> creating the panel bridge is probably even simpler, after all bridges >>>>>>> are supposed to be chainable. I guess there's always going to be drivers >>>>>>> where we need special handling, but I'm kinda hoping that for most cases >>>>>>> simply plugging in a panel bridge is all that's need to glue drm_panel >>>>>>> support into a driver. The simple pipe helpers do support bridges, and >>>>>>> part of the goal there very much was to make it easy to glue in panel >>>>>>> drivers. >>>>>> As I've just explained in another reply, I don't see the point in doing >>>>>> this when we can instead refactor the bridge and panel operations to >>>>>> expose a common base object that will then be easy to handle in core >>>>>> code. This isn't just for panels, as connectors should have DT nodes, it >>>>>> makes sense to instantiate an object for them that can be handled by the >>>>>> DRM core, without having to push connector handling in all bridge >>>>>> drivers. >>>>> Imo you're aiming too high. We have 21 bridge drivers and 11 panel >>>>> drivers. Asking someone to refactor them all (plus all the callers and >>>>> everything) means it won't happen. At least I personally will definitely >>>>> not block a contribution on this happening, that's a totally outsized >>>>> demand. >>>> I think you're aiming too low. When the atomic update API was introduced I >>>> could have told you that converting all drivers was an impossible task ;-) >> We still have non-atomic drivers. We will have non-atomic drivers for >> a _very_ long time. Heck, we still have non-kms drivers in drm. And >> especially with atomic we've started out with some helpers to glue the >> new world into the old one, not by refactoring existing interfaces. >> That's somewhat starting to happen now, 2 years and 20+ drivers later. >> You've just proven my point I think :-) >> >>>> Jokes aside, I believe it might be possible to implement something simple. I'm >>>> flexible about the naming, so instead of creating a new base structure and >>>> refactor drm_bridge and drm_panel to embed it, we could as a first step use >>>> drm_bridge as that base structure. We would only need to embed a drm_bridge >>>> instance in drm_panel and add a few connector-related operations to the bridge >>>> ops structure. As existing bridge drivers wouldn't need to provide those new >>>> ops, they wouldn't need to be touched. >>> I think drm_bridge itself should be the base structure, as it is today. >>> It's got the entrypoints we want, connected in at the right level in the >>> modeset chain. You could call it the "drm_display_chain_link" or >>> something, but "bridge" is short and I don't think renaming is worth the >>> trouble. This helper just makes it easy to make one of these display >>> chain links for the endpoint of your display chain when it's a >>> drm_panel. >>> >>> One of the followons I'd like to see after this is to make a helper that >>> drivers get to use that does the "grab a bridge? no? grab a panel? >>> Ok? wrap it in a bridge" pattern. Then the drm_panel/drm_connector are >>> an implementation detail of whatever is at the end of the chain, and not >>> something that the middle of the chain needs to know about. >>> >>> The alternative to that that I see would be to have panel drivers call >>> this code to advertise themselves through the list of bridges. >> Yeah, I do believe that Eric's series here is actual the right (first, >> among a lot more) steps into the direction of unifying panels and >> bridges. Atm, if you want to have this shiny new unified world you get >> to deal with n bridges, m panels and bunch of callers for each. With >> Eric's work here we can get the n:m out of this refactoring problem by >> making panels looks like bridges for everyone outside at least. And >> then, once that part is done (we might, or might not need to clarify >> something in the bridge interface), we can then push that bridge >> interface down, or at least embed it into drm_panel and unify them. >> But at that point it's only a 1:m issue (hopefully at least). > I'm not sure I understand what the goal here is, or what you think we'd > gain. Bridges and panels are fundamentally different things. One sits > between an encoder and a connector, the other sits behind the connector. > The only reason the interfaces have some resemblance is because they are > fairly simple things that just need a two step initialization and two > step deinitialization. They provide the same function - video sink, the difference is that bridge provides also the function of video source. But since pipeline in drm is build and usually used in source->sink direction role of sink in bridges is more exposed, so the resemblance of bridges and panels is much higher. And regarding connector, in case of panel pipelines there is no physical connector, but as framework requires it must be put somewhere. It is just convention (quite painful) to implement it in the last bridge, it could be implemented in the panel as well, or even in DRM core. Regards Andrzej
diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst index c075aadd7078..60eb3b41702b 100644 --- a/Documentation/gpu/drm-kms-helpers.rst +++ b/Documentation/gpu/drm-kms-helpers.rst @@ -143,6 +143,9 @@ Bridge Helper Reference .. kernel-doc:: drivers/gpu/drm/drm_bridge.c :export: +.. kernel-doc:: drivers/gpu/drm/bridge/panel.c + :export: + .. _drm_panel_helper: Panel Helper Reference diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index f6968d3b4b41..c4daca38743c 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -4,6 +4,14 @@ config DRM_BRIDGE help Bridge registration and lookup framework. +config DRM_PANEL_BRIDGE + def_bool y + depends on DRM_BRIDGE + select DRM_KMS_HELPER + select DRM_PANEL + help + DRM bridge wrapper of DRM panels + menu "Display Interface Bridges" depends on DRM && DRM_BRIDGE @@ -27,8 +35,7 @@ config DRM_DUMB_VGA_DAC config DRM_LVDS_ENCODER tristate "Transparent parallel to LVDS encoder support" depends on OF - select DRM_KMS_HELPER - select DRM_PANEL + select DRM_PANEL_BRIDGE help Support for transparent parallel to LVDS encoders that don't require any configuration. diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile index 3fe2226ee2f2..40a43750ad55 100644 --- a/drivers/gpu/drm/bridge/Makefile +++ b/drivers/gpu/drm/bridge/Makefile @@ -5,6 +5,7 @@ obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o +obj-$(CONFIG_DRM_PANEL_BRIDGE) += panel.o obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o obj-$(CONFIG_DRM_SII902X) += sii902x.o diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c index f1f67a279426..04e1504c4d8f 100644 --- a/drivers/gpu/drm/bridge/lvds-encoder.c +++ b/drivers/gpu/drm/bridge/lvds-encoder.c @@ -8,144 +8,18 @@ */ #include <drm/drmP.h> -#include <drm/drm_atomic_helper.h> -#include <drm/drm_connector.h> -#include <drm/drm_crtc_helper.h> -#include <drm/drm_encoder.h> -#include <drm/drm_modeset_helper_vtables.h> +#include <drm/drm_bridge.h> #include <drm/drm_panel.h> #include <linux/of_graph.h> -struct lvds_encoder { - struct device *dev; - - struct drm_bridge bridge; - struct drm_connector connector; - struct drm_panel *panel; -}; - -static inline struct lvds_encoder * -drm_bridge_to_lvds_encoder(struct drm_bridge *bridge) -{ - return container_of(bridge, struct lvds_encoder, bridge); -} - -static inline struct lvds_encoder * -drm_connector_to_lvds_encoder(struct drm_connector *connector) -{ - return container_of(connector, struct lvds_encoder, connector); -} - -static int lvds_connector_get_modes(struct drm_connector *connector) -{ - struct lvds_encoder *lvds = drm_connector_to_lvds_encoder(connector); - - return drm_panel_get_modes(lvds->panel); -} - -static const struct drm_connector_helper_funcs lvds_connector_helper_funcs = { - .get_modes = lvds_connector_get_modes, -}; - -static const struct drm_connector_funcs lvds_connector_funcs = { - .dpms = drm_atomic_helper_connector_dpms, - .reset = drm_atomic_helper_connector_reset, - .fill_modes = drm_helper_probe_single_connector_modes, - .destroy = drm_connector_cleanup, - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, -}; - -static int lvds_encoder_attach(struct drm_bridge *bridge) -{ - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); - struct drm_connector *connector = &lvds->connector; - int ret; - - if (!bridge->encoder) { - DRM_ERROR("Missing encoder\n"); - return -ENODEV; - } - - drm_connector_helper_add(connector, &lvds_connector_helper_funcs); - - ret = drm_connector_init(bridge->dev, connector, &lvds_connector_funcs, - DRM_MODE_CONNECTOR_LVDS); - if (ret) { - DRM_ERROR("Failed to initialize connector\n"); - return ret; - } - - drm_mode_connector_attach_encoder(&lvds->connector, bridge->encoder); - - ret = drm_panel_attach(lvds->panel, &lvds->connector); - if (ret < 0) - return ret; - - return 0; -} - -static void lvds_encoder_detach(struct drm_bridge *bridge) -{ - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); - - drm_panel_detach(lvds->panel); -} - -static void lvds_encoder_pre_enable(struct drm_bridge *bridge) -{ - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); - - drm_panel_prepare(lvds->panel); -} - -static void lvds_encoder_enable(struct drm_bridge *bridge) -{ - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); - - drm_panel_enable(lvds->panel); -} - -static void lvds_encoder_disable(struct drm_bridge *bridge) -{ - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); - - drm_panel_disable(lvds->panel); -} - -static void lvds_encoder_post_disable(struct drm_bridge *bridge) -{ - struct lvds_encoder *lvds = drm_bridge_to_lvds_encoder(bridge); - - drm_panel_unprepare(lvds->panel); -} - -static const struct drm_bridge_funcs lvds_encoder_bridge_funcs = { - .attach = lvds_encoder_attach, - .detach = lvds_encoder_detach, - .pre_enable = lvds_encoder_pre_enable, - .enable = lvds_encoder_enable, - .disable = lvds_encoder_disable, - .post_disable = lvds_encoder_post_disable, -}; - static int lvds_encoder_probe(struct platform_device *pdev) { - struct lvds_encoder *lvds; struct device_node *port; struct device_node *endpoint; - struct device_node *panel; - - lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL); - if (!lvds) - return -ENOMEM; - - lvds->dev = &pdev->dev; - platform_set_drvdata(pdev, lvds); - - lvds->bridge.funcs = &lvds_encoder_bridge_funcs; - lvds->bridge.of_node = pdev->dev.of_node; + struct device_node *panel_node; + struct drm_panel *panel; + struct drm_bridge *bridge; /* Locate the panel DT node. */ port = of_graph_get_port_by_id(pdev->dev.of_node, 1); @@ -161,29 +35,35 @@ static int lvds_encoder_probe(struct platform_device *pdev) return -ENXIO; } - panel = of_graph_get_remote_port_parent(endpoint); + panel_node = of_graph_get_remote_port_parent(endpoint); of_node_put(endpoint); - if (!panel) { + if (!panel_node) { dev_dbg(&pdev->dev, "no remote endpoint for port 1\n"); return -ENXIO; } - lvds->panel = of_drm_find_panel(panel); - of_node_put(panel); - if (!lvds->panel) { + panel = of_drm_find_panel(panel_node); + of_node_put(panel_node); + if (!panel) { dev_dbg(&pdev->dev, "panel not found, deferring probe\n"); return -EPROBE_DEFER; } - /* Register the bridge. */ - return drm_bridge_add(&lvds->bridge); + bridge = drm_panel_bridge_add(&pdev->dev, panel, + DRM_MODE_CONNECTOR_LVDS); + if (IS_ERR(bridge)) + return PTR_ERR(bridge); + + platform_set_drvdata(pdev, bridge); + + return 0; } static int lvds_encoder_remove(struct platform_device *pdev) { - struct lvds_encoder *encoder = platform_get_drvdata(pdev); + struct drm_bridge *bridge = platform_get_drvdata(pdev); - drm_bridge_remove(&encoder->bridge); + drm_bridge_remove(bridge); return 0; } diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c new file mode 100644 index 000000000000..2081245455c6 --- /dev/null +++ b/drivers/gpu/drm/bridge/panel.c @@ -0,0 +1,188 @@ +/* + * Copyright (C) 2016 Laurent Pinchart <laurent.pinchart@ideasonboard.com> + * Copyright (C) 2017 Broadcom + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + */ + +#include <drm/drmP.h> +#include <drm/drm_panel.h> +#include <drm/drm_atomic_helper.h> +#include <drm/drm_connector.h> +#include <drm/drm_crtc_helper.h> +#include <drm/drm_encoder.h> +#include <drm/drm_modeset_helper_vtables.h> +#include <drm/drm_panel.h> + +struct panel_bridge { + struct drm_bridge bridge; + struct device *dev; + struct drm_connector connector; + struct drm_panel *panel; + u32 connector_type; +}; + +static inline struct panel_bridge * +drm_bridge_to_panel_bridge(struct drm_bridge *bridge) +{ + return container_of(bridge, struct panel_bridge, bridge); +} + +static inline struct panel_bridge * +drm_connector_to_panel_bridge(struct drm_connector *connector) +{ + return container_of(connector, struct panel_bridge, connector); +} + +static int panel_bridge_connector_get_modes(struct drm_connector *connector) +{ + struct panel_bridge *panel_bridge = + drm_connector_to_panel_bridge(connector); + + return drm_panel_get_modes(panel_bridge->panel); +} + +static const struct drm_connector_helper_funcs panel_bridge_connector_helper_funcs = { + .get_modes = panel_bridge_connector_get_modes, +}; + +static const struct drm_connector_funcs panel_bridge_connector_funcs = { + .dpms = drm_atomic_helper_connector_dpms, + .reset = drm_atomic_helper_connector_reset, + .fill_modes = drm_helper_probe_single_connector_modes, + .destroy = drm_connector_cleanup, + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, +}; + +static int panel_bridge_attach(struct drm_bridge *bridge) +{ + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); + struct drm_connector *connector = &panel_bridge->connector; + int ret; + + if (!bridge->encoder) { + DRM_ERROR("Missing encoder\n"); + return -ENODEV; + } + + drm_connector_helper_add(connector, + &panel_bridge_connector_helper_funcs); + + ret = drm_connector_init(bridge->dev, connector, + &panel_bridge_connector_funcs, + panel_bridge->connector_type); + if (ret) { + DRM_ERROR("Failed to initialize connector\n"); + return ret; + } + + drm_mode_connector_attach_encoder(&panel_bridge->connector, + bridge->encoder); + + ret = drm_panel_attach(panel_bridge->panel, &panel_bridge->connector); + if (ret < 0) + return ret; + + return 0; +} + +static void panel_bridge_detach(struct drm_bridge *bridge) +{ + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); + + drm_panel_detach(panel_bridge->panel); +} + +static void panel_bridge_pre_enable(struct drm_bridge *bridge) +{ + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); + + drm_panel_prepare(panel_bridge->panel); +} + +static void panel_bridge_enable(struct drm_bridge *bridge) +{ + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); + + drm_panel_enable(panel_bridge->panel); +} + +static void panel_bridge_disable(struct drm_bridge *bridge) +{ + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); + + drm_panel_disable(panel_bridge->panel); +} + +static void panel_bridge_post_disable(struct drm_bridge *bridge) +{ + struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); + + drm_panel_unprepare(panel_bridge->panel); +} + +static const struct drm_bridge_funcs panel_bridge_bridge_funcs = { + .attach = panel_bridge_attach, + .detach = panel_bridge_detach, + .pre_enable = panel_bridge_pre_enable, + .enable = panel_bridge_enable, + .disable = panel_bridge_disable, + .post_disable = panel_bridge_post_disable, +}; + +/** + * drm_panel_bridge_add - Creates a drm_bridge and drm_connector that + * just call the appropriate functions from drm_panel. + * + * @dev: The struct device of the panel device. This is used for + * registering the drm_bridge. + * @panel: The drm_panel being wrapped. Must be non-NULL. + * @connector_type: The DRM_MODE_CONNECTOR_* for the connector to be + * created. + * + * For drivers converting from directly using drm_panel: The expected + * usage pattern is that during either encoder module probe or DSI + * host attach, a drm_panel will be looked up through + * drm_of_find_panel_or_bridge(). drm_panel_bridge_add() is used to + * wrap that panel in the new bridge, and the result can then be + * passed to drm_bridge_attach(). The drm_panel_prepare() and related + * functions can be dropped from the encoder driver (they're now + * called by the KMS helpers before calling into the encoder), along + * with connector creation. + */ +struct drm_bridge *drm_panel_bridge_add(struct device *dev, + struct drm_panel *panel, + u32 connector_type) +{ + struct panel_bridge *panel_bridge = + devm_kzalloc(dev, sizeof(*panel_bridge), GFP_KERNEL); + int ret; + + if (!dev || !panel) + return ERR_PTR(EINVAL); + + panel_bridge->dev = dev; + panel_bridge->connector_type = connector_type; + panel_bridge->panel = panel; + + panel_bridge->bridge.funcs = &panel_bridge_bridge_funcs; + panel_bridge->bridge.of_node = dev->of_node; + + ret = drm_bridge_add(&panel_bridge->bridge); + if (ret) + return ERR_PTR(ret); + + return &panel_bridge->bridge; +} +EXPORT_SYMBOL(drm_panel_bridge_add); + +void drm_panel_bridge_remove(struct device *dev, struct drm_bridge *bridge) +{ + drm_bridge_remove(bridge); + devm_kfree(dev, bridge); +} +EXPORT_SYMBOL(drm_panel_bridge_remove); diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index fdd82fcbf168..e08563ee0546 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -29,6 +29,7 @@ #include <drm/drm_modes.h> struct drm_bridge; +struct drm_panel; /** * struct drm_bridge_funcs - drm_bridge control functions @@ -221,4 +222,11 @@ void drm_bridge_mode_set(struct drm_bridge *bridge, void drm_bridge_pre_enable(struct drm_bridge *bridge); void drm_bridge_enable(struct drm_bridge *bridge); +#ifdef CONFIG_DRM_PANEL +struct drm_bridge *drm_panel_bridge_add(struct device *dev, + struct drm_panel *panel, + u32 connector_type); +void drm_panel_bridge_remove(struct device *dev, struct drm_bridge *bridge); +#endif + #endif
Many DRM drivers have common code to make a stub connector implementation that wraps a drm_panel. By wrapping the panel in a DRM bridge, all of the connector code (including calls during encoder enable/disable) goes away. Signed-off-by: Eric Anholt <eric@anholt.net> --- Documentation/gpu/drm-kms-helpers.rst | 3 + drivers/gpu/drm/bridge/Kconfig | 11 +- drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/lvds-encoder.c | 158 ++++------------------------ drivers/gpu/drm/bridge/panel.c | 188 ++++++++++++++++++++++++++++++++++ include/drm/drm_bridge.h | 8 ++ 6 files changed, 228 insertions(+), 141 deletions(-) create mode 100644 drivers/gpu/drm/bridge/panel.c