| Message ID | 20200501152335.1805790-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 Chun-Kuang, Missatge de Enric Balletbo i Serra <enric.balletbo@collabora.com> del dia dv., 1 de maig 2020 a les 17:25: > > 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> > Acked-by: Sam Ravnborg <sam@ravnborg.org> A gentle ping on this, I think that this one is the only one that still needs a review in the series. Thanks, Enric > --- > > Changes in v4: None > Changes in v3: > - Move the bridge.type line to the patch that adds drm_bridge support. (Laurent Pinchart) > > Changes in v2: None > > drivers/gpu/drm/mediatek/mtk_dsi.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c > index 4f3bd095c1ee..471fcafdf348 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> > @@ -183,6 +184,7 @@ struct mtk_dsi { > struct drm_encoder encoder; > struct drm_bridge bridge; > struct drm_bridge *next_bridge; > + struct drm_connector *connector; > struct phy *phy; > > void __iomem *regs; > @@ -977,10 +979,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: > -- > 2.26.2 > > > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-mediatek
Hi, Enric: Enric Balletbo Serra <eballetbo@gmail.com> 於 2020年5月14日 週四 上午12:41寫道： > > Hi Chun-Kuang, > > Missatge de Enric Balletbo i Serra <enric.balletbo@collabora.com> del > dia dv., 1 de maig 2020 a les 17:25: > > > > 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> > > Acked-by: Sam Ravnborg <sam@ravnborg.org> > > A gentle ping on this, I think that this one is the only one that > still needs a review in the series. This is what I reply in patch v3: I think the panel is wrapped into next_bridge here, if (panel) { dsi->next_bridge = devm_drm_panel_bridge_add(dev, panel); so the next_bridge is a panel_bridge, in its attach function panel_bridge_attach(), according to the flag DRM_BRIDGE_ATTACH_NO_CONNECTOR, if not exist, it would create connector and attach connector to panel. I'm not sure this flag would exist or not, but for both case, it's strange. If exist, you create connector in this patch but no where to attach connector to panel. If not exist, the next_brige would create one connector and this brige would create another connector. I think in your case, mtk_dsi does not directly connect to a panel, so I need a exact explain. Or someone could test this on a directly-connect-panel platform. Regards, Chun-Kuang. > > Thanks, > Enric > > > --- > > > > Changes in v4: None > > Changes in v3: > > - Move the bridge.type line to the patch that adds drm_bridge support. (Laurent Pinchart) > > > > Changes in v2: None > > > > drivers/gpu/drm/mediatek/mtk_dsi.c | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c > > index 4f3bd095c1ee..471fcafdf348 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> > > @@ -183,6 +184,7 @@ struct mtk_dsi { > > struct drm_encoder encoder; > > struct drm_bridge bridge; > > struct drm_bridge *next_bridge; > > + struct drm_connector *connector; > > struct phy *phy; > > > > void __iomem *regs; > > @@ -977,10 +979,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: > > -- > > 2.26.2 > > > > > > _______________________________________________ > > Linux-mediatek mailing list > > Linux-mediatek@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-mediatek
Hi Chun-Kuang, On 14/5/20 16:28, Chun-Kuang Hu wrote: > Hi, Enric: > > Enric Balletbo Serra <eballetbo@gmail.com> 於 2020年5月14日 週四 上午12:41寫道： >> >> Hi Chun-Kuang, >> >> Missatge de Enric Balletbo i Serra <enric.balletbo@collabora.com> del >> dia dv., 1 de maig 2020 a les 17:25: >>> >>> 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> >>> Acked-by: Sam Ravnborg <sam@ravnborg.org> >> >> A gentle ping on this, I think that this one is the only one that >> still needs a review in the series. > > This is what I reply in patch v3: > Sorry for missing this. > I think the panel is wrapped into next_bridge here, > Yes, you can have for example: 1. drm_bridge (mtk_dsi) -> drm_bridge (ps8640 - dsi-to-edp) -> drm_panel_bridge (edp panel) or a 2. drm_bridge (mtk_dsi)-> drm_panel_bridge (dsi panel) The _first_ one is my use case > if (panel) { This handles the second case, where you attach a dsi panel. > dsi->next_bridge = devm_drm_panel_bridge_add(dev, panel); > > so the next_bridge is a panel_bridge, in its attach function > panel_bridge_attach(), > according to the flag DRM_BRIDGE_ATTACH_NO_CONNECTOR, if not exist, > it would create connector and attach connector to panel. > > I'm not sure this flag would exist or not, but for both case, it's strange. > If exist, you create connector in this patch but no where to attach > connector to panel. Yes, in fact, this is transitional patch needed, as once I converted mtk_dpi, mtk_dsi and mtk_hdmi to the new drm_bridge API the drm_bridge_connector_init() will be done in mtk_drm_drv. We will need to call drm_bridge_connector_init for dpi and dsi pipes and remove that call from mtk_dsi and mtk_dpi drivers. The graphic controller driver should create connectors and CRTCs, as example you can take a look at drivers/gpu/drm/omapdrm/omap_drv.c > If not exist, the next_brige would create one connector and this brige > would create another connector. > > I think in your case, mtk_dsi does not directly connect to a panel, so Exactly > I need a exact explain. Or someone could test this on a > directly-connect-panel platform. I don't think I am breaking this use case but AFAICS there is no users in mainline that directly connect a panel using the mediatek driver. As I said my use case is the other so I can't really test. Do you know anyone that can test this? Thanks, Enric > > Regards, > Chun-Kuang. > >> >> Thanks, >> Enric >> >>> --- >>> >>> Changes in v4: None >>> Changes in v3: >>> - Move the bridge.type line to the patch that adds drm_bridge support. (Laurent Pinchart) >>> >>> Changes in v2: None >>> >>> drivers/gpu/drm/mediatek/mtk_dsi.c | 13 ++++++++++++- >>> 1 file changed, 12 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c >>> index 4f3bd095c1ee..471fcafdf348 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> >>> @@ -183,6 +184,7 @@ struct mtk_dsi { >>> struct drm_encoder encoder; >>> struct drm_bridge bridge; >>> struct drm_bridge *next_bridge; >>> + struct drm_connector *connector; >>> struct phy *phy; >>> >>> void __iomem *regs; >>> @@ -977,10 +979,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: >>> -- >>> 2.26.2 >>> >>> >>> _______________________________________________ >>> Linux-mediatek mailing list >>> Linux-mediatek@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-mediatek
Hi, Enric: Enric Balletbo i Serra <enric.balletbo@collabora.com> 於 2020年5月14日 週四 下午11:42寫道： > > Hi Chun-Kuang, > > On 14/5/20 16:28, Chun-Kuang Hu wrote: > > Hi, Enric: > > > > Enric Balletbo Serra <eballetbo@gmail.com> 於 2020年5月14日 週四 上午12:41寫道： > >> > >> Hi Chun-Kuang, > >> > >> Missatge de Enric Balletbo i Serra <enric.balletbo@collabora.com> del > >> dia dv., 1 de maig 2020 a les 17:25: > >>> > >>> 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> > >>> Acked-by: Sam Ravnborg <sam@ravnborg.org> > >> > >> A gentle ping on this, I think that this one is the only one that > >> still needs a review in the series. > > > > This is what I reply in patch v3: > > > > Sorry for missing this. > > > I think the panel is wrapped into next_bridge here, > > > > Yes, you can have for example: > > 1. drm_bridge (mtk_dsi) -> drm_bridge (ps8640 - dsi-to-edp) -> drm_panel_bridge > (edp panel) > > or a > > 2. drm_bridge (mtk_dsi)-> drm_panel_bridge (dsi panel) > > The _first_ one is my use case > > > if (panel) { > > This handles the second case, where you attach a dsi panel. > > > dsi->next_bridge = devm_drm_panel_bridge_add(dev, panel); > > > > so the next_bridge is a panel_bridge, in its attach function > > panel_bridge_attach(), > > according to the flag DRM_BRIDGE_ATTACH_NO_CONNECTOR, if not exist, > > it would create connector and attach connector to panel. > > > > I'm not sure this flag would exist or not, but for both case, it's strange. > > If exist, you create connector in this patch but no where to attach > > connector to panel. > > Yes, in fact, this is transitional patch needed, as once I converted mtk_dpi, > mtk_dsi and mtk_hdmi to the new drm_bridge API the drm_bridge_connector_init() > will be done in mtk_drm_drv. We will need to call drm_bridge_connector_init for > dpi and dsi pipes and remove that call from mtk_dsi and mtk_dpi drivers. The > graphic controller driver should create connectors and CRTCs, as example you can > take a look at drivers/gpu/drm/omapdrm/omap_drv.c > I have such question because I've reviewed omap's driver. In omap's driver, after it call drm_bridge_connector_init(), it does this: if (pipe->output->panel) { ret = drm_panel_attach(pipe->output->panel, pipe->connector); if (ret < 0) return ret; } In this patch, you does not do this. > > If not exist, the next_brige would create one connector and this brige > > would create another connector. > > > > I think in your case, mtk_dsi does not directly connect to a panel, so > > Exactly > > > I need a exact explain. Or someone could test this on a > > directly-connect-panel platform. > > I don't think I am breaking this use case but AFAICS there is no users in > mainline that directly connect a panel using the mediatek driver. As I said my > use case is the other so I can't really test. Do you know anyone that can test this? I'm not sure who can test this, but [1], which is sent by YT Shen in a series, is a patch to support dsi command mode so dsi could directly connect to panel. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpu/drm/mediatek?h=v5.7-rc5&id=21898816831fc60c92dd634ab4316a24da7eb4af It's better that someone could test this case, but if no one would test this, I could also accept a good-look patch. Regards, Chun-Kuang. > > Thanks, > Enric > > > > > Regards, > > Chun-Kuang. > > > >> > >> Thanks, > >> Enric > >> > >>> --- > >>> > >>> Changes in v4: None > >>> Changes in v3: > >>> - Move the bridge.type line to the patch that adds drm_bridge support. (Laurent Pinchart) > >>> > >>> Changes in v2: None > >>> > >>> drivers/gpu/drm/mediatek/mtk_dsi.c | 13 ++++++++++++- > >>> 1 file changed, 12 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c > >>> index 4f3bd095c1ee..471fcafdf348 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> > >>> @@ -183,6 +184,7 @@ struct mtk_dsi { > >>> struct drm_encoder encoder; > >>> struct drm_bridge bridge; > >>> struct drm_bridge *next_bridge; > >>> + struct drm_connector *connector; > >>> struct phy *phy; > >>> > >>> void __iomem *regs; > >>> @@ -977,10 +979,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: > >>> -- > >>> 2.26.2 > >>> > >>> > >>> _______________________________________________ > >>> Linux-mediatek mailing list > >>> Linux-mediatek@lists.infradead.org > >>> http://lists.infradead.org/mailman/listinfo/linux-mediatek
Hi Chun-Kuang, On 14/5/20 18:44, Chun-Kuang Hu wrote: > Hi, Enric: > > Enric Balletbo i Serra <enric.balletbo@collabora.com> 於 2020年5月14日 週四 下午11:42寫道： >> >> Hi Chun-Kuang, >> >> On 14/5/20 16:28, Chun-Kuang Hu wrote: >>> Hi, Enric: >>> >>> Enric Balletbo Serra <eballetbo@gmail.com> 於 2020年5月14日 週四 上午12:41寫道： >>>> >>>> Hi Chun-Kuang, >>>> >>>> Missatge de Enric Balletbo i Serra <enric.balletbo@collabora.com> del >>>> dia dv., 1 de maig 2020 a les 17:25: >>>>> >>>>> 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> >>>>> Acked-by: Sam Ravnborg <sam@ravnborg.org> >>>> >>>> A gentle ping on this, I think that this one is the only one that >>>> still needs a review in the series. >>> >>> This is what I reply in patch v3: >>> >> >> Sorry for missing this. >> >>> I think the panel is wrapped into next_bridge here, >>> >> >> Yes, you can have for example: >> >> 1. drm_bridge (mtk_dsi) -> drm_bridge (ps8640 - dsi-to-edp) -> drm_panel_bridge >> (edp panel) >> >> or a >> >> 2. drm_bridge (mtk_dsi)-> drm_panel_bridge (dsi panel) >> >> The _first_ one is my use case >> >>> if (panel) { >> >> This handles the second case, where you attach a dsi panel. >> >>> dsi->next_bridge = devm_drm_panel_bridge_add(dev, panel); >>> >>> so the next_bridge is a panel_bridge, in its attach function >>> panel_bridge_attach(), >>> according to the flag DRM_BRIDGE_ATTACH_NO_CONNECTOR, if not exist, >>> it would create connector and attach connector to panel. >>> >>> I'm not sure this flag would exist or not, but for both case, it's strange. >>> If exist, you create connector in this patch but no where to attach >>> connector to panel. >> >> Yes, in fact, this is transitional patch needed, as once I converted mtk_dpi, >> mtk_dsi and mtk_hdmi to the new drm_bridge API the drm_bridge_connector_init() >> will be done in mtk_drm_drv. We will need to call drm_bridge_connector_init for >> dpi and dsi pipes and remove that call from mtk_dsi and mtk_dpi drivers. The >> graphic controller driver should create connectors and CRTCs, as example you can >> take a look at drivers/gpu/drm/omapdrm/omap_drv.c >> > > I have such question because I've reviewed omap's driver. In omap's > driver, after it call drm_bridge_connector_init(), it does this: > > if (pipe->output->panel) { > ret = drm_panel_attach(pipe->output->panel, > pipe->connector); > if (ret < 0) > return ret; > } > > In this patch, you does not do this. > I see, so yes, I am probably missing call drm_panel_attach in case there is a direct panel attached. Thanks for pointing it. I'll send a new version adding the drm_panel_attach call. >>> If not exist, the next_brige would create one connector and this brige >>> would create another connector. >>> >>> I think in your case, mtk_dsi does not directly connect to a panel, so >> >> Exactly >> >>> I need a exact explain. Or someone could test this on a >>> directly-connect-panel platform. >> >> I don't think I am breaking this use case but AFAICS there is no users in >> mainline that directly connect a panel using the mediatek driver. As I said my >> use case is the other so I can't really test. Do you know anyone that can test this? > > I'm not sure who can test this, but [1], which is sent by YT Shen in a > series, is a patch to support dsi command mode so dsi could directly > connect to panel. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpu/drm/mediatek?h=v5.7-rc5&id=21898816831fc60c92dd634ab4316a24da7eb4af > > It's better that someone could test this case, but if no one would > test this, I could also accept a good-look patch. > > Regards, > Chun-Kuang. > >> >> Thanks, >> Enric >> >>> >>> Regards, >>> Chun-Kuang. >>> >>>> >>>> Thanks, >>>> Enric >>>> >>>>> --- >>>>> >>>>> Changes in v4: None >>>>> Changes in v3: >>>>> - Move the bridge.type line to the patch that adds drm_bridge support. (Laurent Pinchart) >>>>> >>>>> Changes in v2: None >>>>> >>>>> drivers/gpu/drm/mediatek/mtk_dsi.c | 13 ++++++++++++- >>>>> 1 file changed, 12 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c >>>>> index 4f3bd095c1ee..471fcafdf348 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> >>>>> @@ -183,6 +184,7 @@ struct mtk_dsi { >>>>> struct drm_encoder encoder; >>>>> struct drm_bridge bridge; >>>>> struct drm_bridge *next_bridge; >>>>> + struct drm_connector *connector; >>>>> struct phy *phy; >>>>> >>>>> void __iomem *regs; >>>>> @@ -977,10 +979,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: >>>>> -- >>>>> 2.26.2 >>>>> >>>>> >>>>> _______________________________________________ >>>>> Linux-mediatek mailing list >>>>> Linux-mediatek@lists.infradead.org >>>>> http://lists.infradead.org/mailman/listinfo/linux-mediatek
Hi again, On 14/5/20 19:12, Enric Balletbo i Serra wrote: > Hi Chun-Kuang, > > On 14/5/20 18:44, Chun-Kuang Hu wrote: >> Hi, Enric: >> >> Enric Balletbo i Serra <enric.balletbo@collabora.com> 於 2020年5月14日 週四 下午11:42寫道： >>> >>> Hi Chun-Kuang, >>> >>> On 14/5/20 16:28, Chun-Kuang Hu wrote: >>>> Hi, Enric: >>>> >>>> Enric Balletbo Serra <eballetbo@gmail.com> 於 2020年5月14日 週四 上午12:41寫道： >>>>> >>>>> Hi Chun-Kuang, >>>>> >>>>> Missatge de Enric Balletbo i Serra <enric.balletbo@collabora.com> del >>>>> dia dv., 1 de maig 2020 a les 17:25: >>>>>> >>>>>> 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> >>>>>> Acked-by: Sam Ravnborg <sam@ravnborg.org> >>>>> >>>>> A gentle ping on this, I think that this one is the only one that >>>>> still needs a review in the series. >>>> >>>> This is what I reply in patch v3: >>>> >>> >>> Sorry for missing this. >>> >>>> I think the panel is wrapped into next_bridge here, >>>> >>> >>> Yes, you can have for example: >>> >>> 1. drm_bridge (mtk_dsi) -> drm_bridge (ps8640 - dsi-to-edp) -> drm_panel_bridge >>> (edp panel) >>> >>> or a >>> >>> 2. drm_bridge (mtk_dsi)-> drm_panel_bridge (dsi panel) >>> >>> The _first_ one is my use case >>> >>>> if (panel) { >>> >>> This handles the second case, where you attach a dsi panel. >>> >>>> dsi->next_bridge = devm_drm_panel_bridge_add(dev, panel); >>>> >>>> so the next_bridge is a panel_bridge, in its attach function >>>> panel_bridge_attach(), >>>> according to the flag DRM_BRIDGE_ATTACH_NO_CONNECTOR, if not exist, >>>> it would create connector and attach connector to panel. >>>> >>>> I'm not sure this flag would exist or not, but for both case, it's strange. >>>> If exist, you create connector in this patch but no where to attach >>>> connector to panel. >>> >>> Yes, in fact, this is transitional patch needed, as once I converted mtk_dpi, >>> mtk_dsi and mtk_hdmi to the new drm_bridge API the drm_bridge_connector_init() >>> will be done in mtk_drm_drv. We will need to call drm_bridge_connector_init for >>> dpi and dsi pipes and remove that call from mtk_dsi and mtk_dpi drivers. The >>> graphic controller driver should create connectors and CRTCs, as example you can >>> take a look at drivers/gpu/drm/omapdrm/omap_drv.c >>> >> >> I have such question because I've reviewed omap's driver. In omap's >> driver, after it call drm_bridge_connector_init(), it does this: >> >> if (pipe->output->panel) { >> ret = drm_panel_attach(pipe->output->panel, >> pipe->connector); >> if (ret < 0) >> return ret; >> } >> >> In this patch, you does not do this. >> > > I see, so yes, I am probably missing call drm_panel_attach in case there is a > direct panel attached. Thanks for pointing it. > > I'll send a new version adding the drm_panel_attach call. > Wait, shouldn't panel be attached on the call of mtk_dsi_bridge_attach as next_bridge points to a bridge or a panel? static int mtk_dsi_bridge_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) { struct mtk_dsi *dsi = bridge_to_dsi(bridge); /* Attach the panel or bridge to the dsi bridge */ return drm_bridge_attach(bridge->encoder, dsi->next_bridge, &dsi->bridge, flags); } Or I am continuing misunderstanding all this? >>>> If not exist, the next_brige would create one connector and this brige >>>> would create another connector. >>>> >>>> I think in your case, mtk_dsi does not directly connect to a panel, so >>> >>> Exactly >>> >>>> I need a exact explain. Or someone could test this on a >>>> directly-connect-panel platform. >>> >>> I don't think I am breaking this use case but AFAICS there is no users in >>> mainline that directly connect a panel using the mediatek driver. As I said my >>> use case is the other so I can't really test. Do you know anyone that can test this? >> >> I'm not sure who can test this, but [1], which is sent by YT Shen in a >> series, is a patch to support dsi command mode so dsi could directly >> connect to panel. >> >> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpu/drm/mediatek?h=v5.7-rc5&id=21898816831fc60c92dd634ab4316a24da7eb4af >> >> It's better that someone could test this case, but if no one would >> test this, I could also accept a good-look patch. >> >> Regards, >> Chun-Kuang. >> >>> >>> Thanks, >>> Enric >>> >>>> >>>> Regards, >>>> Chun-Kuang. >>>> >>>>> >>>>> Thanks, >>>>> Enric >>>>> >>>>>> --- >>>>>> >>>>>> Changes in v4: None >>>>>> Changes in v3: >>>>>> - Move the bridge.type line to the patch that adds drm_bridge support. (Laurent Pinchart) >>>>>> >>>>>> Changes in v2: None >>>>>> >>>>>> drivers/gpu/drm/mediatek/mtk_dsi.c | 13 ++++++++++++- >>>>>> 1 file changed, 12 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c >>>>>> index 4f3bd095c1ee..471fcafdf348 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> >>>>>> @@ -183,6 +184,7 @@ struct mtk_dsi { >>>>>> struct drm_encoder encoder; >>>>>> struct drm_bridge bridge; >>>>>> struct drm_bridge *next_bridge; >>>>>> + struct drm_connector *connector; >>>>>> struct phy *phy; >>>>>> >>>>>> void __iomem *regs; >>>>>> @@ -977,10 +979,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: >>>>>> -- >>>>>> 2.26.2 >>>>>> >>>>>> >>>>>> _______________________________________________ >>>>>> Linux-mediatek mailing list >>>>>> Linux-mediatek@lists.infradead.org >>>>>> http://lists.infradead.org/mailman/listinfo/linux-mediatek >
Hi Enric, On Thu, May 14, 2020 at 07:35:33PM +0200, Enric Balletbo i Serra wrote: > On 14/5/20 19:12, Enric Balletbo i Serra wrote: > > On 14/5/20 18:44, Chun-Kuang Hu wrote: > >> Enric Balletbo i Serra <enric.balletbo@collabora.com> 於 2020年5月14日 週四 下午11:42寫道： > >>> On 14/5/20 16:28, Chun-Kuang Hu wrote: > >>>> Enric Balletbo Serra <eballetbo@gmail.com> 於 2020年5月14日 週四 上午12:41寫道： > >>>>> Missatge de Enric Balletbo i Serra <enric.balletbo@collabora.com> del > >>>>> dia dv., 1 de maig 2020 a les 17:25: > >>>>>> > >>>>>> 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> > >>>>>> Acked-by: Sam Ravnborg <sam@ravnborg.org> > >>>>> > >>>>> A gentle ping on this, I think that this one is the only one that > >>>>> still needs a review in the series. > >>>> > >>>> This is what I reply in patch v3: > >>> > >>> Sorry for missing this. > >>> > >>>> I think the panel is wrapped into next_bridge here, > >>> > >>> Yes, you can have for example: > >>> > >>> 1. drm_bridge (mtk_dsi) -> drm_bridge (ps8640 - dsi-to-edp) -> drm_panel_bridge > >>> (edp panel) > >>> > >>> or a > >>> > >>> 2. drm_bridge (mtk_dsi)-> drm_panel_bridge (dsi panel) > >>> > >>> The _first_ one is my use case > >>> > >>>> if (panel) { > >>> > >>> This handles the second case, where you attach a dsi panel. > >>> > >>>> dsi->next_bridge = devm_drm_panel_bridge_add(dev, panel); > >>>> > >>>> so the next_bridge is a panel_bridge, in its attach function > >>>> panel_bridge_attach(), > >>>> according to the flag DRM_BRIDGE_ATTACH_NO_CONNECTOR, if not exist, > >>>> it would create connector and attach connector to panel. > >>>> > >>>> I'm not sure this flag would exist or not, but for both case, it's strange. > >>>> If exist, you create connector in this patch but no where to attach > >>>> connector to panel. > >>> > >>> Yes, in fact, this is transitional patch needed, as once I converted mtk_dpi, > >>> mtk_dsi and mtk_hdmi to the new drm_bridge API the drm_bridge_connector_init() > >>> will be done in mtk_drm_drv. We will need to call drm_bridge_connector_init for > >>> dpi and dsi pipes and remove that call from mtk_dsi and mtk_dpi drivers. The > >>> graphic controller driver should create connectors and CRTCs, as example you can > >>> take a look at drivers/gpu/drm/omapdrm/omap_drv.c > >> > >> I have such question because I've reviewed omap's driver. In omap's > >> driver, after it call drm_bridge_connector_init(), it does this: > >> > >> if (pipe->output->panel) { > >> ret = drm_panel_attach(pipe->output->panel, > >> pipe->connector); > >> if (ret < 0) > >> return ret; > >> } > >> > >> In this patch, you does not do this. > >> > > > > I see, so yes, I am probably missing call drm_panel_attach in case there is a > > direct panel attached. Thanks for pointing it. > > > > I'll send a new version adding the drm_panel_attach call. > > > > Wait, shouldn't panel be attached on the call of mtk_dsi_bridge_attach as > next_bridge points to a bridge or a panel? > > static int mtk_dsi_bridge_attach(struct drm_bridge *bridge, > enum drm_bridge_attach_flags flags) > { > struct mtk_dsi *dsi = bridge_to_dsi(bridge); > > /* Attach the panel or bridge to the dsi bridge */ > return drm_bridge_attach(bridge->encoder, dsi->next_bridge, > &dsi->bridge, flags); > } > > Or I am continuing misunderstanding all this? Panels should always be wrapped in a drm_bridge, so I think you're doing right. I believe the call to drm_panel_attach() in omapdrm is a leftover that can be removed. I'll have a look at it. > >>>> If not exist, the next_brige would create one connector and this brige > >>>> would create another connector. > >>>> > >>>> I think in your case, mtk_dsi does not directly connect to a panel, so > >>> > >>> Exactly > >>> > >>>> I need a exact explain. Or someone could test this on a > >>>> directly-connect-panel platform. > >>> > >>> I don't think I am breaking this use case but AFAICS there is no users in > >>> mainline that directly connect a panel using the mediatek driver. As I said my > >>> use case is the other so I can't really test. Do you know anyone that can test this? > >> > >> I'm not sure who can test this, but [1], which is sent by YT Shen in a > >> series, is a patch to support dsi command mode so dsi could directly > >> connect to panel. > >> > >> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpu/drm/mediatek?h=v5.7-rc5&id=21898816831fc60c92dd634ab4316a24da7eb4af > >> > >> It's better that someone could test this case, but if no one would > >> test this, I could also accept a good-look patch. > >> > >>>>>> Changes in v4: None > >>>>>> Changes in v3: > >>>>>> - Move the bridge.type line to the patch that adds drm_bridge support. (Laurent Pinchart) > >>>>>> > >>>>>> Changes in v2: None > >>>>>> > >>>>>> drivers/gpu/drm/mediatek/mtk_dsi.c | 13 ++++++++++++- > >>>>>> 1 file changed, 12 insertions(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c > >>>>>> index 4f3bd095c1ee..471fcafdf348 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> > >>>>>> @@ -183,6 +184,7 @@ struct mtk_dsi { > >>>>>> struct drm_encoder encoder; > >>>>>> struct drm_bridge bridge; > >>>>>> struct drm_bridge *next_bridge; > >>>>>> + struct drm_connector *connector; > >>>>>> struct phy *phy; > >>>>>> > >>>>>> void __iomem *regs; > >>>>>> @@ -977,10 +979,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:
Hi, Enric: Enric Balletbo i Serra <enric.balletbo@collabora.com> 於 2020年5月15日 週五 上午1:35寫道： > > Hi again, > > On 14/5/20 19:12, Enric Balletbo i Serra wrote: > > Hi Chun-Kuang, > > > > On 14/5/20 18:44, Chun-Kuang Hu wrote: > >> Hi, Enric: > >> > >> Enric Balletbo i Serra <enric.balletbo@collabora.com> 於 2020年5月14日 週四 下午11:42寫道： > >>> > >>> Hi Chun-Kuang, > >>> > >>> On 14/5/20 16:28, Chun-Kuang Hu wrote: > >>>> Hi, Enric: > >>>> > >>>> Enric Balletbo Serra <eballetbo@gmail.com> 於 2020年5月14日 週四 上午12:41寫道： > >>>>> > >>>>> Hi Chun-Kuang, > >>>>> > >>>>> Missatge de Enric Balletbo i Serra <enric.balletbo@collabora.com> del > >>>>> dia dv., 1 de maig 2020 a les 17:25: > >>>>>> > >>>>>> 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> > >>>>>> Acked-by: Sam Ravnborg <sam@ravnborg.org> > >>>>> > >>>>> A gentle ping on this, I think that this one is the only one that > >>>>> still needs a review in the series. > >>>> > >>>> This is what I reply in patch v3: > >>>> > >>> > >>> Sorry for missing this. > >>> > >>>> I think the panel is wrapped into next_bridge here, > >>>> > >>> > >>> Yes, you can have for example: > >>> > >>> 1. drm_bridge (mtk_dsi) -> drm_bridge (ps8640 - dsi-to-edp) -> drm_panel_bridge > >>> (edp panel) > >>> > >>> or a > >>> > >>> 2. drm_bridge (mtk_dsi)-> drm_panel_bridge (dsi panel) > >>> > >>> The _first_ one is my use case > >>> > >>>> if (panel) { > >>> > >>> This handles the second case, where you attach a dsi panel. > >>> > >>>> dsi->next_bridge = devm_drm_panel_bridge_add(dev, panel); > >>>> > >>>> so the next_bridge is a panel_bridge, in its attach function > >>>> panel_bridge_attach(), > >>>> according to the flag DRM_BRIDGE_ATTACH_NO_CONNECTOR, if not exist, > >>>> it would create connector and attach connector to panel. > >>>> > >>>> I'm not sure this flag would exist or not, but for both case, it's strange. > >>>> If exist, you create connector in this patch but no where to attach > >>>> connector to panel. > >>> > >>> Yes, in fact, this is transitional patch needed, as once I converted mtk_dpi, > >>> mtk_dsi and mtk_hdmi to the new drm_bridge API the drm_bridge_connector_init() > >>> will be done in mtk_drm_drv. We will need to call drm_bridge_connector_init for > >>> dpi and dsi pipes and remove that call from mtk_dsi and mtk_dpi drivers. The > >>> graphic controller driver should create connectors and CRTCs, as example you can > >>> take a look at drivers/gpu/drm/omapdrm/omap_drv.c > >>> > >> > >> I have such question because I've reviewed omap's driver. In omap's > >> driver, after it call drm_bridge_connector_init(), it does this: > >> > >> if (pipe->output->panel) { > >> ret = drm_panel_attach(pipe->output->panel, > >> pipe->connector); > >> if (ret < 0) > >> return ret; > >> } > >> > >> In this patch, you does not do this. > >> > > > > I see, so yes, I am probably missing call drm_panel_attach in case there is a > > direct panel attached. Thanks for pointing it. > > > > I'll send a new version adding the drm_panel_attach call. > > > > Wait, shouldn't panel be attached on the call of mtk_dsi_bridge_attach as > next_bridge points to a bridge or a panel? > > static int mtk_dsi_bridge_attach(struct drm_bridge *bridge, > enum drm_bridge_attach_flags flags) > { > struct mtk_dsi *dsi = bridge_to_dsi(bridge); > > /* Attach the panel or bridge to the dsi bridge */ > return drm_bridge_attach(bridge->encoder, dsi->next_bridge, > &dsi->bridge, flags); > } > > Or I am continuing misunderstanding all this? > My point is: when do you attach panel to a connector? In this patch, ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); it would call into mtk_dsi_bridge_attach() with DRM_BRIDGE_ATTACH_NO_CONNECTOR, and call into panel_bridge_attach() with DRM_BRIDGE_ATTACH_NO_CONNECTOR. If you does not pass DRM_BRIDGE_ATTACH_NO_CONNECTOR into panel_bridge_attach(), it would create a connector and attach panel to that connector. And you pass DRM_BRIDGE_ATTACH_NO_CONNECTOR into panel_bridge_attach(), so I thiink you need to create connector and attach panel to that connector by yourself (this is what omap does). Regards, Chun-Kuang. > >>>> If not exist, the next_brige would create one connector and this brige > >>>> would create another connector. > >>>> > >>>> I think in your case, mtk_dsi does not directly connect to a panel, so > >>> > >>> Exactly > >>> > >>>> I need a exact explain. Or someone could test this on a > >>>> directly-connect-panel platform. > >>> > >>> I don't think I am breaking this use case but AFAICS there is no users in > >>> mainline that directly connect a panel using the mediatek driver. As I said my > >>> use case is the other so I can't really test. Do you know anyone that can test this? > >> > >> I'm not sure who can test this, but [1], which is sent by YT Shen in a > >> series, is a patch to support dsi command mode so dsi could directly > >> connect to panel. > >> > >> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpu/drm/mediatek?h=v5.7-rc5&id=21898816831fc60c92dd634ab4316a24da7eb4af > >> > >> It's better that someone could test this case, but if no one would > >> test this, I could also accept a good-look patch. > >> > >> Regards, > >> Chun-Kuang. > >> > >>> > >>> Thanks, > >>> Enric > >>> > >>>> > >>>> Regards, > >>>> Chun-Kuang. > >>>> > >>>>> > >>>>> Thanks, > >>>>> Enric > >>>>> > >>>>>> --- > >>>>>> > >>>>>> Changes in v4: None > >>>>>> Changes in v3: > >>>>>> - Move the bridge.type line to the patch that adds drm_bridge support. (Laurent Pinchart) > >>>>>> > >>>>>> Changes in v2: None > >>>>>> > >>>>>> drivers/gpu/drm/mediatek/mtk_dsi.c | 13 ++++++++++++- > >>>>>> 1 file changed, 12 insertions(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c > >>>>>> index 4f3bd095c1ee..471fcafdf348 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> > >>>>>> @@ -183,6 +184,7 @@ struct mtk_dsi { > >>>>>> struct drm_encoder encoder; > >>>>>> struct drm_bridge bridge; > >>>>>> struct drm_bridge *next_bridge; > >>>>>> + struct drm_connector *connector; > >>>>>> struct phy *phy; > >>>>>> > >>>>>> void __iomem *regs; > >>>>>> @@ -977,10 +979,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: > >>>>>> -- > >>>>>> 2.26.2 > >>>>>> > >>>>>> > >>>>>> _______________________________________________ > >>>>>> Linux-mediatek mailing list > >>>>>> Linux-mediatek@lists.infradead.org > >>>>>> http://lists.infradead.org/mailman/listinfo/linux-mediatek > >
Hi Chun-Kuang, Missatge de Chun-Kuang Hu <chunkuang.hu@kernel.org> del dia ds., 16 de maig 2020 a les 12:11: > > Hi, Enric: > > Enric Balletbo i Serra <enric.balletbo@collabora.com> 於 2020年5月15日 週五 上午1:35寫道： > > > > Hi again, > > > > On 14/5/20 19:12, Enric Balletbo i Serra wrote: > > > Hi Chun-Kuang, > > > > > > On 14/5/20 18:44, Chun-Kuang Hu wrote: > > >> Hi, Enric: > > >> > > >> Enric Balletbo i Serra <enric.balletbo@collabora.com> 於 2020年5月14日 週四 下午11:42寫道： > > >>> > > >>> Hi Chun-Kuang, > > >>> > > >>> On 14/5/20 16:28, Chun-Kuang Hu wrote: > > >>>> Hi, Enric: > > >>>> > > >>>> Enric Balletbo Serra <eballetbo@gmail.com> 於 2020年5月14日 週四 上午12:41寫道： > > >>>>> > > >>>>> Hi Chun-Kuang, > > >>>>> > > >>>>> Missatge de Enric Balletbo i Serra <enric.balletbo@collabora.com> del > > >>>>> dia dv., 1 de maig 2020 a les 17:25: > > >>>>>> > > >>>>>> 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> > > >>>>>> Acked-by: Sam Ravnborg <sam@ravnborg.org> > > >>>>> > > >>>>> A gentle ping on this, I think that this one is the only one that > > >>>>> still needs a review in the series. > > >>>> > > >>>> This is what I reply in patch v3: > > >>>> > > >>> > > >>> Sorry for missing this. > > >>> > > >>>> I think the panel is wrapped into next_bridge here, > > >>>> > > >>> > > >>> Yes, you can have for example: > > >>> > > >>> 1. drm_bridge (mtk_dsi) -> drm_bridge (ps8640 - dsi-to-edp) -> drm_panel_bridge > > >>> (edp panel) > > >>> > > >>> or a > > >>> > > >>> 2. drm_bridge (mtk_dsi)-> drm_panel_bridge (dsi panel) > > >>> > > >>> The _first_ one is my use case > > >>> > > >>>> if (panel) { > > >>> > > >>> This handles the second case, where you attach a dsi panel. > > >>> > > >>>> dsi->next_bridge = devm_drm_panel_bridge_add(dev, panel); > > >>>> > > >>>> so the next_bridge is a panel_bridge, in its attach function > > >>>> panel_bridge_attach(), > > >>>> according to the flag DRM_BRIDGE_ATTACH_NO_CONNECTOR, if not exist, > > >>>> it would create connector and attach connector to panel. > > >>>> > > >>>> I'm not sure this flag would exist or not, but for both case, it's strange. > > >>>> If exist, you create connector in this patch but no where to attach > > >>>> connector to panel. > > >>> > > >>> Yes, in fact, this is transitional patch needed, as once I converted mtk_dpi, > > >>> mtk_dsi and mtk_hdmi to the new drm_bridge API the drm_bridge_connector_init() > > >>> will be done in mtk_drm_drv. We will need to call drm_bridge_connector_init for > > >>> dpi and dsi pipes and remove that call from mtk_dsi and mtk_dpi drivers. The > > >>> graphic controller driver should create connectors and CRTCs, as example you can > > >>> take a look at drivers/gpu/drm/omapdrm/omap_drv.c > > >>> > > >> > > >> I have such question because I've reviewed omap's driver. In omap's > > >> driver, after it call drm_bridge_connector_init(), it does this: > > >> > > >> if (pipe->output->panel) { > > >> ret = drm_panel_attach(pipe->output->panel, > > >> pipe->connector); > > >> if (ret < 0) > > >> return ret; > > >> } > > >> > > >> In this patch, you does not do this. > > >> > > > > > > I see, so yes, I am probably missing call drm_panel_attach in case there is a > > > direct panel attached. Thanks for pointing it. > > > > > > I'll send a new version adding the drm_panel_attach call. > > > > > > > Wait, shouldn't panel be attached on the call of mtk_dsi_bridge_attach as > > next_bridge points to a bridge or a panel? > > > > static int mtk_dsi_bridge_attach(struct drm_bridge *bridge, > > enum drm_bridge_attach_flags flags) > > { > > struct mtk_dsi *dsi = bridge_to_dsi(bridge); > > > > /* Attach the panel or bridge to the dsi bridge */ > > return drm_bridge_attach(bridge->encoder, dsi->next_bridge, > > &dsi->bridge, flags); > > } > > > > Or I am continuing misunderstanding all this? > > > > My point is: when do you attach panel to a connector? > In this patch, > > ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, > DRM_BRIDGE_ATTACH_NO_CONNECTOR); > > it would call into mtk_dsi_bridge_attach() with > DRM_BRIDGE_ATTACH_NO_CONNECTOR, and call into panel_bridge_attach() > with DRM_BRIDGE_ATTACH_NO_CONNECTOR. My understanding is that the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is to ease transition between the old and the new model. The drivers that support the new model shall set that flag. > If you does not pass DRM_BRIDGE_ATTACH_NO_CONNECTOR into > panel_bridge_attach(), it would create a connector and attach panel to > that connector. Yes, and create a connector an attach the panel is the old model, I guess. Hence we are passing DRM_BRIDGE_ATTACH_NO_CONNECTOR. > And you pass DRM_BRIDGE_ATTACH_NO_CONNECTOR into > panel_bridge_attach(), so I thiink you need to create connector and > attach panel to that connector by yourself (this is what omap does). > Yes, omap driver supports both models the old and the new. For mtk_dsi I expect all the drivers in the chain use the new model. IIUC create the connector here and attach to the panel is only needed to support also the old model. drm_panel_attach() is called in panel_bridge_attach() which is the drm_brige_funcs.attach() handler for the panel bridge. As we're using the panel bridge dsi->next_bridge = devm_drm_panel_bridge_add(dev, panel); there should be no need to call drm_panel_attach(). My point is, do we need to support the old model for mtk_dsi driver even we don't have users for the old model, or can we migrate unconditionally? Please correct me if I'm still misunderstanding the problem. Regards, Enric > Regards, > Chun-Kuang. > > > >>>> If not exist, the next_brige would create one connector and this brige > > >>>> would create another connector. > > >>>> > > >>>> I think in your case, mtk_dsi does not directly connect to a panel, so > > >>> > > >>> Exactly > > >>> > > >>>> I need a exact explain. Or someone could test this on a > > >>>> directly-connect-panel platform. > > >>> > > >>> I don't think I am breaking this use case but AFAICS there is no users in > > >>> mainline that directly connect a panel using the mediatek driver. As I said my > > >>> use case is the other so I can't really test. Do you know anyone that can test this? > > >> > > >> I'm not sure who can test this, but [1], which is sent by YT Shen in a > > >> series, is a patch to support dsi command mode so dsi could directly > > >> connect to panel. > > >> > > >> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpu/drm/mediatek?h=v5.7-rc5&id=21898816831fc60c92dd634ab4316a24da7eb4af > > >> > > >> It's better that someone could test this case, but if no one would > > >> test this, I could also accept a good-look patch. > > >> > > >> Regards, > > >> Chun-Kuang. > > >> > > >>> > > >>> Thanks, > > >>> Enric > > >>> > > >>>> > > >>>> Regards, > > >>>> Chun-Kuang. > > >>>> > > >>>>> > > >>>>> Thanks, > > >>>>> Enric > > >>>>> > > >>>>>> --- > > >>>>>> > > >>>>>> Changes in v4: None > > >>>>>> Changes in v3: > > >>>>>> - Move the bridge.type line to the patch that adds drm_bridge support. (Laurent Pinchart) > > >>>>>> > > >>>>>> Changes in v2: None > > >>>>>> > > >>>>>> drivers/gpu/drm/mediatek/mtk_dsi.c | 13 ++++++++++++- > > >>>>>> 1 file changed, 12 insertions(+), 1 deletion(-) > > >>>>>> > > >>>>>> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c > > >>>>>> index 4f3bd095c1ee..471fcafdf348 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> > > >>>>>> @@ -183,6 +184,7 @@ struct mtk_dsi { > > >>>>>> struct drm_encoder encoder; > > >>>>>> struct drm_bridge bridge; > > >>>>>> struct drm_bridge *next_bridge; > > >>>>>> + struct drm_connector *connector; > > >>>>>> struct phy *phy; > > >>>>>> > > >>>>>> void __iomem *regs; > > >>>>>> @@ -977,10 +979,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: > > >>>>>> -- > > >>>>>> 2.26.2 > > >>>>>> > > >>>>>> > > >>>>>> _______________________________________________ > > >>>>>> Linux-mediatek mailing list > > >>>>>> Linux-mediatek@lists.infradead.org > > >>>>>> http://lists.infradead.org/mailman/listinfo/linux-mediatek > > >
Hi Enric/Chun-Kuang. > > > > My point is: when do you attach panel to a connector? > > In this patch, > > > > ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, > > DRM_BRIDGE_ATTACH_NO_CONNECTOR); > > > > it would call into mtk_dsi_bridge_attach() with > > DRM_BRIDGE_ATTACH_NO_CONNECTOR, and call into panel_bridge_attach() > > with DRM_BRIDGE_ATTACH_NO_CONNECTOR. > > My understanding is that the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is to > ease transition between the old and the new model. The drivers that > support the new model shall set that flag. Yes, right now we have fous on migrating all bridge drivers to the new model and next step is to make the transition for the display drivers one by one. Display drivers that uses the old model rely on the bridge driver to create the connector, whereas display drivers using the new model will create the connector themself. Display drivers following the new model will pass DRM_BRIDGE_ATTACH_NO_CONNECTOR to tell the bridge drive that no connector shall be created by the bridge driver. For this driver where only the new model is needed there is no reason to try to support both models. So the display driver shall always create the connector, and never ask the bridge driver to do it (always pass DRM_BRIDGE_ATTACH_NO_CONNECTOR). I hope this confirm and clarifies it. Sam
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index 4f3bd095c1ee..471fcafdf348 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> @@ -183,6 +184,7 @@ struct mtk_dsi { struct drm_encoder encoder; struct drm_bridge bridge; struct drm_bridge *next_bridge; + struct drm_connector *connector; struct phy *phy; void __iomem *regs; @@ -977,10 +979,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: