* [PATCH v9 0/4] Add support for the eDP panel over aux_bus
@ 2022-04-22 9:11 ` Sankeerth Billakanti
0 siblings, 0 replies; 44+ messages in thread
From: Sankeerth Billakanti @ 2022-04-22 9:11 UTC (permalink / raw)
To: dri-devel, linux-arm-msm, freedreno, linux-kernel, devicetree
Cc: quic_kalyant, Sankeerth Billakanti, dianders, bjorn.andersson,
quic_vproddut, airlied, quic_abhinavk, steev, swboyd, seanpaul,
dmitry.baryshkov, quic_aravindh, quic_khsieh, sean
This series adds support for generic eDP panel over aux_bus.
These changes are dependent on the following patches:
https://patchwork.kernel.org/project/linux-arm-msm/patch/20220211224006.1797846-5-dmitry.baryshkov@linaro.org/
https://patchwork.kernel.org/project/linux-arm-msm/patch/20220211224006.1797846-6-dmitry.baryshkov@linaro.org/
Sankeerth Billakanti (4):
drm/msm/dp: Add eDP support via aux_bus
drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
drm/msm/dp: wait for hpd high before aux transaction
drm/msm/dp: Support the eDP modes given by panel
drivers/gpu/drm/msm/dp/dp_aux.c | 21 +++++++-
drivers/gpu/drm/msm/dp/dp_aux.h | 3 +-
drivers/gpu/drm/msm/dp/dp_catalog.c | 29 +++++++---
drivers/gpu/drm/msm/dp/dp_catalog.h | 1 +
drivers/gpu/drm/msm/dp/dp_display.c | 105 +++++++++++++++++++++++++++++++++---
drivers/gpu/drm/msm/dp/dp_display.h | 1 +
drivers/gpu/drm/msm/dp/dp_drm.c | 21 ++++++--
drivers/gpu/drm/msm/dp/dp_parser.c | 23 +-------
drivers/gpu/drm/msm/dp/dp_parser.h | 13 ++++-
9 files changed, 177 insertions(+), 40 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v9 0/4] Add support for the eDP panel over aux_bus
@ 2022-04-22 9:11 ` Sankeerth Billakanti
0 siblings, 0 replies; 44+ messages in thread
From: Sankeerth Billakanti @ 2022-04-22 9:11 UTC (permalink / raw)
To: dri-devel, linux-arm-msm, freedreno, linux-kernel, devicetree
Cc: Sankeerth Billakanti, robdclark, seanpaul, swboyd, quic_kalyant,
quic_abhinavk, dianders, quic_khsieh, bjorn.andersson, sean,
airlied, daniel, dmitry.baryshkov, quic_vproddut, quic_aravindh,
steev
This series adds support for generic eDP panel over aux_bus.
These changes are dependent on the following patches:
https://patchwork.kernel.org/project/linux-arm-msm/patch/20220211224006.1797846-5-dmitry.baryshkov@linaro.org/
https://patchwork.kernel.org/project/linux-arm-msm/patch/20220211224006.1797846-6-dmitry.baryshkov@linaro.org/
Sankeerth Billakanti (4):
drm/msm/dp: Add eDP support via aux_bus
drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
drm/msm/dp: wait for hpd high before aux transaction
drm/msm/dp: Support the eDP modes given by panel
drivers/gpu/drm/msm/dp/dp_aux.c | 21 +++++++-
drivers/gpu/drm/msm/dp/dp_aux.h | 3 +-
drivers/gpu/drm/msm/dp/dp_catalog.c | 29 +++++++---
drivers/gpu/drm/msm/dp/dp_catalog.h | 1 +
drivers/gpu/drm/msm/dp/dp_display.c | 105 +++++++++++++++++++++++++++++++++---
drivers/gpu/drm/msm/dp/dp_display.h | 1 +
drivers/gpu/drm/msm/dp/dp_drm.c | 21 ++++++--
drivers/gpu/drm/msm/dp/dp_parser.c | 23 +-------
drivers/gpu/drm/msm/dp/dp_parser.h | 13 ++++-
9 files changed, 177 insertions(+), 40 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v9 1/4] drm/msm/dp: Add eDP support via aux_bus
2022-04-22 9:11 ` Sankeerth Billakanti
@ 2022-04-22 9:11 ` Sankeerth Billakanti
-1 siblings, 0 replies; 44+ messages in thread
From: Sankeerth Billakanti @ 2022-04-22 9:11 UTC (permalink / raw)
To: dri-devel, linux-arm-msm, freedreno, linux-kernel, devicetree
Cc: quic_kalyant, Sankeerth Billakanti, dianders, bjorn.andersson,
quic_vproddut, airlied, quic_abhinavk, steev, swboyd, seanpaul,
dmitry.baryshkov, quic_aravindh, quic_khsieh, sean
This patch adds support for generic eDP sink through aux_bus. The eDP/DP
controller driver should support aux transactions originating from the
panel-edp driver and hence should be initialized and ready.
The panel bridge supporting the panel should be ready before the bridge
connector is initialized. The generic panel probe needs the controller
resources to be enabled to support the aux transactions originating from
the panel probe.
Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
---
Changes in v9:
- add comments for panel probe
- modify the error handling checks
Changes in v8:
- handle corner cases
- add comment for the bridge ops
Changes in v7:
- aux_bus is mandatory for eDP
- connector type check modified to just check for eDP
Changes in v6:
- Remove initialization
- Fix aux_bus node leak
- Split the patches
drivers/gpu/drm/msm/dp/dp_display.c | 73 +++++++++++++++++++++++++++++++++++--
drivers/gpu/drm/msm/dp/dp_display.h | 1 +
drivers/gpu/drm/msm/dp/dp_drm.c | 21 +++++++++--
drivers/gpu/drm/msm/dp/dp_parser.c | 23 +-----------
drivers/gpu/drm/msm/dp/dp_parser.h | 13 ++++++-
5 files changed, 101 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index d7a19d6..055681a 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -10,6 +10,7 @@
#include <linux/component.h>
#include <linux/of_irq.h>
#include <linux/delay.h>
+#include <drm/dp/drm_dp_aux_bus.h>
#include "msm_drv.h"
#include "msm_kms.h"
@@ -259,14 +260,12 @@ 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;
}
- dp->dp_display.next_bridge = dp->parser->next_bridge;
-
dp->aux->drm_dev = drm;
rc = dp_aux_register(dp->aux);
if (rc) {
@@ -1319,6 +1318,8 @@ static int dp_display_probe(struct platform_device *pdev)
dp->pdev = pdev;
dp->name = "drm_dp";
dp->dp_display.connector_type = desc->connector_type;
+ dp->dp_display.is_edp =
+ (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP);
rc = dp_init_sub_modules(dp);
if (rc) {
@@ -1508,7 +1509,8 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
dp_hpd_event_setup(dp);
- dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
+ if (!dp_display->is_edp)
+ dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
}
void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
@@ -1530,6 +1532,65 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
}
}
+static int dp_display_get_next_bridge(struct msm_dp *dp)
+{
+ int rc;
+ struct dp_display_private *dp_priv;
+ struct device_node *aux_bus;
+ struct device *dev;
+
+ dp_priv = container_of(dp, struct dp_display_private, dp_display);
+ dev = &dp_priv->pdev->dev;
+ aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
+
+ if (aux_bus && dp->is_edp) {
+ dp_display_host_init(dp_priv);
+ dp_catalog_ctrl_hpd_config(dp_priv->catalog);
+ dp_display_host_phy_init(dp_priv);
+ enable_irq(dp_priv->irq);
+
+ /*
+ * The code below assumes that the panel will finish probing
+ * by the time devm_of_dp_aux_populate_ep_devices() returns.
+ * This isn't a great assumption since it will fail if the
+ * panel driver is probed asynchronously but is the best we
+ * can do without a bigger driver reorganization.
+ */
+ rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
+ of_node_put(aux_bus);
+ if (rc)
+ goto error;
+ } else if (dp->is_edp) {
+ DRM_ERROR("eDP aux_bus not found\n");
+ return -ENODEV;
+ }
+
+ /*
+ * External bridges are mandatory for eDP interfaces: one has to
+ * provide at least an eDP panel (which gets wrapped into panel-bridge).
+ *
+ * For DisplayPort interfaces external bridges are optional, so
+ * silently ignore an error if one is not present (-ENODEV).
+ */
+ rc = dp_parser_find_next_bridge(dp_priv->parser);
+ if (!dp->is_edp && rc == -ENODEV)
+ return 0;
+ else if (rc)
+ goto error;
+
+ dp->next_bridge = dp_priv->parser->next_bridge;
+
+ return 0;
+
+error:
+ if (dp->is_edp) {
+ disable_irq(dp_priv->irq);
+ dp_display_host_phy_exit(dp_priv);
+ dp_display_host_deinit(dp_priv);
+ }
+ return rc;
+}
+
int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
struct drm_encoder *encoder)
{
@@ -1553,6 +1614,10 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
dp_display->encoder = encoder;
+ ret = dp_display_get_next_bridge(dp_display);
+ if (ret)
+ return ret;
+
dp_display->bridge = dp_bridge_init(dp_display, dev, encoder);
if (IS_ERR(dp_display->bridge)) {
ret = PTR_ERR(dp_display->bridge);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
index 49a1d89..1377cc3 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -21,6 +21,7 @@ struct msm_dp {
bool audio_enabled;
bool power_on;
unsigned int connector_type;
+ bool is_edp;
hdmi_codec_plugged_cb plugged_cb;
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 7ce1aca..8a75c55 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -114,10 +114,23 @@ struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct drm_device *
bridge->funcs = &dp_bridge_ops;
bridge->type = dp_display->connector_type;
- bridge->ops =
- DRM_BRIDGE_OP_DETECT |
- DRM_BRIDGE_OP_HPD |
- DRM_BRIDGE_OP_MODES;
+ /*
+ * Many ops only make sense for DP. Why?
+ * - Detect/HPD are used by DRM to know if a display is _physically_
+ * there, not whether the display is powered on / finished initting.
+ * On eDP we assume the display is always there because you can't
+ * know until power is applied. If we don't implement the ops DRM will
+ * assume our display is always there.
+ * - Currently eDP mode reading is driven by the panel driver. This
+ * allows the panel driver to properly power itself on to read the
+ * modes.
+ */
+ if (!dp_display->is_edp) {
+ 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) {
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
index 1056b8d..4bdbf91 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -265,7 +265,7 @@ static int dp_parser_clock(struct dp_parser *parser)
return 0;
}
-static int dp_parser_find_next_bridge(struct dp_parser *parser)
+int dp_parser_find_next_bridge(struct dp_parser *parser)
{
struct device *dev = &parser->pdev->dev;
struct drm_bridge *bridge;
@@ -279,7 +279,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;
@@ -300,25 +300,6 @@ static int dp_parser_parse(struct dp_parser *parser, int connector_type)
if (rc)
return rc;
- /*
- * External bridges are mandatory for eDP interfaces: one has to
- * provide at least an eDP panel (which gets wrapped into panel-bridge).
- *
- * For DisplayPort interfaces external bridges are optional, so
- * silently ignore an error if one is not present (-ENODEV).
- */
- rc = dp_parser_find_next_bridge(parser);
- if (rc == -ENODEV) {
- if (connector_type == DRM_MODE_CONNECTOR_eDP) {
- DRM_ERROR("eDP: next bridge is not present\n");
- return rc;
- }
- } else if (rc) {
- if (rc != -EPROBE_DEFER)
- DRM_ERROR("DP: error parsing next bridge: %d\n", rc);
- return rc;
- }
-
/* Map the corresponding regulator information according to
* version. Currently, since we only have one supported platform,
* mapping the regulator directly.
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
index d371bae..950416c 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);
};
/**
@@ -141,4 +141,15 @@ struct dp_parser {
*/
struct dp_parser *dp_parser_get(struct platform_device *pdev);
+/**
+ * dp_parser_find_next_bridge() - find an additional bridge to DP
+ *
+ * @parser: dp_parser data from client
+ * return: 0 if able to get the bridge else return an error code
+ *
+ * This function is used to find any additional bridge attached to
+ * the DP controller. The eDP interface requires a panel bridge.
+ */
+int dp_parser_find_next_bridge(struct dp_parser *parser);
+
#endif
--
2.7.4
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v9 1/4] drm/msm/dp: Add eDP support via aux_bus
@ 2022-04-22 9:11 ` Sankeerth Billakanti
0 siblings, 0 replies; 44+ messages in thread
From: Sankeerth Billakanti @ 2022-04-22 9:11 UTC (permalink / raw)
To: dri-devel, linux-arm-msm, freedreno, linux-kernel, devicetree
Cc: Sankeerth Billakanti, robdclark, seanpaul, swboyd, quic_kalyant,
quic_abhinavk, dianders, quic_khsieh, bjorn.andersson, sean,
airlied, daniel, dmitry.baryshkov, quic_vproddut, quic_aravindh,
steev
This patch adds support for generic eDP sink through aux_bus. The eDP/DP
controller driver should support aux transactions originating from the
panel-edp driver and hence should be initialized and ready.
The panel bridge supporting the panel should be ready before the bridge
connector is initialized. The generic panel probe needs the controller
resources to be enabled to support the aux transactions originating from
the panel probe.
Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
---
Changes in v9:
- add comments for panel probe
- modify the error handling checks
Changes in v8:
- handle corner cases
- add comment for the bridge ops
Changes in v7:
- aux_bus is mandatory for eDP
- connector type check modified to just check for eDP
Changes in v6:
- Remove initialization
- Fix aux_bus node leak
- Split the patches
drivers/gpu/drm/msm/dp/dp_display.c | 73 +++++++++++++++++++++++++++++++++++--
drivers/gpu/drm/msm/dp/dp_display.h | 1 +
drivers/gpu/drm/msm/dp/dp_drm.c | 21 +++++++++--
drivers/gpu/drm/msm/dp/dp_parser.c | 23 +-----------
drivers/gpu/drm/msm/dp/dp_parser.h | 13 ++++++-
5 files changed, 101 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index d7a19d6..055681a 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -10,6 +10,7 @@
#include <linux/component.h>
#include <linux/of_irq.h>
#include <linux/delay.h>
+#include <drm/dp/drm_dp_aux_bus.h>
#include "msm_drv.h"
#include "msm_kms.h"
@@ -259,14 +260,12 @@ 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;
}
- dp->dp_display.next_bridge = dp->parser->next_bridge;
-
dp->aux->drm_dev = drm;
rc = dp_aux_register(dp->aux);
if (rc) {
@@ -1319,6 +1318,8 @@ static int dp_display_probe(struct platform_device *pdev)
dp->pdev = pdev;
dp->name = "drm_dp";
dp->dp_display.connector_type = desc->connector_type;
+ dp->dp_display.is_edp =
+ (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP);
rc = dp_init_sub_modules(dp);
if (rc) {
@@ -1508,7 +1509,8 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
dp_hpd_event_setup(dp);
- dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
+ if (!dp_display->is_edp)
+ dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
}
void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
@@ -1530,6 +1532,65 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
}
}
+static int dp_display_get_next_bridge(struct msm_dp *dp)
+{
+ int rc;
+ struct dp_display_private *dp_priv;
+ struct device_node *aux_bus;
+ struct device *dev;
+
+ dp_priv = container_of(dp, struct dp_display_private, dp_display);
+ dev = &dp_priv->pdev->dev;
+ aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
+
+ if (aux_bus && dp->is_edp) {
+ dp_display_host_init(dp_priv);
+ dp_catalog_ctrl_hpd_config(dp_priv->catalog);
+ dp_display_host_phy_init(dp_priv);
+ enable_irq(dp_priv->irq);
+
+ /*
+ * The code below assumes that the panel will finish probing
+ * by the time devm_of_dp_aux_populate_ep_devices() returns.
+ * This isn't a great assumption since it will fail if the
+ * panel driver is probed asynchronously but is the best we
+ * can do without a bigger driver reorganization.
+ */
+ rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
+ of_node_put(aux_bus);
+ if (rc)
+ goto error;
+ } else if (dp->is_edp) {
+ DRM_ERROR("eDP aux_bus not found\n");
+ return -ENODEV;
+ }
+
+ /*
+ * External bridges are mandatory for eDP interfaces: one has to
+ * provide at least an eDP panel (which gets wrapped into panel-bridge).
+ *
+ * For DisplayPort interfaces external bridges are optional, so
+ * silently ignore an error if one is not present (-ENODEV).
+ */
+ rc = dp_parser_find_next_bridge(dp_priv->parser);
+ if (!dp->is_edp && rc == -ENODEV)
+ return 0;
+ else if (rc)
+ goto error;
+
+ dp->next_bridge = dp_priv->parser->next_bridge;
+
+ return 0;
+
+error:
+ if (dp->is_edp) {
+ disable_irq(dp_priv->irq);
+ dp_display_host_phy_exit(dp_priv);
+ dp_display_host_deinit(dp_priv);
+ }
+ return rc;
+}
+
int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
struct drm_encoder *encoder)
{
@@ -1553,6 +1614,10 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
dp_display->encoder = encoder;
+ ret = dp_display_get_next_bridge(dp_display);
+ if (ret)
+ return ret;
+
dp_display->bridge = dp_bridge_init(dp_display, dev, encoder);
if (IS_ERR(dp_display->bridge)) {
ret = PTR_ERR(dp_display->bridge);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
index 49a1d89..1377cc3 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -21,6 +21,7 @@ struct msm_dp {
bool audio_enabled;
bool power_on;
unsigned int connector_type;
+ bool is_edp;
hdmi_codec_plugged_cb plugged_cb;
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 7ce1aca..8a75c55 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -114,10 +114,23 @@ struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct drm_device *
bridge->funcs = &dp_bridge_ops;
bridge->type = dp_display->connector_type;
- bridge->ops =
- DRM_BRIDGE_OP_DETECT |
- DRM_BRIDGE_OP_HPD |
- DRM_BRIDGE_OP_MODES;
+ /*
+ * Many ops only make sense for DP. Why?
+ * - Detect/HPD are used by DRM to know if a display is _physically_
+ * there, not whether the display is powered on / finished initting.
+ * On eDP we assume the display is always there because you can't
+ * know until power is applied. If we don't implement the ops DRM will
+ * assume our display is always there.
+ * - Currently eDP mode reading is driven by the panel driver. This
+ * allows the panel driver to properly power itself on to read the
+ * modes.
+ */
+ if (!dp_display->is_edp) {
+ 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) {
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
index 1056b8d..4bdbf91 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -265,7 +265,7 @@ static int dp_parser_clock(struct dp_parser *parser)
return 0;
}
-static int dp_parser_find_next_bridge(struct dp_parser *parser)
+int dp_parser_find_next_bridge(struct dp_parser *parser)
{
struct device *dev = &parser->pdev->dev;
struct drm_bridge *bridge;
@@ -279,7 +279,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;
@@ -300,25 +300,6 @@ static int dp_parser_parse(struct dp_parser *parser, int connector_type)
if (rc)
return rc;
- /*
- * External bridges are mandatory for eDP interfaces: one has to
- * provide at least an eDP panel (which gets wrapped into panel-bridge).
- *
- * For DisplayPort interfaces external bridges are optional, so
- * silently ignore an error if one is not present (-ENODEV).
- */
- rc = dp_parser_find_next_bridge(parser);
- if (rc == -ENODEV) {
- if (connector_type == DRM_MODE_CONNECTOR_eDP) {
- DRM_ERROR("eDP: next bridge is not present\n");
- return rc;
- }
- } else if (rc) {
- if (rc != -EPROBE_DEFER)
- DRM_ERROR("DP: error parsing next bridge: %d\n", rc);
- return rc;
- }
-
/* Map the corresponding regulator information according to
* version. Currently, since we only have one supported platform,
* mapping the regulator directly.
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
index d371bae..950416c 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);
};
/**
@@ -141,4 +141,15 @@ struct dp_parser {
*/
struct dp_parser *dp_parser_get(struct platform_device *pdev);
+/**
+ * dp_parser_find_next_bridge() - find an additional bridge to DP
+ *
+ * @parser: dp_parser data from client
+ * return: 0 if able to get the bridge else return an error code
+ *
+ * This function is used to find any additional bridge attached to
+ * the DP controller. The eDP interface requires a panel bridge.
+ */
+int dp_parser_find_next_bridge(struct dp_parser *parser);
+
#endif
--
2.7.4
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
2022-04-22 9:11 ` Sankeerth Billakanti
@ 2022-04-22 9:11 ` Sankeerth Billakanti
-1 siblings, 0 replies; 44+ messages in thread
From: Sankeerth Billakanti @ 2022-04-22 9:11 UTC (permalink / raw)
To: dri-devel, linux-arm-msm, freedreno, linux-kernel, devicetree
Cc: quic_kalyant, Sankeerth Billakanti, dianders, bjorn.andersson,
quic_vproddut, airlied, quic_abhinavk, steev, swboyd, seanpaul,
dmitry.baryshkov, quic_aravindh, quic_khsieh, sean
The panel-edp enables the eDP panel power during probe, get_modes
and pre-enable. The eDP connect and disconnect interrupts for the eDP/DP
controller are directly dependent on panel power. As eDP display can be
assumed as always connected, the controller driver can skip the eDP
connect and disconnect interrupts. Any disruption in the link status
will be indicated via the IRQ_HPD interrupts.
So, the eDP controller driver can just enable the IRQ_HPD and replug
interrupts. The DP controller driver still needs to enable all the
interrupts.
Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
---
Changes in v9:
- add comment explaining the interrupt status register
Changes in v8:
- add comment explaining the interrupt status return
Changes in v7:
- reordered the patch in the series
- modified the return statement for isr
- connector check modified to just check for eDP
drivers/gpu/drm/msm/dp/dp_catalog.c | 16 ++++++++++------
drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++-
2 files changed, 31 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index fac815f..df9670d 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -569,10 +569,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog)
u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
- /* enable HPD plug and unplug interrupts */
- dp_catalog_hpd_config_intr(dp_catalog,
- DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK, true);
-
/* Configure REFTIMER and enable it */
reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
@@ -599,13 +595,21 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog)
{
struct dp_catalog_private *catalog = container_of(dp_catalog,
struct dp_catalog_private, dp_catalog);
- int isr = 0;
+ int isr, mask;
isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
(isr & DP_DP_HPD_INT_MASK));
+ mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
- return isr;
+ /*
+ * We only want to return interrupts that are unmasked to the caller.
+ * However, the interrupt status field also contains other
+ * informational bits about the HPD state status, so we only mask
+ * out the part of the register that tells us about which interrupts
+ * are pending.
+ */
+ return isr & (mask | ~DP_DP_HPD_INT_MASK);
}
int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 055681a..dea4de9 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -683,7 +683,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
dp_display_handle_plugged_change(&dp->dp_display, false);
/* enable HDP plug interrupt to prepare for next plugin */
- dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
+ if (!dp->dp_display.is_edp)
+ dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
DRM_DEBUG_DP("After, type=%d hpd_state=%d\n",
dp->dp_display.connector_type, state);
@@ -1096,6 +1097,13 @@ static void dp_display_config_hpd(struct dp_display_private *dp)
dp_display_host_init(dp);
dp_catalog_ctrl_hpd_config(dp->catalog);
+ /* Enable plug and unplug interrupts only for external DisplayPort */
+ if (!dp->dp_display.is_edp)
+ dp_catalog_hpd_config_intr(dp->catalog,
+ DP_DP_HPD_PLUG_INT_MASK |
+ DP_DP_HPD_UNPLUG_INT_MASK,
+ true);
+
/* Enable interrupt first time
* we are leaving dp clocks on during disconnect
* and never disable interrupt
@@ -1381,6 +1389,12 @@ static int dp_pm_resume(struct device *dev)
dp_catalog_ctrl_hpd_config(dp->catalog);
+ if (!dp->dp_display.is_edp)
+ dp_catalog_hpd_config_intr(dp->catalog,
+ DP_DP_HPD_PLUG_INT_MASK |
+ DP_DP_HPD_UNPLUG_INT_MASK,
+ true);
+
if (dp_catalog_link_is_connected(dp->catalog)) {
/*
* set sink to normal operation mode -- D0
@@ -1659,6 +1673,9 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
return;
}
+ if (dp->is_edp)
+ dp_hpd_plug_handle(dp_display, 0);
+
mutex_lock(&dp_display->event_mutex);
/* stop sentinel checking */
@@ -1723,6 +1740,9 @@ void dp_bridge_post_disable(struct drm_bridge *drm_bridge)
dp_display = container_of(dp, struct dp_display_private, dp_display);
+ if (dp->is_edp)
+ dp_hpd_unplug_handle(dp_display, 0);
+
mutex_lock(&dp_display->event_mutex);
/* stop sentinel checking */
--
2.7.4
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
@ 2022-04-22 9:11 ` Sankeerth Billakanti
0 siblings, 0 replies; 44+ messages in thread
From: Sankeerth Billakanti @ 2022-04-22 9:11 UTC (permalink / raw)
To: dri-devel, linux-arm-msm, freedreno, linux-kernel, devicetree
Cc: Sankeerth Billakanti, robdclark, seanpaul, swboyd, quic_kalyant,
quic_abhinavk, dianders, quic_khsieh, bjorn.andersson, sean,
airlied, daniel, dmitry.baryshkov, quic_vproddut, quic_aravindh,
steev
The panel-edp enables the eDP panel power during probe, get_modes
and pre-enable. The eDP connect and disconnect interrupts for the eDP/DP
controller are directly dependent on panel power. As eDP display can be
assumed as always connected, the controller driver can skip the eDP
connect and disconnect interrupts. Any disruption in the link status
will be indicated via the IRQ_HPD interrupts.
So, the eDP controller driver can just enable the IRQ_HPD and replug
interrupts. The DP controller driver still needs to enable all the
interrupts.
Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
---
Changes in v9:
- add comment explaining the interrupt status register
Changes in v8:
- add comment explaining the interrupt status return
Changes in v7:
- reordered the patch in the series
- modified the return statement for isr
- connector check modified to just check for eDP
drivers/gpu/drm/msm/dp/dp_catalog.c | 16 ++++++++++------
drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++-
2 files changed, 31 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index fac815f..df9670d 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -569,10 +569,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog)
u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
- /* enable HPD plug and unplug interrupts */
- dp_catalog_hpd_config_intr(dp_catalog,
- DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK, true);
-
/* Configure REFTIMER and enable it */
reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
@@ -599,13 +595,21 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog)
{
struct dp_catalog_private *catalog = container_of(dp_catalog,
struct dp_catalog_private, dp_catalog);
- int isr = 0;
+ int isr, mask;
isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
(isr & DP_DP_HPD_INT_MASK));
+ mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
- return isr;
+ /*
+ * We only want to return interrupts that are unmasked to the caller.
+ * However, the interrupt status field also contains other
+ * informational bits about the HPD state status, so we only mask
+ * out the part of the register that tells us about which interrupts
+ * are pending.
+ */
+ return isr & (mask | ~DP_DP_HPD_INT_MASK);
}
int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 055681a..dea4de9 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -683,7 +683,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
dp_display_handle_plugged_change(&dp->dp_display, false);
/* enable HDP plug interrupt to prepare for next plugin */
- dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
+ if (!dp->dp_display.is_edp)
+ dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
DRM_DEBUG_DP("After, type=%d hpd_state=%d\n",
dp->dp_display.connector_type, state);
@@ -1096,6 +1097,13 @@ static void dp_display_config_hpd(struct dp_display_private *dp)
dp_display_host_init(dp);
dp_catalog_ctrl_hpd_config(dp->catalog);
+ /* Enable plug and unplug interrupts only for external DisplayPort */
+ if (!dp->dp_display.is_edp)
+ dp_catalog_hpd_config_intr(dp->catalog,
+ DP_DP_HPD_PLUG_INT_MASK |
+ DP_DP_HPD_UNPLUG_INT_MASK,
+ true);
+
/* Enable interrupt first time
* we are leaving dp clocks on during disconnect
* and never disable interrupt
@@ -1381,6 +1389,12 @@ static int dp_pm_resume(struct device *dev)
dp_catalog_ctrl_hpd_config(dp->catalog);
+ if (!dp->dp_display.is_edp)
+ dp_catalog_hpd_config_intr(dp->catalog,
+ DP_DP_HPD_PLUG_INT_MASK |
+ DP_DP_HPD_UNPLUG_INT_MASK,
+ true);
+
if (dp_catalog_link_is_connected(dp->catalog)) {
/*
* set sink to normal operation mode -- D0
@@ -1659,6 +1673,9 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
return;
}
+ if (dp->is_edp)
+ dp_hpd_plug_handle(dp_display, 0);
+
mutex_lock(&dp_display->event_mutex);
/* stop sentinel checking */
@@ -1723,6 +1740,9 @@ void dp_bridge_post_disable(struct drm_bridge *drm_bridge)
dp_display = container_of(dp, struct dp_display_private, dp_display);
+ if (dp->is_edp)
+ dp_hpd_unplug_handle(dp_display, 0);
+
mutex_lock(&dp_display->event_mutex);
/* stop sentinel checking */
--
2.7.4
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v9 3/4] drm/msm/dp: wait for hpd high before aux transaction
2022-04-22 9:11 ` Sankeerth Billakanti
@ 2022-04-22 9:11 ` Sankeerth Billakanti
-1 siblings, 0 replies; 44+ messages in thread
From: Sankeerth Billakanti @ 2022-04-22 9:11 UTC (permalink / raw)
To: dri-devel, linux-arm-msm, freedreno, linux-kernel, devicetree
Cc: quic_kalyant, Sankeerth Billakanti, dianders, bjorn.andersson,
quic_vproddut, airlied, quic_abhinavk, steev, swboyd, seanpaul,
dmitry.baryshkov, quic_aravindh, quic_khsieh, sean
The source device should ensure the sink is ready before proceeding to
read the sink capability or perform any aux transactions. The sink
will indicate its readiness by asserting the HPD line. The controller
driver needs to wait for the hpd line to be asserted by the sink before
it performs any aux transactions.
The eDP sink is assumed to be always connected. It needs power from the
source and its HPD line will be asserted only after the panel is powered
on. The panel power will be enabled from the panel-edp driver and only
after that, the hpd line will be asserted.
Whereas for DP, the sink can be hotplugged and unplugged anytime. The hpd
line gets asserted to indicate the sink is connected and ready. Hence
there is no need to wait for the hpd line to be asserted for a DP sink.
Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
These changes may be handled in is_hpd_asserted when
https://lore.kernel.org/r/20220408193536.RFC.3.Icf57bb12233a47727013c6ab69eebf803e22ebc1@changeid/
is accepted upstream.
Changes in v9:
- none
Changes in v8:
- correct the indentation
Changes in v7:
- add a comment to say why the wait is done for eDP
- correct the commit text
Changes in v6:
- Wait for hpd high only for eDP
- Split into smaller patches
drivers/gpu/drm/msm/dp/dp_aux.c | 21 ++++++++++++++++++++-
drivers/gpu/drm/msm/dp/dp_aux.h | 3 ++-
drivers/gpu/drm/msm/dp/dp_catalog.c | 13 +++++++++++++
drivers/gpu/drm/msm/dp/dp_catalog.h | 1 +
drivers/gpu/drm/msm/dp/dp_display.c | 2 +-
5 files changed, 37 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 6d36f63..d030a93 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -34,6 +34,7 @@ struct dp_aux_private {
bool no_send_addr;
bool no_send_stop;
bool initted;
+ bool is_edp;
u32 offset;
u32 segment;
@@ -337,6 +338,22 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
goto exit;
}
+ /*
+ * For eDP it's important to give a reasonably long wait here for HPD
+ * to be asserted. This is because the panel driver may have _just_
+ * turned on the panel and then tried to do an AUX transfer. The panel
+ * driver has no way of knowing when the panel is ready, so it's up
+ * to us to wait. For DP we never get into this situation so let's
+ * avoid ever doing the extra long wait for DP.
+ */
+ if (aux->is_edp) {
+ ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
+ if (ret) {
+ DRM_DEBUG_DP("Panel not ready for aux transactions\n");
+ goto exit;
+ }
+ }
+
dp_aux_update_offset_and_segment(aux, msg);
dp_aux_transfer_helper(aux, msg, true);
@@ -491,7 +508,8 @@ void dp_aux_unregister(struct drm_dp_aux *dp_aux)
drm_dp_aux_unregister(dp_aux);
}
-struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog)
+struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
+ bool is_edp)
{
struct dp_aux_private *aux;
@@ -506,6 +524,7 @@ struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog)
init_completion(&aux->comp);
aux->cmd_busy = false;
+ aux->is_edp = is_edp;
mutex_init(&aux->mutex);
aux->dev = dev;
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h b/drivers/gpu/drm/msm/dp/dp_aux.h
index 82afc8d..5a50c08 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.h
+++ b/drivers/gpu/drm/msm/dp/dp_aux.h
@@ -16,7 +16,8 @@ void dp_aux_init(struct drm_dp_aux *dp_aux);
void dp_aux_deinit(struct drm_dp_aux *dp_aux);
void dp_aux_reconfig(struct drm_dp_aux *dp_aux);
-struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog);
+struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
+ bool is_edp);
void dp_aux_put(struct drm_dp_aux *aux);
#endif /*__DP_AUX_H_*/
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index df9670d..0c6a96e 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -242,6 +242,19 @@ void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog)
phy_calibrate(phy);
}
+int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog)
+{
+ u32 state;
+ struct dp_catalog_private *catalog = container_of(dp_catalog,
+ struct dp_catalog_private, dp_catalog);
+
+ /* poll for hpd connected status every 2ms and timeout after 500ms */
+ return readl_poll_timeout(catalog->io->dp_controller.aux.base +
+ REG_DP_DP_HPD_INT_STATUS,
+ state, state & DP_DP_HPD_STATE_STATUS_CONNECTED,
+ 2000, 500000);
+}
+
static void dump_regs(void __iomem *base, int len)
{
int i;
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
index 7dea101..45140a3 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.h
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
@@ -84,6 +84,7 @@ int dp_catalog_aux_clear_hw_interrupts(struct dp_catalog *dp_catalog);
void dp_catalog_aux_reset(struct dp_catalog *dp_catalog);
void dp_catalog_aux_enable(struct dp_catalog *dp_catalog, bool enable);
void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog);
+int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog);
u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
/* DP Controller APIs */
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index dea4de9..f197694 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -805,7 +805,7 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
goto error;
}
- dp->aux = dp_aux_get(dev, dp->catalog);
+ dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
if (IS_ERR(dp->aux)) {
rc = PTR_ERR(dp->aux);
DRM_ERROR("failed to initialize aux, rc = %d\n", rc);
--
2.7.4
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v9 3/4] drm/msm/dp: wait for hpd high before aux transaction
@ 2022-04-22 9:11 ` Sankeerth Billakanti
0 siblings, 0 replies; 44+ messages in thread
From: Sankeerth Billakanti @ 2022-04-22 9:11 UTC (permalink / raw)
To: dri-devel, linux-arm-msm, freedreno, linux-kernel, devicetree
Cc: Sankeerth Billakanti, robdclark, seanpaul, swboyd, quic_kalyant,
quic_abhinavk, dianders, quic_khsieh, bjorn.andersson, sean,
airlied, daniel, dmitry.baryshkov, quic_vproddut, quic_aravindh,
steev
The source device should ensure the sink is ready before proceeding to
read the sink capability or perform any aux transactions. The sink
will indicate its readiness by asserting the HPD line. The controller
driver needs to wait for the hpd line to be asserted by the sink before
it performs any aux transactions.
The eDP sink is assumed to be always connected. It needs power from the
source and its HPD line will be asserted only after the panel is powered
on. The panel power will be enabled from the panel-edp driver and only
after that, the hpd line will be asserted.
Whereas for DP, the sink can be hotplugged and unplugged anytime. The hpd
line gets asserted to indicate the sink is connected and ready. Hence
there is no need to wait for the hpd line to be asserted for a DP sink.
Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
These changes may be handled in is_hpd_asserted when
https://lore.kernel.org/r/20220408193536.RFC.3.Icf57bb12233a47727013c6ab69eebf803e22ebc1@changeid/
is accepted upstream.
Changes in v9:
- none
Changes in v8:
- correct the indentation
Changes in v7:
- add a comment to say why the wait is done for eDP
- correct the commit text
Changes in v6:
- Wait for hpd high only for eDP
- Split into smaller patches
drivers/gpu/drm/msm/dp/dp_aux.c | 21 ++++++++++++++++++++-
drivers/gpu/drm/msm/dp/dp_aux.h | 3 ++-
drivers/gpu/drm/msm/dp/dp_catalog.c | 13 +++++++++++++
drivers/gpu/drm/msm/dp/dp_catalog.h | 1 +
drivers/gpu/drm/msm/dp/dp_display.c | 2 +-
5 files changed, 37 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 6d36f63..d030a93 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -34,6 +34,7 @@ struct dp_aux_private {
bool no_send_addr;
bool no_send_stop;
bool initted;
+ bool is_edp;
u32 offset;
u32 segment;
@@ -337,6 +338,22 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
goto exit;
}
+ /*
+ * For eDP it's important to give a reasonably long wait here for HPD
+ * to be asserted. This is because the panel driver may have _just_
+ * turned on the panel and then tried to do an AUX transfer. The panel
+ * driver has no way of knowing when the panel is ready, so it's up
+ * to us to wait. For DP we never get into this situation so let's
+ * avoid ever doing the extra long wait for DP.
+ */
+ if (aux->is_edp) {
+ ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
+ if (ret) {
+ DRM_DEBUG_DP("Panel not ready for aux transactions\n");
+ goto exit;
+ }
+ }
+
dp_aux_update_offset_and_segment(aux, msg);
dp_aux_transfer_helper(aux, msg, true);
@@ -491,7 +508,8 @@ void dp_aux_unregister(struct drm_dp_aux *dp_aux)
drm_dp_aux_unregister(dp_aux);
}
-struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog)
+struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
+ bool is_edp)
{
struct dp_aux_private *aux;
@@ -506,6 +524,7 @@ struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog)
init_completion(&aux->comp);
aux->cmd_busy = false;
+ aux->is_edp = is_edp;
mutex_init(&aux->mutex);
aux->dev = dev;
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h b/drivers/gpu/drm/msm/dp/dp_aux.h
index 82afc8d..5a50c08 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.h
+++ b/drivers/gpu/drm/msm/dp/dp_aux.h
@@ -16,7 +16,8 @@ void dp_aux_init(struct drm_dp_aux *dp_aux);
void dp_aux_deinit(struct drm_dp_aux *dp_aux);
void dp_aux_reconfig(struct drm_dp_aux *dp_aux);
-struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog);
+struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
+ bool is_edp);
void dp_aux_put(struct drm_dp_aux *aux);
#endif /*__DP_AUX_H_*/
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index df9670d..0c6a96e 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -242,6 +242,19 @@ void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog)
phy_calibrate(phy);
}
+int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog)
+{
+ u32 state;
+ struct dp_catalog_private *catalog = container_of(dp_catalog,
+ struct dp_catalog_private, dp_catalog);
+
+ /* poll for hpd connected status every 2ms and timeout after 500ms */
+ return readl_poll_timeout(catalog->io->dp_controller.aux.base +
+ REG_DP_DP_HPD_INT_STATUS,
+ state, state & DP_DP_HPD_STATE_STATUS_CONNECTED,
+ 2000, 500000);
+}
+
static void dump_regs(void __iomem *base, int len)
{
int i;
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
index 7dea101..45140a3 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.h
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
@@ -84,6 +84,7 @@ int dp_catalog_aux_clear_hw_interrupts(struct dp_catalog *dp_catalog);
void dp_catalog_aux_reset(struct dp_catalog *dp_catalog);
void dp_catalog_aux_enable(struct dp_catalog *dp_catalog, bool enable);
void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog);
+int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog);
u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
/* DP Controller APIs */
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index dea4de9..f197694 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -805,7 +805,7 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
goto error;
}
- dp->aux = dp_aux_get(dev, dp->catalog);
+ dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
if (IS_ERR(dp->aux)) {
rc = PTR_ERR(dp->aux);
DRM_ERROR("failed to initialize aux, rc = %d\n", rc);
--
2.7.4
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v9 4/4] drm/msm/dp: Support the eDP modes given by panel
2022-04-22 9:11 ` Sankeerth Billakanti
@ 2022-04-22 9:11 ` Sankeerth Billakanti
-1 siblings, 0 replies; 44+ messages in thread
From: Sankeerth Billakanti @ 2022-04-22 9:11 UTC (permalink / raw)
To: dri-devel, linux-arm-msm, freedreno, linux-kernel, devicetree
Cc: quic_kalyant, Sankeerth Billakanti, dianders, bjorn.andersson,
quic_vproddut, airlied, quic_abhinavk, steev, swboyd, seanpaul,
dmitry.baryshkov, quic_aravindh, quic_khsieh, sean
The eDP controller does not have a reliable way keep panel
powered on to read the sink capabilities. So, the controller
driver cannot validate if a mode can be supported by the
source. We will rely on the panel driver to populate only
the supported modes for now.
Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v9:
- none
Changes in v8:
- add the drm/msm/dp tag in the commit title
drivers/gpu/drm/msm/dp/dp_display.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index f197694..49fac955 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -998,6 +998,14 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge,
return -EINVAL;
}
+ /*
+ * The eDP controller currently does not have a reliable way of
+ * enabling panel power to read sink capabilities. So, we rely
+ * on the panel driver to populate only supported modes for now.
+ */
+ if (dp->is_edp)
+ return MODE_OK;
+
if ((dp->max_pclk_khz <= 0) ||
(dp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) ||
(mode->clock > dp->max_pclk_khz))
--
2.7.4
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v9 4/4] drm/msm/dp: Support the eDP modes given by panel
@ 2022-04-22 9:11 ` Sankeerth Billakanti
0 siblings, 0 replies; 44+ messages in thread
From: Sankeerth Billakanti @ 2022-04-22 9:11 UTC (permalink / raw)
To: dri-devel, linux-arm-msm, freedreno, linux-kernel, devicetree
Cc: Sankeerth Billakanti, robdclark, seanpaul, swboyd, quic_kalyant,
quic_abhinavk, dianders, quic_khsieh, bjorn.andersson, sean,
airlied, daniel, dmitry.baryshkov, quic_vproddut, quic_aravindh,
steev
The eDP controller does not have a reliable way keep panel
powered on to read the sink capabilities. So, the controller
driver cannot validate if a mode can be supported by the
source. We will rely on the panel driver to populate only
the supported modes for now.
Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v9:
- none
Changes in v8:
- add the drm/msm/dp tag in the commit title
drivers/gpu/drm/msm/dp/dp_display.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index f197694..49fac955 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -998,6 +998,14 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge,
return -EINVAL;
}
+ /*
+ * The eDP controller currently does not have a reliable way of
+ * enabling panel power to read sink capabilities. So, we rely
+ * on the panel driver to populate only supported modes for now.
+ */
+ if (dp->is_edp)
+ return MODE_OK;
+
if ((dp->max_pclk_khz <= 0) ||
(dp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) ||
(mode->clock > dp->max_pclk_khz))
--
2.7.4
^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH v9 1/4] drm/msm/dp: Add eDP support via aux_bus
2022-04-22 9:11 ` Sankeerth Billakanti
@ 2022-04-22 14:03 ` Doug Anderson
-1 siblings, 0 replies; 44+ messages in thread
From: Doug Anderson @ 2022-04-22 14:03 UTC (permalink / raw)
To: Sankeerth Billakanti
Cc: dri-devel, linux-arm-msm, freedreno, LKML,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Rob Clark, Sean Paul, Stephen Boyd, quic_kalyant,
Abhinav Kumar (QUIC), Kuogee Hsieh (QUIC),
Bjorn Andersson, Sean Paul, David Airlie, Daniel Vetter,
Dmitry Baryshkov, quic_vproddut, Aravind Venkateswaran (QUIC),
Steev Klimaszewski
Hi,
On Fri, Apr 22, 2022 at 2:11 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> This patch adds support for generic eDP sink through aux_bus. The eDP/DP
> controller driver should support aux transactions originating from the
> panel-edp driver and hence should be initialized and ready.
>
> The panel bridge supporting the panel should be ready before the bridge
> connector is initialized. The generic panel probe needs the controller
> resources to be enabled to support the aux transactions originating from
> the panel probe.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> ---
> Changes in v9:
> - add comments for panel probe
> - modify the error handling checks
>
> Changes in v8:
> - handle corner cases
> - add comment for the bridge ops
>
> Changes in v7:
> - aux_bus is mandatory for eDP
> - connector type check modified to just check for eDP
>
> Changes in v6:
> - Remove initialization
> - Fix aux_bus node leak
> - Split the patches
>
> drivers/gpu/drm/msm/dp/dp_display.c | 73 +++++++++++++++++++++++++++++++++++--
> drivers/gpu/drm/msm/dp/dp_display.h | 1 +
> drivers/gpu/drm/msm/dp/dp_drm.c | 21 +++++++++--
> drivers/gpu/drm/msm/dp/dp_parser.c | 23 +-----------
> drivers/gpu/drm/msm/dp/dp_parser.h | 13 ++++++-
> 5 files changed, 101 insertions(+), 30 deletions(-)
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v9 1/4] drm/msm/dp: Add eDP support via aux_bus
@ 2022-04-22 14:03 ` Doug Anderson
0 siblings, 0 replies; 44+ messages in thread
From: Doug Anderson @ 2022-04-22 14:03 UTC (permalink / raw)
To: Sankeerth Billakanti
Cc: quic_kalyant,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Bjorn Andersson, Abhinav Kumar (QUIC),
quic_vproddut, David Airlie, linux-arm-msm, LKML, dri-devel,
Stephen Boyd, Sean Paul, Sean Paul, Steev Klimaszewski,
Dmitry Baryshkov, Aravind Venkateswaran (QUIC),
Kuogee Hsieh (QUIC),
freedreno
Hi,
On Fri, Apr 22, 2022 at 2:11 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> This patch adds support for generic eDP sink through aux_bus. The eDP/DP
> controller driver should support aux transactions originating from the
> panel-edp driver and hence should be initialized and ready.
>
> The panel bridge supporting the panel should be ready before the bridge
> connector is initialized. The generic panel probe needs the controller
> resources to be enabled to support the aux transactions originating from
> the panel probe.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> ---
> Changes in v9:
> - add comments for panel probe
> - modify the error handling checks
>
> Changes in v8:
> - handle corner cases
> - add comment for the bridge ops
>
> Changes in v7:
> - aux_bus is mandatory for eDP
> - connector type check modified to just check for eDP
>
> Changes in v6:
> - Remove initialization
> - Fix aux_bus node leak
> - Split the patches
>
> drivers/gpu/drm/msm/dp/dp_display.c | 73 +++++++++++++++++++++++++++++++++++--
> drivers/gpu/drm/msm/dp/dp_display.h | 1 +
> drivers/gpu/drm/msm/dp/dp_drm.c | 21 +++++++++--
> drivers/gpu/drm/msm/dp/dp_parser.c | 23 +-----------
> drivers/gpu/drm/msm/dp/dp_parser.h | 13 ++++++-
> 5 files changed, 101 insertions(+), 30 deletions(-)
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
2022-04-22 9:11 ` Sankeerth Billakanti
@ 2022-04-22 14:03 ` Doug Anderson
-1 siblings, 0 replies; 44+ messages in thread
From: Doug Anderson @ 2022-04-22 14:03 UTC (permalink / raw)
To: Sankeerth Billakanti
Cc: dri-devel, linux-arm-msm, freedreno, LKML,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Rob Clark, Sean Paul, Stephen Boyd, quic_kalyant,
Abhinav Kumar (QUIC), Kuogee Hsieh (QUIC),
Bjorn Andersson, Sean Paul, David Airlie, Daniel Vetter,
Dmitry Baryshkov, quic_vproddut, Aravind Venkateswaran (QUIC),
Steev Klimaszewski
Hi,
On Fri, Apr 22, 2022 at 2:11 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> The panel-edp enables the eDP panel power during probe, get_modes
> and pre-enable. The eDP connect and disconnect interrupts for the eDP/DP
> controller are directly dependent on panel power. As eDP display can be
> assumed as always connected, the controller driver can skip the eDP
> connect and disconnect interrupts. Any disruption in the link status
> will be indicated via the IRQ_HPD interrupts.
>
> So, the eDP controller driver can just enable the IRQ_HPD and replug
> interrupts. The DP controller driver still needs to enable all the
> interrupts.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> ---
> Changes in v9:
> - add comment explaining the interrupt status register
>
> Changes in v8:
> - add comment explaining the interrupt status return
>
> Changes in v7:
> - reordered the patch in the series
> - modified the return statement for isr
> - connector check modified to just check for eDP
>
> drivers/gpu/drm/msm/dp/dp_catalog.c | 16 ++++++++++------
> drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++-
> 2 files changed, 31 insertions(+), 7 deletions(-)
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
@ 2022-04-22 14:03 ` Doug Anderson
0 siblings, 0 replies; 44+ messages in thread
From: Doug Anderson @ 2022-04-22 14:03 UTC (permalink / raw)
To: Sankeerth Billakanti
Cc: quic_kalyant,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Bjorn Andersson, Abhinav Kumar (QUIC),
quic_vproddut, David Airlie, linux-arm-msm, LKML, dri-devel,
Stephen Boyd, Sean Paul, Sean Paul, Steev Klimaszewski,
Dmitry Baryshkov, Aravind Venkateswaran (QUIC),
Kuogee Hsieh (QUIC),
freedreno
Hi,
On Fri, Apr 22, 2022 at 2:11 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> The panel-edp enables the eDP panel power during probe, get_modes
> and pre-enable. The eDP connect and disconnect interrupts for the eDP/DP
> controller are directly dependent on panel power. As eDP display can be
> assumed as always connected, the controller driver can skip the eDP
> connect and disconnect interrupts. Any disruption in the link status
> will be indicated via the IRQ_HPD interrupts.
>
> So, the eDP controller driver can just enable the IRQ_HPD and replug
> interrupts. The DP controller driver still needs to enable all the
> interrupts.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> ---
> Changes in v9:
> - add comment explaining the interrupt status register
>
> Changes in v8:
> - add comment explaining the interrupt status return
>
> Changes in v7:
> - reordered the patch in the series
> - modified the return statement for isr
> - connector check modified to just check for eDP
>
> drivers/gpu/drm/msm/dp/dp_catalog.c | 16 ++++++++++------
> drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++-
> 2 files changed, 31 insertions(+), 7 deletions(-)
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
2022-04-22 9:11 ` Sankeerth Billakanti
@ 2022-04-22 15:55 ` Doug Anderson
-1 siblings, 0 replies; 44+ messages in thread
From: Doug Anderson @ 2022-04-22 15:55 UTC (permalink / raw)
To: Sankeerth Billakanti
Cc: dri-devel, linux-arm-msm, freedreno, LKML,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Rob Clark, Sean Paul, Stephen Boyd, quic_kalyant,
Abhinav Kumar (QUIC), Kuogee Hsieh (QUIC),
Bjorn Andersson, Sean Paul, David Airlie, Daniel Vetter,
Dmitry Baryshkov, quic_vproddut, Aravind Venkateswaran (QUIC),
Steev Klimaszewski
Hi,
On Fri, Apr 22, 2022 at 2:11 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> The panel-edp enables the eDP panel power during probe, get_modes
> and pre-enable. The eDP connect and disconnect interrupts for the eDP/DP
> controller are directly dependent on panel power. As eDP display can be
> assumed as always connected, the controller driver can skip the eDP
> connect and disconnect interrupts. Any disruption in the link status
> will be indicated via the IRQ_HPD interrupts.
>
> So, the eDP controller driver can just enable the IRQ_HPD and replug
> interrupts. The DP controller driver still needs to enable all the
> interrupts.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> ---
> Changes in v9:
> - add comment explaining the interrupt status register
>
> Changes in v8:
> - add comment explaining the interrupt status return
>
> Changes in v7:
> - reordered the patch in the series
> - modified the return statement for isr
> - connector check modified to just check for eDP
>
> drivers/gpu/drm/msm/dp/dp_catalog.c | 16 ++++++++++------
> drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++-
> 2 files changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index fac815f..df9670d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -569,10 +569,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog)
>
> u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
>
> - /* enable HPD plug and unplug interrupts */
> - dp_catalog_hpd_config_intr(dp_catalog,
> - DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK, true);
> -
> /* Configure REFTIMER and enable it */
> reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
> dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
> @@ -599,13 +595,21 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog)
> {
> struct dp_catalog_private *catalog = container_of(dp_catalog,
> struct dp_catalog_private, dp_catalog);
> - int isr = 0;
> + int isr, mask;
>
> isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
> dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
> (isr & DP_DP_HPD_INT_MASK));
> + mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
>
> - return isr;
> + /*
> + * We only want to return interrupts that are unmasked to the caller.
> + * However, the interrupt status field also contains other
> + * informational bits about the HPD state status, so we only mask
> + * out the part of the register that tells us about which interrupts
> + * are pending.
> + */
> + return isr & (mask | ~DP_DP_HPD_INT_MASK);
> }
>
> int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 055681a..dea4de9 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -683,7 +683,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
> dp_display_handle_plugged_change(&dp->dp_display, false);
>
> /* enable HDP plug interrupt to prepare for next plugin */
> - dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
> + if (!dp->dp_display.is_edp)
> + dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
>
> DRM_DEBUG_DP("After, type=%d hpd_state=%d\n",
> dp->dp_display.connector_type, state);
> @@ -1096,6 +1097,13 @@ static void dp_display_config_hpd(struct dp_display_private *dp)
> dp_display_host_init(dp);
> dp_catalog_ctrl_hpd_config(dp->catalog);
>
> + /* Enable plug and unplug interrupts only for external DisplayPort */
> + if (!dp->dp_display.is_edp)
> + dp_catalog_hpd_config_intr(dp->catalog,
> + DP_DP_HPD_PLUG_INT_MASK |
> + DP_DP_HPD_UNPLUG_INT_MASK,
> + true);
> +
> /* Enable interrupt first time
> * we are leaving dp clocks on during disconnect
> * and never disable interrupt
> @@ -1381,6 +1389,12 @@ static int dp_pm_resume(struct device *dev)
> dp_catalog_ctrl_hpd_config(dp->catalog);
>
>
> + if (!dp->dp_display.is_edp)
> + dp_catalog_hpd_config_intr(dp->catalog,
> + DP_DP_HPD_PLUG_INT_MASK |
> + DP_DP_HPD_UNPLUG_INT_MASK,
> + true);
> +
> if (dp_catalog_link_is_connected(dp->catalog)) {
> /*
> * set sink to normal operation mode -- D0
> @@ -1659,6 +1673,9 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
> return;
> }
>
> + if (dp->is_edp)
> + dp_hpd_plug_handle(dp_display, 0);
So I finally got a chance to test and unfortunately this is getting a
lockdep error. :( Here's the crawl with my current set of patches
(which, admittedly is on the chromeos-5.15 tree) instead of pure
upstream. I avoid the errors with this (sorry for the whitespace
damage, but it's really just a one-line change):
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -582,7 +582,8 @@ static int dp_hpd_plug_handle(struct
dp_display_private *dp, u32 data)
* add fail safe mode outside event_mutex scope
* to avoid potiential circular lock with drm thread
*/
- dp_panel_add_fail_safe_mode(dp->dp_display.connector);
+ if (!dp->dp_display.is_edp)
+ dp_panel_add_fail_safe_mode(dp->dp_display.connector);
/* uevent will complete connection part */
return 0;
That's a bit gross, but at least shows the problem. It's not a
_terrible_ fix because the failsafe modes don't make sense for eDP,
right? That being said, why are we hacking this in here? Shouldn't
this be in the core? ...or at least we should just be providing them
in get_modes()?
FWIW: the error was:
======================================================
WARNING: possible circular locking dependency detected
5.15.35-lockdep #6 Tainted: G W
------------------------------------------------------
frecon/429 is trying to acquire lock:
ffffff808dc3c4e8 (&dev->mode_config.mutex){+.+.}-{3:3}, at:
dp_panel_add_fail_safe_mode+0x4c/0xa0
but task is already holding lock:
ffffff808dc441e0 (&kms->commit_lock[i]){+.+.}-{3:3}, at: lock_crtcs+0xb4/0x124
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #3 (&kms->commit_lock[i]){+.+.}-{3:3}:
__mutex_lock_common+0x174/0x1a64
mutex_lock_nested+0x98/0xac
lock_crtcs+0xb4/0x124
msm_atomic_commit_tail+0x330/0x748
commit_tail+0x19c/0x278
drm_atomic_helper_commit+0x1dc/0x1f0
drm_atomic_commit+0xc0/0xd8
drm_atomic_helper_set_config+0xb4/0x134
drm_mode_setcrtc+0x688/0x1248
drm_ioctl_kernel+0x1e4/0x338
drm_ioctl+0x3a4/0x684
__arm64_sys_ioctl+0x118/0x154
invoke_syscall+0x78/0x224
el0_svc_common+0x178/0x200
do_el0_svc+0x94/0x13c
el0_svc+0x5c/0xec
el0t_64_sync_handler+0x78/0x108
el0t_64_sync+0x1a4/0x1a8
-> #2 (crtc_ww_class_mutex){+.+.}-{3:3}:
__mutex_lock_common+0x174/0x1a64
ww_mutex_lock+0xb8/0x278
modeset_lock+0x304/0x4ac
drm_modeset_lock+0x4c/0x7c
drmm_mode_config_init+0x4a8/0xc50
msm_drm_init+0x274/0xac0
msm_drm_bind+0x20/0x2c
try_to_bring_up_master+0x3dc/0x470
__component_add+0x18c/0x3c0
component_add+0x1c/0x28
dp_display_probe+0x954/0xa98
platform_probe+0x124/0x15c
really_probe+0x1b0/0x5f8
__driver_probe_device+0x174/0x20c
driver_probe_device+0x70/0x134
__device_attach_driver+0x130/0x1d0
bus_for_each_drv+0xfc/0x14c
__device_attach+0x1bc/0x2bc
device_initial_probe+0x1c/0x28
bus_probe_device+0x94/0x178
deferred_probe_work_func+0x1a4/0x1f0
process_one_work+0x5d4/0x9dc
worker_thread+0x898/0xccc
kthread+0x2d4/0x3d4
ret_from_fork+0x10/0x20
-> #1 (crtc_ww_class_acquire){+.+.}-{0:0}:
ww_acquire_init+0x1c4/0x2c8
drm_modeset_acquire_init+0x44/0xc8
drm_helper_probe_single_connector_modes+0xb0/0x12dc
drm_mode_getconnector+0x5dc/0xfe8
drm_ioctl_kernel+0x1e4/0x338
drm_ioctl+0x3a4/0x684
__arm64_sys_ioctl+0x118/0x154
invoke_syscall+0x78/0x224
el0_svc_common+0x178/0x200
do_el0_svc+0x94/0x13c
el0_svc+0x5c/0xec
el0t_64_sync_handler+0x78/0x108
el0t_64_sync+0x1a4/0x1a8
-> #0 (&dev->mode_config.mutex){+.+.}-{3:3}:
__lock_acquire+0x2650/0x672c
lock_acquire+0x1b4/0x4ac
__mutex_lock_common+0x174/0x1a64
mutex_lock_nested+0x98/0xac
dp_panel_add_fail_safe_mode+0x4c/0xa0
dp_hpd_plug_handle+0x1f0/0x280
dp_bridge_enable+0x94/0x2b8
drm_atomic_bridge_chain_enable+0x11c/0x168
drm_atomic_helper_commit_modeset_enables+0x500/0x740
msm_atomic_commit_tail+0x3e4/0x748
commit_tail+0x19c/0x278
drm_atomic_helper_commit+0x1dc/0x1f0
drm_atomic_commit+0xc0/0xd8
drm_atomic_helper_set_config+0xb4/0x134
drm_mode_setcrtc+0x688/0x1248
drm_ioctl_kernel+0x1e4/0x338
drm_ioctl+0x3a4/0x684
__arm64_sys_ioctl+0x118/0x154
invoke_syscall+0x78/0x224
el0_svc_common+0x178/0x200
do_el0_svc+0x94/0x13c
el0_svc+0x5c/0xec
el0t_64_sync_handler+0x78/0x108
el0t_64_sync+0x1a4/0x1a8
other info that might help us debug this:
Chain exists of:
&dev->mode_config.mutex --> crtc_ww_class_mutex --> &kms->commit_lock[i]
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&kms->commit_lock[i]);
lock(crtc_ww_class_mutex);
lock(&kms->commit_lock[i]);
lock(&dev->mode_config.mutex);
*** DEADLOCK ***
3 locks held by frecon/429:
#0: ffffffc00e197ab0 (crtc_ww_class_acquire){+.+.}-{0:0}, at:
drm_modeset_acquire_init+0x44/0xc8
#1: ffffff808dc3c588 (crtc_ww_class_mutex){+.+.}-{3:3}, at:
modeset_lock+0x18c/0x4ac
#2: ffffff808dc441e0 (&kms->commit_lock[i]){+.+.}-{3:3}, at:
lock_crtcs+0xb4/0x124
stack backtrace:
CPU: 5 PID: 429 Comm: frecon Tainted: G W
5.15.35-lockdep #6 9ba2ecd8f15354021fe165873da3aaa99f5b6798
Hardware name: Google Herobrine (rev1+) (DT)
Call trace:
dump_backtrace+0x0/0x3c4
show_stack+0x20/0x2c
dump_stack_lvl+0x78/0x9c
dump_stack+0x18/0x44
print_circular_bug+0x17c/0x1a8
check_noncircular+0x260/0x30c
__lock_acquire+0x2650/0x672c
lock_acquire+0x1b4/0x4ac
__mutex_lock_common+0x174/0x1a64
mutex_lock_nested+0x98/0xac
dp_panel_add_fail_safe_mode+0x4c/0xa0
dp_hpd_plug_handle+0x1f0/0x280
dp_bridge_enable+0x94/0x2b8
drm_atomic_bridge_chain_enable+0x11c/0x168
drm_atomic_helper_commit_modeset_enables+0x500/0x740
msm_atomic_commit_tail+0x3e4/0x748
commit_tail+0x19c/0x278
drm_atomic_helper_commit+0x1dc/0x1f0
drm_atomic_commit+0xc0/0xd8
drm_atomic_helper_set_config+0xb4/0x134
drm_mode_setcrtc+0x688/0x1248
drm_ioctl_kernel+0x1e4/0x338
drm_ioctl+0x3a4/0x684
__arm64_sys_ioctl+0x118/0x154
invoke_syscall+0x78/0x224
el0_svc_common+0x178/0x200
do_el0_svc+0x94/0x13c
el0_svc+0x5c/0xec
el0t_64_sync_handler+0x78/0x108
el0t_64_sync+0x1a4/0x1a8
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
@ 2022-04-22 15:55 ` Doug Anderson
0 siblings, 0 replies; 44+ messages in thread
From: Doug Anderson @ 2022-04-22 15:55 UTC (permalink / raw)
To: Sankeerth Billakanti
Cc: quic_kalyant,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Bjorn Andersson, Abhinav Kumar (QUIC),
quic_vproddut, David Airlie, linux-arm-msm, LKML, dri-devel,
Stephen Boyd, Sean Paul, Sean Paul, Steev Klimaszewski,
Dmitry Baryshkov, Aravind Venkateswaran (QUIC),
Kuogee Hsieh (QUIC),
freedreno
Hi,
On Fri, Apr 22, 2022 at 2:11 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> The panel-edp enables the eDP panel power during probe, get_modes
> and pre-enable. The eDP connect and disconnect interrupts for the eDP/DP
> controller are directly dependent on panel power. As eDP display can be
> assumed as always connected, the controller driver can skip the eDP
> connect and disconnect interrupts. Any disruption in the link status
> will be indicated via the IRQ_HPD interrupts.
>
> So, the eDP controller driver can just enable the IRQ_HPD and replug
> interrupts. The DP controller driver still needs to enable all the
> interrupts.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> ---
> Changes in v9:
> - add comment explaining the interrupt status register
>
> Changes in v8:
> - add comment explaining the interrupt status return
>
> Changes in v7:
> - reordered the patch in the series
> - modified the return statement for isr
> - connector check modified to just check for eDP
>
> drivers/gpu/drm/msm/dp/dp_catalog.c | 16 ++++++++++------
> drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++-
> 2 files changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index fac815f..df9670d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -569,10 +569,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog)
>
> u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
>
> - /* enable HPD plug and unplug interrupts */
> - dp_catalog_hpd_config_intr(dp_catalog,
> - DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK, true);
> -
> /* Configure REFTIMER and enable it */
> reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
> dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
> @@ -599,13 +595,21 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog)
> {
> struct dp_catalog_private *catalog = container_of(dp_catalog,
> struct dp_catalog_private, dp_catalog);
> - int isr = 0;
> + int isr, mask;
>
> isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
> dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
> (isr & DP_DP_HPD_INT_MASK));
> + mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
>
> - return isr;
> + /*
> + * We only want to return interrupts that are unmasked to the caller.
> + * However, the interrupt status field also contains other
> + * informational bits about the HPD state status, so we only mask
> + * out the part of the register that tells us about which interrupts
> + * are pending.
> + */
> + return isr & (mask | ~DP_DP_HPD_INT_MASK);
> }
>
> int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 055681a..dea4de9 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -683,7 +683,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
> dp_display_handle_plugged_change(&dp->dp_display, false);
>
> /* enable HDP plug interrupt to prepare for next plugin */
> - dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
> + if (!dp->dp_display.is_edp)
> + dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
>
> DRM_DEBUG_DP("After, type=%d hpd_state=%d\n",
> dp->dp_display.connector_type, state);
> @@ -1096,6 +1097,13 @@ static void dp_display_config_hpd(struct dp_display_private *dp)
> dp_display_host_init(dp);
> dp_catalog_ctrl_hpd_config(dp->catalog);
>
> + /* Enable plug and unplug interrupts only for external DisplayPort */
> + if (!dp->dp_display.is_edp)
> + dp_catalog_hpd_config_intr(dp->catalog,
> + DP_DP_HPD_PLUG_INT_MASK |
> + DP_DP_HPD_UNPLUG_INT_MASK,
> + true);
> +
> /* Enable interrupt first time
> * we are leaving dp clocks on during disconnect
> * and never disable interrupt
> @@ -1381,6 +1389,12 @@ static int dp_pm_resume(struct device *dev)
> dp_catalog_ctrl_hpd_config(dp->catalog);
>
>
> + if (!dp->dp_display.is_edp)
> + dp_catalog_hpd_config_intr(dp->catalog,
> + DP_DP_HPD_PLUG_INT_MASK |
> + DP_DP_HPD_UNPLUG_INT_MASK,
> + true);
> +
> if (dp_catalog_link_is_connected(dp->catalog)) {
> /*
> * set sink to normal operation mode -- D0
> @@ -1659,6 +1673,9 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
> return;
> }
>
> + if (dp->is_edp)
> + dp_hpd_plug_handle(dp_display, 0);
So I finally got a chance to test and unfortunately this is getting a
lockdep error. :( Here's the crawl with my current set of patches
(which, admittedly is on the chromeos-5.15 tree) instead of pure
upstream. I avoid the errors with this (sorry for the whitespace
damage, but it's really just a one-line change):
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -582,7 +582,8 @@ static int dp_hpd_plug_handle(struct
dp_display_private *dp, u32 data)
* add fail safe mode outside event_mutex scope
* to avoid potiential circular lock with drm thread
*/
- dp_panel_add_fail_safe_mode(dp->dp_display.connector);
+ if (!dp->dp_display.is_edp)
+ dp_panel_add_fail_safe_mode(dp->dp_display.connector);
/* uevent will complete connection part */
return 0;
That's a bit gross, but at least shows the problem. It's not a
_terrible_ fix because the failsafe modes don't make sense for eDP,
right? That being said, why are we hacking this in here? Shouldn't
this be in the core? ...or at least we should just be providing them
in get_modes()?
FWIW: the error was:
======================================================
WARNING: possible circular locking dependency detected
5.15.35-lockdep #6 Tainted: G W
------------------------------------------------------
frecon/429 is trying to acquire lock:
ffffff808dc3c4e8 (&dev->mode_config.mutex){+.+.}-{3:3}, at:
dp_panel_add_fail_safe_mode+0x4c/0xa0
but task is already holding lock:
ffffff808dc441e0 (&kms->commit_lock[i]){+.+.}-{3:3}, at: lock_crtcs+0xb4/0x124
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #3 (&kms->commit_lock[i]){+.+.}-{3:3}:
__mutex_lock_common+0x174/0x1a64
mutex_lock_nested+0x98/0xac
lock_crtcs+0xb4/0x124
msm_atomic_commit_tail+0x330/0x748
commit_tail+0x19c/0x278
drm_atomic_helper_commit+0x1dc/0x1f0
drm_atomic_commit+0xc0/0xd8
drm_atomic_helper_set_config+0xb4/0x134
drm_mode_setcrtc+0x688/0x1248
drm_ioctl_kernel+0x1e4/0x338
drm_ioctl+0x3a4/0x684
__arm64_sys_ioctl+0x118/0x154
invoke_syscall+0x78/0x224
el0_svc_common+0x178/0x200
do_el0_svc+0x94/0x13c
el0_svc+0x5c/0xec
el0t_64_sync_handler+0x78/0x108
el0t_64_sync+0x1a4/0x1a8
-> #2 (crtc_ww_class_mutex){+.+.}-{3:3}:
__mutex_lock_common+0x174/0x1a64
ww_mutex_lock+0xb8/0x278
modeset_lock+0x304/0x4ac
drm_modeset_lock+0x4c/0x7c
drmm_mode_config_init+0x4a8/0xc50
msm_drm_init+0x274/0xac0
msm_drm_bind+0x20/0x2c
try_to_bring_up_master+0x3dc/0x470
__component_add+0x18c/0x3c0
component_add+0x1c/0x28
dp_display_probe+0x954/0xa98
platform_probe+0x124/0x15c
really_probe+0x1b0/0x5f8
__driver_probe_device+0x174/0x20c
driver_probe_device+0x70/0x134
__device_attach_driver+0x130/0x1d0
bus_for_each_drv+0xfc/0x14c
__device_attach+0x1bc/0x2bc
device_initial_probe+0x1c/0x28
bus_probe_device+0x94/0x178
deferred_probe_work_func+0x1a4/0x1f0
process_one_work+0x5d4/0x9dc
worker_thread+0x898/0xccc
kthread+0x2d4/0x3d4
ret_from_fork+0x10/0x20
-> #1 (crtc_ww_class_acquire){+.+.}-{0:0}:
ww_acquire_init+0x1c4/0x2c8
drm_modeset_acquire_init+0x44/0xc8
drm_helper_probe_single_connector_modes+0xb0/0x12dc
drm_mode_getconnector+0x5dc/0xfe8
drm_ioctl_kernel+0x1e4/0x338
drm_ioctl+0x3a4/0x684
__arm64_sys_ioctl+0x118/0x154
invoke_syscall+0x78/0x224
el0_svc_common+0x178/0x200
do_el0_svc+0x94/0x13c
el0_svc+0x5c/0xec
el0t_64_sync_handler+0x78/0x108
el0t_64_sync+0x1a4/0x1a8
-> #0 (&dev->mode_config.mutex){+.+.}-{3:3}:
__lock_acquire+0x2650/0x672c
lock_acquire+0x1b4/0x4ac
__mutex_lock_common+0x174/0x1a64
mutex_lock_nested+0x98/0xac
dp_panel_add_fail_safe_mode+0x4c/0xa0
dp_hpd_plug_handle+0x1f0/0x280
dp_bridge_enable+0x94/0x2b8
drm_atomic_bridge_chain_enable+0x11c/0x168
drm_atomic_helper_commit_modeset_enables+0x500/0x740
msm_atomic_commit_tail+0x3e4/0x748
commit_tail+0x19c/0x278
drm_atomic_helper_commit+0x1dc/0x1f0
drm_atomic_commit+0xc0/0xd8
drm_atomic_helper_set_config+0xb4/0x134
drm_mode_setcrtc+0x688/0x1248
drm_ioctl_kernel+0x1e4/0x338
drm_ioctl+0x3a4/0x684
__arm64_sys_ioctl+0x118/0x154
invoke_syscall+0x78/0x224
el0_svc_common+0x178/0x200
do_el0_svc+0x94/0x13c
el0_svc+0x5c/0xec
el0t_64_sync_handler+0x78/0x108
el0t_64_sync+0x1a4/0x1a8
other info that might help us debug this:
Chain exists of:
&dev->mode_config.mutex --> crtc_ww_class_mutex --> &kms->commit_lock[i]
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&kms->commit_lock[i]);
lock(crtc_ww_class_mutex);
lock(&kms->commit_lock[i]);
lock(&dev->mode_config.mutex);
*** DEADLOCK ***
3 locks held by frecon/429:
#0: ffffffc00e197ab0 (crtc_ww_class_acquire){+.+.}-{0:0}, at:
drm_modeset_acquire_init+0x44/0xc8
#1: ffffff808dc3c588 (crtc_ww_class_mutex){+.+.}-{3:3}, at:
modeset_lock+0x18c/0x4ac
#2: ffffff808dc441e0 (&kms->commit_lock[i]){+.+.}-{3:3}, at:
lock_crtcs+0xb4/0x124
stack backtrace:
CPU: 5 PID: 429 Comm: frecon Tainted: G W
5.15.35-lockdep #6 9ba2ecd8f15354021fe165873da3aaa99f5b6798
Hardware name: Google Herobrine (rev1+) (DT)
Call trace:
dump_backtrace+0x0/0x3c4
show_stack+0x20/0x2c
dump_stack_lvl+0x78/0x9c
dump_stack+0x18/0x44
print_circular_bug+0x17c/0x1a8
check_noncircular+0x260/0x30c
__lock_acquire+0x2650/0x672c
lock_acquire+0x1b4/0x4ac
__mutex_lock_common+0x174/0x1a64
mutex_lock_nested+0x98/0xac
dp_panel_add_fail_safe_mode+0x4c/0xa0
dp_hpd_plug_handle+0x1f0/0x280
dp_bridge_enable+0x94/0x2b8
drm_atomic_bridge_chain_enable+0x11c/0x168
drm_atomic_helper_commit_modeset_enables+0x500/0x740
msm_atomic_commit_tail+0x3e4/0x748
commit_tail+0x19c/0x278
drm_atomic_helper_commit+0x1dc/0x1f0
drm_atomic_commit+0xc0/0xd8
drm_atomic_helper_set_config+0xb4/0x134
drm_mode_setcrtc+0x688/0x1248
drm_ioctl_kernel+0x1e4/0x338
drm_ioctl+0x3a4/0x684
__arm64_sys_ioctl+0x118/0x154
invoke_syscall+0x78/0x224
el0_svc_common+0x178/0x200
do_el0_svc+0x94/0x13c
el0_svc+0x5c/0xec
el0t_64_sync_handler+0x78/0x108
el0t_64_sync+0x1a4/0x1a8
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
2022-04-22 15:55 ` Doug Anderson
@ 2022-04-22 16:04 ` Abhinav Kumar
-1 siblings, 0 replies; 44+ messages in thread
From: Abhinav Kumar @ 2022-04-22 16:04 UTC (permalink / raw)
To: Doug Anderson, Sankeerth Billakanti
Cc: quic_kalyant,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
quic_vproddut, David Airlie, linux-arm-msm, Kuogee Hsieh (QUIC),
freedreno, LKML, dri-devel, Bjorn Andersson, Sean Paul,
Steev Klimaszewski, Dmitry Baryshkov,
Aravind Venkateswaran (QUIC),
Stephen Boyd, Sean Paul
Hi Doug
For the lockdep error, the splat looks similar to what kuogee fixed
recently.
Can you please check if below patch is present in your tree?
https://patchwork.freedesktop.org/patch/481396/
Thanks
Abhinav
On 4/22/2022 8:55 AM, Doug Anderson wrote:
> Hi,
>
> On Fri, Apr 22, 2022 at 2:11 AM Sankeerth Billakanti
> <quic_sbillaka@quicinc.com> wrote:
>>
>> The panel-edp enables the eDP panel power during probe, get_modes
>> and pre-enable. The eDP connect and disconnect interrupts for the eDP/DP
>> controller are directly dependent on panel power. As eDP display can be
>> assumed as always connected, the controller driver can skip the eDP
>> connect and disconnect interrupts. Any disruption in the link status
>> will be indicated via the IRQ_HPD interrupts.
>>
>> So, the eDP controller driver can just enable the IRQ_HPD and replug
>> interrupts. The DP controller driver still needs to enable all the
>> interrupts.
>>
>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
>> ---
>> Changes in v9:
>> - add comment explaining the interrupt status register
>>
>> Changes in v8:
>> - add comment explaining the interrupt status return
>>
>> Changes in v7:
>> - reordered the patch in the series
>> - modified the return statement for isr
>> - connector check modified to just check for eDP
>>
>> drivers/gpu/drm/msm/dp/dp_catalog.c | 16 ++++++++++------
>> drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++-
>> 2 files changed, 31 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> index fac815f..df9670d 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> @@ -569,10 +569,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog)
>>
>> u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
>>
>> - /* enable HPD plug and unplug interrupts */
>> - dp_catalog_hpd_config_intr(dp_catalog,
>> - DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK, true);
>> -
>> /* Configure REFTIMER and enable it */
>> reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
>> dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
>> @@ -599,13 +595,21 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog)
>> {
>> struct dp_catalog_private *catalog = container_of(dp_catalog,
>> struct dp_catalog_private, dp_catalog);
>> - int isr = 0;
>> + int isr, mask;
>>
>> isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
>> dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
>> (isr & DP_DP_HPD_INT_MASK));
>> + mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
>>
>> - return isr;
>> + /*
>> + * We only want to return interrupts that are unmasked to the caller.
>> + * However, the interrupt status field also contains other
>> + * informational bits about the HPD state status, so we only mask
>> + * out the part of the register that tells us about which interrupts
>> + * are pending.
>> + */
>> + return isr & (mask | ~DP_DP_HPD_INT_MASK);
>> }
>>
>> int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 055681a..dea4de9 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -683,7 +683,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
>> dp_display_handle_plugged_change(&dp->dp_display, false);
>>
>> /* enable HDP plug interrupt to prepare for next plugin */
>> - dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
>> + if (!dp->dp_display.is_edp)
>> + dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
>>
>> DRM_DEBUG_DP("After, type=%d hpd_state=%d\n",
>> dp->dp_display.connector_type, state);
>> @@ -1096,6 +1097,13 @@ static void dp_display_config_hpd(struct dp_display_private *dp)
>> dp_display_host_init(dp);
>> dp_catalog_ctrl_hpd_config(dp->catalog);
>>
>> + /* Enable plug and unplug interrupts only for external DisplayPort */
>> + if (!dp->dp_display.is_edp)
>> + dp_catalog_hpd_config_intr(dp->catalog,
>> + DP_DP_HPD_PLUG_INT_MASK |
>> + DP_DP_HPD_UNPLUG_INT_MASK,
>> + true);
>> +
>> /* Enable interrupt first time
>> * we are leaving dp clocks on during disconnect
>> * and never disable interrupt
>> @@ -1381,6 +1389,12 @@ static int dp_pm_resume(struct device *dev)
>> dp_catalog_ctrl_hpd_config(dp->catalog);
>>
>>
>> + if (!dp->dp_display.is_edp)
>> + dp_catalog_hpd_config_intr(dp->catalog,
>> + DP_DP_HPD_PLUG_INT_MASK |
>> + DP_DP_HPD_UNPLUG_INT_MASK,
>> + true);
>> +
>> if (dp_catalog_link_is_connected(dp->catalog)) {
>> /*
>> * set sink to normal operation mode -- D0
>> @@ -1659,6 +1673,9 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
>> return;
>> }
>>
>> + if (dp->is_edp)
>> + dp_hpd_plug_handle(dp_display, 0);
>
> So I finally got a chance to test and unfortunately this is getting a
> lockdep error. :( Here's the crawl with my current set of patches
> (which, admittedly is on the chromeos-5.15 tree) instead of pure
> upstream. I avoid the errors with this (sorry for the whitespace
> damage, but it's really just a one-line change):
>
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -582,7 +582,8 @@ static int dp_hpd_plug_handle(struct
> dp_display_private *dp, u32 data)
> * add fail safe mode outside event_mutex scope
> * to avoid potiential circular lock with drm thread
> */
> - dp_panel_add_fail_safe_mode(dp->dp_display.connector);
> + if (!dp->dp_display.is_edp)
> + dp_panel_add_fail_safe_mode(dp->dp_display.connector);
>
> /* uevent will complete connection part */
> return 0;
>
> That's a bit gross, but at least shows the problem. It's not a
> _terrible_ fix because the failsafe modes don't make sense for eDP,
> right? That being said, why are we hacking this in here? Shouldn't
> this be in the core? ...or at least we should just be providing them
> in get_modes()?
>
> FWIW: the error was:
>
> ======================================================
> WARNING: possible circular locking dependency detected
> 5.15.35-lockdep #6 Tainted: G W
> ------------------------------------------------------
> frecon/429 is trying to acquire lock:
> ffffff808dc3c4e8 (&dev->mode_config.mutex){+.+.}-{3:3}, at:
> dp_panel_add_fail_safe_mode+0x4c/0xa0
>
> but task is already holding lock:
> ffffff808dc441e0 (&kms->commit_lock[i]){+.+.}-{3:3}, at: lock_crtcs+0xb4/0x124
>
> which lock already depends on the new lock.
>
>
> the existing dependency chain (in reverse order) is:
>
> -> #3 (&kms->commit_lock[i]){+.+.}-{3:3}:
> __mutex_lock_common+0x174/0x1a64
> mutex_lock_nested+0x98/0xac
> lock_crtcs+0xb4/0x124
> msm_atomic_commit_tail+0x330/0x748
> commit_tail+0x19c/0x278
> drm_atomic_helper_commit+0x1dc/0x1f0
> drm_atomic_commit+0xc0/0xd8
> drm_atomic_helper_set_config+0xb4/0x134
> drm_mode_setcrtc+0x688/0x1248
> drm_ioctl_kernel+0x1e4/0x338
> drm_ioctl+0x3a4/0x684
> __arm64_sys_ioctl+0x118/0x154
> invoke_syscall+0x78/0x224
> el0_svc_common+0x178/0x200
> do_el0_svc+0x94/0x13c
> el0_svc+0x5c/0xec
> el0t_64_sync_handler+0x78/0x108
> el0t_64_sync+0x1a4/0x1a8
>
> -> #2 (crtc_ww_class_mutex){+.+.}-{3:3}:
> __mutex_lock_common+0x174/0x1a64
> ww_mutex_lock+0xb8/0x278
> modeset_lock+0x304/0x4ac
> drm_modeset_lock+0x4c/0x7c
> drmm_mode_config_init+0x4a8/0xc50
> msm_drm_init+0x274/0xac0
> msm_drm_bind+0x20/0x2c
> try_to_bring_up_master+0x3dc/0x470
> __component_add+0x18c/0x3c0
> component_add+0x1c/0x28
> dp_display_probe+0x954/0xa98
> platform_probe+0x124/0x15c
> really_probe+0x1b0/0x5f8
> __driver_probe_device+0x174/0x20c
> driver_probe_device+0x70/0x134
> __device_attach_driver+0x130/0x1d0
> bus_for_each_drv+0xfc/0x14c
> __device_attach+0x1bc/0x2bc
> device_initial_probe+0x1c/0x28
> bus_probe_device+0x94/0x178
> deferred_probe_work_func+0x1a4/0x1f0
> process_one_work+0x5d4/0x9dc
> worker_thread+0x898/0xccc
> kthread+0x2d4/0x3d4
> ret_from_fork+0x10/0x20
>
> -> #1 (crtc_ww_class_acquire){+.+.}-{0:0}:
> ww_acquire_init+0x1c4/0x2c8
> drm_modeset_acquire_init+0x44/0xc8
> drm_helper_probe_single_connector_modes+0xb0/0x12dc
> drm_mode_getconnector+0x5dc/0xfe8
> drm_ioctl_kernel+0x1e4/0x338
> drm_ioctl+0x3a4/0x684
> __arm64_sys_ioctl+0x118/0x154
> invoke_syscall+0x78/0x224
> el0_svc_common+0x178/0x200
> do_el0_svc+0x94/0x13c
> el0_svc+0x5c/0xec
> el0t_64_sync_handler+0x78/0x108
> el0t_64_sync+0x1a4/0x1a8
>
> -> #0 (&dev->mode_config.mutex){+.+.}-{3:3}:
> __lock_acquire+0x2650/0x672c
> lock_acquire+0x1b4/0x4ac
> __mutex_lock_common+0x174/0x1a64
> mutex_lock_nested+0x98/0xac
> dp_panel_add_fail_safe_mode+0x4c/0xa0
> dp_hpd_plug_handle+0x1f0/0x280
> dp_bridge_enable+0x94/0x2b8
> drm_atomic_bridge_chain_enable+0x11c/0x168
> drm_atomic_helper_commit_modeset_enables+0x500/0x740
> msm_atomic_commit_tail+0x3e4/0x748
> commit_tail+0x19c/0x278
> drm_atomic_helper_commit+0x1dc/0x1f0
> drm_atomic_commit+0xc0/0xd8
> drm_atomic_helper_set_config+0xb4/0x134
> drm_mode_setcrtc+0x688/0x1248
> drm_ioctl_kernel+0x1e4/0x338
> drm_ioctl+0x3a4/0x684
> __arm64_sys_ioctl+0x118/0x154
> invoke_syscall+0x78/0x224
> el0_svc_common+0x178/0x200
> do_el0_svc+0x94/0x13c
> el0_svc+0x5c/0xec
> el0t_64_sync_handler+0x78/0x108
> el0t_64_sync+0x1a4/0x1a8
>
> other info that might help us debug this:
>
> Chain exists of:
> &dev->mode_config.mutex --> crtc_ww_class_mutex --> &kms->commit_lock[i]
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(&kms->commit_lock[i]);
> lock(crtc_ww_class_mutex);
> lock(&kms->commit_lock[i]);
> lock(&dev->mode_config.mutex);
>
> *** DEADLOCK ***
>
> 3 locks held by frecon/429:
> #0: ffffffc00e197ab0 (crtc_ww_class_acquire){+.+.}-{0:0}, at:
> drm_modeset_acquire_init+0x44/0xc8
> #1: ffffff808dc3c588 (crtc_ww_class_mutex){+.+.}-{3:3}, at:
> modeset_lock+0x18c/0x4ac
> #2: ffffff808dc441e0 (&kms->commit_lock[i]){+.+.}-{3:3}, at:
> lock_crtcs+0xb4/0x124
>
> stack backtrace:
> CPU: 5 PID: 429 Comm: frecon Tainted: G W
> 5.15.35-lockdep #6 9ba2ecd8f15354021fe165873da3aaa99f5b6798
> Hardware name: Google Herobrine (rev1+) (DT)
> Call trace:
> dump_backtrace+0x0/0x3c4
> show_stack+0x20/0x2c
> dump_stack_lvl+0x78/0x9c
> dump_stack+0x18/0x44
> print_circular_bug+0x17c/0x1a8
> check_noncircular+0x260/0x30c
> __lock_acquire+0x2650/0x672c
> lock_acquire+0x1b4/0x4ac
> __mutex_lock_common+0x174/0x1a64
> mutex_lock_nested+0x98/0xac
> dp_panel_add_fail_safe_mode+0x4c/0xa0
> dp_hpd_plug_handle+0x1f0/0x280
> dp_bridge_enable+0x94/0x2b8
> drm_atomic_bridge_chain_enable+0x11c/0x168
> drm_atomic_helper_commit_modeset_enables+0x500/0x740
> msm_atomic_commit_tail+0x3e4/0x748
> commit_tail+0x19c/0x278
> drm_atomic_helper_commit+0x1dc/0x1f0
> drm_atomic_commit+0xc0/0xd8
> drm_atomic_helper_set_config+0xb4/0x134
> drm_mode_setcrtc+0x688/0x1248
> drm_ioctl_kernel+0x1e4/0x338
> drm_ioctl+0x3a4/0x684
> __arm64_sys_ioctl+0x118/0x154
> invoke_syscall+0x78/0x224
> el0_svc_common+0x178/0x200
> do_el0_svc+0x94/0x13c
> el0_svc+0x5c/0xec
> el0t_64_sync_handler+0x78/0x108
> el0t_64_sync+0x1a4/0x1a8
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
@ 2022-04-22 16:04 ` Abhinav Kumar
0 siblings, 0 replies; 44+ messages in thread
From: Abhinav Kumar @ 2022-04-22 16:04 UTC (permalink / raw)
To: Doug Anderson, Sankeerth Billakanti
Cc: quic_kalyant,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Bjorn Andersson, quic_vproddut, David Airlie, linux-arm-msm,
LKML, dri-devel, Stephen Boyd, Sean Paul, Sean Paul,
Steev Klimaszewski, Dmitry Baryshkov,
Aravind Venkateswaran (QUIC), Kuogee Hsieh (QUIC),
freedreno
Hi Doug
For the lockdep error, the splat looks similar to what kuogee fixed
recently.
Can you please check if below patch is present in your tree?
https://patchwork.freedesktop.org/patch/481396/
Thanks
Abhinav
On 4/22/2022 8:55 AM, Doug Anderson wrote:
> Hi,
>
> On Fri, Apr 22, 2022 at 2:11 AM Sankeerth Billakanti
> <quic_sbillaka@quicinc.com> wrote:
>>
>> The panel-edp enables the eDP panel power during probe, get_modes
>> and pre-enable. The eDP connect and disconnect interrupts for the eDP/DP
>> controller are directly dependent on panel power. As eDP display can be
>> assumed as always connected, the controller driver can skip the eDP
>> connect and disconnect interrupts. Any disruption in the link status
>> will be indicated via the IRQ_HPD interrupts.
>>
>> So, the eDP controller driver can just enable the IRQ_HPD and replug
>> interrupts. The DP controller driver still needs to enable all the
>> interrupts.
>>
>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
>> ---
>> Changes in v9:
>> - add comment explaining the interrupt status register
>>
>> Changes in v8:
>> - add comment explaining the interrupt status return
>>
>> Changes in v7:
>> - reordered the patch in the series
>> - modified the return statement for isr
>> - connector check modified to just check for eDP
>>
>> drivers/gpu/drm/msm/dp/dp_catalog.c | 16 ++++++++++------
>> drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++-
>> 2 files changed, 31 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> index fac815f..df9670d 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> @@ -569,10 +569,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog)
>>
>> u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
>>
>> - /* enable HPD plug and unplug interrupts */
>> - dp_catalog_hpd_config_intr(dp_catalog,
>> - DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK, true);
>> -
>> /* Configure REFTIMER and enable it */
>> reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
>> dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
>> @@ -599,13 +595,21 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog)
>> {
>> struct dp_catalog_private *catalog = container_of(dp_catalog,
>> struct dp_catalog_private, dp_catalog);
>> - int isr = 0;
>> + int isr, mask;
>>
>> isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
>> dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
>> (isr & DP_DP_HPD_INT_MASK));
>> + mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
>>
>> - return isr;
>> + /*
>> + * We only want to return interrupts that are unmasked to the caller.
>> + * However, the interrupt status field also contains other
>> + * informational bits about the HPD state status, so we only mask
>> + * out the part of the register that tells us about which interrupts
>> + * are pending.
>> + */
>> + return isr & (mask | ~DP_DP_HPD_INT_MASK);
>> }
>>
>> int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 055681a..dea4de9 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -683,7 +683,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
>> dp_display_handle_plugged_change(&dp->dp_display, false);
>>
>> /* enable HDP plug interrupt to prepare for next plugin */
>> - dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
>> + if (!dp->dp_display.is_edp)
>> + dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
>>
>> DRM_DEBUG_DP("After, type=%d hpd_state=%d\n",
>> dp->dp_display.connector_type, state);
>> @@ -1096,6 +1097,13 @@ static void dp_display_config_hpd(struct dp_display_private *dp)
>> dp_display_host_init(dp);
>> dp_catalog_ctrl_hpd_config(dp->catalog);
>>
>> + /* Enable plug and unplug interrupts only for external DisplayPort */
>> + if (!dp->dp_display.is_edp)
>> + dp_catalog_hpd_config_intr(dp->catalog,
>> + DP_DP_HPD_PLUG_INT_MASK |
>> + DP_DP_HPD_UNPLUG_INT_MASK,
>> + true);
>> +
>> /* Enable interrupt first time
>> * we are leaving dp clocks on during disconnect
>> * and never disable interrupt
>> @@ -1381,6 +1389,12 @@ static int dp_pm_resume(struct device *dev)
>> dp_catalog_ctrl_hpd_config(dp->catalog);
>>
>>
>> + if (!dp->dp_display.is_edp)
>> + dp_catalog_hpd_config_intr(dp->catalog,
>> + DP_DP_HPD_PLUG_INT_MASK |
>> + DP_DP_HPD_UNPLUG_INT_MASK,
>> + true);
>> +
>> if (dp_catalog_link_is_connected(dp->catalog)) {
>> /*
>> * set sink to normal operation mode -- D0
>> @@ -1659,6 +1673,9 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
>> return;
>> }
>>
>> + if (dp->is_edp)
>> + dp_hpd_plug_handle(dp_display, 0);
>
> So I finally got a chance to test and unfortunately this is getting a
> lockdep error. :( Here's the crawl with my current set of patches
> (which, admittedly is on the chromeos-5.15 tree) instead of pure
> upstream. I avoid the errors with this (sorry for the whitespace
> damage, but it's really just a one-line change):
>
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -582,7 +582,8 @@ static int dp_hpd_plug_handle(struct
> dp_display_private *dp, u32 data)
> * add fail safe mode outside event_mutex scope
> * to avoid potiential circular lock with drm thread
> */
> - dp_panel_add_fail_safe_mode(dp->dp_display.connector);
> + if (!dp->dp_display.is_edp)
> + dp_panel_add_fail_safe_mode(dp->dp_display.connector);
>
> /* uevent will complete connection part */
> return 0;
>
> That's a bit gross, but at least shows the problem. It's not a
> _terrible_ fix because the failsafe modes don't make sense for eDP,
> right? That being said, why are we hacking this in here? Shouldn't
> this be in the core? ...or at least we should just be providing them
> in get_modes()?
>
> FWIW: the error was:
>
> ======================================================
> WARNING: possible circular locking dependency detected
> 5.15.35-lockdep #6 Tainted: G W
> ------------------------------------------------------
> frecon/429 is trying to acquire lock:
> ffffff808dc3c4e8 (&dev->mode_config.mutex){+.+.}-{3:3}, at:
> dp_panel_add_fail_safe_mode+0x4c/0xa0
>
> but task is already holding lock:
> ffffff808dc441e0 (&kms->commit_lock[i]){+.+.}-{3:3}, at: lock_crtcs+0xb4/0x124
>
> which lock already depends on the new lock.
>
>
> the existing dependency chain (in reverse order) is:
>
> -> #3 (&kms->commit_lock[i]){+.+.}-{3:3}:
> __mutex_lock_common+0x174/0x1a64
> mutex_lock_nested+0x98/0xac
> lock_crtcs+0xb4/0x124
> msm_atomic_commit_tail+0x330/0x748
> commit_tail+0x19c/0x278
> drm_atomic_helper_commit+0x1dc/0x1f0
> drm_atomic_commit+0xc0/0xd8
> drm_atomic_helper_set_config+0xb4/0x134
> drm_mode_setcrtc+0x688/0x1248
> drm_ioctl_kernel+0x1e4/0x338
> drm_ioctl+0x3a4/0x684
> __arm64_sys_ioctl+0x118/0x154
> invoke_syscall+0x78/0x224
> el0_svc_common+0x178/0x200
> do_el0_svc+0x94/0x13c
> el0_svc+0x5c/0xec
> el0t_64_sync_handler+0x78/0x108
> el0t_64_sync+0x1a4/0x1a8
>
> -> #2 (crtc_ww_class_mutex){+.+.}-{3:3}:
> __mutex_lock_common+0x174/0x1a64
> ww_mutex_lock+0xb8/0x278
> modeset_lock+0x304/0x4ac
> drm_modeset_lock+0x4c/0x7c
> drmm_mode_config_init+0x4a8/0xc50
> msm_drm_init+0x274/0xac0
> msm_drm_bind+0x20/0x2c
> try_to_bring_up_master+0x3dc/0x470
> __component_add+0x18c/0x3c0
> component_add+0x1c/0x28
> dp_display_probe+0x954/0xa98
> platform_probe+0x124/0x15c
> really_probe+0x1b0/0x5f8
> __driver_probe_device+0x174/0x20c
> driver_probe_device+0x70/0x134
> __device_attach_driver+0x130/0x1d0
> bus_for_each_drv+0xfc/0x14c
> __device_attach+0x1bc/0x2bc
> device_initial_probe+0x1c/0x28
> bus_probe_device+0x94/0x178
> deferred_probe_work_func+0x1a4/0x1f0
> process_one_work+0x5d4/0x9dc
> worker_thread+0x898/0xccc
> kthread+0x2d4/0x3d4
> ret_from_fork+0x10/0x20
>
> -> #1 (crtc_ww_class_acquire){+.+.}-{0:0}:
> ww_acquire_init+0x1c4/0x2c8
> drm_modeset_acquire_init+0x44/0xc8
> drm_helper_probe_single_connector_modes+0xb0/0x12dc
> drm_mode_getconnector+0x5dc/0xfe8
> drm_ioctl_kernel+0x1e4/0x338
> drm_ioctl+0x3a4/0x684
> __arm64_sys_ioctl+0x118/0x154
> invoke_syscall+0x78/0x224
> el0_svc_common+0x178/0x200
> do_el0_svc+0x94/0x13c
> el0_svc+0x5c/0xec
> el0t_64_sync_handler+0x78/0x108
> el0t_64_sync+0x1a4/0x1a8
>
> -> #0 (&dev->mode_config.mutex){+.+.}-{3:3}:
> __lock_acquire+0x2650/0x672c
> lock_acquire+0x1b4/0x4ac
> __mutex_lock_common+0x174/0x1a64
> mutex_lock_nested+0x98/0xac
> dp_panel_add_fail_safe_mode+0x4c/0xa0
> dp_hpd_plug_handle+0x1f0/0x280
> dp_bridge_enable+0x94/0x2b8
> drm_atomic_bridge_chain_enable+0x11c/0x168
> drm_atomic_helper_commit_modeset_enables+0x500/0x740
> msm_atomic_commit_tail+0x3e4/0x748
> commit_tail+0x19c/0x278
> drm_atomic_helper_commit+0x1dc/0x1f0
> drm_atomic_commit+0xc0/0xd8
> drm_atomic_helper_set_config+0xb4/0x134
> drm_mode_setcrtc+0x688/0x1248
> drm_ioctl_kernel+0x1e4/0x338
> drm_ioctl+0x3a4/0x684
> __arm64_sys_ioctl+0x118/0x154
> invoke_syscall+0x78/0x224
> el0_svc_common+0x178/0x200
> do_el0_svc+0x94/0x13c
> el0_svc+0x5c/0xec
> el0t_64_sync_handler+0x78/0x108
> el0t_64_sync+0x1a4/0x1a8
>
> other info that might help us debug this:
>
> Chain exists of:
> &dev->mode_config.mutex --> crtc_ww_class_mutex --> &kms->commit_lock[i]
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(&kms->commit_lock[i]);
> lock(crtc_ww_class_mutex);
> lock(&kms->commit_lock[i]);
> lock(&dev->mode_config.mutex);
>
> *** DEADLOCK ***
>
> 3 locks held by frecon/429:
> #0: ffffffc00e197ab0 (crtc_ww_class_acquire){+.+.}-{0:0}, at:
> drm_modeset_acquire_init+0x44/0xc8
> #1: ffffff808dc3c588 (crtc_ww_class_mutex){+.+.}-{3:3}, at:
> modeset_lock+0x18c/0x4ac
> #2: ffffff808dc441e0 (&kms->commit_lock[i]){+.+.}-{3:3}, at:
> lock_crtcs+0xb4/0x124
>
> stack backtrace:
> CPU: 5 PID: 429 Comm: frecon Tainted: G W
> 5.15.35-lockdep #6 9ba2ecd8f15354021fe165873da3aaa99f5b6798
> Hardware name: Google Herobrine (rev1+) (DT)
> Call trace:
> dump_backtrace+0x0/0x3c4
> show_stack+0x20/0x2c
> dump_stack_lvl+0x78/0x9c
> dump_stack+0x18/0x44
> print_circular_bug+0x17c/0x1a8
> check_noncircular+0x260/0x30c
> __lock_acquire+0x2650/0x672c
> lock_acquire+0x1b4/0x4ac
> __mutex_lock_common+0x174/0x1a64
> mutex_lock_nested+0x98/0xac
> dp_panel_add_fail_safe_mode+0x4c/0xa0
> dp_hpd_plug_handle+0x1f0/0x280
> dp_bridge_enable+0x94/0x2b8
> drm_atomic_bridge_chain_enable+0x11c/0x168
> drm_atomic_helper_commit_modeset_enables+0x500/0x740
> msm_atomic_commit_tail+0x3e4/0x748
> commit_tail+0x19c/0x278
> drm_atomic_helper_commit+0x1dc/0x1f0
> drm_atomic_commit+0xc0/0xd8
> drm_atomic_helper_set_config+0xb4/0x134
> drm_mode_setcrtc+0x688/0x1248
> drm_ioctl_kernel+0x1e4/0x338
> drm_ioctl+0x3a4/0x684
> __arm64_sys_ioctl+0x118/0x154
> invoke_syscall+0x78/0x224
> el0_svc_common+0x178/0x200
> do_el0_svc+0x94/0x13c
> el0_svc+0x5c/0xec
> el0t_64_sync_handler+0x78/0x108
> el0t_64_sync+0x1a4/0x1a8
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
2022-04-22 16:04 ` Abhinav Kumar
@ 2022-04-22 16:10 ` Doug Anderson
-1 siblings, 0 replies; 44+ messages in thread
From: Doug Anderson @ 2022-04-22 16:10 UTC (permalink / raw)
To: Abhinav Kumar
Cc: Sankeerth Billakanti, quic_kalyant,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Bjorn Andersson, quic_vproddut, David Airlie, linux-arm-msm,
LKML, dri-devel, Stephen Boyd, Sean Paul, Sean Paul,
Steev Klimaszewski, Dmitry Baryshkov,
Aravind Venkateswaran (QUIC), Kuogee Hsieh (QUIC),
freedreno
Hi,
On Fri, Apr 22, 2022 at 9:05 AM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Hi Doug
>
> For the lockdep error, the splat looks similar to what kuogee fixed
> recently.
>
> Can you please check if below patch is present in your tree?
>
> https://patchwork.freedesktop.org/patch/481396/
Indeed I did have that in my tree already, but the lockdep splat is
still there. I think the problem is that we're now calling
dp_hpd_plug_handle() directly in dp_bridge_enable()
-Doug
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
@ 2022-04-22 16:10 ` Doug Anderson
0 siblings, 0 replies; 44+ messages in thread
From: Doug Anderson @ 2022-04-22 16:10 UTC (permalink / raw)
To: Abhinav Kumar
Cc: quic_kalyant,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Sankeerth Billakanti, quic_vproddut, David Airlie, linux-arm-msm,
Kuogee Hsieh (QUIC),
freedreno, LKML, dri-devel, Bjorn Andersson, Sean Paul,
Steev Klimaszewski, Dmitry Baryshkov,
Aravind Venkateswaran (QUIC),
Stephen Boyd, Sean Paul
Hi,
On Fri, Apr 22, 2022 at 9:05 AM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Hi Doug
>
> For the lockdep error, the splat looks similar to what kuogee fixed
> recently.
>
> Can you please check if below patch is present in your tree?
>
> https://patchwork.freedesktop.org/patch/481396/
Indeed I did have that in my tree already, but the lockdep splat is
still there. I think the problem is that we're now calling
dp_hpd_plug_handle() directly in dp_bridge_enable()
-Doug
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
2022-04-22 16:10 ` Doug Anderson
@ 2022-04-22 17:35 ` Abhinav Kumar
-1 siblings, 0 replies; 44+ messages in thread
From: Abhinav Kumar @ 2022-04-22 17:35 UTC (permalink / raw)
To: Doug Anderson
Cc: quic_kalyant,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Sankeerth Billakanti, quic_vproddut, David Airlie, linux-arm-msm,
Kuogee Hsieh (QUIC),
freedreno, LKML, dri-devel, Bjorn Andersson, Sean Paul,
Steev Klimaszewski, Dmitry Baryshkov,
Aravind Venkateswaran (QUIC),
Stephen Boyd, Sean Paul
Hi Doug
On 4/22/2022 9:10 AM, Doug Anderson wrote:
> Hi,
>
> On Fri, Apr 22, 2022 at 9:05 AM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> Hi Doug
>>
>> For the lockdep error, the splat looks similar to what kuogee fixed
>> recently.
>>
>> Can you please check if below patch is present in your tree?
>>
>> https://patchwork.freedesktop.org/patch/481396/
>
> Indeed I did have that in my tree already, but the lockdep splat is
> still there. I think the problem is that we're now calling
> dp_hpd_plug_handle() directly in dp_bridge_enable()
>
> -Doug
Yes, now i understood this particular issue better and not sure how this
wasn't caught. Perhaps some difference in the USE flags. Sankeerth didnt
have lockdebug and thats why didnt hit this.
I have discussed with kuogee about why this change is needed and why
this wasnt being done in get_modes().
It seems like originally, this was done for a quirk in the DP compliance
equipment that it did not publish the fail safe mode ( even though some
other modes were present ). Typically, any sink (as long as EDID read
went through ) adds the 640x480 fail safe mode.
We could have done it in get_modes() even earlier but not sure how it
was missed or was there some other reason.
Nonetheless, kuogee will post the change to move this to get_modes()
shortly.
Thanks
Abhinav
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
@ 2022-04-22 17:35 ` Abhinav Kumar
0 siblings, 0 replies; 44+ messages in thread
From: Abhinav Kumar @ 2022-04-22 17:35 UTC (permalink / raw)
To: Doug Anderson
Cc: Sankeerth Billakanti, quic_kalyant,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Bjorn Andersson, quic_vproddut, David Airlie, linux-arm-msm,
LKML, dri-devel, Stephen Boyd, Sean Paul, Sean Paul,
Steev Klimaszewski, Dmitry Baryshkov,
Aravind Venkateswaran (QUIC), Kuogee Hsieh (QUIC),
freedreno
Hi Doug
On 4/22/2022 9:10 AM, Doug Anderson wrote:
> Hi,
>
> On Fri, Apr 22, 2022 at 9:05 AM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> Hi Doug
>>
>> For the lockdep error, the splat looks similar to what kuogee fixed
>> recently.
>>
>> Can you please check if below patch is present in your tree?
>>
>> https://patchwork.freedesktop.org/patch/481396/
>
> Indeed I did have that in my tree already, but the lockdep splat is
> still there. I think the problem is that we're now calling
> dp_hpd_plug_handle() directly in dp_bridge_enable()
>
> -Doug
Yes, now i understood this particular issue better and not sure how this
wasn't caught. Perhaps some difference in the USE flags. Sankeerth didnt
have lockdebug and thats why didnt hit this.
I have discussed with kuogee about why this change is needed and why
this wasnt being done in get_modes().
It seems like originally, this was done for a quirk in the DP compliance
equipment that it did not publish the fail safe mode ( even though some
other modes were present ). Typically, any sink (as long as EDID read
went through ) adds the 640x480 fail safe mode.
We could have done it in get_modes() even earlier but not sure how it
was missed or was there some other reason.
Nonetheless, kuogee will post the change to move this to get_modes()
shortly.
Thanks
Abhinav
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v9 1/4] drm/msm/dp: Add eDP support via aux_bus
2022-04-22 9:11 ` Sankeerth Billakanti
@ 2022-04-23 0:13 ` Stephen Boyd
-1 siblings, 0 replies; 44+ messages in thread
From: Stephen Boyd @ 2022-04-23 0:13 UTC (permalink / raw)
To: Sankeerth Billakanti, devicetree, dri-devel, freedreno,
linux-arm-msm, linux-kernel
Cc: robdclark, seanpaul, quic_kalyant, quic_abhinavk, dianders,
quic_khsieh, bjorn.andersson, sean, airlied, daniel,
dmitry.baryshkov, quic_vproddut, quic_aravindh, steev
Quoting Sankeerth Billakanti (2022-04-22 02:11:03)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index d7a19d6..055681a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
Some nitpicks
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> @@ -1508,7 +1509,8 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
>
> dp_hpd_event_setup(dp);
>
> - dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
> + if (!dp_display->is_edp)
> + dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
Did it turn out that in fact DP isn't ready still to setup even after
delaying the irq?
> }
>
> void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
> @@ -1530,6 +1532,65 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
> }
> }
>
> +static int dp_display_get_next_bridge(struct msm_dp *dp)
> +{
> + int rc;
> + struct dp_display_private *dp_priv;
> + struct device_node *aux_bus;
> + struct device *dev;
> +
> + dp_priv = container_of(dp, struct dp_display_private, dp_display);
> + dev = &dp_priv->pdev->dev;
> + aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
> +
> + if (aux_bus && dp->is_edp) {
> + dp_display_host_init(dp_priv);
> + dp_catalog_ctrl_hpd_config(dp_priv->catalog);
> + dp_display_host_phy_init(dp_priv);
> + enable_irq(dp_priv->irq);
> +
> + /*
> + * The code below assumes that the panel will finish probing
> + * by the time devm_of_dp_aux_populate_ep_devices() returns.
> + * This isn't a great assumption since it will fail if the
> + * panel driver is probed asynchronously but is the best we
> + * can do without a bigger driver reorganization.
> + */
> + rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
> + of_node_put(aux_bus);
> + if (rc)
> + goto error;
> + } else if (dp->is_edp) {
> + DRM_ERROR("eDP aux_bus not found\n");
> + return -ENODEV;
> + }
> +
> + /*
> + * External bridges are mandatory for eDP interfaces: one has to
> + * provide at least an eDP panel (which gets wrapped into panel-bridge).
> + *
> + * For DisplayPort interfaces external bridges are optional, so
> + * silently ignore an error if one is not present (-ENODEV).
> + */
> + rc = dp_parser_find_next_bridge(dp_priv->parser);
> + if (!dp->is_edp && rc == -ENODEV)
> + return 0;
> + else if (rc)
Just an if because there's a return above.
> + goto error;
> +
> + dp->next_bridge = dp_priv->parser->next_bridge;
In which case it almost feels like it could be written
if (!dp->is_edp && rc == -ENODEV)
return 0;
if (!rc) {
dp->next_bridge = dp_priv->parser->next_bridge;
return 0;
}
error:
if (dp->is_edp) {
but I'm not worried either way, besides removing the else from the else-if.
> +
> + return 0;
> +
> +error:
> + if (dp->is_edp) {
> + disable_irq(dp_priv->irq);
> + dp_display_host_phy_exit(dp_priv);
> + dp_display_host_deinit(dp_priv);
> + }
> + return rc;
> +}
> +
> int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
> struct drm_encoder *encoder)
> {
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
> index d371bae..950416c 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);
> };
>
> /**
> @@ -141,4 +141,15 @@ struct dp_parser {
> */
> struct dp_parser *dp_parser_get(struct platform_device *pdev);
>
> +/**
> + * dp_parser_find_next_bridge() - find an additional bridge to DP
> + *
> + * @parser: dp_parser data from client
> + * return: 0 if able to get the bridge else return an error code
return comes after the description below. Also should be capitalized.
You can check this by compiling with W=1 I believe, or run the
kernel doc format script on the file..
> + *
> + * This function is used to find any additional bridge attached to
> + * the DP controller. The eDP interface requires a panel bridge.
Return: 0 if able to get the bridge, otherwise negative errno for failure
> + */
> +int dp_parser_find_next_bridge(struct dp_parser *parser);
> +
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v9 1/4] drm/msm/dp: Add eDP support via aux_bus
@ 2022-04-23 0:13 ` Stephen Boyd
0 siblings, 0 replies; 44+ messages in thread
From: Stephen Boyd @ 2022-04-23 0:13 UTC (permalink / raw)
To: Sankeerth Billakanti, devicetree, dri-devel, freedreno,
linux-arm-msm, linux-kernel
Cc: quic_kalyant, dianders, quic_vproddut, airlied, quic_abhinavk,
steev, quic_khsieh, seanpaul, dmitry.baryshkov, quic_aravindh,
bjorn.andersson, sean
Quoting Sankeerth Billakanti (2022-04-22 02:11:03)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index d7a19d6..055681a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
Some nitpicks
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> @@ -1508,7 +1509,8 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
>
> dp_hpd_event_setup(dp);
>
> - dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
> + if (!dp_display->is_edp)
> + dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
Did it turn out that in fact DP isn't ready still to setup even after
delaying the irq?
> }
>
> void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
> @@ -1530,6 +1532,65 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
> }
> }
>
> +static int dp_display_get_next_bridge(struct msm_dp *dp)
> +{
> + int rc;
> + struct dp_display_private *dp_priv;
> + struct device_node *aux_bus;
> + struct device *dev;
> +
> + dp_priv = container_of(dp, struct dp_display_private, dp_display);
> + dev = &dp_priv->pdev->dev;
> + aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
> +
> + if (aux_bus && dp->is_edp) {
> + dp_display_host_init(dp_priv);
> + dp_catalog_ctrl_hpd_config(dp_priv->catalog);
> + dp_display_host_phy_init(dp_priv);
> + enable_irq(dp_priv->irq);
> +
> + /*
> + * The code below assumes that the panel will finish probing
> + * by the time devm_of_dp_aux_populate_ep_devices() returns.
> + * This isn't a great assumption since it will fail if the
> + * panel driver is probed asynchronously but is the best we
> + * can do without a bigger driver reorganization.
> + */
> + rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
> + of_node_put(aux_bus);
> + if (rc)
> + goto error;
> + } else if (dp->is_edp) {
> + DRM_ERROR("eDP aux_bus not found\n");
> + return -ENODEV;
> + }
> +
> + /*
> + * External bridges are mandatory for eDP interfaces: one has to
> + * provide at least an eDP panel (which gets wrapped into panel-bridge).
> + *
> + * For DisplayPort interfaces external bridges are optional, so
> + * silently ignore an error if one is not present (-ENODEV).
> + */
> + rc = dp_parser_find_next_bridge(dp_priv->parser);
> + if (!dp->is_edp && rc == -ENODEV)
> + return 0;
> + else if (rc)
Just an if because there's a return above.
> + goto error;
> +
> + dp->next_bridge = dp_priv->parser->next_bridge;
In which case it almost feels like it could be written
if (!dp->is_edp && rc == -ENODEV)
return 0;
if (!rc) {
dp->next_bridge = dp_priv->parser->next_bridge;
return 0;
}
error:
if (dp->is_edp) {
but I'm not worried either way, besides removing the else from the else-if.
> +
> + return 0;
> +
> +error:
> + if (dp->is_edp) {
> + disable_irq(dp_priv->irq);
> + dp_display_host_phy_exit(dp_priv);
> + dp_display_host_deinit(dp_priv);
> + }
> + return rc;
> +}
> +
> int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
> struct drm_encoder *encoder)
> {
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
> index d371bae..950416c 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);
> };
>
> /**
> @@ -141,4 +141,15 @@ struct dp_parser {
> */
> struct dp_parser *dp_parser_get(struct platform_device *pdev);
>
> +/**
> + * dp_parser_find_next_bridge() - find an additional bridge to DP
> + *
> + * @parser: dp_parser data from client
> + * return: 0 if able to get the bridge else return an error code
return comes after the description below. Also should be capitalized.
You can check this by compiling with W=1 I believe, or run the
kernel doc format script on the file..
> + *
> + * This function is used to find any additional bridge attached to
> + * the DP controller. The eDP interface requires a panel bridge.
Return: 0 if able to get the bridge, otherwise negative errno for failure
> + */
> +int dp_parser_find_next_bridge(struct dp_parser *parser);
> +
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
2022-04-22 9:11 ` Sankeerth Billakanti
@ 2022-04-23 0:29 ` Stephen Boyd
-1 siblings, 0 replies; 44+ messages in thread
From: Stephen Boyd @ 2022-04-23 0:29 UTC (permalink / raw)
To: Sankeerth Billakanti, devicetree, dri-devel, freedreno,
linux-arm-msm, linux-kernel
Cc: robdclark, seanpaul, quic_kalyant, quic_abhinavk, dianders,
quic_khsieh, bjorn.andersson, sean, airlied, daniel,
dmitry.baryshkov, quic_vproddut, quic_aravindh, steev
Quoting Sankeerth Billakanti (2022-04-22 02:11:04)
> The panel-edp enables the eDP panel power during probe, get_modes
> and pre-enable. The eDP connect and disconnect interrupts for the eDP/DP
> controller are directly dependent on panel power. As eDP display can be
> assumed as always connected, the controller driver can skip the eDP
> connect and disconnect interrupts. Any disruption in the link status
> will be indicated via the IRQ_HPD interrupts.
>
> So, the eDP controller driver can just enable the IRQ_HPD and replug
> interrupts. The DP controller driver still needs to enable all the
> interrupts.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
The sprinkling of if conditions and manual driving of the DP plug/unplug
state machine is pretty convoluted. To make it better the driver needs
an overhaul. Anyway, it looks mostly fine to me except for this replug
interrupt question below. Otherwise
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 055681a..dea4de9 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1096,6 +1097,13 @@ static void dp_display_config_hpd(struct dp_display_private *dp)
> dp_display_host_init(dp);
> dp_catalog_ctrl_hpd_config(dp->catalog);
>
> + /* Enable plug and unplug interrupts only for external DisplayPort */
> + if (!dp->dp_display.is_edp)
> + dp_catalog_hpd_config_intr(dp->catalog,
> + DP_DP_HPD_PLUG_INT_MASK |
> + DP_DP_HPD_UNPLUG_INT_MASK,
> + true);
> +
It seems like only the plug and unplug is enabled for DP here. Is replug
enabled for eDP when it shouldn't be?
> /* Enable interrupt first time
> * we are leaving dp clocks on during disconnect
> * and never disable interrupt
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
@ 2022-04-23 0:29 ` Stephen Boyd
0 siblings, 0 replies; 44+ messages in thread
From: Stephen Boyd @ 2022-04-23 0:29 UTC (permalink / raw)
To: Sankeerth Billakanti, devicetree, dri-devel, freedreno,
linux-arm-msm, linux-kernel
Cc: quic_kalyant, dianders, quic_vproddut, airlied, quic_abhinavk,
steev, quic_khsieh, seanpaul, dmitry.baryshkov, quic_aravindh,
bjorn.andersson, sean
Quoting Sankeerth Billakanti (2022-04-22 02:11:04)
> The panel-edp enables the eDP panel power during probe, get_modes
> and pre-enable. The eDP connect and disconnect interrupts for the eDP/DP
> controller are directly dependent on panel power. As eDP display can be
> assumed as always connected, the controller driver can skip the eDP
> connect and disconnect interrupts. Any disruption in the link status
> will be indicated via the IRQ_HPD interrupts.
>
> So, the eDP controller driver can just enable the IRQ_HPD and replug
> interrupts. The DP controller driver still needs to enable all the
> interrupts.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
The sprinkling of if conditions and manual driving of the DP plug/unplug
state machine is pretty convoluted. To make it better the driver needs
an overhaul. Anyway, it looks mostly fine to me except for this replug
interrupt question below. Otherwise
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 055681a..dea4de9 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1096,6 +1097,13 @@ static void dp_display_config_hpd(struct dp_display_private *dp)
> dp_display_host_init(dp);
> dp_catalog_ctrl_hpd_config(dp->catalog);
>
> + /* Enable plug and unplug interrupts only for external DisplayPort */
> + if (!dp->dp_display.is_edp)
> + dp_catalog_hpd_config_intr(dp->catalog,
> + DP_DP_HPD_PLUG_INT_MASK |
> + DP_DP_HPD_UNPLUG_INT_MASK,
> + true);
> +
It seems like only the plug and unplug is enabled for DP here. Is replug
enabled for eDP when it shouldn't be?
> /* Enable interrupt first time
> * we are leaving dp clocks on during disconnect
> * and never disable interrupt
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v9 3/4] drm/msm/dp: wait for hpd high before aux transaction
2022-04-22 9:11 ` Sankeerth Billakanti
@ 2022-04-23 1:38 ` Stephen Boyd
-1 siblings, 0 replies; 44+ messages in thread
From: Stephen Boyd @ 2022-04-23 1:38 UTC (permalink / raw)
To: Sankeerth Billakanti, devicetree, dri-devel, freedreno,
linux-arm-msm, linux-kernel
Cc: robdclark, seanpaul, quic_kalyant, quic_abhinavk, dianders,
quic_khsieh, bjorn.andersson, sean, airlied, daniel,
dmitry.baryshkov, quic_vproddut, quic_aravindh, steev
Quoting Sankeerth Billakanti (2022-04-22 02:11:05)
> The source device should ensure the sink is ready before proceeding to
> read the sink capability or perform any aux transactions. The sink
> will indicate its readiness by asserting the HPD line. The controller
> driver needs to wait for the hpd line to be asserted by the sink before
> it performs any aux transactions.
>
> The eDP sink is assumed to be always connected. It needs power from the
> source and its HPD line will be asserted only after the panel is powered
> on. The panel power will be enabled from the panel-edp driver and only
> after that, the hpd line will be asserted.
>
> Whereas for DP, the sink can be hotplugged and unplugged anytime. The hpd
> line gets asserted to indicate the sink is connected and ready. Hence
> there is no need to wait for the hpd line to be asserted for a DP sink.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> ---
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v9 3/4] drm/msm/dp: wait for hpd high before aux transaction
@ 2022-04-23 1:38 ` Stephen Boyd
0 siblings, 0 replies; 44+ messages in thread
From: Stephen Boyd @ 2022-04-23 1:38 UTC (permalink / raw)
To: Sankeerth Billakanti, devicetree, dri-devel, freedreno,
linux-arm-msm, linux-kernel
Cc: quic_kalyant, dianders, quic_vproddut, airlied, quic_abhinavk,
steev, quic_khsieh, seanpaul, dmitry.baryshkov, quic_aravindh,
bjorn.andersson, sean
Quoting Sankeerth Billakanti (2022-04-22 02:11:05)
> The source device should ensure the sink is ready before proceeding to
> read the sink capability or perform any aux transactions. The sink
> will indicate its readiness by asserting the HPD line. The controller
> driver needs to wait for the hpd line to be asserted by the sink before
> it performs any aux transactions.
>
> The eDP sink is assumed to be always connected. It needs power from the
> source and its HPD line will be asserted only after the panel is powered
> on. The panel power will be enabled from the panel-edp driver and only
> after that, the hpd line will be asserted.
>
> Whereas for DP, the sink can be hotplugged and unplugged anytime. The hpd
> line gets asserted to indicate the sink is connected and ready. Hence
> there is no need to wait for the hpd line to be asserted for a DP sink.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> ---
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v9 4/4] drm/msm/dp: Support the eDP modes given by panel
2022-04-22 9:11 ` Sankeerth Billakanti
@ 2022-04-23 1:39 ` Stephen Boyd
-1 siblings, 0 replies; 44+ messages in thread
From: Stephen Boyd @ 2022-04-23 1:39 UTC (permalink / raw)
To: Sankeerth Billakanti, devicetree, dri-devel, freedreno,
linux-arm-msm, linux-kernel
Cc: robdclark, seanpaul, quic_kalyant, quic_abhinavk, dianders,
quic_khsieh, bjorn.andersson, sean, airlied, daniel,
dmitry.baryshkov, quic_vproddut, quic_aravindh, steev
Quoting Sankeerth Billakanti (2022-04-22 02:11:06)
> The eDP controller does not have a reliable way keep panel
> powered on to read the sink capabilities. So, the controller
> driver cannot validate if a mode can be supported by the
> source. We will rely on the panel driver to populate only
> the supported modes for now.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> ---
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v9 4/4] drm/msm/dp: Support the eDP modes given by panel
@ 2022-04-23 1:39 ` Stephen Boyd
0 siblings, 0 replies; 44+ messages in thread
From: Stephen Boyd @ 2022-04-23 1:39 UTC (permalink / raw)
To: Sankeerth Billakanti, devicetree, dri-devel, freedreno,
linux-arm-msm, linux-kernel
Cc: quic_kalyant, dianders, quic_vproddut, airlied, quic_abhinavk,
steev, quic_khsieh, seanpaul, dmitry.baryshkov, quic_aravindh,
bjorn.andersson, sean
Quoting Sankeerth Billakanti (2022-04-22 02:11:06)
> The eDP controller does not have a reliable way keep panel
> powered on to read the sink capabilities. So, the controller
> driver cannot validate if a mode can be supported by the
> source. We will rely on the panel driver to populate only
> the supported modes for now.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> ---
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
^ permalink raw reply [flat|nested] 44+ messages in thread
* RE: [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
2022-04-23 0:29 ` Stephen Boyd
@ 2022-04-25 2:55 ` Sankeerth Billakanti (QUIC)
-1 siblings, 0 replies; 44+ messages in thread
From: Sankeerth Billakanti (QUIC) @ 2022-04-25 2:55 UTC (permalink / raw)
To: Stephen Boyd, Sankeerth Billakanti (QUIC),
devicetree, dri-devel, freedreno, linux-arm-msm, linux-kernel
Cc: robdclark, seanpaul, quic_kalyant, Abhinav Kumar (QUIC),
dianders, Kuogee Hsieh (QUIC),
bjorn.andersson, sean, airlied, daniel, dmitry.baryshkov,
quic_vproddut, Aravind Venkateswaran (QUIC),
steev
Hi Stephen,
>Quoting Sankeerth Billakanti (2022-04-22 02:11:04)
>> The panel-edp enables the eDP panel power during probe, get_modes and
>> pre-enable. The eDP connect and disconnect interrupts for the eDP/DP
>> controller are directly dependent on panel power. As eDP display can
>> be assumed as always connected, the controller driver can skip the eDP
>> connect and disconnect interrupts. Any disruption in the link status
>> will be indicated via the IRQ_HPD interrupts.
>>
>> So, the eDP controller driver can just enable the IRQ_HPD and replug
>> interrupts. The DP controller driver still needs to enable all the
>> interrupts.
>>
>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
>
>The sprinkling of if conditions and manual driving of the DP plug/unplug state
>machine is pretty convoluted. To make it better the driver needs an overhaul.
>Anyway, it looks mostly fine to me except for this replug interrupt question
>below. Otherwise
>
>Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>
>> int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog) diff
>> --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 055681a..dea4de9 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -1096,6 +1097,13 @@ static void dp_display_config_hpd(struct
>dp_display_private *dp)
>> dp_display_host_init(dp);
>> dp_catalog_ctrl_hpd_config(dp->catalog);
>>
>> + /* Enable plug and unplug interrupts only for external DisplayPort */
>> + if (!dp->dp_display.is_edp)
>> + dp_catalog_hpd_config_intr(dp->catalog,
>> + DP_DP_HPD_PLUG_INT_MASK |
>> + DP_DP_HPD_UNPLUG_INT_MASK,
>> + true);
>> +
>
>It seems like only the plug and unplug is enabled for DP here. Is replug
>enabled for eDP when it shouldn't be?
>
By default, all the interrupts are masked. This function is not executed for eDP
anymore because the host_init, phy_init and enable_irq are all done from
modeset_init for eDP with aux_bus. So, none of the eDP hpd interrupts are
enabled or unmasked before pre-enable.
The replug interrupt is unmasked for DP and eDP from the dp_hpd_plug_handle()
and masked from dp_hpd_unplug_handle().
>> /* Enable interrupt first time
>> * we are leaving dp clocks on during disconnect
>> * and never disable interrupt
^ permalink raw reply [flat|nested] 44+ messages in thread
* RE: [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
@ 2022-04-25 2:55 ` Sankeerth Billakanti (QUIC)
0 siblings, 0 replies; 44+ messages in thread
From: Sankeerth Billakanti (QUIC) @ 2022-04-25 2:55 UTC (permalink / raw)
To: Stephen Boyd, Sankeerth Billakanti (QUIC),
devicetree, dri-devel, freedreno, linux-arm-msm, linux-kernel
Cc: quic_kalyant, dianders, quic_vproddut, airlied,
Abhinav Kumar (QUIC), steev, Kuogee Hsieh (QUIC),
seanpaul, dmitry.baryshkov, Aravind Venkateswaran (QUIC),
bjorn.andersson, sean
Hi Stephen,
>Quoting Sankeerth Billakanti (2022-04-22 02:11:04)
>> The panel-edp enables the eDP panel power during probe, get_modes and
>> pre-enable. The eDP connect and disconnect interrupts for the eDP/DP
>> controller are directly dependent on panel power. As eDP display can
>> be assumed as always connected, the controller driver can skip the eDP
>> connect and disconnect interrupts. Any disruption in the link status
>> will be indicated via the IRQ_HPD interrupts.
>>
>> So, the eDP controller driver can just enable the IRQ_HPD and replug
>> interrupts. The DP controller driver still needs to enable all the
>> interrupts.
>>
>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
>
>The sprinkling of if conditions and manual driving of the DP plug/unplug state
>machine is pretty convoluted. To make it better the driver needs an overhaul.
>Anyway, it looks mostly fine to me except for this replug interrupt question
>below. Otherwise
>
>Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>
>> int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog) diff
>> --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 055681a..dea4de9 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -1096,6 +1097,13 @@ static void dp_display_config_hpd(struct
>dp_display_private *dp)
>> dp_display_host_init(dp);
>> dp_catalog_ctrl_hpd_config(dp->catalog);
>>
>> + /* Enable plug and unplug interrupts only for external DisplayPort */
>> + if (!dp->dp_display.is_edp)
>> + dp_catalog_hpd_config_intr(dp->catalog,
>> + DP_DP_HPD_PLUG_INT_MASK |
>> + DP_DP_HPD_UNPLUG_INT_MASK,
>> + true);
>> +
>
>It seems like only the plug and unplug is enabled for DP here. Is replug
>enabled for eDP when it shouldn't be?
>
By default, all the interrupts are masked. This function is not executed for eDP
anymore because the host_init, phy_init and enable_irq are all done from
modeset_init for eDP with aux_bus. So, none of the eDP hpd interrupts are
enabled or unmasked before pre-enable.
The replug interrupt is unmasked for DP and eDP from the dp_hpd_plug_handle()
and masked from dp_hpd_unplug_handle().
>> /* Enable interrupt first time
>> * we are leaving dp clocks on during disconnect
>> * and never disable interrupt
^ permalink raw reply [flat|nested] 44+ messages in thread
* RE: [PATCH v9 1/4] drm/msm/dp: Add eDP support via aux_bus
2022-04-23 0:13 ` Stephen Boyd
@ 2022-04-25 9:39 ` Sankeerth Billakanti (QUIC)
-1 siblings, 0 replies; 44+ messages in thread
From: Sankeerth Billakanti (QUIC) @ 2022-04-25 9:39 UTC (permalink / raw)
To: Stephen Boyd, Sankeerth Billakanti (QUIC),
devicetree, dri-devel, freedreno, linux-arm-msm, linux-kernel
Cc: quic_kalyant, dianders, quic_vproddut, airlied,
Abhinav Kumar (QUIC), steev, Kuogee Hsieh (QUIC),
seanpaul, dmitry.baryshkov, Aravind Venkateswaran (QUIC),
bjorn.andersson, sean
Hi Stephen,
>Quoting Sankeerth Billakanti (2022-04-22 02:11:03)
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index d7a19d6..055681a 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>
>Some nitpicks
>
>Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>
>> @@ -1508,7 +1509,8 @@ void msm_dp_irq_postinstall(struct msm_dp
>> *dp_display)
>>
>> dp_hpd_event_setup(dp);
>>
>> - dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
>> + if (!dp_display->is_edp)
>> + dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
>
>Did it turn out that in fact DP isn't ready still to setup even after delaying the
>irq?
>
The host_init, config_hpd, phy_init and enable_irq are happening in modeset_init already for eDP.
So, I am not scheduling the EV_HPD_INIT_SETUP event for eDP. I am not modifying the delay for DP.
>> }
>>
>> void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor
>> *minor) @@ -1530,6 +1532,65 @@ void msm_dp_debugfs_init(struct
>msm_dp *dp_display, struct drm_minor *minor)
>> }
>> }
>>
>> +static int dp_display_get_next_bridge(struct msm_dp *dp) {
>> + int rc;
>> + struct dp_display_private *dp_priv;
>> + struct device_node *aux_bus;
>> + struct device *dev;
>> +
>> + dp_priv = container_of(dp, struct dp_display_private, dp_display);
>> + dev = &dp_priv->pdev->dev;
>> + aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
>> +
>> + if (aux_bus && dp->is_edp) {
>> + dp_display_host_init(dp_priv);
>> + dp_catalog_ctrl_hpd_config(dp_priv->catalog);
>> + dp_display_host_phy_init(dp_priv);
>> + enable_irq(dp_priv->irq);
>> +
>> + /*
>> + * The code below assumes that the panel will finish probing
>> + * by the time devm_of_dp_aux_populate_ep_devices() returns.
>> + * This isn't a great assumption since it will fail if the
>> + * panel driver is probed asynchronously but is the best we
>> + * can do without a bigger driver reorganization.
>> + */
>> + rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
>> + of_node_put(aux_bus);
>> + if (rc)
>> + goto error;
>> + } else if (dp->is_edp) {
>> + DRM_ERROR("eDP aux_bus not found\n");
>> + return -ENODEV;
>> + }
>> +
>> + /*
>> + * External bridges are mandatory for eDP interfaces: one has to
>> + * provide at least an eDP panel (which gets wrapped into panel-
>bridge).
>> + *
>> + * For DisplayPort interfaces external bridges are optional, so
>> + * silently ignore an error if one is not present (-ENODEV).
>> + */
>> + rc = dp_parser_find_next_bridge(dp_priv->parser);
>> + if (!dp->is_edp && rc == -ENODEV)
>> + return 0;
>> + else if (rc)
>
>Just an if because there's a return above.
>
Okay
>> + goto error;
>> +
>> + dp->next_bridge = dp_priv->parser->next_bridge;
>
>In which case it almost feels like it could be written
>
> if (!dp->is_edp && rc == -ENODEV)
> return 0;
> if (!rc) {
> dp->next_bridge = dp_priv->parser->next_bridge;
> return 0;
> }
>error:
> if (dp->is_edp) {
>
>but I'm not worried either way, besides removing the else from the else-if.
>
Okay
>> +
>> + return 0;
>> +
>> +error:
>> + if (dp->is_edp) {
>> + disable_irq(dp_priv->irq);
>> + dp_display_host_phy_exit(dp_priv);
>> + dp_display_host_deinit(dp_priv);
>> + }
>> + return rc;
>> +}
>> +
>> int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device
>*dev,
>> struct drm_encoder *encoder) { diff --git
>> a/drivers/gpu/drm/msm/dp/dp_parser.h
>> b/drivers/gpu/drm/msm/dp/dp_parser.h
>> index d371bae..950416c 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);
>> };
>>
>> /**
>> @@ -141,4 +141,15 @@ struct dp_parser {
>> */
>> struct dp_parser *dp_parser_get(struct platform_device *pdev);
>>
>> +/**
>> + * dp_parser_find_next_bridge() - find an additional bridge to DP
>> + *
>> + * @parser: dp_parser data from client
>> + * return: 0 if able to get the bridge else return an error code
>
>return comes after the description below. Also should be capitalized.
>You can check this by compiling with W=1 I believe, or run the kernel doc
>format script on the file..
>
Okay
>> + *
>> + * This function is used to find any additional bridge attached to
>> + * the DP controller. The eDP interface requires a panel bridge.
>
>Return: 0 if able to get the bridge, otherwise negative errno for failure
>
Okay
>> + */
>> +int dp_parser_find_next_bridge(struct dp_parser *parser);
>> +
^ permalink raw reply [flat|nested] 44+ messages in thread
* RE: [PATCH v9 1/4] drm/msm/dp: Add eDP support via aux_bus
@ 2022-04-25 9:39 ` Sankeerth Billakanti (QUIC)
0 siblings, 0 replies; 44+ messages in thread
From: Sankeerth Billakanti (QUIC) @ 2022-04-25 9:39 UTC (permalink / raw)
To: Stephen Boyd, Sankeerth Billakanti (QUIC),
devicetree, dri-devel, freedreno, linux-arm-msm, linux-kernel
Cc: robdclark, seanpaul, quic_kalyant, Abhinav Kumar (QUIC),
dianders, Kuogee Hsieh (QUIC),
bjorn.andersson, sean, airlied, daniel, dmitry.baryshkov,
quic_vproddut, Aravind Venkateswaran (QUIC),
steev
Hi Stephen,
>Quoting Sankeerth Billakanti (2022-04-22 02:11:03)
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index d7a19d6..055681a 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>
>Some nitpicks
>
>Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>
>> @@ -1508,7 +1509,8 @@ void msm_dp_irq_postinstall(struct msm_dp
>> *dp_display)
>>
>> dp_hpd_event_setup(dp);
>>
>> - dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
>> + if (!dp_display->is_edp)
>> + dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
>
>Did it turn out that in fact DP isn't ready still to setup even after delaying the
>irq?
>
The host_init, config_hpd, phy_init and enable_irq are happening in modeset_init already for eDP.
So, I am not scheduling the EV_HPD_INIT_SETUP event for eDP. I am not modifying the delay for DP.
>> }
>>
>> void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor
>> *minor) @@ -1530,6 +1532,65 @@ void msm_dp_debugfs_init(struct
>msm_dp *dp_display, struct drm_minor *minor)
>> }
>> }
>>
>> +static int dp_display_get_next_bridge(struct msm_dp *dp) {
>> + int rc;
>> + struct dp_display_private *dp_priv;
>> + struct device_node *aux_bus;
>> + struct device *dev;
>> +
>> + dp_priv = container_of(dp, struct dp_display_private, dp_display);
>> + dev = &dp_priv->pdev->dev;
>> + aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
>> +
>> + if (aux_bus && dp->is_edp) {
>> + dp_display_host_init(dp_priv);
>> + dp_catalog_ctrl_hpd_config(dp_priv->catalog);
>> + dp_display_host_phy_init(dp_priv);
>> + enable_irq(dp_priv->irq);
>> +
>> + /*
>> + * The code below assumes that the panel will finish probing
>> + * by the time devm_of_dp_aux_populate_ep_devices() returns.
>> + * This isn't a great assumption since it will fail if the
>> + * panel driver is probed asynchronously but is the best we
>> + * can do without a bigger driver reorganization.
>> + */
>> + rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
>> + of_node_put(aux_bus);
>> + if (rc)
>> + goto error;
>> + } else if (dp->is_edp) {
>> + DRM_ERROR("eDP aux_bus not found\n");
>> + return -ENODEV;
>> + }
>> +
>> + /*
>> + * External bridges are mandatory for eDP interfaces: one has to
>> + * provide at least an eDP panel (which gets wrapped into panel-
>bridge).
>> + *
>> + * For DisplayPort interfaces external bridges are optional, so
>> + * silently ignore an error if one is not present (-ENODEV).
>> + */
>> + rc = dp_parser_find_next_bridge(dp_priv->parser);
>> + if (!dp->is_edp && rc == -ENODEV)
>> + return 0;
>> + else if (rc)
>
>Just an if because there's a return above.
>
Okay
>> + goto error;
>> +
>> + dp->next_bridge = dp_priv->parser->next_bridge;
>
>In which case it almost feels like it could be written
>
> if (!dp->is_edp && rc == -ENODEV)
> return 0;
> if (!rc) {
> dp->next_bridge = dp_priv->parser->next_bridge;
> return 0;
> }
>error:
> if (dp->is_edp) {
>
>but I'm not worried either way, besides removing the else from the else-if.
>
Okay
>> +
>> + return 0;
>> +
>> +error:
>> + if (dp->is_edp) {
>> + disable_irq(dp_priv->irq);
>> + dp_display_host_phy_exit(dp_priv);
>> + dp_display_host_deinit(dp_priv);
>> + }
>> + return rc;
>> +}
>> +
>> int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device
>*dev,
>> struct drm_encoder *encoder) { diff --git
>> a/drivers/gpu/drm/msm/dp/dp_parser.h
>> b/drivers/gpu/drm/msm/dp/dp_parser.h
>> index d371bae..950416c 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);
>> };
>>
>> /**
>> @@ -141,4 +141,15 @@ struct dp_parser {
>> */
>> struct dp_parser *dp_parser_get(struct platform_device *pdev);
>>
>> +/**
>> + * dp_parser_find_next_bridge() - find an additional bridge to DP
>> + *
>> + * @parser: dp_parser data from client
>> + * return: 0 if able to get the bridge else return an error code
>
>return comes after the description below. Also should be capitalized.
>You can check this by compiling with W=1 I believe, or run the kernel doc
>format script on the file..
>
Okay
>> + *
>> + * This function is used to find any additional bridge attached to
>> + * the DP controller. The eDP interface requires a panel bridge.
>
>Return: 0 if able to get the bridge, otherwise negative errno for failure
>
Okay
>> + */
>> +int dp_parser_find_next_bridge(struct dp_parser *parser);
>> +
^ permalink raw reply [flat|nested] 44+ messages in thread
* RE: [PATCH v9 1/4] drm/msm/dp: Add eDP support via aux_bus
2022-04-25 9:39 ` Sankeerth Billakanti (QUIC)
@ 2022-04-25 20:26 ` Stephen Boyd
-1 siblings, 0 replies; 44+ messages in thread
From: Stephen Boyd @ 2022-04-25 20:26 UTC (permalink / raw)
To: Sankeerth Billakanti, devicetree, dri-devel, freedreno,
linux-arm-msm, linux-kernel
Cc: quic_kalyant, dianders, quic_vproddut, airlied, Abhinav Kumar,
steev, Kuogee Hsieh, seanpaul, dmitry.baryshkov,
Aravind Venkateswaran, bjorn.andersson, sean
Quoting Sankeerth Billakanti (QUIC) (2022-04-25 02:39:43)
> Hi Stephen,
>
> >Quoting Sankeerth Billakanti (2022-04-22 02:11:03)
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> >> b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index d7a19d6..055681a 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >
> >Some nitpicks
> >
> >Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> >
> >> @@ -1508,7 +1509,8 @@ void msm_dp_irq_postinstall(struct msm_dp
> >> *dp_display)
> >>
> >> dp_hpd_event_setup(dp);
> >>
> >> - dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
> >> + if (!dp_display->is_edp)
> >> + dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
> >
> >Did it turn out that in fact DP isn't ready still to setup even after delaying the
> >irq?
> >
>
> The host_init, config_hpd, phy_init and enable_irq are happening in modeset_init already for eDP.
> So, I am not scheduling the EV_HPD_INIT_SETUP event for eDP. I am not modifying the delay for DP.
Cool. That didn't answer my question though. Why does DP still need the
delay? I thought recent changes made it unnecessary.
^ permalink raw reply [flat|nested] 44+ messages in thread
* RE: [PATCH v9 1/4] drm/msm/dp: Add eDP support via aux_bus
@ 2022-04-25 20:26 ` Stephen Boyd
0 siblings, 0 replies; 44+ messages in thread
From: Stephen Boyd @ 2022-04-25 20:26 UTC (permalink / raw)
To: Sankeerth Billakanti, devicetree, dri-devel, freedreno,
linux-arm-msm, linux-kernel
Cc: robdclark, seanpaul, quic_kalyant, Abhinav Kumar, dianders,
Kuogee Hsieh, bjorn.andersson, sean, airlied, daniel,
dmitry.baryshkov, quic_vproddut, Aravind Venkateswaran, steev
Quoting Sankeerth Billakanti (QUIC) (2022-04-25 02:39:43)
> Hi Stephen,
>
> >Quoting Sankeerth Billakanti (2022-04-22 02:11:03)
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> >> b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index d7a19d6..055681a 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >
> >Some nitpicks
> >
> >Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> >
> >> @@ -1508,7 +1509,8 @@ void msm_dp_irq_postinstall(struct msm_dp
> >> *dp_display)
> >>
> >> dp_hpd_event_setup(dp);
> >>
> >> - dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
> >> + if (!dp_display->is_edp)
> >> + dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
> >
> >Did it turn out that in fact DP isn't ready still to setup even after delaying the
> >irq?
> >
>
> The host_init, config_hpd, phy_init and enable_irq are happening in modeset_init already for eDP.
> So, I am not scheduling the EV_HPD_INIT_SETUP event for eDP. I am not modifying the delay for DP.
Cool. That didn't answer my question though. Why does DP still need the
delay? I thought recent changes made it unnecessary.
^ permalink raw reply [flat|nested] 44+ messages in thread
* RE: [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
2022-04-25 2:55 ` Sankeerth Billakanti (QUIC)
@ 2022-04-25 20:27 ` Stephen Boyd
-1 siblings, 0 replies; 44+ messages in thread
From: Stephen Boyd @ 2022-04-25 20:27 UTC (permalink / raw)
To: Sankeerth Billakanti, devicetree, dri-devel, freedreno,
linux-arm-msm, linux-kernel
Cc: quic_kalyant, dianders, quic_vproddut, airlied, Abhinav Kumar,
steev, Kuogee Hsieh, seanpaul, dmitry.baryshkov,
Aravind Venkateswaran, bjorn.andersson, sean
Quoting Sankeerth Billakanti (QUIC) (2022-04-24 19:55:29)
> >Quoting Sankeerth Billakanti (2022-04-22 02:11:04)
> >
> >> int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog) diff
> >> --git a/drivers/gpu/drm/msm/dp/dp_display.c
> >> b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index 055681a..dea4de9 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -1096,6 +1097,13 @@ static void dp_display_config_hpd(struct
> >dp_display_private *dp)
> >> dp_display_host_init(dp);
> >> dp_catalog_ctrl_hpd_config(dp->catalog);
> >>
> >> + /* Enable plug and unplug interrupts only for external DisplayPort */
> >> + if (!dp->dp_display.is_edp)
> >> + dp_catalog_hpd_config_intr(dp->catalog,
> >> + DP_DP_HPD_PLUG_INT_MASK |
> >> + DP_DP_HPD_UNPLUG_INT_MASK,
> >> + true);
> >> +
> >
> >It seems like only the plug and unplug is enabled for DP here. Is replug
> >enabled for eDP when it shouldn't be?
> >
>
> By default, all the interrupts are masked. This function is not executed for eDP
> anymore because the host_init, phy_init and enable_irq are all done from
> modeset_init for eDP with aux_bus. So, none of the eDP hpd interrupts are
> enabled or unmasked before pre-enable.
>
> The replug interrupt is unmasked for DP and eDP from the dp_hpd_plug_handle()
> and masked from dp_hpd_unplug_handle().
Why is replug enabled for eDP?
^ permalink raw reply [flat|nested] 44+ messages in thread
* RE: [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
@ 2022-04-25 20:27 ` Stephen Boyd
0 siblings, 0 replies; 44+ messages in thread
From: Stephen Boyd @ 2022-04-25 20:27 UTC (permalink / raw)
To: Sankeerth Billakanti, devicetree, dri-devel, freedreno,
linux-arm-msm, linux-kernel
Cc: robdclark, seanpaul, quic_kalyant, Abhinav Kumar, dianders,
Kuogee Hsieh, bjorn.andersson, sean, airlied, daniel,
dmitry.baryshkov, quic_vproddut, Aravind Venkateswaran, steev
Quoting Sankeerth Billakanti (QUIC) (2022-04-24 19:55:29)
> >Quoting Sankeerth Billakanti (2022-04-22 02:11:04)
> >
> >> int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog) diff
> >> --git a/drivers/gpu/drm/msm/dp/dp_display.c
> >> b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index 055681a..dea4de9 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -1096,6 +1097,13 @@ static void dp_display_config_hpd(struct
> >dp_display_private *dp)
> >> dp_display_host_init(dp);
> >> dp_catalog_ctrl_hpd_config(dp->catalog);
> >>
> >> + /* Enable plug and unplug interrupts only for external DisplayPort */
> >> + if (!dp->dp_display.is_edp)
> >> + dp_catalog_hpd_config_intr(dp->catalog,
> >> + DP_DP_HPD_PLUG_INT_MASK |
> >> + DP_DP_HPD_UNPLUG_INT_MASK,
> >> + true);
> >> +
> >
> >It seems like only the plug and unplug is enabled for DP here. Is replug
> >enabled for eDP when it shouldn't be?
> >
>
> By default, all the interrupts are masked. This function is not executed for eDP
> anymore because the host_init, phy_init and enable_irq are all done from
> modeset_init for eDP with aux_bus. So, none of the eDP hpd interrupts are
> enabled or unmasked before pre-enable.
>
> The replug interrupt is unmasked for DP and eDP from the dp_hpd_plug_handle()
> and masked from dp_hpd_unplug_handle().
Why is replug enabled for eDP?
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v9 1/4] drm/msm/dp: Add eDP support via aux_bus
2022-04-25 20:26 ` Stephen Boyd
@ 2022-04-25 21:12 ` Dmitry Baryshkov
-1 siblings, 0 replies; 44+ messages in thread
From: Dmitry Baryshkov @ 2022-04-25 21:12 UTC (permalink / raw)
To: Stephen Boyd, Sankeerth Billakanti, devicetree, dri-devel,
freedreno, linux-arm-msm, linux-kernel
Cc: robdclark, seanpaul, quic_kalyant, Abhinav Kumar, dianders,
Kuogee Hsieh, bjorn.andersson, sean, airlied, daniel,
quic_vproddut, Aravind Venkateswaran, steev
On 25/04/2022 23:26, Stephen Boyd wrote:
> Quoting Sankeerth Billakanti (QUIC) (2022-04-25 02:39:43)
>> Hi Stephen,
>>
>>> Quoting Sankeerth Billakanti (2022-04-22 02:11:03)
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> index d7a19d6..055681a 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>
>>> Some nitpicks
>>>
>>> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>>>
>>>> @@ -1508,7 +1509,8 @@ void msm_dp_irq_postinstall(struct msm_dp
>>>> *dp_display)
>>>>
>>>> dp_hpd_event_setup(dp);
>>>>
>>>> - dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
>>>> + if (!dp_display->is_edp)
>>>> + dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
>>>
>>> Did it turn out that in fact DP isn't ready still to setup even after delaying the
>>> irq?
>>>
>>
>> The host_init, config_hpd, phy_init and enable_irq are happening in modeset_init already for eDP.
>> So, I am not scheduling the EV_HPD_INIT_SETUP event for eDP. I am not modifying the delay for DP.
>
> Cool. That didn't answer my question though. Why does DP still need the
> delay? I thought recent changes made it unnecessary.
I'd say that if it is not necessary, it should be changed in the
separate commit. The question is valid nevertheless.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v9 1/4] drm/msm/dp: Add eDP support via aux_bus
@ 2022-04-25 21:12 ` Dmitry Baryshkov
0 siblings, 0 replies; 44+ messages in thread
From: Dmitry Baryshkov @ 2022-04-25 21:12 UTC (permalink / raw)
To: Stephen Boyd, Sankeerth Billakanti, devicetree, dri-devel,
freedreno, linux-arm-msm, linux-kernel
Cc: quic_kalyant, dianders, quic_vproddut, airlied, Abhinav Kumar,
steev, Kuogee Hsieh, seanpaul, Aravind Venkateswaran,
bjorn.andersson, sean
On 25/04/2022 23:26, Stephen Boyd wrote:
> Quoting Sankeerth Billakanti (QUIC) (2022-04-25 02:39:43)
>> Hi Stephen,
>>
>>> Quoting Sankeerth Billakanti (2022-04-22 02:11:03)
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> index d7a19d6..055681a 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>
>>> Some nitpicks
>>>
>>> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>>>
>>>> @@ -1508,7 +1509,8 @@ void msm_dp_irq_postinstall(struct msm_dp
>>>> *dp_display)
>>>>
>>>> dp_hpd_event_setup(dp);
>>>>
>>>> - dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
>>>> + if (!dp_display->is_edp)
>>>> + dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
>>>
>>> Did it turn out that in fact DP isn't ready still to setup even after delaying the
>>> irq?
>>>
>>
>> The host_init, config_hpd, phy_init and enable_irq are happening in modeset_init already for eDP.
>> So, I am not scheduling the EV_HPD_INIT_SETUP event for eDP. I am not modifying the delay for DP.
>
> Cool. That didn't answer my question though. Why does DP still need the
> delay? I thought recent changes made it unnecessary.
I'd say that if it is not necessary, it should be changed in the
separate commit. The question is valid nevertheless.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 44+ messages in thread
* RE: [PATCH v9 1/4] drm/msm/dp: Add eDP support via aux_bus
2022-04-25 21:12 ` Dmitry Baryshkov
@ 2022-04-26 1:58 ` Sankeerth Billakanti (QUIC)
-1 siblings, 0 replies; 44+ messages in thread
From: Sankeerth Billakanti (QUIC) @ 2022-04-26 1:58 UTC (permalink / raw)
To: dmitry.baryshkov, Stephen Boyd, Sankeerth Billakanti (QUIC),
devicetree, dri-devel, freedreno, linux-arm-msm, linux-kernel
Cc: robdclark, seanpaul, quic_kalyant, Abhinav Kumar (QUIC),
dianders, Kuogee Hsieh (QUIC),
bjorn.andersson, sean, airlied, daniel, quic_vproddut,
Aravind Venkateswaran (QUIC),
steev
Hi Stephen,
>>>> Quoting Sankeerth Billakanti (2022-04-22 02:11:03)
>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> index d7a19d6..055681a 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>
>>>> Some nitpicks
>>>>
>>>> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>>>>
>>>>> @@ -1508,7 +1509,8 @@ void msm_dp_irq_postinstall(struct msm_dp
>>>>> *dp_display)
>>>>>
>>>>> dp_hpd_event_setup(dp);
>>>>>
>>>>> - dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
>>>>> + if (!dp_display->is_edp)
>>>>> + dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
>>>>
>>>> Did it turn out that in fact DP isn't ready still to setup even
>>>> after delaying the irq?
>>>>
>>>
>>> The host_init, config_hpd, phy_init and enable_irq are happening in
>modeset_init already for eDP.
>>> So, I am not scheduling the EV_HPD_INIT_SETUP event for eDP. I am not
>modifying the delay for DP.
>>
>> Cool. That didn't answer my question though. Why does DP still need
>> the delay? I thought recent changes made it unnecessary.
>
>I'd say that if it is not necessary, it should be changed in the separate commit.
>The question is valid nevertheless.
>
Yes, that is right. The delay is unnecessary with the recent changes.
Like Dmitry rightly suggested, we will remove the delay in a separate commit.
>
>--
>With best wishes
>Dmitry
^ permalink raw reply [flat|nested] 44+ messages in thread
* RE: [PATCH v9 1/4] drm/msm/dp: Add eDP support via aux_bus
@ 2022-04-26 1:58 ` Sankeerth Billakanti (QUIC)
0 siblings, 0 replies; 44+ messages in thread
From: Sankeerth Billakanti (QUIC) @ 2022-04-26 1:58 UTC (permalink / raw)
To: dmitry.baryshkov, Stephen Boyd, Sankeerth Billakanti (QUIC),
devicetree, dri-devel, freedreno, linux-arm-msm, linux-kernel
Cc: quic_kalyant, dianders, quic_vproddut, airlied,
Abhinav Kumar (QUIC), steev, Kuogee Hsieh (QUIC),
seanpaul, Aravind Venkateswaran (QUIC),
bjorn.andersson, sean
Hi Stephen,
>>>> Quoting Sankeerth Billakanti (2022-04-22 02:11:03)
>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> index d7a19d6..055681a 100644
>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>
>>>> Some nitpicks
>>>>
>>>> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>>>>
>>>>> @@ -1508,7 +1509,8 @@ void msm_dp_irq_postinstall(struct msm_dp
>>>>> *dp_display)
>>>>>
>>>>> dp_hpd_event_setup(dp);
>>>>>
>>>>> - dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
>>>>> + if (!dp_display->is_edp)
>>>>> + dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
>>>>
>>>> Did it turn out that in fact DP isn't ready still to setup even
>>>> after delaying the irq?
>>>>
>>>
>>> The host_init, config_hpd, phy_init and enable_irq are happening in
>modeset_init already for eDP.
>>> So, I am not scheduling the EV_HPD_INIT_SETUP event for eDP. I am not
>modifying the delay for DP.
>>
>> Cool. That didn't answer my question though. Why does DP still need
>> the delay? I thought recent changes made it unnecessary.
>
>I'd say that if it is not necessary, it should be changed in the separate commit.
>The question is valid nevertheless.
>
Yes, that is right. The delay is unnecessary with the recent changes.
Like Dmitry rightly suggested, we will remove the delay in a separate commit.
>
>--
>With best wishes
>Dmitry
^ permalink raw reply [flat|nested] 44+ messages in thread
* RE: [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
2022-04-25 20:27 ` Stephen Boyd
@ 2022-04-26 3:45 ` Sankeerth Billakanti (QUIC)
-1 siblings, 0 replies; 44+ messages in thread
From: Sankeerth Billakanti (QUIC) @ 2022-04-26 3:45 UTC (permalink / raw)
To: Stephen Boyd, Sankeerth Billakanti (QUIC),
devicetree, dri-devel, freedreno, linux-arm-msm, linux-kernel
Cc: robdclark, seanpaul, quic_kalyant, Abhinav Kumar (QUIC),
dianders, Kuogee Hsieh (QUIC),
bjorn.andersson, sean, airlied, daniel, dmitry.baryshkov,
quic_vproddut, Aravind Venkateswaran (QUIC),
steev
Hi Stephen,
>> >> int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
>> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> >> b/drivers/gpu/drm/msm/dp/dp_display.c
>> >> index 055681a..dea4de9 100644
>> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> >> @@ -1096,6 +1097,13 @@ static void dp_display_config_hpd(struct
>> >dp_display_private *dp)
>> >> dp_display_host_init(dp);
>> >> dp_catalog_ctrl_hpd_config(dp->catalog);
>> >>
>> >> + /* Enable plug and unplug interrupts only for external DisplayPort */
>> >> + if (!dp->dp_display.is_edp)
>> >> + dp_catalog_hpd_config_intr(dp->catalog,
>> >> + DP_DP_HPD_PLUG_INT_MASK |
>> >> + DP_DP_HPD_UNPLUG_INT_MASK,
>> >> + true);
>> >> +
>> >
>> >It seems like only the plug and unplug is enabled for DP here. Is
>> >replug enabled for eDP when it shouldn't be?
>> >
>>
>> By default, all the interrupts are masked. This function is not
>> executed for eDP anymore because the host_init, phy_init and
>> enable_irq are all done from modeset_init for eDP with aux_bus. So,
>> none of the eDP hpd interrupts are enabled or unmasked before pre-
>enable.
>>
>> The replug interrupt is unmasked for DP and eDP from the
>> dp_hpd_plug_handle() and masked from dp_hpd_unplug_handle().
>
>Why is replug enabled for eDP?
As the eDP panel is assumed to be always connected, just enabling the IRQ_HPD is sufficient.
The REPLUG is enabled or unmasked along with IRQ_HPD in code.
I did not remove the REPLUG event support for eDP so that we have an option to see if the eDP panel
can undergo a short disconnect/connect cycle after pre-enable (while the panel power is enabled).
REPLUG can be generated for eDP if,
a) the panel power turns off and on OR
b) the sink itself issues a fast disconnect-connect.
REPLUG event initiated by sink is rare and we observed it only during the DP compliance test.
Some more information on HPD events generated by the source:
Replug interrupt is something our controller HW supports and not part of the DP/eDP specification.
The programmed values for HPD on the HW controller indicates the following:
1. The HOTPLUG interrupt will be generated if the HPD line is continuously high for 100ms.
2. Similarly, UNPLUG interrupt will be generated when the HPD line transitions from high to low and remains low for 100ms.
3. IRQ_HPD will be generated when the HPD line transitions from high to low and remains low for less than 2ms.
4. REPLUG will be generated if the HPD line remains low for more than 2ms but less than 100ms.
According to the DP spec, replug event should be considered as a disconnect and then connect.
To answer your question, I did not remove REPLUG support for eDP because I felt it will not affect the eDP normal functioning in anyway.
Thank you,
Sankeerth
^ permalink raw reply [flat|nested] 44+ messages in thread
* RE: [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
@ 2022-04-26 3:45 ` Sankeerth Billakanti (QUIC)
0 siblings, 0 replies; 44+ messages in thread
From: Sankeerth Billakanti (QUIC) @ 2022-04-26 3:45 UTC (permalink / raw)
To: Stephen Boyd, Sankeerth Billakanti (QUIC),
devicetree, dri-devel, freedreno, linux-arm-msm, linux-kernel
Cc: quic_kalyant, dianders, quic_vproddut, airlied,
Abhinav Kumar (QUIC), steev, Kuogee Hsieh (QUIC),
seanpaul, dmitry.baryshkov, Aravind Venkateswaran (QUIC),
bjorn.andersson, sean
Hi Stephen,
>> >> int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
>> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> >> b/drivers/gpu/drm/msm/dp/dp_display.c
>> >> index 055681a..dea4de9 100644
>> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> >> @@ -1096,6 +1097,13 @@ static void dp_display_config_hpd(struct
>> >dp_display_private *dp)
>> >> dp_display_host_init(dp);
>> >> dp_catalog_ctrl_hpd_config(dp->catalog);
>> >>
>> >> + /* Enable plug and unplug interrupts only for external DisplayPort */
>> >> + if (!dp->dp_display.is_edp)
>> >> + dp_catalog_hpd_config_intr(dp->catalog,
>> >> + DP_DP_HPD_PLUG_INT_MASK |
>> >> + DP_DP_HPD_UNPLUG_INT_MASK,
>> >> + true);
>> >> +
>> >
>> >It seems like only the plug and unplug is enabled for DP here. Is
>> >replug enabled for eDP when it shouldn't be?
>> >
>>
>> By default, all the interrupts are masked. This function is not
>> executed for eDP anymore because the host_init, phy_init and
>> enable_irq are all done from modeset_init for eDP with aux_bus. So,
>> none of the eDP hpd interrupts are enabled or unmasked before pre-
>enable.
>>
>> The replug interrupt is unmasked for DP and eDP from the
>> dp_hpd_plug_handle() and masked from dp_hpd_unplug_handle().
>
>Why is replug enabled for eDP?
As the eDP panel is assumed to be always connected, just enabling the IRQ_HPD is sufficient.
The REPLUG is enabled or unmasked along with IRQ_HPD in code.
I did not remove the REPLUG event support for eDP so that we have an option to see if the eDP panel
can undergo a short disconnect/connect cycle after pre-enable (while the panel power is enabled).
REPLUG can be generated for eDP if,
a) the panel power turns off and on OR
b) the sink itself issues a fast disconnect-connect.
REPLUG event initiated by sink is rare and we observed it only during the DP compliance test.
Some more information on HPD events generated by the source:
Replug interrupt is something our controller HW supports and not part of the DP/eDP specification.
The programmed values for HPD on the HW controller indicates the following:
1. The HOTPLUG interrupt will be generated if the HPD line is continuously high for 100ms.
2. Similarly, UNPLUG interrupt will be generated when the HPD line transitions from high to low and remains low for 100ms.
3. IRQ_HPD will be generated when the HPD line transitions from high to low and remains low for less than 2ms.
4. REPLUG will be generated if the HPD line remains low for more than 2ms but less than 100ms.
According to the DP spec, replug event should be considered as a disconnect and then connect.
To answer your question, I did not remove REPLUG support for eDP because I felt it will not affect the eDP normal functioning in anyway.
Thank you,
Sankeerth
^ permalink raw reply [flat|nested] 44+ messages in thread
end of thread, other threads:[~2022-04-26 3:46 UTC | newest]
Thread overview: 44+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-22 9:11 [PATCH v9 0/4] Add support for the eDP panel over aux_bus Sankeerth Billakanti
2022-04-22 9:11 ` Sankeerth Billakanti
2022-04-22 9:11 ` [PATCH v9 1/4] drm/msm/dp: Add eDP support via aux_bus Sankeerth Billakanti
2022-04-22 9:11 ` Sankeerth Billakanti
2022-04-22 14:03 ` Doug Anderson
2022-04-22 14:03 ` Doug Anderson
2022-04-23 0:13 ` Stephen Boyd
2022-04-23 0:13 ` Stephen Boyd
2022-04-25 9:39 ` Sankeerth Billakanti (QUIC)
2022-04-25 9:39 ` Sankeerth Billakanti (QUIC)
2022-04-25 20:26 ` Stephen Boyd
2022-04-25 20:26 ` Stephen Boyd
2022-04-25 21:12 ` Dmitry Baryshkov
2022-04-25 21:12 ` Dmitry Baryshkov
2022-04-26 1:58 ` Sankeerth Billakanti (QUIC)
2022-04-26 1:58 ` Sankeerth Billakanti (QUIC)
2022-04-22 9:11 ` [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP Sankeerth Billakanti
2022-04-22 9:11 ` Sankeerth Billakanti
2022-04-22 14:03 ` Doug Anderson
2022-04-22 14:03 ` Doug Anderson
2022-04-22 15:55 ` Doug Anderson
2022-04-22 15:55 ` Doug Anderson
2022-04-22 16:04 ` Abhinav Kumar
2022-04-22 16:04 ` Abhinav Kumar
2022-04-22 16:10 ` Doug Anderson
2022-04-22 16:10 ` Doug Anderson
2022-04-22 17:35 ` Abhinav Kumar
2022-04-22 17:35 ` Abhinav Kumar
2022-04-23 0:29 ` Stephen Boyd
2022-04-23 0:29 ` Stephen Boyd
2022-04-25 2:55 ` Sankeerth Billakanti (QUIC)
2022-04-25 2:55 ` Sankeerth Billakanti (QUIC)
2022-04-25 20:27 ` Stephen Boyd
2022-04-25 20:27 ` Stephen Boyd
2022-04-26 3:45 ` Sankeerth Billakanti (QUIC)
2022-04-26 3:45 ` Sankeerth Billakanti (QUIC)
2022-04-22 9:11 ` [PATCH v9 3/4] drm/msm/dp: wait for hpd high before aux transaction Sankeerth Billakanti
2022-04-22 9:11 ` Sankeerth Billakanti
2022-04-23 1:38 ` Stephen Boyd
2022-04-23 1:38 ` Stephen Boyd
2022-04-22 9:11 ` [PATCH v9 4/4] drm/msm/dp: Support the eDP modes given by panel Sankeerth Billakanti
2022-04-22 9:11 ` Sankeerth Billakanti
2022-04-23 1:39 ` Stephen Boyd
2022-04-23 1:39 ` Stephen Boyd
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.