[PATCH v7 0/4] Add support for the eDP panel over aux_bus

[PATCH v7 0/4] Add support for the eDP panel over aux_bus

[Sankeerth Billakanti]

This series adds support for generic eDP panel over aux_bus.

These changes are dependent on the following series:
https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654&state=*

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
  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 |  22 +++++---
 drivers/gpu/drm/msm/dp/dp_catalog.h |   1 +
 drivers/gpu/drm/msm/dp/dp_display.c | 100 +++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/msm/dp/dp_display.h |   1 +
 drivers/gpu/drm/msm/dp/dp_drm.c     |  10 ++--
 drivers/gpu/drm/msm/dp/dp_parser.c  |  23 +--------
 drivers/gpu/drm/msm/dp/dp_parser.h  |  13 ++++-
 9 files changed, 154 insertions(+), 40 deletions(-)

-- 
2.7.4

[PATCH v7 0/4] Add support for the eDP panel over aux_bus

[Sankeerth Billakanti]

This series adds support for generic eDP panel over aux_bus.

These changes are dependent on the following series:
https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654&state=*

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
  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 |  22 +++++---
 drivers/gpu/drm/msm/dp/dp_catalog.h |   1 +
 drivers/gpu/drm/msm/dp/dp_display.c | 100 +++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/msm/dp/dp_display.h |   1 +
 drivers/gpu/drm/msm/dp/dp_drm.c     |  10 ++--
 drivers/gpu/drm/msm/dp/dp_parser.c  |  23 +--------
 drivers/gpu/drm/msm/dp/dp_parser.h  |  13 ++++-
 9 files changed, 154 insertions(+), 40 deletions(-)

-- 
2.7.4

[PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus

[Sankeerth Billakanti]

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 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 | 68 ++++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/msm/dp/dp_display.h |  1 +
 drivers/gpu/drm/msm/dp/dp_drm.c     | 10 +++---
 drivers/gpu/drm/msm/dp/dp_parser.c  | 23 ++-----------
 drivers/gpu/drm/msm/dp/dp_parser.h  | 13 ++++++-
 5 files changed, 85 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..43c59cb 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,60 @@ 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);
+
+		rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
+		of_node_put(aux_bus);
+		if (rc) {
+			disable_irq(dp_priv->irq);
+			dp_display_host_phy_exit(dp_priv);
+			dp_display_host_deinit(dp_priv);
+			return rc;
+		}
+	} 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 (rc == -ENODEV) {
+		if (dp->is_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;
+	}
+
+	dp->next_bridge = dp_priv->parser->next_bridge;
+
+	return 0;
+}
+
 int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
 			struct drm_encoder *encoder)
 {
@@ -1553,6 +1609,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..5f0293f 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -114,10 +114,12 @@ 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;
+	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

[PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus

[Sankeerth Billakanti]

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 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 | 68 ++++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/msm/dp/dp_display.h |  1 +
 drivers/gpu/drm/msm/dp/dp_drm.c     | 10 +++---
 drivers/gpu/drm/msm/dp/dp_parser.c  | 23 ++-----------
 drivers/gpu/drm/msm/dp/dp_parser.h  | 13 ++++++-
 5 files changed, 85 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..43c59cb 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,60 @@ 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);
+
+		rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
+		of_node_put(aux_bus);
+		if (rc) {
+			disable_irq(dp_priv->irq);
+			dp_display_host_phy_exit(dp_priv);
+			dp_display_host_deinit(dp_priv);
+			return rc;
+		}
+	} 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 (rc == -ENODEV) {
+		if (dp->is_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;
+	}
+
+	dp->next_bridge = dp_priv->parser->next_bridge;
+
+	return 0;
+}
+
 int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
 			struct drm_encoder *encoder)
 {
@@ -1553,6 +1609,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..5f0293f 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -114,10 +114,12 @@ 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;
+	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

[PATCH v7 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP

[Sankeerth Billakanti]

The panel-edp enables the eDP panel power during probe, get_modes
and 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 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 |  9 +++------
 drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++-
 2 files changed, 24 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..07f2389 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,14 @@ 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;
+	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 43c59cb..1339c45 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
@@ -1654,6 +1668,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 */
@@ -1718,6 +1735,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

[PATCH v7 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP

[Sankeerth Billakanti]

The panel-edp enables the eDP panel power during probe, get_modes
and 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 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 |  9 +++------
 drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++-
 2 files changed, 24 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..07f2389 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,14 @@ 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;
+	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 43c59cb..1339c45 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
@@ -1654,6 +1668,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 */
@@ -1718,6 +1735,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

[PATCH v7 3/4] drm/msm/dp: wait for hpd high before aux transaction

[Sankeerth Billakanti]

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>
---

Changes in v7:
  - add a comment to say why the wait si 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..cf0739f 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -36,6 +36,7 @@ struct dp_aux_private {
 	bool initted;
 	u32 offset;
 	u32 segment;
+	bool is_edp;
 
 	struct drm_dp_aux dp_aux;
 };
@@ -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..c99aeec 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 07f2389..4282a98 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 1339c45..c7277f0 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

[PATCH v7 3/4] drm/msm/dp: wait for hpd high before aux transaction

[Sankeerth Billakanti]

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>
---

Changes in v7:
  - add a comment to say why the wait si 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..cf0739f 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -36,6 +36,7 @@ struct dp_aux_private {
 	bool initted;
 	u32 offset;
 	u32 segment;
+	bool is_edp;
 
 	struct drm_dp_aux dp_aux;
 };
@@ -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..c99aeec 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 07f2389..4282a98 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 1339c45..c7277f0 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

[PATCH v7 4/4] Support the eDP modes given by panel

[Sankeerth Billakanti]

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>
---
 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 c7277f0..0f18a16 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

[PATCH v7 4/4] Support the eDP modes given by panel

[Sankeerth Billakanti]

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>
---
 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 c7277f0..0f18a16 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

Re: [PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus

[Doug Anderson]

Hi,

On Thu, Apr 14, 2022 at 5:20 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> @@ -1530,6 +1532,60 @@ 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);
> +
> +               rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
> +               of_node_put(aux_bus);
> +               if (rc) {
> +                       disable_irq(dp_priv->irq);
> +                       dp_display_host_phy_exit(dp_priv);
> +                       dp_display_host_deinit(dp_priv);
> +                       return rc;
> +               }
> +       } 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);

This gets into the same problem that Dmitry pointed out that ps8640
has that's addressed by my recent series [1].  Namely it's not
guaranteed that the panel will have finished probing by the time
devm_of_dp_aux_populate_ep_devices() finishes probing. I don't think
it's going to be really solvable without the bigger rewrite that we've
been discussing, though. ...it's probably OK to land something like
what you have here, but it might at least deserve a comment in the
code?

[1] https://lore.kernel.org/r/20220409023628.2104952-1-dianders@chromium.org


> +       if (rc == -ENODEV) {
> +               if (dp->is_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;

In both of your two error returns here isn't it a problem that you don't do:

  disable_irq(dp_priv->irq);
  dp_display_host_phy_exit(dp_priv);
  dp_display_host_deinit(dp_priv);

Should probably at least fix that clear error before landing, unless
I'm misunderstanding and there's some reason not to do that?


As discussed previously, I'm not convinced that we've covered every
corner case for properly doing and undoing the above things. I'm
hoping that once we do the cleanup and move to pm_runtime() management
that it will be cleaned up?


> @@ -114,10 +114,12 @@ 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;
> +       if (!dp_display->is_edp) {
> +               bridge->ops =
> +                       DRM_BRIDGE_OP_DETECT |
> +                       DRM_BRIDGE_OP_HPD |
> +                       DRM_BRIDGE_OP_MODES;

Given that Dmitry had questions about why eDP has different ops in his
previous review of this code, the above probably deserves an inline
code comment. If you want to use my wording, you could paste this into
your code:

  /*
   * 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.
   */


Overall: as discussed, I think that the current implementation is a
bit fragile and might have some wrong corner cases since it's hard for
me to reason about exactly when we init/de-init things. Even if it
works great, the fact that it's hard to reason about isn't wonderful.
That being said, I honestly believe that would benefit upstream to get
this landed and iterate on it. I don't think this should be causing
any existing behavior to be _worse_ and getting it landed upstream
will keep more people focused on the same codebase.


-Doug

Re: [PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus

[Doug Anderson]

Hi,

On Thu, Apr 14, 2022 at 5:20 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> @@ -1530,6 +1532,60 @@ 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);
> +
> +               rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
> +               of_node_put(aux_bus);
> +               if (rc) {
> +                       disable_irq(dp_priv->irq);
> +                       dp_display_host_phy_exit(dp_priv);
> +                       dp_display_host_deinit(dp_priv);
> +                       return rc;
> +               }
> +       } 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);

This gets into the same problem that Dmitry pointed out that ps8640
has that's addressed by my recent series [1].  Namely it's not
guaranteed that the panel will have finished probing by the time
devm_of_dp_aux_populate_ep_devices() finishes probing. I don't think
it's going to be really solvable without the bigger rewrite that we've
been discussing, though. ...it's probably OK to land something like
what you have here, but it might at least deserve a comment in the
code?

[1] https://lore.kernel.org/r/20220409023628.2104952-1-dianders@chromium.org


> +       if (rc == -ENODEV) {
> +               if (dp->is_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;

In both of your two error returns here isn't it a problem that you don't do:

  disable_irq(dp_priv->irq);
  dp_display_host_phy_exit(dp_priv);
  dp_display_host_deinit(dp_priv);

Should probably at least fix that clear error before landing, unless
I'm misunderstanding and there's some reason not to do that?


As discussed previously, I'm not convinced that we've covered every
corner case for properly doing and undoing the above things. I'm
hoping that once we do the cleanup and move to pm_runtime() management
that it will be cleaned up?


> @@ -114,10 +114,12 @@ 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;
> +       if (!dp_display->is_edp) {
> +               bridge->ops =
> +                       DRM_BRIDGE_OP_DETECT |
> +                       DRM_BRIDGE_OP_HPD |
> +                       DRM_BRIDGE_OP_MODES;

Given that Dmitry had questions about why eDP has different ops in his
previous review of this code, the above probably deserves an inline
code comment. If you want to use my wording, you could paste this into
your code:

  /*
   * 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.
   */


Overall: as discussed, I think that the current implementation is a
bit fragile and might have some wrong corner cases since it's hard for
me to reason about exactly when we init/de-init things. Even if it
works great, the fact that it's hard to reason about isn't wonderful.
That being said, I honestly believe that would benefit upstream to get
this landed and iterate on it. I don't think this should be causing
any existing behavior to be _worse_ and getting it landed upstream
will keep more people focused on the same codebase.


-Doug

Re: [PATCH v7 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP

[Doug Anderson]

Hi,

On Thu, Apr 14, 2022 at 5:20 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> The panel-edp enables the eDP panel power during probe, get_modes
> and enable.

Technically the panel-edp powers on the panel in 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 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 |  9 +++------
>  drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++-
>  2 files changed, 24 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..07f2389 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,14 @@ 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;
> +       return isr & (mask | ~DP_DP_HPD_INT_MASK);

Please add a comment above this explaining what the goal of the above
statement is. I guess it's something like this, though you might want
to modify it to remove snark and insert the real reason unless you
like being snarky:

  /*
   * Report the raw status of all interrupts (AKA we still report the
   * interrupt as asserted even if it's masked) _except_ for HPD-related.
   * interrupts. We only report HPD-related interrupts if they're
   * unmasked. We do it this way because we thought it would be extra
   * confusing for readers of this code and we were bribed by Mordac to
   * confuse you.  OK, maybe that's not true. We actually do it this way
   * because of <insert your compelling reason here>.
   */

Along the same lines as my comments in patch #1, I don't have a great
feel for exactly when the various HPD bits are enabled / disabled and
it feels like it need to be made super obvious / well documented. That
being said, I'd be OK w/ that happening in the proposed cleanup.


-Doug

Re: [PATCH v7 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP

[Doug Anderson]

Hi,

On Thu, Apr 14, 2022 at 5:20 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> The panel-edp enables the eDP panel power during probe, get_modes
> and enable.

Technically the panel-edp powers on the panel in 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 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 |  9 +++------
>  drivers/gpu/drm/msm/dp/dp_display.c | 22 +++++++++++++++++++++-
>  2 files changed, 24 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..07f2389 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,14 @@ 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;
> +       return isr & (mask | ~DP_DP_HPD_INT_MASK);

Please add a comment above this explaining what the goal of the above
statement is. I guess it's something like this, though you might want
to modify it to remove snark and insert the real reason unless you
like being snarky:

  /*
   * Report the raw status of all interrupts (AKA we still report the
   * interrupt as asserted even if it's masked) _except_ for HPD-related.
   * interrupts. We only report HPD-related interrupts if they're
   * unmasked. We do it this way because we thought it would be extra
   * confusing for readers of this code and we were bribed by Mordac to
   * confuse you.  OK, maybe that's not true. We actually do it this way
   * because of <insert your compelling reason here>.
   */

Along the same lines as my comments in patch #1, I don't have a great
feel for exactly when the various HPD bits are enabled / disabled and
it feels like it need to be made super obvious / well documented. That
being said, I'd be OK w/ that happening in the proposed cleanup.


-Doug

Re: [PATCH v7 3/4] drm/msm/dp: wait for hpd high before aux transaction

[Doug Anderson]

Hi,

On Thu, Apr 14, 2022 at 5:20 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> 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>
> ---

It might be worth mentioning "after the cut" that we may eventually
end up changing the rules if people like my proposal [1]. However,
what your code is doing here for eDP is correct as things are
currently intended to work and it would make sense to land it while we
debate about whether we want to add the is_hpd_asserted() callback
like my patch does.

[1] https://lore.kernel.org/r/20220408193536.RFC.3.Icf57bb12233a47727013c6ab69eebf803e22ebc1@changeid/


> Changes in v7:
>   - add a comment to say why the wait si 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..cf0739f 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -36,6 +36,7 @@ struct dp_aux_private {
>         bool initted;
>         u32 offset;
>         u32 segment;
> +       bool is_edp;

Kinda nitty, but can you put it next to the other booleans? This will
help with structure packing.


>         struct drm_dp_aux dp_aux;
>  };
> @@ -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)

nit: I think indentation rules for this file are that the type of the
argument for the 2nd line should line up right under the 1st. Thus you
should delete one tab character and insert 6 spaces before the "bool".

Similar in other places, like your header file.


Stuff above is all nits and this looks right to me. I'm happy with:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v7 3/4] drm/msm/dp: wait for hpd high before aux transaction

[Doug Anderson]

Hi,

On Thu, Apr 14, 2022 at 5:20 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> 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>
> ---

It might be worth mentioning "after the cut" that we may eventually
end up changing the rules if people like my proposal [1]. However,
what your code is doing here for eDP is correct as things are
currently intended to work and it would make sense to land it while we
debate about whether we want to add the is_hpd_asserted() callback
like my patch does.

[1] https://lore.kernel.org/r/20220408193536.RFC.3.Icf57bb12233a47727013c6ab69eebf803e22ebc1@changeid/


> Changes in v7:
>   - add a comment to say why the wait si 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..cf0739f 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -36,6 +36,7 @@ struct dp_aux_private {
>         bool initted;
>         u32 offset;
>         u32 segment;
> +       bool is_edp;

Kinda nitty, but can you put it next to the other booleans? This will
help with structure packing.


>         struct drm_dp_aux dp_aux;
>  };
> @@ -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)

nit: I think indentation rules for this file are that the type of the
argument for the 2nd line should line up right under the 1st. Thus you
should delete one tab character and insert 6 spaces before the "bool".

Similar in other places, like your header file.


Stuff above is all nits and this looks right to me. I'm happy with:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v7 4/4] Support the eDP modes given by panel

[Doug Anderson]

Hi,

On Thu, Apr 14, 2022 at 5:20 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> 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>
> ---
>  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 c7277f0..0f18a16 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;

As discussed out-of-band, I agree that this is the right thing for now
and making this assumption won't break anything. In general the set of
eDP panels is known ahead of time it's fairly unlikely someone would
set things up so that a panel couldn't use the mode it was reporting.

Longer term we should figure out a way to solve this but it doesn't
have to be today. To properly implement mode_valid() we've got to
combine knowledge from the panel (mostly rates supported and number of
lanes supported) with the controller (rates supported, number of lanes
supported/hooked up on this board).

In any case:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v7 4/4] Support the eDP modes given by panel

[Doug Anderson]

Hi,

On Thu, Apr 14, 2022 at 5:20 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> 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>
> ---
>  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 c7277f0..0f18a16 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;

As discussed out-of-band, I agree that this is the right thing for now
and making this assumption won't break anything. In general the set of
eDP panels is known ahead of time it's fairly unlikely someone would
set things up so that a panel couldn't use the mode it was reporting.

Longer term we should figure out a way to solve this but it doesn't
have to be today. To properly implement mode_valid() we've got to
combine knowledge from the panel (mostly rates supported and number of
lanes supported) with the controller (rates supported, number of lanes
supported/hooked up on this board).

In any case:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v7 0/4] Add support for the eDP panel over aux_bus

[Doug Anderson]

Hi,

On Thu, Apr 14, 2022 at 5:19 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> This series adds support for generic eDP panel over aux_bus.
>
> These changes are dependent on the following series:
> https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654&state=*

You're basically depending on the last two patches of that series.
What's the plan there? In patchwork they're marked as "Not
Applicable". If they're good to go, maybe we should land them? If not,
maybe you should include them (with Dmitry as the author, of course)
at the beginning of your series?


> 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
>   Support the eDP modes given by panel

One of these things is not like the others. One of these things just
doesn't belong. Can you spot which patch is missing the prefix by
looking at the subject line of all 4 patches? ;-)

Re: [PATCH v7 0/4] Add support for the eDP panel over aux_bus

[Doug Anderson]

Hi,

On Thu, Apr 14, 2022 at 5:19 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> This series adds support for generic eDP panel over aux_bus.
>
> These changes are dependent on the following series:
> https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654&state=*

You're basically depending on the last two patches of that series.
What's the plan there? In patchwork they're marked as "Not
Applicable". If they're good to go, maybe we should land them? If not,
maybe you should include them (with Dmitry as the author, of course)
at the beginning of your series?


> 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
>   Support the eDP modes given by panel

One of these things is not like the others. One of these things just
doesn't belong. Can you spot which patch is missing the prefix by
looking at the subject line of all 4 patches? ;-)

Re: [PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus

[Dmitry Baryshkov]

On 14/04/2022 19:39, Doug Anderson wrote:
> Hi,
> 
> On Thu, Apr 14, 2022 at 5:20 AM Sankeerth Billakanti
> <quic_sbillaka@quicinc.com> wrote:
>>
>> @@ -1530,6 +1532,60 @@ 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);
>> +
>> +               rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
>> +               of_node_put(aux_bus);
>> +               if (rc) {
>> +                       disable_irq(dp_priv->irq);
>> +                       dp_display_host_phy_exit(dp_priv);
>> +                       dp_display_host_deinit(dp_priv);
>> +                       return rc;
>> +               }
>> +       } 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);
> 
> This gets into the same problem that Dmitry pointed out that ps8640
> has that's addressed by my recent series [1].  Namely it's not
> guaranteed that the panel will have finished probing by the time
> devm_of_dp_aux_populate_ep_devices() finishes probing. I don't think
> it's going to be really solvable without the bigger rewrite that we've
> been discussing, though. ...it's probably OK to land something like
> what you have here, but it might at least deserve a comment in the
> code?
> 
> [1] https://lore.kernel.org/r/20220409023628.2104952-1-dianders@chromium.org

We agreed that rework would follow up in a timely manner if these 
patches are applied. However a comment would be still a good thing.

> 
> 
>> +       if (rc == -ENODEV) {
>> +               if (dp->is_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;
> 
> In both of your two error returns here isn't it a problem that you don't do:
> 
>    disable_irq(dp_priv->irq);
>    dp_display_host_phy_exit(dp_priv);
>    dp_display_host_deinit(dp_priv);
> 
> Should probably at least fix that clear error before landing, unless
> I'm misunderstanding and there's some reason not to do that?
> 
> 
> As discussed previously, I'm not convinced that we've covered every
> corner case for properly doing and undoing the above things. I'm
> hoping that once we do the cleanup and move to pm_runtime() management
> that it will be cleaned up?
> 
> 
>> @@ -114,10 +114,12 @@ 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;
>> +       if (!dp_display->is_edp) {
>> +               bridge->ops =
>> +                       DRM_BRIDGE_OP_DETECT |
>> +                       DRM_BRIDGE_OP_HPD |
>> +                       DRM_BRIDGE_OP_MODES;
> 
> Given that Dmitry had questions about why eDP has different ops in his
> previous review of this code, the above probably deserves an inline
> code comment. If you want to use my wording, you could paste this into
> your code:
> 
>    /*
>     * 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.
>     */

I think it's too verbose and a bit incorrect.
This is a bit saner:
/*
  * These ops do not make sense for eDP, since they are provided
  * by the panel-bridge corresponding to the attached eDP panel.
  */

My question was whether we really need to disable them for eDP since for 
eDP the detect and and get_modes will be overridden anyway.

> Overall: as discussed, I think that the current implementation is a
> bit fragile and might have some wrong corner cases since it's hard for
> me to reason about exactly when we init/de-init things. Even if it
> works great, the fact that it's hard to reason about isn't wonderful.
> That being said, I honestly believe that would benefit upstream to get
> this landed and iterate on it. I don't think this should be causing
> any existing behavior to be _worse_ and getting it landed upstream
> will keep more people focused on the same codebase.
> 
> 
> -Doug


-- 
With best wishes
Dmitry

Re: [PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus

[Dmitry Baryshkov]

On 14/04/2022 19:39, Doug Anderson wrote:
> Hi,
> 
> On Thu, Apr 14, 2022 at 5:20 AM Sankeerth Billakanti
> <quic_sbillaka@quicinc.com> wrote:
>>
>> @@ -1530,6 +1532,60 @@ 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);
>> +
>> +               rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
>> +               of_node_put(aux_bus);
>> +               if (rc) {
>> +                       disable_irq(dp_priv->irq);
>> +                       dp_display_host_phy_exit(dp_priv);
>> +                       dp_display_host_deinit(dp_priv);
>> +                       return rc;
>> +               }
>> +       } 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);
> 
> This gets into the same problem that Dmitry pointed out that ps8640
> has that's addressed by my recent series [1].  Namely it's not
> guaranteed that the panel will have finished probing by the time
> devm_of_dp_aux_populate_ep_devices() finishes probing. I don't think
> it's going to be really solvable without the bigger rewrite that we've
> been discussing, though. ...it's probably OK to land something like
> what you have here, but it might at least deserve a comment in the
> code?
> 
> [1] https://lore.kernel.org/r/20220409023628.2104952-1-dianders@chromium.org

We agreed that rework would follow up in a timely manner if these 
patches are applied. However a comment would be still a good thing.

> 
> 
>> +       if (rc == -ENODEV) {
>> +               if (dp->is_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;
> 
> In both of your two error returns here isn't it a problem that you don't do:
> 
>    disable_irq(dp_priv->irq);
>    dp_display_host_phy_exit(dp_priv);
>    dp_display_host_deinit(dp_priv);
> 
> Should probably at least fix that clear error before landing, unless
> I'm misunderstanding and there's some reason not to do that?
> 
> 
> As discussed previously, I'm not convinced that we've covered every
> corner case for properly doing and undoing the above things. I'm
> hoping that once we do the cleanup and move to pm_runtime() management
> that it will be cleaned up?
> 
> 
>> @@ -114,10 +114,12 @@ 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;
>> +       if (!dp_display->is_edp) {
>> +               bridge->ops =
>> +                       DRM_BRIDGE_OP_DETECT |
>> +                       DRM_BRIDGE_OP_HPD |
>> +                       DRM_BRIDGE_OP_MODES;
> 
> Given that Dmitry had questions about why eDP has different ops in his
> previous review of this code, the above probably deserves an inline
> code comment. If you want to use my wording, you could paste this into
> your code:
> 
>    /*
>     * 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.
>     */

I think it's too verbose and a bit incorrect.
This is a bit saner:
/*
  * These ops do not make sense for eDP, since they are provided
  * by the panel-bridge corresponding to the attached eDP panel.
  */

My question was whether we really need to disable them for eDP since for 
eDP the detect and and get_modes will be overridden anyway.

> Overall: as discussed, I think that the current implementation is a
> bit fragile and might have some wrong corner cases since it's hard for
> me to reason about exactly when we init/de-init things. Even if it
> works great, the fact that it's hard to reason about isn't wonderful.
> That being said, I honestly believe that would benefit upstream to get
> this landed and iterate on it. I don't think this should be causing
> any existing behavior to be _worse_ and getting it landed upstream
> will keep more people focused on the same codebase.
> 
> 
> -Doug


-- 
With best wishes
Dmitry

Re: [PATCH v7 0/4] Add support for the eDP panel over aux_bus

[Dmitry Baryshkov]

On 14/04/2022 19:40, Doug Anderson wrote:
> Hi,
> 
> On Thu, Apr 14, 2022 at 5:19 AM Sankeerth Billakanti
> <quic_sbillaka@quicinc.com> wrote:
>>
>> This series adds support for generic eDP panel over aux_bus.
>>
>> These changes are dependent on the following series:
>> https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654&state=*
> 
> You're basically depending on the last two patches of that series.
> What's the plan there? In patchwork they're marked as "Not
> Applicable". If they're good to go, maybe we should land them? If not,
> maybe you should include them (with Dmitry as the author, of course)
> at the beginning of your series?

No, please do not resend patches. The patches in question are marked as 
'Not applicable' as they are really not applicable to Bjorn's tree.
It would be better to point to the correct patchwork:

https://patchwork.freedesktop.org/series/98585/

Note those patches still lack the R-B tag. I can include them anyway, 
basing on Sankeerth's Tested-by tag, but the formal R-B would also be good.


> 
> 
>> 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
>>    Support the eDP modes given by panel
> 
> One of these things is not like the others. One of these things just
> doesn't belong. Can you spot which patch is missing the prefix by
> looking at the subject line of all 4 patches? ;-)

:-)


-- 
With best wishes
Dmitry

Re: [PATCH v7 0/4] Add support for the eDP panel over aux_bus

[Dmitry Baryshkov]

On 14/04/2022 19:40, Doug Anderson wrote:
> Hi,
> 
> On Thu, Apr 14, 2022 at 5:19 AM Sankeerth Billakanti
> <quic_sbillaka@quicinc.com> wrote:
>>
>> This series adds support for generic eDP panel over aux_bus.
>>
>> These changes are dependent on the following series:
>> https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654&state=*
> 
> You're basically depending on the last two patches of that series.
> What's the plan there? In patchwork they're marked as "Not
> Applicable". If they're good to go, maybe we should land them? If not,
> maybe you should include them (with Dmitry as the author, of course)
> at the beginning of your series?

No, please do not resend patches. The patches in question are marked as 
'Not applicable' as they are really not applicable to Bjorn's tree.
It would be better to point to the correct patchwork:

https://patchwork.freedesktop.org/series/98585/

Note those patches still lack the R-B tag. I can include them anyway, 
basing on Sankeerth's Tested-by tag, but the formal R-B would also be good.


> 
> 
>> 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
>>    Support the eDP modes given by panel
> 
> One of these things is not like the others. One of these things just
> doesn't belong. Can you spot which patch is missing the prefix by
> looking at the subject line of all 4 patches? ;-)

:-)


-- 
With best wishes
Dmitry

Re: [PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus

[Stephen Boyd]

Quoting Dmitry Baryshkov (2022-04-14 12:16:14)
>
> I think it's too verbose and a bit incorrect.
> This is a bit saner:
> /*
>   * These ops do not make sense for eDP, since they are provided
>   * by the panel-bridge corresponding to the attached eDP panel.
>   */
>
> My question was whether we really need to disable them for eDP since for
> eDP the detect and and get_modes will be overridden anyway.

And to go further, I'd expect that a bridge should expose the
functionality that it supports, regardless of what is connected down the
chain. Otherwise we won't be able to mix and match bridges because the
code is brittle, making assumptions about what is connected.

Re: [PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus

[Stephen Boyd]

Quoting Dmitry Baryshkov (2022-04-14 12:16:14)
>
> I think it's too verbose and a bit incorrect.
> This is a bit saner:
> /*
>   * These ops do not make sense for eDP, since they are provided
>   * by the panel-bridge corresponding to the attached eDP panel.
>   */
>
> My question was whether we really need to disable them for eDP since for
> eDP the detect and and get_modes will be overridden anyway.

And to go further, I'd expect that a bridge should expose the
functionality that it supports, regardless of what is connected down the
chain. Otherwise we won't be able to mix and match bridges because the
code is brittle, making assumptions about what is connected.

Re: [PATCH v7 0/4] Add support for the eDP panel over aux_bus

[Stephen Boyd]

Quoting Dmitry Baryshkov (2022-04-14 12:20:31)
> On 14/04/2022 19:40, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Apr 14, 2022 at 5:19 AM Sankeerth Billakanti
> > <quic_sbillaka@quicinc.com> wrote:
> >>
> >> This series adds support for generic eDP panel over aux_bus.
> >>
> >> These changes are dependent on the following series:
> >> https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654&state=*
> >
> > You're basically depending on the last two patches of that series.
> > What's the plan there? In patchwork they're marked as "Not
> > Applicable". If they're good to go, maybe we should land them? If not,
> > maybe you should include them (with Dmitry as the author, of course)
> > at the beginning of your series?
>
> No, please do not resend patches. The patches in question are marked as
> 'Not applicable' as they are really not applicable to Bjorn's tree.
> It would be better to point to the correct patchwork:
>
> https://patchwork.freedesktop.org/series/98585/
>
> Note those patches still lack the R-B tag. I can include them anyway,
> basing on Sankeerth's Tested-by tag, but the formal R-B would also be good.
>

Can you resend those as not RFC?

Re: [PATCH v7 0/4] Add support for the eDP panel over aux_bus

[Stephen Boyd]

Quoting Dmitry Baryshkov (2022-04-14 12:20:31)
> On 14/04/2022 19:40, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Apr 14, 2022 at 5:19 AM Sankeerth Billakanti
> > <quic_sbillaka@quicinc.com> wrote:
> >>
> >> This series adds support for generic eDP panel over aux_bus.
> >>
> >> These changes are dependent on the following series:
> >> https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654&state=*
> >
> > You're basically depending on the last two patches of that series.
> > What's the plan there? In patchwork they're marked as "Not
> > Applicable". If they're good to go, maybe we should land them? If not,
> > maybe you should include them (with Dmitry as the author, of course)
> > at the beginning of your series?
>
> No, please do not resend patches. The patches in question are marked as
> 'Not applicable' as they are really not applicable to Bjorn's tree.
> It would be better to point to the correct patchwork:
>
> https://patchwork.freedesktop.org/series/98585/
>
> Note those patches still lack the R-B tag. I can include them anyway,
> basing on Sankeerth's Tested-by tag, but the formal R-B would also be good.
>

Can you resend those as not RFC?

Re: [Freedreno] [PATCH v7 0/4] Add support for the eDP panel over aux_bus

[Abhinav Kumar]

Hi Dmitry

On 4/14/2022 12:43 PM, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2022-04-14 12:20:31)
>> On 14/04/2022 19:40, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Thu, Apr 14, 2022 at 5:19 AM Sankeerth Billakanti
>>> <quic_sbillaka@quicinc.com> wrote:
>>>>
>>>> This series adds support for generic eDP panel over aux_bus.
>>>>
>>>> These changes are dependent on the following series:
>>>> https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654&state=*
>>>
>>> You're basically depending on the last two patches of that series.
>>> What's the plan there? In patchwork they're marked as "Not
>>> Applicable". If they're good to go, maybe we should land them? If not,
>>> maybe you should include them (with Dmitry as the author, of course)
>>> at the beginning of your series?
>>
>> No, please do not resend patches. The patches in question are marked as
>> 'Not applicable' as they are really not applicable to Bjorn's tree.
>> It would be better to point to the correct patchwork:
>>
>> https://patchwork.freedesktop.org/series/98585/
>>
>> Note those patches still lack the R-B tag. I can include them anyway,
>> basing on Sankeerth's Tested-by tag, but the formal R-B would also be good.
>>
> 
> Can you resend those as not RFC?

Yes, please resend these, I can ack them.

Previously I held off my ack, as kuogee ran into some issues testing 
them which was later concluded to be a mismatch in QC internal trees due 
to different versions of the changes.( another reason why we should get 
these landed ).

Now, that Sankeerth has tested these, if you can remove RFC and post 
them, I can ack the.

Thanks

Abhinav

Re: [Freedreno] [PATCH v7 0/4] Add support for the eDP panel over aux_bus

[Abhinav Kumar]

Hi Dmitry

On 4/14/2022 12:43 PM, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2022-04-14 12:20:31)
>> On 14/04/2022 19:40, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Thu, Apr 14, 2022 at 5:19 AM Sankeerth Billakanti
>>> <quic_sbillaka@quicinc.com> wrote:
>>>>
>>>> This series adds support for generic eDP panel over aux_bus.
>>>>
>>>> These changes are dependent on the following series:
>>>> https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654&state=*
>>>
>>> You're basically depending on the last two patches of that series.
>>> What's the plan there? In patchwork they're marked as "Not
>>> Applicable". If they're good to go, maybe we should land them? If not,
>>> maybe you should include them (with Dmitry as the author, of course)
>>> at the beginning of your series?
>>
>> No, please do not resend patches. The patches in question are marked as
>> 'Not applicable' as they are really not applicable to Bjorn's tree.
>> It would be better to point to the correct patchwork:
>>
>> https://patchwork.freedesktop.org/series/98585/
>>
>> Note those patches still lack the R-B tag. I can include them anyway,
>> basing on Sankeerth's Tested-by tag, but the formal R-B would also be good.
>>
> 
> Can you resend those as not RFC?

Yes, please resend these, I can ack them.

Previously I held off my ack, as kuogee ran into some issues testing 
them which was later concluded to be a mismatch in QC internal trees due 
to different versions of the changes.( another reason why we should get 
these landed ).

Now, that Sankeerth has tested these, if you can remove RFC and post 
them, I can ack the.

Thanks

Abhinav

Re: [Freedreno] [PATCH v7 0/4] Add support for the eDP panel over aux_bus

[Dmitry Baryshkov]

On 14/04/2022 23:00, Abhinav Kumar wrote:
> Hi Dmitry
> 
> On 4/14/2022 12:43 PM, Stephen Boyd wrote:
>> Quoting Dmitry Baryshkov (2022-04-14 12:20:31)
>>> On 14/04/2022 19:40, Doug Anderson wrote:
>>>> Hi,
>>>>
>>>> On Thu, Apr 14, 2022 at 5:19 AM Sankeerth Billakanti
>>>> <quic_sbillaka@quicinc.com> wrote:
>>>>>
>>>>> This series adds support for generic eDP panel over aux_bus.
>>>>>
>>>>> These changes are dependent on the following series:
>>>>> https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654&state=* 
>>>>>
>>>>
>>>> You're basically depending on the last two patches of that series.
>>>> What's the plan there? In patchwork they're marked as "Not
>>>> Applicable". If they're good to go, maybe we should land them? If not,
>>>> maybe you should include them (with Dmitry as the author, of course)
>>>> at the beginning of your series?
>>>
>>> No, please do not resend patches. The patches in question are marked as
>>> 'Not applicable' as they are really not applicable to Bjorn's tree.
>>> It would be better to point to the correct patchwork:
>>>
>>> https://patchwork.freedesktop.org/series/98585/
>>>
>>> Note those patches still lack the R-B tag. I can include them anyway,
>>> basing on Sankeerth's Tested-by tag, but the formal R-B would also be 
>>> good.
>>>
>>
>> Can you resend those as not RFC?
> 
> Yes, please resend these, I can ack them.
> 
> Previously I held off my ack, as kuogee ran into some issues testing 
> them which was later concluded to be a mismatch in QC internal trees due 
> to different versions of the changes.( another reason why we should get 
> these landed ).
> 
> Now, that Sankeerth has tested these, if you can remove RFC and post 
> them, I can ack the.

Well, you can ack those patches without them being resent. You have 
already added your Reviewed-by to first three patches (which were merged 
during last window).


-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH v7 0/4] Add support for the eDP panel over aux_bus

[Dmitry Baryshkov]

On 14/04/2022 23:00, Abhinav Kumar wrote:
> Hi Dmitry
> 
> On 4/14/2022 12:43 PM, Stephen Boyd wrote:
>> Quoting Dmitry Baryshkov (2022-04-14 12:20:31)
>>> On 14/04/2022 19:40, Doug Anderson wrote:
>>>> Hi,
>>>>
>>>> On Thu, Apr 14, 2022 at 5:19 AM Sankeerth Billakanti
>>>> <quic_sbillaka@quicinc.com> wrote:
>>>>>
>>>>> This series adds support for generic eDP panel over aux_bus.
>>>>>
>>>>> These changes are dependent on the following series:
>>>>> https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654&state=* 
>>>>>
>>>>
>>>> You're basically depending on the last two patches of that series.
>>>> What's the plan there? In patchwork they're marked as "Not
>>>> Applicable". If they're good to go, maybe we should land them? If not,
>>>> maybe you should include them (with Dmitry as the author, of course)
>>>> at the beginning of your series?
>>>
>>> No, please do not resend patches. The patches in question are marked as
>>> 'Not applicable' as they are really not applicable to Bjorn's tree.
>>> It would be better to point to the correct patchwork:
>>>
>>> https://patchwork.freedesktop.org/series/98585/
>>>
>>> Note those patches still lack the R-B tag. I can include them anyway,
>>> basing on Sankeerth's Tested-by tag, but the formal R-B would also be 
>>> good.
>>>
>>
>> Can you resend those as not RFC?
> 
> Yes, please resend these, I can ack them.
> 
> Previously I held off my ack, as kuogee ran into some issues testing 
> them which was later concluded to be a mismatch in QC internal trees due 
> to different versions of the changes.( another reason why we should get 
> these landed ).
> 
> Now, that Sankeerth has tested these, if you can remove RFC and post 
> them, I can ack the.

Well, you can ack those patches without them being resent. You have 
already added your Reviewed-by to first three patches (which were merged 
during last window).


-- 
With best wishes
Dmitry

Re: [PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus

[Doug Anderson]

Hi,

On Thu, Apr 14, 2022 at 12:40 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Dmitry Baryshkov (2022-04-14 12:16:14)
> >
> > I think it's too verbose and a bit incorrect.

Not sure which part you're asserting is incorrect, but shorter is OK w/ me too.


> > This is a bit saner:
> > /*
> >   * These ops do not make sense for eDP, since they are provided
> >   * by the panel-bridge corresponding to the attached eDP panel.
> >   */
> >
> > My question was whether we really need to disable them for eDP since for
> > eDP the detect and and get_modes will be overridden anyway.

Hmm, interesting. Probably for DRM_BRIDGE_OP_MODES that will work?
It's definitely worth confirming but from my reading of the code it
_probably_ wouldn't hurt.

One thing someone would want to confirm would be what would happen if
we move this code and the panel code to implement DRM_BRIDGE_OP_EDID
properly. It looks as if both actually ought to be implementing that
instead of DRM_BRIDGE_OP_MODES, at least in some cases. A fix for a
future day. Could we get into trouble if one moved before the other?
Then the panel would no longer override the eDP controller and the eDP
controller would try to read from a possibly unpowered panel?

So I guess in the end my preference would be that we know that driving
the EDID read from the controller isn't a great idea for eDP (since we
have no way to ensure that the panel is powered) so why risk it and
set the bit saying we can do it?


For hotplug/detect I'm even less confident that setting the bits would
be harmless. I haven't sat down and traced everything, but from what I
can see the panel _doesn't_ set these bits, does it? I believe that
the rule is that when every bridge in the chain _doesn't_ implement
detect/hotplug that the panel is always present. The moment someone
says "hey, I can detect" then it suddenly becomes _not_ always
present. Yes, I guess we could have the panel implement "detect" and
return true, but I'm not convinced that's actually better...


> And to go further, I'd expect that a bridge should expose the
> functionality that it supports, regardless of what is connected down the
> chain. Otherwise we won't be able to mix and match bridges because the
> code is brittle, making assumptions about what is connected.

From my point of view the bridge simply doesn't support any of the
three things when we're in eDP mode. Yes, it's the same driver. Yes,
eDP and DP share nearly the same signalling on the wire. Yes, it's
easily possible to make a single controller that supports DP and eDP.
...but the rules around detection and power sequencing are simply
different for the two cases. The controller simply _cannot_ detect
whether the panel is connected in the eDP case and it _must_ assume
that the panel is always connected. Yes, it has an HPD pin. No, that
HPD pin doesn't tell when the panel is present. The panel is always
present. The panel is always present.

So, IMO, it is _incorrect_ to say we can support HPD and DETECT if we
know we're in eDP mode.

-Doug

Re: [PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus

[Doug Anderson]

Hi,

On Thu, Apr 14, 2022 at 12:40 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Dmitry Baryshkov (2022-04-14 12:16:14)
> >
> > I think it's too verbose and a bit incorrect.

Not sure which part you're asserting is incorrect, but shorter is OK w/ me too.


> > This is a bit saner:
> > /*
> >   * These ops do not make sense for eDP, since they are provided
> >   * by the panel-bridge corresponding to the attached eDP panel.
> >   */
> >
> > My question was whether we really need to disable them for eDP since for
> > eDP the detect and and get_modes will be overridden anyway.

Hmm, interesting. Probably for DRM_BRIDGE_OP_MODES that will work?
It's definitely worth confirming but from my reading of the code it
_probably_ wouldn't hurt.

One thing someone would want to confirm would be what would happen if
we move this code and the panel code to implement DRM_BRIDGE_OP_EDID
properly. It looks as if both actually ought to be implementing that
instead of DRM_BRIDGE_OP_MODES, at least in some cases. A fix for a
future day. Could we get into trouble if one moved before the other?
Then the panel would no longer override the eDP controller and the eDP
controller would try to read from a possibly unpowered panel?

So I guess in the end my preference would be that we know that driving
the EDID read from the controller isn't a great idea for eDP (since we
have no way to ensure that the panel is powered) so why risk it and
set the bit saying we can do it?


For hotplug/detect I'm even less confident that setting the bits would
be harmless. I haven't sat down and traced everything, but from what I
can see the panel _doesn't_ set these bits, does it? I believe that
the rule is that when every bridge in the chain _doesn't_ implement
detect/hotplug that the panel is always present. The moment someone
says "hey, I can detect" then it suddenly becomes _not_ always
present. Yes, I guess we could have the panel implement "detect" and
return true, but I'm not convinced that's actually better...


> And to go further, I'd expect that a bridge should expose the
> functionality that it supports, regardless of what is connected down the
> chain. Otherwise we won't be able to mix and match bridges because the
> code is brittle, making assumptions about what is connected.

From my point of view the bridge simply doesn't support any of the
three things when we're in eDP mode. Yes, it's the same driver. Yes,
eDP and DP share nearly the same signalling on the wire. Yes, it's
easily possible to make a single controller that supports DP and eDP.
...but the rules around detection and power sequencing are simply
different for the two cases. The controller simply _cannot_ detect
whether the panel is connected in the eDP case and it _must_ assume
that the panel is always connected. Yes, it has an HPD pin. No, that
HPD pin doesn't tell when the panel is present. The panel is always
present. The panel is always present.

So, IMO, it is _incorrect_ to say we can support HPD and DETECT if we
know we're in eDP mode.

-Doug

Re: [Freedreno] [PATCH v7 0/4] Add support for the eDP panel over aux_bus

[Abhinav Kumar]

On 4/14/2022 1:03 PM, Dmitry Baryshkov wrote:
> On 14/04/2022 23:00, Abhinav Kumar wrote:
>> Hi Dmitry
>>
>> On 4/14/2022 12:43 PM, Stephen Boyd wrote:
>>> Quoting Dmitry Baryshkov (2022-04-14 12:20:31)
>>>> On 14/04/2022 19:40, Doug Anderson wrote:
>>>>> Hi,
>>>>>
>>>>> On Thu, Apr 14, 2022 at 5:19 AM Sankeerth Billakanti
>>>>> <quic_sbillaka@quicinc.com> wrote:
>>>>>>
>>>>>> This series adds support for generic eDP panel over aux_bus.
>>>>>>
>>>>>> These changes are dependent on the following series:
>>>>>> https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654&state=* 
>>>>>>
>>>>>
>>>>> You're basically depending on the last two patches of that series.
>>>>> What's the plan there? In patchwork they're marked as "Not
>>>>> Applicable". If they're good to go, maybe we should land them? If not,
>>>>> maybe you should include them (with Dmitry as the author, of course)
>>>>> at the beginning of your series?
>>>>
>>>> No, please do not resend patches. The patches in question are marked as
>>>> 'Not applicable' as they are really not applicable to Bjorn's tree.
>>>> It would be better to point to the correct patchwork:
>>>>
>>>> https://patchwork.freedesktop.org/series/98585/
>>>>
>>>> Note those patches still lack the R-B tag. I can include them anyway,
>>>> basing on Sankeerth's Tested-by tag, but the formal R-B would also 
>>>> be good.
>>>>
>>>
>>> Can you resend those as not RFC?
>>
>> Yes, please resend these, I can ack them.
>>
>> Previously I held off my ack, as kuogee ran into some issues testing 
>> them which was later concluded to be a mismatch in QC internal trees 
>> due to different versions of the changes.( another reason why we 
>> should get these landed ).
>>
>> Now, that Sankeerth has tested these, if you can remove RFC and post 
>> them, I can ack the.
> 
> Well, you can ack those patches without them being resent. You have 
> already added your Reviewed-by to first three patches (which were merged 
> during last window).
> 
I thought you might have to rebase them :) that way you could have 
resent the rebased patch with the RFC tag removed.

If you dont, you now have my R-b.

Thanks

Abhinav
> 

Re: [Freedreno] [PATCH v7 0/4] Add support for the eDP panel over aux_bus

[Abhinav Kumar]

On 4/14/2022 1:03 PM, Dmitry Baryshkov wrote:
> On 14/04/2022 23:00, Abhinav Kumar wrote:
>> Hi Dmitry
>>
>> On 4/14/2022 12:43 PM, Stephen Boyd wrote:
>>> Quoting Dmitry Baryshkov (2022-04-14 12:20:31)
>>>> On 14/04/2022 19:40, Doug Anderson wrote:
>>>>> Hi,
>>>>>
>>>>> On Thu, Apr 14, 2022 at 5:19 AM Sankeerth Billakanti
>>>>> <quic_sbillaka@quicinc.com> wrote:
>>>>>>
>>>>>> This series adds support for generic eDP panel over aux_bus.
>>>>>>
>>>>>> These changes are dependent on the following series:
>>>>>> https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654&state=* 
>>>>>>
>>>>>
>>>>> You're basically depending on the last two patches of that series.
>>>>> What's the plan there? In patchwork they're marked as "Not
>>>>> Applicable". If they're good to go, maybe we should land them? If not,
>>>>> maybe you should include them (with Dmitry as the author, of course)
>>>>> at the beginning of your series?
>>>>
>>>> No, please do not resend patches. The patches in question are marked as
>>>> 'Not applicable' as they are really not applicable to Bjorn's tree.
>>>> It would be better to point to the correct patchwork:
>>>>
>>>> https://patchwork.freedesktop.org/series/98585/
>>>>
>>>> Note those patches still lack the R-B tag. I can include them anyway,
>>>> basing on Sankeerth's Tested-by tag, but the formal R-B would also 
>>>> be good.
>>>>
>>>
>>> Can you resend those as not RFC?
>>
>> Yes, please resend these, I can ack them.
>>
>> Previously I held off my ack, as kuogee ran into some issues testing 
>> them which was later concluded to be a mismatch in QC internal trees 
>> due to different versions of the changes.( another reason why we 
>> should get these landed ).
>>
>> Now, that Sankeerth has tested these, if you can remove RFC and post 
>> them, I can ack the.
> 
> Well, you can ack those patches without them being resent. You have 
> already added your Reviewed-by to first three patches (which were merged 
> during last window).
> 
I thought you might have to rebase them :) that way you could have 
resent the rebased patch with the RFC tag removed.

If you dont, you now have my R-b.

Thanks

Abhinav
> 

Re: [Freedreno] [PATCH v7 0/4] Add support for the eDP panel over aux_bus

[Dmitry Baryshkov]

On 14/04/2022 23:19, Abhinav Kumar wrote:
> 
> 
> On 4/14/2022 1:03 PM, Dmitry Baryshkov wrote:
>> On 14/04/2022 23:00, Abhinav Kumar wrote:
>>> Hi Dmitry
>>>
>>> On 4/14/2022 12:43 PM, Stephen Boyd wrote:
>>>> Quoting Dmitry Baryshkov (2022-04-14 12:20:31)
>>>>> On 14/04/2022 19:40, Doug Anderson wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Thu, Apr 14, 2022 at 5:19 AM Sankeerth Billakanti
>>>>>> <quic_sbillaka@quicinc.com> wrote:
>>>>>>>
>>>>>>> This series adds support for generic eDP panel over aux_bus.
>>>>>>>
>>>>>>> These changes are dependent on the following series:
>>>>>>> https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654&state=* 
>>>>>>>
>>>>>>
>>>>>> You're basically depending on the last two patches of that series.
>>>>>> What's the plan there? In patchwork they're marked as "Not
>>>>>> Applicable". If they're good to go, maybe we should land them? If 
>>>>>> not,
>>>>>> maybe you should include them (with Dmitry as the author, of course)
>>>>>> at the beginning of your series?
>>>>>
>>>>> No, please do not resend patches. The patches in question are 
>>>>> marked as
>>>>> 'Not applicable' as they are really not applicable to Bjorn's tree.
>>>>> It would be better to point to the correct patchwork:
>>>>>
>>>>> https://patchwork.freedesktop.org/series/98585/
>>>>>
>>>>> Note those patches still lack the R-B tag. I can include them anyway,
>>>>> basing on Sankeerth's Tested-by tag, but the formal R-B would also 
>>>>> be good.
>>>>>
>>>>
>>>> Can you resend those as not RFC?
>>>
>>> Yes, please resend these, I can ack them.
>>>
>>> Previously I held off my ack, as kuogee ran into some issues testing 
>>> them which was later concluded to be a mismatch in QC internal trees 
>>> due to different versions of the changes.( another reason why we 
>>> should get these landed ).
>>>
>>> Now, that Sankeerth has tested these, if you can remove RFC and post 
>>> them, I can ack the.
>>
>> Well, you can ack those patches without them being resent. You have 
>> already added your Reviewed-by to first three patches (which were 
>> merged during last window).
>>
> I thought you might have to rebase them :) that way you could have 
> resent the rebased patch with the RFC tag removed.
> 
> If you dont, you now have my R-b.

Thank you!

-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH v7 0/4] Add support for the eDP panel over aux_bus

[Dmitry Baryshkov]

On 14/04/2022 23:19, Abhinav Kumar wrote:
> 
> 
> On 4/14/2022 1:03 PM, Dmitry Baryshkov wrote:
>> On 14/04/2022 23:00, Abhinav Kumar wrote:
>>> Hi Dmitry
>>>
>>> On 4/14/2022 12:43 PM, Stephen Boyd wrote:
>>>> Quoting Dmitry Baryshkov (2022-04-14 12:20:31)
>>>>> On 14/04/2022 19:40, Doug Anderson wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Thu, Apr 14, 2022 at 5:19 AM Sankeerth Billakanti
>>>>>> <quic_sbillaka@quicinc.com> wrote:
>>>>>>>
>>>>>>> This series adds support for generic eDP panel over aux_bus.
>>>>>>>
>>>>>>> These changes are dependent on the following series:
>>>>>>> https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654&state=* 
>>>>>>>
>>>>>>
>>>>>> You're basically depending on the last two patches of that series.
>>>>>> What's the plan there? In patchwork they're marked as "Not
>>>>>> Applicable". If they're good to go, maybe we should land them? If 
>>>>>> not,
>>>>>> maybe you should include them (with Dmitry as the author, of course)
>>>>>> at the beginning of your series?
>>>>>
>>>>> No, please do not resend patches. The patches in question are 
>>>>> marked as
>>>>> 'Not applicable' as they are really not applicable to Bjorn's tree.
>>>>> It would be better to point to the correct patchwork:
>>>>>
>>>>> https://patchwork.freedesktop.org/series/98585/
>>>>>
>>>>> Note those patches still lack the R-B tag. I can include them anyway,
>>>>> basing on Sankeerth's Tested-by tag, but the formal R-B would also 
>>>>> be good.
>>>>>
>>>>
>>>> Can you resend those as not RFC?
>>>
>>> Yes, please resend these, I can ack them.
>>>
>>> Previously I held off my ack, as kuogee ran into some issues testing 
>>> them which was later concluded to be a mismatch in QC internal trees 
>>> due to different versions of the changes.( another reason why we 
>>> should get these landed ).
>>>
>>> Now, that Sankeerth has tested these, if you can remove RFC and post 
>>> them, I can ack the.
>>
>> Well, you can ack those patches without them being resent. You have 
>> already added your Reviewed-by to first three patches (which were 
>> merged during last window).
>>
> I thought you might have to rebase them :) that way you could have 
> resent the rebased patch with the RFC tag removed.
> 
> If you dont, you now have my R-b.

Thank you!

-- 
With best wishes
Dmitry

Re: [PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus

[Dmitry Baryshkov]

On 14/04/2022 23:09, Doug Anderson wrote:
> Hi,
> 
> On Thu, Apr 14, 2022 at 12:40 PM Stephen Boyd <swboyd@chromium.org> wrote:
>>
>> Quoting Dmitry Baryshkov (2022-04-14 12:16:14)
>>>
>>> I think it's too verbose and a bit incorrect.
> 
> Not sure which part you're asserting is incorrect, but shorter is OK w/ me too.

I was referring to the "If we don't implement the ops..." part. For some 
reason I thought that panel implements detect() callback (and thus the 
DRM will not care because the next bridge takes precedence).

However I was mistaken, please excuse me. Your description was correct 
and I was wrong. The panel bridge doesn't implement callback. Most 
probably I mixed it with the display_connector bridge.

So... your description is more correct.

> 
> 
>>> This is a bit saner:
>>> /*
>>>    * These ops do not make sense for eDP, since they are provided
>>>    * by the panel-bridge corresponding to the attached eDP panel.
>>>    */
>>>
>>> My question was whether we really need to disable them for eDP since for
>>> eDP the detect and and get_modes will be overridden anyway.
> 
> Hmm, interesting. Probably for DRM_BRIDGE_OP_MODES that will work?
> It's definitely worth confirming but from my reading of the code it
> _probably_ wouldn't hurt.
> 
> One thing someone would want to confirm would be what would happen if
> we move this code and the panel code to implement DRM_BRIDGE_OP_EDID
> properly. It looks as if both actually ought to be implementing that
> instead of DRM_BRIDGE_OP_MODES, at least in some cases. A fix for a
> future day. Could we get into trouble if one moved before the other?
> Then the panel would no longer override the eDP controller and the eDP
> controller would try to read from a possibly unpowered panel?

That would depend on the way the get_edid would be implemented in DP 
driver. Currently the edid is cached via the 
dp_display_process_hpd_high() -> dp_panel_read_sink_caps() call chain.

With this patchset, the dp_hpd_plug_handle() -> 
dp_display_usbpd_configure_cb() -> dp_display_process_hpd_high() will be 
called too late for the get_modes/get_edid (from dp_bridge's enable() op).

There is another issue. drm_panel has only get_modes() callback, so 
panel_bridge can not implement get_edid() unless we extend the panel 
interface (which might be a good idea).

> 
> So I guess in the end my preference would be that we know that driving
> the EDID read from the controller isn't a great idea for eDP (since we
> have no way to ensure that the panel is powered) so why risk it and
> set the bit saying we can do it?

Yep.


> For hotplug/detect I'm even less confident that setting the bits would
> be harmless. I haven't sat down and traced everything, but from what I
> can see the panel _doesn't_ set these bits, does it? I believe that
> the rule is that when every bridge in the chain _doesn't_ implement
> detect/hotplug that the panel is always present. The moment someone
> says "hey, I can detect" then it suddenly becomes _not_ always
> present. Yes, I guess we could have the panel implement "detect" and
> return true, but I'm not convinced that's actually better...

I think it makes sense to implement OP_DETECT in panel bridge (that 
always returns connector_status_connected) at least to override the 
possible detect ops in previous bridges.

>> And to go further, I'd expect that a bridge should expose the
>> functionality that it supports, regardless of what is connected down the
>> chain. Otherwise we won't be able to mix and match bridges because the
>> code is brittle, making assumptions about what is connected.
> 
>  From my point of view the bridge simply doesn't support any of the
> three things when we're in eDP mode. Yes, it's the same driver. Yes,
> eDP and DP share nearly the same signalling on the wire. Yes, it's
> easily possible to make a single controller that supports DP and eDP.
> ...but the rules around detection and power sequencing are simply
> different for the two cases.

I just hope that during refactoring this can be expressed in a more 
natural way.

> The controller simply _cannot_ detect
> whether the panel is connected in the eDP case and it _must_ assume
> that the panel is always connected. Yes, it has an HPD pin. No, that
> HPD pin doesn't tell when the panel is present. The panel is always
> present. The panel is always present.

Yep, I remember regarding the HPD pin. And I still think that panel-edp 
(and panel bridge) should provide an overriding OP_DETECT.

> So, IMO, it is _incorrect_ to say we can support HPD and DETECT if we
> know we're in eDP mode.

I see your point. Let's do it this way. Maybe (hopefully) it will become 
more logical during refactoring. Or maybe I'll just tune myself into the 
DP/eDP logic :D

-- 
With best wishes
Dmitry

Re: [PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus

[Dmitry Baryshkov]

On 14/04/2022 23:09, Doug Anderson wrote:
> Hi,
> 
> On Thu, Apr 14, 2022 at 12:40 PM Stephen Boyd <swboyd@chromium.org> wrote:
>>
>> Quoting Dmitry Baryshkov (2022-04-14 12:16:14)
>>>
>>> I think it's too verbose and a bit incorrect.
> 
> Not sure which part you're asserting is incorrect, but shorter is OK w/ me too.

I was referring to the "If we don't implement the ops..." part. For some 
reason I thought that panel implements detect() callback (and thus the 
DRM will not care because the next bridge takes precedence).

However I was mistaken, please excuse me. Your description was correct 
and I was wrong. The panel bridge doesn't implement callback. Most 
probably I mixed it with the display_connector bridge.

So... your description is more correct.

> 
> 
>>> This is a bit saner:
>>> /*
>>>    * These ops do not make sense for eDP, since they are provided
>>>    * by the panel-bridge corresponding to the attached eDP panel.
>>>    */
>>>
>>> My question was whether we really need to disable them for eDP since for
>>> eDP the detect and and get_modes will be overridden anyway.
> 
> Hmm, interesting. Probably for DRM_BRIDGE_OP_MODES that will work?
> It's definitely worth confirming but from my reading of the code it
> _probably_ wouldn't hurt.
> 
> One thing someone would want to confirm would be what would happen if
> we move this code and the panel code to implement DRM_BRIDGE_OP_EDID
> properly. It looks as if both actually ought to be implementing that
> instead of DRM_BRIDGE_OP_MODES, at least in some cases. A fix for a
> future day. Could we get into trouble if one moved before the other?
> Then the panel would no longer override the eDP controller and the eDP
> controller would try to read from a possibly unpowered panel?

That would depend on the way the get_edid would be implemented in DP 
driver. Currently the edid is cached via the 
dp_display_process_hpd_high() -> dp_panel_read_sink_caps() call chain.

With this patchset, the dp_hpd_plug_handle() -> 
dp_display_usbpd_configure_cb() -> dp_display_process_hpd_high() will be 
called too late for the get_modes/get_edid (from dp_bridge's enable() op).

There is another issue. drm_panel has only get_modes() callback, so 
panel_bridge can not implement get_edid() unless we extend the panel 
interface (which might be a good idea).

> 
> So I guess in the end my preference would be that we know that driving
> the EDID read from the controller isn't a great idea for eDP (since we
> have no way to ensure that the panel is powered) so why risk it and
> set the bit saying we can do it?

Yep.


> For hotplug/detect I'm even less confident that setting the bits would
> be harmless. I haven't sat down and traced everything, but from what I
> can see the panel _doesn't_ set these bits, does it? I believe that
> the rule is that when every bridge in the chain _doesn't_ implement
> detect/hotplug that the panel is always present. The moment someone
> says "hey, I can detect" then it suddenly becomes _not_ always
> present. Yes, I guess we could have the panel implement "detect" and
> return true, but I'm not convinced that's actually better...

I think it makes sense to implement OP_DETECT in panel bridge (that 
always returns connector_status_connected) at least to override the 
possible detect ops in previous bridges.

>> And to go further, I'd expect that a bridge should expose the
>> functionality that it supports, regardless of what is connected down the
>> chain. Otherwise we won't be able to mix and match bridges because the
>> code is brittle, making assumptions about what is connected.
> 
>  From my point of view the bridge simply doesn't support any of the
> three things when we're in eDP mode. Yes, it's the same driver. Yes,
> eDP and DP share nearly the same signalling on the wire. Yes, it's
> easily possible to make a single controller that supports DP and eDP.
> ...but the rules around detection and power sequencing are simply
> different for the two cases.

I just hope that during refactoring this can be expressed in a more 
natural way.

> The controller simply _cannot_ detect
> whether the panel is connected in the eDP case and it _must_ assume
> that the panel is always connected. Yes, it has an HPD pin. No, that
> HPD pin doesn't tell when the panel is present. The panel is always
> present. The panel is always present.

Yep, I remember regarding the HPD pin. And I still think that panel-edp 
(and panel bridge) should provide an overriding OP_DETECT.

> So, IMO, it is _incorrect_ to say we can support HPD and DETECT if we
> know we're in eDP mode.

I see your point. Let's do it this way. Maybe (hopefully) it will become 
more logical during refactoring. Or maybe I'll just tune myself into the 
DP/eDP logic :D

-- 
With best wishes
Dmitry

Re: [PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus

[Doug Anderson]

Hi,

On Thu, Apr 14, 2022 at 2:16 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> > Hmm, interesting. Probably for DRM_BRIDGE_OP_MODES that will work?
> > It's definitely worth confirming but from my reading of the code it
> > _probably_ wouldn't hurt.
> >
> > One thing someone would want to confirm would be what would happen if
> > we move this code and the panel code to implement DRM_BRIDGE_OP_EDID
> > properly. It looks as if both actually ought to be implementing that
> > instead of DRM_BRIDGE_OP_MODES, at least in some cases. A fix for a
> > future day. Could we get into trouble if one moved before the other?
> > Then the panel would no longer override the eDP controller and the eDP
> > controller would try to read from a possibly unpowered panel?
>
> That would depend on the way the get_edid would be implemented in DP
> driver. Currently the edid is cached via the
> dp_display_process_hpd_high() -> dp_panel_read_sink_caps() call chain.
>
> With this patchset, the dp_hpd_plug_handle() ->
> dp_display_usbpd_configure_cb() -> dp_display_process_hpd_high() will be
> called too late for the get_modes/get_edid (from dp_bridge's enable() op).
>
> There is another issue. drm_panel has only get_modes() callback, so
> panel_bridge can not implement get_edid() unless we extend the panel
> interface (which might be a good idea).

Ah, that makes sense and explains why the current panel code does the
EDID reading in its get_modes() function even though get_modes() is
_documented_ that it doesn't read the EDID. ;-) I guess it's another
of the "let's move some people over to the new way but we'll keep the
old code working". Definitely makes it hard to understand at times.


> > For hotplug/detect I'm even less confident that setting the bits would
> > be harmless. I haven't sat down and traced everything, but from what I
> > can see the panel _doesn't_ set these bits, does it? I believe that
> > the rule is that when every bridge in the chain _doesn't_ implement
> > detect/hotplug that the panel is always present. The moment someone
> > says "hey, I can detect" then it suddenly becomes _not_ always
> > present. Yes, I guess we could have the panel implement "detect" and
> > return true, but I'm not convinced that's actually better...
>
> I think it makes sense to implement OP_DETECT in panel bridge (that
> always returns connector_status_connected) at least to override the
> possible detect ops in previous bridges.

So I truly don't know the right answer, but are you sure that's the
best design? I _think_ that panel_bridge is used for all kinds of
panels, right? So what if there's some type of display that uses a
panel but there's still a mechanism that supports physical detection
of the panel? By implementing "detect" in the generic panel_bridge
then you're _preventing_ anyone higher up in the chain from
implementing it and you're forcing it to be "always connected".

For instance, we could come up with a new display standard called
"pluggable eDP" that is just like eDP except that you can physically
detect it. This imaginary new display standard is different from DP
because it has eDP power sequencing (fully powers the display off when
the screen is off) but it's hot pluggable! It introduces a new pin
that goes to the DP controller called RT-HPD for "really, truly hot
plug detect" that works even when the panel is off. The existing "HPD"
pin continues to mean that the panel is read to communicate. If the
drm_panel hardcodes "always connected" then I can't implement my
"pluggable eDP" system, right? However, if we leave it just like it is
today then my new system would be easy to implement. ;-)

The above example is obviously not truly a real one but I guess my
point is that I find it more intuitive / useful to say that we should
only implement "detect" if we truly think we can detect and that if
nobody says they can detect then we must be always connected.

As an aside; I think in general it's not always easy to fit every
possible graphics system into these "bridge chains" and the simple
sequence of pre-enable, enable, etc, so we have to do our best and
accept the fact that sometimes we'll need special cases. Dave
Stephenson's patches [1] should tell us that, at least.

[1] https://lore.kernel.org/all/cover.1646406653.git.dave.stevenson@raspberrypi.com/


-Doug

Re: [PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus

[Doug Anderson]

Hi,

On Thu, Apr 14, 2022 at 2:16 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> > Hmm, interesting. Probably for DRM_BRIDGE_OP_MODES that will work?
> > It's definitely worth confirming but from my reading of the code it
> > _probably_ wouldn't hurt.
> >
> > One thing someone would want to confirm would be what would happen if
> > we move this code and the panel code to implement DRM_BRIDGE_OP_EDID
> > properly. It looks as if both actually ought to be implementing that
> > instead of DRM_BRIDGE_OP_MODES, at least in some cases. A fix for a
> > future day. Could we get into trouble if one moved before the other?
> > Then the panel would no longer override the eDP controller and the eDP
> > controller would try to read from a possibly unpowered panel?
>
> That would depend on the way the get_edid would be implemented in DP
> driver. Currently the edid is cached via the
> dp_display_process_hpd_high() -> dp_panel_read_sink_caps() call chain.
>
> With this patchset, the dp_hpd_plug_handle() ->
> dp_display_usbpd_configure_cb() -> dp_display_process_hpd_high() will be
> called too late for the get_modes/get_edid (from dp_bridge's enable() op).
>
> There is another issue. drm_panel has only get_modes() callback, so
> panel_bridge can not implement get_edid() unless we extend the panel
> interface (which might be a good idea).

Ah, that makes sense and explains why the current panel code does the
EDID reading in its get_modes() function even though get_modes() is
_documented_ that it doesn't read the EDID. ;-) I guess it's another
of the "let's move some people over to the new way but we'll keep the
old code working". Definitely makes it hard to understand at times.


> > For hotplug/detect I'm even less confident that setting the bits would
> > be harmless. I haven't sat down and traced everything, but from what I
> > can see the panel _doesn't_ set these bits, does it? I believe that
> > the rule is that when every bridge in the chain _doesn't_ implement
> > detect/hotplug that the panel is always present. The moment someone
> > says "hey, I can detect" then it suddenly becomes _not_ always
> > present. Yes, I guess we could have the panel implement "detect" and
> > return true, but I'm not convinced that's actually better...
>
> I think it makes sense to implement OP_DETECT in panel bridge (that
> always returns connector_status_connected) at least to override the
> possible detect ops in previous bridges.

So I truly don't know the right answer, but are you sure that's the
best design? I _think_ that panel_bridge is used for all kinds of
panels, right? So what if there's some type of display that uses a
panel but there's still a mechanism that supports physical detection
of the panel? By implementing "detect" in the generic panel_bridge
then you're _preventing_ anyone higher up in the chain from
implementing it and you're forcing it to be "always connected".

For instance, we could come up with a new display standard called
"pluggable eDP" that is just like eDP except that you can physically
detect it. This imaginary new display standard is different from DP
because it has eDP power sequencing (fully powers the display off when
the screen is off) but it's hot pluggable! It introduces a new pin
that goes to the DP controller called RT-HPD for "really, truly hot
plug detect" that works even when the panel is off. The existing "HPD"
pin continues to mean that the panel is read to communicate. If the
drm_panel hardcodes "always connected" then I can't implement my
"pluggable eDP" system, right? However, if we leave it just like it is
today then my new system would be easy to implement. ;-)

The above example is obviously not truly a real one but I guess my
point is that I find it more intuitive / useful to say that we should
only implement "detect" if we truly think we can detect and that if
nobody says they can detect then we must be always connected.

As an aside; I think in general it's not always easy to fit every
possible graphics system into these "bridge chains" and the simple
sequence of pre-enable, enable, etc, so we have to do our best and
accept the fact that sometimes we'll need special cases. Dave
Stephenson's patches [1] should tell us that, at least.

[1] https://lore.kernel.org/all/cover.1646406653.git.dave.stevenson@raspberrypi.com/


-Doug