This series convert Allwinner DSI controller to full functional drm bridge driver for supporting slave panel, bridges.
Here, are the previous version changes[1].
The key concern about these changes is about kms hotplug which indeed not compatible with bridge conversion. However, I did try several ways to support hotplug with the bridge but it's resulting in a deadlock where bind never attach bridge until bridge pointer found and bridge pointer cannot found until bind finishes. Any inputs on this would be appreciated.
[1] https://lwn.net/Articles/783127/
Any inputs? Jagan.
Jagan Teki (7): drm: sun4i: dsi: Use drm_of_find_panel_or_bridge drm: sun4i: dsi: Add bridge support drm: sun4i: dsi: Convert to bridge driver drm: sun4i: dsi: Separate code for bridge pre_enable drm: bridge: Queue the bridge chain instead of stacking drm: sun4i: dsi: Use drm_panel_bridge, connector API [DO NOT MERGE] ARM: dts: sun8i: bananapi-m2m: Enable S070WV20-CT16 panel
arch/arm/boot/dts/sun8i-r16-bananapi-m2m.dts | 85 ++++++++++++ drivers/gpu/drm/drm_bridge.c | 4 +- drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 128 +++++++++---------- drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 11 +- 4 files changed, 150 insertions(+), 78 deletions(-)
Replace of_drm_find_panel with drm_of_find_panel_or_bridge for finding panel, this indeed help to find the bridge if bridge support added.
Added NULL in bridge argument, same will replace with bridge parameter once bridge supported.
Signed-off-by: Jagan Teki jagan@amarulasolutions.com --- Changes for v3: - none
drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index 4f5efcace68e..2e9e7b2d4145 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c @@ -21,6 +21,7 @@
#include <drm/drm_atomic_helper.h> #include <drm/drm_mipi_dsi.h> +#include <drm/drm_of.h> #include <drm/drm_panel.h> #include <drm/drm_print.h> #include <drm/drm_probe_helper.h> @@ -963,10 +964,14 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host, struct mipi_dsi_device *device) { struct sun6i_dsi *dsi = host_to_sun6i_dsi(host); - struct drm_panel *panel = of_drm_find_panel(device->dev.of_node); + struct drm_panel *panel; + int ret; + + ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 0, 0, + &panel, NULL); + if (ret) + return ret;
- if (IS_ERR(panel)) - return PTR_ERR(panel); if (!dsi->drm || !dsi->drm->registered) return -EPROBE_DEFER;
Some display panels would come up with a non-DSI output which can have an option to connect DSI interface by means of bridge converter.
This DSI to non-DSI bridge converter would require a bridge driver that would communicate the DSI controller for bridge functionalities.
So, add support for bridge functionalities in Allwinner DSI controller.
Cc: Samuel Holland samuel@sholland.org Signed-off-by: Jagan Teki jagan@amarulasolutions.com --- Note: Samuel Holland, The existing kms hotplug dropped in order to attach the bridge properly.
However, I did try several ways to support hotplug with the bridge but it's resulting in a deadlock where bind never attach bridge until bridge pointer found and bridge pointer cannot found until bind finishes. Any inputs on this would be appreciated.
Changes for v3: - updated with new API's
drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 34 +++++++++++++++++--------- drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 2 +- 2 files changed, 23 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index 2e9e7b2d4145..39321299dc27 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c @@ -773,6 +773,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder) if (dsi->panel) drm_panel_prepare(dsi->panel);
+ if (dsi->panel_bridge) + dsi->panel_bridge->funcs->pre_enable(dsi->panel_bridge); + /* * FIXME: This should be moved after the switch to HS mode. * @@ -788,6 +791,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder) if (dsi->panel) drm_panel_enable(dsi->panel);
+ if (dsi->panel_bridge) + dsi->panel_bridge->funcs->enable(dsi->panel_bridge); + sun6i_dsi_start(dsi, DSI_START_HSC);
udelay(1000); @@ -804,6 +810,9 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder) if (dsi->panel) { drm_panel_disable(dsi->panel); drm_panel_unprepare(dsi->panel); + } else if (dsi->panel_bridge) { + dsi->panel_bridge->funcs->disable(dsi->panel_bridge); + dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); }
phy_power_off(dsi->dphy); @@ -964,23 +973,17 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host, struct mipi_dsi_device *device) { struct sun6i_dsi *dsi = host_to_sun6i_dsi(host); - struct drm_panel *panel; int ret;
ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 0, 0, - &panel, NULL); + &dsi->panel, &dsi->panel_bridge); if (ret) return ret;
- if (!dsi->drm || !dsi->drm->registered) - return -EPROBE_DEFER; - - dsi->panel = panel; dsi->device = device;
- drm_kms_helper_hotplug_event(dsi->drm); - - dev_info(host->dev, "Attached device %s\n", device->name); + dev_info(host->dev, "Attached %s %s\n", + device->name, dsi->panel ? "panel" : "bridge");
return 0; } @@ -991,9 +994,10 @@ static int sun6i_dsi_detach(struct mipi_dsi_host *host, struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
dsi->panel = NULL; + dsi->panel_bridge = NULL; dsi->device = NULL;
- drm_kms_helper_hotplug_event(dsi->drm); + drm_of_panel_bridge_remove(dsi->dev->of_node, 0, 0);
return 0; } @@ -1082,7 +1086,13 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,
drm_connector_attach_encoder(&dsi->connector, &dsi->encoder);
- dsi->drm = drm; + if (dsi->panel_bridge) { + ret = drm_bridge_attach(&dsi->encoder, dsi->panel_bridge, NULL, 0); + if (ret) { + dev_err(dsi->dev, "Couldn't attach drm bridge\n"); + goto err_cleanup_connector; + } + }
return 0;
@@ -1096,7 +1106,7 @@ static void sun6i_dsi_unbind(struct device *dev, struct device *master, { struct sun6i_dsi *dsi = dev_get_drvdata(dev);
- dsi->drm = NULL; + drm_encoder_cleanup(&dsi->encoder); }
static const struct component_ops sun6i_dsi_ops = { diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h index c863900ae3b4..370ecb356a63 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h @@ -29,8 +29,8 @@ struct sun6i_dsi {
struct device *dev; struct mipi_dsi_device *device; - struct drm_device *drm; struct drm_panel *panel; + struct drm_bridge *panel_bridge; };
static inline struct sun6i_dsi *host_to_sun6i_dsi(struct mipi_dsi_host *host)
DRM bridge drivers have build-in handling of treating all display pipeline components as bridges.
So, convert the existing to a drm bridge driver with a built-in encoder support for compatibility with existing component drivers.
Signed-off-by: Jagan Teki jagan@amarulasolutions.com --- Changes for v3: - new patch
drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 75 ++++++++++++++++---------- drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 6 +++ 2 files changed, 54 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index 39321299dc27..6f3c5330a468 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c @@ -714,10 +714,10 @@ static int sun6i_dsi_start(struct sun6i_dsi *dsi, return 0; }
-static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder) +static void sun6i_dsi_bridge_enable(struct drm_bridge *bridge) { - struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode; - struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder); + struct drm_display_mode *mode = &bridge->encoder->crtc->state->adjusted_mode; + struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge); struct mipi_dsi_device *device = dsi->device; union phy_configure_opts opts = { }; struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy; @@ -801,9 +801,9 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder *encoder) sun6i_dsi_start(dsi, DSI_START_HSD); }
-static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder) +static void sun6i_dsi_bridge_disable(struct drm_bridge *bridge) { - struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder); + struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
DRM_DEBUG_DRIVER("Disabling DSI output\n");
@@ -852,9 +852,40 @@ static const struct drm_connector_funcs sun6i_dsi_connector_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };
-static const struct drm_encoder_helper_funcs sun6i_dsi_enc_helper_funcs = { - .disable = sun6i_dsi_encoder_disable, - .enable = sun6i_dsi_encoder_enable, +static int sun6i_dsi_bridge_attach(struct drm_bridge *bridge, + enum drm_bridge_attach_flags flags) +{ + struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge); + int ret; + + if (dsi->panel_bridge) + return drm_bridge_attach(bridge->encoder, dsi->panel_bridge, NULL, 0); + + if (dsi->panel) { + drm_connector_helper_add(&dsi->connector, + &sun6i_dsi_connector_helper_funcs); + ret = drm_connector_init(bridge->dev, &dsi->connector, + &sun6i_dsi_connector_funcs, + DRM_MODE_CONNECTOR_DSI); + if (ret) { + dev_err(dsi->dev, "Couldn't initialise the DSI connector\n"); + goto err_cleanup_connector; + } + + drm_connector_attach_encoder(&dsi->connector, &dsi->encoder); + } + + return 0; + +err_cleanup_connector: + drm_encoder_cleanup(&dsi->encoder); + return ret; +} + +static const struct drm_bridge_funcs sun6i_dsi_bridge_funcs = { + .enable = sun6i_dsi_bridge_enable, + .disable = sun6i_dsi_bridge_disable, + .attach = sun6i_dsi_bridge_attach, };
static u32 sun6i_dsi_dcs_build_pkt_hdr(struct sun6i_dsi *dsi, @@ -1063,8 +1094,6 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master, struct sun6i_dsi *dsi = dev_get_drvdata(dev); int ret;
- drm_encoder_helper_add(&dsi->encoder, - &sun6i_dsi_enc_helper_funcs); ret = drm_simple_encoder_init(drm, &dsi->encoder, DRM_MODE_ENCODER_DSI); if (ret) { @@ -1073,27 +1102,12 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master, } dsi->encoder.possible_crtcs = BIT(0);
- drm_connector_helper_add(&dsi->connector, - &sun6i_dsi_connector_helper_funcs); - ret = drm_connector_init(drm, &dsi->connector, - &sun6i_dsi_connector_funcs, - DRM_MODE_CONNECTOR_DSI); + ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, 0); if (ret) { - dev_err(dsi->dev, - "Couldn't initialise the DSI connector\n"); + dev_err(dsi->dev, "Couldn't attach drm bridge\n"); goto err_cleanup_connector; }
- drm_connector_attach_encoder(&dsi->connector, &dsi->encoder); - - if (dsi->panel_bridge) { - ret = drm_bridge_attach(&dsi->encoder, dsi->panel_bridge, NULL, 0); - if (ret) { - dev_err(dsi->dev, "Couldn't attach drm bridge\n"); - goto err_cleanup_connector; - } - } - return 0;
err_cleanup_connector: @@ -1199,6 +1213,12 @@ static int sun6i_dsi_probe(struct platform_device *pdev) goto err_unprotect_clk; }
+ dsi->bridge.funcs = &sun6i_dsi_bridge_funcs; + dsi->bridge.of_node = dev->of_node; + dsi->bridge.type = DRM_MODE_CONNECTOR_DSI; + + drm_bridge_add(&dsi->bridge); + ret = component_add(&pdev->dev, &sun6i_dsi_ops); if (ret) { dev_err(dev, "Couldn't register our component\n"); @@ -1222,6 +1242,7 @@ static int sun6i_dsi_remove(struct platform_device *pdev) struct device *dev = &pdev->dev; struct sun6i_dsi *dsi = dev_get_drvdata(dev);
+ drm_bridge_remove(&dsi->bridge); component_del(&pdev->dev, &sun6i_dsi_ops); mipi_dsi_host_unregister(&dsi->host); clk_rate_exclusive_put(dsi->mod_clk); diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h index 370ecb356a63..5e70666089ad 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h @@ -16,6 +16,7 @@ #define SUN6I_DSI_TCON_DIV 4
struct sun6i_dsi { + struct drm_bridge bridge; struct drm_connector connector; struct drm_encoder encoder; struct mipi_dsi_host host; @@ -38,6 +39,11 @@ static inline struct sun6i_dsi *host_to_sun6i_dsi(struct mipi_dsi_host *host) return container_of(host, struct sun6i_dsi, host); };
+static inline struct sun6i_dsi *bridge_to_sun6i_dsi(struct drm_bridge *bridge) +{ + return container_of(bridge, struct sun6i_dsi, bridge); +} + static inline struct sun6i_dsi *connector_to_sun6i_dsi(struct drm_connector *connector) { return container_of(connector, struct sun6i_dsi, connector);
The existing driver has an enablement code for initializing clock, reset, PHY, DSI timings, and finally switching to HS mode.
Move the clock, reset. PHY and DSI timings code into bridge pre_enable and keep HS mode switch in enable.
As the driver supports fully enabled bridge functionalities, this new enablement code separation will help to initialize the host and slave bridge pre_enable, enable functions properly.
Signed-off-by: Jagan Teki jagan@amarulasolutions.com --- Changes for v3: - new patch
drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index 6f3c5330a468..3cdc14daf25c 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c @@ -714,7 +714,7 @@ static int sun6i_dsi_start(struct sun6i_dsi *dsi, return 0; }
-static void sun6i_dsi_bridge_enable(struct drm_bridge *bridge) +static void sun6i_dsi_bridge_pre_enable(struct drm_bridge *bridge) { struct drm_display_mode *mode = &bridge->encoder->crtc->state->adjusted_mode; struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge); @@ -775,6 +775,11 @@ static void sun6i_dsi_bridge_enable(struct drm_bridge *bridge)
if (dsi->panel_bridge) dsi->panel_bridge->funcs->pre_enable(dsi->panel_bridge); +} + +static void sun6i_dsi_bridge_enable(struct drm_bridge *bridge) +{ + struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
/* * FIXME: This should be moved after the switch to HS mode. @@ -883,6 +888,7 @@ static int sun6i_dsi_bridge_attach(struct drm_bridge *bridge, }
static const struct drm_bridge_funcs sun6i_dsi_bridge_funcs = { + .pre_enable = sun6i_dsi_bridge_pre_enable, .enable = sun6i_dsi_bridge_enable, .disable = sun6i_dsi_bridge_disable, .attach = sun6i_dsi_bridge_attach,
drm_bridge_attach has stacked the bridge chain, so the bridge that gets pushed last can trigger its bridge function pre_enable first from drm_atomic_bridge_chain_pre_enable.
This indeed gives a chance to trigger slave bridge pre_enable first without triggering its host bridge pre_enable for the usual host to slave device model like DSI host with panel slave.
For fully enabled bridge drivers, host bridge pre_enable has all host related clock, reset, PHY configuration code that needs to initialized before sending commands or configuration from a slave to communicate its host.
Queue the bridge chain instead of stacking it so-that the bridges that got enqueued first can have a chance to trigger first.
Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Thomas Zimmermann tzimmermann@suse.de Signed-off-by: Jagan Teki jagan@amarulasolutions.com --- Changes for v3: - new patch
drivers/gpu/drm/drm_bridge.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 64f0effb52ac..e75d1a080c55 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -191,9 +191,9 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, bridge->encoder = encoder;
if (previous) - list_add(&bridge->chain_node, &previous->chain_node); + list_add_tail(&bridge->chain_node, &previous->chain_node); else - list_add(&bridge->chain_node, &encoder->bridge_chain); + list_add_tail(&bridge->chain_node, &encoder->bridge_chain);
if (bridge->funcs->attach) { ret = bridge->funcs->attach(bridge, flags);
Hi Jagan,
Thank you for the patch.
On Mon, Feb 15, 2021 at 01:11:00AM +0530, Jagan Teki wrote:
drm_bridge_attach has stacked the bridge chain, so the bridge that gets pushed last can trigger its bridge function pre_enable first from drm_atomic_bridge_chain_pre_enable.
This indeed gives a chance to trigger slave bridge pre_enable first without triggering its host bridge pre_enable for the usual host to slave device model like DSI host with panel slave.
For fully enabled bridge drivers, host bridge pre_enable has all host related clock, reset, PHY configuration code that needs to initialized before sending commands or configuration from a slave to communicate its host.
Queue the bridge chain instead of stacking it so-that the bridges that got enqueued first can have a chance to trigger first.
First of all, won't thus break all the drivers that currently rely on the existing behaviour ?
Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Thomas Zimmermann tzimmermann@suse.de Signed-off-by: Jagan Teki jagan@amarulasolutions.com
Changes for v3:
- new patch
drivers/gpu/drm/drm_bridge.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 64f0effb52ac..e75d1a080c55 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -191,9 +191,9 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, bridge->encoder = encoder;
if (previous)
list_add(&bridge->chain_node, &previous->chain_node);
list_add_tail(&bridge->chain_node, &previous->chain_node);
list_add(&bridge->chain_node, &encoder->bridge_chain);
list_add_tail(&bridge->chain_node, &encoder->bridge_chain);
Then, this will create a really weird order, as the list will contain bridges in the reverse order. Assuming three bridges, A, B and C, which are connected at the hardware level as follows:
Encoder -> A -> B -> C
the list would contain
Encoder -> C -> B -> A
This isn't intuitive, and if you want to reverse the order in which bridge operations are called, it would be better to do so in the operations themselves, for instance replacing list_for_each_entry_reverse() with list_for_each_entry() in drm_atomic_bridge_chain_pre_enable(). Still, this will likely break drivers that depend on the existing order, so I don't think that's an acceptable solution as-is.
if (bridge->funcs->attach) { ret = bridge->funcs->attach(bridge, flags);
Use drm_panel_bridge to replace manual panel handling code.
This simplifies the driver to allows all components in the display pipeline to be treated as bridges, paving the way to generic connector handling.
Use drm_bridge_connector_init to create a connector for display pipelines that use drm_bridge.
This allows splitting connector operations across multiple bridges when necessary, instead of having the last bridge in the chain creating the connector and handling all connector operations internally.
Signed-off-by: Jagan Teki jagan@amarulasolutions.com --- Changes for v3: - new patch
drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 108 +++++++------------------ drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 7 -- 2 files changed, 27 insertions(+), 88 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index 3cdc14daf25c..5e5d3789b3df 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c @@ -20,6 +20,7 @@ #include <linux/slab.h>
#include <drm/drm_atomic_helper.h> +#include <drm/drm_bridge_connector.h> #include <drm/drm_mipi_dsi.h> #include <drm/drm_of.h> #include <drm/drm_panel.h> @@ -769,12 +770,6 @@ static void sun6i_dsi_bridge_pre_enable(struct drm_bridge *bridge) phy_set_mode(dsi->dphy, PHY_MODE_MIPI_DPHY); phy_configure(dsi->dphy, &opts); phy_power_on(dsi->dphy); - - if (dsi->panel) - drm_panel_prepare(dsi->panel); - - if (dsi->panel_bridge) - dsi->panel_bridge->funcs->pre_enable(dsi->panel_bridge); }
static void sun6i_dsi_bridge_enable(struct drm_bridge *bridge) @@ -793,12 +788,6 @@ static void sun6i_dsi_bridge_enable(struct drm_bridge *bridge) * ordering on the panels I've tested it with, so I guess this * will do for now, until that IP is better understood. */ - if (dsi->panel) - drm_panel_enable(dsi->panel); - - if (dsi->panel_bridge) - dsi->panel_bridge->funcs->enable(dsi->panel_bridge); - sun6i_dsi_start(dsi, DSI_START_HSC);
udelay(1000); @@ -812,14 +801,6 @@ static void sun6i_dsi_bridge_disable(struct drm_bridge *bridge)
DRM_DEBUG_DRIVER("Disabling DSI output\n");
- if (dsi->panel) { - drm_panel_disable(dsi->panel); - drm_panel_unprepare(dsi->panel); - } else if (dsi->panel_bridge) { - dsi->panel_bridge->funcs->disable(dsi->panel_bridge); - dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge); - } - phy_power_off(dsi->dphy); phy_exit(dsi->dphy);
@@ -828,63 +809,13 @@ static void sun6i_dsi_bridge_disable(struct drm_bridge *bridge) regulator_disable(dsi->regulator); }
-static int sun6i_dsi_get_modes(struct drm_connector *connector) -{ - struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector); - - return drm_panel_get_modes(dsi->panel, connector); -} - -static const struct drm_connector_helper_funcs sun6i_dsi_connector_helper_funcs = { - .get_modes = sun6i_dsi_get_modes, -}; - -static enum drm_connector_status -sun6i_dsi_connector_detect(struct drm_connector *connector, bool force) -{ - struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector); - - return dsi->panel ? connector_status_connected : - connector_status_disconnected; -} - -static const struct drm_connector_funcs sun6i_dsi_connector_funcs = { - .detect = sun6i_dsi_connector_detect, - .fill_modes = drm_helper_probe_single_connector_modes, - .destroy = drm_connector_cleanup, - .reset = drm_atomic_helper_connector_reset, - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, -}; - static int sun6i_dsi_bridge_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) { struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge); - int ret; - - if (dsi->panel_bridge) - return drm_bridge_attach(bridge->encoder, dsi->panel_bridge, NULL, 0); - - if (dsi->panel) { - drm_connector_helper_add(&dsi->connector, - &sun6i_dsi_connector_helper_funcs); - ret = drm_connector_init(bridge->dev, &dsi->connector, - &sun6i_dsi_connector_funcs, - DRM_MODE_CONNECTOR_DSI); - if (ret) { - dev_err(dsi->dev, "Couldn't initialise the DSI connector\n"); - goto err_cleanup_connector; - } - - drm_connector_attach_encoder(&dsi->connector, &dsi->encoder); - } - - return 0;
-err_cleanup_connector: - drm_encoder_cleanup(&dsi->encoder); - return ret; + return drm_bridge_attach(bridge->encoder, dsi->panel_bridge, + &dsi->bridge, flags); }
static const struct drm_bridge_funcs sun6i_dsi_bridge_funcs = { @@ -1010,17 +941,24 @@ static int sun6i_dsi_attach(struct mipi_dsi_host *host, struct mipi_dsi_device *device) { struct sun6i_dsi *dsi = host_to_sun6i_dsi(host); + struct drm_panel *panel; int ret;
ret = drm_of_find_panel_or_bridge(dsi->dev->of_node, 0, 0, - &dsi->panel, &dsi->panel_bridge); + &panel, &dsi->panel_bridge); if (ret) return ret;
+ if (panel) { + dsi->panel_bridge = devm_drm_panel_bridge_add(dsi->dev, panel); + if (IS_ERR(dsi->panel_bridge)) + return PTR_ERR(dsi->panel_bridge); + } + dsi->device = device;
- dev_info(host->dev, "Attached %s %s\n", - device->name, dsi->panel ? "panel" : "bridge"); + dev_info(host->dev, + "Attached %s %s\n", device->name, panel ? "panel" : "bridge");
return 0; } @@ -1030,7 +968,6 @@ static int sun6i_dsi_detach(struct mipi_dsi_host *host, { struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
- dsi->panel = NULL; dsi->panel_bridge = NULL; dsi->device = NULL;
@@ -1098,6 +1035,7 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master, { struct drm_device *drm = data; struct sun6i_dsi *dsi = dev_get_drvdata(dev); + struct drm_connector *connector; int ret;
ret = drm_simple_encoder_init(drm, &dsi->encoder, @@ -1108,15 +1046,23 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master, } dsi->encoder.possible_crtcs = BIT(0);
- ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, 0); - if (ret) { - dev_err(dsi->dev, "Couldn't attach drm bridge\n"); - goto err_cleanup_connector; + ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, + DRM_BRIDGE_ATTACH_NO_CONNECTOR); + if (ret) + goto err_cleanup_encoder; + + connector = drm_bridge_connector_init(drm, &dsi->encoder); + if (IS_ERR(connector)) { + DRM_ERROR("Unable to create bridge connector\n"); + ret = PTR_ERR(connector); + goto err_cleanup_encoder; }
+ drm_connector_attach_encoder(connector, &dsi->encoder); + return 0;
-err_cleanup_connector: +err_cleanup_encoder: drm_encoder_cleanup(&dsi->encoder); return ret; } diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h index 5e70666089ad..91ea95326ed4 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h @@ -17,7 +17,6 @@
struct sun6i_dsi { struct drm_bridge bridge; - struct drm_connector connector; struct drm_encoder encoder; struct mipi_dsi_host host;
@@ -30,7 +29,6 @@ struct sun6i_dsi {
struct device *dev; struct mipi_dsi_device *device; - struct drm_panel *panel; struct drm_bridge *panel_bridge; };
@@ -44,11 +42,6 @@ static inline struct sun6i_dsi *bridge_to_sun6i_dsi(struct drm_bridge *bridge) return container_of(bridge, struct sun6i_dsi, bridge); }
-static inline struct sun6i_dsi *connector_to_sun6i_dsi(struct drm_connector *connector) -{ - return container_of(connector, struct sun6i_dsi, connector); -}; - static inline struct sun6i_dsi *encoder_to_sun6i_dsi(const struct drm_encoder *encoder) { return container_of(encoder, struct sun6i_dsi, encoder);
Hi,
On Mon, Feb 15, 2021 at 01:11:01AM +0530, Jagan Teki wrote:
Use drm_panel_bridge to replace manual panel handling code.
This simplifies the driver to allows all components in the display pipeline to be treated as bridges, paving the way to generic connector handling.
Use drm_bridge_connector_init to create a connector for display pipelines that use drm_bridge.
This allows splitting connector operations across multiple bridges when necessary, instead of having the last bridge in the chain creating the connector and handling all connector operations internally.
Signed-off-by: Jagan Teki jagan@amarulasolutions.com
Most of the code removed in that patch was actually introduced earlier which feels a bit weird. Is there a reason we can't do that one first, and then introduce the bridge support?
Maxime
On Fri, Feb 26, 2021 at 10:27 PM Maxime Ripard mripard@kernel.org wrote:
Hi,
On Mon, Feb 15, 2021 at 01:11:01AM +0530, Jagan Teki wrote:
Use drm_panel_bridge to replace manual panel handling code.
This simplifies the driver to allows all components in the display pipeline to be treated as bridges, paving the way to generic connector handling.
Use drm_bridge_connector_init to create a connector for display pipelines that use drm_bridge.
This allows splitting connector operations across multiple bridges when necessary, instead of having the last bridge in the chain creating the connector and handling all connector operations internally.
Signed-off-by: Jagan Teki jagan@amarulasolutions.com
Most of the code removed in that patch was actually introduced earlier which feels a bit weird. Is there a reason we can't do that one first, and then introduce the bridge support?
This patch adds new bridge API's which requires the driver has to support the bridge first.
Jagan.
On Fri, Feb 26, 2021 at 10:40:24PM +0530, Jagan Teki wrote:
On Fri, Feb 26, 2021 at 10:27 PM Maxime Ripard mripard@kernel.org wrote:
Hi,
On Mon, Feb 15, 2021 at 01:11:01AM +0530, Jagan Teki wrote:
Use drm_panel_bridge to replace manual panel handling code.
This simplifies the driver to allows all components in the display pipeline to be treated as bridges, paving the way to generic connector handling.
Use drm_bridge_connector_init to create a connector for display pipelines that use drm_bridge.
This allows splitting connector operations across multiple bridges when necessary, instead of having the last bridge in the chain creating the connector and handling all connector operations internally.
Signed-off-by: Jagan Teki jagan@amarulasolutions.com
Most of the code removed in that patch was actually introduced earlier which feels a bit weird. Is there a reason we can't do that one first, and then introduce the bridge support?
This patch adds new bridge API's which requires the driver has to support the bridge first.
I'm not sure what you're saying, you can definitely have a bridge without support for a downstream bridge.
Anyway, my point is that:
Changes for v3:
- new patch
drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 108 +++++++------------------ drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 7 -- 2 files changed, 27 insertions(+), 88 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index 3cdc14daf25c..5e5d3789b3df 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c @@ -20,6 +20,7 @@ #include <linux/slab.h>
#include <drm/drm_atomic_helper.h> +#include <drm/drm_bridge_connector.h> #include <drm/drm_mipi_dsi.h> #include <drm/drm_of.h> #include <drm/drm_panel.h> @@ -769,12 +770,6 @@ static void sun6i_dsi_bridge_pre_enable(struct drm_bridge *bridge) phy_set_mode(dsi->dphy, PHY_MODE_MIPI_DPHY); phy_configure(dsi->dphy, &opts); phy_power_on(dsi->dphy);
- if (dsi->panel)
drm_panel_prepare(dsi->panel);- if (dsi->panel_bridge)
dsi->panel_bridge->funcs->pre_enable(dsi->panel_bridge);
This is added in patch 2
}
static void sun6i_dsi_bridge_enable(struct drm_bridge *bridge) @@ -793,12 +788,6 @@ static void sun6i_dsi_bridge_enable(struct drm_bridge *bridge) * ordering on the panels I've tested it with, so I guess this * will do for now, until that IP is better understood. */
- if (dsi->panel)
drm_panel_enable(dsi->panel);- if (dsi->panel_bridge)
dsi->panel_bridge->funcs->enable(dsi->panel_bridge);
This is added in patch 2
sun6i_dsi_start(dsi, DSI_START_HSC);
udelay(1000); @@ -812,14 +801,6 @@ static void sun6i_dsi_bridge_disable(struct drm_bridge *bridge)
DRM_DEBUG_DRIVER("Disabling DSI output\n");
- if (dsi->panel) {
drm_panel_disable(dsi->panel);drm_panel_unprepare(dsi->panel);- } else if (dsi->panel_bridge) {
dsi->panel_bridge->funcs->disable(dsi->panel_bridge);dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge);- }
This is added in patch 2
phy_power_off(dsi->dphy); phy_exit(dsi->dphy);
@@ -828,63 +809,13 @@ static void sun6i_dsi_bridge_disable(struct drm_bridge *bridge) regulator_disable(dsi->regulator); }
-static int sun6i_dsi_get_modes(struct drm_connector *connector) -{
- struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector);
- return drm_panel_get_modes(dsi->panel, connector);
-}
-static const struct drm_connector_helper_funcs sun6i_dsi_connector_helper_funcs = {
- .get_modes = sun6i_dsi_get_modes,
-};
-static enum drm_connector_status -sun6i_dsi_connector_detect(struct drm_connector *connector, bool force) -{
- struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector);
- return dsi->panel ? connector_status_connected :
connector_status_disconnected;-}
-static const struct drm_connector_funcs sun6i_dsi_connector_funcs = {
- .detect = sun6i_dsi_connector_detect,
- .fill_modes = drm_helper_probe_single_connector_modes,
- .destroy = drm_connector_cleanup,
- .reset = drm_atomic_helper_connector_reset,
- .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
- .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-};
static int sun6i_dsi_bridge_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) { struct sun6i_dsi *dsi = bridge_to_sun6i_dsi(bridge);
- int ret;
- if (dsi->panel_bridge)
return drm_bridge_attach(bridge->encoder, dsi->panel_bridge, NULL, 0);
This is added in patch 2
- if (dsi->panel) {
drm_connector_helper_add(&dsi->connector,&sun6i_dsi_connector_helper_funcs);ret = drm_connector_init(bridge->dev, &dsi->connector,&sun6i_dsi_connector_funcs,DRM_MODE_CONNECTOR_DSI);if (ret) {dev_err(dsi->dev, "Couldn't initialise the DSI connector\n");goto err_cleanup_connector;}drm_connector_attach_encoder(&dsi->connector, &dsi->encoder);- }
This has been added (through a rework) in patch 3
Surely we can do better?
Maxime
On Tue, Mar 2, 2021 at 10:05 PM Maxime Ripard maxime@cerno.tech wrote:
On Fri, Feb 26, 2021 at 10:40:24PM +0530, Jagan Teki wrote:
On Fri, Feb 26, 2021 at 10:27 PM Maxime Ripard mripard@kernel.org wrote:
Hi,
On Mon, Feb 15, 2021 at 01:11:01AM +0530, Jagan Teki wrote:
Use drm_panel_bridge to replace manual panel handling code.
This simplifies the driver to allows all components in the display pipeline to be treated as bridges, paving the way to generic connector handling.
Use drm_bridge_connector_init to create a connector for display pipelines that use drm_bridge.
This allows splitting connector operations across multiple bridges when necessary, instead of having the last bridge in the chain creating the connector and handling all connector operations internally.
Signed-off-by: Jagan Teki jagan@amarulasolutions.com
Most of the code removed in that patch was actually introduced earlier which feels a bit weird. Is there a reason we can't do that one first, and then introduce the bridge support?
This patch adds new bridge API's which requires the driver has to support the bridge first.
I'm not sure what you're saying, you can definitely have a bridge without support for a downstream bridge.
I understand your point. what I'm saying here is, This patch introduces two new bridge API's
devm_drm_panel_bridge_add drm_bridge_connector_init
In order to add these API's the driver has to support the bridge first. All the patches before this one support bridge and this patch introduce new APIs, ie the reason we have code removed in this patch which has been added before.
Okay. I think I will send the next version series till bridge conversion. Improvement patches like this can take care of later versions and even it depends on Patch v3 5/7 which indeed require a separate discussion. This way it makes less confusion.
Hope it's fine for you?
Jagan.
This patch add support for Bananapi S070WV20-CT16 panel to BPI-M2M board.
Bananapi S070WV20-CT16 is a pure RGB output panel with ICN6211 DSI/RGB convertor bridge, so enable bridge along with associated panel.
DSI panel connected via board DSI port with, - DCDC1 as VCC-DSI supply - PL5 gpio for bridge reset gpio pin - PB7 gpio for lcd enable gpio pin - PL4 gpio for backlight enable pin
Signed-off-by: Jagan Teki jagan@amarulasolutions.com --- Changes for v3: - none
arch/arm/boot/dts/sun8i-r16-bananapi-m2m.dts | 85 ++++++++++++++++++++ 1 file changed, 85 insertions(+)
diff --git a/arch/arm/boot/dts/sun8i-r16-bananapi-m2m.dts b/arch/arm/boot/dts/sun8i-r16-bananapi-m2m.dts index e1c75f7fa3ca..f3f63187badc 100644 --- a/arch/arm/boot/dts/sun8i-r16-bananapi-m2m.dts +++ b/arch/arm/boot/dts/sun8i-r16-bananapi-m2m.dts @@ -44,6 +44,7 @@ #include "sun8i-a33.dtsi"
#include <dt-bindings/gpio/gpio.h> +#include <dt-bindings/pwm/pwm.h>
/ { model = "BananaPi M2 Magic"; @@ -55,12 +56,21 @@ aliases { i2c2 = &i2c2; serial0 = &uart0; serial1 = &uart1; + mmc0 = &mmc0; };
chosen { stdout-path = "serial0:115200n8"; };
+ backlight: backlight { + compatible = "pwm-backlight"; + pwms = <&pwm 0 50000 PWM_POLARITY_INVERTED>; + brightness-levels = <1 2 4 8 16 32 64 128 255>; + default-brightness-level = <8>; + enable-gpios = <&r_pio 0 4 GPIO_ACTIVE_HIGH>; /* LCD-BL-EN: PL4 */ + }; + leds { compatible = "gpio-leds";
@@ -81,6 +91,18 @@ red { }; };
+ panel { + compatible = "bananapi,s070wv20-ct16", "simple-panel"; + enable-gpios = <&pio 1 7 GPIO_ACTIVE_HIGH>; /* LCD-PWR-EN: PB7 */ + backlight = <&backlight>; + + port { + panel_out_bridge: endpoint { + remote-endpoint = <&bridge_out_panel>; + }; + }; + }; + reg_vcc5v0: vcc5v0 { compatible = "regulator-fixed"; regulator-name = "vcc5v0"; @@ -122,6 +144,59 @@ &dai { status = "okay"; };
+&de { + status = "okay"; +}; + +&dphy { + status = "okay"; +}; + +&dsi { + vcc-dsi-supply = <®_dcdc1>; /* VCC-DSI */ + status = "okay"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + dsi_out: port@0 { + reg = <0>; + + dsi_out_bridge: endpoint { + remote-endpoint = <&bridge_out_dsi>; + }; + }; + }; + + bridge@0 { + compatible = "chipone,icn6211"; + reg = <0>; + reset-gpios = <&r_pio 0 5 GPIO_ACTIVE_HIGH>; /* LCD-RST: PL5 */ + + ports { + #address-cells = <1>; + #size-cells = <0>; + + bridge_in: port@0 { + reg = <0>; + + bridge_out_dsi: endpoint { + remote-endpoint = <&dsi_out_bridge>; + }; + }; + + bridge_out: port@1 { + reg = <1>; + + bridge_out_panel: endpoint { + remote-endpoint = <&panel_out_bridge>; + }; + }; + }; + }; +}; + &ehci0 { status = "okay"; }; @@ -157,6 +232,12 @@ &ohci0 { status = "okay"; };
+&pwm { + pinctrl-names = "default"; + pinctrl-0 = <&pwm0_pin>; + status = "okay"; +}; + &r_rsb { status = "okay";
@@ -269,6 +350,10 @@ &sound { status = "okay"; };
+&tcon0 { + status = "okay"; +}; + &uart0 { pinctrl-names = "default"; pinctrl-0 = <&uart0_pb_pins>;
dri-devel@lists.freedesktop.org
-
Jagan Teki -
Laurent Pinchart -
Maxime Ripard -
Maxime Ripard