| Message ID | 20211210113617.196204-1-angelogioacchino.delregno@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/mediatek: mtk_dsi: Avoid EPROBE_DEFER loop with external bridge | expand |
Il 10/12/21 12:36, AngeloGioacchino Del Regno ha scritto: > DRM bridge drivers are now attaching their DSI device at probe time, > which requires us to register our DSI host in order to let the bridge > to probe: this recently started producing an endless -EPROBE_DEFER > loop on some machines that are using external bridges, like the > parade-ps8640, found on the ACER Chromebook R13. > > Now that the DSI hosts/devices probe sequence is documented, we can > do adjustments to the mtk_dsi driver as to both fix now and make sure > to avoid this situation in the future: for this, following what is > documented in drm_bridge.c, move the mtk_dsi component_add() to the > mtk_dsi_ops.attach callback and delete it in the detach callback; > keeping in mind that we are registering a drm_bridge for our DSI, > which is only used/attached if the DSI Host is bound, it wouldn't > make sense to keep adding our bridge at probe time (as it would > be useless to have it if mtk_dsi_ops.attach() fails!), so also move > that one to the dsi host attach function (and remove it in detach). > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> Due to the latest changes in drm bridge, we need this patch to land as soon as possible, otherwise breakages will be seen on all MediaTek powered devices using DSI to eDP/DP bridges... ...hence, friendly ping for a review :)
Hi, On 10.12.2021 12:36, AngeloGioacchino Del Regno wrote: > DRM bridge drivers are now attaching their DSI device at probe time, > which requires us to register our DSI host in order to let the bridge > to probe: this recently started producing an endless -EPROBE_DEFER > loop on some machines that are using external bridges, like the > parade-ps8640, found on the ACER Chromebook R13. > > Now that the DSI hosts/devices probe sequence is documented, we can > do adjustments to the mtk_dsi driver as to both fix now and make sure > to avoid this situation in the future: for this, following what is > documented in drm_bridge.c, move the mtk_dsi component_add() to the > mtk_dsi_ops.attach callback and delete it in the detach callback; > keeping in mind that we are registering a drm_bridge for our DSI, > which is only used/attached if the DSI Host is bound, it wouldn't > make sense to keep adding our bridge at probe time (as it would > be useless to have it if mtk_dsi_ops.attach() fails!), so also move > that one to the dsi host attach function (and remove it in detach). > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > --- > drivers/gpu/drm/mediatek/mtk_dsi.c | 167 +++++++++++++++-------------- > 1 file changed, 84 insertions(+), 83 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c > index 5d90d2eb0019..2ff347da35c2 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c > @@ -786,18 +786,101 @@ void mtk_dsi_ddp_stop(struct device *dev) > mtk_dsi_poweroff(dsi); > } > > +static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi) > +{ > + int ret; > + > + ret = drm_simple_encoder_init(drm, &dsi->encoder, > + DRM_MODE_ENCODER_DSI); > + if (ret) { > + DRM_ERROR("Failed to encoder init to drm\n"); > + return ret; > + } > + > + dsi->encoder.possible_crtcs = mtk_drm_find_possible_crtc_by_comp(drm, dsi->host.dev); > + > + ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, > + DRM_BRIDGE_ATTACH_NO_CONNECTOR); > + if (ret) > + goto err_cleanup_encoder; > + > + dsi->connector = drm_bridge_connector_init(drm, &dsi->encoder); > + if (IS_ERR(dsi->connector)) { > + DRM_ERROR("Unable to create bridge connector\n"); > + ret = PTR_ERR(dsi->connector); > + goto err_cleanup_encoder; > + } > + drm_connector_attach_encoder(dsi->connector, &dsi->encoder); > + > + return 0; > + > +err_cleanup_encoder: > + drm_encoder_cleanup(&dsi->encoder); > + return ret; > +} > + > +static int mtk_dsi_bind(struct device *dev, struct device *master, void *data) > +{ > + int ret; > + struct drm_device *drm = data; > + struct mtk_dsi *dsi = dev_get_drvdata(dev); > + > + ret = mtk_dsi_encoder_init(drm, dsi); > + if (ret) > + return ret; > + > + return device_reset_optional(dev); > +} > + > +static void mtk_dsi_unbind(struct device *dev, struct device *master, > + void *data) > +{ > + struct mtk_dsi *dsi = dev_get_drvdata(dev); > + > + drm_encoder_cleanup(&dsi->encoder); > +} > + > +static const struct component_ops mtk_dsi_component_ops = { > + .bind = mtk_dsi_bind, > + .unbind = mtk_dsi_unbind, > +}; > + > static int mtk_dsi_host_attach(struct mipi_dsi_host *host, > struct mipi_dsi_device *device) > { > struct mtk_dsi *dsi = host_to_dsi(host); > + struct device *dev = host->dev; > + int ret; > > dsi->lanes = device->lanes; > dsi->format = device->format; > dsi->mode_flags = device->mode_flags; > + dsi->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0); > + if (IS_ERR(dsi->next_bridge)) > + return PTR_ERR(dsi->next_bridge); Here we have implicit assumption that port node (0,0) points always to the 'device'. I guess it is true for now, but could bite someday. Anyway improving it would require adding some additional helper for getting bridge based on mipi_dsi_device and should be done in separate patchset. > + > + drm_bridge_add(&dsi->bridge); > + > + ret = component_add(host->dev, &mtk_dsi_component_ops); > + if (ret) { > + DRM_ERROR("failed to add dsi_host component: %d\n", ret); > + drm_bridge_remove(&dsi->bridge); > + return ret; > + } > > return 0; > } > > +static int mtk_dsi_host_detach(struct mipi_dsi_host *host, > + struct mipi_dsi_device *device) > +{ > + struct mtk_dsi *dsi = host_to_dsi(host); > + > + drm_bridge_remove(&dsi->bridge); > + component_del(host->dev, &mtk_dsi_component_ops); Order should be reversed. With this fixed: Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> Regards Andrzej > + return 0; > +} > + > static void mtk_dsi_wait_for_idle(struct mtk_dsi *dsi) > { > int ret; > @@ -938,73 +1021,14 @@ static ssize_t mtk_dsi_host_transfer(struct mipi_dsi_host *host, > > static const struct mipi_dsi_host_ops mtk_dsi_ops = { > .attach = mtk_dsi_host_attach, > + .detach = mtk_dsi_host_detach, > .transfer = mtk_dsi_host_transfer, > }; > > -static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi) > -{ > - int ret; > - > - ret = drm_simple_encoder_init(drm, &dsi->encoder, > - DRM_MODE_ENCODER_DSI); > - if (ret) { > - DRM_ERROR("Failed to encoder init to drm\n"); > - return ret; > - } > - > - dsi->encoder.possible_crtcs = mtk_drm_find_possible_crtc_by_comp(drm, dsi->host.dev); > - > - ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, > - DRM_BRIDGE_ATTACH_NO_CONNECTOR); > - if (ret) > - goto err_cleanup_encoder; > - > - dsi->connector = drm_bridge_connector_init(drm, &dsi->encoder); > - if (IS_ERR(dsi->connector)) { > - DRM_ERROR("Unable to create bridge connector\n"); > - ret = PTR_ERR(dsi->connector); > - goto err_cleanup_encoder; > - } > - drm_connector_attach_encoder(dsi->connector, &dsi->encoder); > - > - return 0; > - > -err_cleanup_encoder: > - drm_encoder_cleanup(&dsi->encoder); > - return ret; > -} > - > -static int mtk_dsi_bind(struct device *dev, struct device *master, void *data) > -{ > - int ret; > - struct drm_device *drm = data; > - struct mtk_dsi *dsi = dev_get_drvdata(dev); > - > - ret = mtk_dsi_encoder_init(drm, dsi); > - if (ret) > - return ret; > - > - return device_reset_optional(dev); > -} > - > -static void mtk_dsi_unbind(struct device *dev, struct device *master, > - void *data) > -{ > - struct mtk_dsi *dsi = dev_get_drvdata(dev); > - > - drm_encoder_cleanup(&dsi->encoder); > -} > - > -static const struct component_ops mtk_dsi_component_ops = { > - .bind = mtk_dsi_bind, > - .unbind = mtk_dsi_unbind, > -}; > - > static int mtk_dsi_probe(struct platform_device *pdev) > { > struct mtk_dsi *dsi; > struct device *dev = &pdev->dev; > - struct drm_panel *panel; > struct resource *regs; > int irq_num; > int ret; > @@ -1021,19 +1045,6 @@ static int mtk_dsi_probe(struct platform_device *pdev) > return ret; > } > > - ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, > - &panel, &dsi->next_bridge); > - if (ret) > - goto err_unregister_host; > - > - if (panel) { > - dsi->next_bridge = devm_drm_panel_bridge_add(dev, panel); > - if (IS_ERR(dsi->next_bridge)) { > - ret = PTR_ERR(dsi->next_bridge); > - goto err_unregister_host; > - } > - } > - > dsi->driver_data = of_device_get_match_data(dev); > > dsi->engine_clk = devm_clk_get(dev, "engine"); > @@ -1098,14 +1109,6 @@ static int mtk_dsi_probe(struct platform_device *pdev) > dsi->bridge.of_node = dev->of_node; > dsi->bridge.type = DRM_MODE_CONNECTOR_DSI; > > - drm_bridge_add(&dsi->bridge); > - > - ret = component_add(&pdev->dev, &mtk_dsi_component_ops); > - if (ret) { > - dev_err(&pdev->dev, "failed to add component: %d\n", ret); > - goto err_unregister_host; > - } > - > return 0; > > err_unregister_host: > @@ -1118,8 +1121,6 @@ static int mtk_dsi_remove(struct platform_device *pdev) > struct mtk_dsi *dsi = platform_get_drvdata(pdev); > > mtk_output_dsi_disable(dsi); > - drm_bridge_remove(&dsi->bridge); > - component_del(&pdev->dev, &mtk_dsi_component_ops); > mipi_dsi_host_unregister(&dsi->host); > > return 0;
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index 5d90d2eb0019..2ff347da35c2 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -786,18 +786,101 @@ void mtk_dsi_ddp_stop(struct device *dev) mtk_dsi_poweroff(dsi); } +static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi) +{ + int ret; + + ret = drm_simple_encoder_init(drm, &dsi->encoder, + DRM_MODE_ENCODER_DSI); + if (ret) { + DRM_ERROR("Failed to encoder init to drm\n"); + return ret; + } + + dsi->encoder.possible_crtcs = mtk_drm_find_possible_crtc_by_comp(drm, dsi->host.dev); + + ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, + DRM_BRIDGE_ATTACH_NO_CONNECTOR); + if (ret) + goto err_cleanup_encoder; + + dsi->connector = drm_bridge_connector_init(drm, &dsi->encoder); + if (IS_ERR(dsi->connector)) { + DRM_ERROR("Unable to create bridge connector\n"); + ret = PTR_ERR(dsi->connector); + goto err_cleanup_encoder; + } + drm_connector_attach_encoder(dsi->connector, &dsi->encoder); + + return 0; + +err_cleanup_encoder: + drm_encoder_cleanup(&dsi->encoder); + return ret; +} + +static int mtk_dsi_bind(struct device *dev, struct device *master, void *data) +{ + int ret; + struct drm_device *drm = data; + struct mtk_dsi *dsi = dev_get_drvdata(dev); + + ret = mtk_dsi_encoder_init(drm, dsi); + if (ret) + return ret; + + return device_reset_optional(dev); +} + +static void mtk_dsi_unbind(struct device *dev, struct device *master, + void *data) +{ + struct mtk_dsi *dsi = dev_get_drvdata(dev); + + drm_encoder_cleanup(&dsi->encoder); +} + +static const struct component_ops mtk_dsi_component_ops = { + .bind = mtk_dsi_bind, + .unbind = mtk_dsi_unbind, +}; + static int mtk_dsi_host_attach(struct mipi_dsi_host *host, struct mipi_dsi_device *device) { struct mtk_dsi *dsi = host_to_dsi(host); + struct device *dev = host->dev; + int ret; dsi->lanes = device->lanes; dsi->format = device->format; dsi->mode_flags = device->mode_flags; + dsi->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0); + if (IS_ERR(dsi->next_bridge)) + return PTR_ERR(dsi->next_bridge); + + drm_bridge_add(&dsi->bridge); + + ret = component_add(host->dev, &mtk_dsi_component_ops); + if (ret) { + DRM_ERROR("failed to add dsi_host component: %d\n", ret); + drm_bridge_remove(&dsi->bridge); + return ret; + } return 0; } +static int mtk_dsi_host_detach(struct mipi_dsi_host *host, + struct mipi_dsi_device *device) +{ + struct mtk_dsi *dsi = host_to_dsi(host); + + drm_bridge_remove(&dsi->bridge); + component_del(host->dev, &mtk_dsi_component_ops); + return 0; +} + static void mtk_dsi_wait_for_idle(struct mtk_dsi *dsi) { int ret; @@ -938,73 +1021,14 @@ static ssize_t mtk_dsi_host_transfer(struct mipi_dsi_host *host, static const struct mipi_dsi_host_ops mtk_dsi_ops = { .attach = mtk_dsi_host_attach, + .detach = mtk_dsi_host_detach, .transfer = mtk_dsi_host_transfer, }; -static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi) -{ - int ret; - - ret = drm_simple_encoder_init(drm, &dsi->encoder, - DRM_MODE_ENCODER_DSI); - if (ret) { - DRM_ERROR("Failed to encoder init to drm\n"); - return ret; - } - - dsi->encoder.possible_crtcs = mtk_drm_find_possible_crtc_by_comp(drm, dsi->host.dev); - - ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, - DRM_BRIDGE_ATTACH_NO_CONNECTOR); - if (ret) - goto err_cleanup_encoder; - - dsi->connector = drm_bridge_connector_init(drm, &dsi->encoder); - if (IS_ERR(dsi->connector)) { - DRM_ERROR("Unable to create bridge connector\n"); - ret = PTR_ERR(dsi->connector); - goto err_cleanup_encoder; - } - drm_connector_attach_encoder(dsi->connector, &dsi->encoder); - - return 0; - -err_cleanup_encoder: - drm_encoder_cleanup(&dsi->encoder); - return ret; -} - -static int mtk_dsi_bind(struct device *dev, struct device *master, void *data) -{ - int ret; - struct drm_device *drm = data; - struct mtk_dsi *dsi = dev_get_drvdata(dev); - - ret = mtk_dsi_encoder_init(drm, dsi); - if (ret) - return ret; - - return device_reset_optional(dev); -} - -static void mtk_dsi_unbind(struct device *dev, struct device *master, - void *data) -{ - struct mtk_dsi *dsi = dev_get_drvdata(dev); - - drm_encoder_cleanup(&dsi->encoder); -} - -static const struct component_ops mtk_dsi_component_ops = { - .bind = mtk_dsi_bind, - .unbind = mtk_dsi_unbind, -}; - static int mtk_dsi_probe(struct platform_device *pdev) { struct mtk_dsi *dsi; struct device *dev = &pdev->dev; - struct drm_panel *panel; struct resource *regs; int irq_num; int ret; @@ -1021,19 +1045,6 @@ static int mtk_dsi_probe(struct platform_device *pdev) return ret; } - ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, - &panel, &dsi->next_bridge); - if (ret) - goto err_unregister_host; - - if (panel) { - dsi->next_bridge = devm_drm_panel_bridge_add(dev, panel); - if (IS_ERR(dsi->next_bridge)) { - ret = PTR_ERR(dsi->next_bridge); - goto err_unregister_host; - } - } - dsi->driver_data = of_device_get_match_data(dev); dsi->engine_clk = devm_clk_get(dev, "engine"); @@ -1098,14 +1109,6 @@ static int mtk_dsi_probe(struct platform_device *pdev) dsi->bridge.of_node = dev->of_node; dsi->bridge.type = DRM_MODE_CONNECTOR_DSI; - drm_bridge_add(&dsi->bridge); - - ret = component_add(&pdev->dev, &mtk_dsi_component_ops); - if (ret) { - dev_err(&pdev->dev, "failed to add component: %d\n", ret); - goto err_unregister_host; - } - return 0; err_unregister_host: @@ -1118,8 +1121,6 @@ static int mtk_dsi_remove(struct platform_device *pdev) struct mtk_dsi *dsi = platform_get_drvdata(pdev); mtk_output_dsi_disable(dsi); - drm_bridge_remove(&dsi->bridge); - component_del(&pdev->dev, &mtk_dsi_component_ops); mipi_dsi_host_unregister(&dsi->host); return 0;
DRM bridge drivers are now attaching their DSI device at probe time, which requires us to register our DSI host in order to let the bridge to probe: this recently started producing an endless -EPROBE_DEFER loop on some machines that are using external bridges, like the parade-ps8640, found on the ACER Chromebook R13. Now that the DSI hosts/devices probe sequence is documented, we can do adjustments to the mtk_dsi driver as to both fix now and make sure to avoid this situation in the future: for this, following what is documented in drm_bridge.c, move the mtk_dsi component_add() to the mtk_dsi_ops.attach callback and delete it in the detach callback; keeping in mind that we are registering a drm_bridge for our DSI, which is only used/attached if the DSI Host is bound, it wouldn't make sense to keep adding our bridge at probe time (as it would be useless to have it if mtk_dsi_ops.attach() fails!), so also move that one to the dsi host attach function (and remove it in detach). Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> --- drivers/gpu/drm/mediatek/mtk_dsi.c | 167 +++++++++++++++-------------- 1 file changed, 84 insertions(+), 83 deletions(-)