linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x
@ 2021-07-25  4:24 Bjorn Andersson
  2021-07-25  4:24 ` [PATCH 1/5] drm/msm/dp: Remove global g_dp_display variable Bjorn Andersson
                   ` (6 more replies)
  0 siblings, 7 replies; 16+ messages in thread
From: Bjorn Andersson @ 2021-07-25  4:24 UTC (permalink / raw)
  To: Rob Clark, Sean Paul, David Airlie, Daniel Vetter, Abhinav Kumar,
	Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh
  Cc: Rob Herring, Stephen Boyd, linux-arm-msm, dri-devel, freedreno,
	linux-kernel

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.

This is based on v4 of Dmitry's work on multiple DSI interfaces:
https://lore.kernel.org/linux-arm-msm/20210717124016.316020-1-dmitry.baryshkov@linaro.org/

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   |  17 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |  60 +++---
 .../gpu/drm/msm/disp/msm_disp_snapshot_util.c |   8 +-
 drivers/gpu/drm/msm/dp/dp_display.c           | 183 +++++++++++++-----
 drivers/gpu/drm/msm/msm_drv.h                 |  33 ++--
 6 files changed, 200 insertions(+), 103 deletions(-)

-- 
2.29.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/5] drm/msm/dp: Remove global g_dp_display variable
  2021-07-25  4:24 [PATCH 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x Bjorn Andersson
@ 2021-07-25  4:24 ` Bjorn Andersson
  2021-07-26 23:11   ` Stephen Boyd
                     ` (2 more replies)
  2021-07-25  4:24 ` [PATCH 2/5] drm/msm/dp: Modify prototype of encoder based API Bjorn Andersson
                   ` (5 subsequent siblings)
  6 siblings, 3 replies; 16+ messages in thread
From: Bjorn Andersson @ 2021-07-25  4:24 UTC (permalink / raw)
  To: Rob Clark, Sean Paul, David Airlie, Daniel Vetter, Abhinav Kumar,
	Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh
  Cc: Rob Herring, Stephen Boyd, linux-arm-msm, dri-devel, freedreno,
	linux-kernel

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.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 78 ++++++++++++++---------------
 1 file changed, 37 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 70b319a8fe83..8696b36d30e4 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 {
@@ -122,6 +121,13 @@ static const struct of_device_id dp_dt_match[] = {
 	{}
 };
 
+static struct dp_display_private *dev_to_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)
 {
@@ -198,14 +204,16 @@ static int dp_display_bind(struct device *dev, struct device *master,
 			   void *data)
 {
 	int rc = 0;
-	struct dp_display_private *dp;
+	struct dp_display_private *dp = dev_to_dp_display_private(dev);
 	struct drm_device *drm;
 	struct msm_drm_private *priv;
 
 	drm = dev_get_drvdata(master);
 
-	dp = container_of(g_dp_display,
-			struct dp_display_private, dp_display);
+	if (!dp) {
+		DRM_ERROR("DP driver bind failed. Invalid driver data\n");
+		return -EINVAL;
+	}
 
 	dp->dp_display.drm_dev = drm;
 	priv = drm->dev_private;
@@ -240,12 +248,14 @@ 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_to_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);
+	if (!dp) {
+		DRM_ERROR("Invalid DP driver data\n");
+		return;
+	}
 
 	dp_power_client_deinit(dp->power);
 	dp_aux_unregister(dp->aux);
@@ -376,17 +386,14 @@ 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;
+	struct dp_display_private *dp = dev_to_dp_display_private(dev);
 
-	if (!dev) {
-		DRM_ERROR("invalid dev\n");
-		rc = -EINVAL;
+	if (!dp) {
+		DRM_ERROR("no driver data found\n");
+		rc = -ENODEV;
 		goto end;
 	}
 
-	dp = container_of(g_dp_display,
-			struct dp_display_private, dp_display);
-
 	dp_display_host_init(dp, false);
 
 	rc = dp_display_process_hpd_high(dp);
@@ -397,17 +404,14 @@ static int dp_display_usbpd_configure_cb(struct device *dev)
 static int dp_display_usbpd_disconnect_cb(struct device *dev)
 {
 	int rc = 0;
-	struct dp_display_private *dp;
+	struct dp_display_private *dp = dev_to_dp_display_private(dev);
 
-	if (!dev) {
-		DRM_ERROR("invalid dev\n");
-		rc = -EINVAL;
+	if (!dp) {
+		DRM_ERROR("no driver data found\n");
+		rc = -ENODEV;
 		return rc;
 	}
 
-	dp = container_of(g_dp_display,
-			struct dp_display_private, dp_display);
-
 	dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
 
 	return rc;
@@ -466,15 +470,15 @@ static int dp_display_usbpd_attention_cb(struct device *dev)
 {
 	int rc = 0;
 	u32 sink_request;
-	struct dp_display_private *dp;
+	struct dp_display_private *dp = dev_to_dp_display_private(dev);
+	struct dp_usbpd *hpd;
 
-	if (!dev) {
-		DRM_ERROR("invalid dev\n");
-		return -EINVAL;
+	if (!dp) {
+		DRM_ERROR("no driver data found\n");
+		return -ENODEV;
 	}
 
-	dp = container_of(g_dp_display,
-			struct dp_display_private, dp_display);
+	hpd = dp->usbpd;
 
 	/* check for any test request issued by sink */
 	rc = dp_link_process_request(dp->link);
@@ -638,7 +642,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
 	dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
 
 	/* 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);
@@ -832,9 +836,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;
 
 	if (dp_display->power_on) {
 		DRM_DEBUG_DP("Link already setup, return\n");
@@ -869,9 +871,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;
@@ -1229,14 +1229,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) {
@@ -1249,10 +1248,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 = platform_get_drvdata(pdev);
 
 	dp_display_deinit_sub_modules(dp);
 
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/5] drm/msm/dp: Modify prototype of encoder based API
  2021-07-25  4:24 [PATCH 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x Bjorn Andersson
  2021-07-25  4:24 ` [PATCH 1/5] drm/msm/dp: Remove global g_dp_display variable Bjorn Andersson
@ 2021-07-25  4:24 ` Bjorn Andersson
  2021-07-26 23:55   ` Stephen Boyd
  2021-07-27 18:42   ` Dmitry Baryshkov
  2021-07-25  4:24 ` [PATCH 3/5] drm/msm/dp: Support up to 3 DP controllers Bjorn Andersson
                   ` (4 subsequent siblings)
  6 siblings, 2 replies; 16+ messages in thread
From: Bjorn Andersson @ 2021-07-25  4:24 UTC (permalink / raw)
  To: Rob Clark, Sean Paul, David Airlie, Daniel Vetter, Abhinav Kumar,
	Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh
  Cc: Rob Herring, Stephen Boyd, linux-arm-msm, dri-devel, freedreno,
	linux-kernel

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.

The information for doing this lookup is available inside the DP driver,
so update the API to take the struct msm_drm_private and the struct
drm_encoder and have the DP code figure out which struct msm_dp the
operation relates to.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 17 +++++----
 drivers/gpu/drm/msm/dp/dp_display.c         | 38 +++++++++++++++++----
 drivers/gpu/drm/msm/msm_drv.h               | 31 +++++++++--------
 3 files changed, 56 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 1c04b7cce43e..0d64ef0819af 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1002,8 +1002,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(priv, drm_enc, mode, adj_mode);
 
 	list_for_each_entry(conn_iter, connector_list, head)
 		if (conn_iter->encoder == drm_enc)
@@ -1184,9 +1184,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(priv, drm_enc);
 		if (ret) {
 			DPU_ERROR_ENC(dpu_enc, "dp display enable failed: %d\n",
 				ret);
@@ -1226,8 +1225,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(priv, drm_enc))
 			DPU_ERROR_ENC(dpu_enc, "dp display push idle failed\n");
 	}
 
@@ -1255,8 +1254,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(priv, drm_enc))
 			DPU_ERROR_ENC(dpu_enc, "dp display disable failed\n");
 	}
 
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 8696b36d30e4..59ffd6c8f41f 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1432,12 +1432,25 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
 	return 0;
 }
 
-int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
+static struct msm_dp *msm_dp_from_drm_encoder(struct msm_drm_private *priv,
+					      struct drm_encoder *encoder)
+{
+	if (priv->dp && priv->dp->encoder == encoder)
+		return priv->dp;
+
+	return NULL;
+}
+
+int msm_dp_display_enable(struct msm_drm_private *priv, struct drm_encoder *encoder)
 {
 	int rc = 0;
 	struct dp_display_private *dp_display;
+	struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);
 	u32 state;
 
+	if (!dp)
+		return -EINVAL;
+
 	dp_display = container_of(dp, struct dp_display_private, dp_display);
 	if (!dp_display->dp_mode.drm_mode.clock) {
 		DRM_ERROR("invalid params\n");
@@ -1489,9 +1502,13 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
 	return rc;
 }
 
-int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder *encoder)
+int msm_dp_display_pre_disable(struct msm_drm_private *priv, struct drm_encoder *encoder)
 {
 	struct dp_display_private *dp_display;
+	struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);
+
+	if (!dp)
+		return 0;
 
 	dp_display = container_of(dp, struct dp_display_private, dp_display);
 
@@ -1500,11 +1517,15 @@ int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder *encoder)
 	return 0;
 }
 
-int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
+int msm_dp_display_disable(struct msm_drm_private *priv, struct drm_encoder *encoder)
 {
 	int rc = 0;
 	u32 state;
 	struct dp_display_private *dp_display;
+	struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);
+
+	if (!dp)
+		return 0;
 
 	dp_display = container_of(dp, struct dp_display_private, dp_display);
 
@@ -1531,11 +1552,16 @@ int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
 	return rc;
 }
 
-void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder *encoder,
-				struct drm_display_mode *mode,
-				struct drm_display_mode *adjusted_mode)
+void msm_dp_display_mode_set(struct msm_drm_private *priv,
+			     struct drm_encoder *encoder,
+			     struct drm_display_mode *mode,
+			     struct drm_display_mode *adjusted_mode)
 {
 	struct dp_display_private *dp_display;
+	struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);
+
+	if (!dp)
+		return;
 
 	dp_display = container_of(dp, struct dp_display_private, dp_display);
 
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 9bfd37855969..e9232032b266 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -388,12 +388,13 @@ int __init msm_dp_register(void);
 void __exit msm_dp_unregister(void);
 int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
 			 struct drm_encoder *encoder);
-int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder);
-int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder);
-int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder *encoder);
-void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder *encoder,
-				struct drm_display_mode *mode,
-				struct drm_display_mode *adjusted_mode);
+int msm_dp_display_enable(struct msm_drm_private *priv, struct drm_encoder *encoder);
+int msm_dp_display_disable(struct msm_drm_private *priv, struct drm_encoder *encoder);
+int msm_dp_display_pre_disable(struct msm_drm_private *priv, struct drm_encoder *encoder);
+void msm_dp_display_mode_set(struct msm_drm_private *priv,
+			     struct drm_encoder *encoder,
+			     struct drm_display_mode *mode,
+			     struct drm_display_mode *adjusted_mode);
 void msm_dp_irq_postinstall(struct msm_dp *dp_display);
 void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_display);
 
@@ -413,25 +414,25 @@ static inline int msm_dp_modeset_init(struct msm_dp *dp_display,
 {
 	return -EINVAL;
 }
-static inline int msm_dp_display_enable(struct msm_dp *dp,
+static inline int msm_dp_display_enable(struct msm_drm_private *priv,
 					struct drm_encoder *encoder)
 {
 	return -EINVAL;
 }
-static inline int msm_dp_display_disable(struct msm_dp *dp,
-					struct drm_encoder *encoder)
+static inline int msm_dp_display_disable(struct msm_drm_private *priv,
+					 struct drm_encoder *encoder)
 {
 	return -EINVAL;
 }
-static inline int msm_dp_display_pre_disable(struct msm_dp *dp,
-					struct drm_encoder *encoder)
+static inline int msm_dp_display_pre_disable(struct msm_drm_private *priv,
+					     struct drm_encoder *encoder)
 {
 	return -EINVAL;
 }
-static inline void msm_dp_display_mode_set(struct msm_dp *dp,
-				struct drm_encoder *encoder,
-				struct drm_display_mode *mode,
-				struct drm_display_mode *adjusted_mode)
+static inline void msm_dp_display_mode_set(struct msm_drm_private *priv,
+					   struct drm_encoder *encoder,
+					   struct drm_display_mode *mode,
+					   struct drm_display_mode *adjusted_mode)
 {
 }
 
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 3/5] drm/msm/dp: Support up to 3 DP controllers
  2021-07-25  4:24 [PATCH 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x Bjorn Andersson
  2021-07-25  4:24 ` [PATCH 1/5] drm/msm/dp: Remove global g_dp_display variable Bjorn Andersson
  2021-07-25  4:24 ` [PATCH 2/5] drm/msm/dp: Modify prototype of encoder based API Bjorn Andersson
@ 2021-07-25  4:24 ` Bjorn Andersson
  2021-07-26 23:51   ` Stephen Boyd
  2021-07-25  4:24 ` [PATCH 4/4] drm/msm/dp: Add sc8180x " Bjorn Andersson
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Bjorn Andersson @ 2021-07-25  4:24 UTC (permalink / raw)
  To: Rob Clark, Sean Paul, David Airlie, Daniel Vetter, Abhinav Kumar,
	Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh
  Cc: Rob Herring, Stephen Boyd, linux-arm-msm, dri-devel, freedreno,
	linux-kernel

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 finding a
struct msm_dp from a struct drm_encoder.

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>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       | 60 +++++++++++--------
 .../gpu/drm/msm/disp/msm_disp_snapshot_util.c |  8 ++-
 drivers/gpu/drm/msm/dp/dp_display.c           | 59 ++++++++++++++++--
 drivers/gpu/drm/msm/msm_drv.h                 |  2 +-
 4 files changed, 95 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index f655adbc2421..a793cc8a007e 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,8 @@ 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++)
+		msm_dp_debugfs_init(priv->dp[i], minor);
 
 	return dpu_core_perf_debugfs_init(dpu_kms, entry);
 }
@@ -545,33 +546,40 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
 	struct drm_encoder *encoder = NULL;
 	struct msm_display_info info;
 	int rc = 0;
+	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.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;
+		}
+	}
 
-	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;
 }
 
@@ -792,6 +800,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 +809,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 59ffd6c8f41f..92b7646a1bb7 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;
 
+	int id;
+
 	/* state variables */
 	bool core_initialized;
 	bool hpd_irq_on;
@@ -116,8 +118,19 @@ struct dp_display_private {
 	struct dp_audio *audio;
 };
 
+
+struct msm_dp_config {
+	phys_addr_t io_start[3];
+	size_t num_dp;
+};
+
+static const struct msm_dp_config sc7180_dp_cfg = {
+	.io_start = { 0x0ae90000 },
+	.num_dp = 1,
+};
+
 static const struct of_device_id dp_dt_match[] = {
-	{.compatible = "qcom,sc7180-dp"},
+	{ .compatible = "qcom,sc7180-dp", .data = &sc7180_dp_cfg },
 	{}
 };
 
@@ -217,7 +230,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) {
@@ -238,8 +251,11 @@ static int dp_display_bind(struct device *dev, struct device *master,
 	}
 
 	rc = dp_register_audio_driver(dev, dp->audio);
-	if (rc)
+	if (rc) {
 		DRM_ERROR("Audio registration Dp failed\n");
+		goto end;
+	}
+
 
 end:
 	return rc;
@@ -259,7 +275,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 = {
@@ -1205,6 +1221,26 @@ int dp_display_request_irq(struct msm_dp *dp_display)
 	return 0;
 }
 
+static int dp_display_get_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_dp; 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;
@@ -1219,6 +1255,10 @@ static int dp_display_probe(struct platform_device *pdev)
 	if (!dp)
 		return -ENOMEM;
 
+	dp->id = dp_display_get_id(pdev);
+	if (dp->id < 0)
+		return -EINVAL;
+
 	dp->pdev = pdev;
 	dp->name = "drm_dp";
 
@@ -1386,6 +1426,9 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
 	struct device *dev;
 	int rc;
 
+	if (!dp_display)
+		return;
+
 	dp = container_of(dp_display, struct dp_display_private, dp_display);
 	dev = &dp->pdev->dev;
 
@@ -1435,8 +1478,12 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
 static struct msm_dp *msm_dp_from_drm_encoder(struct msm_drm_private *priv,
 					      struct drm_encoder *encoder)
 {
-	if (priv->dp && priv->dp->encoder == encoder)
-		return priv->dp;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
+		if (priv->dp[i] && priv->dp[i]->encoder == encoder)
+			return priv->dp[i];
+	}
 
 	return NULL;
 }
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index e9232032b266..62d54ef6c2c4 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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 4/4] drm/msm/dp: Add sc8180x DP controllers
  2021-07-25  4:24 [PATCH 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x Bjorn Andersson
                   ` (2 preceding siblings ...)
  2021-07-25  4:24 ` [PATCH 3/5] drm/msm/dp: Support up to 3 DP controllers Bjorn Andersson
@ 2021-07-25  4:24 ` Bjorn Andersson
  2021-07-25  4:24 ` [PATCH 4/5] dt-bindings: msm/dp: Add SC8180x compatibles Bjorn Andersson
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Bjorn Andersson @ 2021-07-25  4:24 UTC (permalink / raw)
  To: Rob Clark, Sean Paul, David Airlie, Daniel Vetter, Abhinav Kumar,
	Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh
  Cc: Rob Herring, Stephen Boyd, linux-arm-msm, dri-devel, freedreno,
	linux-kernel

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

Link: https://lore.kernel.org/linux-arm-msm/20210511042043.592802-5-bjorn.andersson@linaro.org/
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 92b7646a1bb7..c26805cfcdd1 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -129,8 +129,20 @@ static const struct msm_dp_config sc7180_dp_cfg = {
 	.num_dp = 1,
 };
 
+static const struct msm_dp_config sc8180x_dp_cfg = {
+	.io_start = { 0xae90000, 0xae98000, 0 },
+	.num_dp = 3,
+};
+
+static const struct msm_dp_config sc8180x_edp_cfg = {
+	.io_start = { 0, 0, 0xae9a000 },
+	.num_dp = 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_edp_cfg },
 	{}
 };
 
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 4/5] dt-bindings: msm/dp: Add SC8180x compatibles
  2021-07-25  4:24 [PATCH 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x Bjorn Andersson
                   ` (3 preceding siblings ...)
  2021-07-25  4:24 ` [PATCH 4/4] drm/msm/dp: Add sc8180x " Bjorn Andersson
@ 2021-07-25  4:24 ` Bjorn Andersson
  2021-07-26 23:52   ` Stephen Boyd
  2021-07-25  4:24 ` [PATCH 5/5] drm/msm/dp: Add sc8180x DP controllers Bjorn Andersson
  2021-07-31  1:21 ` [PATCH 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x abhinavk
  6 siblings, 1 reply; 16+ messages in thread
From: Bjorn Andersson @ 2021-07-25  4:24 UTC (permalink / raw)
  To: Rob Clark, Sean Paul, David Airlie, Daniel Vetter, Abhinav Kumar,
	Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh
  Cc: Rob Herring, Stephen Boyd, linux-arm-msm, dri-devel, freedreno,
	linux-kernel

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

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 .../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 a6e41be038fc..c6056e0b0845 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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 5/5] drm/msm/dp: Add sc8180x DP controllers
  2021-07-25  4:24 [PATCH 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x Bjorn Andersson
                   ` (4 preceding siblings ...)
  2021-07-25  4:24 ` [PATCH 4/5] dt-bindings: msm/dp: Add SC8180x compatibles Bjorn Andersson
@ 2021-07-25  4:24 ` Bjorn Andersson
  2021-07-26 23:54   ` Stephen Boyd
  2021-07-31  1:21 ` [PATCH 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x abhinavk
  6 siblings, 1 reply; 16+ messages in thread
From: Bjorn Andersson @ 2021-07-25  4:24 UTC (permalink / raw)
  To: Rob Clark, Sean Paul, David Airlie, Daniel Vetter, Abhinav Kumar,
	Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh
  Cc: Rob Herring, Stephen Boyd, linux-arm-msm, dri-devel, freedreno,
	linux-kernel

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

Link: https://lore.kernel.org/linux-arm-msm/20210511042043.592802-5-bjorn.andersson@linaro.org/
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 92b7646a1bb7..c26805cfcdd1 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -129,8 +129,20 @@ static const struct msm_dp_config sc7180_dp_cfg = {
 	.num_dp = 1,
 };
 
+static const struct msm_dp_config sc8180x_dp_cfg = {
+	.io_start = { 0xae90000, 0xae98000, 0 },
+	.num_dp = 3,
+};
+
+static const struct msm_dp_config sc8180x_edp_cfg = {
+	.io_start = { 0, 0, 0xae9a000 },
+	.num_dp = 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_edp_cfg },
 	{}
 };
 
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/5] drm/msm/dp: Remove global g_dp_display variable
  2021-07-25  4:24 ` [PATCH 1/5] drm/msm/dp: Remove global g_dp_display variable Bjorn Andersson
@ 2021-07-26 23:11   ` Stephen Boyd
  2021-07-27 18:15   ` Dmitry Baryshkov
  2021-07-30 23:37   ` [Freedreno] " abhinavk
  2 siblings, 0 replies; 16+ messages in thread
From: Stephen Boyd @ 2021-07-26 23:11 UTC (permalink / raw)
  To: Abhinav Kumar, Bjorn Andersson, Daniel Vetter, David Airlie,
	Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh, Rob Clark,
	Sean Paul
  Cc: Rob Herring, linux-arm-msm, dri-devel, freedreno, linux-kernel

Quoting Bjorn Andersson (2021-07-24 21:24:31)
> 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.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---

Thanks for removing the global.

>  drivers/gpu/drm/msm/dp/dp_display.c | 78 ++++++++++++++---------------
>  1 file changed, 37 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 70b319a8fe83..8696b36d30e4 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 {
> @@ -122,6 +121,13 @@ static const struct of_device_id dp_dt_match[] = {
>         {}
>  };
>
> +static struct dp_display_private *dev_to_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)
>  {
> @@ -198,14 +204,16 @@ static int dp_display_bind(struct device *dev, struct device *master,
>                            void *data)
>  {
>         int rc = 0;
> -       struct dp_display_private *dp;
> +       struct dp_display_private *dp = dev_to_dp_display_private(dev);
>         struct drm_device *drm;
>         struct msm_drm_private *priv;
>
>         drm = dev_get_drvdata(master);
>
> -       dp = container_of(g_dp_display,
> -                       struct dp_display_private, dp_display);
> +       if (!dp) {

How can it be NULL? dev_to_dp_display_private() returns container_of()
pointer so it doesn't look possible.

> +               DRM_ERROR("DP driver bind failed. Invalid driver data\n");
> +               return -EINVAL;
> +       }
>
>         dp->dp_display.drm_dev = drm;
>         priv = drm->dev_private;

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/5] drm/msm/dp: Support up to 3 DP controllers
  2021-07-25  4:24 ` [PATCH 3/5] drm/msm/dp: Support up to 3 DP controllers Bjorn Andersson
@ 2021-07-26 23:51   ` Stephen Boyd
  0 siblings, 0 replies; 16+ messages in thread
From: Stephen Boyd @ 2021-07-26 23:51 UTC (permalink / raw)
  To: Abhinav Kumar, Bjorn Andersson, Daniel Vetter, David Airlie,
	Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh, Rob Clark,
	Sean Paul
  Cc: Rob Herring, linux-arm-msm, dri-devel, freedreno, linux-kernel

Quoting Bjorn Andersson (2021-07-24 21:24:33)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 59ffd6c8f41f..92b7646a1bb7 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -238,8 +251,11 @@ static int dp_display_bind(struct device *dev, struct device *master,
>         }
>
>         rc = dp_register_audio_driver(dev, dp->audio);
> -       if (rc)
> +       if (rc) {
>                 DRM_ERROR("Audio registration Dp failed\n");
> +               goto end;
> +       }
> +
>
>  end:
>         return rc;

This hunk looks useless. Drop it?

> @@ -1205,6 +1221,26 @@ int dp_display_request_irq(struct msm_dp *dp_display)
>         return 0;
>  }
>
> +static int dp_display_get_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_dp; 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;
> @@ -1219,6 +1255,10 @@ static int dp_display_probe(struct platform_device *pdev)
>         if (!dp)
>                 return -ENOMEM;
>
> +       dp->id = dp_display_get_id(pdev);
> +       if (dp->id < 0)
> +               return -EINVAL;
> +
>         dp->pdev = pdev;
>         dp->name = "drm_dp";
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index e9232032b266..62d54ef6c2c4 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];

It would be nice to either make this dynamically sized (probably little
gain), somehow make a BUILD_BUG_ON(), or have a WARN_ON if
ARRAY_SIZE(dp) is less than a num_dp so we know somebody messed up.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/5] dt-bindings: msm/dp: Add SC8180x compatibles
  2021-07-25  4:24 ` [PATCH 4/5] dt-bindings: msm/dp: Add SC8180x compatibles Bjorn Andersson
@ 2021-07-26 23:52   ` Stephen Boyd
  0 siblings, 0 replies; 16+ messages in thread
From: Stephen Boyd @ 2021-07-26 23:52 UTC (permalink / raw)
  To: Abhinav Kumar, Bjorn Andersson, Daniel Vetter, David Airlie,
	Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh, Rob Clark,
	Sean Paul
  Cc: Rob Herring, linux-arm-msm, dri-devel, freedreno, linux-kernel

Quoting Bjorn Andersson (2021-07-24 21:24:35)
> The Qualcomm SC8180x has 2 DP controllers and 1 eDP controller, add
> compatibles for these to the msm/dp binding.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 5/5] drm/msm/dp: Add sc8180x DP controllers
  2021-07-25  4:24 ` [PATCH 5/5] drm/msm/dp: Add sc8180x DP controllers Bjorn Andersson
@ 2021-07-26 23:54   ` Stephen Boyd
  0 siblings, 0 replies; 16+ messages in thread
From: Stephen Boyd @ 2021-07-26 23:54 UTC (permalink / raw)
  To: Abhinav Kumar, Bjorn Andersson, Daniel Vetter, David Airlie,
	Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh, Rob Clark,
	Sean Paul
  Cc: Rob Herring, linux-arm-msm, dri-devel, freedreno, linux-kernel

Quoting Bjorn Andersson (2021-07-24 21:24:36)
> The sc8180x has 2 DP and 1 eDP controllers, add support for these to the
> DP driver.
>
> Link: https://lore.kernel.org/linux-arm-msm/20210511042043.592802-5-bjorn.andersson@linaro.org/
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 92b7646a1bb7..c26805cfcdd1 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -129,8 +129,20 @@ static const struct msm_dp_config sc7180_dp_cfg = {
>         .num_dp = 1,
>  };
>
> +static const struct msm_dp_config sc8180x_dp_cfg = {
> +       .io_start = { 0xae90000, 0xae98000, 0 },
> +       .num_dp = 3,
> +};
> +
> +static const struct msm_dp_config sc8180x_edp_cfg = {
> +       .io_start = { 0, 0, 0xae9a000 },
> +       .num_dp = 3,
> +};

Can the two structs not be combined into one struct and set as .data for
either compatible?

> +
>  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_edp_cfg },
>         {}

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/5] drm/msm/dp: Modify prototype of encoder based API
  2021-07-25  4:24 ` [PATCH 2/5] drm/msm/dp: Modify prototype of encoder based API Bjorn Andersson
@ 2021-07-26 23:55   ` Stephen Boyd
  2021-07-27 18:42   ` Dmitry Baryshkov
  1 sibling, 0 replies; 16+ messages in thread
From: Stephen Boyd @ 2021-07-26 23:55 UTC (permalink / raw)
  To: Abhinav Kumar, Bjorn Andersson, Daniel Vetter, David Airlie,
	Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh, Rob Clark,
	Sean Paul
  Cc: Rob Herring, linux-arm-msm, dri-devel, freedreno, linux-kernel

Quoting Bjorn Andersson (2021-07-24 21:24:32)
> 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.
>
> The information for doing this lookup is available inside the DP driver,
> so update the API to take the struct msm_drm_private and the struct
> drm_encoder and have the DP code figure out which struct msm_dp the
> operation relates to.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/5] drm/msm/dp: Remove global g_dp_display variable
  2021-07-25  4:24 ` [PATCH 1/5] drm/msm/dp: Remove global g_dp_display variable Bjorn Andersson
  2021-07-26 23:11   ` Stephen Boyd
@ 2021-07-27 18:15   ` Dmitry Baryshkov
  2021-07-30 23:37   ` [Freedreno] " abhinavk
  2 siblings, 0 replies; 16+ messages in thread
From: Dmitry Baryshkov @ 2021-07-27 18:15 UTC (permalink / raw)
  To: Bjorn Andersson, Rob Clark, Sean Paul, David Airlie,
	Daniel Vetter, Abhinav Kumar, Kalyan Thota, Kuogee Hsieh
  Cc: Rob Herring, Stephen Boyd, linux-arm-msm, dri-devel, freedreno,
	linux-kernel

On 25/07/2021 07:24, 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.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>   drivers/gpu/drm/msm/dp/dp_display.c | 78 ++++++++++++++---------------
>   1 file changed, 37 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 70b319a8fe83..8696b36d30e4 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 {
> @@ -122,6 +121,13 @@ static const struct of_device_id dp_dt_match[] = {
>   	{}
>   };
>   
> +static struct dp_display_private *dev_to_dp_display_private(struct device *dev)

dev_get_dp_display_private() ?

> +{
> +	struct msm_dp *dp = dev_get_drvdata(dev);
> +
> +	return container_of(dp, struct dp_display_private, dp_display);
> +}
> +

As a matter of preference, it might be cleaner to inline dev_get_drvdata 
and then define msm_dp_get_private to convert from msm_dp to 
dp_display_private, see below.

>   static int dp_add_event(struct dp_display_private *dp_priv, u32 event,
>   						u32 data, u32 delay)
>   {
> @@ -198,14 +204,16 @@ static int dp_display_bind(struct device *dev, struct device *master,
>   			   void *data)
>   {
>   	int rc = 0;
> -	struct dp_display_private *dp;
> +	struct dp_display_private *dp = dev_to_dp_display_private(dev);
>   	struct drm_device *drm;
>   	struct msm_drm_private *priv;
>   
>   	drm = dev_get_drvdata(master);
>   
> -	dp = container_of(g_dp_display,
> -			struct dp_display_private, dp_display);
> +	if (!dp) {

This is not correct, if I'm not mistaken. you wanted to check if dev's 
private data is NULL (correct check), but ended up checking whether the 
result of container_of is NULL (incorrect).

> +		DRM_ERROR("DP driver bind failed. Invalid driver data\n");
> +		return -EINVAL;
> +	}
>   
>   	dp->dp_display.drm_dev = drm;
>   	priv = drm->dev_private;
> @@ -240,12 +248,14 @@ 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_to_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);
> +	if (!dp) {
> +		DRM_ERROR("Invalid DP driver data\n");
> +		return;
> +	}
>   
>   	dp_power_client_deinit(dp->power);
>   	dp_aux_unregister(dp->aux);
> @@ -376,17 +386,14 @@ 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;
> +	struct dp_display_private *dp = dev_to_dp_display_private(dev);
>   
> -	if (!dev) {
> -		DRM_ERROR("invalid dev\n");
> -		rc = -EINVAL;
> +	if (!dp) {
> +		DRM_ERROR("no driver data found\n");
> +		rc = -ENODEV;
>   		goto end;
>   	}
>   
> -	dp = container_of(g_dp_display,
> -			struct dp_display_private, dp_display);
> -
>   	dp_display_host_init(dp, false);
>   
>   	rc = dp_display_process_hpd_high(dp);
> @@ -397,17 +404,14 @@ static int dp_display_usbpd_configure_cb(struct device *dev)
>   static int dp_display_usbpd_disconnect_cb(struct device *dev)
>   {
>   	int rc = 0;
> -	struct dp_display_private *dp;
> +	struct dp_display_private *dp = dev_to_dp_display_private(dev);
>   
> -	if (!dev) {
> -		DRM_ERROR("invalid dev\n");
> -		rc = -EINVAL;

`!dev` check should remain in place. And dp should be fetched 
afterwards. This applies to all the checks in the patch.

> +	if (!dp) {
> +		DRM_ERROR("no driver data found\n");
> +		rc = -ENODEV;
>   		return rc;
>   	}
>   
> -	dp = container_of(g_dp_display,
> -			struct dp_display_private, dp_display);
> -
>   	dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
>   
>   	return rc;
> @@ -466,15 +470,15 @@ static int dp_display_usbpd_attention_cb(struct device *dev)
>   {
>   	int rc = 0;
>   	u32 sink_request;
> -	struct dp_display_private *dp;
> +	struct dp_display_private *dp = dev_to_dp_display_private(dev);
> +	struct dp_usbpd *hpd;
>   
> -	if (!dev) {
> -		DRM_ERROR("invalid dev\n");
> -		return -EINVAL;
> +	if (!dp) {
> +		DRM_ERROR("no driver data found\n");
> +		return -ENODEV;
>   	}
>   
> -	dp = container_of(g_dp_display,
> -			struct dp_display_private, dp_display);
> +	hpd = dp->usbpd;
>   
>   	/* check for any test request issued by sink */
>   	rc = dp_link_process_request(dp->link);
> @@ -638,7 +642,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
>   	dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
>   
>   	/* 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);
> @@ -832,9 +836,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;
>   
>   	if (dp_display->power_on) {
>   		DRM_DEBUG_DP("Link already setup, return\n");
> @@ -869,9 +871,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;
> @@ -1229,14 +1229,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) {
> @@ -1249,10 +1248,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 = platform_get_drvdata(pdev);

dev_to_dp_display_private() rather than just get_drvdata?

>   
>   	dp_display_deinit_sub_modules(dp);
>   
> 


-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/5] drm/msm/dp: Modify prototype of encoder based API
  2021-07-25  4:24 ` [PATCH 2/5] drm/msm/dp: Modify prototype of encoder based API Bjorn Andersson
  2021-07-26 23:55   ` Stephen Boyd
@ 2021-07-27 18:42   ` Dmitry Baryshkov
  1 sibling, 0 replies; 16+ messages in thread
From: Dmitry Baryshkov @ 2021-07-27 18:42 UTC (permalink / raw)
  To: Bjorn Andersson, Rob Clark, Sean Paul, David Airlie,
	Daniel Vetter, Abhinav Kumar, Kalyan Thota, Kuogee Hsieh
  Cc: Rob Herring, Stephen Boyd, linux-arm-msm, dri-devel, freedreno,
	linux-kernel

On 25/07/2021 07:24, 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.
> 
> The information for doing this lookup is available inside the DP driver,
> so update the API to take the struct msm_drm_private and the struct
> drm_encoder and have the DP code figure out which struct msm_dp the
> operation relates to.

Initially I thought to propose moving encoder->dp lookup into dpu code 
by adding msm_dp_display_get_encoder() function. However as I was 
writing that, I remembered that at some point I had to refactor my own 
patchset in the way to get rid of calling msm_FOO_get_encoder().

I'd propose simpler solution. In dpu_encoder_setup() you have the DP 
index and the encoder. So you can store valid msm_dp pointer in the 
dpu_encoder_virt and remove all the lookups. Then you can replace all 
priv->dp with bare dpu_enc->dp accesses. Will this work for you?

> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 17 +++++----
>   drivers/gpu/drm/msm/dp/dp_display.c         | 38 +++++++++++++++++----
>   drivers/gpu/drm/msm/msm_drv.h               | 31 +++++++++--------
>   3 files changed, 56 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 1c04b7cce43e..0d64ef0819af 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1002,8 +1002,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(priv, drm_enc, mode, adj_mode);
>   
>   	list_for_each_entry(conn_iter, connector_list, head)
>   		if (conn_iter->encoder == drm_enc)
> @@ -1184,9 +1184,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(priv, drm_enc);
>   		if (ret) {
>   			DPU_ERROR_ENC(dpu_enc, "dp display enable failed: %d\n",
>   				ret);
> @@ -1226,8 +1225,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(priv, drm_enc))
>   			DPU_ERROR_ENC(dpu_enc, "dp display push idle failed\n");
>   	}
>   
> @@ -1255,8 +1254,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(priv, drm_enc))
>   			DPU_ERROR_ENC(dpu_enc, "dp display disable failed\n");
>   	}
>   
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 8696b36d30e4..59ffd6c8f41f 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1432,12 +1432,25 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
>   	return 0;
>   }
>   
> -int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
> +static struct msm_dp *msm_dp_from_drm_encoder(struct msm_drm_private *priv,
> +					      struct drm_encoder *encoder)
> +{
> +	if (priv->dp && priv->dp->encoder == encoder)
> +		return priv->dp;
> +
> +	return NULL;
> +}
> +
> +int msm_dp_display_enable(struct msm_drm_private *priv, struct drm_encoder *encoder)
>   {
>   	int rc = 0;
>   	struct dp_display_private *dp_display;
> +	struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);
>   	u32 state;
>   
> +	if (!dp)
> +		return -EINVAL;
> +
>   	dp_display = container_of(dp, struct dp_display_private, dp_display);
>   	if (!dp_display->dp_mode.drm_mode.clock) {
>   		DRM_ERROR("invalid params\n");
> @@ -1489,9 +1502,13 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
>   	return rc;
>   }
>   
> -int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder *encoder)
> +int msm_dp_display_pre_disable(struct msm_drm_private *priv, struct drm_encoder *encoder)
>   {
>   	struct dp_display_private *dp_display;
> +	struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);
> +
> +	if (!dp)
> +		return 0;
>   
>   	dp_display = container_of(dp, struct dp_display_private, dp_display);
>   
> @@ -1500,11 +1517,15 @@ int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder *encoder)
>   	return 0;
>   }
>   
> -int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
> +int msm_dp_display_disable(struct msm_drm_private *priv, struct drm_encoder *encoder)
>   {
>   	int rc = 0;
>   	u32 state;
>   	struct dp_display_private *dp_display;
> +	struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);
> +
> +	if (!dp)
> +		return 0;
>   
>   	dp_display = container_of(dp, struct dp_display_private, dp_display);
>   
> @@ -1531,11 +1552,16 @@ int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder)
>   	return rc;
>   }
>   
> -void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder *encoder,
> -				struct drm_display_mode *mode,
> -				struct drm_display_mode *adjusted_mode)
> +void msm_dp_display_mode_set(struct msm_drm_private *priv,
> +			     struct drm_encoder *encoder,
> +			     struct drm_display_mode *mode,
> +			     struct drm_display_mode *adjusted_mode)
>   {
>   	struct dp_display_private *dp_display;
> +	struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);
> +
> +	if (!dp)
> +		return;
>   
>   	dp_display = container_of(dp, struct dp_display_private, dp_display);
>   
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 9bfd37855969..e9232032b266 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -388,12 +388,13 @@ int __init msm_dp_register(void);
>   void __exit msm_dp_unregister(void);
>   int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
>   			 struct drm_encoder *encoder);
> -int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder);
> -int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder);
> -int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder *encoder);
> -void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder *encoder,
> -				struct drm_display_mode *mode,
> -				struct drm_display_mode *adjusted_mode);
> +int msm_dp_display_enable(struct msm_drm_private *priv, struct drm_encoder *encoder);
> +int msm_dp_display_disable(struct msm_drm_private *priv, struct drm_encoder *encoder);
> +int msm_dp_display_pre_disable(struct msm_drm_private *priv, struct drm_encoder *encoder);
> +void msm_dp_display_mode_set(struct msm_drm_private *priv,
> +			     struct drm_encoder *encoder,
> +			     struct drm_display_mode *mode,
> +			     struct drm_display_mode *adjusted_mode);
>   void msm_dp_irq_postinstall(struct msm_dp *dp_display);
>   void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_display);
>   
> @@ -413,25 +414,25 @@ static inline int msm_dp_modeset_init(struct msm_dp *dp_display,
>   {
>   	return -EINVAL;
>   }
> -static inline int msm_dp_display_enable(struct msm_dp *dp,
> +static inline int msm_dp_display_enable(struct msm_drm_private *priv,
>   					struct drm_encoder *encoder)
>   {
>   	return -EINVAL;
>   }
> -static inline int msm_dp_display_disable(struct msm_dp *dp,
> -					struct drm_encoder *encoder)
> +static inline int msm_dp_display_disable(struct msm_drm_private *priv,
> +					 struct drm_encoder *encoder)
>   {
>   	return -EINVAL;
>   }
> -static inline int msm_dp_display_pre_disable(struct msm_dp *dp,
> -					struct drm_encoder *encoder)
> +static inline int msm_dp_display_pre_disable(struct msm_drm_private *priv,
> +					     struct drm_encoder *encoder)
>   {
>   	return -EINVAL;
>   }
> -static inline void msm_dp_display_mode_set(struct msm_dp *dp,
> -				struct drm_encoder *encoder,
> -				struct drm_display_mode *mode,
> -				struct drm_display_mode *adjusted_mode)
> +static inline void msm_dp_display_mode_set(struct msm_drm_private *priv,
> +					   struct drm_encoder *encoder,
> +					   struct drm_display_mode *mode,
> +					   struct drm_display_mode *adjusted_mode)
>   {
>   }
>   
> 


-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Freedreno] [PATCH 1/5] drm/msm/dp: Remove global g_dp_display variable
  2021-07-25  4:24 ` [PATCH 1/5] drm/msm/dp: Remove global g_dp_display variable Bjorn Andersson
  2021-07-26 23:11   ` Stephen Boyd
  2021-07-27 18:15   ` Dmitry Baryshkov
@ 2021-07-30 23:37   ` abhinavk
  2 siblings, 0 replies; 16+ messages in thread
From: abhinavk @ 2021-07-30 23:37 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Rob Clark, Sean Paul, David Airlie, Daniel Vetter,
	Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh, linux-arm-msm,
	linux-kernel, dri-devel, Stephen Boyd, Rob Herring, freedreno

On 2021-07-24 21:24, 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.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 78 ++++++++++++++---------------
>  1 file changed, 37 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 70b319a8fe83..8696b36d30e4 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 {
> @@ -122,6 +121,13 @@ static const struct of_device_id dp_dt_match[] = {
>  	{}
>  };
> 
> +static struct dp_display_private *dev_to_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)
>  {
> @@ -198,14 +204,16 @@ static int dp_display_bind(struct device *dev,
> struct device *master,
>  			   void *data)
>  {
>  	int rc = 0;
> -	struct dp_display_private *dp;
> +	struct dp_display_private *dp = dev_to_dp_display_private(dev);
>  	struct drm_device *drm;
>  	struct msm_drm_private *priv;
> 
>  	drm = dev_get_drvdata(master);
> 
> -	dp = container_of(g_dp_display,
> -			struct dp_display_private, dp_display);
> +	if (!dp) {
> +		DRM_ERROR("DP driver bind failed. Invalid driver data\n");
> +		return -EINVAL;
> +	}
> 
>  	dp->dp_display.drm_dev = drm;
>  	priv = drm->dev_private;
> @@ -240,12 +248,14 @@ 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_to_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);
> +	if (!dp) {
> +		DRM_ERROR("Invalid DP driver data\n");
> +		return;
> +	}
> 
>  	dp_power_client_deinit(dp->power);
>  	dp_aux_unregister(dp->aux);
> @@ -376,17 +386,14 @@ 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;
> +	struct dp_display_private *dp = dev_to_dp_display_private(dev);
> 
> -	if (!dev) {
> -		DRM_ERROR("invalid dev\n");
> -		rc = -EINVAL;
> +	if (!dp) {
> +		DRM_ERROR("no driver data found\n");
> +		rc = -ENODEV;
>  		goto end;
>  	}
> 
> -	dp = container_of(g_dp_display,
> -			struct dp_display_private, dp_display);
> -
>  	dp_display_host_init(dp, false);
> 
>  	rc = dp_display_process_hpd_high(dp);
> @@ -397,17 +404,14 @@ static int dp_display_usbpd_configure_cb(struct
> device *dev)
>  static int dp_display_usbpd_disconnect_cb(struct device *dev)
>  {
>  	int rc = 0;
> -	struct dp_display_private *dp;
> +	struct dp_display_private *dp = dev_to_dp_display_private(dev);
> 
> -	if (!dev) {
> -		DRM_ERROR("invalid dev\n");
> -		rc = -EINVAL;
> +	if (!dp) {
> +		DRM_ERROR("no driver data found\n");
> +		rc = -ENODEV;
>  		return rc;
>  	}
> 
> -	dp = container_of(g_dp_display,
> -			struct dp_display_private, dp_display);
> -
>  	dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
> 
>  	return rc;
> @@ -466,15 +470,15 @@ static int dp_display_usbpd_attention_cb(struct
> device *dev)
>  {
>  	int rc = 0;
>  	u32 sink_request;
> -	struct dp_display_private *dp;
> +	struct dp_display_private *dp = dev_to_dp_display_private(dev);
> +	struct dp_usbpd *hpd;
> 
> -	if (!dev) {
> -		DRM_ERROR("invalid dev\n");
> -		return -EINVAL;
> +	if (!dp) {
> +		DRM_ERROR("no driver data found\n");
> +		return -ENODEV;
>  	}
> 
> -	dp = container_of(g_dp_display,
> -			struct dp_display_private, dp_display);
> +	hpd = dp->usbpd;
hpd is unused here. It was removed with 
https://patches.linaro.org/patch/416670/

> 
>  	/* check for any test request issued by sink */
>  	rc = dp_link_process_request(dp->link);
> @@ -638,7 +642,7 @@ static int dp_hpd_unplug_handle(struct
> dp_display_private *dp, u32 data)
>  	dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, 
> DP_TIMEOUT_5_SECOND);
> 
>  	/* 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);
> @@ -832,9 +836,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;
> 
>  	if (dp_display->power_on) {
>  		DRM_DEBUG_DP("Link already setup, return\n");
> @@ -869,9 +871,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;
> @@ -1229,14 +1229,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) {
> @@ -1249,10 +1248,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 = platform_get_drvdata(pdev);
> 
>  	dp_display_deinit_sub_modules(dp);

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x
  2021-07-25  4:24 [PATCH 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x Bjorn Andersson
                   ` (5 preceding siblings ...)
  2021-07-25  4:24 ` [PATCH 5/5] drm/msm/dp: Add sc8180x DP controllers Bjorn Andersson
@ 2021-07-31  1:21 ` abhinavk
  6 siblings, 0 replies; 16+ messages in thread
From: abhinavk @ 2021-07-31  1:21 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Rob Clark, Sean Paul, David Airlie, Daniel Vetter,
	Dmitry Baryshkov, Kalyan Thota, Kuogee Hsieh, Rob Herring,
	Stephen Boyd, linux-arm-msm, dri-devel, freedreno, linux-kernel

Hi Bjorn

On 2021-07-24 21:24, Bjorn Andersson wrote:
> 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.
> 
> This is based on v4 of Dmitry's work on multiple DSI interfaces:
> https://lore.kernel.org/linux-arm-msm/20210717124016.316020-1-dmitry.baryshkov@linaro.org/
> 
> With that in place add SC8180x DP and eDP controllers.

Thanks for posting the changes.

I dont have major concerns on the series as such apart from minor 
comments which i will post in a day or two
but I will check and get back if this has been validated on sc7280 
without any concerns.

One question i had is not directly related to this series but related to 
multi-DP in general.
Does audio work fine across both the DPs when both are connected?

The reason I ask this question is that, I dont know how two hdmi-codec 
instances are handled today.
So we will register twice with hdmi-codec so there should be two audio 
streams.

But I am not sure if this works correctly in todays design with 
hdmi-codec.

Any chance you had validated this?

> 
> 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   |  17 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |  60 +++---
>  .../gpu/drm/msm/disp/msm_disp_snapshot_util.c |   8 +-
>  drivers/gpu/drm/msm/dp/dp_display.c           | 183 +++++++++++++-----
>  drivers/gpu/drm/msm/msm_drv.h                 |  33 ++--
>  6 files changed, 200 insertions(+), 103 deletions(-)

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2021-07-31  1:21 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-25  4:24 [PATCH 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x Bjorn Andersson
2021-07-25  4:24 ` [PATCH 1/5] drm/msm/dp: Remove global g_dp_display variable Bjorn Andersson
2021-07-26 23:11   ` Stephen Boyd
2021-07-27 18:15   ` Dmitry Baryshkov
2021-07-30 23:37   ` [Freedreno] " abhinavk
2021-07-25  4:24 ` [PATCH 2/5] drm/msm/dp: Modify prototype of encoder based API Bjorn Andersson
2021-07-26 23:55   ` Stephen Boyd
2021-07-27 18:42   ` Dmitry Baryshkov
2021-07-25  4:24 ` [PATCH 3/5] drm/msm/dp: Support up to 3 DP controllers Bjorn Andersson
2021-07-26 23:51   ` Stephen Boyd
2021-07-25  4:24 ` [PATCH 4/4] drm/msm/dp: Add sc8180x " Bjorn Andersson
2021-07-25  4:24 ` [PATCH 4/5] dt-bindings: msm/dp: Add SC8180x compatibles Bjorn Andersson
2021-07-26 23:52   ` Stephen Boyd
2021-07-25  4:24 ` [PATCH 5/5] drm/msm/dp: Add sc8180x DP controllers Bjorn Andersson
2021-07-26 23:54   ` Stephen Boyd
2021-07-31  1:21 ` [PATCH 0/5] drm/msm/dp: Support multiple DP instances and add sc8180x abhinavk

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).