I noticed that commit 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable") conflicts with the panel-edp (panel bridge) support. Both bridges will try to attach directly to the drm encoder itself. I started writing lengthy letter describing what is broken and how it should be fixed. Then at some point I stopped and quickly coded this RFC (which is compile-tested only).
Comments and tests (on both DP and eDP setups) are more than welcome.
The following changes since commit 6ed95285382d6f90a3c3a11d5806a5eb7db715c3:
drm/msm/a5xx: Fix missing CP_PROTECT for SMMU on A540 (2021-12-17 15:09:46 -0800)
are available in the Git repository at:
https://git.linaro.org/people/dmitry.baryshkov/kernel.git msm-dp-bridges
for you to fetch changes up to 7eff304d50ba520e9193a293a8e42bbd9d7aa0e1:
drm/msm/dp: stop carying about the connector type (2022-01-07 04:56:06 +0300)
---------------------------------------------------------------- Dmitry Baryshkov (7): drm/msm/dp: fix panel bridge attachment drm/msm/dp: support attaching bridges to the DP encoder drm/msm/dp: replace dp_connector with drm_bridge_connector drm/msm/dp: remove extra wrappers and public functions drm/msm/dp: remove unused stubs drm/msm/dp: remove dp_display_en/disable prototypes and data argument drm/msm/dp: stop carying about the connector type
drivers/gpu/drm/msm/Makefile | 1 - drivers/gpu/drm/msm/dp/dp_display.c | 263 ++++++++++++++++++++++++++---------- drivers/gpu/drm/msm/dp/dp_display.h | 5 +- drivers/gpu/drm/msm/dp/dp_drm.c | 250 ---------------------------------- drivers/gpu/drm/msm/dp/dp_parser.c | 28 ++-- drivers/gpu/drm/msm/dp/dp_parser.h | 4 +- drivers/gpu/drm/msm/msm_drv.h | 32 +---- 7 files changed, 203 insertions(+), 380 deletions(-) delete mode 100644 drivers/gpu/drm/msm/dp/dp_drm.c
In commit 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable") the DP driver received a drm_bridge instance, which is always attached to the encoder as a root bridge. However it conflicts with the panel_bridge support for eDP panels. Change panel_bridge attachment to come after dp_bridge attachment.
Fixes: 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable") Cc: Kuogee Hsieh quic_khsieh@quicinc.com Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org --- drivers/gpu/drm/msm/dp/dp_drm.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c index d4d360d19eba..26ef41a4c1b6 100644 --- a/drivers/gpu/drm/msm/dp/dp_drm.c +++ b/drivers/gpu/drm/msm/dp/dp_drm.c @@ -169,16 +169,6 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display)
drm_connector_attach_encoder(connector, dp_display->encoder);
- if (dp_display->panel_bridge) { - ret = drm_bridge_attach(dp_display->encoder, - dp_display->panel_bridge, NULL, - DRM_BRIDGE_ATTACH_NO_CONNECTOR); - if (ret < 0) { - DRM_ERROR("failed to attach panel bridge: %d\n", ret); - return ERR_PTR(ret); - } - } - return connector; }
@@ -246,5 +236,16 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_devi return ERR_PTR(rc); }
+ if (dp_display->panel_bridge) { + rc = drm_bridge_attach(dp_display->encoder, + dp_display->panel_bridge, bridge, + DRM_BRIDGE_ATTACH_NO_CONNECTOR); + if (rc < 0) { + DRM_ERROR("failed to attach panel bridge: %d\n", rc); + drm_bridge_remove(bridge); + return ERR_PTR(rc); + } + } + return bridge; }
Quoting Dmitry Baryshkov (2022-01-06 18:01:26)
In commit 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable") the DP driver received a drm_bridge instance, which is always attached to the encoder as a root bridge. However it conflicts with the panel_bridge support for eDP panels.
Can you elaborate here? How does it conflict? Could be as simple as "it attaches before the panel bridge can attach to the root" or something like that.
Change panel_bridge attachment to come after dp_bridge attachment.
On 07/01/2022 06:37, Stephen Boyd wrote:
Quoting Dmitry Baryshkov (2022-01-06 18:01:26)
In commit 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable") the DP driver received a drm_bridge instance, which is always attached to the encoder as a root bridge. However it conflicts with the panel_bridge support for eDP panels.
Can you elaborate here? How does it conflict? Could be as simple as "it attaches before the panel bridge can attach to the root" or something like that.
Actually it would be the other way around: panel bridge attaching before the "dp" one. But yes, you got the idea. I'll extend the patch's description.
Change panel_bridge attachment to come after dp_bridge attachment.
On 1/6/2022 6:01 PM, Dmitry Baryshkov wrote:
In commit 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable") the DP driver received a drm_bridge instance, which is always attached to the encoder as a root bridge. However it conflicts with the panel_bridge support for eDP panels. Change panel_bridge attachment to come after dp_bridge attachment.
Fixes: 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable") Cc: Kuogee Hsieh quic_khsieh@quicinc.com Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
drivers/gpu/drm/msm/dp/dp_drm.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c index d4d360d19eba..26ef41a4c1b6 100644 --- a/drivers/gpu/drm/msm/dp/dp_drm.c +++ b/drivers/gpu/drm/msm/dp/dp_drm.c @@ -169,16 +169,6 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display)
drm_connector_attach_encoder(connector, dp_display->encoder);
- if (dp_display->panel_bridge) {
ret = drm_bridge_attach(dp_display->encoder,dp_display->panel_bridge, NULL,DRM_BRIDGE_ATTACH_NO_CONNECTOR);if (ret < 0) {DRM_ERROR("failed to attach panel bridge: %d\n", ret);return ERR_PTR(ret);}- }
- return connector; }
@@ -246,5 +236,16 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_devi return ERR_PTR(rc); }
can check connector_type here and if connector_type == DRM_MODE_CONNECTOR_eDP then no drm_bridge add to eDP? So that eDP only has panel_bridge and DP only has drm_bridge?
is this fix all your concerns?
- if (dp_display->panel_bridge) {
rc = drm_bridge_attach(dp_display->encoder,dp_display->panel_bridge, bridge,DRM_BRIDGE_ATTACH_NO_CONNECTOR);if (rc < 0) {DRM_ERROR("failed to attach panel bridge: %d\n", rc);drm_bridge_remove(bridge);return ERR_PTR(rc);}- }
- return bridge; }
On Wed, 12 Jan 2022 at 20:41, Kuogee Hsieh quic_khsieh@quicinc.com wrote:
On 1/6/2022 6:01 PM, Dmitry Baryshkov wrote:
In commit 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable") the DP driver received a drm_bridge instance, which is always attached to the encoder as a root bridge. However it conflicts with the panel_bridge support for eDP panels. Change panel_bridge attachment to come after dp_bridge attachment.
Fixes: 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable") Cc: Kuogee Hsieh quic_khsieh@quicinc.com Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
drivers/gpu/drm/msm/dp/dp_drm.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c index d4d360d19eba..26ef41a4c1b6 100644 --- a/drivers/gpu/drm/msm/dp/dp_drm.c +++ b/drivers/gpu/drm/msm/dp/dp_drm.c @@ -169,16 +169,6 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display)
drm_connector_attach_encoder(connector, dp_display->encoder);
if (dp_display->panel_bridge) {ret = drm_bridge_attach(dp_display->encoder,dp_display->panel_bridge, NULL,DRM_BRIDGE_ATTACH_NO_CONNECTOR);if (ret < 0) {DRM_ERROR("failed to attach panel bridge: %d\n", ret);return ERR_PTR(ret);}}return connector;@@ -246,5 +236,16 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_devi return ERR_PTR(rc); }
can check connector_type here and if connector_type == DRM_MODE_CONNECTOR_eDP then no drm_bridge add to eDP? So that eDP only has panel_bridge and DP only has drm_bridge?
No, we still need the DP bridge for the eDP. It handles modesetting, enabling and disabling of the DP controller, etc.
is this fix all your concerns?
if (dp_display->panel_bridge) {rc = drm_bridge_attach(dp_display->encoder,dp_display->panel_bridge, bridge,DRM_BRIDGE_ATTACH_NO_CONNECTOR);if (rc < 0) {DRM_ERROR("failed to attach panel bridge: %d\n", rc);drm_bridge_remove(bridge);return ERR_PTR(rc);}}return bridge;
Currently DP driver will allocate panel bridge for eDP panels. Simplify this code to just check if there is any next bridge in the chain (be it a panel bridge or regular bridge). Rename panel_bridge field to next_bridge accordingly.
Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org --- drivers/gpu/drm/msm/dp/dp_display.c | 2 +- drivers/gpu/drm/msm/dp/dp_display.h | 2 +- drivers/gpu/drm/msm/dp/dp_drm.c | 4 ++-- drivers/gpu/drm/msm/dp/dp_parser.c | 26 ++++++++------------------ drivers/gpu/drm/msm/dp/dp_parser.h | 2 +- 5 files changed, 13 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 7cc4d21f2091..184a1d1470e6 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -246,7 +246,7 @@ static int dp_display_bind(struct device *dev, struct device *master, goto end; }
- dp->dp_display.panel_bridge = dp->parser->panel_bridge; + dp->dp_display.next_bridge = dp->parser->next_bridge;
dp->aux->drm_dev = drm; rc = dp_aux_register(dp->aux); diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h index e3adcd578a90..7af2b186d2d9 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.h +++ b/drivers/gpu/drm/msm/dp/dp_display.h @@ -16,7 +16,7 @@ struct msm_dp { struct drm_bridge *bridge; struct drm_connector *connector; struct drm_encoder *encoder; - struct drm_bridge *panel_bridge; + struct drm_bridge *next_bridge; bool is_connected; bool audio_enabled; bool power_on; diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c index 26ef41a4c1b6..80f59cf99089 100644 --- a/drivers/gpu/drm/msm/dp/dp_drm.c +++ b/drivers/gpu/drm/msm/dp/dp_drm.c @@ -236,9 +236,9 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_devi return ERR_PTR(rc); }
- if (dp_display->panel_bridge) { + if (dp_display->next_bridge) { rc = drm_bridge_attach(dp_display->encoder, - dp_display->panel_bridge, bridge, + dp_display->next_bridge, bridge, DRM_BRIDGE_ATTACH_NO_CONNECTOR); if (rc < 0) { DRM_ERROR("failed to attach panel bridge: %d\n", rc); diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c index a7acc23f742b..5de21f3d0812 100644 --- a/drivers/gpu/drm/msm/dp/dp_parser.c +++ b/drivers/gpu/drm/msm/dp/dp_parser.c @@ -265,22 +265,14 @@ static int dp_parser_clock(struct dp_parser *parser) return 0; }
-static int dp_parser_find_panel(struct dp_parser *parser) +static int dp_parser_find_next_bridge(struct dp_parser *parser) { struct device *dev = &parser->pdev->dev; - struct drm_panel *panel; - int rc;
- rc = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL); - if (rc) { - DRM_ERROR("failed to acquire DRM panel: %d\n", rc); - return rc; - } - - parser->panel_bridge = devm_drm_panel_bridge_add(dev, panel); - if (IS_ERR(parser->panel_bridge)) { - DRM_ERROR("failed to create panel bridge\n"); - return PTR_ERR(parser->panel_bridge); + parser->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0); + if (IS_ERR(parser->next_bridge)) { + DRM_ERROR("failed to create next bridge\n"); + return PTR_ERR(parser->next_bridge); }
return 0; @@ -307,11 +299,9 @@ static int dp_parser_parse(struct dp_parser *parser, int connector_type) if (rc) return rc;
- if (connector_type == DRM_MODE_CONNECTOR_eDP) { - rc = dp_parser_find_panel(parser); - if (rc) - return rc; - } + rc = dp_parser_find_next_bridge(parser); + if (rc) + return rc;
/* Map the corresponding regulator information according to * version. Currently, since we only have one supported platform, diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h index 3172da089421..4cec851e38d9 100644 --- a/drivers/gpu/drm/msm/dp/dp_parser.h +++ b/drivers/gpu/drm/msm/dp/dp_parser.h @@ -123,7 +123,7 @@ struct dp_parser { struct dp_display_data disp_data; const struct dp_regulator_cfg *regulator_cfg; u32 max_dp_lanes; - struct drm_bridge *panel_bridge; + struct drm_bridge *next_bridge;
int (*parse)(struct dp_parser *parser, int connector_type); };
Quoting Dmitry Baryshkov (2022-01-06 18:01:27)
Currently DP driver will allocate panel bridge for eDP panels. Simplify this code to just check if there is any next bridge in the chain (be it a panel bridge or regular bridge). Rename panel_bridge field to next_bridge accordingly.
Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
drivers/gpu/drm/msm/dp/dp_display.c | 2 +- drivers/gpu/drm/msm/dp/dp_display.h | 2 +- drivers/gpu/drm/msm/dp/dp_drm.c | 4 ++-- drivers/gpu/drm/msm/dp/dp_parser.c | 26 ++++++++------------------ drivers/gpu/drm/msm/dp/dp_parser.h | 2 +- 5 files changed, 13 insertions(+), 23 deletions(-)
I like this one, it certainly makes it easier to understand.
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c index a7acc23f742b..5de21f3d0812 100644 --- a/drivers/gpu/drm/msm/dp/dp_parser.c +++ b/drivers/gpu/drm/msm/dp/dp_parser.c @@ -307,11 +299,9 @@ static int dp_parser_parse(struct dp_parser *parser, int connector_type) if (rc) return rc;
if (connector_type == DRM_MODE_CONNECTOR_eDP) {
It feels like this is on purpose, but I don't see any comment so I have no idea. I think qcom folks are concerned about changing how not eDP works. I'll have to test it out locally.
rc = dp_parser_find_panel(parser);if (rc)return rc;}
rc = dp_parser_find_next_bridge(parser);if (rc)return rc; /* Map the corresponding regulator information according to * version. Currently, since we only have one supported platform,
On 07/01/2022 06:42, Stephen Boyd wrote:
Quoting Dmitry Baryshkov (2022-01-06 18:01:27)
Currently DP driver will allocate panel bridge for eDP panels. Simplify this code to just check if there is any next bridge in the chain (be it a panel bridge or regular bridge). Rename panel_bridge field to next_bridge accordingly.
Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
drivers/gpu/drm/msm/dp/dp_display.c | 2 +- drivers/gpu/drm/msm/dp/dp_display.h | 2 +- drivers/gpu/drm/msm/dp/dp_drm.c | 4 ++-- drivers/gpu/drm/msm/dp/dp_parser.c | 26 ++++++++------------------ drivers/gpu/drm/msm/dp/dp_parser.h | 2 +- 5 files changed, 13 insertions(+), 23 deletions(-)
I like this one, it certainly makes it easier to understand.
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c index a7acc23f742b..5de21f3d0812 100644 --- a/drivers/gpu/drm/msm/dp/dp_parser.c +++ b/drivers/gpu/drm/msm/dp/dp_parser.c @@ -307,11 +299,9 @@ static int dp_parser_parse(struct dp_parser *parser, int connector_type) if (rc) return rc;
if (connector_type == DRM_MODE_CONNECTOR_eDP) {It feels like this is on purpose, but I don't see any comment so I have no idea. I think qcom folks are concerned about changing how not eDP works. I'll have to test it out locally.
Ah, another thing that should go into the commit message.
Current situation: - DP: no external bridges supported. - eDP: only a drm_panel wrapped into the panel bridge
After this patch: - both DP and eDP support any chain of bridges attached.
While the change means nothing for the DP (IIUC, it will not have any bridges), it simplifies the code path, lowering the amount of checks.
And for eDP this means that we can attach any eDP-to-something bridges (e.g. NXP PTN3460).
Well... After re-checking the source code for devm_drm_of_get_bridge/drm_of_find_panel_or_bridge I should probably revert removal of the check. The function will return -ENODEV if neither bridge nor panel are specified.
rc = dp_parser_find_panel(parser);if (rc)return rc;}
rc = dp_parser_find_next_bridge(parser);if (rc)return rc; /* Map the corresponding regulator information according to * version. Currently, since we only have one supported platform,
On 1/6/2022 9:26 PM, Dmitry Baryshkov wrote:
On 07/01/2022 06:42, Stephen Boyd wrote:
Quoting Dmitry Baryshkov (2022-01-06 18:01:27)
Currently DP driver will allocate panel bridge for eDP panels. Simplify this code to just check if there is any next bridge in the chain (be it a panel bridge or regular bridge). Rename panel_bridge field to next_bridge accordingly.
Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
drivers/gpu/drm/msm/dp/dp_display.c | 2 +- drivers/gpu/drm/msm/dp/dp_display.h | 2 +- drivers/gpu/drm/msm/dp/dp_drm.c | 4 ++-- drivers/gpu/drm/msm/dp/dp_parser.c | 26 ++++++++------------------ drivers/gpu/drm/msm/dp/dp_parser.h | 2 +- 5 files changed, 13 insertions(+), 23 deletions(-)
I like this one, it certainly makes it easier to understand.
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c index a7acc23f742b..5de21f3d0812 100644 --- a/drivers/gpu/drm/msm/dp/dp_parser.c +++ b/drivers/gpu/drm/msm/dp/dp_parser.c @@ -307,11 +299,9 @@ static int dp_parser_parse(struct dp_parser *parser, int connector_type) if (rc) return rc;
- if (connector_type == DRM_MODE_CONNECTOR_eDP) {
It feels like this is on purpose, but I don't see any comment so I have no idea. I think qcom folks are concerned about changing how not eDP works. I'll have to test it out locally.
Ah, another thing that should go into the commit message.
Current situation:
- DP: no external bridges supported.
- eDP: only a drm_panel wrapped into the panel bridge
After this patch:
- both DP and eDP support any chain of bridges attached.
While the change means nothing for the DP (IIUC, it will not have any bridges), it simplifies the code path, lowering the amount of checks.
And for eDP this means that we can attach any eDP-to-something bridges (e.g. NXP PTN3460).
Well... After re-checking the source code for devm_drm_of_get_bridge/drm_of_find_panel_or_bridge I should probably revert removal of the check. The function will return -ENODEV if neither bridge nor panel are specified.
I am new to drm and confusing with bridge here.
Isn't bridge used to bridging two different kind of interface together?
for example, dsi <--> bridge <--> dp.
why edp need bridge here?
Can you give me more info regrading what bridge try to do here.
- rc = dp_parser_find_panel(parser); - if (rc) - return rc; - } + rc = dp_parser_find_next_bridge(parser); + if (rc) + return rc;
/* Map the corresponding regulator information according to * version. Currently, since we only have one supported platform,
On Wed, 12 Jan 2022 at 02:12, Kuogee Hsieh quic_khsieh@quicinc.com wrote:
On 1/6/2022 9:26 PM, Dmitry Baryshkov wrote:
On 07/01/2022 06:42, Stephen Boyd wrote:
Quoting Dmitry Baryshkov (2022-01-06 18:01:27)
Currently DP driver will allocate panel bridge for eDP panels. Simplify this code to just check if there is any next bridge in the chain (be it a panel bridge or regular bridge). Rename panel_bridge field to next_bridge accordingly.
Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
drivers/gpu/drm/msm/dp/dp_display.c | 2 +- drivers/gpu/drm/msm/dp/dp_display.h | 2 +- drivers/gpu/drm/msm/dp/dp_drm.c | 4 ++-- drivers/gpu/drm/msm/dp/dp_parser.c | 26 ++++++++------------------ drivers/gpu/drm/msm/dp/dp_parser.h | 2 +- 5 files changed, 13 insertions(+), 23 deletions(-)
I like this one, it certainly makes it easier to understand.
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c index a7acc23f742b..5de21f3d0812 100644 --- a/drivers/gpu/drm/msm/dp/dp_parser.c +++ b/drivers/gpu/drm/msm/dp/dp_parser.c @@ -307,11 +299,9 @@ static int dp_parser_parse(struct dp_parser *parser, int connector_type) if (rc) return rc;
if (connector_type == DRM_MODE_CONNECTOR_eDP) {It feels like this is on purpose, but I don't see any comment so I have no idea. I think qcom folks are concerned about changing how not eDP works. I'll have to test it out locally.
Ah, another thing that should go into the commit message.
Current situation:
- DP: no external bridges supported.
- eDP: only a drm_panel wrapped into the panel bridge
After this patch:
- both DP and eDP support any chain of bridges attached.
While the change means nothing for the DP (IIUC, it will not have any bridges), it simplifies the code path, lowering the amount of checks.
And for eDP this means that we can attach any eDP-to-something bridges (e.g. NXP PTN3460).
Well... After re-checking the source code for devm_drm_of_get_bridge/drm_of_find_panel_or_bridge I should probably revert removal of the check. The function will return -ENODEV if neither bridge nor panel are specified.
I am new to drm and confusing with bridge here.
Isn't bridge used to bridging two different kind of interface together?
for example, dsi <--> bridge <--> dp.
why edp need bridge here?
Can you give me more info regrading what bridge try to do here.
First, there are bridges converting the eDP interface to another interface. The mentioned NXP PTN3460 converts (embedded) DisplayPort to LVDS.
Second (and this is the case here) drm_bridge can be used to wrap drm_panel (panel-bridge), so that the driver doesn't have to care about the drm_panel interface.
Last, but not least, external display connectors can also be abstracted as bridges (see display-connector.c).
This becomes even more appealing as the driver can then switch to drm_bridge_connector, supporting any kinds of pipelines attached to the encoder, supporting any kind of converters, panel or external connector pipelines.Think about the following (sometimes crazy, but possible) examples. With drm_bridge/panel-bridge/display-connector/drm_bridge_connector there is no difference for the driver at all: - DP encoder ⇒ DP port ⇒ DP monitor - DP encoder ⇒ DP connector supporting generic GPIO as HPD ⇒ DP port ⇒ DP monitor - eDP encoder ⇒ eDP panel - eDP encoder ⇒ ptn3460 ⇒ fixed LVDS panel - eDP encoder ⇒ ptn3460 ⇒ LVDS connector with EDID lines for panel autodetect - eDP encoder ⇒ ptn3460 ⇒ THC63LVD1024 ⇒ DPI panel. - eDP encoder ⇒ LT8912 ⇒ DSI panel
rc = dp_parser_find_panel(parser);if (rc)return rc;}
rc = dp_parser_find_next_bridge(parser);if (rc)return rc; /* Map the corresponding regulator information according to * version. Currently, since we only have one supportedplatform,
There is little point in having both connector and root bridge implementation in the same driver. Move connector's functionality to the bridge to let next bridge in chain to override it.
Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org --- drivers/gpu/drm/msm/dp/dp_display.c | 22 +++--- drivers/gpu/drm/msm/dp/dp_drm.c | 111 ++++++++++------------------ 2 files changed, 51 insertions(+), 82 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 184a1d1470e6..539aac117184 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -1476,17 +1476,6 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
dp_display->encoder = encoder;
- dp_display->connector = dp_drm_connector_init(dp_display); - if (IS_ERR(dp_display->connector)) { - ret = PTR_ERR(dp_display->connector); - DRM_DEV_ERROR(dev->dev, - "failed to create dp connector: %d\n", ret); - dp_display->connector = NULL; - return ret; - } - - priv->connectors[priv->num_connectors++] = dp_display->connector; - dp_display->bridge = msm_dp_bridge_init(dp_display, dev, encoder); if (IS_ERR(dp_display->bridge)) { ret = PTR_ERR(dp_display->bridge); @@ -1498,6 +1487,17 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
priv->bridges[priv->num_bridges++] = dp_display->bridge;
+ dp_display->connector = dp_drm_connector_init(dp_display); + if (IS_ERR(dp_display->connector)) { + ret = PTR_ERR(dp_display->connector); + DRM_DEV_ERROR(dev->dev, + "failed to create dp connector: %d\n", ret); + dp_display->connector = NULL; + return ret; + } + + priv->connectors[priv->num_connectors++] = dp_display->connector; + return 0; }
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c index 80f59cf99089..5f093d90d227 100644 --- a/drivers/gpu/drm/msm/dp/dp_drm.c +++ b/drivers/gpu/drm/msm/dp/dp_drm.c @@ -6,6 +6,7 @@ #include <drm/drm_atomic_helper.h> #include <drm/drm_atomic.h> #include <drm/drm_bridge.h> +#include <drm/drm_bridge_connector.h> #include <drm/drm_crtc.h>
#include "msm_drv.h" @@ -20,24 +21,16 @@ struct msm_dp_bridge {
#define to_dp_display(x) container_of((x), struct msm_dp_bridge, bridge)
-struct dp_connector { - struct drm_connector base; - struct msm_dp *dp_display; -}; -#define to_dp_connector(x) container_of(x, struct dp_connector, base) - /** - * dp_connector_detect - callback to determine if connector is connected - * @conn: Pointer to drm connector structure - * @force: Force detect setting from drm framework - * Returns: Connector 'is connected' status + * dp_bridge_detect - callback to determine if connector is connected + * @bridge: Pointer to drm bridge structure + * Returns: Bridge's 'is connected' status */ -static enum drm_connector_status dp_connector_detect(struct drm_connector *conn, - bool force) +static enum drm_connector_status dp_bridge_detect(struct drm_bridge *bridge) { struct msm_dp *dp;
- dp = to_dp_connector(conn)->dp_display; + dp = to_dp_display(bridge)->dp_display;
DRM_DEBUG_DP("is_connected = %s\n", (dp->is_connected) ? "true" : "false"); @@ -48,10 +41,11 @@ static enum drm_connector_status dp_connector_detect(struct drm_connector *conn,
/** * dp_connector_get_modes - callback to add drm modes via drm_mode_probed_add() + * @bridge: Poiner to drm bridge * @connector: Pointer to drm connector structure * Returns: Number of modes added */ -static int dp_connector_get_modes(struct drm_connector *connector) +static int dp_bridge_get_modes(struct drm_bridge *bridge, struct drm_connector *connector) { int rc = 0; struct msm_dp *dp; @@ -61,7 +55,7 @@ static int dp_connector_get_modes(struct drm_connector *connector) if (!connector) return 0;
- dp = to_dp_connector(connector)->dp_display; + dp = to_dp_display(bridge)->dp_display;
dp_mode = kzalloc(sizeof(*dp_mode), GFP_KERNEL); if (!dp_mode) @@ -102,18 +96,20 @@ static int dp_connector_get_modes(struct drm_connector *connector) }
/** - * dp_connector_mode_valid - callback to determine if specified mode is valid - * @connector: Pointer to drm connector structure + * dp_bridge_mode_valid - callback to determine if specified mode is valid + * @bridge: Pointer to drm bridge structure + * @info: display info * @mode: Pointer to drm mode structure * Returns: Validity status for specified mode */ -static enum drm_mode_status dp_connector_mode_valid( - struct drm_connector *connector, - struct drm_display_mode *mode) +static enum drm_mode_status dp_bridge_mode_valid( + struct drm_bridge *bridge, + const struct drm_display_info *info, + const struct drm_display_mode *mode) { struct msm_dp *dp_disp;
- dp_disp = to_dp_connector(connector)->dp_display; + dp_disp = to_dp_display(bridge)->dp_display;
if ((dp_disp->max_pclk_khz <= 0) || (dp_disp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) || @@ -123,55 +119,6 @@ static enum drm_mode_status dp_connector_mode_valid( return dp_display_validate_mode(dp_disp, mode->clock); }
-static const struct drm_connector_funcs dp_connector_funcs = { - .detect = dp_connector_detect, - .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 dp_connector_helper_funcs = { - .get_modes = dp_connector_get_modes, - .mode_valid = dp_connector_mode_valid, -}; - -/* connector initialization */ -struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display) -{ - struct drm_connector *connector = NULL; - struct dp_connector *dp_connector; - int ret; - - dp_connector = devm_kzalloc(dp_display->drm_dev->dev, - sizeof(*dp_connector), - GFP_KERNEL); - if (!dp_connector) - return ERR_PTR(-ENOMEM); - - dp_connector->dp_display = dp_display; - - connector = &dp_connector->base; - - ret = drm_connector_init(dp_display->drm_dev, connector, - &dp_connector_funcs, - dp_display->connector_type); - if (ret) - return ERR_PTR(ret); - - drm_connector_helper_add(connector, &dp_connector_helper_funcs); - - /* - * Enable HPD to let hpd event is handled when cable is connected. - */ - connector->polled = DRM_CONNECTOR_POLL_HPD; - - drm_connector_attach_encoder(connector, dp_display->encoder); - - return connector; -} - static void dp_bridge_mode_set(struct drm_bridge *drm_bridge, const struct drm_display_mode *mode, const struct drm_display_mode *adjusted_mode) @@ -211,6 +158,9 @@ static const struct drm_bridge_funcs dp_bridge_ops = { .disable = dp_bridge_disable, .post_disable = dp_bridge_post_disable, .mode_set = dp_bridge_mode_set, + .mode_valid = dp_bridge_mode_valid, + .get_modes = dp_bridge_get_modes, + .detect = dp_bridge_detect, };
struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev, @@ -228,7 +178,12 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_devi
bridge = &dp_bridge->bridge; bridge->funcs = &dp_bridge_ops; - bridge->encoder = encoder; + bridge->type = DRM_MODE_CONNECTOR_DisplayPort; + + bridge->ops = + DRM_BRIDGE_OP_DETECT | + DRM_BRIDGE_OP_HPD | + DRM_BRIDGE_OP_MODES;
rc = drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); if (rc) { @@ -249,3 +204,17 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_devi
return bridge; } + +/* connector initialization */ +struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display) +{ + struct drm_connector *connector = NULL; + + connector = drm_bridge_connector_init(dp_display->drm_dev, dp_display->encoder); + if (IS_ERR(connector)) + return connector; + + drm_connector_attach_encoder(connector, dp_display->encoder); + + return connector; +}
dp_bridge's functions are thin wrappers around the msm_dp_display_* family. Squash dp_bridge callbacks into respective msm_dp_display functions, removing the latter functions from public space.
Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org --- drivers/gpu/drm/msm/Makefile | 1 - drivers/gpu/drm/msm/dp/dp_display.c | 184 +++++++++++++++++++++-- drivers/gpu/drm/msm/dp/dp_display.h | 3 - drivers/gpu/drm/msm/dp/dp_drm.c | 220 ---------------------------- drivers/gpu/drm/msm/msm_drv.h | 32 +--- 5 files changed, 171 insertions(+), 269 deletions(-) delete mode 100644 drivers/gpu/drm/msm/dp/dp_drm.c
diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile index 03ab55c37beb..354ccf793f4a 100644 --- a/drivers/gpu/drm/msm/Makefile +++ b/drivers/gpu/drm/msm/Makefile @@ -104,7 +104,6 @@ msm-$(CONFIG_DRM_MSM_DP)+= dp/dp_aux.o \ dp/dp_catalog.o \ dp/dp_ctrl.o \ dp/dp_display.o \ - dp/dp_drm.o \ dp/dp_hpd.o \ dp/dp_link.o \ dp/dp_panel.o \ diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 539aac117184..7b4f40cb9a58 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -10,7 +10,9 @@ #include <linux/component.h> #include <linux/of_irq.h> #include <linux/delay.h> -#include <drm/drm_panel.h> + +#include <drm/drm_bridge.h> +#include <drm/drm_bridge_connector.h>
#include "msm_drv.h" #include "msm_kms.h" @@ -30,6 +32,13 @@
#define HPD_STRING_SIZE 30
+struct msm_dp_bridge { + struct drm_bridge bridge; + struct msm_dp *dp_display; +}; + +#define to_dp_display(x) container_of((x), struct msm_dp_bridge, bridge) + enum { ISR_DISCONNECTED, ISR_CONNECT_PENDING, @@ -924,18 +933,30 @@ int dp_display_set_plugged_cb(struct msm_dp *dp_display, return 0; }
-int dp_display_validate_mode(struct msm_dp *dp, u32 mode_pclk_khz) +static enum drm_mode_status dp_bridge_mode_valid( + struct drm_bridge *bridge, + const struct drm_display_info *info, + const struct drm_display_mode *mode) { const u32 num_components = 3, default_bpp = 24; struct dp_display_private *dp_display; struct dp_link_info *link_info; u32 mode_rate_khz = 0, supported_rate_khz = 0, mode_bpp = 0; + struct msm_dp *dp; + int mode_pclk_khz = mode->clock; + + dp = to_dp_display(bridge)->dp_display;
if (!dp || !mode_pclk_khz || !dp->connector) { DRM_ERROR("invalid params\n"); return -EINVAL; }
+ if ((dp->max_pclk_khz <= 0) || + (dp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) || + (mode->clock > dp->max_pclk_khz)) + return MODE_BAD; + dp_display = container_of(dp, struct dp_display_private, dp_display); link_info = &dp_display->panel->link_info;
@@ -1456,6 +1477,23 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor) } }
+/* connector initialization */ +struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display) +{ + struct drm_connector *connector = NULL; + + connector = drm_bridge_connector_init(dp_display->drm_dev, dp_display->encoder); + if (IS_ERR(connector)) + return connector; + + drm_connector_attach_encoder(connector, dp_display->encoder); + + return connector; +} + +static struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev, + struct drm_encoder *encoder); + int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, struct drm_encoder *encoder) { @@ -1501,8 +1539,10 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, return 0; }
-int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder) +static void dp_bridge_enable(struct drm_bridge *drm_bridge) { + struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge); + struct msm_dp *dp = dp_bridge->dp_display; int rc = 0; struct dp_display_private *dp_display; u32 state; @@ -1510,7 +1550,7 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder) dp_display = container_of(dp, struct dp_display_private, dp_display); if (!dp_display->dp_mode.drm_mode.clock) { DRM_ERROR("invalid params\n"); - return -EINVAL; + return; }
mutex_lock(&dp_display->event_mutex); @@ -1522,14 +1562,14 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder) if (rc) { DRM_ERROR("Failed to perform a mode set, rc=%d\n", rc); mutex_unlock(&dp_display->event_mutex); - return rc; + return; }
rc = dp_display_prepare(dp); if (rc) { DRM_ERROR("DP display prepare failed, rc=%d\n", rc); mutex_unlock(&dp_display->event_mutex); - return rc; + return; }
state = dp_display->hpd_state; @@ -1554,23 +1594,23 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder) dp_display->hpd_state = ST_CONNECTED;
mutex_unlock(&dp_display->event_mutex); - - return rc; }
-int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder *encoder) +static void dp_bridge_disable(struct drm_bridge *drm_bridge) { + struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge); + struct msm_dp *dp = dp_bridge->dp_display; struct dp_display_private *dp_display;
dp_display = container_of(dp, struct dp_display_private, dp_display);
dp_ctrl_push_idle(dp_display->ctrl); - - return 0; }
-int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder) +static void dp_bridge_post_disable(struct drm_bridge *drm_bridge) { + struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge); + struct msm_dp *dp = dp_bridge->dp_display; int rc = 0; u32 state; struct dp_display_private *dp_display; @@ -1597,13 +1637,14 @@ int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder) }
mutex_unlock(&dp_display->event_mutex); - return rc; }
-void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder *encoder, +static void dp_bridge_mode_set(struct drm_bridge *drm_bridge, const struct drm_display_mode *mode, const struct drm_display_mode *adjusted_mode) { + struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge); + struct msm_dp *dp = dp_bridge->dp_display; struct dp_display_private *dp_display;
dp_display = container_of(dp, struct dp_display_private, dp_display); @@ -1626,3 +1667,118 @@ void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder *encoder, dp_display->dp_mode.h_active_low = !!(dp_display->dp_mode.drm_mode.flags & DRM_MODE_FLAG_NHSYNC); } + +static enum drm_connector_status dp_bridge_detect(struct drm_bridge *bridge) +{ + struct msm_dp *dp; + + dp = to_dp_display(bridge)->dp_display; + + DRM_DEBUG_DP("is_connected = %s\n", + (dp->is_connected) ? "true" : "false"); + + return (dp->is_connected) ? connector_status_connected : + connector_status_disconnected; +} + +static int dp_bridge_get_modes(struct drm_bridge *bridge, struct drm_connector *connector) +{ + int rc = 0; + struct msm_dp *dp; + struct dp_display_mode *dp_mode = NULL; + struct drm_display_mode *m, drm_mode; + + if (!connector) + return 0; + + dp = to_dp_display(bridge)->dp_display; + + dp_mode = kzalloc(sizeof(*dp_mode), GFP_KERNEL); + if (!dp_mode) + return 0; + + /* pluggable case assumes EDID is read when HPD */ + if (dp->is_connected) { + /* + *The get_modes() function might return one mode that is stored + * in dp_mode when compliance test is in progress. If not, the + * return value is equal to the total number of modes supported + * by the sink + */ + rc = dp_display_get_modes(dp, dp_mode); + if (rc <= 0) { + DRM_ERROR("failed to get DP sink modes, rc=%d\n", rc); + kfree(dp_mode); + return rc; + } + if (dp_mode->drm_mode.clock) { /* valid DP mode */ + memset(&drm_mode, 0x0, sizeof(drm_mode)); + drm_mode_copy(&drm_mode, &dp_mode->drm_mode); + m = drm_mode_duplicate(connector->dev, &drm_mode); + if (!m) { + DRM_ERROR("failed to add mode %ux%u\n", + drm_mode.hdisplay, + drm_mode.vdisplay); + kfree(dp_mode); + return 0; + } + drm_mode_probed_add(connector, m); + } + } else { + DRM_DEBUG_DP("No sink connected\n"); + } + kfree(dp_mode); + return rc; +} + +static const struct drm_bridge_funcs dp_bridge_ops = { + .enable = dp_bridge_enable, + .disable = dp_bridge_disable, + .post_disable = dp_bridge_post_disable, + .mode_set = dp_bridge_mode_set, + .mode_valid = dp_bridge_mode_valid, + .get_modes = dp_bridge_get_modes, + .detect = dp_bridge_detect, +}; + +static struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev, + struct drm_encoder *encoder) +{ + int rc; + struct msm_dp_bridge *dp_bridge; + struct drm_bridge *bridge; + + dp_bridge = devm_kzalloc(dev->dev, sizeof(*dp_bridge), GFP_KERNEL); + if (!dp_bridge) + return ERR_PTR(-ENOMEM); + + dp_bridge->dp_display = dp_display; + + bridge = &dp_bridge->bridge; + bridge->funcs = &dp_bridge_ops; + bridge->type = DRM_MODE_CONNECTOR_DisplayPort; + + bridge->ops = + DRM_BRIDGE_OP_DETECT | + DRM_BRIDGE_OP_HPD | + DRM_BRIDGE_OP_MODES; + + rc = drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); + if (rc) { + DRM_ERROR("failed to attach bridge, rc=%d\n", rc); + return ERR_PTR(rc); + } + + if (dp_display->next_bridge) { + rc = drm_bridge_attach(dp_display->encoder, + dp_display->next_bridge, bridge, + DRM_BRIDGE_ATTACH_NO_CONNECTOR); + if (rc < 0) { + DRM_ERROR("failed to attach panel bridge: %d\n", rc); + drm_bridge_remove(bridge); + return ERR_PTR(rc); + } + } + + return bridge; +} diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h index 7af2b186d2d9..cabf439af9ee 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.h +++ b/drivers/gpu/drm/msm/dp/dp_display.h @@ -32,9 +32,6 @@ struct msm_dp {
int dp_display_set_plugged_cb(struct msm_dp *dp_display, hdmi_codec_plugged_cb fn, struct device *codec_dev); -int dp_display_validate_mode(struct msm_dp *dp_display, u32 mode_pclk_khz); -int dp_display_get_modes(struct msm_dp *dp_display, - struct dp_display_mode *dp_mode); int dp_display_request_irq(struct msm_dp *dp_display); bool dp_display_check_video_test(struct msm_dp *dp_display); int dp_display_get_test_bpp(struct msm_dp *dp_display); diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c deleted file mode 100644 index 5f093d90d227..000000000000 --- a/drivers/gpu/drm/msm/dp/dp_drm.c +++ /dev/null @@ -1,220 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-only -/* - * Copyright (c) 2017-2020, The Linux Foundation. All rights reserved. - */ - -#include <drm/drm_atomic_helper.h> -#include <drm/drm_atomic.h> -#include <drm/drm_bridge.h> -#include <drm/drm_bridge_connector.h> -#include <drm/drm_crtc.h> - -#include "msm_drv.h" -#include "msm_kms.h" -#include "dp_drm.h" - - -struct msm_dp_bridge { - struct drm_bridge bridge; - struct msm_dp *dp_display; -}; - -#define to_dp_display(x) container_of((x), struct msm_dp_bridge, bridge) - -/** - * dp_bridge_detect - callback to determine if connector is connected - * @bridge: Pointer to drm bridge structure - * Returns: Bridge's 'is connected' status - */ -static enum drm_connector_status dp_bridge_detect(struct drm_bridge *bridge) -{ - struct msm_dp *dp; - - dp = to_dp_display(bridge)->dp_display; - - DRM_DEBUG_DP("is_connected = %s\n", - (dp->is_connected) ? "true" : "false"); - - return (dp->is_connected) ? connector_status_connected : - connector_status_disconnected; -} - -/** - * dp_connector_get_modes - callback to add drm modes via drm_mode_probed_add() - * @bridge: Poiner to drm bridge - * @connector: Pointer to drm connector structure - * Returns: Number of modes added - */ -static int dp_bridge_get_modes(struct drm_bridge *bridge, struct drm_connector *connector) -{ - int rc = 0; - struct msm_dp *dp; - struct dp_display_mode *dp_mode = NULL; - struct drm_display_mode *m, drm_mode; - - if (!connector) - return 0; - - dp = to_dp_display(bridge)->dp_display; - - dp_mode = kzalloc(sizeof(*dp_mode), GFP_KERNEL); - if (!dp_mode) - return 0; - - /* pluggable case assumes EDID is read when HPD */ - if (dp->is_connected) { - /* - *The get_modes() function might return one mode that is stored - * in dp_mode when compliance test is in progress. If not, the - * return value is equal to the total number of modes supported - * by the sink - */ - rc = dp_display_get_modes(dp, dp_mode); - if (rc <= 0) { - DRM_ERROR("failed to get DP sink modes, rc=%d\n", rc); - kfree(dp_mode); - return rc; - } - if (dp_mode->drm_mode.clock) { /* valid DP mode */ - memset(&drm_mode, 0x0, sizeof(drm_mode)); - drm_mode_copy(&drm_mode, &dp_mode->drm_mode); - m = drm_mode_duplicate(connector->dev, &drm_mode); - if (!m) { - DRM_ERROR("failed to add mode %ux%u\n", - drm_mode.hdisplay, - drm_mode.vdisplay); - kfree(dp_mode); - return 0; - } - drm_mode_probed_add(connector, m); - } - } else { - DRM_DEBUG_DP("No sink connected\n"); - } - kfree(dp_mode); - return rc; -} - -/** - * dp_bridge_mode_valid - callback to determine if specified mode is valid - * @bridge: Pointer to drm bridge structure - * @info: display info - * @mode: Pointer to drm mode structure - * Returns: Validity status for specified mode - */ -static enum drm_mode_status dp_bridge_mode_valid( - struct drm_bridge *bridge, - const struct drm_display_info *info, - const struct drm_display_mode *mode) -{ - struct msm_dp *dp_disp; - - dp_disp = to_dp_display(bridge)->dp_display; - - if ((dp_disp->max_pclk_khz <= 0) || - (dp_disp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) || - (mode->clock > dp_disp->max_pclk_khz)) - return MODE_BAD; - - return dp_display_validate_mode(dp_disp, mode->clock); -} - -static void dp_bridge_mode_set(struct drm_bridge *drm_bridge, - const struct drm_display_mode *mode, - const struct drm_display_mode *adjusted_mode) -{ - struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge); - struct msm_dp *dp_display = dp_bridge->dp_display; - - msm_dp_display_mode_set(dp_display, drm_bridge->encoder, mode, adjusted_mode); -} - -static void dp_bridge_enable(struct drm_bridge *drm_bridge) -{ - struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge); - struct msm_dp *dp_display = dp_bridge->dp_display; - - msm_dp_display_enable(dp_display, drm_bridge->encoder); -} - -static void dp_bridge_disable(struct drm_bridge *drm_bridge) -{ - struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge); - struct msm_dp *dp_display = dp_bridge->dp_display; - - msm_dp_display_pre_disable(dp_display, drm_bridge->encoder); -} - -static void dp_bridge_post_disable(struct drm_bridge *drm_bridge) -{ - struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge); - struct msm_dp *dp_display = dp_bridge->dp_display; - - msm_dp_display_disable(dp_display, drm_bridge->encoder); -} - -static const struct drm_bridge_funcs dp_bridge_ops = { - .enable = dp_bridge_enable, - .disable = dp_bridge_disable, - .post_disable = dp_bridge_post_disable, - .mode_set = dp_bridge_mode_set, - .mode_valid = dp_bridge_mode_valid, - .get_modes = dp_bridge_get_modes, - .detect = dp_bridge_detect, -}; - -struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev, - struct drm_encoder *encoder) -{ - int rc; - struct msm_dp_bridge *dp_bridge; - struct drm_bridge *bridge; - - dp_bridge = devm_kzalloc(dev->dev, sizeof(*dp_bridge), GFP_KERNEL); - if (!dp_bridge) - return ERR_PTR(-ENOMEM); - - dp_bridge->dp_display = dp_display; - - bridge = &dp_bridge->bridge; - bridge->funcs = &dp_bridge_ops; - bridge->type = DRM_MODE_CONNECTOR_DisplayPort; - - bridge->ops = - DRM_BRIDGE_OP_DETECT | - DRM_BRIDGE_OP_HPD | - DRM_BRIDGE_OP_MODES; - - rc = drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); - if (rc) { - DRM_ERROR("failed to attach bridge, rc=%d\n", rc); - return ERR_PTR(rc); - } - - if (dp_display->next_bridge) { - rc = drm_bridge_attach(dp_display->encoder, - dp_display->next_bridge, bridge, - DRM_BRIDGE_ATTACH_NO_CONNECTOR); - if (rc < 0) { - DRM_ERROR("failed to attach panel bridge: %d\n", rc); - drm_bridge_remove(bridge); - return ERR_PTR(rc); - } - } - - return bridge; -} - -/* connector initialization */ -struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display) -{ - struct drm_connector *connector = NULL; - - connector = drm_bridge_connector_init(dp_display->drm_dev, dp_display->encoder); - if (IS_ERR(connector)) - return connector; - - drm_connector_attach_encoder(connector, dp_display->encoder); - - return connector; -} diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index d7574e6bd4e4..a48e0737692c 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -385,16 +385,7 @@ int __init msm_dp_register(void); void __exit msm_dp_unregister(void); int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, struct drm_encoder *encoder); -int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder); -int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder); -int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder *encoder); -void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder *encoder, - const struct drm_display_mode *mode, - const struct drm_display_mode *adjusted_mode); - -struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, - struct drm_device *dev, - struct drm_encoder *encoder); + void msm_dp_irq_postinstall(struct msm_dp *dp_display); void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_display);
@@ -414,27 +405,6 @@ static inline int msm_dp_modeset_init(struct msm_dp *dp_display, { return -EINVAL; } -static inline int msm_dp_display_enable(struct msm_dp *dp, - struct drm_encoder *encoder) -{ - return -EINVAL; -} -static inline int msm_dp_display_disable(struct msm_dp *dp, - struct drm_encoder *encoder) -{ - return -EINVAL; -} -static inline int msm_dp_display_pre_disable(struct msm_dp *dp, - struct drm_encoder *encoder) -{ - return -EINVAL; -} -static inline void msm_dp_display_mode_set(struct msm_dp *dp, - struct drm_encoder *encoder, - const struct drm_display_mode *mode, - const struct drm_display_mode *adjusted_mode) -{ -}
static inline void msm_dp_irq_postinstall(struct msm_dp *dp_display) {
Refactoring DP code transformed several functions into empty stubs. Remove them.
Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org --- drivers/gpu/drm/msm/dp/dp_display.c | 35 ----------------------------- 1 file changed, 35 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 7b4f40cb9a58..e63d6056e39d 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -841,11 +841,6 @@ static int dp_display_set_mode(struct msm_dp *dp_display, return 0; }
-static int dp_display_prepare(struct msm_dp *dp_display) -{ - return 0; -} - static int dp_display_enable(struct dp_display_private *dp, u32 data) { int rc = 0; @@ -915,11 +910,6 @@ static int dp_display_disable(struct dp_display_private *dp, u32 data) return 0; }
-static int dp_display_unprepare(struct msm_dp *dp_display) -{ - return 0; -} - int dp_display_set_plugged_cb(struct msm_dp *dp_display, hdmi_codec_plugged_cb fn, struct device *codec_dev) { @@ -1400,21 +1390,9 @@ static int dp_pm_suspend(struct device *dev) return 0; }
-static int dp_pm_prepare(struct device *dev) -{ - return 0; -} - -static void dp_pm_complete(struct device *dev) -{ - -} - static const struct dev_pm_ops dp_pm_ops = { .suspend = dp_pm_suspend, .resume = dp_pm_resume, - .prepare = dp_pm_prepare, - .complete = dp_pm_complete, };
static struct platform_driver dp_display_driver = { @@ -1565,13 +1543,6 @@ static void dp_bridge_enable(struct drm_bridge *drm_bridge) return; }
- rc = dp_display_prepare(dp); - if (rc) { - DRM_ERROR("DP display prepare failed, rc=%d\n", rc); - mutex_unlock(&dp_display->event_mutex); - return; - } - state = dp_display->hpd_state;
if (state == ST_DISPLAY_OFF) @@ -1583,7 +1554,6 @@ static void dp_bridge_enable(struct drm_bridge *drm_bridge) if (rc) { DRM_ERROR("DP display post enable failed, rc=%d\n", rc); dp_display_disable(dp_display, 0); - dp_display_unprepare(dp); }
/* manual kick off plug event to train link */ @@ -1611,7 +1581,6 @@ static void dp_bridge_post_disable(struct drm_bridge *drm_bridge) { struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge); struct msm_dp *dp = dp_bridge->dp_display; - int rc = 0; u32 state; struct dp_display_private *dp_display;
@@ -1624,10 +1593,6 @@ static void dp_bridge_post_disable(struct drm_bridge *drm_bridge)
dp_display_disable(dp_display, 0);
- rc = dp_display_unprepare(dp); - if (rc) - DRM_ERROR("DP display unprepare failed, rc=%d\n", rc); - state = dp_display->hpd_state; if (state == ST_DISCONNECT_PENDING) { /* completed disconnection */
Remove unused dp_display_en/disable prototypes. While we are at it, remove extra 'data' argument that is unused.
Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org --- drivers/gpu/drm/msm/dp/dp_display.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index e63d6056e39d..720e80ea99cb 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -559,9 +559,6 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data) return 0; };
-static int dp_display_enable(struct dp_display_private *dp, u32 data); -static int dp_display_disable(struct dp_display_private *dp, u32 data); - static int dp_connect_pending_timeout(struct dp_display_private *dp, u32 data) { u32 state; @@ -841,7 +838,7 @@ static int dp_display_set_mode(struct msm_dp *dp_display, return 0; }
-static int dp_display_enable(struct dp_display_private *dp, u32 data) +static int dp_display_enable(struct dp_display_private *dp) { int rc = 0; struct msm_dp *dp_display = &dp->dp_display; @@ -878,7 +875,7 @@ static int dp_display_post_enable(struct msm_dp *dp_display) return 0; }
-static int dp_display_disable(struct dp_display_private *dp, u32 data) +static int dp_display_disable(struct dp_display_private *dp) { struct msm_dp *dp_display = &dp->dp_display;
@@ -1548,12 +1545,12 @@ static void dp_bridge_enable(struct drm_bridge *drm_bridge) if (state == ST_DISPLAY_OFF) dp_display_host_init(dp_display, true);
- dp_display_enable(dp_display, 0); + dp_display_enable(dp_display);
rc = dp_display_post_enable(dp); if (rc) { DRM_ERROR("DP display post enable failed, rc=%d\n", rc); - dp_display_disable(dp_display, 0); + dp_display_disable(dp_display); }
/* manual kick off plug event to train link */ @@ -1591,7 +1588,7 @@ static void dp_bridge_post_disable(struct drm_bridge *drm_bridge) /* stop sentinel checking */ dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);
- dp_display_disable(dp_display, 0); + dp_display_disable(dp_display);
state = dp_display->hpd_state; if (state == ST_DISCONNECT_PENDING) {
After changing dp_parser code to always check for the next bridge, it does not check connector type anymore. Remove connector type from the dp_paser_parse() arguments list and from the struct msm_dp_desc fields list.
Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org --- drivers/gpu/drm/msm/dp/dp_display.c | 9 ++++----- drivers/gpu/drm/msm/dp/dp_parser.c | 2 +- drivers/gpu/drm/msm/dp/dp_parser.h | 2 +- 3 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 720e80ea99cb..0a71a17975b0 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -129,7 +129,6 @@ struct dp_display_private {
struct msm_dp_desc { phys_addr_t io_start; - unsigned int connector_type; };
struct msm_dp_config { @@ -139,15 +138,15 @@ struct msm_dp_config {
static const struct msm_dp_config sc7180_dp_cfg = { .descs = (const struct msm_dp_desc[]) { - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort }, + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000 }, }, .num_descs = 1, };
static const struct msm_dp_config sc7280_dp_cfg = { .descs = (const struct msm_dp_desc[]) { - [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort }, - [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000, .connector_type = DRM_MODE_CONNECTOR_eDP }, + [MSM_DP_CONTROLLER_0] = { .io_start = 0x0ae90000 }, + [MSM_DP_CONTROLLER_1] = { .io_start = 0x0aea0000 }, }, .num_descs = 2, }; @@ -249,7 +248,7 @@ static int dp_display_bind(struct device *dev, struct device *master, dp->dp_display.drm_dev = drm; priv->dp[dp->id] = &dp->dp_display;
- rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type); + rc = dp->parser->parse(dp->parser); if (rc) { DRM_ERROR("device tree parsing failed\n"); goto end; diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c index 5de21f3d0812..044ab0b63b14 100644 --- a/drivers/gpu/drm/msm/dp/dp_parser.c +++ b/drivers/gpu/drm/msm/dp/dp_parser.c @@ -278,7 +278,7 @@ static int dp_parser_find_next_bridge(struct dp_parser *parser) return 0; }
-static int dp_parser_parse(struct dp_parser *parser, int connector_type) +static int dp_parser_parse(struct dp_parser *parser) { int rc = 0;
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h index 4cec851e38d9..1f036dd3e224 100644 --- a/drivers/gpu/drm/msm/dp/dp_parser.h +++ b/drivers/gpu/drm/msm/dp/dp_parser.h @@ -125,7 +125,7 @@ struct dp_parser { u32 max_dp_lanes; struct drm_bridge *next_bridge;
- int (*parse)(struct dp_parser *parser, int connector_type); + int (*parse)(struct dp_parser *parser); };
/**
Quoting Dmitry Baryshkov (2022-01-06 18:01:25)
I noticed that commit 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable") conflicts with the panel-edp (panel bridge) support. Both bridges will try to attach directly to the drm encoder itself. I started writing lengthy letter describing what is broken and how it should be fixed. Then at some point I stopped and quickly coded this RFC (which is compile-tested only).
Comments and tests (on both DP and eDP setups) are more than welcome.
There are some DP patches dribbling in every day or so and it's really hard to follow along. I asked Kuogee to resend all outstanding patches as a single series but that hasn't happened. I'm not super interested in reviewing/testing out these patches until the outstanding patches for DP on the list are reviewed and landed. Have you looked at those patches? See [1] for an example.
The following changes since commit 6ed95285382d6f90a3c3a11d5806a5eb7db715c3:
drm/msm/a5xx: Fix missing CP_PROTECT for SMMU on A540 (2021-12-17 15:09:46 -0800)
are available in the Git repository at:
https://git.linaro.org/people/dmitry.baryshkov/kernel.git msm-dp-bridges
for you to fetch changes up to 7eff304d50ba520e9193a293a8e42bbd9d7aa0e1:
drm/msm/dp: stop carying about the connector type (2022-01-07 04:56:06 +0300)
Dmitry Baryshkov (7): drm/msm/dp: fix panel bridge attachment drm/msm/dp: support attaching bridges to the DP encoder drm/msm/dp: replace dp_connector with drm_bridge_connector drm/msm/dp: remove extra wrappers and public functions drm/msm/dp: remove unused stubs drm/msm/dp: remove dp_display_en/disable prototypes and data argument drm/msm/dp: stop carying about the connector type
drivers/gpu/drm/msm/Makefile | 1 - drivers/gpu/drm/msm/dp/dp_display.c | 263 ++++++++++++++++++++++++++---------- drivers/gpu/drm/msm/dp/dp_display.h | 5 +- drivers/gpu/drm/msm/dp/dp_drm.c | 250 ---------------------------------- drivers/gpu/drm/msm/dp/dp_parser.c | 28 ++-- drivers/gpu/drm/msm/dp/dp_parser.h | 4 +- drivers/gpu/drm/msm/msm_drv.h | 32 +---- 7 files changed, 203 insertions(+), 380 deletions(-) delete mode 100644 drivers/gpu/drm/msm/dp/dp_drm.c
[1] https://lore.kernel.org/r/1640220845-25266-1-git-send-email-quic_khsieh@quic...
On 07/01/2022 05:16, Stephen Boyd wrote:
Quoting Dmitry Baryshkov (2022-01-06 18:01:25)
I noticed that commit 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable") conflicts with the panel-edp (panel bridge) support. Both bridges will try to attach directly to the drm encoder itself. I started writing lengthy letter describing what is broken and how it should be fixed. Then at some point I stopped and quickly coded this RFC (which is compile-tested only).
Comments and tests (on both DP and eDP setups) are more than welcome.
There are some DP patches dribbling in every day or so and it's really hard to follow along. I asked Kuogee to resend all outstanding patches as a single series but that hasn't happened. I'm not super interested in reviewing/testing out these patches until the outstanding patches for DP on the list are reviewed and landed. Have you looked at those patches?
I haven't been following the DP patches. Well, in fact I was mostly stopping myself from looking onto the DP driver and getting elbow deep in it. Partially because some of the patches circulating on the list were clear hacks (e.g. PHY timeouts). Some would be too complex to review them without deep diving into DP. Most of my attention (and spare time) goes to the DPU/DSI/MDP5 (and to lesser extent MDP4/HDMI) drives.
With regards to this patch series, the patch 1 is probably most important (and might warrant sending it separately), as it should fix eDP support for Bjorn.
So, initially I wrote just patch 1. And then the surrounding code immediately prompted me to update the rest of the drm glue code. Elbow deep, as I said. Patch 7 might be a bit advantageous (and maybe I should remove it in future),
See [1] for an example.
I think most of the patches circulating through the list are irrelevant to this patch series, as they do not touch the drm glue code.
The following changes since commit 6ed95285382d6f90a3c3a11d5806a5eb7db715c3:
drm/msm/a5xx: Fix missing CP_PROTECT for SMMU on A540 (2021-12-17 15:09:46 -0800)
are available in the Git repository at:
https://git.linaro.org/people/dmitry.baryshkov/kernel.git msm-dp-bridges
for you to fetch changes up to 7eff304d50ba520e9193a293a8e42bbd9d7aa0e1:
drm/msm/dp: stop carying about the connector type (2022-01-07 04:56:06 +0300)
Dmitry Baryshkov (7): drm/msm/dp: fix panel bridge attachment drm/msm/dp: support attaching bridges to the DP encoder drm/msm/dp: replace dp_connector with drm_bridge_connector drm/msm/dp: remove extra wrappers and public functions drm/msm/dp: remove unused stubs drm/msm/dp: remove dp_display_en/disable prototypes and data argument drm/msm/dp: stop carying about the connector type
drivers/gpu/drm/msm/Makefile | 1 - drivers/gpu/drm/msm/dp/dp_display.c | 263 ++++++++++++++++++++++++++---------- drivers/gpu/drm/msm/dp/dp_display.h | 5 +- drivers/gpu/drm/msm/dp/dp_drm.c | 250 ---------------------------------- drivers/gpu/drm/msm/dp/dp_parser.c | 28 ++-- drivers/gpu/drm/msm/dp/dp_parser.h | 4 +- drivers/gpu/drm/msm/msm_drv.h | 32 +---- 7 files changed, 203 insertions(+), 380 deletions(-) delete mode 100644 drivers/gpu/drm/msm/dp/dp_drm.c
[1] https://lore.kernel.org/r/1640220845-25266-1-git-send-email-quic_khsieh@quic...
dri-devel@lists.freedesktop.org
-
Dmitry Baryshkov -
Kuogee Hsieh -
Stephen Boyd