| Message ID | 20200416155720.2360443-8-enric.balletbo@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Convert mtk-dsi to drm_bridge API and get EDID for ps8640 bridge | expand |
Hi Enric, Thank you for the patch. On Thu, Apr 16, 2020 at 05:57:19PM +0200, Enric Balletbo i Serra wrote: > Use the drm_bridge_connector helper to create a connector for pipelines > that use drm_bridge. This allows splitting connector operations across > multiple bridges when necessary, instead of having the last bridge in > the chain creating the connector and handling all connector operations > internally. That's the right direction, but this should be done in the mtk display controller driver core, not in here. I'm OK with the code being here as an interim measure if needed to move forward, but that should then be temporary only. > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > --- > > Changes in v2: None > > drivers/gpu/drm/mediatek/mtk_dsi.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c > index 44718fa3d1ca..2f8876c32864 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c > @@ -17,6 +17,7 @@ > > #include <drm/drm_atomic_helper.h> > #include <drm/drm_bridge.h> > +#include <drm/drm_bridge_connector.h> > #include <drm/drm_mipi_dsi.h> > #include <drm/drm_of.h> > #include <drm/drm_panel.h> > @@ -184,6 +185,7 @@ struct mtk_dsi { > struct drm_bridge bridge; > struct drm_bridge *panel_bridge; > struct drm_bridge *next_bridge; > + struct drm_connector *connector; > struct phy *phy; > > void __iomem *regs; > @@ -983,10 +985,19 @@ static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi) > */ > dsi->encoder.possible_crtcs = 1; > > - ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, 0); > + 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: > @@ -1144,6 +1155,7 @@ static int mtk_dsi_probe(struct platform_device *pdev) > > dsi->bridge.funcs = &mtk_dsi_bridge_funcs; > dsi->bridge.of_node = dev->of_node; > + dsi->bridge.type = DRM_MODE_CONNECTOR_DSI; I think this line belongs to the patch that adds drm_bridge support to this driver. > > drm_bridge_add(&dsi->bridge); >
Hi Enric, On Thu, Apr 16, 2020 at 08:35:26PM +0300, Laurent Pinchart wrote: > On Thu, Apr 16, 2020 at 05:57:19PM +0200, Enric Balletbo i Serra wrote: > > Use the drm_bridge_connector helper to create a connector for pipelines > > that use drm_bridge. This allows splitting connector operations across > > multiple bridges when necessary, instead of having the last bridge in > > the chain creating the connector and handling all connector operations > > internally. > > That's the right direction, but this should be done in the mtk display > controller driver core, not in here. I'm OK with the code being here as > an interim measure if needed to move forward, but that should then be > temporary only. I forgot to mention that the drm_encoder should also move out of the bridge driver to the display controller driver. > > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > > --- > > > > Changes in v2: None > > > > drivers/gpu/drm/mediatek/mtk_dsi.c | 14 +++++++++++++- > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c > > index 44718fa3d1ca..2f8876c32864 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c > > @@ -17,6 +17,7 @@ > > > > #include <drm/drm_atomic_helper.h> > > #include <drm/drm_bridge.h> > > +#include <drm/drm_bridge_connector.h> > > #include <drm/drm_mipi_dsi.h> > > #include <drm/drm_of.h> > > #include <drm/drm_panel.h> > > @@ -184,6 +185,7 @@ struct mtk_dsi { > > struct drm_bridge bridge; > > struct drm_bridge *panel_bridge; > > struct drm_bridge *next_bridge; > > + struct drm_connector *connector; > > struct phy *phy; > > > > void __iomem *regs; > > @@ -983,10 +985,19 @@ static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi) > > */ > > dsi->encoder.possible_crtcs = 1; > > > > - ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, 0); > > + 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: > > @@ -1144,6 +1155,7 @@ static int mtk_dsi_probe(struct platform_device *pdev) > > > > dsi->bridge.funcs = &mtk_dsi_bridge_funcs; > > dsi->bridge.of_node = dev->of_node; > > + dsi->bridge.type = DRM_MODE_CONNECTOR_DSI; > > I think this line belongs to the patch that adds drm_bridge support to > this driver. > > > > > drm_bridge_add(&dsi->bridge); > >
Hi Laurent, On 16/4/20 19:36, Laurent Pinchart wrote: > Hi Enric, > > On Thu, Apr 16, 2020 at 08:35:26PM +0300, Laurent Pinchart wrote: >> On Thu, Apr 16, 2020 at 05:57:19PM +0200, Enric Balletbo i Serra wrote: >>> Use the drm_bridge_connector helper to create a connector for pipelines >>> that use drm_bridge. This allows splitting connector operations across >>> multiple bridges when necessary, instead of having the last bridge in >>> the chain creating the connector and handling all connector operations >>> internally. >> >> That's the right direction, but this should be done in the mtk display >> controller driver core, not in here. I'm OK with the code being here as >> an interim measure if needed to move forward, but that should then be >> temporary only. It'd be nice if we can do this as an interim measure for now, so at least we have the embedded display working. IIUC to move that to the display controller driver core I should also convert/rework the mtk_dpi and mtk_hdmi drivers. This is used for the external display on my device but to fully support this I'll also need to rework the bridge chain logic to handle the multi-sink/multi-source use case. This is something I plan to work on but I suspect won't be easy and will trigger lots of discussions, and, of course, some time. So, if is fine I won't move this for now. Thanks, Enric > > I forgot to mention that the drm_encoder should also move out of the > bridge driver to the display controller driver. > >>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> >>> --- >>> >>> Changes in v2: None >>> >>> drivers/gpu/drm/mediatek/mtk_dsi.c | 14 +++++++++++++- >>> 1 file changed, 13 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c >>> index 44718fa3d1ca..2f8876c32864 100644 >>> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c >>> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c >>> @@ -17,6 +17,7 @@ >>> >>> #include <drm/drm_atomic_helper.h> >>> #include <drm/drm_bridge.h> >>> +#include <drm/drm_bridge_connector.h> >>> #include <drm/drm_mipi_dsi.h> >>> #include <drm/drm_of.h> >>> #include <drm/drm_panel.h> >>> @@ -184,6 +185,7 @@ struct mtk_dsi { >>> struct drm_bridge bridge; >>> struct drm_bridge *panel_bridge; >>> struct drm_bridge *next_bridge; >>> + struct drm_connector *connector; >>> struct phy *phy; >>> >>> void __iomem *regs; >>> @@ -983,10 +985,19 @@ static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi) >>> */ >>> dsi->encoder.possible_crtcs = 1; >>> >>> - ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, 0); >>> + 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: >>> @@ -1144,6 +1155,7 @@ static int mtk_dsi_probe(struct platform_device *pdev) >>> >>> dsi->bridge.funcs = &mtk_dsi_bridge_funcs; >>> dsi->bridge.of_node = dev->of_node; >>> + dsi->bridge.type = DRM_MODE_CONNECTOR_DSI; >> >> I think this line belongs to the patch that adds drm_bridge support to >> this driver. >> >>> >>> drm_bridge_add(&dsi->bridge); >>> >
Hi Enric, On Thu, Apr 16, 2020 at 11:33:24PM +0200, Enric Balletbo i Serra wrote: > On 16/4/20 19:36, Laurent Pinchart wrote: > > On Thu, Apr 16, 2020 at 08:35:26PM +0300, Laurent Pinchart wrote: > >> On Thu, Apr 16, 2020 at 05:57:19PM +0200, Enric Balletbo i Serra wrote: > >>> Use the drm_bridge_connector helper to create a connector for pipelines > >>> that use drm_bridge. This allows splitting connector operations across > >>> multiple bridges when necessary, instead of having the last bridge in > >>> the chain creating the connector and handling all connector operations > >>> internally. > >> > >> That's the right direction, but this should be done in the mtk display > >> controller driver core, not in here. I'm OK with the code being here as > >> an interim measure if needed to move forward, but that should then be > >> temporary only. > > It'd be nice if we can do this as an interim measure for now, so at least we > have the embedded display working. IIUC to move that to the display controller > driver core I should also convert/rework the mtk_dpi and mtk_hdmi drivers. This > is used for the external display on my device but to fully support this I'll > also need to rework the bridge chain logic to handle the multi-sink/multi-source > use case. This is something I plan to work on but I suspect won't be easy and > will trigger lots of discussions, and, of course, some time. > > So, if is fine I won't move this for now. That's totally fine with me, I just wanted to make sure you were aware that more work was needed :-) Thanks for all your efforts ! > > I forgot to mention that the drm_encoder should also move out of the > > bridge driver to the display controller driver. > > > >>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > >>> --- > >>> > >>> Changes in v2: None > >>> > >>> drivers/gpu/drm/mediatek/mtk_dsi.c | 14 +++++++++++++- > >>> 1 file changed, 13 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c > >>> index 44718fa3d1ca..2f8876c32864 100644 > >>> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > >>> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c > >>> @@ -17,6 +17,7 @@ > >>> > >>> #include <drm/drm_atomic_helper.h> > >>> #include <drm/drm_bridge.h> > >>> +#include <drm/drm_bridge_connector.h> > >>> #include <drm/drm_mipi_dsi.h> > >>> #include <drm/drm_of.h> > >>> #include <drm/drm_panel.h> > >>> @@ -184,6 +185,7 @@ struct mtk_dsi { > >>> struct drm_bridge bridge; > >>> struct drm_bridge *panel_bridge; > >>> struct drm_bridge *next_bridge; > >>> + struct drm_connector *connector; > >>> struct phy *phy; > >>> > >>> void __iomem *regs; > >>> @@ -983,10 +985,19 @@ static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi) > >>> */ > >>> dsi->encoder.possible_crtcs = 1; > >>> > >>> - ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, 0); > >>> + 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: > >>> @@ -1144,6 +1155,7 @@ static int mtk_dsi_probe(struct platform_device *pdev) > >>> > >>> dsi->bridge.funcs = &mtk_dsi_bridge_funcs; > >>> dsi->bridge.of_node = dev->of_node; > >>> + dsi->bridge.type = DRM_MODE_CONNECTOR_DSI; > >> > >> I think this line belongs to the patch that adds drm_bridge support to > >> this driver. > >> > >>> > >>> drm_bridge_add(&dsi->bridge); > >>>
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index 44718fa3d1ca..2f8876c32864 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -17,6 +17,7 @@ #include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> +#include <drm/drm_bridge_connector.h> #include <drm/drm_mipi_dsi.h> #include <drm/drm_of.h> #include <drm/drm_panel.h> @@ -184,6 +185,7 @@ struct mtk_dsi { struct drm_bridge bridge; struct drm_bridge *panel_bridge; struct drm_bridge *next_bridge; + struct drm_connector *connector; struct phy *phy; void __iomem *regs; @@ -983,10 +985,19 @@ static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi) */ dsi->encoder.possible_crtcs = 1; - ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, 0); + 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: @@ -1144,6 +1155,7 @@ static int mtk_dsi_probe(struct platform_device *pdev) dsi->bridge.funcs = &mtk_dsi_bridge_funcs; dsi->bridge.of_node = dev->of_node; + dsi->bridge.type = DRM_MODE_CONNECTOR_DSI; drm_bridge_add(&dsi->bridge);
Use the drm_bridge_connector helper to create a connector for pipelines that use drm_bridge. This allows splitting connector operations across multiple bridges when necessary, instead of having the last bridge in the chain creating the connector and handling all connector operations internally. Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> --- Changes in v2: None drivers/gpu/drm/mediatek/mtk_dsi.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)