[PATCH v4 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x

[PATCH v4 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x

[Bjorn Andersson]

The current implementation supports a single DP instance and the DPU code will
only match it against INTF_DP instance 0. These patches extends this to allow
multiple DP instances and support for matching against DP instances beyond 0.

With that in place add SC8180x DP and eDP controllers.

Bjorn Andersson (7):
  drm/msm/dp: Remove global g_dp_display variable
  drm/msm/dp: Modify prototype of encoder based API
  drm/msm/dp: Allow specifying connector_type per controller
  drm/msm/dp: Allow attaching a drm_panel
  drm/msm/dp: Support up to 3 DP controllers
  dt-bindings: msm/dp: Add SC8180x compatibles
  drm/msm/dp: Add sc8180x DP controllers

 .../bindings/display/msm/dp-controller.yaml   |   2 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   |  23 +--
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |  66 ++++----
 .../gpu/drm/msm/disp/msm_disp_snapshot_util.c |   8 +-
 drivers/gpu/drm/msm/dp/dp_display.c           | 153 ++++++++++--------
 drivers/gpu/drm/msm/dp/dp_display.h           |   2 +
 drivers/gpu/drm/msm/dp/dp_drm.c               |  13 +-
 drivers/gpu/drm/msm/dp/dp_parser.c            |  30 +++-
 drivers/gpu/drm/msm/dp/dp_parser.h            |   3 +-
 drivers/gpu/drm/msm/msm_drv.h                 |   2 +-
 10 files changed, 194 insertions(+), 108 deletions(-)

-- 
2.29.2

[PATCH v4 1/7] drm/msm/dp: Remove global g_dp_display variable

[Bjorn Andersson]

As the Qualcomm DisplayPort driver only supports a single instance of
the driver the commonly used struct dp_display is kept in a global
variable. As we introduce additional instances this obviously doesn't
work.

Replace this with a combination of existing references to adjacent
objects and drvdata.

Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v3:
- None

 drivers/gpu/drm/msm/dp/dp_display.c | 80 ++++++++---------------------
 1 file changed, 21 insertions(+), 59 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index fbe4c2cd52a3..5d3ee5ef07c2 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -27,7 +27,6 @@
 #include "dp_audio.h"
 #include "dp_debug.h"
 
-static struct msm_dp *g_dp_display;
 #define HPD_STRING_SIZE 30
 
 enum {
@@ -121,6 +120,13 @@ static const struct of_device_id dp_dt_match[] = {
 	{}
 };
 
+static struct dp_display_private *dev_get_dp_display_private(struct device *dev)
+{
+	struct msm_dp *dp = dev_get_drvdata(dev);
+
+	return container_of(dp, struct dp_display_private, dp_display);
+}
+
 static int dp_add_event(struct dp_display_private *dp_priv, u32 event,
 						u32 data, u32 delay)
 {
@@ -197,15 +203,12 @@ static int dp_display_bind(struct device *dev, struct device *master,
 			   void *data)
 {
 	int rc = 0;
-	struct dp_display_private *dp;
-	struct drm_device *drm;
+	struct dp_display_private *dp = dev_get_dp_display_private(dev);
 	struct msm_drm_private *priv;
+	struct drm_device *drm;
 
 	drm = dev_get_drvdata(master);
 
-	dp = container_of(g_dp_display,
-			struct dp_display_private, dp_display);
-
 	dp->dp_display.drm_dev = drm;
 	priv = drm->dev_private;
 	priv->dp = &(dp->dp_display);
@@ -240,13 +243,10 @@ static int dp_display_bind(struct device *dev, struct device *master,
 static void dp_display_unbind(struct device *dev, struct device *master,
 			      void *data)
 {
-	struct dp_display_private *dp;
+	struct dp_display_private *dp = dev_get_dp_display_private(dev);
 	struct drm_device *drm = dev_get_drvdata(master);
 	struct msm_drm_private *priv = drm->dev_private;
 
-	dp = container_of(g_dp_display,
-			struct dp_display_private, dp_display);
-
 	dp_power_client_deinit(dp->power);
 	dp_aux_unregister(dp->aux);
 	priv->dp = NULL;
@@ -379,38 +379,17 @@ static void dp_display_host_deinit(struct dp_display_private *dp)
 
 static int dp_display_usbpd_configure_cb(struct device *dev)
 {
-	int rc = 0;
-	struct dp_display_private *dp;
-
-	if (!dev) {
-		DRM_ERROR("invalid dev\n");
-		rc = -EINVAL;
-		goto end;
-	}
-
-	dp = container_of(g_dp_display,
-			struct dp_display_private, dp_display);
+	struct dp_display_private *dp = dev_get_dp_display_private(dev);
 
 	dp_display_host_init(dp, false);
 
-	rc = dp_display_process_hpd_high(dp);
-end:
-	return rc;
+	return dp_display_process_hpd_high(dp);
 }
 
 static int dp_display_usbpd_disconnect_cb(struct device *dev)
 {
 	int rc = 0;
-	struct dp_display_private *dp;
-
-	if (!dev) {
-		DRM_ERROR("invalid dev\n");
-		rc = -EINVAL;
-		return rc;
-	}
-
-	dp = container_of(g_dp_display,
-			struct dp_display_private, dp_display);
+	struct dp_display_private *dp = dev_get_dp_display_private(dev);
 
 	dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
 
@@ -472,15 +451,7 @@ static int dp_display_usbpd_attention_cb(struct device *dev)
 {
 	int rc = 0;
 	u32 sink_request;
-	struct dp_display_private *dp;
-
-	if (!dev) {
-		DRM_ERROR("invalid dev\n");
-		return -EINVAL;
-	}
-
-	dp = container_of(g_dp_display,
-			struct dp_display_private, dp_display);
+	struct dp_display_private *dp = dev_get_dp_display_private(dev);
 
 	/* check for any test request issued by sink */
 	rc = dp_link_process_request(dp->link);
@@ -647,7 +618,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
 
 	DRM_DEBUG_DP("hpd_state=%d\n", state);
 	/* signal the disconnect event early to ensure proper teardown */
-	dp_display_handle_plugged_change(g_dp_display, false);
+	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);
@@ -842,9 +813,7 @@ static int dp_display_prepare(struct msm_dp *dp)
 static int dp_display_enable(struct dp_display_private *dp, u32 data)
 {
 	int rc = 0;
-	struct msm_dp *dp_display;
-
-	dp_display = g_dp_display;
+	struct msm_dp *dp_display = &dp->dp_display;
 
 	DRM_DEBUG_DP("sink_count=%d\n", dp->link->sink_count);
 	if (dp_display->power_on) {
@@ -880,9 +849,7 @@ static int dp_display_post_enable(struct msm_dp *dp_display)
 
 static int dp_display_disable(struct dp_display_private *dp, u32 data)
 {
-	struct msm_dp *dp_display;
-
-	dp_display = g_dp_display;
+	struct msm_dp *dp_display = &dp->dp_display;
 
 	if (!dp_display->power_on)
 		return 0;
@@ -1237,14 +1204,13 @@ static int dp_display_probe(struct platform_device *pdev)
 	}
 
 	mutex_init(&dp->event_mutex);
-	g_dp_display = &dp->dp_display;
 
 	/* Store DP audio handle inside DP display */
-	g_dp_display->dp_audio = dp->audio;
+	dp->dp_display.dp_audio = dp->audio;
 
 	init_completion(&dp->audio_comp);
 
-	platform_set_drvdata(pdev, g_dp_display);
+	platform_set_drvdata(pdev, &dp->dp_display);
 
 	rc = component_add(&pdev->dev, &dp_display_comp_ops);
 	if (rc) {
@@ -1257,10 +1223,7 @@ static int dp_display_probe(struct platform_device *pdev)
 
 static int dp_display_remove(struct platform_device *pdev)
 {
-	struct dp_display_private *dp;
-
-	dp = container_of(g_dp_display,
-			struct dp_display_private, dp_display);
+	struct dp_display_private *dp = dev_get_dp_display_private(&pdev->dev);
 
 	dp_display_deinit_sub_modules(dp);
 
@@ -1315,8 +1278,7 @@ static int dp_pm_resume(struct device *dev)
 	else
 		dp->dp_display.is_connected = false;
 
-	dp_display_handle_plugged_change(g_dp_display,
-				dp->dp_display.is_connected);
+	dp_display_handle_plugged_change(dp_display, dp->dp_display.is_connected);
 
 	DRM_DEBUG_DP("After, sink_count=%d is_connected=%d core_inited=%d power_on=%d\n",
 			dp->link->sink_count, dp->dp_display.is_connected,
-- 
2.29.2

[PATCH v4 2/7] drm/msm/dp: Modify prototype of encoder based API

[Bjorn Andersson]

Functions in the DisplayPort code that relates to individual instances
(encoders) are passed both the struct msm_dp and the struct drm_encoder.
But in a situation where multiple DP instances would exist this means
that the caller need to resolve which struct msm_dp relates to the
struct drm_encoder at hand.

Store a reference to the struct msm_dp associated with each
dpu_encoder_virt to allow the particular instance to be associate with
the encoder in the following patch.

Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v3:
- None

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 23 ++++++++++++---------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 0e9d3fa1544b..b7f33da2799c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -168,6 +168,7 @@ enum dpu_enc_rc_states {
  * @vsync_event_work:		worker to handle vsync event for autorefresh
  * @topology:                   topology of the display
  * @idle_timeout:		idle timeout duration in milliseconds
+ * @dp:				msm_dp pointer, for DP encoders
  */
 struct dpu_encoder_virt {
 	struct drm_encoder base;
@@ -206,6 +207,8 @@ struct dpu_encoder_virt {
 	struct msm_display_topology topology;
 
 	u32 idle_timeout;
+
+	struct msm_dp *dp;
 };
 
 #define to_dpu_encoder_virt(x) container_of(x, struct dpu_encoder_virt, base)
@@ -1000,8 +1003,8 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
 
 	trace_dpu_enc_mode_set(DRMID(drm_enc));
 
-	if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp)
-		msm_dp_display_mode_set(priv->dp, drm_enc, mode, adj_mode);
+	if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS)
+		msm_dp_display_mode_set(dpu_enc->dp, drm_enc, mode, adj_mode);
 
 	list_for_each_entry(conn_iter, connector_list, head)
 		if (conn_iter->encoder == drm_enc)
@@ -1182,9 +1185,8 @@ static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc)
 
 	_dpu_encoder_virt_enable_helper(drm_enc);
 
-	if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
-		ret = msm_dp_display_enable(priv->dp,
-						drm_enc);
+	if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
+		ret = msm_dp_display_enable(dpu_enc->dp, drm_enc);
 		if (ret) {
 			DPU_ERROR_ENC(dpu_enc, "dp display enable failed: %d\n",
 				ret);
@@ -1224,8 +1226,8 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
 	/* wait for idle */
 	dpu_encoder_wait_for_event(drm_enc, MSM_ENC_TX_COMPLETE);
 
-	if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
-		if (msm_dp_display_pre_disable(priv->dp, drm_enc))
+	if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
+		if (msm_dp_display_pre_disable(dpu_enc->dp, drm_enc))
 			DPU_ERROR_ENC(dpu_enc, "dp display push idle failed\n");
 	}
 
@@ -1253,8 +1255,8 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
 
 	DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
 
-	if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {
-		if (msm_dp_display_disable(priv->dp, drm_enc))
+	if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {
+		if (msm_dp_display_disable(dpu_enc->dp, drm_enc))
 			DPU_ERROR_ENC(dpu_enc, "dp display disable failed\n");
 	}
 
@@ -2170,7 +2172,8 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
 		timer_setup(&dpu_enc->vsync_event_timer,
 				dpu_encoder_vsync_event_handler,
 				0);
-
+	else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
+		dpu_enc->dp = priv->dp;
 
 	INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
 			dpu_encoder_off_work);
-- 
2.29.2

[PATCH v4 3/7] drm/msm/dp: Allow specifying connector_type per controller

[Bjorn Andersson]

As the following patches introduced support for multiple DP blocks in a
platform and some of those block might be eDP it becomes useful to be
able to specify the connector type per block.

Although there's only a single block at this point, the array of descs
and the search in dp_display_get_desc() are introduced here to simplify
the next patch, that does introduce support for multiple DP blocks.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v3:
- New patch
- Extended msm_dp_config with connector_type, wrapped in inner struct
- Refactored out of the next patch
- Pass the connector_type to drm_connector_init(), from yet another patch
- Dropped double newline and unnecessary {}

 drivers/gpu/drm/msm/dp/dp_display.c | 43 ++++++++++++++++++++++++++++-
 drivers/gpu/drm/msm/dp/dp_display.h |  1 +
 drivers/gpu/drm/msm/dp/dp_drm.c     |  2 +-
 3 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 5d3ee5ef07c2..eaf08f9e7d87 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -115,8 +115,25 @@ struct dp_display_private {
 	struct dp_audio *audio;
 };
 
+struct msm_dp_desc {
+	phys_addr_t io_start;
+	int connector_type;
+};
+
+struct msm_dp_config {
+	struct msm_dp_desc *descs;
+	size_t num_descs;
+};
+
+static const struct msm_dp_config sc7180_dp_cfg = {
+	.descs = (struct msm_dp_desc[]) {
+		{ .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
+	},
+	.num_descs = 1,
+};
+
 static const struct of_device_id dp_dt_match[] = {
-	{.compatible = "qcom,sc7180-dp"},
+	{ .compatible = "qcom,sc7180-dp", .data = &sc7180_dp_cfg },
 	{}
 };
 
@@ -1180,10 +1197,29 @@ int dp_display_request_irq(struct msm_dp *dp_display)
 	return 0;
 }
 
+static struct msm_dp_desc *dp_display_get_desc(struct platform_device *pdev)
+{
+	const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
+	struct resource *res;
+	int i;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return NULL;
+
+	for (i = 0; i < cfg->num_descs; i++)
+		if (cfg->descs[i].io_start == res->start)
+			return &cfg->descs[i];
+
+	dev_err(&pdev->dev, "unknown displayport instance\n");
+	return NULL;
+}
+
 static int dp_display_probe(struct platform_device *pdev)
 {
 	int rc = 0;
 	struct dp_display_private *dp;
+	struct msm_dp_desc *desc;
 
 	if (!pdev || !pdev->dev.of_node) {
 		DRM_ERROR("pdev not found\n");
@@ -1194,8 +1230,13 @@ static int dp_display_probe(struct platform_device *pdev)
 	if (!dp)
 		return -ENOMEM;
 
+	desc = dp_display_get_desc(pdev);
+	if (!desc)
+		return -EINVAL;
+
 	dp->pdev = pdev;
 	dp->name = "drm_dp";
+	dp->dp_display.connector_type = desc->connector_type;
 
 	rc = dp_init_sub_modules(dp);
 	if (rc) {
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
index 8b47cdabb67e..02999408c052 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -18,6 +18,7 @@ struct msm_dp {
 	bool is_connected;
 	bool audio_enabled;
 	bool power_on;
+	int connector_type;
 
 	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 764f4b81017e..f33e31523f56 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -147,7 +147,7 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display)
 
 	ret = drm_connector_init(dp_display->drm_dev, connector,
 			&dp_connector_funcs,
-			DRM_MODE_CONNECTOR_DisplayPort);
+			dp_display->connector_type);
 	if (ret)
 		return ERR_PTR(ret);
 
-- 
2.29.2

[PATCH v4 4/7] drm/msm/dp: Allow attaching a drm_panel

[Bjorn Andersson]

eDP panels might need some power sequencing and backlight management,
so make it possible to associate a drm_panel with an eDP instance and
prepare and enable the panel accordingly.

Now that we know which hardware instance is DP and which is eDP,
parser->parse() is passed the connector_type and the parser is limited
to only search for a panel in the eDP case.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v3:
- Previously posted separately, now added to series
- Make the search for a panel conditional on it being an eDP connector
- Move the search to of_graph port 1 (was 2 to distinguish from DP link to USB-C connector)

 drivers/gpu/drm/msm/dp/dp_display.c |  9 ++++++---
 drivers/gpu/drm/msm/dp/dp_display.h |  1 +
 drivers/gpu/drm/msm/dp/dp_drm.c     | 11 +++++++++++
 drivers/gpu/drm/msm/dp/dp_parser.c  | 30 ++++++++++++++++++++++++++++-
 drivers/gpu/drm/msm/dp/dp_parser.h  |  3 ++-
 5 files changed, 49 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index eaf08f9e7d87..bdaf227f05dc 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/drm_panel.h>
 
 #include "msm_drv.h"
 #include "msm_kms.h"
@@ -230,12 +231,14 @@ static int dp_display_bind(struct device *dev, struct device *master,
 	priv = drm->dev_private;
 	priv->dp = &(dp->dp_display);
 
-	rc = dp->parser->parse(dp->parser);
+	rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
 	if (rc) {
 		DRM_ERROR("device tree parsing failed\n");
 		goto end;
 	}
 
+	dp->dp_display.panel_bridge = dp->parser->panel_bridge;
+
 	dp->aux->drm_dev = drm;
 	rc = dp_aux_register(dp->aux);
 	if (rc) {
@@ -822,7 +825,7 @@ static int dp_display_set_mode(struct msm_dp *dp_display,
 	return 0;
 }
 
-static int dp_display_prepare(struct msm_dp *dp)
+static int dp_display_prepare(struct msm_dp *dp_display)
 {
 	return 0;
 }
@@ -896,7 +899,7 @@ static int dp_display_disable(struct dp_display_private *dp, u32 data)
 	return 0;
 }
 
-static int dp_display_unprepare(struct msm_dp *dp)
+static int dp_display_unprepare(struct msm_dp *dp_display)
 {
 	return 0;
 }
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
index 02999408c052..24aefca66029 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -15,6 +15,7 @@ struct msm_dp {
 	struct device *codec_dev;
 	struct drm_connector *connector;
 	struct drm_encoder *encoder;
+	struct drm_bridge *panel_bridge;
 	bool is_connected;
 	bool audio_enabled;
 	bool power_on;
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index f33e31523f56..76856c4ee1d6 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -5,6 +5,7 @@
 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_atomic.h>
+#include <drm/drm_bridge.h>
 #include <drm/drm_crtc.h>
 
 #include "msm_drv.h"
@@ -160,5 +161,15 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display)
 
 	drm_connector_attach_encoder(connector, dp_display->encoder);
 
+	if (dp_display->panel_bridge) {
+		ret = drm_bridge_attach(dp_display->encoder,
+					dp_display->panel_bridge, NULL,
+					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+		if (ret < 0) {
+			DRM_ERROR("failed to attach panel bridge: %d\n", ret);
+			return ERR_PTR(ret);
+		}
+	}
+
 	return connector;
 }
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
index 4d6e047f803d..eb6bbfbea484 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -6,6 +6,7 @@
 #include <linux/of_gpio.h>
 #include <linux/phy/phy.h>
 
+#include <drm/drm_of.h>
 #include <drm/drm_print.h>
 
 #include "dp_parser.h"
@@ -263,7 +264,28 @@ static int dp_parser_clock(struct dp_parser *parser)
 	return 0;
 }
 
-static int dp_parser_parse(struct dp_parser *parser)
+static int dp_parser_find_panel(struct dp_parser *parser)
+{
+	struct device *dev = &parser->pdev->dev;
+	struct drm_panel *panel;
+	int rc;
+
+	rc = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
+	if (rc) {
+		DRM_ERROR("failed to acquire DRM panel: %d\n", rc);
+		return rc;
+	}
+
+	parser->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
+	if (IS_ERR(parser->panel_bridge)) {
+		DRM_ERROR("failed to create panel bridge\n");
+		return PTR_ERR(parser->panel_bridge);
+	}
+
+	return 0;
+}
+
+static int dp_parser_parse(struct dp_parser *parser, int connector_type)
 {
 	int rc = 0;
 
@@ -284,6 +306,12 @@ static int dp_parser_parse(struct dp_parser *parser)
 	if (rc)
 		return rc;
 
+	if (connector_type == DRM_MODE_CONNECTOR_eDP) {
+		rc = dp_parser_find_panel(parser);
+		if (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 dac10923abde..3172da089421 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.h
+++ b/drivers/gpu/drm/msm/dp/dp_parser.h
@@ -123,8 +123,9 @@ struct dp_parser {
 	struct dp_display_data disp_data;
 	const struct dp_regulator_cfg *regulator_cfg;
 	u32 max_dp_lanes;
+	struct drm_bridge *panel_bridge;
 
-	int (*parse)(struct dp_parser *parser);
+	int (*parse)(struct dp_parser *parser, int connector_type);
 };
 
 /**
-- 
2.29.2

[PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers

[Bjorn Andersson]

Based on the removal of the g_dp_display and the movement of the
priv->dp lookup into the DP code it's now possible to have multiple
DP instances.

In line with the other controllers in the MSM driver, introduce a
per-compatible list of base addresses which is used to resolve the
"instance id" for the given DP controller. This instance id is used as
index in the priv->dp[] array.

Then extend the initialization code to initialize struct drm_encoder for
each of the registered priv->dp[] and update the logic for associating
each struct msm_dp with the struct dpu_encoder_virt.

Lastly, bump the number of struct msm_dp instances carries by priv->dp
to 3, the currently known maximum number of controllers found in a
Qualcomm SoC.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v3:
- Rebased ontop of previous patches

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       | 66 +++++++++++--------
 .../gpu/drm/msm/disp/msm_disp_snapshot_util.c |  8 ++-
 drivers/gpu/drm/msm/dp/dp_display.c           | 18 +++--
 drivers/gpu/drm/msm/msm_drv.h                 |  2 +-
 5 files changed, 59 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index b7f33da2799c..9cd9539a1504 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2173,7 +2173,7 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
 				dpu_encoder_vsync_event_handler,
 				0);
 	else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
-		dpu_enc->dp = priv->dp;
+		dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
 
 	INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
 			dpu_encoder_off_work);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index f655adbc2421..875b07e7183d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -188,6 +188,7 @@ static int dpu_kms_debugfs_init(struct msm_kms *kms, struct drm_minor *minor)
 	struct dentry *entry;
 	struct drm_device *dev;
 	struct msm_drm_private *priv;
+	int i;
 
 	if (!p)
 		return -EINVAL;
@@ -203,8 +204,10 @@ static int dpu_kms_debugfs_init(struct msm_kms *kms, struct drm_minor *minor)
 	dpu_debugfs_vbif_init(dpu_kms, entry);
 	dpu_debugfs_core_irq_init(dpu_kms, entry);
 
-	if (priv->dp)
-		msm_dp_debugfs_init(priv->dp, minor);
+	for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
+		if (priv->dp[i])
+			msm_dp_debugfs_init(priv->dp[i], minor);
+	}
 
 	return dpu_core_perf_debugfs_init(dpu_kms, entry);
 }
@@ -544,35 +547,42 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
 {
 	struct drm_encoder *encoder = NULL;
 	struct msm_display_info info;
-	int rc = 0;
+	int rc;
+	int i;
 
-	if (!priv->dp)
-		return rc;
+	for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
+		if (!priv->dp[i])
+			continue;
 
-	encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS);
-	if (IS_ERR(encoder)) {
-		DPU_ERROR("encoder init failed for dsi display\n");
-		return PTR_ERR(encoder);
-	}
+		encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS);
+		if (IS_ERR(encoder)) {
+			DPU_ERROR("encoder init failed for dsi display\n");
+			return PTR_ERR(encoder);
+		}
 
-	memset(&info, 0, sizeof(info));
-	rc = msm_dp_modeset_init(priv->dp, dev, encoder);
-	if (rc) {
-		DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
-		drm_encoder_cleanup(encoder);
-		return rc;
-	}
+		memset(&info, 0, sizeof(info));
+		rc = msm_dp_modeset_init(priv->dp[i], dev, encoder);
+		if (rc) {
+			DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
+			drm_encoder_cleanup(encoder);
+			return rc;
+		}
 
-	priv->encoders[priv->num_encoders++] = encoder;
+		priv->encoders[priv->num_encoders++] = encoder;
 
-	info.num_of_h_tiles = 1;
-	info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
-	info.intf_type = encoder->encoder_type;
-	rc = dpu_encoder_setup(dev, encoder, &info);
-	if (rc)
-		DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
-			  encoder->base.id, rc);
-	return rc;
+		info.num_of_h_tiles = 1;
+		info.h_tile_instance[0] = i;
+		info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
+		info.intf_type = encoder->encoder_type;
+		rc = dpu_encoder_setup(dev, encoder, &info);
+		if (rc) {
+			DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
+				  encoder->base.id, rc);
+			return rc;
+		}
+	}
+
+	return 0;
 }
 
 /**
@@ -792,6 +802,7 @@ static int dpu_irq_postinstall(struct msm_kms *kms)
 {
 	struct msm_drm_private *priv;
 	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
+	int i;
 
 	if (!dpu_kms || !dpu_kms->dev)
 		return -EINVAL;
@@ -800,7 +811,8 @@ static int dpu_irq_postinstall(struct msm_kms *kms)
 	if (!priv)
 		return -EINVAL;
 
-	msm_dp_irq_postinstall(priv->dp);
+	for (i = 0; i < ARRAY_SIZE(priv->dp); i++)
+		msm_dp_irq_postinstall(priv->dp[i]);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
index cabe15190ec1..2e1acb1bc390 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
@@ -126,8 +126,12 @@ void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state)
 	priv = drm_dev->dev_private;
 	kms = priv->kms;
 
-	if (priv->dp)
-		msm_dp_snapshot(disp_state, priv->dp);
+	for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
+		if (!priv->dp[i])
+			continue;
+
+		msm_dp_snapshot(disp_state, priv->dp[i]);
+	}
 
 	for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
 		if (!priv->dsi[i])
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index bdaf227f05dc..674cddfee5b0 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -79,6 +79,8 @@ struct dp_display_private {
 	char *name;
 	int irq;
 
+	unsigned int id;
+
 	/* state variables */
 	bool core_initialized;
 	bool hpd_irq_on;
@@ -229,7 +231,7 @@ static int dp_display_bind(struct device *dev, struct device *master,
 
 	dp->dp_display.drm_dev = drm;
 	priv = drm->dev_private;
-	priv->dp = &(dp->dp_display);
+	priv->dp[dp->id] = &(dp->dp_display);
 
 	rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
 	if (rc) {
@@ -269,7 +271,7 @@ static void dp_display_unbind(struct device *dev, struct device *master,
 
 	dp_power_client_deinit(dp->power);
 	dp_aux_unregister(dp->aux);
-	priv->dp = NULL;
+	priv->dp[dp->id] = NULL;
 }
 
 static const struct component_ops dp_display_comp_ops = {
@@ -1200,7 +1202,8 @@ int dp_display_request_irq(struct msm_dp *dp_display)
 	return 0;
 }
 
-static struct msm_dp_desc *dp_display_get_desc(struct platform_device *pdev)
+static struct msm_dp_desc *dp_display_get_desc(struct platform_device *pdev,
+					       unsigned int *id)
 {
 	const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
 	struct resource *res;
@@ -1210,9 +1213,12 @@ static struct msm_dp_desc *dp_display_get_desc(struct platform_device *pdev)
 	if (!res)
 		return NULL;
 
-	for (i = 0; i < cfg->num_descs; i++)
-		if (cfg->descs[i].io_start == res->start)
+	for (i = 0; i < cfg->num_descs; i++) {
+		if (cfg->descs[i].io_start == res->start) {
+			*id = i;
 			return &cfg->descs[i];
+		}
+	}
 
 	dev_err(&pdev->dev, "unknown displayport instance\n");
 	return NULL;
@@ -1233,7 +1239,7 @@ static int dp_display_probe(struct platform_device *pdev)
 	if (!dp)
 		return -ENOMEM;
 
-	desc = dp_display_get_desc(pdev);
+	desc = dp_display_get_desc(pdev, &dp->id);
 	if (!desc)
 		return -EINVAL;
 
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 8b005d1ac899..2e84dc30e12e 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -161,7 +161,7 @@ struct msm_drm_private {
 	/* DSI is shared by mdp4 and mdp5 */
 	struct msm_dsi *dsi[2];
 
-	struct msm_dp *dp;
+	struct msm_dp *dp[3];
 
 	/* when we have more than one 'msm_gpu' these need to be an array: */
 	struct msm_gpu *gpu;
-- 
2.29.2

[PATCH v4 6/7] dt-bindings: msm/dp: Add SC8180x compatibles

[Bjorn Andersson]

The Qualcomm SC8180x has 2 DP controllers and 1 eDP controller, add
compatibles for these to the msm/dp binding.

Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v3:
- None

 .../devicetree/bindings/display/msm/dp-controller.yaml          | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
index 6bb424c21340..63e585f48789 100644
--- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
+++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
@@ -17,6 +17,8 @@ properties:
   compatible:
     enum:
       - qcom,sc7180-dp
+      - qcom,sc8180x-dp
+      - qcom,sc8180x-edp
 
   reg:
     items:
-- 
2.29.2

[PATCH v4 7/7] drm/msm/dp: Add sc8180x DP controllers

[Bjorn Andersson]

The sc8180x has 2 DP and 1 eDP controllers, add support for these to the
DP driver.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v3:
- Rebased upon previous patches in series

 drivers/gpu/drm/msm/dp/dp_display.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 674cddfee5b0..29c2c1c52ddb 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -135,8 +135,19 @@ static const struct msm_dp_config sc7180_dp_cfg = {
 	.num_descs = 1,
 };
 
+static const struct msm_dp_config sc8180x_dp_cfg = {
+	.descs = (struct msm_dp_desc[]) {
+		{ .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
+		{ .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
+		{ .io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP },
+	},
+	.num_descs = 3,
+};
+
 static const struct of_device_id dp_dt_match[] = {
 	{ .compatible = "qcom,sc7180-dp", .data = &sc7180_dp_cfg },
+	{ .compatible = "qcom,sc8180x-dp", .data = &sc8180x_dp_cfg },
+	{ .compatible = "qcom,sc8180x-edp", .data = &sc8180x_dp_cfg },
 	{}
 };
 
-- 
2.29.2

Re: [PATCH v4 3/7] drm/msm/dp: Allow specifying connector_type per controller

[Stephen Boyd]

Quoting Bjorn Andersson (2021-10-05 16:13:19)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 5d3ee5ef07c2..eaf08f9e7d87 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -115,8 +115,25 @@ struct dp_display_private {
>         struct dp_audio *audio;
>  };
>
> +struct msm_dp_desc {
> +       phys_addr_t io_start;
> +       int connector_type;
> +};
> +
> +struct msm_dp_config {
> +       struct msm_dp_desc *descs;

const?

> +       size_t num_descs;
> +};
> +
> +static const struct msm_dp_config sc7180_dp_cfg = {
> +       .descs = (struct msm_dp_desc[]) {

const?

> +               { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> +       },
> +       .num_descs = 1,
> +};
> +
>  static const struct of_device_id dp_dt_match[] = {
> -       {.compatible = "qcom,sc7180-dp"},
> +       { .compatible = "qcom,sc7180-dp", .data = &sc7180_dp_cfg },
>         {}
>  };
>
> @@ -1180,10 +1197,29 @@ int dp_display_request_irq(struct msm_dp *dp_display)
>         return 0;
>  }
>
> +static struct msm_dp_desc *dp_display_get_desc(struct platform_device *pdev)

const msm_dp_desc?

> +{
> +       const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
> +       struct resource *res;
> +       int i;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!res)
> +               return NULL;
> +
> +       for (i = 0; i < cfg->num_descs; i++)
> +               if (cfg->descs[i].io_start == res->start)
> +                       return &cfg->descs[i];
> +
> +       dev_err(&pdev->dev, "unknown displayport instance\n");
> +       return NULL;
> +}
> +
>  static int dp_display_probe(struct platform_device *pdev)
>  {
>         int rc = 0;
>         struct dp_display_private *dp;
> +       struct msm_dp_desc *desc;

const?

>
>         if (!pdev || !pdev->dev.of_node) {
>                 DRM_ERROR("pdev not found\n");
> @@ -1194,8 +1230,13 @@ static int dp_display_probe(struct platform_device *pdev)
>         if (!dp)
>                 return -ENOMEM;
>
> +       desc = dp_display_get_desc(pdev);
> +       if (!desc)
> +               return -EINVAL;
> +
>         dp->pdev = pdev;
>         dp->name = "drm_dp";
> +       dp->dp_display.connector_type = desc->connector_type;
>
>         rc = dp_init_sub_modules(dp);
>         if (rc) {

Re: [PATCH v4 3/7] drm/msm/dp: Allow specifying connector_type per controller

[Stephen Boyd]

Quoting Bjorn Andersson (2021-10-05 16:13:19)
> As the following patches introduced support for multiple DP blocks in a
> platform and some of those block might be eDP it becomes useful to be
> able to specify the connector type per block.
>
> Although there's only a single block at this point, the array of descs
> and the search in dp_display_get_desc() are introduced here to simplify
> the next patch, that does introduce support for multiple DP blocks.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>
> Changes since v3:
> - New patch
> - Extended msm_dp_config with connector_type, wrapped in inner struct
> - Refactored out of the next patch
> - Pass the connector_type to drm_connector_init(), from yet another patch
> - Dropped double newline and unnecessary {}

BTW, I see that we check for the connector type in debugfs.

$ git grep DRM_MODE_CONNECTOR_DisplayPort -- drivers/gpu/drm/msm/dp/
drivers/gpu/drm/msm/dp/dp_debug.c:
DRM_MODE_CONNECTOR_DisplayPort)
drivers/gpu/drm/msm/dp/dp_debug.c:
DRM_MODE_CONNECTOR_DisplayPort)
drivers/gpu/drm/msm/dp/dp_debug.c:
DRM_MODE_CONNECTOR_DisplayPort)
drivers/gpu/drm/msm/dp/dp_debug.c:
DRM_MODE_CONNECTOR_DisplayPort)

So do those need to be updated to handle either connector type?

Re: [PATCH v4 3/7] drm/msm/dp: Allow specifying connector_type per controller

[Stephen Boyd]

Quoting Bjorn Andersson (2021-10-05 16:13:19)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 5d3ee5ef07c2..eaf08f9e7d87 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -115,8 +115,25 @@ struct dp_display_private {
>         struct dp_audio *audio;
>  };
>
> +struct msm_dp_desc {
> +       phys_addr_t io_start;
> +       int connector_type;

unsigned?

> +};
> +

Re: [Freedreno] [PATCH v4 4/7] drm/msm/dp: Allow attaching a drm_panel

[abhinavk]

Hi Bjorn

On 2021-10-05 16:13, Bjorn Andersson wrote:
> eDP panels might need some power sequencing and backlight management,
> so make it possible to associate a drm_panel with an eDP instance and
> prepare and enable the panel accordingly.
> 
> Now that we know which hardware instance is DP and which is eDP,
> parser->parse() is passed the connector_type and the parser is limited
> to only search for a panel in the eDP case.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v3:
> - Previously posted separately, now added to series
> - Make the search for a panel conditional on it being an eDP connector
> - Move the search to of_graph port 1 (was 2 to distinguish from DP
> link to USB-C connector)
> 
>  drivers/gpu/drm/msm/dp/dp_display.c |  9 ++++++---
>  drivers/gpu/drm/msm/dp/dp_display.h |  1 +
>  drivers/gpu/drm/msm/dp/dp_drm.c     | 11 +++++++++++
>  drivers/gpu/drm/msm/dp/dp_parser.c  | 30 ++++++++++++++++++++++++++++-
>  drivers/gpu/drm/msm/dp/dp_parser.h  |  3 ++-
>  5 files changed, 49 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index eaf08f9e7d87..bdaf227f05dc 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/drm_panel.h>
> 
>  #include "msm_drv.h"
>  #include "msm_kms.h"
> @@ -230,12 +231,14 @@ static int dp_display_bind(struct device *dev,
> struct device *master,
>  	priv = drm->dev_private;
>  	priv->dp = &(dp->dp_display);
> 
> -	rc = dp->parser->parse(dp->parser);
> +	rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
>  	if (rc) {
>  		DRM_ERROR("device tree parsing failed\n");
>  		goto end;
>  	}
> 
> +	dp->dp_display.panel_bridge = dp->parser->panel_bridge;
> +
>  	dp->aux->drm_dev = drm;
>  	rc = dp_aux_register(dp->aux);
>  	if (rc) {
> @@ -822,7 +825,7 @@ static int dp_display_set_mode(struct msm_dp 
> *dp_display,
>  	return 0;
>  }
> 
> -static int dp_display_prepare(struct msm_dp *dp)
> +static int dp_display_prepare(struct msm_dp *dp_display)
>  {
>  	return 0;
>  }
> @@ -896,7 +899,7 @@ static int dp_display_disable(struct
> dp_display_private *dp, u32 data)
>  	return 0;
>  }
> 
> -static int dp_display_unprepare(struct msm_dp *dp)
> +static int dp_display_unprepare(struct msm_dp *dp_display)
>  {
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h
> b/drivers/gpu/drm/msm/dp/dp_display.h
> index 02999408c052..24aefca66029 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.h
> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> @@ -15,6 +15,7 @@ struct msm_dp {
>  	struct device *codec_dev;
>  	struct drm_connector *connector;
>  	struct drm_encoder *encoder;
> +	struct drm_bridge *panel_bridge;
>  	bool is_connected;
>  	bool audio_enabled;
>  	bool power_on;
> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c 
> b/drivers/gpu/drm/msm/dp/dp_drm.c
> index f33e31523f56..76856c4ee1d6 100644
> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> @@ -5,6 +5,7 @@
> 
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_atomic.h>
> +#include <drm/drm_bridge.h>
>  #include <drm/drm_crtc.h>
> 
>  #include "msm_drv.h"
> @@ -160,5 +161,15 @@ struct drm_connector
> *dp_drm_connector_init(struct msm_dp *dp_display)
> 
>  	drm_connector_attach_encoder(connector, dp_display->encoder);
> 
> +	if (dp_display->panel_bridge) {
> +		ret = drm_bridge_attach(dp_display->encoder,
> +					dp_display->panel_bridge, NULL,
> +					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> +		if (ret < 0) {
> +			DRM_ERROR("failed to attach panel bridge: %d\n", ret);
> +			return ERR_PTR(ret);
> +		}
> +	}
> +
>  	return connector;
>  }
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
> b/drivers/gpu/drm/msm/dp/dp_parser.c
> index 4d6e047f803d..eb6bbfbea484 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> @@ -6,6 +6,7 @@
>  #include <linux/of_gpio.h>
>  #include <linux/phy/phy.h>
> 
> +#include <drm/drm_of.h>
>  #include <drm/drm_print.h>
> 
>  #include "dp_parser.h"
> @@ -263,7 +264,28 @@ static int dp_parser_clock(struct dp_parser 
> *parser)
>  	return 0;
>  }
> 
> -static int dp_parser_parse(struct dp_parser *parser)
> +static int dp_parser_find_panel(struct dp_parser *parser)
> +{
> +	struct device *dev = &parser->pdev->dev;
> +	struct drm_panel *panel;
> +	int rc;
> +
> +	rc = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
> +	if (rc) {
> +		DRM_ERROR("failed to acquire DRM panel: %d\n", rc);
> +		return rc;
> +	}
> +
> +	parser->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> +	if (IS_ERR(parser->panel_bridge)) {
> +		DRM_ERROR("failed to create panel bridge\n");
> +		return PTR_ERR(parser->panel_bridge);
> +	}

When we add a bridge using devm_drm_panel_bridge_add(), it will register 
with default bridge functions which is fine
because we need the panel power to be controlled here.


140 static const struct drm_bridge_funcs panel_bridge_bridge_funcs = {
141 	.attach = panel_bridge_attach,
142 	.detach = panel_bridge_detach,
143 	.pre_enable = panel_bridge_pre_enable,
144 	.enable = panel_bridge_enable,
145 	.disable = panel_bridge_disable,
146 	.post_disable = panel_bridge_post_disable,
147 	.get_modes = panel_bridge_get_modes,

But what about the EDID related things, the DP/eDP driver already reads 
the EDID and gets the modes so we need to skip
that in this case as otherwise it will end up calling the 
panel_get_modes in the eDP panel which will be redundant.

Let me know if I am missing something in this proposal.

> +
> +	return 0;
> +}
> +
> +static int dp_parser_parse(struct dp_parser *parser, int 
> connector_type)
>  {
>  	int rc = 0;
> 
> @@ -284,6 +306,12 @@ static int dp_parser_parse(struct dp_parser 
> *parser)
>  	if (rc)
>  		return rc;
> 
> +	if (connector_type == DRM_MODE_CONNECTOR_eDP) {
> +		rc = dp_parser_find_panel(parser);
> +		if (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 dac10923abde..3172da089421 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> @@ -123,8 +123,9 @@ struct dp_parser {
>  	struct dp_display_data disp_data;
>  	const struct dp_regulator_cfg *regulator_cfg;
>  	u32 max_dp_lanes;
> +	struct drm_bridge *panel_bridge;
> 
> -	int (*parse)(struct dp_parser *parser);
> +	int (*parse)(struct dp_parser *parser, int connector_type);
>  };
> 
>  /**

Re: [PATCH v4 7/7] drm/msm/dp: Add sc8180x DP controllers

[Stephen Boyd]

Quoting Bjorn Andersson (2021-10-05 16:13:23)
> The sc8180x has 2 DP and 1 eDP controllers, add support for these to the
> DP driver.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>
> Changes since v3:
> - Rebased upon previous patches in series
>
>  drivers/gpu/drm/msm/dp/dp_display.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 674cddfee5b0..29c2c1c52ddb 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -135,8 +135,19 @@ static const struct msm_dp_config sc7180_dp_cfg = {
>         .num_descs = 1,
>  };
>
> +static const struct msm_dp_config sc8180x_dp_cfg = {
> +       .descs = (struct msm_dp_desc[]) {

const?

> +               { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> +               { .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
> +               { .io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP },
> +       },
> +       .num_descs = 3,
> +};
> +
>  static const struct of_device_id dp_dt_match[] = {
>         { .compatible = "qcom,sc7180-dp", .data = &sc7180_dp_cfg },
> +       { .compatible = "qcom,sc8180x-dp", .data = &sc8180x_dp_cfg },
> +       { .compatible = "qcom,sc8180x-edp", .data = &sc8180x_dp_cfg },

Otherwise

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers

[Stephen Boyd]

Quoting Bjorn Andersson (2021-10-05 16:13:21)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index bdaf227f05dc..674cddfee5b0 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -79,6 +79,8 @@ struct dp_display_private {
>         char *name;
>         int irq;
>
> +       unsigned int id;
> +
>         /* state variables */
>         bool core_initialized;
>         bool hpd_irq_on;
> @@ -229,7 +231,7 @@ static int dp_display_bind(struct device *dev, struct device *master,
>
>         dp->dp_display.drm_dev = drm;
>         priv = drm->dev_private;
> -       priv->dp = &(dp->dp_display);
> +       priv->dp[dp->id] = &(dp->dp_display);

Can we drop the extra parenthesis?

>
>         rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
>         if (rc) {
> @@ -269,7 +271,7 @@ static void dp_display_unbind(struct device *dev, struct device *master,
>
>         dp_power_client_deinit(dp->power);
>         dp_aux_unregister(dp->aux);
> -       priv->dp = NULL;
> +       priv->dp[dp->id] = NULL;
>  }
>
>  static const struct component_ops dp_display_comp_ops = {
> @@ -1233,7 +1239,7 @@ static int dp_display_probe(struct platform_device *pdev)
>         if (!dp)
>                 return -ENOMEM;
>
> -       desc = dp_display_get_desc(pdev);
> +       desc = dp_display_get_desc(pdev, &dp->id);

I'm sad that dp->id has to match the number in the SoC specific
dpu_intf_cfg array in drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
still. Is there any way we can avoid that? Also, notice how those arrays
already have INTF_DP macros, which makes me think that it may be better
to connect this to those arrays instead of making an msm_dp_desc
structure and then make sure the 'type' member matches a connector
type number. Otherwise this code is super fragile.

>         if (!desc)
>                 return -EINVAL;
>

Re: [PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers

[Bjorn Andersson]

On Tue 05 Oct 17:43 PDT 2021, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2021-10-05 16:13:21)
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > index bdaf227f05dc..674cddfee5b0 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -79,6 +79,8 @@ struct dp_display_private {
> >         char *name;
> >         int irq;
> >
> > +       unsigned int id;
> > +
> >         /* state variables */
> >         bool core_initialized;
> >         bool hpd_irq_on;
> > @@ -229,7 +231,7 @@ static int dp_display_bind(struct device *dev, struct device *master,
> >
> >         dp->dp_display.drm_dev = drm;
> >         priv = drm->dev_private;
> > -       priv->dp = &(dp->dp_display);
> > +       priv->dp[dp->id] = &(dp->dp_display);
> 
> Can we drop the extra parenthesis?
> 

Definitely.

> >
> >         rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
> >         if (rc) {
> > @@ -269,7 +271,7 @@ static void dp_display_unbind(struct device *dev, struct device *master,
> >
> >         dp_power_client_deinit(dp->power);
> >         dp_aux_unregister(dp->aux);
> > -       priv->dp = NULL;
> > +       priv->dp[dp->id] = NULL;
> >  }
> >
> >  static const struct component_ops dp_display_comp_ops = {
> > @@ -1233,7 +1239,7 @@ static int dp_display_probe(struct platform_device *pdev)
> >         if (!dp)
> >                 return -ENOMEM;
> >
> > -       desc = dp_display_get_desc(pdev);
> > +       desc = dp_display_get_desc(pdev, &dp->id);
> 
> I'm sad that dp->id has to match the number in the SoC specific
> dpu_intf_cfg array in drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> still. Is there any way we can avoid that? Also, notice how those arrays
> already have INTF_DP macros, which makes me think that it may be better
> to connect this to those arrays instead of making an msm_dp_desc
> structure and then make sure the 'type' member matches a connector
> type number. Otherwise this code is super fragile.
> 

I'm afraid I don't understand what you're proposing. Or which part you
consider fragile, the indices of the INTF_DP instances aren't going to
move around...

I have N instances of the DP driver that I need to match to N entries
from the platform specific intf array, I need some stable reference
between them. When I started this journey I figured I could rely on the
of_graph between the DPU and the interface controllers, but the values
used there today are just bogus, so that was a no go.

We can use whatever, as long as _dpu_kms_initialize_displayport() can
come up with an identifier to put in h_tile_instance[0] so that
dpu_encoder_setup_display() can find the relevant INTF.

Regards,
Bjorn

> >         if (!desc)
> >                 return -EINVAL;
> >

Re: [PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers

[Stephen Boyd]

Quoting Bjorn Andersson (2021-10-05 18:43:16)
> On Tue 05 Oct 17:43 PDT 2021, Stephen Boyd wrote:
>
> > Quoting Bjorn Andersson (2021-10-05 16:13:21)
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > > index bdaf227f05dc..674cddfee5b0 100644
> > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > @@ -1233,7 +1239,7 @@ static int dp_display_probe(struct platform_device *pdev)
> > >         if (!dp)
> > >                 return -ENOMEM;
> > >
> > > -       desc = dp_display_get_desc(pdev);
> > > +       desc = dp_display_get_desc(pdev, &dp->id);
> >
> > I'm sad that dp->id has to match the number in the SoC specific
> > dpu_intf_cfg array in drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > still. Is there any way we can avoid that? Also, notice how those arrays
> > already have INTF_DP macros, which makes me think that it may be better
> > to connect this to those arrays instead of making an msm_dp_desc
> > structure and then make sure the 'type' member matches a connector
> > type number. Otherwise this code is super fragile.
> >
>
> I'm afraid I don't understand what you're proposing. Or which part you
> consider fragile, the indices of the INTF_DP instances aren't going to
> move around...
>
> I have N instances of the DP driver that I need to match to N entries
> from the platform specific intf array, I need some stable reference
> between them. When I started this journey I figured I could rely on the
> of_graph between the DPU and the interface controllers, but the values
> used there today are just bogus, so that was a no go.
>
> We can use whatever, as long as _dpu_kms_initialize_displayport() can
> come up with an identifier to put in h_tile_instance[0] so that
> dpu_encoder_setup_display() can find the relevant INTF.
>

To make it more concrete we can look at sc7180

static const struct dpu_intf_cfg sc7180_intf[] = {
        INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24,
INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
                                                     ^
                                                     |

intf0 is irrelevant. Also the address is irrelevant. But here we have a
zero, the number after INTF_DP, and that is very relevant. That number
needs to match the dp->id. Somewhere we have a match between
controller_id and dp->id in the code.

Re: [Freedreno] [PATCH v4 4/7] drm/msm/dp: Allow attaching a drm_panel

[Bjorn Andersson]

On Tue 05 Oct 17:35 PDT 2021, abhinavk@codeaurora.org wrote:

> Hi Bjorn
> 
> On 2021-10-05 16:13, Bjorn Andersson wrote:
> > eDP panels might need some power sequencing and backlight management,
> > so make it possible to associate a drm_panel with an eDP instance and
> > prepare and enable the panel accordingly.
> > 
> > Now that we know which hardware instance is DP and which is eDP,
> > parser->parse() is passed the connector_type and the parser is limited
> > to only search for a panel in the eDP case.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> > 
> > Changes since v3:
> > - Previously posted separately, now added to series
> > - Make the search for a panel conditional on it being an eDP connector
> > - Move the search to of_graph port 1 (was 2 to distinguish from DP
> > link to USB-C connector)
> > 
> >  drivers/gpu/drm/msm/dp/dp_display.c |  9 ++++++---
> >  drivers/gpu/drm/msm/dp/dp_display.h |  1 +
> >  drivers/gpu/drm/msm/dp/dp_drm.c     | 11 +++++++++++
> >  drivers/gpu/drm/msm/dp/dp_parser.c  | 30 ++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/msm/dp/dp_parser.h  |  3 ++-
> >  5 files changed, 49 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index eaf08f9e7d87..bdaf227f05dc 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/drm_panel.h>
> > 
> >  #include "msm_drv.h"
> >  #include "msm_kms.h"
> > @@ -230,12 +231,14 @@ static int dp_display_bind(struct device *dev,
> > struct device *master,
> >  	priv = drm->dev_private;
> >  	priv->dp = &(dp->dp_display);
> > 
> > -	rc = dp->parser->parse(dp->parser);
> > +	rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
> >  	if (rc) {
> >  		DRM_ERROR("device tree parsing failed\n");
> >  		goto end;
> >  	}
> > 
> > +	dp->dp_display.panel_bridge = dp->parser->panel_bridge;
> > +
> >  	dp->aux->drm_dev = drm;
> >  	rc = dp_aux_register(dp->aux);
> >  	if (rc) {
> > @@ -822,7 +825,7 @@ static int dp_display_set_mode(struct msm_dp
> > *dp_display,
> >  	return 0;
> >  }
> > 
> > -static int dp_display_prepare(struct msm_dp *dp)
> > +static int dp_display_prepare(struct msm_dp *dp_display)
> >  {
> >  	return 0;
> >  }
> > @@ -896,7 +899,7 @@ static int dp_display_disable(struct
> > dp_display_private *dp, u32 data)
> >  	return 0;
> >  }
> > 
> > -static int dp_display_unprepare(struct msm_dp *dp)
> > +static int dp_display_unprepare(struct msm_dp *dp_display)
> >  {
> >  	return 0;
> >  }
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.h
> > b/drivers/gpu/drm/msm/dp/dp_display.h
> > index 02999408c052..24aefca66029 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.h
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> > @@ -15,6 +15,7 @@ struct msm_dp {
> >  	struct device *codec_dev;
> >  	struct drm_connector *connector;
> >  	struct drm_encoder *encoder;
> > +	struct drm_bridge *panel_bridge;
> >  	bool is_connected;
> >  	bool audio_enabled;
> >  	bool power_on;
> > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c
> > b/drivers/gpu/drm/msm/dp/dp_drm.c
> > index f33e31523f56..76856c4ee1d6 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> > @@ -5,6 +5,7 @@
> > 
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_atomic.h>
> > +#include <drm/drm_bridge.h>
> >  #include <drm/drm_crtc.h>
> > 
> >  #include "msm_drv.h"
> > @@ -160,5 +161,15 @@ struct drm_connector
> > *dp_drm_connector_init(struct msm_dp *dp_display)
> > 
> >  	drm_connector_attach_encoder(connector, dp_display->encoder);
> > 
> > +	if (dp_display->panel_bridge) {
> > +		ret = drm_bridge_attach(dp_display->encoder,
> > +					dp_display->panel_bridge, NULL,
> > +					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> > +		if (ret < 0) {
> > +			DRM_ERROR("failed to attach panel bridge: %d\n", ret);
> > +			return ERR_PTR(ret);
> > +		}
> > +	}
> > +
> >  	return connector;
> >  }
> > diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
> > b/drivers/gpu/drm/msm/dp/dp_parser.c
> > index 4d6e047f803d..eb6bbfbea484 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> > @@ -6,6 +6,7 @@
> >  #include <linux/of_gpio.h>
> >  #include <linux/phy/phy.h>
> > 
> > +#include <drm/drm_of.h>
> >  #include <drm/drm_print.h>
> > 
> >  #include "dp_parser.h"
> > @@ -263,7 +264,28 @@ static int dp_parser_clock(struct dp_parser
> > *parser)
> >  	return 0;
> >  }
> > 
> > -static int dp_parser_parse(struct dp_parser *parser)
> > +static int dp_parser_find_panel(struct dp_parser *parser)
> > +{
> > +	struct device *dev = &parser->pdev->dev;
> > +	struct drm_panel *panel;
> > +	int rc;
> > +
> > +	rc = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
> > +	if (rc) {
> > +		DRM_ERROR("failed to acquire DRM panel: %d\n", rc);
> > +		return rc;
> > +	}
> > +
> > +	parser->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> > +	if (IS_ERR(parser->panel_bridge)) {
> > +		DRM_ERROR("failed to create panel bridge\n");
> > +		return PTR_ERR(parser->panel_bridge);
> > +	}
> 
> When we add a bridge using devm_drm_panel_bridge_add(), it will register
> with default bridge functions which is fine
> because we need the panel power to be controlled here.
> 
> 
> 140 static const struct drm_bridge_funcs panel_bridge_bridge_funcs = {
> 141 	.attach = panel_bridge_attach,
> 142 	.detach = panel_bridge_detach,
> 143 	.pre_enable = panel_bridge_pre_enable,
> 144 	.enable = panel_bridge_enable,
> 145 	.disable = panel_bridge_disable,
> 146 	.post_disable = panel_bridge_post_disable,
> 147 	.get_modes = panel_bridge_get_modes,
> 
> But what about the EDID related things, the DP/eDP driver already reads the
> EDID and gets the modes so we need to skip
> that in this case as otherwise it will end up calling the panel_get_modes in
> the eDP panel which will be redundant.
> 
> Let me know if I am missing something in this proposal.
> 

I'm slightly lost when it comes to these types, so can you please help
me understand why the panel_bridge_bridge_funcs->get_modes() would be
invoked with the proposed patches.

Because adding a log print in panel_bridge_get_modes() shows that it's
not invoked.


So I think we have the opposite problem, rather than having two
redundant get_modes we only support relying on the EDID information
directly in the DP driver, at this point.

So unless I'm missing something critical, this works and provides panel
and backlight controls and the additional work to move the EDID read out
to the panel driver would better be done after this initial support
lands?

Regards,
Bjorn

> > +
> > +	return 0;
> > +}
> > +
> > +static int dp_parser_parse(struct dp_parser *parser, int
> > connector_type)
> >  {
> >  	int rc = 0;
> > 
> > @@ -284,6 +306,12 @@ static int dp_parser_parse(struct dp_parser
> > *parser)
> >  	if (rc)
> >  		return rc;
> > 
> > +	if (connector_type == DRM_MODE_CONNECTOR_eDP) {
> > +		rc = dp_parser_find_panel(parser);
> > +		if (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 dac10923abde..3172da089421 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> > +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> > @@ -123,8 +123,9 @@ struct dp_parser {
> >  	struct dp_display_data disp_data;
> >  	const struct dp_regulator_cfg *regulator_cfg;
> >  	u32 max_dp_lanes;
> > +	struct drm_bridge *panel_bridge;
> > 
> > -	int (*parse)(struct dp_parser *parser);
> > +	int (*parse)(struct dp_parser *parser, int connector_type);
> >  };
> > 
> >  /**

Re: [PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers

[Bjorn Andersson]

On Tue 05 Oct 19:06 PDT 2021, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2021-10-05 18:43:16)
> > On Tue 05 Oct 17:43 PDT 2021, Stephen Boyd wrote:
> >
> > > Quoting Bjorn Andersson (2021-10-05 16:13:21)
> > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > index bdaf227f05dc..674cddfee5b0 100644
> > > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > @@ -1233,7 +1239,7 @@ static int dp_display_probe(struct platform_device *pdev)
> > > >         if (!dp)
> > > >                 return -ENOMEM;
> > > >
> > > > -       desc = dp_display_get_desc(pdev);
> > > > +       desc = dp_display_get_desc(pdev, &dp->id);
> > >
> > > I'm sad that dp->id has to match the number in the SoC specific
> > > dpu_intf_cfg array in drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > > still. Is there any way we can avoid that? Also, notice how those arrays
> > > already have INTF_DP macros, which makes me think that it may be better
> > > to connect this to those arrays instead of making an msm_dp_desc
> > > structure and then make sure the 'type' member matches a connector
> > > type number. Otherwise this code is super fragile.
> > >
> >
> > I'm afraid I don't understand what you're proposing. Or which part you
> > consider fragile, the indices of the INTF_DP instances aren't going to
> > move around...
> >
> > I have N instances of the DP driver that I need to match to N entries
> > from the platform specific intf array, I need some stable reference
> > between them. When I started this journey I figured I could rely on the
> > of_graph between the DPU and the interface controllers, but the values
> > used there today are just bogus, so that was a no go.
> >
> > We can use whatever, as long as _dpu_kms_initialize_displayport() can
> > come up with an identifier to put in h_tile_instance[0] so that
> > dpu_encoder_setup_display() can find the relevant INTF.
> >
> 
> To make it more concrete we can look at sc7180
> 
> static const struct dpu_intf_cfg sc7180_intf[] = {
>         INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24,
> INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
>                                                      ^
>                                                      |
> 
> intf0 is irrelevant. Also the address is irrelevant. But here we have a
> zero, the number after INTF_DP, and that is very relevant. That number
> needs to match the dp->id. Somewhere we have a match between
> controller_id and dp->id in the code.

That number (the 0, not INTF_0) is what the code matches against dp->id
in _dpu_kms_initialize_displayport(), in order to figure out that this
is INTF_0 in dpu_encoder_setup_display().

I.e. look at the sc8180x patch:

INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
INTF_BLK("intf_1", INTF_1, 0x6A800, INTF_DSI, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
INTF_BLK("intf_2", INTF_2, 0x6B000, INTF_DSI, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 28, 29),
/* INTF_3 is for MST, wired to INTF_DP 0 and 1, use dummy index until this is supported */
INTF_BLK("intf_3", INTF_3, 0x6B800, INTF_DP, 999, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 30, 31),
INTF_BLK("intf_4", INTF_4, 0x6C000, INTF_DP, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 20, 21),
INTF_BLK("intf_5", INTF_5, 0x6C800, INTF_DP, 2, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 22, 23),

Where the DP driver defines the 3 controllers with dp->id of 0, 1 and 2,
which the DPU code will match against to INTF_0, INTF_4 and INTF_5.

Regards,
Bjorn

Re: [Freedreno] [PATCH v4 4/7] drm/msm/dp: Allow attaching a drm_panel

[abhinavk]

On 2021-10-05 19:09, Bjorn Andersson wrote:
> On Tue 05 Oct 17:35 PDT 2021, abhinavk@codeaurora.org wrote:
> 
>> Hi Bjorn
>> 
>> On 2021-10-05 16:13, Bjorn Andersson wrote:
>> > eDP panels might need some power sequencing and backlight management,
>> > so make it possible to associate a drm_panel with an eDP instance and
>> > prepare and enable the panel accordingly.
>> >
>> > Now that we know which hardware instance is DP and which is eDP,
>> > parser->parse() is passed the connector_type and the parser is limited
>> > to only search for a panel in the eDP case.
>> >
>> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>> > ---
>> >
>> > Changes since v3:
>> > - Previously posted separately, now added to series
>> > - Make the search for a panel conditional on it being an eDP connector
>> > - Move the search to of_graph port 1 (was 2 to distinguish from DP
>> > link to USB-C connector)
>> >
>> >  drivers/gpu/drm/msm/dp/dp_display.c |  9 ++++++---
>> >  drivers/gpu/drm/msm/dp/dp_display.h |  1 +
>> >  drivers/gpu/drm/msm/dp/dp_drm.c     | 11 +++++++++++
>> >  drivers/gpu/drm/msm/dp/dp_parser.c  | 30 ++++++++++++++++++++++++++++-
>> >  drivers/gpu/drm/msm/dp/dp_parser.h  |  3 ++-
>> >  5 files changed, 49 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> > b/drivers/gpu/drm/msm/dp/dp_display.c
>> > index eaf08f9e7d87..bdaf227f05dc 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/drm_panel.h>
>> >
>> >  #include "msm_drv.h"
>> >  #include "msm_kms.h"
>> > @@ -230,12 +231,14 @@ static int dp_display_bind(struct device *dev,
>> > struct device *master,
>> >  	priv = drm->dev_private;
>> >  	priv->dp = &(dp->dp_display);
>> >
>> > -	rc = dp->parser->parse(dp->parser);
>> > +	rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type);
>> >  	if (rc) {
>> >  		DRM_ERROR("device tree parsing failed\n");
>> >  		goto end;
>> >  	}
>> >
>> > +	dp->dp_display.panel_bridge = dp->parser->panel_bridge;
>> > +
>> >  	dp->aux->drm_dev = drm;
>> >  	rc = dp_aux_register(dp->aux);
>> >  	if (rc) {
>> > @@ -822,7 +825,7 @@ static int dp_display_set_mode(struct msm_dp
>> > *dp_display,
>> >  	return 0;
>> >  }
>> >
>> > -static int dp_display_prepare(struct msm_dp *dp)
>> > +static int dp_display_prepare(struct msm_dp *dp_display)
>> >  {
>> >  	return 0;
>> >  }
>> > @@ -896,7 +899,7 @@ static int dp_display_disable(struct
>> > dp_display_private *dp, u32 data)
>> >  	return 0;
>> >  }
>> >
>> > -static int dp_display_unprepare(struct msm_dp *dp)
>> > +static int dp_display_unprepare(struct msm_dp *dp_display)
>> >  {
>> >  	return 0;
>> >  }
>> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.h
>> > b/drivers/gpu/drm/msm/dp/dp_display.h
>> > index 02999408c052..24aefca66029 100644
>> > --- a/drivers/gpu/drm/msm/dp/dp_display.h
>> > +++ b/drivers/gpu/drm/msm/dp/dp_display.h
>> > @@ -15,6 +15,7 @@ struct msm_dp {
>> >  	struct device *codec_dev;
>> >  	struct drm_connector *connector;
>> >  	struct drm_encoder *encoder;
>> > +	struct drm_bridge *panel_bridge;
>> >  	bool is_connected;
>> >  	bool audio_enabled;
>> >  	bool power_on;
>> > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c
>> > b/drivers/gpu/drm/msm/dp/dp_drm.c
>> > index f33e31523f56..76856c4ee1d6 100644
>> > --- a/drivers/gpu/drm/msm/dp/dp_drm.c
>> > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
>> > @@ -5,6 +5,7 @@
>> >
>> >  #include <drm/drm_atomic_helper.h>
>> >  #include <drm/drm_atomic.h>
>> > +#include <drm/drm_bridge.h>
>> >  #include <drm/drm_crtc.h>
>> >
>> >  #include "msm_drv.h"
>> > @@ -160,5 +161,15 @@ struct drm_connector
>> > *dp_drm_connector_init(struct msm_dp *dp_display)
>> >
>> >  	drm_connector_attach_encoder(connector, dp_display->encoder);
>> >
>> > +	if (dp_display->panel_bridge) {
>> > +		ret = drm_bridge_attach(dp_display->encoder,
>> > +					dp_display->panel_bridge, NULL,
>> > +					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>> > +		if (ret < 0) {
>> > +			DRM_ERROR("failed to attach panel bridge: %d\n", ret);
>> > +			return ERR_PTR(ret);
>> > +		}
>> > +	}
>> > +
>> >  	return connector;
>> >  }
>> > diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
>> > b/drivers/gpu/drm/msm/dp/dp_parser.c
>> > index 4d6e047f803d..eb6bbfbea484 100644
>> > --- a/drivers/gpu/drm/msm/dp/dp_parser.c
>> > +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
>> > @@ -6,6 +6,7 @@
>> >  #include <linux/of_gpio.h>
>> >  #include <linux/phy/phy.h>
>> >
>> > +#include <drm/drm_of.h>
>> >  #include <drm/drm_print.h>
>> >
>> >  #include "dp_parser.h"
>> > @@ -263,7 +264,28 @@ static int dp_parser_clock(struct dp_parser
>> > *parser)
>> >  	return 0;
>> >  }
>> >
>> > -static int dp_parser_parse(struct dp_parser *parser)
>> > +static int dp_parser_find_panel(struct dp_parser *parser)
>> > +{
>> > +	struct device *dev = &parser->pdev->dev;
>> > +	struct drm_panel *panel;
>> > +	int rc;
>> > +
>> > +	rc = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
>> > +	if (rc) {
>> > +		DRM_ERROR("failed to acquire DRM panel: %d\n", rc);
>> > +		return rc;
>> > +	}
>> > +
>> > +	parser->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
>> > +	if (IS_ERR(parser->panel_bridge)) {
>> > +		DRM_ERROR("failed to create panel bridge\n");
>> > +		return PTR_ERR(parser->panel_bridge);
>> > +	}
>> 
>> When we add a bridge using devm_drm_panel_bridge_add(), it will 
>> register
>> with default bridge functions which is fine
>> because we need the panel power to be controlled here.
>> 
>> 
>> 140 static const struct drm_bridge_funcs panel_bridge_bridge_funcs = {
>> 141 	.attach = panel_bridge_attach,
>> 142 	.detach = panel_bridge_detach,
>> 143 	.pre_enable = panel_bridge_pre_enable,
>> 144 	.enable = panel_bridge_enable,
>> 145 	.disable = panel_bridge_disable,
>> 146 	.post_disable = panel_bridge_post_disable,
>> 147 	.get_modes = panel_bridge_get_modes,
>> 
>> But what about the EDID related things, the DP/eDP driver already 
>> reads the
>> EDID and gets the modes so we need to skip
>> that in this case as otherwise it will end up calling the 
>> panel_get_modes in
>> the eDP panel which will be redundant.
>> 
>> Let me know if I am missing something in this proposal.
>> 
> 
> I'm slightly lost when it comes to these types, so can you please help
> me understand why the panel_bridge_bridge_funcs->get_modes() would be
> invoked with the proposed patches.
> 
> Because adding a log print in panel_bridge_get_modes() shows that it's
> not invoked.
> 
> 
> So I think we have the opposite problem, rather than having two
> redundant get_modes we only support relying on the EDID information
> directly in the DP driver, at this point.
> 
> So unless I'm missing something critical, this works and provides panel
> and backlight controls and the additional work to move the EDID read 
> out
> to the panel driver would better be done after this initial support
> lands?
> 
> Regards,
> Bjorn

I missed this line which uses the DRM_BRIDGE_ATTACH_NO_CONNECTOR

+		ret = drm_bridge_attach(dp_display->encoder,
+					dp_display->panel_bridge, NULL,
+					DRM_BRIDGE_ATTACH_NO_CONNECTOR);

In this case, no separate connector will get created and hence get_modes 
will not
be called again since this is just a virtual bridge. That explains why 
your log
didnt show up.

I will continue checking the rest of this patch.

> 
>> > +
>> > +	return 0;
>> > +}
>> > +
>> > +static int dp_parser_parse(struct dp_parser *parser, int
>> > connector_type)
>> >  {
>> >  	int rc = 0;
>> >
>> > @@ -284,6 +306,12 @@ static int dp_parser_parse(struct dp_parser
>> > *parser)
>> >  	if (rc)
>> >  		return rc;
>> >
>> > +	if (connector_type == DRM_MODE_CONNECTOR_eDP) {
>> > +		rc = dp_parser_find_panel(parser);
>> > +		if (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 dac10923abde..3172da089421 100644
>> > --- a/drivers/gpu/drm/msm/dp/dp_parser.h
>> > +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
>> > @@ -123,8 +123,9 @@ struct dp_parser {
>> >  	struct dp_display_data disp_data;
>> >  	const struct dp_regulator_cfg *regulator_cfg;
>> >  	u32 max_dp_lanes;
>> > +	struct drm_bridge *panel_bridge;
>> >
>> > -	int (*parse)(struct dp_parser *parser);
>> > +	int (*parse)(struct dp_parser *parser, int connector_type);
>> >  };
>> >
>> >  /**

Re: [PATCH v4 3/7] drm/msm/dp: Allow specifying connector_type per controller

[Bjorn Andersson]

On Tue 05 Oct 19:29 CDT 2021, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2021-10-05 16:13:19)
> > As the following patches introduced support for multiple DP blocks in a
> > platform and some of those block might be eDP it becomes useful to be
> > able to specify the connector type per block.
> >
> > Although there's only a single block at this point, the array of descs
> > and the search in dp_display_get_desc() are introduced here to simplify
> > the next patch, that does introduce support for multiple DP blocks.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >
> > Changes since v3:
> > - New patch
> > - Extended msm_dp_config with connector_type, wrapped in inner struct
> > - Refactored out of the next patch
> > - Pass the connector_type to drm_connector_init(), from yet another patch
> > - Dropped double newline and unnecessary {}
> 
> BTW, I see that we check for the connector type in debugfs.
> 
> $ git grep DRM_MODE_CONNECTOR_DisplayPort -- drivers/gpu/drm/msm/dp/
> drivers/gpu/drm/msm/dp/dp_debug.c:
> DRM_MODE_CONNECTOR_DisplayPort)
> drivers/gpu/drm/msm/dp/dp_debug.c:
> DRM_MODE_CONNECTOR_DisplayPort)
> drivers/gpu/drm/msm/dp/dp_debug.c:
> DRM_MODE_CONNECTOR_DisplayPort)
> drivers/gpu/drm/msm/dp/dp_debug.c:
> DRM_MODE_CONNECTOR_DisplayPort)
> 
> So do those need to be updated to handle either connector type?

The debugfs code loops over all the connectors for the DRM device and
skips those that aren't DisplayPort ones. So fixing this up to properly
support multiple instances in the dp_debugfs code as well should resolve
this.

Regards,
Bjorn

Re: [PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers

[Stephen Boyd]

Quoting Bjorn Andersson (2021-10-05 19:37:52)
> On Tue 05 Oct 19:06 PDT 2021, Stephen Boyd wrote:
>
> > Quoting Bjorn Andersson (2021-10-05 18:43:16)
> > > On Tue 05 Oct 17:43 PDT 2021, Stephen Boyd wrote:
> > >
> > > > Quoting Bjorn Andersson (2021-10-05 16:13:21)
> > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > index bdaf227f05dc..674cddfee5b0 100644
> > > > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > @@ -1233,7 +1239,7 @@ static int dp_display_probe(struct platform_device *pdev)
> > > > >         if (!dp)
> > > > >                 return -ENOMEM;
> > > > >
> > > > > -       desc = dp_display_get_desc(pdev);
> > > > > +       desc = dp_display_get_desc(pdev, &dp->id);
> > > >
> > > > I'm sad that dp->id has to match the number in the SoC specific
> > > > dpu_intf_cfg array in drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > > > still. Is there any way we can avoid that? Also, notice how those arrays
> > > > already have INTF_DP macros, which makes me think that it may be better
> > > > to connect this to those arrays instead of making an msm_dp_desc
> > > > structure and then make sure the 'type' member matches a connector
> > > > type number. Otherwise this code is super fragile.
> > > >
> > >
> > > I'm afraid I don't understand what you're proposing. Or which part you
> > > consider fragile, the indices of the INTF_DP instances aren't going to
> > > move around...
> > >
> > > I have N instances of the DP driver that I need to match to N entries
> > > from the platform specific intf array, I need some stable reference
> > > between them. When I started this journey I figured I could rely on the
> > > of_graph between the DPU and the interface controllers, but the values
> > > used there today are just bogus, so that was a no go.
> > >
> > > We can use whatever, as long as _dpu_kms_initialize_displayport() can
> > > come up with an identifier to put in h_tile_instance[0] so that
> > > dpu_encoder_setup_display() can find the relevant INTF.
> > >
> >
> > To make it more concrete we can look at sc7180
> >
> > static const struct dpu_intf_cfg sc7180_intf[] = {
> >         INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24,
> > INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> >                                                      ^
> >                                                      |
> >
> > intf0 is irrelevant. Also the address is irrelevant. But here we have a
> > zero, the number after INTF_DP, and that is very relevant. That number
> > needs to match the dp->id. Somewhere we have a match between
> > controller_id and dp->id in the code.
>
> That number (the 0, not INTF_0) is what the code matches against dp->id
> in _dpu_kms_initialize_displayport(), in order to figure out that this
> is INTF_0 in dpu_encoder_setup_display().
>
> I.e. look at the sc8180x patch:
>
> INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> INTF_BLK("intf_1", INTF_1, 0x6A800, INTF_DSI, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> INTF_BLK("intf_2", INTF_2, 0x6B000, INTF_DSI, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 28, 29),
> /* INTF_3 is for MST, wired to INTF_DP 0 and 1, use dummy index until this is supported */
> INTF_BLK("intf_3", INTF_3, 0x6B800, INTF_DP, 999, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 30, 31),
> INTF_BLK("intf_4", INTF_4, 0x6C000, INTF_DP, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 20, 21),
> INTF_BLK("intf_5", INTF_5, 0x6C800, INTF_DP, 2, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
>
> Where the DP driver defines the 3 controllers with dp->id of 0, 1 and 2,
> which the DPU code will match against to INTF_0, INTF_4 and INTF_5.
>

Yep. I'm saying that having to make that number in this intf array match
the order of the register mapping descriptor array is fragile. Why can't
we indicate the interface is DP or eDP with INTF_DP or INTF_EDP and then
map from the descriptor array to this intf array somehow so that the
order of the descriptor array doesn't matter? Then we don't have to put
the connector type in the descriptor array, and we don't have to keep
the order of the array a certain way to match this intf descriptor.

Maybe

	struct msm_dp_desc {
		phys_addr_t io_start;
		unsigned int id;
	};

and then have msm_dp_desc::id equal INTF_<N> and then look through the
intf from DPU here in the DP driver to find the id and type of connector
that should be used by default? Still sort of fragile because the only
connection is an unsigned int which isn't great, but at least it's
explicit instead of implicit based on the array order.

Re: [PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers

[Dmitry Baryshkov]

On Wed, 6 Oct 2021 at 07:26, Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Bjorn Andersson (2021-10-05 19:37:52)
> > On Tue 05 Oct 19:06 PDT 2021, Stephen Boyd wrote:
> >
> > > Quoting Bjorn Andersson (2021-10-05 18:43:16)
> > > > On Tue 05 Oct 17:43 PDT 2021, Stephen Boyd wrote:
> > > >
> > > > > Quoting Bjorn Andersson (2021-10-05 16:13:21)
> > > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > index bdaf227f05dc..674cddfee5b0 100644
> > > > > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > @@ -1233,7 +1239,7 @@ static int dp_display_probe(struct platform_device *pdev)
> > > > > >         if (!dp)
> > > > > >                 return -ENOMEM;
> > > > > >
> > > > > > -       desc = dp_display_get_desc(pdev);
> > > > > > +       desc = dp_display_get_desc(pdev, &dp->id);
> > > > >
> > > > > I'm sad that dp->id has to match the number in the SoC specific
> > > > > dpu_intf_cfg array in drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > > > > still. Is there any way we can avoid that? Also, notice how those arrays
> > > > > already have INTF_DP macros, which makes me think that it may be better
> > > > > to connect this to those arrays instead of making an msm_dp_desc
> > > > > structure and then make sure the 'type' member matches a connector
> > > > > type number. Otherwise this code is super fragile.
> > > > >
> > > >
> > > > I'm afraid I don't understand what you're proposing. Or which part you
> > > > consider fragile, the indices of the INTF_DP instances aren't going to
> > > > move around...
> > > >
> > > > I have N instances of the DP driver that I need to match to N entries
> > > > from the platform specific intf array, I need some stable reference
> > > > between them. When I started this journey I figured I could rely on the
> > > > of_graph between the DPU and the interface controllers, but the values
> > > > used there today are just bogus, so that was a no go.
> > > >
> > > > We can use whatever, as long as _dpu_kms_initialize_displayport() can
> > > > come up with an identifier to put in h_tile_instance[0] so that
> > > > dpu_encoder_setup_display() can find the relevant INTF.
> > > >
> > >
> > > To make it more concrete we can look at sc7180
> > >
> > > static const struct dpu_intf_cfg sc7180_intf[] = {
> > >         INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24,
> > > INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> > >                                                      ^
> > >                                                      |
> > >
> > > intf0 is irrelevant. Also the address is irrelevant. But here we have a
> > > zero, the number after INTF_DP, and that is very relevant. That number
> > > needs to match the dp->id. Somewhere we have a match between
> > > controller_id and dp->id in the code.
> >
> > That number (the 0, not INTF_0) is what the code matches against dp->id
> > in _dpu_kms_initialize_displayport(), in order to figure out that this
> > is INTF_0 in dpu_encoder_setup_display().
> >
> > I.e. look at the sc8180x patch:
> >
> > INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> > INTF_BLK("intf_1", INTF_1, 0x6A800, INTF_DSI, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> > INTF_BLK("intf_2", INTF_2, 0x6B000, INTF_DSI, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 28, 29),
> > /* INTF_3 is for MST, wired to INTF_DP 0 and 1, use dummy index until this is supported */
> > INTF_BLK("intf_3", INTF_3, 0x6B800, INTF_DP, 999, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 30, 31),
> > INTF_BLK("intf_4", INTF_4, 0x6C000, INTF_DP, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 20, 21),
> > INTF_BLK("intf_5", INTF_5, 0x6C800, INTF_DP, 2, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
> >
> > Where the DP driver defines the 3 controllers with dp->id of 0, 1 and 2,
> > which the DPU code will match against to INTF_0, INTF_4 and INTF_5.
> >
>
> Yep. I'm saying that having to make that number in this intf array match
> the order of the register mapping descriptor array is fragile. Why can't
> we indicate the interface is DP or eDP with INTF_DP or INTF_EDP and then
> map from the descriptor array to this intf array somehow so that the
> order of the descriptor array doesn't matter? Then we don't have to put
> the connector type in the descriptor array, and we don't have to keep
> the order of the array a certain way to match this intf descriptor.

The order of the descriptor array does not matter currently (or we do
not understand fully your concern).
The encoder is mapped to intf using type + controller_id (next field
after INTF_foo).
Also having the controller_id in the descs array allows us to simplify
DSI code (where DSI_0 is master and DSI_1 is slave, no matter which
INTF they are associated with).

Last, but not least, maybe I'd point you to one of the proposed
cleanup patches:
https://lore.kernel.org/linux-arm-msm/20210515225757.1989955-5-dmitry.baryshkov@linaro.org/
It removes one extra level of indirection in interface association.

>
> Maybe
>
>         struct msm_dp_desc {
>                 phys_addr_t io_start;
>                 unsigned int id;
>         };
>
> and then have msm_dp_desc::id equal INTF_<N> and then look through the
> intf from DPU here in the DP driver to find the id and type of connector
> that should be used by default? Still sort of fragile because the only
> connection is an unsigned int which isn't great, but at least it's
> explicit instead of implicit based on the array order.

It would move indirection, because we'd still have to map INTF_N <->
priv->dp[j].

-- 
With best wishes
Dmitry

Re: [PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers

[Stephen Boyd]

Quoting Dmitry Baryshkov (2021-10-05 23:10:22)
> On Wed, 6 Oct 2021 at 07:26, Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Bjorn Andersson (2021-10-05 19:37:52)
> > > On Tue 05 Oct 19:06 PDT 2021, Stephen Boyd wrote:
> > >
> > > > Quoting Bjorn Andersson (2021-10-05 18:43:16)
> > > > > On Tue 05 Oct 17:43 PDT 2021, Stephen Boyd wrote:
> > > > >
> > > > > > Quoting Bjorn Andersson (2021-10-05 16:13:21)
> > > > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > > index bdaf227f05dc..674cddfee5b0 100644
> > > > > > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > > @@ -1233,7 +1239,7 @@ static int dp_display_probe(struct platform_device *pdev)
> > > > > > >         if (!dp)
> > > > > > >                 return -ENOMEM;
> > > > > > >
> > > > > > > -       desc = dp_display_get_desc(pdev);
> > > > > > > +       desc = dp_display_get_desc(pdev, &dp->id);
> > > > > >
> > > > > > I'm sad that dp->id has to match the number in the SoC specific
> > > > > > dpu_intf_cfg array in drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > > > > > still. Is there any way we can avoid that? Also, notice how those arrays
> > > > > > already have INTF_DP macros, which makes me think that it may be better
> > > > > > to connect this to those arrays instead of making an msm_dp_desc
> > > > > > structure and then make sure the 'type' member matches a connector
> > > > > > type number. Otherwise this code is super fragile.
> > > > > >
> > > > >
> > > > > I'm afraid I don't understand what you're proposing. Or which part you
> > > > > consider fragile, the indices of the INTF_DP instances aren't going to
> > > > > move around...
> > > > >
> > > > > I have N instances of the DP driver that I need to match to N entries
> > > > > from the platform specific intf array, I need some stable reference
> > > > > between them. When I started this journey I figured I could rely on the
> > > > > of_graph between the DPU and the interface controllers, but the values
> > > > > used there today are just bogus, so that was a no go.
> > > > >
> > > > > We can use whatever, as long as _dpu_kms_initialize_displayport() can
> > > > > come up with an identifier to put in h_tile_instance[0] so that
> > > > > dpu_encoder_setup_display() can find the relevant INTF.
> > > > >
> > > >
> > > > To make it more concrete we can look at sc7180
> > > >
> > > > static const struct dpu_intf_cfg sc7180_intf[] = {
> > > >         INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24,
> > > > INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> > > >                                                      ^
> > > >                                                      |
> > > >
> > > > intf0 is irrelevant. Also the address is irrelevant. But here we have a
> > > > zero, the number after INTF_DP, and that is very relevant. That number
> > > > needs to match the dp->id. Somewhere we have a match between
> > > > controller_id and dp->id in the code.
> > >
> > > That number (the 0, not INTF_0) is what the code matches against dp->id
> > > in _dpu_kms_initialize_displayport(), in order to figure out that this
> > > is INTF_0 in dpu_encoder_setup_display().
> > >
> > > I.e. look at the sc8180x patch:
> > >
> > > INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> > > INTF_BLK("intf_1", INTF_1, 0x6A800, INTF_DSI, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> > > INTF_BLK("intf_2", INTF_2, 0x6B000, INTF_DSI, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 28, 29),
> > > /* INTF_3 is for MST, wired to INTF_DP 0 and 1, use dummy index until this is supported */
> > > INTF_BLK("intf_3", INTF_3, 0x6B800, INTF_DP, 999, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 30, 31),
> > > INTF_BLK("intf_4", INTF_4, 0x6C000, INTF_DP, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 20, 21),
> > > INTF_BLK("intf_5", INTF_5, 0x6C800, INTF_DP, 2, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
> > >
> > > Where the DP driver defines the 3 controllers with dp->id of 0, 1 and 2,
> > > which the DPU code will match against to INTF_0, INTF_4 and INTF_5.
> > >
> >
> > Yep. I'm saying that having to make that number in this intf array match
> > the order of the register mapping descriptor array is fragile. Why can't
> > we indicate the interface is DP or eDP with INTF_DP or INTF_EDP and then
> > map from the descriptor array to this intf array somehow so that the
> > order of the descriptor array doesn't matter? Then we don't have to put
> > the connector type in the descriptor array, and we don't have to keep
> > the order of the array a certain way to match this intf descriptor.
>
> The order of the descriptor array does not matter currently (or we do
> not understand fully your concern).
> The encoder is mapped to intf using type + controller_id (next field
> after INTF_foo).
> Also having the controller_id in the descs array allows us to simplify
> DSI code (where DSI_0 is master and DSI_1 is slave, no matter which
> INTF they are associated with).

The order seems to matter for me. Otherwise I get various vblank
timeouts and the eDP panel doesn't light up. I'm using the previous
version of this patch series though so maybe something got fixed in the
meantime. If I change the controller_id to match my new ordering of the
descriptor array then it works again. So somehow controller_id needs to
match dp->id?

>
> Last, but not least, maybe I'd point you to one of the proposed
> cleanup patches:
> https://lore.kernel.org/linux-arm-msm/20210515225757.1989955-5-dmitry.baryshkov@linaro.org/
> It removes one extra level of indirection in interface association.
>

Thanks for the link.

Re: [PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers

[Dmitry Baryshkov]

Hi,

On Wed, 6 Oct 2021 at 10:06, Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Dmitry Baryshkov (2021-10-05 23:10:22)
> > On Wed, 6 Oct 2021 at 07:26, Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Bjorn Andersson (2021-10-05 19:37:52)
> > > > On Tue 05 Oct 19:06 PDT 2021, Stephen Boyd wrote:
> > > >
> > > > > Quoting Bjorn Andersson (2021-10-05 18:43:16)
> > > > > > On Tue 05 Oct 17:43 PDT 2021, Stephen Boyd wrote:
> > > > > >
> > > > > > > Quoting Bjorn Andersson (2021-10-05 16:13:21)
> > > > > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > > > index bdaf227f05dc..674cddfee5b0 100644
> > > > > > > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > > > @@ -1233,7 +1239,7 @@ static int dp_display_probe(struct platform_device *pdev)
> > > > > > > >         if (!dp)
> > > > > > > >                 return -ENOMEM;
> > > > > > > >
> > > > > > > > -       desc = dp_display_get_desc(pdev);
> > > > > > > > +       desc = dp_display_get_desc(pdev, &dp->id);
> > > > > > >
> > > > > > > I'm sad that dp->id has to match the number in the SoC specific
> > > > > > > dpu_intf_cfg array in drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > > > > > > still. Is there any way we can avoid that? Also, notice how those arrays
> > > > > > > already have INTF_DP macros, which makes me think that it may be better
> > > > > > > to connect this to those arrays instead of making an msm_dp_desc
> > > > > > > structure and then make sure the 'type' member matches a connector
> > > > > > > type number. Otherwise this code is super fragile.
> > > > > > >
> > > > > >
> > > > > > I'm afraid I don't understand what you're proposing. Or which part you
> > > > > > consider fragile, the indices of the INTF_DP instances aren't going to
> > > > > > move around...
> > > > > >
> > > > > > I have N instances of the DP driver that I need to match to N entries
> > > > > > from the platform specific intf array, I need some stable reference
> > > > > > between them. When I started this journey I figured I could rely on the
> > > > > > of_graph between the DPU and the interface controllers, but the values
> > > > > > used there today are just bogus, so that was a no go.
> > > > > >
> > > > > > We can use whatever, as long as _dpu_kms_initialize_displayport() can
> > > > > > come up with an identifier to put in h_tile_instance[0] so that
> > > > > > dpu_encoder_setup_display() can find the relevant INTF.
> > > > > >
> > > > >
> > > > > To make it more concrete we can look at sc7180
> > > > >
> > > > > static const struct dpu_intf_cfg sc7180_intf[] = {
> > > > >         INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24,
> > > > > INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> > > > >                                                      ^
> > > > >                                                      |
> > > > >
> > > > > intf0 is irrelevant. Also the address is irrelevant. But here we have a
> > > > > zero, the number after INTF_DP, and that is very relevant. That number
> > > > > needs to match the dp->id. Somewhere we have a match between
> > > > > controller_id and dp->id in the code.
> > > >
> > > > That number (the 0, not INTF_0) is what the code matches against dp->id
> > > > in _dpu_kms_initialize_displayport(), in order to figure out that this
> > > > is INTF_0 in dpu_encoder_setup_display().
> > > >
> > > > I.e. look at the sc8180x patch:
> > > >
> > > > INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> > > > INTF_BLK("intf_1", INTF_1, 0x6A800, INTF_DSI, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> > > > INTF_BLK("intf_2", INTF_2, 0x6B000, INTF_DSI, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 28, 29),
> > > > /* INTF_3 is for MST, wired to INTF_DP 0 and 1, use dummy index until this is supported */
> > > > INTF_BLK("intf_3", INTF_3, 0x6B800, INTF_DP, 999, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 30, 31),
> > > > INTF_BLK("intf_4", INTF_4, 0x6C000, INTF_DP, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 20, 21),
> > > > INTF_BLK("intf_5", INTF_5, 0x6C800, INTF_DP, 2, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
> > > >
> > > > Where the DP driver defines the 3 controllers with dp->id of 0, 1 and 2,
> > > > which the DPU code will match against to INTF_0, INTF_4 and INTF_5.
> > > >
> > >
> > > Yep. I'm saying that having to make that number in this intf array match
> > > the order of the register mapping descriptor array is fragile. Why can't
> > > we indicate the interface is DP or eDP with INTF_DP or INTF_EDP and then
> > > map from the descriptor array to this intf array somehow so that the
> > > order of the descriptor array doesn't matter? Then we don't have to put
> > > the connector type in the descriptor array, and we don't have to keep
> > > the order of the array a certain way to match this intf descriptor.
> >
> > The order of the descriptor array does not matter currently (or we do
> > not understand fully your concern).
> > The encoder is mapped to intf using type + controller_id (next field
> > after INTF_foo).
> > Also having the controller_id in the descs array allows us to simplify
> > DSI code (where DSI_0 is master and DSI_1 is slave, no matter which
> > INTF they are associated with).
>
> The order seems to matter for me. Otherwise I get various vblank
> timeouts and the eDP panel doesn't light up. I'm using the previous
> version of this patch series though so maybe something got fixed in the
> meantime. If I change the controller_id to match my new ordering of the
> descriptor array then it works again. So somehow controller_id needs to
> match dp->id?

Yes, controller_id should match. However the order of entries in the
array should not matter. If it does, it's clearly an issue somewhere.

>
> >
> > Last, but not least, maybe I'd point you to one of the proposed
> > cleanup patches:
> > https://lore.kernel.org/linux-arm-msm/20210515225757.1989955-5-dmitry.baryshkov@linaro.org/
> > It removes one extra level of indirection in interface association.
> >
>
> Thanks for the link.



-- 
With best wishes
Dmitry

Re: [PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers

[Bjorn Andersson]

On Tue 05 Oct 21:26 PDT 2021, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2021-10-05 19:37:52)
> > On Tue 05 Oct 19:06 PDT 2021, Stephen Boyd wrote:
> >
> > > Quoting Bjorn Andersson (2021-10-05 18:43:16)
> > > > On Tue 05 Oct 17:43 PDT 2021, Stephen Boyd wrote:
> > > >
> > > > > Quoting Bjorn Andersson (2021-10-05 16:13:21)
> > > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > index bdaf227f05dc..674cddfee5b0 100644
> > > > > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > @@ -1233,7 +1239,7 @@ static int dp_display_probe(struct platform_device *pdev)
> > > > > >         if (!dp)
> > > > > >                 return -ENOMEM;
> > > > > >
> > > > > > -       desc = dp_display_get_desc(pdev);
> > > > > > +       desc = dp_display_get_desc(pdev, &dp->id);
> > > > >
> > > > > I'm sad that dp->id has to match the number in the SoC specific
> > > > > dpu_intf_cfg array in drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > > > > still. Is there any way we can avoid that? Also, notice how those arrays
> > > > > already have INTF_DP macros, which makes me think that it may be better
> > > > > to connect this to those arrays instead of making an msm_dp_desc
> > > > > structure and then make sure the 'type' member matches a connector
> > > > > type number. Otherwise this code is super fragile.
> > > > >
> > > >
> > > > I'm afraid I don't understand what you're proposing. Or which part you
> > > > consider fragile, the indices of the INTF_DP instances aren't going to
> > > > move around...
> > > >
> > > > I have N instances of the DP driver that I need to match to N entries
> > > > from the platform specific intf array, I need some stable reference
> > > > between them. When I started this journey I figured I could rely on the
> > > > of_graph between the DPU and the interface controllers, but the values
> > > > used there today are just bogus, so that was a no go.
> > > >
> > > > We can use whatever, as long as _dpu_kms_initialize_displayport() can
> > > > come up with an identifier to put in h_tile_instance[0] so that
> > > > dpu_encoder_setup_display() can find the relevant INTF.
> > > >
> > >
> > > To make it more concrete we can look at sc7180
> > >
> > > static const struct dpu_intf_cfg sc7180_intf[] = {
> > >         INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24,
> > > INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> > >                                                      ^
> > >                                                      |
> > >
> > > intf0 is irrelevant. Also the address is irrelevant. But here we have a
> > > zero, the number after INTF_DP, and that is very relevant. That number
> > > needs to match the dp->id. Somewhere we have a match between
> > > controller_id and dp->id in the code.
> >
> > That number (the 0, not INTF_0) is what the code matches against dp->id
> > in _dpu_kms_initialize_displayport(), in order to figure out that this
> > is INTF_0 in dpu_encoder_setup_display().
> >
> > I.e. look at the sc8180x patch:
> >
> > INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> > INTF_BLK("intf_1", INTF_1, 0x6A800, INTF_DSI, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> > INTF_BLK("intf_2", INTF_2, 0x6B000, INTF_DSI, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 28, 29),
> > /* INTF_3 is for MST, wired to INTF_DP 0 and 1, use dummy index until this is supported */
> > INTF_BLK("intf_3", INTF_3, 0x6B800, INTF_DP, 999, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 30, 31),
> > INTF_BLK("intf_4", INTF_4, 0x6C000, INTF_DP, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 20, 21),
> > INTF_BLK("intf_5", INTF_5, 0x6C800, INTF_DP, 2, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
> >
> > Where the DP driver defines the 3 controllers with dp->id of 0, 1 and 2,
> > which the DPU code will match against to INTF_0, INTF_4 and INTF_5.
> >
> 
> Yep. I'm saying that having to make that number in this intf array match
> the order of the register mapping descriptor array is fragile. Why can't
> we indicate the interface is DP or eDP with INTF_DP or INTF_EDP and then
> map from the descriptor array to this intf array somehow so that the
> order of the descriptor array doesn't matter? Then we don't have to put
> the connector type in the descriptor array, and we don't have to keep
> the order of the array a certain way to match this intf descriptor.
> 
> Maybe
> 
> 	struct msm_dp_desc {
> 		phys_addr_t io_start;
> 		unsigned int id;

The INTF_<N> constants are a property of the DPU driver and not
available in the DP driver and the msm_dp struct is a property of the DP
driver and can't be dereferenced in the DPU driver.

The proposed way around this is that the descs array defines the order
in priv->dp[N] and this N is used as controller_id.


So the only thing that I don't find straight forward here is that the
eDP controller is considered just a DP controller, so you have to use
INTF_DP, <N> for that, and not just INTF_EDP, 0.

> 	};
> 
> and then have msm_dp_desc::id equal INTF_<N> and then look through the
> intf from DPU here in the DP driver to find the id and type of connector
> that should be used by default? Still sort of fragile because the only
> connection is an unsigned int which isn't great, but at least it's
> explicit instead of implicit based on the array order.

No matter how I look at this, you need to put some number somewhere here
that will be used to match up the INTF with the right DSI/DP encoder.

Using the proposed number scheme follows the numbering of all the DP
controllers from the documentation.

Regards,
Bjorn

Re: [PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers

[Stephen Boyd]

Quoting Bjorn Andersson (2021-10-06 10:07:17)
> On Tue 05 Oct 21:26 PDT 2021, Stephen Boyd wrote:
>
> > Quoting Bjorn Andersson (2021-10-05 19:37:52)
> > > On Tue 05 Oct 19:06 PDT 2021, Stephen Boyd wrote:
> > >
> > > > Quoting Bjorn Andersson (2021-10-05 18:43:16)
> > > > > On Tue 05 Oct 17:43 PDT 2021, Stephen Boyd wrote:
> > > > >
> > > > > > Quoting Bjorn Andersson (2021-10-05 16:13:21)
> > > > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > > index bdaf227f05dc..674cddfee5b0 100644
> > > > > > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > > @@ -1233,7 +1239,7 @@ static int dp_display_probe(struct platform_device *pdev)
> > > > > > >         if (!dp)
> > > > > > >                 return -ENOMEM;
> > > > > > >
> > > > > > > -       desc = dp_display_get_desc(pdev);
> > > > > > > +       desc = dp_display_get_desc(pdev, &dp->id);
> > > > > >
> > > > > > I'm sad that dp->id has to match the number in the SoC specific
> > > > > > dpu_intf_cfg array in drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > > > > > still. Is there any way we can avoid that? Also, notice how those arrays
> > > > > > already have INTF_DP macros, which makes me think that it may be better
> > > > > > to connect this to those arrays instead of making an msm_dp_desc
> > > > > > structure and then make sure the 'type' member matches a connector
> > > > > > type number. Otherwise this code is super fragile.
> > > > > >
> > > > >
> > > > > I'm afraid I don't understand what you're proposing. Or which part you
> > > > > consider fragile, the indices of the INTF_DP instances aren't going to
> > > > > move around...
> > > > >
> > > > > I have N instances of the DP driver that I need to match to N entries
> > > > > from the platform specific intf array, I need some stable reference
> > > > > between them. When I started this journey I figured I could rely on the
> > > > > of_graph between the DPU and the interface controllers, but the values
> > > > > used there today are just bogus, so that was a no go.
> > > > >
> > > > > We can use whatever, as long as _dpu_kms_initialize_displayport() can
> > > > > come up with an identifier to put in h_tile_instance[0] so that
> > > > > dpu_encoder_setup_display() can find the relevant INTF.
> > > > >
> > > >
> > > > To make it more concrete we can look at sc7180
> > > >
> > > > static const struct dpu_intf_cfg sc7180_intf[] = {
> > > >         INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24,
> > > > INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> > > >                                                      ^
> > > >                                                      |
> > > >
> > > > intf0 is irrelevant. Also the address is irrelevant. But here we have a
> > > > zero, the number after INTF_DP, and that is very relevant. That number
> > > > needs to match the dp->id. Somewhere we have a match between
> > > > controller_id and dp->id in the code.
> > >
> > > That number (the 0, not INTF_0) is what the code matches against dp->id
> > > in _dpu_kms_initialize_displayport(), in order to figure out that this
> > > is INTF_0 in dpu_encoder_setup_display().
> > >
> > > I.e. look at the sc8180x patch:
> > >
> > > INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> > > INTF_BLK("intf_1", INTF_1, 0x6A800, INTF_DSI, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> > > INTF_BLK("intf_2", INTF_2, 0x6B000, INTF_DSI, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 28, 29),
> > > /* INTF_3 is for MST, wired to INTF_DP 0 and 1, use dummy index until this is supported */
> > > INTF_BLK("intf_3", INTF_3, 0x6B800, INTF_DP, 999, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 30, 31),
> > > INTF_BLK("intf_4", INTF_4, 0x6C000, INTF_DP, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 20, 21),
> > > INTF_BLK("intf_5", INTF_5, 0x6C800, INTF_DP, 2, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
> > >
> > > Where the DP driver defines the 3 controllers with dp->id of 0, 1 and 2,
> > > which the DPU code will match against to INTF_0, INTF_4 and INTF_5.
> > >
> >
> > Yep. I'm saying that having to make that number in this intf array match
> > the order of the register mapping descriptor array is fragile. Why can't
> > we indicate the interface is DP or eDP with INTF_DP or INTF_EDP and then
> > map from the descriptor array to this intf array somehow so that the
> > order of the descriptor array doesn't matter? Then we don't have to put
> > the connector type in the descriptor array, and we don't have to keep
> > the order of the array a certain way to match this intf descriptor.
> >
> > Maybe
> >
> >       struct msm_dp_desc {
> >               phys_addr_t io_start;
> >               unsigned int id;
>
> The INTF_<N> constants are a property of the DPU driver and not
> available in the DP driver and the msm_dp struct is a property of the DP
> driver and can't be dereferenced in the DPU driver.
>
> The proposed way around this is that the descs array defines the order
> in priv->dp[N] and this N is used as controller_id.

I'm pretty sure I'm following along.

>
> So the only thing that I don't find straight forward here is that the
> eDP controller is considered just a DP controller, so you have to use
> INTF_DP, <N> for that, and not just INTF_EDP, 0.
>
> >       };
> >
> > and then have msm_dp_desc::id equal INTF_<N> and then look through the
> > intf from DPU here in the DP driver to find the id and type of connector
> > that should be used by default? Still sort of fragile because the only
> > connection is an unsigned int which isn't great, but at least it's
> > explicit instead of implicit based on the array order.
>
> No matter how I look at this, you need to put some number somewhere here
> that will be used to match up the INTF with the right DSI/DP encoder.

Correct.

>
> Using the proposed number scheme follows the numbering of all the DP
> controllers from the documentation.
>

Maybe I can make a better example. I have this for sc7280 in dpu_hw_catalog.c:

	static const struct dpu_intf_cfg sc7280_intf[] = {
		INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, CONTROLLER_ID_A, 24,
INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
		INTF_BLK("intf_1", INTF_1, 0x35000, INTF_DSI, 0, 24,
INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
		INTF_BLK("intf_5", INTF_5, 0x39000, INTF_DP, CONTROLLER_ID_B, 24,
INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
	};

And then this array for sc7280 in dp_display.c:

	static const struct msm_dp_desc sc7280_dp_cfg = {
		.desc = {
			[CONTROLLER_ID_A] = { 0xaea0000, DRM_MODE_CONNECTOR_eDP },
			[CONTROLLER_ID_B] = { 0xae90000, DRM_MODE_CONNECTOR_DisplayPort },
		},
		.num_dp = 2,
	};

So these two arrays must match based on CONTROLLER_ID_{A,B}. I don't
like having to make these two numbers match so if it was explicit, even
possibly by having a bunch of macros put in both places then I would be
happy. I spent a few hours when I messed up the order of the
sc7280_dp_cfg.desc array trying to figure out why things weren't
working.

Re: [PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers

[Dmitry Baryshkov]

On 06/10/2021 20:07, Bjorn Andersson wrote:
> On Tue 05 Oct 21:26 PDT 2021, Stephen Boyd wrote:
> 
>> Quoting Bjorn Andersson (2021-10-05 19:37:52)
>>> On Tue 05 Oct 19:06 PDT 2021, Stephen Boyd wrote:
>>>
>>>> Quoting Bjorn Andersson (2021-10-05 18:43:16)
>>>>> On Tue 05 Oct 17:43 PDT 2021, Stephen Boyd wrote:
>>>>>
>>>>>> Quoting Bjorn Andersson (2021-10-05 16:13:21)
>>>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>>>> index bdaf227f05dc..674cddfee5b0 100644
>>>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>>>>> @@ -1233,7 +1239,7 @@ static int dp_display_probe(struct platform_device *pdev)
>>>>>>>          if (!dp)
>>>>>>>                  return -ENOMEM;
>>>>>>>
>>>>>>> -       desc = dp_display_get_desc(pdev);
>>>>>>> +       desc = dp_display_get_desc(pdev, &dp->id);
>>>>>>
>>>>>> I'm sad that dp->id has to match the number in the SoC specific
>>>>>> dpu_intf_cfg array in drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>>> still. Is there any way we can avoid that? Also, notice how those arrays
>>>>>> already have INTF_DP macros, which makes me think that it may be better
>>>>>> to connect this to those arrays instead of making an msm_dp_desc
>>>>>> structure and then make sure the 'type' member matches a connector
>>>>>> type number. Otherwise this code is super fragile.
>>>>>>
>>>>>
>>>>> I'm afraid I don't understand what you're proposing. Or which part you
>>>>> consider fragile, the indices of the INTF_DP instances aren't going to
>>>>> move around...
>>>>>
>>>>> I have N instances of the DP driver that I need to match to N entries
>>>>> from the platform specific intf array, I need some stable reference
>>>>> between them. When I started this journey I figured I could rely on the
>>>>> of_graph between the DPU and the interface controllers, but the values
>>>>> used there today are just bogus, so that was a no go.
>>>>>
>>>>> We can use whatever, as long as _dpu_kms_initialize_displayport() can
>>>>> come up with an identifier to put in h_tile_instance[0] so that
>>>>> dpu_encoder_setup_display() can find the relevant INTF.
>>>>>
>>>>
>>>> To make it more concrete we can look at sc7180
>>>>
>>>> static const struct dpu_intf_cfg sc7180_intf[] = {
>>>>          INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24,
>>>> INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
>>>>                                                       ^
>>>>                                                       |
>>>>
>>>> intf0 is irrelevant. Also the address is irrelevant. But here we have a
>>>> zero, the number after INTF_DP, and that is very relevant. That number
>>>> needs to match the dp->id. Somewhere we have a match between
>>>> controller_id and dp->id in the code.
>>>
>>> That number (the 0, not INTF_0) is what the code matches against dp->id
>>> in _dpu_kms_initialize_displayport(), in order to figure out that this
>>> is INTF_0 in dpu_encoder_setup_display().
>>>
>>> I.e. look at the sc8180x patch:
>>>
>>> INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
>>> INTF_BLK("intf_1", INTF_1, 0x6A800, INTF_DSI, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
>>> INTF_BLK("intf_2", INTF_2, 0x6B000, INTF_DSI, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 28, 29),
>>> /* INTF_3 is for MST, wired to INTF_DP 0 and 1, use dummy index until this is supported */
>>> INTF_BLK("intf_3", INTF_3, 0x6B800, INTF_DP, 999, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 30, 31),
>>> INTF_BLK("intf_4", INTF_4, 0x6C000, INTF_DP, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 20, 21),
>>> INTF_BLK("intf_5", INTF_5, 0x6C800, INTF_DP, 2, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
>>>
>>> Where the DP driver defines the 3 controllers with dp->id of 0, 1 and 2,
>>> which the DPU code will match against to INTF_0, INTF_4 and INTF_5.
>>>
>>
>> Yep. I'm saying that having to make that number in this intf array match
>> the order of the register mapping descriptor array is fragile. Why can't
>> we indicate the interface is DP or eDP with INTF_DP or INTF_EDP and then
>> map from the descriptor array to this intf array somehow so that the
>> order of the descriptor array doesn't matter? Then we don't have to put
>> the connector type in the descriptor array, and we don't have to keep
>> the order of the array a certain way to match this intf descriptor.
>>
>> Maybe
>>
>> 	struct msm_dp_desc {
>> 		phys_addr_t io_start;
>> 		unsigned int id;
> 
> The INTF_<N> constants are a property of the DPU driver and not
> available in the DP driver and the msm_dp struct is a property of the DP
> driver and can't be dereferenced in the DPU driver.
> 
> The proposed way around this is that the descs array defines the order
> in priv->dp[N] and this N is used as controller_id.
> 
> 
> So the only thing that I don't find straight forward here is that the
> eDP controller is considered just a DP controller, so you have to use
> INTF_DP, <N> for that, and not just INTF_EDP, 0.

Would it be better if we change the DPU driver to handle INTF_EDP too?

> 
>> 	};
>>
>> and then have msm_dp_desc::id equal INTF_<N> and then look through the
>> intf from DPU here in the DP driver to find the id and type of connector
>> that should be used by default? Still sort of fragile because the only
>> connection is an unsigned int which isn't great, but at least it's
>> explicit instead of implicit based on the array order.
> 
> No matter how I look at this, you need to put some number somewhere here
> that will be used to match up the INTF with the right DSI/DP encoder.
> 
> Using the proposed number scheme follows the numbering of all the DP
> controllers from the documentation.
> 
> Regards,
> Bjorn
> 


-- 
With best wishes
Dmitry

Re: [PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers

[Bjorn Andersson]

On Wed 06 Oct 10:19 PDT 2021, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2021-10-06 10:07:17)
> > On Tue 05 Oct 21:26 PDT 2021, Stephen Boyd wrote:
> >
> > > Quoting Bjorn Andersson (2021-10-05 19:37:52)
> > > > On Tue 05 Oct 19:06 PDT 2021, Stephen Boyd wrote:
> > > >
> > > > > Quoting Bjorn Andersson (2021-10-05 18:43:16)
> > > > > > On Tue 05 Oct 17:43 PDT 2021, Stephen Boyd wrote:
> > > > > >
> > > > > > > Quoting Bjorn Andersson (2021-10-05 16:13:21)
> > > > > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > > > index bdaf227f05dc..674cddfee5b0 100644
> > > > > > > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > > > @@ -1233,7 +1239,7 @@ static int dp_display_probe(struct platform_device *pdev)
> > > > > > > >         if (!dp)
> > > > > > > >                 return -ENOMEM;
> > > > > > > >
> > > > > > > > -       desc = dp_display_get_desc(pdev);
> > > > > > > > +       desc = dp_display_get_desc(pdev, &dp->id);
> > > > > > >
> > > > > > > I'm sad that dp->id has to match the number in the SoC specific
> > > > > > > dpu_intf_cfg array in drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > > > > > > still. Is there any way we can avoid that? Also, notice how those arrays
> > > > > > > already have INTF_DP macros, which makes me think that it may be better
> > > > > > > to connect this to those arrays instead of making an msm_dp_desc
> > > > > > > structure and then make sure the 'type' member matches a connector
> > > > > > > type number. Otherwise this code is super fragile.
> > > > > > >
> > > > > >
> > > > > > I'm afraid I don't understand what you're proposing. Or which part you
> > > > > > consider fragile, the indices of the INTF_DP instances aren't going to
> > > > > > move around...
> > > > > >
> > > > > > I have N instances of the DP driver that I need to match to N entries
> > > > > > from the platform specific intf array, I need some stable reference
> > > > > > between them. When I started this journey I figured I could rely on the
> > > > > > of_graph between the DPU and the interface controllers, but the values
> > > > > > used there today are just bogus, so that was a no go.
> > > > > >
> > > > > > We can use whatever, as long as _dpu_kms_initialize_displayport() can
> > > > > > come up with an identifier to put in h_tile_instance[0] so that
> > > > > > dpu_encoder_setup_display() can find the relevant INTF.
> > > > > >
> > > > >
> > > > > To make it more concrete we can look at sc7180
> > > > >
> > > > > static const struct dpu_intf_cfg sc7180_intf[] = {
> > > > >         INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24,
> > > > > INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> > > > >                                                      ^
> > > > >                                                      |
> > > > >
> > > > > intf0 is irrelevant. Also the address is irrelevant. But here we have a
> > > > > zero, the number after INTF_DP, and that is very relevant. That number
> > > > > needs to match the dp->id. Somewhere we have a match between
> > > > > controller_id and dp->id in the code.
> > > >
> > > > That number (the 0, not INTF_0) is what the code matches against dp->id
> > > > in _dpu_kms_initialize_displayport(), in order to figure out that this
> > > > is INTF_0 in dpu_encoder_setup_display().
> > > >
> > > > I.e. look at the sc8180x patch:
> > > >
> > > > INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> > > > INTF_BLK("intf_1", INTF_1, 0x6A800, INTF_DSI, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> > > > INTF_BLK("intf_2", INTF_2, 0x6B000, INTF_DSI, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 28, 29),
> > > > /* INTF_3 is for MST, wired to INTF_DP 0 and 1, use dummy index until this is supported */
> > > > INTF_BLK("intf_3", INTF_3, 0x6B800, INTF_DP, 999, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 30, 31),
> > > > INTF_BLK("intf_4", INTF_4, 0x6C000, INTF_DP, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 20, 21),
> > > > INTF_BLK("intf_5", INTF_5, 0x6C800, INTF_DP, 2, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
> > > >
> > > > Where the DP driver defines the 3 controllers with dp->id of 0, 1 and 2,
> > > > which the DPU code will match against to INTF_0, INTF_4 and INTF_5.
> > > >
> > >
> > > Yep. I'm saying that having to make that number in this intf array match
> > > the order of the register mapping descriptor array is fragile. Why can't
> > > we indicate the interface is DP or eDP with INTF_DP or INTF_EDP and then
> > > map from the descriptor array to this intf array somehow so that the
> > > order of the descriptor array doesn't matter? Then we don't have to put
> > > the connector type in the descriptor array, and we don't have to keep
> > > the order of the array a certain way to match this intf descriptor.
> > >
> > > Maybe
> > >
> > >       struct msm_dp_desc {
> > >               phys_addr_t io_start;
> > >               unsigned int id;
> >
> > The INTF_<N> constants are a property of the DPU driver and not
> > available in the DP driver and the msm_dp struct is a property of the DP
> > driver and can't be dereferenced in the DPU driver.
> >
> > The proposed way around this is that the descs array defines the order
> > in priv->dp[N] and this N is used as controller_id.
> 
> I'm pretty sure I'm following along.
> 
> >
> > So the only thing that I don't find straight forward here is that the
> > eDP controller is considered just a DP controller, so you have to use
> > INTF_DP, <N> for that, and not just INTF_EDP, 0.
> >
> > >       };
> > >
> > > and then have msm_dp_desc::id equal INTF_<N> and then look through the
> > > intf from DPU here in the DP driver to find the id and type of connector
> > > that should be used by default? Still sort of fragile because the only
> > > connection is an unsigned int which isn't great, but at least it's
> > > explicit instead of implicit based on the array order.
> >
> > No matter how I look at this, you need to put some number somewhere here
> > that will be used to match up the INTF with the right DSI/DP encoder.
> 
> Correct.
> 
> >
> > Using the proposed number scheme follows the numbering of all the DP
> > controllers from the documentation.
> >
> 
> Maybe I can make a better example. I have this for sc7280 in dpu_hw_catalog.c:
> 
> 	static const struct dpu_intf_cfg sc7280_intf[] = {
> 		INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, CONTROLLER_ID_A, 24,
> INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> 		INTF_BLK("intf_1", INTF_1, 0x35000, INTF_DSI, 0, 24,
> INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> 		INTF_BLK("intf_5", INTF_5, 0x39000, INTF_DP, CONTROLLER_ID_B, 24,
> INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
> 	};
> 
> And then this array for sc7280 in dp_display.c:
> 
> 	static const struct msm_dp_desc sc7280_dp_cfg = {
> 		.desc = {
> 			[CONTROLLER_ID_A] = { 0xaea0000, DRM_MODE_CONNECTOR_eDP },
> 			[CONTROLLER_ID_B] = { 0xae90000, DRM_MODE_CONNECTOR_DisplayPort },
> 		},
> 		.num_dp = 2,
> 	};
> 
> So these two arrays must match based on CONTROLLER_ID_{A,B}. I don't
> like having to make these two numbers match so if it was explicit, even
> possibly by having a bunch of macros put in both places then I would be
> happy. I spent a few hours when I messed up the order of the
> sc7280_dp_cfg.desc array trying to figure out why things weren't
> working.

So essentially, you didn't know that the controller_id has to match the
index in priv->dsi[] and priv->dp[] and providing a define for them
would make this more obvious?

I think per your argument the 0 following INTF_DSI should also be using
this scheme, so we'd have multiple CONTROLLER_ID_A, which probably is
confusing as well.

I tried it out with below patch; it documents the relationship, provides
constants for the magic 2 and 3 for number of DSI and DP controllers in
struct msm_drm_private.

I like it.

Regards,
Bjorn

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 60eed3128b54..e9510083f568 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -918,13 +918,13 @@ static const struct dpu_intf_cfg sc7280_intf[] = {
 };
 
 static const struct dpu_intf_cfg sc8180x_intf[] = {
-	INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
-	INTF_BLK("intf_1", INTF_1, 0x6A800, INTF_DSI, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
-	INTF_BLK("intf_2", INTF_2, 0x6B000, INTF_DSI, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 28, 29),
+	INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, DP_CONTROLLER_0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
+	INTF_BLK("intf_1", INTF_1, 0x6A800, INTF_DSI, DSI_CONTROLLER_0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
+	INTF_BLK("intf_2", INTF_2, 0x6B000, INTF_DSI, DSI_CONTROLLER_1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 28, 29),
 	/* INTF_3 is for MST, wired to INTF_DP 0 and 1, use dummy index until this is supported */
 	INTF_BLK("intf_3", INTF_3, 0x6B800, INTF_DP, 999, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 30, 31),
-	INTF_BLK("intf_4", INTF_4, 0x6C000, INTF_DP, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 20, 21),
-	INTF_BLK("intf_5", INTF_5, 0x6C800, INTF_DP, 2, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
+	INTF_BLK("intf_4", INTF_4, 0x6C000, INTF_DP, DP_CONTROLLER_1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 20, 21),
+	INTF_BLK("intf_5", INTF_5, 0x6C800, INTF_DP, DP_CONTROLLER_2, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
 };
 
 /*************************************************************
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 41a6f58916e6..feccdc59b181 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -132,16 +132,16 @@ struct msm_dp_config {
 
 static const struct msm_dp_config sc7180_dp_cfg = {
 	.descs = (struct msm_dp_desc[]) {
-		{ .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
+		[DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
 	},
 	.num_descs = 1,
 };
 
 static const struct msm_dp_config sc8180x_dp_cfg = {
 	.descs = (struct msm_dp_desc[]) {
-		{ .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
-		{ .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
-		{ .io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP },
+		[DP_CONTROLLER_0] = { .io_start = 0x0ae90000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
+		[DP_CONTROLLER_1] = { .io_start = 0x0ae98000, .connector_type = DRM_MODE_CONNECTOR_DisplayPort },
+		[DP_CONTROLLER_2] = { .io_start = 0x0ae9a000, .connector_type = DRM_MODE_CONNECTOR_eDP },
 	},
 	.num_descs = 3,
 };
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 2e84dc30e12e..af572d1297ea 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -68,6 +68,19 @@ enum msm_mdp_plane_property {
 	PLANE_PROP_MAX_NUM
 };
 
+enum msm_dsi_controllers {
+	DSI_CONTROLLER_0,
+	DSI_CONTROLLER_1,
+	DSI_CONTROLLER_COUNT,
+};
+
+enum msm_dp_controllers {
+	DP_CONTROLLER_0,
+	DP_CONTROLLER_1,
+	DP_CONTROLLER_2,
+	DP_CONTROLLER_COUNT,
+};
+
 #define MSM_GPU_MAX_RINGS 4
 #define MAX_H_TILES_PER_DISPLAY 2
 
@@ -159,9 +172,9 @@ struct msm_drm_private {
 	struct msm_edp *edp;
 
 	/* DSI is shared by mdp4 and mdp5 */
-	struct msm_dsi *dsi[2];
+	struct msm_dsi *dsi[DSI_CONTROLLER_COUNT];
 
-	struct msm_dp *dp[3];
+	struct msm_dp *dp[DP_CONTROLLER_COUNT];
 
 	/* when we have more than one 'msm_gpu' these need to be an array: */
 	struct msm_gpu *gpu;

Re: [PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers

[Bjorn Andersson]

On Wed 06 Oct 10:19 PDT 2021, Dmitry Baryshkov wrote:

> On 06/10/2021 20:07, Bjorn Andersson wrote:
> > On Tue 05 Oct 21:26 PDT 2021, Stephen Boyd wrote:
> > 
> > > Quoting Bjorn Andersson (2021-10-05 19:37:52)
> > > > On Tue 05 Oct 19:06 PDT 2021, Stephen Boyd wrote:
> > > > 
> > > > > Quoting Bjorn Andersson (2021-10-05 18:43:16)
> > > > > > On Tue 05 Oct 17:43 PDT 2021, Stephen Boyd wrote:
> > > > > > 
> > > > > > > Quoting Bjorn Andersson (2021-10-05 16:13:21)
> > > > > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > > > index bdaf227f05dc..674cddfee5b0 100644
> > > > > > > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > > > @@ -1233,7 +1239,7 @@ static int dp_display_probe(struct platform_device *pdev)
> > > > > > > >          if (!dp)
> > > > > > > >                  return -ENOMEM;
> > > > > > > > 
> > > > > > > > -       desc = dp_display_get_desc(pdev);
> > > > > > > > +       desc = dp_display_get_desc(pdev, &dp->id);
> > > > > > > 
> > > > > > > I'm sad that dp->id has to match the number in the SoC specific
> > > > > > > dpu_intf_cfg array in drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > > > > > > still. Is there any way we can avoid that? Also, notice how those arrays
> > > > > > > already have INTF_DP macros, which makes me think that it may be better
> > > > > > > to connect this to those arrays instead of making an msm_dp_desc
> > > > > > > structure and then make sure the 'type' member matches a connector
> > > > > > > type number. Otherwise this code is super fragile.
> > > > > > > 
> > > > > > 
> > > > > > I'm afraid I don't understand what you're proposing. Or which part you
> > > > > > consider fragile, the indices of the INTF_DP instances aren't going to
> > > > > > move around...
> > > > > > 
> > > > > > I have N instances of the DP driver that I need to match to N entries
> > > > > > from the platform specific intf array, I need some stable reference
> > > > > > between them. When I started this journey I figured I could rely on the
> > > > > > of_graph between the DPU and the interface controllers, but the values
> > > > > > used there today are just bogus, so that was a no go.
> > > > > > 
> > > > > > We can use whatever, as long as _dpu_kms_initialize_displayport() can
> > > > > > come up with an identifier to put in h_tile_instance[0] so that
> > > > > > dpu_encoder_setup_display() can find the relevant INTF.
> > > > > > 
> > > > > 
> > > > > To make it more concrete we can look at sc7180
> > > > > 
> > > > > static const struct dpu_intf_cfg sc7180_intf[] = {
> > > > >          INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24,
> > > > > INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> > > > >                                                       ^
> > > > >                                                       |
> > > > > 
> > > > > intf0 is irrelevant. Also the address is irrelevant. But here we have a
> > > > > zero, the number after INTF_DP, and that is very relevant. That number
> > > > > needs to match the dp->id. Somewhere we have a match between
> > > > > controller_id and dp->id in the code.
> > > > 
> > > > That number (the 0, not INTF_0) is what the code matches against dp->id
> > > > in _dpu_kms_initialize_displayport(), in order to figure out that this
> > > > is INTF_0 in dpu_encoder_setup_display().
> > > > 
> > > > I.e. look at the sc8180x patch:
> > > > 
> > > > INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> > > > INTF_BLK("intf_1", INTF_1, 0x6A800, INTF_DSI, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> > > > INTF_BLK("intf_2", INTF_2, 0x6B000, INTF_DSI, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 28, 29),
> > > > /* INTF_3 is for MST, wired to INTF_DP 0 and 1, use dummy index until this is supported */
> > > > INTF_BLK("intf_3", INTF_3, 0x6B800, INTF_DP, 999, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 30, 31),
> > > > INTF_BLK("intf_4", INTF_4, 0x6C000, INTF_DP, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 20, 21),
> > > > INTF_BLK("intf_5", INTF_5, 0x6C800, INTF_DP, 2, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
> > > > 
> > > > Where the DP driver defines the 3 controllers with dp->id of 0, 1 and 2,
> > > > which the DPU code will match against to INTF_0, INTF_4 and INTF_5.
> > > > 
> > > 
> > > Yep. I'm saying that having to make that number in this intf array match
> > > the order of the register mapping descriptor array is fragile. Why can't
> > > we indicate the interface is DP or eDP with INTF_DP or INTF_EDP and then
> > > map from the descriptor array to this intf array somehow so that the
> > > order of the descriptor array doesn't matter? Then we don't have to put
> > > the connector type in the descriptor array, and we don't have to keep
> > > the order of the array a certain way to match this intf descriptor.
> > > 
> > > Maybe
> > > 
> > > 	struct msm_dp_desc {
> > > 		phys_addr_t io_start;
> > > 		unsigned int id;
> > 
> > The INTF_<N> constants are a property of the DPU driver and not
> > available in the DP driver and the msm_dp struct is a property of the DP
> > driver and can't be dereferenced in the DPU driver.
> > 
> > The proposed way around this is that the descs array defines the order
> > in priv->dp[N] and this N is used as controller_id.
> > 
> > 
> > So the only thing that I don't find straight forward here is that the
> > eDP controller is considered just a DP controller, so you have to use
> > INTF_DP, <N> for that, and not just INTF_EDP, 0.
> 
> Would it be better if we change the DPU driver to handle INTF_EDP too?
> 

I looked at that a while back and given that we can't look inside any of
the DP structs the only sensible solution I could come up with was to
create another array in struct msm_drm_private with "edp controllers".

But that means everywhere we today poke at priv->dp we need to also poke
at priv->edp. And the only gain is that we can say that the eDP
controller is INTF_EDP. And if there's ever a controller that could do
both, then that breaks down anyways.

Regards,
Bjorn

> > 
> > > 	};
> > > 
> > > and then have msm_dp_desc::id equal INTF_<N> and then look through the
> > > intf from DPU here in the DP driver to find the id and type of connector
> > > that should be used by default? Still sort of fragile because the only
> > > connection is an unsigned int which isn't great, but at least it's
> > > explicit instead of implicit based on the array order.
> > 
> > No matter how I look at this, you need to put some number somewhere here
> > that will be used to match up the INTF with the right DSI/DP encoder.
> > 
> > Using the proposed number scheme follows the numbering of all the DP
> > controllers from the documentation.
> > 
> > Regards,
> > Bjorn
> > 
> 
> 
> -- 
> With best wishes
> Dmitry

Re: [PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers

[Stephen Boyd]

Quoting Bjorn Andersson (2021-10-06 11:05:09)
> On Wed 06 Oct 10:19 PDT 2021, Stephen Boyd wrote:
>
> > Quoting Bjorn Andersson (2021-10-06 10:07:17)
> > > On Tue 05 Oct 21:26 PDT 2021, Stephen Boyd wrote:
> > >
> > > > Quoting Bjorn Andersson (2021-10-05 19:37:52)
> > > > > On Tue 05 Oct 19:06 PDT 2021, Stephen Boyd wrote:
> > > > >
> > > > > > Quoting Bjorn Andersson (2021-10-05 18:43:16)
> > > > > > > On Tue 05 Oct 17:43 PDT 2021, Stephen Boyd wrote:
> > > > > > >
> > > > > > > > Quoting Bjorn Andersson (2021-10-05 16:13:21)
> > > > > > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > > > > index bdaf227f05dc..674cddfee5b0 100644
> > > > > > > > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > > > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > > > > @@ -1233,7 +1239,7 @@ static int dp_display_probe(struct platform_device *pdev)
> > > > > > > > >         if (!dp)
> > > > > > > > >                 return -ENOMEM;
> > > > > > > > >
> > > > > > > > > -       desc = dp_display_get_desc(pdev);
> > > > > > > > > +       desc = dp_display_get_desc(pdev, &dp->id);
> > > > > > > >
> > > > > > > > I'm sad that dp->id has to match the number in the SoC specific
> > > > > > > > dpu_intf_cfg array in drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > > > > > > > still. Is there any way we can avoid that? Also, notice how those arrays
> > > > > > > > already have INTF_DP macros, which makes me think that it may be better
> > > > > > > > to connect this to those arrays instead of making an msm_dp_desc
> > > > > > > > structure and then make sure the 'type' member matches a connector
> > > > > > > > type number. Otherwise this code is super fragile.
> > > > > > > >
> > > > > > >
> > > > > > > I'm afraid I don't understand what you're proposing. Or which part you
> > > > > > > consider fragile, the indices of the INTF_DP instances aren't going to
> > > > > > > move around...
> > > > > > >
> > > > > > > I have N instances of the DP driver that I need to match to N entries
> > > > > > > from the platform specific intf array, I need some stable reference
> > > > > > > between them. When I started this journey I figured I could rely on the
> > > > > > > of_graph between the DPU and the interface controllers, but the values
> > > > > > > used there today are just bogus, so that was a no go.
> > > > > > >
> > > > > > > We can use whatever, as long as _dpu_kms_initialize_displayport() can
> > > > > > > come up with an identifier to put in h_tile_instance[0] so that
> > > > > > > dpu_encoder_setup_display() can find the relevant INTF.
> > > > > > >
> > > > > >
> > > > > > To make it more concrete we can look at sc7180
> > > > > >
> > > > > > static const struct dpu_intf_cfg sc7180_intf[] = {
> > > > > >         INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24,
> > > > > > INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> > > > > >                                                      ^
> > > > > >                                                      |
> > > > > >
> > > > > > intf0 is irrelevant. Also the address is irrelevant. But here we have a
> > > > > > zero, the number after INTF_DP, and that is very relevant. That number
> > > > > > needs to match the dp->id. Somewhere we have a match between
> > > > > > controller_id and dp->id in the code.
> > > > >
> > > > > That number (the 0, not INTF_0) is what the code matches against dp->id
> > > > > in _dpu_kms_initialize_displayport(), in order to figure out that this
> > > > > is INTF_0 in dpu_encoder_setup_display().
> > > > >
> > > > > I.e. look at the sc8180x patch:
> > > > >
> > > > > INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> > > > > INTF_BLK("intf_1", INTF_1, 0x6A800, INTF_DSI, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> > > > > INTF_BLK("intf_2", INTF_2, 0x6B000, INTF_DSI, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 28, 29),
> > > > > /* INTF_3 is for MST, wired to INTF_DP 0 and 1, use dummy index until this is supported */
> > > > > INTF_BLK("intf_3", INTF_3, 0x6B800, INTF_DP, 999, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 30, 31),
> > > > > INTF_BLK("intf_4", INTF_4, 0x6C000, INTF_DP, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 20, 21),
> > > > > INTF_BLK("intf_5", INTF_5, 0x6C800, INTF_DP, 2, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
> > > > >
> > > > > Where the DP driver defines the 3 controllers with dp->id of 0, 1 and 2,
> > > > > which the DPU code will match against to INTF_0, INTF_4 and INTF_5.
> > > > >
> > > >
> > > > Yep. I'm saying that having to make that number in this intf array match
> > > > the order of the register mapping descriptor array is fragile. Why can't
> > > > we indicate the interface is DP or eDP with INTF_DP or INTF_EDP and then
> > > > map from the descriptor array to this intf array somehow so that the
> > > > order of the descriptor array doesn't matter? Then we don't have to put
> > > > the connector type in the descriptor array, and we don't have to keep
> > > > the order of the array a certain way to match this intf descriptor.
> > > >
> > > > Maybe
> > > >
> > > >       struct msm_dp_desc {
> > > >               phys_addr_t io_start;
> > > >               unsigned int id;
> > >
> > > The INTF_<N> constants are a property of the DPU driver and not
> > > available in the DP driver and the msm_dp struct is a property of the DP
> > > driver and can't be dereferenced in the DPU driver.
> > >
> > > The proposed way around this is that the descs array defines the order
> > > in priv->dp[N] and this N is used as controller_id.
> >
> > I'm pretty sure I'm following along.
> >
> > >
> > > So the only thing that I don't find straight forward here is that the
> > > eDP controller is considered just a DP controller, so you have to use
> > > INTF_DP, <N> for that, and not just INTF_EDP, 0.
> > >
> > > >       };
> > > >
> > > > and then have msm_dp_desc::id equal INTF_<N> and then look through the
> > > > intf from DPU here in the DP driver to find the id and type of connector
> > > > that should be used by default? Still sort of fragile because the only
> > > > connection is an unsigned int which isn't great, but at least it's
> > > > explicit instead of implicit based on the array order.
> > >
> > > No matter how I look at this, you need to put some number somewhere here
> > > that will be used to match up the INTF with the right DSI/DP encoder.
> >
> > Correct.
> >
> > >
> > > Using the proposed number scheme follows the numbering of all the DP
> > > controllers from the documentation.
> > >
> >
> > Maybe I can make a better example. I have this for sc7280 in dpu_hw_catalog.c:
> >
> >       static const struct dpu_intf_cfg sc7280_intf[] = {
> >               INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, CONTROLLER_ID_A, 24,
> > INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> >               INTF_BLK("intf_1", INTF_1, 0x35000, INTF_DSI, 0, 24,
> > INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> >               INTF_BLK("intf_5", INTF_5, 0x39000, INTF_DP, CONTROLLER_ID_B, 24,
> > INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
> >       };
> >
> > And then this array for sc7280 in dp_display.c:
> >
> >       static const struct msm_dp_desc sc7280_dp_cfg = {
> >               .desc = {
> >                       [CONTROLLER_ID_A] = { 0xaea0000, DRM_MODE_CONNECTOR_eDP },
> >                       [CONTROLLER_ID_B] = { 0xae90000, DRM_MODE_CONNECTOR_DisplayPort },
> >               },
> >               .num_dp = 2,
> >       };
> >
> > So these two arrays must match based on CONTROLLER_ID_{A,B}. I don't
> > like having to make these two numbers match so if it was explicit, even
> > possibly by having a bunch of macros put in both places then I would be
> > happy. I spent a few hours when I messed up the order of the
> > sc7280_dp_cfg.desc array trying to figure out why things weren't
> > working.
>
> So essentially, you didn't know that the controller_id has to match the
> index in priv->dsi[] and priv->dp[] and providing a define for them
> would make this more obvious?

Now you got it!

>
> I think per your argument the 0 following INTF_DSI should also be using
> this scheme, so we'd have multiple CONTROLLER_ID_A, which probably is
> confusing as well.

Agreed.

>
> I tried it out with below patch; it documents the relationship, provides
> constants for the magic 2 and 3 for number of DSI and DP controllers in
> struct msm_drm_private.
>
> I like it.

Thanks. I prefer this approach as well. I can see now why qcom always
wants to change the output ports on the DPU node in DT to match the
INTF number. If they would have described this problem it may have made
sense to have the graph endpoints with reg properties matching the
interface number in the intf array. Sigh.

Re: [PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers

[Bjorn Andersson]

On Wed 06 Oct 11:59 PDT 2021, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2021-10-06 11:05:09)
> > On Wed 06 Oct 10:19 PDT 2021, Stephen Boyd wrote:
> >
> > > Quoting Bjorn Andersson (2021-10-06 10:07:17)
> > > > On Tue 05 Oct 21:26 PDT 2021, Stephen Boyd wrote:
> > > >
> > > > > Quoting Bjorn Andersson (2021-10-05 19:37:52)
> > > > > > On Tue 05 Oct 19:06 PDT 2021, Stephen Boyd wrote:
> > > > > >
> > > > > > > Quoting Bjorn Andersson (2021-10-05 18:43:16)
> > > > > > > > On Tue 05 Oct 17:43 PDT 2021, Stephen Boyd wrote:
> > > > > > > >
> > > > > > > > > Quoting Bjorn Andersson (2021-10-05 16:13:21)
> > > > > > > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > > > > > index bdaf227f05dc..674cddfee5b0 100644
> > > > > > > > > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > > > > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > > > > > @@ -1233,7 +1239,7 @@ static int dp_display_probe(struct platform_device *pdev)
> > > > > > > > > >         if (!dp)
> > > > > > > > > >                 return -ENOMEM;
> > > > > > > > > >
> > > > > > > > > > -       desc = dp_display_get_desc(pdev);
> > > > > > > > > > +       desc = dp_display_get_desc(pdev, &dp->id);
> > > > > > > > >
> > > > > > > > > I'm sad that dp->id has to match the number in the SoC specific
> > > > > > > > > dpu_intf_cfg array in drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > > > > > > > > still. Is there any way we can avoid that? Also, notice how those arrays
> > > > > > > > > already have INTF_DP macros, which makes me think that it may be better
> > > > > > > > > to connect this to those arrays instead of making an msm_dp_desc
> > > > > > > > > structure and then make sure the 'type' member matches a connector
> > > > > > > > > type number. Otherwise this code is super fragile.
> > > > > > > > >
> > > > > > > >
> > > > > > > > I'm afraid I don't understand what you're proposing. Or which part you
> > > > > > > > consider fragile, the indices of the INTF_DP instances aren't going to
> > > > > > > > move around...
> > > > > > > >
> > > > > > > > I have N instances of the DP driver that I need to match to N entries
> > > > > > > > from the platform specific intf array, I need some stable reference
> > > > > > > > between them. When I started this journey I figured I could rely on the
> > > > > > > > of_graph between the DPU and the interface controllers, but the values
> > > > > > > > used there today are just bogus, so that was a no go.
> > > > > > > >
> > > > > > > > We can use whatever, as long as _dpu_kms_initialize_displayport() can
> > > > > > > > come up with an identifier to put in h_tile_instance[0] so that
> > > > > > > > dpu_encoder_setup_display() can find the relevant INTF.
> > > > > > > >
> > > > > > >
> > > > > > > To make it more concrete we can look at sc7180
> > > > > > >
> > > > > > > static const struct dpu_intf_cfg sc7180_intf[] = {
> > > > > > >         INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24,
> > > > > > > INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> > > > > > >                                                      ^
> > > > > > >                                                      |
> > > > > > >
> > > > > > > intf0 is irrelevant. Also the address is irrelevant. But here we have a
> > > > > > > zero, the number after INTF_DP, and that is very relevant. That number
> > > > > > > needs to match the dp->id. Somewhere we have a match between
> > > > > > > controller_id and dp->id in the code.
> > > > > >
> > > > > > That number (the 0, not INTF_0) is what the code matches against dp->id
> > > > > > in _dpu_kms_initialize_displayport(), in order to figure out that this
> > > > > > is INTF_0 in dpu_encoder_setup_display().
> > > > > >
> > > > > > I.e. look at the sc8180x patch:
> > > > > >
> > > > > > INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> > > > > > INTF_BLK("intf_1", INTF_1, 0x6A800, INTF_DSI, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> > > > > > INTF_BLK("intf_2", INTF_2, 0x6B000, INTF_DSI, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 28, 29),
> > > > > > /* INTF_3 is for MST, wired to INTF_DP 0 and 1, use dummy index until this is supported */
> > > > > > INTF_BLK("intf_3", INTF_3, 0x6B800, INTF_DP, 999, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 30, 31),
> > > > > > INTF_BLK("intf_4", INTF_4, 0x6C000, INTF_DP, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 20, 21),
> > > > > > INTF_BLK("intf_5", INTF_5, 0x6C800, INTF_DP, 2, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
> > > > > >
> > > > > > Where the DP driver defines the 3 controllers with dp->id of 0, 1 and 2,
> > > > > > which the DPU code will match against to INTF_0, INTF_4 and INTF_5.
> > > > > >
> > > > >
> > > > > Yep. I'm saying that having to make that number in this intf array match
> > > > > the order of the register mapping descriptor array is fragile. Why can't
> > > > > we indicate the interface is DP or eDP with INTF_DP or INTF_EDP and then
> > > > > map from the descriptor array to this intf array somehow so that the
> > > > > order of the descriptor array doesn't matter? Then we don't have to put
> > > > > the connector type in the descriptor array, and we don't have to keep
> > > > > the order of the array a certain way to match this intf descriptor.
> > > > >
> > > > > Maybe
> > > > >
> > > > >       struct msm_dp_desc {
> > > > >               phys_addr_t io_start;
> > > > >               unsigned int id;
> > > >
> > > > The INTF_<N> constants are a property of the DPU driver and not
> > > > available in the DP driver and the msm_dp struct is a property of the DP
> > > > driver and can't be dereferenced in the DPU driver.
> > > >
> > > > The proposed way around this is that the descs array defines the order
> > > > in priv->dp[N] and this N is used as controller_id.
> > >
> > > I'm pretty sure I'm following along.
> > >
> > > >
> > > > So the only thing that I don't find straight forward here is that the
> > > > eDP controller is considered just a DP controller, so you have to use
> > > > INTF_DP, <N> for that, and not just INTF_EDP, 0.
> > > >
> > > > >       };
> > > > >
> > > > > and then have msm_dp_desc::id equal INTF_<N> and then look through the
> > > > > intf from DPU here in the DP driver to find the id and type of connector
> > > > > that should be used by default? Still sort of fragile because the only
> > > > > connection is an unsigned int which isn't great, but at least it's
> > > > > explicit instead of implicit based on the array order.
> > > >
> > > > No matter how I look at this, you need to put some number somewhere here
> > > > that will be used to match up the INTF with the right DSI/DP encoder.
> > >
> > > Correct.
> > >
> > > >
> > > > Using the proposed number scheme follows the numbering of all the DP
> > > > controllers from the documentation.
> > > >
> > >
> > > Maybe I can make a better example. I have this for sc7280 in dpu_hw_catalog.c:
> > >
> > >       static const struct dpu_intf_cfg sc7280_intf[] = {
> > >               INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, CONTROLLER_ID_A, 24,
> > > INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> > >               INTF_BLK("intf_1", INTF_1, 0x35000, INTF_DSI, 0, 24,
> > > INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> > >               INTF_BLK("intf_5", INTF_5, 0x39000, INTF_DP, CONTROLLER_ID_B, 24,
> > > INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
> > >       };
> > >
> > > And then this array for sc7280 in dp_display.c:
> > >
> > >       static const struct msm_dp_desc sc7280_dp_cfg = {
> > >               .desc = {
> > >                       [CONTROLLER_ID_A] = { 0xaea0000, DRM_MODE_CONNECTOR_eDP },
> > >                       [CONTROLLER_ID_B] = { 0xae90000, DRM_MODE_CONNECTOR_DisplayPort },
> > >               },
> > >               .num_dp = 2,
> > >       };
> > >
> > > So these two arrays must match based on CONTROLLER_ID_{A,B}. I don't
> > > like having to make these two numbers match so if it was explicit, even
> > > possibly by having a bunch of macros put in both places then I would be
> > > happy. I spent a few hours when I messed up the order of the
> > > sc7280_dp_cfg.desc array trying to figure out why things weren't
> > > working.
> >
> > So essentially, you didn't know that the controller_id has to match the
> > index in priv->dsi[] and priv->dp[] and providing a define for them
> > would make this more obvious?
> 
> Now you got it!
> 
> >
> > I think per your argument the 0 following INTF_DSI should also be using
> > this scheme, so we'd have multiple CONTROLLER_ID_A, which probably is
> > confusing as well.
> 
> Agreed.
> 
> >
> > I tried it out with below patch; it documents the relationship, provides
> > constants for the magic 2 and 3 for number of DSI and DP controllers in
> > struct msm_drm_private.
> >
> > I like it.
> 
> Thanks. I prefer this approach as well.

Sweet, I'll update my patch set accordingly.

> I can see now why qcom always wants to change the output ports on the
> DPU node in DT to match the INTF number. If they would have described
> this problem it may have made sense to have the graph endpoints with
> reg properties matching the interface number in the intf array. Sigh.

Yes, I think the supposed design is that you should use the of_graph and
then call of_find_possible_crtcs() to figure out your links.

Unfortunately that doesn't work with the design of the DPU driver,
because the crtcs doesn't represent the INTFs - and as you say, the
existing of_graphs are full of incorrect data.

Which also means that I don't know why we keep filling out the of_graph,
because afaict it's not used and it contains invalid information.

Thanks,
Bjorn

Re: [PATCH v4 5/7] drm/msm/dp: Support up to 3 DP controllers

[abhinavk]

Hi Bjorn and Stephen

On 2021-10-06 13:39, Bjorn Andersson wrote:
> On Wed 06 Oct 11:59 PDT 2021, Stephen Boyd wrote:
> 
>> Quoting Bjorn Andersson (2021-10-06 11:05:09)
>> > On Wed 06 Oct 10:19 PDT 2021, Stephen Boyd wrote:
>> >
>> > > Quoting Bjorn Andersson (2021-10-06 10:07:17)
>> > > > On Tue 05 Oct 21:26 PDT 2021, Stephen Boyd wrote:
>> > > >
>> > > > > Quoting Bjorn Andersson (2021-10-05 19:37:52)
>> > > > > > On Tue 05 Oct 19:06 PDT 2021, Stephen Boyd wrote:
>> > > > > >
>> > > > > > > Quoting Bjorn Andersson (2021-10-05 18:43:16)
>> > > > > > > > On Tue 05 Oct 17:43 PDT 2021, Stephen Boyd wrote:
>> > > > > > > >
>> > > > > > > > > Quoting Bjorn Andersson (2021-10-05 16:13:21)
>> > > > > > > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> > > > > > > > > > index bdaf227f05dc..674cddfee5b0 100644
>> > > > > > > > > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> > > > > > > > > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> > > > > > > > > > @@ -1233,7 +1239,7 @@ static int dp_display_probe(struct platform_device *pdev)
>> > > > > > > > > >         if (!dp)
>> > > > > > > > > >                 return -ENOMEM;
>> > > > > > > > > >
>> > > > > > > > > > -       desc = dp_display_get_desc(pdev);
>> > > > > > > > > > +       desc = dp_display_get_desc(pdev, &dp->id);
>> > > > > > > > >
>> > > > > > > > > I'm sad that dp->id has to match the number in the SoC specific
>> > > > > > > > > dpu_intf_cfg array in drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> > > > > > > > > still. Is there any way we can avoid that? Also, notice how those arrays
>> > > > > > > > > already have INTF_DP macros, which makes me think that it may be better
>> > > > > > > > > to connect this to those arrays instead of making an msm_dp_desc
>> > > > > > > > > structure and then make sure the 'type' member matches a connector
>> > > > > > > > > type number. Otherwise this code is super fragile.
>> > > > > > > > >
>> > > > > > > >
>> > > > > > > > I'm afraid I don't understand what you're proposing. Or which part you
>> > > > > > > > consider fragile, the indices of the INTF_DP instances aren't going to
>> > > > > > > > move around...
>> > > > > > > >
>> > > > > > > > I have N instances of the DP driver that I need to match to N entries
>> > > > > > > > from the platform specific intf array, I need some stable reference
>> > > > > > > > between them. When I started this journey I figured I could rely on the
>> > > > > > > > of_graph between the DPU and the interface controllers, but the values
>> > > > > > > > used there today are just bogus, so that was a no go.
>> > > > > > > >
>> > > > > > > > We can use whatever, as long as _dpu_kms_initialize_displayport() can
>> > > > > > > > come up with an identifier to put in h_tile_instance[0] so that
>> > > > > > > > dpu_encoder_setup_display() can find the relevant INTF.
>> > > > > > > >
>> > > > > > >
>> > > > > > > To make it more concrete we can look at sc7180
>> > > > > > >
>> > > > > > > static const struct dpu_intf_cfg sc7180_intf[] = {
>> > > > > > >         INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24,
>> > > > > > > INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
>> > > > > > >                                                      ^
>> > > > > > >                                                      |
>> > > > > > >
>> > > > > > > intf0 is irrelevant. Also the address is irrelevant. But here we have a
>> > > > > > > zero, the number after INTF_DP, and that is very relevant. That number
>> > > > > > > needs to match the dp->id. Somewhere we have a match between
>> > > > > > > controller_id and dp->id in the code.
>> > > > > >
>> > > > > > That number (the 0, not INTF_0) is what the code matches against dp->id
>> > > > > > in _dpu_kms_initialize_displayport(), in order to figure out that this
>> > > > > > is INTF_0 in dpu_encoder_setup_display().
>> > > > > >
>> > > > > > I.e. look at the sc8180x patch:
>> > > > > >
>> > > > > > INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
>> > > > > > INTF_BLK("intf_1", INTF_1, 0x6A800, INTF_DSI, 0, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
>> > > > > > INTF_BLK("intf_2", INTF_2, 0x6B000, INTF_DSI, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 28, 29),
>> > > > > > /* INTF_3 is for MST, wired to INTF_DP 0 and 1, use dummy index until this is supported */
>> > > > > > INTF_BLK("intf_3", INTF_3, 0x6B800, INTF_DP, 999, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 30, 31),
>> > > > > > INTF_BLK("intf_4", INTF_4, 0x6C000, INTF_DP, 1, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 20, 21),
>> > > > > > INTF_BLK("intf_5", INTF_5, 0x6C800, INTF_DP, 2, 24, INTF_SC8180X_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
>> > > > > >
>> > > > > > Where the DP driver defines the 3 controllers with dp->id of 0, 1 and 2,
>> > > > > > which the DPU code will match against to INTF_0, INTF_4 and INTF_5.
>> > > > > >
>> > > > >
>> > > > > Yep. I'm saying that having to make that number in this intf array match
>> > > > > the order of the register mapping descriptor array is fragile. Why can't
>> > > > > we indicate the interface is DP or eDP with INTF_DP or INTF_EDP and then
>> > > > > map from the descriptor array to this intf array somehow so that the
>> > > > > order of the descriptor array doesn't matter? Then we don't have to put
>> > > > > the connector type in the descriptor array, and we don't have to keep
>> > > > > the order of the array a certain way to match this intf descriptor.
>> > > > >
>> > > > > Maybe
>> > > > >
>> > > > >       struct msm_dp_desc {
>> > > > >               phys_addr_t io_start;
>> > > > >               unsigned int id;
>> > > >
>> > > > The INTF_<N> constants are a property of the DPU driver and not
>> > > > available in the DP driver and the msm_dp struct is a property of the DP
>> > > > driver and can't be dereferenced in the DPU driver.
>> > > >
>> > > > The proposed way around this is that the descs array defines the order
>> > > > in priv->dp[N] and this N is used as controller_id.
>> > >
>> > > I'm pretty sure I'm following along.
>> > >
>> > > >
>> > > > So the only thing that I don't find straight forward here is that the
>> > > > eDP controller is considered just a DP controller, so you have to use
>> > > > INTF_DP, <N> for that, and not just INTF_EDP, 0.
>> > > >
>> > > > >       };
>> > > > >
>> > > > > and then have msm_dp_desc::id equal INTF_<N> and then look through the
>> > > > > intf from DPU here in the DP driver to find the id and type of connector
>> > > > > that should be used by default? Still sort of fragile because the only
>> > > > > connection is an unsigned int which isn't great, but at least it's
>> > > > > explicit instead of implicit based on the array order.
>> > > >
>> > > > No matter how I look at this, you need to put some number somewhere here
>> > > > that will be used to match up the INTF with the right DSI/DP encoder.
>> > >
>> > > Correct.
>> > >
>> > > >
>> > > > Using the proposed number scheme follows the numbering of all the DP
>> > > > controllers from the documentation.
>> > > >
>> > >
>> > > Maybe I can make a better example. I have this for sc7280 in dpu_hw_catalog.c:
>> > >
>> > >       static const struct dpu_intf_cfg sc7280_intf[] = {
>> > >               INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, CONTROLLER_ID_A, 24,
>> > > INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
>> > >               INTF_BLK("intf_1", INTF_1, 0x35000, INTF_DSI, 0, 24,
>> > > INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
>> > >               INTF_BLK("intf_5", INTF_5, 0x39000, INTF_DP, CONTROLLER_ID_B, 24,
>> > > INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
>> > >       };
>> > >
>> > > And then this array for sc7280 in dp_display.c:
>> > >
>> > >       static const struct msm_dp_desc sc7280_dp_cfg = {
>> > >               .desc = {
>> > >                       [CONTROLLER_ID_A] = { 0xaea0000, DRM_MODE_CONNECTOR_eDP },
>> > >                       [CONTROLLER_ID_B] = { 0xae90000, DRM_MODE_CONNECTOR_DisplayPort },
>> > >               },
>> > >               .num_dp = 2,
>> > >       };
>> > >
>> > > So these two arrays must match based on CONTROLLER_ID_{A,B}. I don't
>> > > like having to make these two numbers match so if it was explicit, even
>> > > possibly by having a bunch of macros put in both places then I would be
>> > > happy. I spent a few hours when I messed up the order of the
>> > > sc7280_dp_cfg.desc array trying to figure out why things weren't
>> > > working.
>> >
>> > So essentially, you didn't know that the controller_id has to match the
>> > index in priv->dsi[] and priv->dp[] and providing a define for them
>> > would make this more obvious?
>> 
>> Now you got it!
>> 
>> >
>> > I think per your argument the 0 following INTF_DSI should also be using
>> > this scheme, so we'd have multiple CONTROLLER_ID_A, which probably is
>> > confusing as well.
>> 
>> Agreed.
>> 
>> >
>> > I tried it out with below patch; it documents the relationship, provides
>> > constants for the magic 2 and 3 for number of DSI and DP controllers in
>> > struct msm_drm_private.
>> >
>> > I like it.
>> 
>> Thanks. I prefer this approach as well.
> 
> Sweet, I'll update my patch set accordingly.
Yes, I also agree with this approach to better document the relationship 
between the hw_catalog array
and the dp_display msm_dp_desc

> 
>> I can see now why qcom always wants to change the output ports on the
>> DPU node in DT to match the INTF number. If they would have described
>> this problem it may have made sense to have the graph endpoints with
>> reg properties matching the interface number in the intf array. Sigh.
> 
> Yes, I think the supposed design is that you should use the of_graph 
> and
> then call of_find_possible_crtcs() to figure out your links.
> 
> Unfortunately that doesn't work with the design of the DPU driver,
> because the crtcs doesn't represent the INTFs - and as you say, the
> existing of_graphs are full of incorrect data.
> 
> Which also means that I don't know why we keep filling out the 
> of_graph,
> because afaict it's not used and it contains invalid information.
> 
> Thanks,
> Bjorn

Re: [Freedreno] [PATCH v4 4/7] drm/msm/dp: Allow attaching a drm_panel

[Doug Anderson]

Hi,

On Tue, Oct 5, 2021 at 5:35 PM <abhinavk@codeaurora.org> wrote:
>
> > +     parser->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> > +     if (IS_ERR(parser->panel_bridge)) {
> > +             DRM_ERROR("failed to create panel bridge\n");
> > +             return PTR_ERR(parser->panel_bridge);
> > +     }
>
> When we add a bridge using devm_drm_panel_bridge_add(), it will register
> with default bridge functions which is fine
> because we need the panel power to be controlled here.
>
>
> 140 static const struct drm_bridge_funcs panel_bridge_bridge_funcs = {
> 141     .attach = panel_bridge_attach,
> 142     .detach = panel_bridge_detach,
> 143     .pre_enable = panel_bridge_pre_enable,
> 144     .enable = panel_bridge_enable,
> 145     .disable = panel_bridge_disable,
> 146     .post_disable = panel_bridge_post_disable,
> 147     .get_modes = panel_bridge_get_modes,
>
> But what about the EDID related things, the DP/eDP driver already reads
> the EDID and gets the modes so we need to skip
> that in this case as otherwise it will end up calling the
> panel_get_modes in the eDP panel which will be redundant.
>
> Let me know if I am missing something in this proposal.

I haven't followed all the discussion of this patch series, but I've
been keenly involved in the EDID problem in ti-sn65dsi86. What we
ended up settling on for the TI bridge chip (ti-sn65dsi86.c) is:

1. If you have a panel, the panel driver is in charge of EDID reading.
This allows the panel to power itself up before it tries to read the
EDID.

2. The EDP controller is in charge of providing the DP AUX bus to the
panel driver.

3. If the EDP controller needs to be powered up for DP AUX bus reads
to happen, it should power itself up in the AUX bus transfer function
and use "autosuspend" to keep from turning off and on constantly.


The above not only solves the EDID problem but also allows the generic
eDP-panel code to work and also allows panels with DP AUX backlight
control to work.


For short term "getting something working", though, you could just
rely on a hardcoded mode in the panel driver for now and forget about
trying to read the EDID case for eDP.


NOTE: At the moment I think if you don't have a panel you should
continue to read the EDID in the DP driver. That could always be
changed in the future, but it was what Laurent was pushing for for
ti-sn65dsi86.c.


-Doug