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

[PATCH v3 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 (5):
  drm/msm/dp: Remove global g_dp_display variable
  drm/msm/dp: Modify prototype of encoder based API
  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           | 131 +++++++++---------
 drivers/gpu/drm/msm/msm_drv.h                 |   4 +-
 6 files changed, 132 insertions(+), 102 deletions(-)

-- 
2.29.2

[PATCH v3 1/5] 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: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v2:
- 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 v3 2/5] 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: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v2:
- 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 v3 3/5] 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 v2:
- Added MSM_DRM_DP_COUNT to link the two 3s
- Moved NULL check for msm_dp_debugfs_init() to the call site
- Made struct dp_display_private->id unsigned

I also implemented added connector_type to each of the DP instances and
propagated this to dp_drm_connector_init() but later dropped this again per
Doug's suggestion that we'll base this on the presence/absence of a associated
drm bridge or panel.

 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           | 44 ++++++++++++-
 drivers/gpu/drm/msm/msm_drv.h                 |  4 +-
 5 files changed, 90 insertions(+), 34 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 5d3ee5ef07c2..ff3477474c5d 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -78,6 +78,8 @@ struct dp_display_private {
 	char *name;
 	int irq;
 
+	unsigned int id;
+
 	/* state variables */
 	bool core_initialized;
 	bool hpd_irq_on;
@@ -115,8 +117,18 @@ struct dp_display_private {
 	struct dp_audio *audio;
 };
 
+struct msm_dp_config {
+	phys_addr_t io_start[MSM_DRM_DP_COUNT];
+	size_t num_descs;
+};
+
+static const struct msm_dp_config sc7180_dp_cfg = {
+	.io_start = { 0x0ae90000 },
+	.num_descs = 1,
+};
+
 static const struct of_device_id dp_dt_match[] = {
-	{.compatible = "qcom,sc7180-dp"},
+	{ .compatible = "qcom,sc7180-dp", .data = &sc7180_dp_cfg },
 	{}
 };
 
@@ -211,7 +223,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);
 	if (rc) {
@@ -249,7 +261,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 = {
@@ -1180,10 +1192,31 @@ int dp_display_request_irq(struct msm_dp *dp_display)
 	return 0;
 }
 
+static int dp_display_find_id(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 -EINVAL;
+
+	for (i = 0; i < cfg->num_descs; i++) {
+		if (cfg->io_start[i] == res->start)
+			return i;
+	}
+
+	dev_err(&pdev->dev, "unknown displayport instance\n");
+	return -EINVAL;
+}
+
 static int dp_display_probe(struct platform_device *pdev)
 {
 	int rc = 0;
 	struct dp_display_private *dp;
+	int id;
 
 	if (!pdev || !pdev->dev.of_node) {
 		DRM_ERROR("pdev not found\n");
@@ -1194,8 +1227,13 @@ static int dp_display_probe(struct platform_device *pdev)
 	if (!dp)
 		return -ENOMEM;
 
+	id = dp_display_find_id(pdev);
+	if (id < 0)
+		return id;
+
 	dp->pdev = pdev;
 	dp->name = "drm_dp";
+	dp->id = id;
 
 	rc = dp_init_sub_modules(dp);
 	if (rc) {
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 8b005d1ac899..b20a6dd221f7 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -135,6 +135,8 @@ struct msm_drm_thread {
 	struct kthread_worker *worker;
 };
 
+#define MSM_DRM_DP_COUNT	3
+
 struct msm_drm_private {
 
 	struct drm_device *dev;
@@ -161,7 +163,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[MSM_DRM_DP_COUNT];
 
 	/* when we have more than one 'msm_gpu' these need to be an array: */
 	struct msm_gpu *gpu;
-- 
2.29.2

[PATCH v3 4/5] 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: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v2:
- 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 v3 5/5] 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.

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

Changes since v2:
- None

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

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index ff3477474c5d..56a79aeffed4 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -127,8 +127,15 @@ static const struct msm_dp_config sc7180_dp_cfg = {
 	.num_descs = 1,
 };
 
+static const struct msm_dp_config sc8180x_dp_cfg = {
+	.io_start = { 0xae90000, 0xae98000, 0xae9a000 },
+	.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 v3 3/5] drm/msm/dp: Support up to 3 DP controllers

[Stephen Boyd]

Quoting Bjorn Andersson (2021-10-01 11:00:56)
> 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>
> ---

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

Some nits below.

>
> Changes since v2:
> - Added MSM_DRM_DP_COUNT to link the two 3s
> - Moved NULL check for msm_dp_debugfs_init() to the call site
> - Made struct dp_display_private->id unsigned
>
> I also implemented added connector_type to each of the DP instances and
> propagated this to dp_drm_connector_init() but later dropped this again per
> Doug's suggestion that we'll base this on the presence/absence of a associated
> drm bridge or panel.

Sad but OK. We can take up that topic in another patch.

> 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
> @@ -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);

This seems to cause a bunch of debugfs warnings when there are multiple
nodes created with the same name.

> +       }
>
>         return dpu_core_perf_debugfs_init(dpu_kms, entry);
>  }
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 5d3ee5ef07c2..ff3477474c5d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1180,10 +1192,31 @@ int dp_display_request_irq(struct msm_dp *dp_display)
>         return 0;
>  }
>
> +static int dp_display_find_id(struct platform_device *pdev)
> +{
> +       const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
> +       struct resource *res;
> +       int i;
> +
> +

Nitpick: Remove a newline here.

> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!res)
> +               return -EINVAL;
> +
> +       for (i = 0; i < cfg->num_descs; i++) {
> +               if (cfg->io_start[i] == res->start)
> +                       return i;
> +       }

Nitpick: Drop braces on single line if inside for loop.

> +
> +       dev_err(&pdev->dev, "unknown displayport instance\n");
> +       return -EINVAL;
> +}
> +

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

[Bjorn Andersson]

On Mon 04 Oct 17:58 PDT 2021, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2021-10-01 11:00:56)
> > 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>
> > ---
> 
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> 
> Some nits below.
> 
> >
> > Changes since v2:
> > - Added MSM_DRM_DP_COUNT to link the two 3s
> > - Moved NULL check for msm_dp_debugfs_init() to the call site
> > - Made struct dp_display_private->id unsigned
> >
> > I also implemented added connector_type to each of the DP instances and
> > propagated this to dp_drm_connector_init() but later dropped this again per
> > Doug's suggestion that we'll base this on the presence/absence of a associated
> > drm bridge or panel.
> 
> Sad but OK. We can take up that topic in another patch.
> 

So you don't agree with the solution from sn65dsi86?

The only reason I haven't yet send this other patch is the of_graph
thing Doug an I are discussing on the RFC. But if we agree to base this
on compatible we could decide to look only for panels for the edp
instances and avoid that problem...

We would however never be able to describe the USB-less DP instance with
a panel explicitly described in DT going that route.

> > 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
> > @@ -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);
> 
> This seems to cause a bunch of debugfs warnings when there are multiple
> nodes created with the same name.
> 

Yes, that's true. I have a half-baked follow up that attempts to create
instance-specific debugfs directories. Can we take that in a follow up?

> > +       }
> >
> >         return dpu_core_perf_debugfs_init(dpu_kms, entry);
> >  }
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > index 5d3ee5ef07c2..ff3477474c5d 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -1180,10 +1192,31 @@ int dp_display_request_irq(struct msm_dp *dp_display)
> >         return 0;
> >  }
> >
> > +static int dp_display_find_id(struct platform_device *pdev)
> > +{
> > +       const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
> > +       struct resource *res;
> > +       int i;
> > +
> > +
> 
> Nitpick: Remove a newline here.
> 
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       if (!res)
> > +               return -EINVAL;
> > +
> > +       for (i = 0; i < cfg->num_descs; i++) {
> > +               if (cfg->io_start[i] == res->start)
> > +                       return i;
> > +       }
> 
> Nitpick: Drop braces on single line if inside for loop.
> 

Not when the loop spans multiple lines?

> > +
> > +       dev_err(&pdev->dev, "unknown displayport instance\n");
> > +       return -EINVAL;
> > +}
> > +

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

[Stephen Boyd]

Quoting Bjorn Andersson (2021-10-04 18:15:20)
> On Mon 04 Oct 17:58 PDT 2021, Stephen Boyd wrote:
>
> > Quoting Bjorn Andersson (2021-10-01 11:00:56)
> > > 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>
> > > ---
> >
> > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> >
> > Some nits below.
> >
> > >
> > > Changes since v2:
> > > - Added MSM_DRM_DP_COUNT to link the two 3s
> > > - Moved NULL check for msm_dp_debugfs_init() to the call site
> > > - Made struct dp_display_private->id unsigned
> > >
> > > I also implemented added connector_type to each of the DP instances and
> > > propagated this to dp_drm_connector_init() but later dropped this again per
> > > Doug's suggestion that we'll base this on the presence/absence of a associated
> > > drm bridge or panel.
> >
> > Sad but OK. We can take up that topic in another patch.
> >
>
> So you don't agree with the solution from sn65dsi86?
>
> The only reason I haven't yet send this other patch is the of_graph
> thing Doug an I are discussing on the RFC. But if we agree to base this
> on compatible we could decide to look only for panels for the edp
> instances and avoid that problem...
>
> We would however never be able to describe the USB-less DP instance with
> a panel explicitly described in DT going that route.

I'll reply on that thread.

>
> > > 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
> > > @@ -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);
> >
> > This seems to cause a bunch of debugfs warnings when there are multiple
> > nodes created with the same name.
> >
>
> Yes, that's true. I have a half-baked follow up that attempts to create
> instance-specific debugfs directories. Can we take that in a follow up?

Sure.

>
> > > +       }
> > >
> > >         return dpu_core_perf_debugfs_init(dpu_kms, entry);
> > >  }
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > > index 5d3ee5ef07c2..ff3477474c5d 100644
> > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > @@ -1180,10 +1192,31 @@ int dp_display_request_irq(struct msm_dp *dp_display)
> > >         return 0;
> > >  }
> > >
> > > +static int dp_display_find_id(struct platform_device *pdev)
> > > +{
> > > +       const struct msm_dp_config *cfg = of_device_get_match_data(&pdev->dev);
> > > +       struct resource *res;
> > > +       int i;
> > > +
> > > +
> >
> > Nitpick: Remove a newline here.
> >
> > > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +       if (!res)
> > > +               return -EINVAL;
> > > +
> > > +       for (i = 0; i < cfg->num_descs; i++) {
> > > +               if (cfg->io_start[i] == res->start)
> > > +                       return i;
> > > +       }
> >
> > Nitpick: Drop braces on single line if inside for loop.
> >
>
> Not when the loop spans multiple lines?

Kernel style is to remove braces from single "statement" for loops where
in this case the statement is the if condition.

>
> > > +
> > > +       dev_err(&pdev->dev, "unknown displayport instance\n");
> > > +       return -EINVAL;
> > > +}
> > > +

Re: [Freedreno] [PATCH v3 1/5] drm/msm/dp: Remove global g_dp_display variable

[abhinavk]

On 2021-10-01 11:00, Bjorn Andersson wrote:
> 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: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
> 
> Changes since v2:
> - 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,

Re: [Freedreno] [PATCH v3 2/5] drm/msm/dp: Modify prototype of encoder based API

[abhinavk]

On 2021-10-01 11:00, Bjorn Andersson wrote:
> 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: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
> 
> Changes since v2:
> - 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);

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

[abhinavk]

On 2021-10-01 11:00, Bjorn Andersson wrote:
> 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 v2:
> - Added MSM_DRM_DP_COUNT to link the two 3s
> - Moved NULL check for msm_dp_debugfs_init() to the call site
> - Made struct dp_display_private->id unsigned
> 
> I also implemented added connector_type to each of the DP instances and
> propagated this to dp_drm_connector_init() but later dropped this again 
> per
> Doug's suggestion that we'll base this on the presence/absence of a 
> associated
> drm bridge or panel.
> 
>  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           | 44 ++++++++++++-
>  drivers/gpu/drm/msm/msm_drv.h                 |  4 +-
>  5 files changed, 90 insertions(+), 34 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]];

At this point this is a nit but not sure if this is right to use the 
tile_instance as the index.
In the future if we chose to assign another index to the 
h_tile_instance, this would break.
But I cant think of the use-case for that yet, Hence
Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>

> 
>  	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 5d3ee5ef07c2..ff3477474c5d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -78,6 +78,8 @@ struct dp_display_private {
>  	char *name;
>  	int irq;
> 
> +	unsigned int id;
> +
>  	/* state variables */
>  	bool core_initialized;
>  	bool hpd_irq_on;
> @@ -115,8 +117,18 @@ struct dp_display_private {
>  	struct dp_audio *audio;
>  };
> 
> +struct msm_dp_config {
> +	phys_addr_t io_start[MSM_DRM_DP_COUNT];
> +	size_t num_descs;
> +};
> +
> +static const struct msm_dp_config sc7180_dp_cfg = {
> +	.io_start = { 0x0ae90000 },
> +	.num_descs = 1,
> +};
> +
>  static const struct of_device_id dp_dt_match[] = {
> -	{.compatible = "qcom,sc7180-dp"},
> +	{ .compatible = "qcom,sc7180-dp", .data = &sc7180_dp_cfg },
>  	{}
>  };
> 
> @@ -211,7 +223,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);
>  	if (rc) {
> @@ -249,7 +261,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 = {
> @@ -1180,10 +1192,31 @@ int dp_display_request_irq(struct msm_dp 
> *dp_display)
>  	return 0;
>  }
> 
> +static int dp_display_find_id(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 -EINVAL;
> +
> +	for (i = 0; i < cfg->num_descs; i++) {
> +		if (cfg->io_start[i] == res->start)
> +			return i;
> +	}
> +
> +	dev_err(&pdev->dev, "unknown displayport instance\n");
> +	return -EINVAL;
> +}
> +
>  static int dp_display_probe(struct platform_device *pdev)
>  {
>  	int rc = 0;
>  	struct dp_display_private *dp;
> +	int id;
> 
>  	if (!pdev || !pdev->dev.of_node) {
>  		DRM_ERROR("pdev not found\n");
> @@ -1194,8 +1227,13 @@ static int dp_display_probe(struct 
> platform_device *pdev)
>  	if (!dp)
>  		return -ENOMEM;
> 
> +	id = dp_display_find_id(pdev);
> +	if (id < 0)
> +		return id;
> +
>  	dp->pdev = pdev;
>  	dp->name = "drm_dp";
> +	dp->id = id;
> 
>  	rc = dp_init_sub_modules(dp);
>  	if (rc) {
> diff --git a/drivers/gpu/drm/msm/msm_drv.h 
> b/drivers/gpu/drm/msm/msm_drv.h
> index 8b005d1ac899..b20a6dd221f7 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -135,6 +135,8 @@ struct msm_drm_thread {
>  	struct kthread_worker *worker;
>  };
> 
> +#define MSM_DRM_DP_COUNT	3
> +
>  struct msm_drm_private {
> 
>  	struct drm_device *dev;
> @@ -161,7 +163,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[MSM_DRM_DP_COUNT];
> 
>  	/* when we have more than one 'msm_gpu' these need to be an array: */
>  	struct msm_gpu *gpu;

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

[abhinavk]

On 2021-10-05 13:16, abhinavk@codeaurora.org wrote:
> On 2021-10-01 11:00, Bjorn Andersson wrote:
>> 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 v2:
>> - Added MSM_DRM_DP_COUNT to link the two 3s
>> - Moved NULL check for msm_dp_debugfs_init() to the call site
>> - Made struct dp_display_private->id unsigned
>> 
>> I also implemented added connector_type to each of the DP instances 
>> and
>> propagated this to dp_drm_connector_init() but later dropped this 
>> again per
>> Doug's suggestion that we'll base this on the presence/absence of a 
>> associated
>> drm bridge or panel.

Hi Bjorn / Doug

I suggest we add the dp_drm_connector_init() part back to this change.
Having the same connector type for DP and eDP is an error and instead of
blocking/pending it on a potential RFC which attaches a panel to DP,
I would rather do it right here. So till thats done, I would retract my
R-B.

Thanks

Abhinav


>> 
>>  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           | 44 ++++++++++++-
>>  drivers/gpu/drm/msm/msm_drv.h                 |  4 +-
>>  5 files changed, 90 insertions(+), 34 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]];
> 
> At this point this is a nit but not sure if this is right to use the
> tile_instance as the index.
> In the future if we chose to assign another index to the
> h_tile_instance, this would break.
> But I cant think of the use-case for that yet.

> 
>> 
>>  	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 5d3ee5ef07c2..ff3477474c5d 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -78,6 +78,8 @@ struct dp_display_private {
>>  	char *name;
>>  	int irq;
>> 
>> +	unsigned int id;
>> +
>>  	/* state variables */
>>  	bool core_initialized;
>>  	bool hpd_irq_on;
>> @@ -115,8 +117,18 @@ struct dp_display_private {
>>  	struct dp_audio *audio;
>>  };
>> 
>> +struct msm_dp_config {
>> +	phys_addr_t io_start[MSM_DRM_DP_COUNT];
>> +	size_t num_descs;
>> +};
>> +
>> +static const struct msm_dp_config sc7180_dp_cfg = {
>> +	.io_start = { 0x0ae90000 },
>> +	.num_descs = 1,
>> +};
>> +
>>  static const struct of_device_id dp_dt_match[] = {
>> -	{.compatible = "qcom,sc7180-dp"},
>> +	{ .compatible = "qcom,sc7180-dp", .data = &sc7180_dp_cfg },
>>  	{}
>>  };
>> 
>> @@ -211,7 +223,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);
>>  	if (rc) {
>> @@ -249,7 +261,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 = {
>> @@ -1180,10 +1192,31 @@ int dp_display_request_irq(struct msm_dp 
>> *dp_display)
>>  	return 0;
>>  }
>> 
>> +static int dp_display_find_id(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 -EINVAL;
>> +
>> +	for (i = 0; i < cfg->num_descs; i++) {
>> +		if (cfg->io_start[i] == res->start)
>> +			return i;
>> +	}
>> +
>> +	dev_err(&pdev->dev, "unknown displayport instance\n");
>> +	return -EINVAL;
>> +}
>> +
>>  static int dp_display_probe(struct platform_device *pdev)
>>  {
>>  	int rc = 0;
>>  	struct dp_display_private *dp;
>> +	int id;
>> 
>>  	if (!pdev || !pdev->dev.of_node) {
>>  		DRM_ERROR("pdev not found\n");
>> @@ -1194,8 +1227,13 @@ static int dp_display_probe(struct 
>> platform_device *pdev)
>>  	if (!dp)
>>  		return -ENOMEM;
>> 
>> +	id = dp_display_find_id(pdev);
>> +	if (id < 0)
>> +		return id;
>> +
>>  	dp->pdev = pdev;
>>  	dp->name = "drm_dp";
>> +	dp->id = id;
>> 
>>  	rc = dp_init_sub_modules(dp);
>>  	if (rc) {
>> diff --git a/drivers/gpu/drm/msm/msm_drv.h 
>> b/drivers/gpu/drm/msm/msm_drv.h
>> index 8b005d1ac899..b20a6dd221f7 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.h
>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>> @@ -135,6 +135,8 @@ struct msm_drm_thread {
>>  	struct kthread_worker *worker;
>>  };
>> 
>> +#define MSM_DRM_DP_COUNT	3
>> +
>>  struct msm_drm_private {
>> 
>>  	struct drm_device *dev;
>> @@ -161,7 +163,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[MSM_DRM_DP_COUNT];
>> 
>>  	/* when we have more than one 'msm_gpu' these need to be an array: 
>> */
>>  	struct msm_gpu *gpu;

Re: [PATCH v3 4/5] dt-bindings: msm/dp: Add SC8180x compatibles

[abhinavk]

On 2021-10-01 11:00, Bjorn Andersson wrote:
> The Qualcomm SC8180x has 2 DP controllers and 1 eDP controller, add
> compatibles for these to the msm/dp binding.
> 
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
> 
> Changes since v2:
> - 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:

Re: [Freedreno] [PATCH v3 5/5] drm/msm/dp: Add sc8180x DP controllers

[abhinavk]

On 2021-10-01 11:00, Bjorn Andersson wrote:
> The sc8180x has 2 DP and 1 eDP controllers, add support for these to 
> the
> DP driver.
> 
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
> 
> Changes since v2:
> - None
> 
>  drivers/gpu/drm/msm/dp/dp_display.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index ff3477474c5d..56a79aeffed4 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -127,8 +127,15 @@ static const struct msm_dp_config sc7180_dp_cfg = 
> {
>  	.num_descs = 1,
>  };
> 
> +static const struct msm_dp_config sc8180x_dp_cfg = {
> +	.io_start = { 0xae90000, 0xae98000, 0xae9a000 },
> +	.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 },
>  	{}
>  };