| Message ID | 20200427081952.3536741-5-adrian.ratiu@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Genericize DW MIPI DSI bridge and add i.MX 6 driver | expand |
Hi Adrian, Thank you for the patch. On Mon, Apr 27, 2020 at 11:19:46AM +0300, Adrian Ratiu wrote: > 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 <laurent.pinchart@ideasonboard.com> > Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com> > --- > 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(). > 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); >
On Wed, 03 Jun 2020, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Adrian, Hi Laurent, > > Thank you for the patch. > > On Mon, Apr 27, 2020 at 11:19:46AM +0300, Adrian Ratiu wrote: >> 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 <laurent.pinchart@ideasonboard.com> Signed-off-by: >> Adrian Ratiu <adrian.ratiu@collabora.com> --- 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(). >> 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
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); 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);
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 <laurent.pinchart@ideasonboard.com> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com> --- 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(-)