[v6,2/6] drm: sun4i: dsi: Add component only once DSI device attached

Message ID 20211210111711.2072660-3-jagan@amarulasolutions.com
State New
Headers show
Series
  • drm: sun4i: dsi: Bridge support
Related show

Commit Message

Jagan Teki Dec. 10, 2021, 11:17 a.m. UTC
Having component_add for running all drm bind callbacks returns
error or unbound due to chain of DSI devices connected across
bridge topology on a display pipeline.

In a typical bridge oriented display pipeline where the host is
connected to the bridge converter and that indeed connected to
a panel.

DRM => SUN6I DSI Host => Chipone ICN6211 => BananaPi Panel

The bridge converter is looking for a panel to probe first and
then attach the host. The host attach is looking for a bridge
converter to probe and preserve bridge pointer, at this movement
the host is trying to bind the all callbacks and one of the bind
callback in the DSI host is trying to find the bridge using the
bridge pointer in sun6i_dsi_attach call.

chipone_probe().start
    drm_of_find_panel_or_bridge
        mipi_dsi_attach
             sun6i_dsi_attach
                 drm_of_find_panel_or_bridge
chipone_probe().done

sun6i_dsi_probe().start
    mipi_dsi_host_register
        component_add
sun6i_dsi_probe().done

However, the movement when panel defers the probe, will make the
bridge converter defer the host attach call which eventually found
a NULL bridge pointer during DSI component bind callback.

So, in order to prevent this scenario of binding invalid bridge,
wait for DSI devices on the pipeline to probe first and start the
binding process by moving component_add in host probe to attach call.

chipone_probe().start
    drm_of_find_panel_or_bridge
        mipi_dsi_attach
             sun6i_dsi_attach
                 drm_of_find_panel_or_bridge
      		      component_add
chipone_probe().done

sun6i_dsi_probe().start
    mipi_dsi_host_register
sun6i_dsi_probe().done

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v6:
- none
Changes for v5:
- new patch

 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 120 +++++++++++++------------
 1 file changed, 61 insertions(+), 59 deletions(-)

Comments

Maxime Ripard Dec. 13, 2021, 5:05 p.m. UTC | #1
On Fri, Dec 10, 2021 at 04:47:07PM +0530, Jagan Teki wrote:
> Having component_add for running all drm bind callbacks returns
> error or unbound due to chain of DSI devices connected across
> bridge topology on a display pipeline.

I'm not sure what that means?

> In a typical bridge oriented display pipeline where the host is
> connected to the bridge converter and that indeed connected to
> a panel.
> 
> DRM => SUN6I DSI Host => Chipone ICN6211 => BananaPi Panel
> 
> The bridge converter is looking for a panel to probe first and
> then attach the host. The host attach is looking for a bridge
> converter to probe and preserve bridge pointer, at this movement

                                                          ^ moment ?

> the host is trying to bind the all callbacks and one of the bind
> callback in the DSI host is trying to find the bridge using the
> bridge pointer in sun6i_dsi_attach call.
> 
> chipone_probe().start
>     drm_of_find_panel_or_bridge
>         mipi_dsi_attach
>              sun6i_dsi_attach
>                  drm_of_find_panel_or_bridge
> chipone_probe().done
> 
> sun6i_dsi_probe().start
>     mipi_dsi_host_register
>         component_add
> sun6i_dsi_probe().done
> 
> However, the movement when panel defers the probe, will make the
> bridge converter defer the host attach call which eventually found
> a NULL bridge pointer during DSI component bind callback.
> 
> So, in order to prevent this scenario of binding invalid bridge,
> wait for DSI devices on the pipeline to probe first and start the
> binding process by moving component_add in host probe to attach call.
> 
> chipone_probe().start
>     drm_of_find_panel_or_bridge
>         mipi_dsi_attach
>              sun6i_dsi_attach
>                  drm_of_find_panel_or_bridge
>       		      component_add
> chipone_probe().done
> 
> sun6i_dsi_probe().start
>     mipi_dsi_host_register
> sun6i_dsi_probe().done
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> Changes for v6:
> - none
> Changes for v5:
> - new patch
> 
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 120 +++++++++++++------------
>  1 file changed, 61 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 4bdcce8f1d84..9cf91dcac3f2 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -959,11 +959,63 @@ static int sun6i_dsi_dcs_read(struct sun6i_dsi *dsi,
>  	return 1;
>  }
>  
> +static int sun6i_dsi_bind(struct device *dev, struct device *master,
> +			 void *data)
> +{
> +	struct drm_device *drm = data;
> +	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) {
> +		dev_err(dsi->dev, "Couldn't initialise the DSI encoder\n");
> +		return ret;
> +	}
> +	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);
> +	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 void sun6i_dsi_unbind(struct device *dev, struct device *master,
> +			    void *data)
> +{
> +	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
> +
> +	drm_encoder_cleanup(&dsi->encoder);
> +}
> +
> +static const struct component_ops sun6i_dsi_ops = {
> +	.bind	= sun6i_dsi_bind,
> +	.unbind	= sun6i_dsi_unbind,
> +};

Just use a forward declaration there, it will make the patch more
straightforward.

Maxime

Patch

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 4bdcce8f1d84..9cf91dcac3f2 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -959,11 +959,63 @@  static int sun6i_dsi_dcs_read(struct sun6i_dsi *dsi,
 	return 1;
 }
 
+static int sun6i_dsi_bind(struct device *dev, struct device *master,
+			 void *data)
+{
+	struct drm_device *drm = data;
+	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) {
+		dev_err(dsi->dev, "Couldn't initialise the DSI encoder\n");
+		return ret;
+	}
+	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);
+	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 void sun6i_dsi_unbind(struct device *dev, struct device *master,
+			    void *data)
+{
+	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
+
+	drm_encoder_cleanup(&dsi->encoder);
+}
+
+static const struct component_ops sun6i_dsi_ops = {
+	.bind	= sun6i_dsi_bind,
+	.unbind	= sun6i_dsi_unbind,
+};
+
 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 device *dev = dsi->dev;
+	int ret;
 
 	if (IS_ERR(panel))
 		return PTR_ERR(panel);
@@ -973,6 +1025,13 @@  static int sun6i_dsi_attach(struct mipi_dsi_host *host,
 
 	dev_info(host->dev, "Attached device %s\n", device->name);
 
+	ret = component_add(dev, &sun6i_dsi_ops);
+	if (ret) {
+		dev_err(dev, "Couldn't register our component\n");
+		mipi_dsi_host_unregister(&dsi->host);
+		return ret;
+	}
+
 	return 0;
 }
 
@@ -984,6 +1043,8 @@  static int sun6i_dsi_detach(struct mipi_dsi_host *host,
 	dsi->panel = NULL;
 	dsi->device = NULL;
 
+	component_del(dsi->dev, &sun6i_dsi_ops);
+
 	return 0;
 }
 
@@ -1041,56 +1102,6 @@  static const struct regmap_config sun6i_dsi_regmap_config = {
 	.name		= "mipi-dsi",
 };
 
-static int sun6i_dsi_bind(struct device *dev, struct device *master,
-			 void *data)
-{
-	struct drm_device *drm = data;
-	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) {
-		dev_err(dsi->dev, "Couldn't initialise the DSI encoder\n");
-		return ret;
-	}
-	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);
-	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 void sun6i_dsi_unbind(struct device *dev, struct device *master,
-			    void *data)
-{
-	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
-
-	drm_encoder_cleanup(&dsi->encoder);
-}
-
-static const struct component_ops sun6i_dsi_ops = {
-	.bind	= sun6i_dsi_bind,
-	.unbind	= sun6i_dsi_unbind,
-};
-
 static int sun6i_dsi_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1172,16 +1183,8 @@  static int sun6i_dsi_probe(struct platform_device *pdev)
 		goto err_unprotect_clk;
 	}
 
-	ret = component_add(&pdev->dev, &sun6i_dsi_ops);
-	if (ret) {
-		dev_err(dev, "Couldn't register our component\n");
-		goto err_remove_dsi_host;
-	}
-
 	return 0;
 
-err_remove_dsi_host:
-	mipi_dsi_host_unregister(&dsi->host);
 err_unprotect_clk:
 	clk_rate_exclusive_put(dsi->mod_clk);
 err_attach_clk:
@@ -1195,7 +1198,6 @@  static int sun6i_dsi_remove(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
 
-	component_del(&pdev->dev, &sun6i_dsi_ops);
 	mipi_dsi_host_unregister(&dsi->host);
 	clk_rate_exclusive_put(dsi->mod_clk);