| Message ID | 20210910101218.1632297-25-maxime@cerno.tech |
|---|---|
| State | New |
| Headers | show |
| Series | drm/bridge: Make panel and bridge probe order consistent | expand |
W dniu 10.09.2021 o 12:12, Maxime Ripard pisze: > Without proper care and an agreement between how DSI hosts and devices > drivers register their MIPI-DSI entities and potential components, we can > end up in a situation where the drivers can never probe. > > Most drivers were taking evasive maneuvers to try to workaround this, > but not all of them were following the same conventions, resulting in > various incompatibilities between DSI hosts and devices. > > Now that we have a sequence agreed upon and documented, let's convert > exynos to it. > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> This patch should be dropped, as it will probably break the driver. Exynos is already compatible with the pattern register-bus-then-get-sink, but it adds/removes panel/bridge dynamically, so it creates drm_device without waiting for downstream sink. Regards Andrzej > --- > drivers/gpu/drm/exynos/exynos_drm_dsi.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > index e39fac889edc..dfda2b259c44 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > @@ -1529,6 +1529,7 @@ static const struct drm_encoder_helper_funcs exynos_dsi_encoder_helper_funcs = { > > MODULE_DEVICE_TABLE(of, exynos_dsi_of_match); > > +static const struct component_ops exynos_dsi_component_ops; > static int exynos_dsi_host_attach(struct mipi_dsi_host *host, > struct mipi_dsi_device *device) > { > @@ -1536,6 +1537,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, > struct drm_encoder *encoder = &dsi->encoder; > struct drm_device *drm = encoder->dev; > struct drm_bridge *out_bridge; > + struct device *dev = host->dev; > > out_bridge = of_drm_find_bridge(device->dev.of_node); > if (out_bridge) { > @@ -1585,7 +1587,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, > if (drm->mode_config.poll_enabled) > drm_kms_helper_hotplug_event(drm); > > - return 0; > + return component_add(dev, &exynos_dsi_component_ops); > } > > static int exynos_dsi_host_detach(struct mipi_dsi_host *host, > @@ -1593,6 +1595,9 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host, > { > struct exynos_dsi *dsi = host_to_dsi(host); > struct drm_device *drm = dsi->encoder.dev; > + struct device *dev = host->dev; > + > + component_del(dev, &exynos_dsi_component_ops); > > if (dsi->panel) { > mutex_lock(&drm->mode_config.mutex); > @@ -1716,7 +1721,7 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, > of_node_put(in_bridge_node); > } > > - return mipi_dsi_host_register(&dsi->dsi_host); > + return 0; > } > > static void exynos_dsi_unbind(struct device *dev, struct device *master, > @@ -1726,8 +1731,6 @@ static void exynos_dsi_unbind(struct device *dev, struct device *master, > struct drm_encoder *encoder = &dsi->encoder; > > exynos_dsi_disable(encoder); > - > - mipi_dsi_host_unregister(&dsi->dsi_host); > } > > static const struct component_ops exynos_dsi_component_ops = { > @@ -1821,7 +1824,7 @@ static int exynos_dsi_probe(struct platform_device *pdev) > > pm_runtime_enable(dev); > > - ret = component_add(dev, &exynos_dsi_component_ops); > + ret = mipi_dsi_host_register(&dsi->dsi_host); > if (ret) > goto err_disable_runtime; > > @@ -1835,10 +1838,12 @@ static int exynos_dsi_probe(struct platform_device *pdev) > > static int exynos_dsi_remove(struct platform_device *pdev) > { > + struct exynos_dsi *dsi = platform_get_drvdata(pdev); > + > + mipi_dsi_host_unregister(&dsi->dsi_host); > + > pm_runtime_disable(&pdev->dev); > > - component_del(&pdev->dev, &exynos_dsi_component_ops); > - > return 0; > } >
Hi, On 13.09.2021 12:30, Andrzej Hajda wrote: > W dniu 10.09.2021 o 12:12, Maxime Ripard pisze: >> Without proper care and an agreement between how DSI hosts and devices >> drivers register their MIPI-DSI entities and potential components, we can >> end up in a situation where the drivers can never probe. >> >> Most drivers were taking evasive maneuvers to try to workaround this, >> but not all of them were following the same conventions, resulting in >> various incompatibilities between DSI hosts and devices. >> >> Now that we have a sequence agreed upon and documented, let's convert >> exynos to it. >> >> Signed-off-by: Maxime Ripard <maxime@cerno.tech> > This patch should be dropped, as it will probably break the driver. > > Exynos is already compatible with the pattern > register-bus-then-get-sink, but it adds/removes panel/bridge > dynamically, so it creates drm_device without waiting for downstream sink. Right, this patch breaks Exynos DSI driver operation. Without it, the whole series works fine on all Exynos based test boards. Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland
Hi Marek, On Fri, Sep 17, 2021 at 02:35:05PM +0200, Marek Szyprowski wrote: > Hi, > > On 13.09.2021 12:30, Andrzej Hajda wrote: > > W dniu 10.09.2021 o 12:12, Maxime Ripard pisze: > >> Without proper care and an agreement between how DSI hosts and devices > >> drivers register their MIPI-DSI entities and potential components, we can > >> end up in a situation where the drivers can never probe. > >> > >> Most drivers were taking evasive maneuvers to try to workaround this, > >> but not all of them were following the same conventions, resulting in > >> various incompatibilities between DSI hosts and devices. > >> > >> Now that we have a sequence agreed upon and documented, let's convert > >> exynos to it. > >> > >> Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > This patch should be dropped, as it will probably break the driver. > > > > Exynos is already compatible with the pattern > > register-bus-then-get-sink, but it adds/removes panel/bridge > > dynamically, so it creates drm_device without waiting for downstream sink. > > Right, this patch breaks Exynos DSI driver operation. Without it, the > whole series works fine on all Exynos based test boards. Thanks for testing. Did you have any board using one of those bridges in your test sample? Thanks! Maxime
Hi Maxime, On 22.09.2021 10:53, Maxime Ripard wrote: > On Fri, Sep 17, 2021 at 02:35:05PM +0200, Marek Szyprowski wrote: >> On 13.09.2021 12:30, Andrzej Hajda wrote: >>> W dniu 10.09.2021 o 12:12, Maxime Ripard pisze: >>>> Without proper care and an agreement between how DSI hosts and devices >>>> drivers register their MIPI-DSI entities and potential components, we can >>>> end up in a situation where the drivers can never probe. >>>> >>>> Most drivers were taking evasive maneuvers to try to workaround this, >>>> but not all of them were following the same conventions, resulting in >>>> various incompatibilities between DSI hosts and devices. >>>> >>>> Now that we have a sequence agreed upon and documented, let's convert >>>> exynos to it. >>>> >>>> Signed-off-by: Maxime Ripard <maxime@cerno.tech> >>> This patch should be dropped, as it will probably break the driver. >>> >>> Exynos is already compatible with the pattern >>> register-bus-then-get-sink, but it adds/removes panel/bridge >>> dynamically, so it creates drm_device without waiting for downstream sink. >> Right, this patch breaks Exynos DSI driver operation. Without it, the >> whole series works fine on all Exynos based test boards. > Thanks for testing. Did you have any board using one of those bridges in > your test sample? Nope, the only bridges I've tested are tc358764 and exynos-mic. However, both are used in a bit special way. The rest of my test boards just have a dsi panel. Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index e39fac889edc..dfda2b259c44 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -1529,6 +1529,7 @@ static const struct drm_encoder_helper_funcs exynos_dsi_encoder_helper_funcs = { MODULE_DEVICE_TABLE(of, exynos_dsi_of_match); +static const struct component_ops exynos_dsi_component_ops; static int exynos_dsi_host_attach(struct mipi_dsi_host *host, struct mipi_dsi_device *device) { @@ -1536,6 +1537,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, struct drm_encoder *encoder = &dsi->encoder; struct drm_device *drm = encoder->dev; struct drm_bridge *out_bridge; + struct device *dev = host->dev; out_bridge = of_drm_find_bridge(device->dev.of_node); if (out_bridge) { @@ -1585,7 +1587,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, if (drm->mode_config.poll_enabled) drm_kms_helper_hotplug_event(drm); - return 0; + return component_add(dev, &exynos_dsi_component_ops); } static int exynos_dsi_host_detach(struct mipi_dsi_host *host, @@ -1593,6 +1595,9 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host, { struct exynos_dsi *dsi = host_to_dsi(host); struct drm_device *drm = dsi->encoder.dev; + struct device *dev = host->dev; + + component_del(dev, &exynos_dsi_component_ops); if (dsi->panel) { mutex_lock(&drm->mode_config.mutex); @@ -1716,7 +1721,7 @@ static int exynos_dsi_bind(struct device *dev, struct device *master, of_node_put(in_bridge_node); } - return mipi_dsi_host_register(&dsi->dsi_host); + return 0; } static void exynos_dsi_unbind(struct device *dev, struct device *master, @@ -1726,8 +1731,6 @@ static void exynos_dsi_unbind(struct device *dev, struct device *master, struct drm_encoder *encoder = &dsi->encoder; exynos_dsi_disable(encoder); - - mipi_dsi_host_unregister(&dsi->dsi_host); } static const struct component_ops exynos_dsi_component_ops = { @@ -1821,7 +1824,7 @@ static int exynos_dsi_probe(struct platform_device *pdev) pm_runtime_enable(dev); - ret = component_add(dev, &exynos_dsi_component_ops); + ret = mipi_dsi_host_register(&dsi->dsi_host); if (ret) goto err_disable_runtime; @@ -1835,10 +1838,12 @@ static int exynos_dsi_probe(struct platform_device *pdev) static int exynos_dsi_remove(struct platform_device *pdev) { + struct exynos_dsi *dsi = platform_get_drvdata(pdev); + + mipi_dsi_host_unregister(&dsi->dsi_host); + pm_runtime_disable(&pdev->dev); - component_del(&pdev->dev, &exynos_dsi_component_ops); - return 0; }
Without proper care and an agreement between how DSI hosts and devices drivers register their MIPI-DSI entities and potential components, we can end up in a situation where the drivers can never probe. Most drivers were taking evasive maneuvers to try to workaround this, but not all of them were following the same conventions, resulting in various incompatibilities between DSI hosts and devices. Now that we have a sequence agreed upon and documented, let's convert exynos to it. Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)