The PS8640 dsi-to-eDP bridge driver is using the panel bridge API, however, not all the components in the chain have been ported to the drm_bridge API. Actually, when a panel is attached the default panel's mode is used, but in some cases we can't get display up if mode getting from eDP control EDID is not chosen.
This series address that problem, first implements the .get_edid() callback in the PS8640 driver (which is not used until the conversion is done) and then, converts the Mediatek DSI driver to use the drm_bridge API.
As far as I know, we're the only users of the mediatek dsi driver in mainline, so should be safe to switch to the new chain of drm_bridge API unconditionally.
The patches has been tested on a Acer Chromebook R13 (Elm) running a Chrome OS userspace and checking that the valid EDID mode reported by the bridge is selected.
Enric Balletbo i Serra (7): drm/bridge: ps8640: Get the EDID from eDP control drm/bridge_connector: Set default status connected for eDP connectors drm/mediatek: mtk_dsi: Rename bridge to next_bridge drm/mediatek: mtk_dsi: Convert to bridge driver drm/mediatek: mtk_dsi: Use simple encoder drm/mediatek: mtk_dsi: Use the drm_panel_bridge API drm/mediatek: mtk_dsi: Create connector for bridges
drivers/gpu/drm/bridge/parade-ps8640.c | 12 ++ drivers/gpu/drm/drm_bridge_connector.c | 1 + drivers/gpu/drm/mediatek/mtk_dsi.c | 277 ++++++++----------------- 3 files changed, 97 insertions(+), 193 deletions(-)
The PS8640 DSI-to-eDP bridge can retrieve the EDID, so implement the .get_edid callback and set the flag to indicate the core to use it.
Signed-off-by: Enric Balletbo i Serra enric.balletbo@collabora.com Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Acked-by: Sam Ravnborg sam@ravnborg.org ---
Changes in v4: None Changes in v3: None Changes in v2: None
drivers/gpu/drm/bridge/parade-ps8640.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c index 4b099196afeb..13755d278db6 100644 --- a/drivers/gpu/drm/bridge/parade-ps8640.c +++ b/drivers/gpu/drm/bridge/parade-ps8640.c @@ -242,8 +242,18 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge, return ret; }
+static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge, + struct drm_connector *connector) +{ + struct ps8640 *ps_bridge = bridge_to_ps8640(bridge); + + return drm_get_edid(connector, + ps_bridge->page[PAGE0_DP_CNTL]->adapter); +} + static const struct drm_bridge_funcs ps8640_bridge_funcs = { .attach = ps8640_bridge_attach, + .get_edid = ps8640_bridge_get_edid, .post_disable = ps8640_post_disable, .pre_enable = ps8640_pre_enable, }; @@ -294,6 +304,8 @@ static int ps8640_probe(struct i2c_client *client)
ps_bridge->bridge.funcs = &ps8640_bridge_funcs; ps_bridge->bridge.of_node = dev->of_node; + ps_bridge->bridge.ops = DRM_BRIDGE_OP_EDID; + ps_bridge->bridge.type = DRM_MODE_CONNECTOR_eDP;
ps_bridge->page[PAGE0_DP_CNTL] = client;
In an eDP application, HPD is not required and on most bridge chips useless. If HPD is not used, we need to set initial status as connected, otherwise the connector created by the drm_bridge_connector API remains in an unknown state.
Signed-off-by: Enric Balletbo i Serra enric.balletbo@collabora.com Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Acked-by: Sam Ravnborg sam@ravnborg.org ---
Changes in v4: None Changes in v3: None Changes in v2: None
drivers/gpu/drm/drm_bridge_connector.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c index c6994fe673f3..a58cbde59c34 100644 --- a/drivers/gpu/drm/drm_bridge_connector.c +++ b/drivers/gpu/drm/drm_bridge_connector.c @@ -187,6 +187,7 @@ drm_bridge_connector_detect(struct drm_connector *connector, bool force) case DRM_MODE_CONNECTOR_DPI: case DRM_MODE_CONNECTOR_LVDS: case DRM_MODE_CONNECTOR_DSI: + case DRM_MODE_CONNECTOR_eDP: status = connector_status_connected; break; default:
This is really a cosmetic change just to make a bit more readable the code after convert the driver to drm_bridge. The bridge variable name will be used by the encoder drm_bridge, and the chained bridge will be named next_bridge.
Signed-off-by: Enric Balletbo i Serra enric.balletbo@collabora.com Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Acked-by: Sam Ravnborg sam@ravnborg.org ---
Changes in v4: None Changes in v3: - Replace s/bridge/next bridge/ for comment. (Laurent Pinchart)
Changes in v2: None
drivers/gpu/drm/mediatek/mtk_dsi.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index cfa45d6abd74..37b8d7ffd835 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -182,7 +182,7 @@ struct mtk_dsi { struct drm_encoder encoder; struct drm_connector conn; struct drm_panel *panel; - struct drm_bridge *bridge; + struct drm_bridge *next_bridge; struct phy *phy;
void __iomem *regs; @@ -902,9 +902,10 @@ static int mtk_dsi_create_conn_enc(struct drm_device *drm, struct mtk_dsi *dsi) */ dsi->encoder.possible_crtcs = 1;
- /* If there's a bridge, attach to it and let it create the connector */ - if (dsi->bridge) { - ret = drm_bridge_attach(&dsi->encoder, dsi->bridge, NULL, 0); + /* If there's a next bridge, attach to it and let it create the connector */ + if (dsi->next_bridge) { + ret = drm_bridge_attach(&dsi->encoder, dsi->next_bridge, NULL, + 0); if (ret) { DRM_ERROR("Failed to attach bridge to drm\n"); goto err_encoder_cleanup; @@ -1185,7 +1186,7 @@ static int mtk_dsi_probe(struct platform_device *pdev) }
ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, - &dsi->panel, &dsi->bridge); + &dsi->panel, &dsi->next_bridge); if (ret) goto err_unregister_host;
Hi, Enric:
Enric Balletbo i Serra enric.balletbo@collabora.com 於 2020年5月1日 週五 下午11:23寫道：
This is really a cosmetic change just to make a bit more readable the code after convert the driver to drm_bridge. The bridge variable name will be used by the encoder drm_bridge, and the chained bridge will be named next_bridge.
Reviewed-by: Chun-Kuang Hu chunkuang.hu@kernel.org
Signed-off-by: Enric Balletbo i Serra enric.balletbo@collabora.com Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Acked-by: Sam Ravnborg sam@ravnborg.org
Changes in v4: None Changes in v3:
- Replace s/bridge/next bridge/ for comment. (Laurent Pinchart)
Changes in v2: None
drivers/gpu/drm/mediatek/mtk_dsi.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index cfa45d6abd74..37b8d7ffd835 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -182,7 +182,7 @@ struct mtk_dsi { struct drm_encoder encoder; struct drm_connector conn; struct drm_panel *panel;
struct drm_bridge *bridge;
struct drm_bridge *next_bridge; struct phy *phy; void __iomem *regs;@@ -902,9 +902,10 @@ static int mtk_dsi_create_conn_enc(struct drm_device *drm, struct mtk_dsi *dsi) */ dsi->encoder.possible_crtcs = 1;
/* If there's a bridge, attach to it and let it create the connector */if (dsi->bridge) {ret = drm_bridge_attach(&dsi->encoder, dsi->bridge, NULL, 0);
/* If there's a next bridge, attach to it and let it create the connector */if (dsi->next_bridge) {ret = drm_bridge_attach(&dsi->encoder, dsi->next_bridge, NULL,0); if (ret) { DRM_ERROR("Failed to attach bridge to drm\n"); goto err_encoder_cleanup;@@ -1185,7 +1186,7 @@ static int mtk_dsi_probe(struct platform_device *pdev) }
ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
&dsi->panel, &dsi->bridge);
&dsi->panel, &dsi->next_bridge); if (ret) goto err_unregister_host;-- 2.26.2
Convert mtk_dsi to a bridge driver with built-in encoder support for compatibility with existing component drivers.
Signed-off-by: Enric Balletbo i Serra enric.balletbo@collabora.com Acked-by: Sam Ravnborg sam@ravnborg.org ---
Changes in v4: - Remove double call to drm_encoder_init(). (Chun-Kuang Hu) - Cleanup the encoder in mtk_dsi_unbind(). (Chun-Kuang Hu)
Changes in v3: - Add the bridge.type. (Laurent Pinchart)
Changes in v2: None
drivers/gpu/drm/mediatek/mtk_dsi.c | 108 +++++++++++++++++++---------- 1 file changed, 70 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index 37b8d7ffd835..38cbdcb15fff 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -180,6 +180,7 @@ struct mtk_dsi { struct device *dev; struct mipi_dsi_host host; struct drm_encoder encoder; + struct drm_bridge bridge; struct drm_connector conn; struct drm_panel *panel; struct drm_bridge *next_bridge; @@ -205,9 +206,9 @@ struct mtk_dsi { const struct mtk_dsi_driver_data *driver_data; };
-static inline struct mtk_dsi *encoder_to_dsi(struct drm_encoder *e) +static inline struct mtk_dsi *bridge_to_dsi(struct drm_bridge *b) { - return container_of(e, struct mtk_dsi, encoder); + return container_of(b, struct mtk_dsi, bridge); }
static inline struct mtk_dsi *connector_to_dsi(struct drm_connector *c) @@ -796,32 +797,43 @@ static const struct drm_encoder_funcs mtk_dsi_encoder_funcs = { .destroy = mtk_dsi_encoder_destroy, };
-static bool mtk_dsi_encoder_mode_fixup(struct drm_encoder *encoder, - const struct drm_display_mode *mode, - struct drm_display_mode *adjusted_mode) +static int mtk_dsi_create_conn_enc(struct drm_device *drm, struct mtk_dsi *dsi); +static void mtk_dsi_destroy_conn_enc(struct mtk_dsi *dsi); + +static int mtk_dsi_bridge_attach(struct drm_bridge *bridge, + enum drm_bridge_attach_flags flags) { - return true; + struct mtk_dsi *dsi = bridge_to_dsi(bridge); + + return mtk_dsi_create_conn_enc(bridge->dev, dsi); +} + +static void mtk_dsi_bridge_detach(struct drm_bridge *bridge) +{ + struct mtk_dsi *dsi = bridge_to_dsi(bridge); + + mtk_dsi_destroy_conn_enc(dsi); }
-static void mtk_dsi_encoder_mode_set(struct drm_encoder *encoder, - struct drm_display_mode *mode, - struct drm_display_mode *adjusted) +static void mtk_dsi_bridge_mode_set(struct drm_bridge *bridge, + const struct drm_display_mode *mode, + const struct drm_display_mode *adjusted) { - struct mtk_dsi *dsi = encoder_to_dsi(encoder); + struct mtk_dsi *dsi = bridge_to_dsi(bridge);
drm_display_mode_to_videomode(adjusted, &dsi->vm); }
-static void mtk_dsi_encoder_disable(struct drm_encoder *encoder) +static void mtk_dsi_bridge_disable(struct drm_bridge *bridge) { - struct mtk_dsi *dsi = encoder_to_dsi(encoder); + struct mtk_dsi *dsi = bridge_to_dsi(bridge);
mtk_output_dsi_disable(dsi); }
-static void mtk_dsi_encoder_enable(struct drm_encoder *encoder) +static void mtk_dsi_bridge_enable(struct drm_bridge *bridge) { - struct mtk_dsi *dsi = encoder_to_dsi(encoder); + struct mtk_dsi *dsi = bridge_to_dsi(bridge);
mtk_output_dsi_enable(dsi); } @@ -833,11 +845,12 @@ static int mtk_dsi_connector_get_modes(struct drm_connector *connector) return drm_panel_get_modes(dsi->panel, connector); }
-static const struct drm_encoder_helper_funcs mtk_dsi_encoder_helper_funcs = { - .mode_fixup = mtk_dsi_encoder_mode_fixup, - .mode_set = mtk_dsi_encoder_mode_set, - .disable = mtk_dsi_encoder_disable, - .enable = mtk_dsi_encoder_enable, +static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = { + .attach = mtk_dsi_bridge_attach, + .detach = mtk_dsi_bridge_detach, + .disable = mtk_dsi_bridge_disable, + .enable = mtk_dsi_bridge_enable, + .mode_set = mtk_dsi_bridge_mode_set, };
static const struct drm_connector_funcs mtk_dsi_connector_funcs = { @@ -888,20 +901,6 @@ static int mtk_dsi_create_conn_enc(struct drm_device *drm, struct mtk_dsi *dsi) { int ret;
- ret = drm_encoder_init(drm, &dsi->encoder, &mtk_dsi_encoder_funcs, - DRM_MODE_ENCODER_DSI, NULL); - if (ret) { - DRM_ERROR("Failed to encoder init to drm\n"); - return ret; - } - drm_encoder_helper_add(&dsi->encoder, &mtk_dsi_encoder_helper_funcs); - - /* - * Currently display data paths are statically assigned to a crtc each. - * crtc 0 is OVL0 -> COLOR0 -> AAL -> OD -> RDMA0 -> UFOE -> DSI0 - */ - dsi->encoder.possible_crtcs = 1; - /* If there's a next bridge, attach to it and let it create the connector */ if (dsi->next_bridge) { ret = drm_bridge_attach(&dsi->encoder, dsi->next_bridge, NULL, @@ -1123,6 +1122,34 @@ static const struct mipi_dsi_host_ops mtk_dsi_ops = { .transfer = mtk_dsi_host_transfer, };
+static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi) +{ + int ret; + + ret = drm_encoder_init(drm, &dsi->encoder, &mtk_dsi_encoder_funcs, + DRM_MODE_ENCODER_DSI, NULL); + if (ret) { + DRM_ERROR("Failed to encoder init to drm\n"); + return ret; + } + + /* + * Currently display data paths are statically assigned to a crtc each. + * crtc 0 is OVL0 -> COLOR0 -> AAL -> OD -> RDMA0 -> UFOE -> DSI0 + */ + dsi->encoder.possible_crtcs = 1; + + ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, 0); + if (ret) + goto err_cleanup_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; @@ -1136,11 +1163,9 @@ static int mtk_dsi_bind(struct device *dev, struct device *master, void *data) return ret; }
- ret = mtk_dsi_create_conn_enc(drm, dsi); - if (ret) { - DRM_ERROR("Encoder create failed with %d\n", ret); + ret = mtk_dsi_encoder_init(drm, dsi); + if (ret) goto err_unregister; - }
return 0;
@@ -1155,7 +1180,7 @@ static void mtk_dsi_unbind(struct device *dev, struct device *master, struct drm_device *drm = data; struct mtk_dsi *dsi = dev_get_drvdata(dev);
- mtk_dsi_destroy_conn_enc(dsi); + drm_encoder_cleanup(&dsi->encoder); mtk_ddp_comp_unregister(drm, &dsi->ddp_comp); }
@@ -1265,6 +1290,12 @@ static int mtk_dsi_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, dsi);
+ 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); + ret = component_add(&pdev->dev, &mtk_dsi_component_ops); if (ret) { dev_err(&pdev->dev, "failed to add component: %d\n", ret); @@ -1283,6 +1314,7 @@ 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);
Hi, Enric:
Enric Balletbo i Serra enric.balletbo@collabora.com 於 2020年5月1日 週五 下午11:23寫道：
Convert mtk_dsi to a bridge driver with built-in encoder support for compatibility with existing component drivers.
Reviewed-by: Chun-Kuang Hu chunkuang.hu@kernel.org
Signed-off-by: Enric Balletbo i Serra enric.balletbo@collabora.com Acked-by: Sam Ravnborg sam@ravnborg.org
Changes in v4:
- Remove double call to drm_encoder_init(). (Chun-Kuang Hu)
- Cleanup the encoder in mtk_dsi_unbind(). (Chun-Kuang Hu)
Changes in v3:
- Add the bridge.type. (Laurent Pinchart)
Changes in v2: None
drivers/gpu/drm/mediatek/mtk_dsi.c | 108 +++++++++++++++++++---------- 1 file changed, 70 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index 37b8d7ffd835..38cbdcb15fff 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -180,6 +180,7 @@ struct mtk_dsi { struct device *dev; struct mipi_dsi_host host; struct drm_encoder encoder;
struct drm_bridge bridge; struct drm_connector conn; struct drm_panel *panel; struct drm_bridge *next_bridge;@@ -205,9 +206,9 @@ struct mtk_dsi { const struct mtk_dsi_driver_data *driver_data; };
-static inline struct mtk_dsi *encoder_to_dsi(struct drm_encoder *e) +static inline struct mtk_dsi *bridge_to_dsi(struct drm_bridge *b) {
return container_of(e, struct mtk_dsi, encoder);
return container_of(b, struct mtk_dsi, bridge);}
static inline struct mtk_dsi *connector_to_dsi(struct drm_connector *c) @@ -796,32 +797,43 @@ static const struct drm_encoder_funcs mtk_dsi_encoder_funcs = { .destroy = mtk_dsi_encoder_destroy, };
-static bool mtk_dsi_encoder_mode_fixup(struct drm_encoder *encoder,
const struct drm_display_mode *mode,struct drm_display_mode *adjusted_mode)+static int mtk_dsi_create_conn_enc(struct drm_device *drm, struct mtk_dsi *dsi); +static void mtk_dsi_destroy_conn_enc(struct mtk_dsi *dsi);
+static int mtk_dsi_bridge_attach(struct drm_bridge *bridge,
enum drm_bridge_attach_flags flags){
return true;
struct mtk_dsi *dsi = bridge_to_dsi(bridge);return mtk_dsi_create_conn_enc(bridge->dev, dsi);+}
+static void mtk_dsi_bridge_detach(struct drm_bridge *bridge) +{
struct mtk_dsi *dsi = bridge_to_dsi(bridge);mtk_dsi_destroy_conn_enc(dsi);}
-static void mtk_dsi_encoder_mode_set(struct drm_encoder *encoder,
struct drm_display_mode *mode,struct drm_display_mode *adjusted)+static void mtk_dsi_bridge_mode_set(struct drm_bridge *bridge,
const struct drm_display_mode *mode,const struct drm_display_mode *adjusted){
struct mtk_dsi *dsi = encoder_to_dsi(encoder);
struct mtk_dsi *dsi = bridge_to_dsi(bridge); drm_display_mode_to_videomode(adjusted, &dsi->vm);}
-static void mtk_dsi_encoder_disable(struct drm_encoder *encoder) +static void mtk_dsi_bridge_disable(struct drm_bridge *bridge) {
struct mtk_dsi *dsi = encoder_to_dsi(encoder);
struct mtk_dsi *dsi = bridge_to_dsi(bridge); mtk_output_dsi_disable(dsi);}
-static void mtk_dsi_encoder_enable(struct drm_encoder *encoder) +static void mtk_dsi_bridge_enable(struct drm_bridge *bridge) {
struct mtk_dsi *dsi = encoder_to_dsi(encoder);
struct mtk_dsi *dsi = bridge_to_dsi(bridge); mtk_output_dsi_enable(dsi);} @@ -833,11 +845,12 @@ static int mtk_dsi_connector_get_modes(struct drm_connector *connector) return drm_panel_get_modes(dsi->panel, connector); }
-static const struct drm_encoder_helper_funcs mtk_dsi_encoder_helper_funcs = {
.mode_fixup = mtk_dsi_encoder_mode_fixup,.mode_set = mtk_dsi_encoder_mode_set,.disable = mtk_dsi_encoder_disable,.enable = mtk_dsi_encoder_enable,+static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = {
.attach = mtk_dsi_bridge_attach,.detach = mtk_dsi_bridge_detach,.disable = mtk_dsi_bridge_disable,.enable = mtk_dsi_bridge_enable,.mode_set = mtk_dsi_bridge_mode_set,};
static const struct drm_connector_funcs mtk_dsi_connector_funcs = { @@ -888,20 +901,6 @@ static int mtk_dsi_create_conn_enc(struct drm_device *drm, struct mtk_dsi *dsi) { int ret;
ret = drm_encoder_init(drm, &dsi->encoder, &mtk_dsi_encoder_funcs,DRM_MODE_ENCODER_DSI, NULL);if (ret) {DRM_ERROR("Failed to encoder init to drm\n");return ret;}drm_encoder_helper_add(&dsi->encoder, &mtk_dsi_encoder_helper_funcs);/** Currently display data paths are statically assigned to a crtc each.* crtc 0 is OVL0 -> COLOR0 -> AAL -> OD -> RDMA0 -> UFOE -> DSI0*/dsi->encoder.possible_crtcs = 1;/* If there's a next bridge, attach to it and let it create the connector */ if (dsi->next_bridge) { ret = drm_bridge_attach(&dsi->encoder, dsi->next_bridge, NULL,@@ -1123,6 +1122,34 @@ static const struct mipi_dsi_host_ops mtk_dsi_ops = { .transfer = mtk_dsi_host_transfer, };
+static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi) +{
int ret;ret = drm_encoder_init(drm, &dsi->encoder, &mtk_dsi_encoder_funcs,DRM_MODE_ENCODER_DSI, NULL);if (ret) {DRM_ERROR("Failed to encoder init to drm\n");return ret;}/** Currently display data paths are statically assigned to a crtc each.* crtc 0 is OVL0 -> COLOR0 -> AAL -> OD -> RDMA0 -> UFOE -> DSI0*/dsi->encoder.possible_crtcs = 1;ret = drm_bridge_attach(&dsi->encoder, &dsi->bridge, NULL, 0);if (ret)goto err_cleanup_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; @@ -1136,11 +1163,9 @@ static int mtk_dsi_bind(struct device *dev, struct device *master, void *data) return ret; }
ret = mtk_dsi_create_conn_enc(drm, dsi);if (ret) {DRM_ERROR("Encoder create failed with %d\n", ret);
ret = mtk_dsi_encoder_init(drm, dsi);if (ret) goto err_unregister;
} return 0;@@ -1155,7 +1180,7 @@ static void mtk_dsi_unbind(struct device *dev, struct device *master, struct drm_device *drm = data; struct mtk_dsi *dsi = dev_get_drvdata(dev);
mtk_dsi_destroy_conn_enc(dsi);
drm_encoder_cleanup(&dsi->encoder); mtk_ddp_comp_unregister(drm, &dsi->ddp_comp);}
@@ -1265,6 +1290,12 @@ static int mtk_dsi_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, dsi);
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);ret = component_add(&pdev->dev, &mtk_dsi_component_ops); if (ret) { dev_err(&pdev->dev, "failed to add component: %d\n", ret);@@ -1283,6 +1314,7 @@ 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);-- 2.26.2
The mtk_dsi driver uses an empty implementation for its encoder. Replace the code with the generic simple encoder.
Signed-off-by: Enric Balletbo i Serra enric.balletbo@collabora.com Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Acked-by: Sam Ravnborg sam@ravnborg.org Reviewed-by: Chun-Kuang Hu chunkuang.hu@kernel.org ---
Changes in v4: None Changes in v3: None Changes in v2: None
drivers/gpu/drm/mediatek/mtk_dsi.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index 38cbdcb15fff..e02d16a086ac 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -22,6 +22,7 @@ #include <drm/drm_panel.h> #include <drm/drm_print.h> #include <drm/drm_probe_helper.h> +#include <drm/drm_simple_kms_helper.h>
#include "mtk_drm_ddp_comp.h"
@@ -788,15 +789,6 @@ static void mtk_output_dsi_disable(struct mtk_dsi *dsi) dsi->enabled = false; }
-static void mtk_dsi_encoder_destroy(struct drm_encoder *encoder) -{ - drm_encoder_cleanup(encoder); -} - -static const struct drm_encoder_funcs mtk_dsi_encoder_funcs = { - .destroy = mtk_dsi_encoder_destroy, -}; - static int mtk_dsi_create_conn_enc(struct drm_device *drm, struct mtk_dsi *dsi); static void mtk_dsi_destroy_conn_enc(struct mtk_dsi *dsi);
@@ -1126,8 +1118,8 @@ static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi) { int ret;
- ret = drm_encoder_init(drm, &dsi->encoder, &mtk_dsi_encoder_funcs, - DRM_MODE_ENCODER_DSI, NULL); + 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;
Replace the manual panel handling code by a drm_panel_bridge. This simplifies the driver and allows all components in the display pipeline to be treated as bridges, paving the way to generic connector handling.
Signed-off-by: Enric Balletbo i Serra enric.balletbo@collabora.com Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Acked-by: Sam Ravnborg sam@ravnborg.org Reviewed-by: Chun-Kuang Hu chunkuang.hu@kernel.org ---
Changes in v4: None Changes in v3: - Use next_bridge field to store the panel bridge. (Laurent Pinchart) - Add the bridge.type field. (Laurent Pinchart) - This patch requires https://lkml.org/lkml/2020/4/16/2080 to work properly.
Changes in v2: - Do not set connector_type for panel here. (Sam Ravnborg)
drivers/gpu/drm/mediatek/mtk_dsi.c | 173 +++-------------------------- 1 file changed, 14 insertions(+), 159 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c index e02d16a086ac..4f3bd095c1ee 100644 --- a/drivers/gpu/drm/mediatek/mtk_dsi.c +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c @@ -182,8 +182,6 @@ struct mtk_dsi { struct mipi_dsi_host host; struct drm_encoder encoder; struct drm_bridge bridge; - struct drm_connector conn; - struct drm_panel *panel; struct drm_bridge *next_bridge; struct phy *phy;
@@ -212,11 +210,6 @@ static inline struct mtk_dsi *bridge_to_dsi(struct drm_bridge *b) return container_of(b, struct mtk_dsi, bridge); }
-static inline struct mtk_dsi *connector_to_dsi(struct drm_connector *c) -{ - return container_of(c, struct mtk_dsi, conn); -} - static inline struct mtk_dsi *host_to_dsi(struct mipi_dsi_host *h) { return container_of(h, struct mtk_dsi, host); @@ -682,16 +675,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi) mtk_dsi_lane0_ulp_mode_leave(dsi); mtk_dsi_clk_hs_mode(dsi, 0);
- if (dsi->panel) { - if (drm_panel_prepare(dsi->panel)) { - DRM_ERROR("failed to prepare the panel\n"); - goto err_disable_digital_clk; - } - } - return 0; -err_disable_digital_clk: - clk_disable_unprepare(dsi->digital_clk); err_disable_engine_clk: clk_disable_unprepare(dsi->engine_clk); err_phy_power_off: @@ -718,15 +702,7 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi) */ mtk_dsi_stop(dsi);
- if (!mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500)) { - if (dsi->panel) { - if (drm_panel_unprepare(dsi->panel)) { - DRM_ERROR("failed to unprepare the panel\n"); - return; - } - } - } - + mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500); mtk_dsi_reset_engine(dsi); mtk_dsi_lane0_ulp_mode_enter(dsi); mtk_dsi_clk_ulp_mode_enter(dsi); @@ -757,19 +733,7 @@ static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
mtk_dsi_start(dsi);
- if (dsi->panel) { - if (drm_panel_enable(dsi->panel)) { - DRM_ERROR("failed to enable the panel\n"); - goto err_dsi_power_off; - } - } - dsi->enabled = true; - - return; -err_dsi_power_off: - mtk_dsi_stop(dsi); - mtk_dsi_poweroff(dsi); }
static void mtk_output_dsi_disable(struct mtk_dsi *dsi) @@ -777,34 +741,19 @@ static void mtk_output_dsi_disable(struct mtk_dsi *dsi) if (!dsi->enabled) return;
- if (dsi->panel) { - if (drm_panel_disable(dsi->panel)) { - DRM_ERROR("failed to disable the panel\n"); - return; - } - } - mtk_dsi_poweroff(dsi);
dsi->enabled = false; }
-static int mtk_dsi_create_conn_enc(struct drm_device *drm, struct mtk_dsi *dsi); -static void mtk_dsi_destroy_conn_enc(struct mtk_dsi *dsi); - static int mtk_dsi_bridge_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) { struct mtk_dsi *dsi = bridge_to_dsi(bridge);
- return mtk_dsi_create_conn_enc(bridge->dev, dsi); -} - -static void mtk_dsi_bridge_detach(struct drm_bridge *bridge) -{ - struct mtk_dsi *dsi = bridge_to_dsi(bridge); - - mtk_dsi_destroy_conn_enc(dsi); + /* Attach the panel or bridge to the dsi bridge */ + return drm_bridge_attach(bridge->encoder, dsi->next_bridge, + &dsi->bridge, flags); }
static void mtk_dsi_bridge_mode_set(struct drm_bridge *bridge, @@ -830,101 +779,13 @@ static void mtk_dsi_bridge_enable(struct drm_bridge *bridge) mtk_output_dsi_enable(dsi); }
-static int mtk_dsi_connector_get_modes(struct drm_connector *connector) -{ - struct mtk_dsi *dsi = connector_to_dsi(connector); - - return drm_panel_get_modes(dsi->panel, connector); -} - static const struct drm_bridge_funcs mtk_dsi_bridge_funcs = { .attach = mtk_dsi_bridge_attach, - .detach = mtk_dsi_bridge_detach, .disable = mtk_dsi_bridge_disable, .enable = mtk_dsi_bridge_enable, .mode_set = mtk_dsi_bridge_mode_set, };
-static const struct drm_connector_funcs mtk_dsi_connector_funcs = { - .fill_modes = drm_helper_probe_single_connector_modes, - .destroy = drm_connector_cleanup, - .reset = drm_atomic_helper_connector_reset, - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, -}; - -static const struct drm_connector_helper_funcs - mtk_dsi_connector_helper_funcs = { - .get_modes = mtk_dsi_connector_get_modes, -}; - -static int mtk_dsi_create_connector(struct drm_device *drm, struct mtk_dsi *dsi) -{ - int ret; - - ret = drm_connector_init(drm, &dsi->conn, &mtk_dsi_connector_funcs, - DRM_MODE_CONNECTOR_DSI); - if (ret) { - DRM_ERROR("Failed to connector init to drm\n"); - return ret; - } - - drm_connector_helper_add(&dsi->conn, &mtk_dsi_connector_helper_funcs); - - dsi->conn.dpms = DRM_MODE_DPMS_OFF; - drm_connector_attach_encoder(&dsi->conn, &dsi->encoder); - - if (dsi->panel) { - ret = drm_panel_attach(dsi->panel, &dsi->conn); - if (ret) { - DRM_ERROR("Failed to attach panel to drm\n"); - goto err_connector_cleanup; - } - } - - return 0; - -err_connector_cleanup: - drm_connector_cleanup(&dsi->conn); - return ret; -} - -static int mtk_dsi_create_conn_enc(struct drm_device *drm, struct mtk_dsi *dsi) -{ - int ret; - - /* If there's a next bridge, attach to it and let it create the connector */ - if (dsi->next_bridge) { - ret = drm_bridge_attach(&dsi->encoder, dsi->next_bridge, NULL, - 0); - if (ret) { - DRM_ERROR("Failed to attach bridge to drm\n"); - goto err_encoder_cleanup; - } - } else { - /* Otherwise create our own connector and attach to a panel */ - ret = mtk_dsi_create_connector(drm, dsi); - if (ret) - goto err_encoder_cleanup; - } - - return 0; - -err_encoder_cleanup: - drm_encoder_cleanup(&dsi->encoder); - return ret; -} - -static void mtk_dsi_destroy_conn_enc(struct mtk_dsi *dsi) -{ - drm_encoder_cleanup(&dsi->encoder); - /* Skip connector cleanup if creation was delegated to the bridge */ - if (dsi->conn.dev) - drm_connector_cleanup(&dsi->conn); - if (dsi->panel) - drm_panel_detach(dsi->panel); -} - static void mtk_dsi_ddp_start(struct mtk_ddp_comp *comp) { struct mtk_dsi *dsi = container_of(comp, struct mtk_dsi, ddp_comp); @@ -953,20 +814,6 @@ static int mtk_dsi_host_attach(struct mipi_dsi_host *host, dsi->format = device->format; dsi->mode_flags = device->mode_flags;
- if (dsi->conn.dev) - drm_helper_hpd_irq_event(dsi->conn.dev); - - 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); - - if (dsi->conn.dev) - drm_helper_hpd_irq_event(dsi->conn.dev); - return 0; }
@@ -1110,7 +957,6 @@ 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, };
@@ -1185,6 +1031,7 @@ 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 comp_id; @@ -1203,10 +1050,18 @@ static int mtk_dsi_probe(struct platform_device *pdev) }
ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, - &dsi->panel, &dsi->next_bridge); + &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");
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 ---
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 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:
- drm_bridge (mtk_dsi) -> drm_bridge (ps8640 - dsi-to-edp) -> drm_panel_bridge
(edp panel)
or a
- 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/dr...
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:
- drm_bridge (mtk_dsi) -> drm_bridge (ps8640 - dsi-to-edp) -> drm_panel_bridge
(edp panel)
or a
- 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/dr...
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:
- drm_bridge (mtk_dsi) -> drm_bridge (ps8640 - dsi-to-edp) -> drm_panel_bridge
(edp panel)
or a
- 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/dr...
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:
- drm_bridge (mtk_dsi) -> drm_bridge (ps8640 - dsi-to-edp) -> drm_panel_bridge
(edp panel)
or a
- 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/dr...
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:
- drm_bridge (mtk_dsi) -> drm_bridge (ps8640 - dsi-to-edp) -> drm_panel_bridge
(edp panel)
or a
- 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/dr...
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:
- drm_bridge (mtk_dsi) -> drm_bridge (ps8640 - dsi-to-edp) -> drm_panel_bridge
(edp panel)
or a
- 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/dr...
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
Hi Chun-Kuan, Enric,
Is there any plan to merge the following commits in this series to the mainline?
drm/bridge: ps8640: Get the EDID from eDP control drm/bridge_connector: Set default status connected for eDP connectors
I see that rest of the patchset is already merged and available in 5.9-RC2.
Thanks, Bilal
Hi Bilal,
On 24/8/20 21:01, Bilal Wasim wrote:
Hi Chun-Kuan, Enric,
Is there any plan to merge the following commits in this series to the mainline?
drm/bridge: ps8640: Get the EDID from eDP control drm/bridge_connector: Set default status connected for eDP connectors
Just sent a new series including this patches: https://lore.kernel.org/patchwork/project/lkml/list/?series=459760
Thanks, Enric
I see that rest of the patchset is already merged and available in 5.9-RC2.
Thanks, Bilal
dri-devel@lists.freedesktop.org
-
Bilal Wasim -
Chun-Kuang Hu -
Enric Balletbo i Serra -
Enric Balletbo Serra -
Laurent Pinchart -
Sam Ravnborg