On Wed, 03 Jun 2020, Laurent Pinchart 
[off-list ref] wrote:

Hi Adrian, 

Hi Laurent,

Thank you for the patch. 

On Mon, Apr 27, 2020 at 11:19:46AM +0300, Adrian Ratiu wrote: 

quoted

Up until now the assumption was that the synopsis dsi bridge 
will directly connect to an encoder provided by the platform 
driver, but the current practice for drivers is to leave the 
encoder empty via the simple encoder API and add their logic to 
their own drm_bridge.   Thus we need an ablility to connect the 
DSI bridge to another bridge provided by the platform driver, 
so we extend the dw_mipi_dsi bind() API with a new "previous 
bridge" arg instead of just hardcoding NULL.   Cc: Laurent 
Pinchart [off-list ref] Signed-off-by: 
Adrian Ratiu [off-list ref] --- New in v8.  --- 
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c   | 6 ++++-- 
 drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c | 2 +- 
 include/drm/bridge/dw_mipi_dsi.h                | 5 ++++- 3 
 files changed, 9 insertions(+), 4 deletions(-) 
 diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c 
b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c index 
16fd87055e7b7..140ff40fa1b62 100644 --- 
a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c +++ 
b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c @@ -1456,11 
+1456,13 @@ EXPORT_SYMBOL_GPL(dw_mipi_dsi_remove); 
 /* 
  * Bind/unbind API, used from platforms based on the component 
  framework.  */ 
-int dw_mipi_dsi_bind(struct dw_mipi_dsi *dsi, struct 
drm_encoder *encoder) +int dw_mipi_dsi_bind(struct dw_mipi_dsi 
*dsi, +		     struct drm_encoder *encoder, + 
struct drm_bridge *prev_bridge) 
 { int ret;  
-	ret = drm_bridge_attach(encoder, &dsi->bridge, NULL, 0); + 
ret = drm_bridge_attach(encoder, &dsi->bridge, prev_bridge, 0); 

Please note that chaining of bridges doesn't work well if 
multiple bridges in the chain try to create a connector. This is 
why a DRM_BRIDGE_ATTACH_NO_CONNECTOR flag has been added, with a 
helper to create a connector for a chain of bridges 
(drm_bridge_connector_init()).  This won't play well with the 
component framework. I would recommend using the 
of_drm_find_bridge() instead in the rockchip driver, and 
deprecating dw_mipi_dsi_bind(). 

Thank you for this insight, indeed the bridge dw_mipi_dsi_bind() 
is clunky and we're making it even more so by possibly 
re-inventing drm_bridge_connector_init() with it in a way which 
can't work (well it does work but can lead to those nasty 
multiple-encoder corner-cases you mention).

I'll address this before posting v9, to try to move to 
of_drm_find_bridge() and remove dw_mipi_dsi_bind().

quoted

 	if (ret) {
 		DRM_ERROR("Failed to initialize bridge with drm\n");
 		return ret;

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
index 3feff0c45b3f7..83ef43be78135 100644
--- a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c

@@ -929,7 +929,7 @@ static int dw_mipi_dsi_rockchip_bind(struct device *dev,
 		return ret;
 	}
 
-	ret = dw_mipi_dsi_bind(dsi->dmd, &dsi->encoder);
+	ret = dw_mipi_dsi_bind(dsi->dmd, &dsi->encoder, NULL);
 	if (ret) {
 		DRM_DEV_ERROR(dev, "Failed to bind: %d\n", ret);
 		return ret;

diff --git a/include/drm/bridge/dw_mipi_dsi.h b/include/drm/bridge/dw_mipi_dsi.h
index b0e390b3288e8..699b3531f5b36 100644
--- a/include/drm/bridge/dw_mipi_dsi.h
+++ b/include/drm/bridge/dw_mipi_dsi.h

@@ -14,6 +14,7 @@
 #include <drm/drm_modes.h>
 
 struct drm_display_mode;
+struct drm_bridge;
 struct drm_encoder;
 struct dw_mipi_dsi;
 struct mipi_dsi_device;

@@ -62,7 +63,9 @@ struct dw_mipi_dsi *dw_mipi_dsi_probe(struct platform_device *pdev,
 				      const struct dw_mipi_dsi_plat_data
 				      *plat_data);
 void dw_mipi_dsi_remove(struct dw_mipi_dsi *dsi);
-int dw_mipi_dsi_bind(struct dw_mipi_dsi *dsi, struct drm_encoder *encoder);
+int dw_mipi_dsi_bind(struct dw_mipi_dsi *dsi,
+		     struct drm_encoder *encoder,
+		     struct drm_bridge *prev_bridge);
 void dw_mipi_dsi_unbind(struct dw_mipi_dsi *dsi);
 void dw_mipi_dsi_set_slave(struct dw_mipi_dsi *dsi, struct dw_mipi_dsi *slave);
 

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel