[RFC PATCH 0/2] drm/msm/dp: refactor the msm dp driver resources

[RFC PATCH 0/2] drm/msm/dp: refactor the msm dp driver resources

[Sankeerth Billakanti]

The DP driver resources are currently enabled and disabled directly based on code flow.
As mentioned in bug 230631602, we want to do the following:

1) Refactor the dp/edp parsing code to move it to probe (it is currently done in bind).

2) Then bind all the power resources needed for AUX in pm_runtime_ops.

3) Handle EPROBE_DEFER cases of the panel-eDP aux device.

4) Verify DP functionality is unaffected.

These code changes will parse the resources and get the edp panel during probe.
All the necessary resources required for the aux transactions are moved to pm_runtime ops.
They are enabled or disabled via get/put sync functions.

This is a RFC to verify with the community if the approach we are taking is correct.

https://partnerissuetracker.corp.google.com/issues/230631602

Sankeerth Billakanti (2):
  drm/msm/dp: enumerate edp panel during driver probe
  drm/msm/dp: enable pm_runtime support for dp driver

 drivers/gpu/drm/msm/dp/dp_aux.c     | 155 +++++++++++++++++++++--
 drivers/gpu/drm/msm/dp/dp_catalog.c |  12 ++
 drivers/gpu/drm/msm/dp/dp_catalog.h |   1 +
 drivers/gpu/drm/msm/dp/dp_display.c | 185 ++++++++++++++--------------
 drivers/gpu/drm/msm/dp/dp_power.c   |   7 --
 5 files changed, 250 insertions(+), 110 deletions(-)

-- 
2.39.0

[RFC PATCH 0/2] drm/msm/dp: refactor the msm dp driver resources

[Sankeerth Billakanti]

The DP driver resources are currently enabled and disabled directly based on code flow.
As mentioned in bug 230631602, we want to do the following:

1) Refactor the dp/edp parsing code to move it to probe (it is currently done in bind).

2) Then bind all the power resources needed for AUX in pm_runtime_ops.

3) Handle EPROBE_DEFER cases of the panel-eDP aux device.

4) Verify DP functionality is unaffected.

These code changes will parse the resources and get the edp panel during probe.
All the necessary resources required for the aux transactions are moved to pm_runtime ops.
They are enabled or disabled via get/put sync functions.

This is a RFC to verify with the community if the approach we are taking is correct.

https://partnerissuetracker.corp.google.com/issues/230631602

Sankeerth Billakanti (2):
  drm/msm/dp: enumerate edp panel during driver probe
  drm/msm/dp: enable pm_runtime support for dp driver

 drivers/gpu/drm/msm/dp/dp_aux.c     | 155 +++++++++++++++++++++--
 drivers/gpu/drm/msm/dp/dp_catalog.c |  12 ++
 drivers/gpu/drm/msm/dp/dp_catalog.h |   1 +
 drivers/gpu/drm/msm/dp/dp_display.c | 185 ++++++++++++++--------------
 drivers/gpu/drm/msm/dp/dp_power.c   |   7 --
 5 files changed, 250 insertions(+), 110 deletions(-)

-- 
2.39.0

[RFC PATCH 1/2] drm/msm/dp: enumerate edp panel during driver probe

[Sankeerth Billakanti]

The eDP panel is identified and enumerated during probe of the panel-edp
driver. The current DP driver triggers this panel-edp driver probe while
getting the panel-bridge associated with the eDP panel from the platform
driver bind. If the panel-edp probe is deferred, then the whole bunch of
MDSS parent and child drivers have to defer and roll back.

So, we want to move the panel enumeration from bind to probe so that the
probe defer is easier to handle and also both the drivers appear consistent
in panel enumeration. This change moves the DP driver panel-bridge
enumeration to probe.

Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_aux.c     | 149 ++++++++++++++++++++++++++--
 drivers/gpu/drm/msm/dp/dp_catalog.c |  12 +++
 drivers/gpu/drm/msm/dp/dp_catalog.h |   1 +
 drivers/gpu/drm/msm/dp/dp_display.c |  80 ++++++---------
 4 files changed, 182 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index cc3efed593aa..5da95dfdeede 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -110,7 +110,7 @@ static ssize_t dp_aux_write(struct dp_aux_private *aux,
 }
 
 static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
-			      struct drm_dp_aux_msg *msg)
+			      struct drm_dp_aux_msg *msg, bool poll)
 {
 	ssize_t ret;
 	unsigned long time_left;
@@ -121,10 +121,16 @@ static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
 	if (ret < 0)
 		return ret;
 
-	time_left = wait_for_completion_timeout(&aux->comp,
+	if (!poll) {
+		time_left = wait_for_completion_timeout(&aux->comp,
 						msecs_to_jiffies(250));
-	if (!time_left)
-		return -ETIMEDOUT;
+		if (!time_left)
+			return -ETIMEDOUT;
+	} else {
+		ret = dp_catalog_aux_poll_and_get_hw_interrupt(aux->catalog);
+		if (!ret)
+			dp_aux_isr(&aux->dp_aux);
+	}
 
 	return ret;
 }
@@ -238,7 +244,7 @@ static void dp_aux_update_offset_and_segment(struct dp_aux_private *aux,
  */
 static void dp_aux_transfer_helper(struct dp_aux_private *aux,
 				   struct drm_dp_aux_msg *input_msg,
-				   bool send_seg)
+				   bool send_seg, bool poll)
 {
 	struct drm_dp_aux_msg helper_msg;
 	u32 message_size = 0x10;
@@ -278,7 +284,7 @@ static void dp_aux_transfer_helper(struct dp_aux_private *aux,
 		helper_msg.address = segment_address;
 		helper_msg.buffer = &aux->segment;
 		helper_msg.size = 1;
-		dp_aux_cmd_fifo_tx(aux, &helper_msg);
+		dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
 	}
 
 	/*
@@ -292,7 +298,7 @@ static void dp_aux_transfer_helper(struct dp_aux_private *aux,
 	helper_msg.address = input_msg->address;
 	helper_msg.buffer = &aux->offset;
 	helper_msg.size = 1;
-	dp_aux_cmd_fifo_tx(aux, &helper_msg);
+	dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
 
 end:
 	aux->offset += message_size;
@@ -300,6 +306,122 @@ static void dp_aux_transfer_helper(struct dp_aux_private *aux,
 		aux->segment = 0x0; /* reset segment at end of block */
 }
 
+/*
+ * This function does the real job to process an AUX transaction.
+ * It will call aux_reset() function to reset the AUX channel,
+ * if the waiting is timeout.
+ */
+static ssize_t dp_aux_transfer_init(struct drm_dp_aux *dp_aux,
+			       struct drm_dp_aux_msg *msg)
+{
+	ssize_t ret;
+	int const aux_cmd_native_max = 16;
+	int const aux_cmd_i2c_max = 128;
+	struct dp_aux_private *aux;
+
+	aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
+
+	aux->native = msg->request & (DP_AUX_NATIVE_WRITE & DP_AUX_NATIVE_READ);
+
+	/* Ignore address only message */
+	if (msg->size == 0 || !msg->buffer) {
+		msg->reply = aux->native ?
+			DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
+		return msg->size;
+	}
+
+	/* msg sanity check */
+	if ((aux->native && msg->size > aux_cmd_native_max) ||
+	    msg->size > aux_cmd_i2c_max) {
+		DRM_ERROR("%s: invalid msg: size(%zu), request(%x)\n",
+			__func__, msg->size, msg->request);
+		return -EINVAL;
+	}
+
+	mutex_lock(&aux->mutex);
+	if (!aux->initted) {
+		ret = -EIO;
+		goto exit;
+	}
+
+	/*
+	 * For eDP it's important to give a reasonably long wait here for HPD
+	 * to be asserted. This is because the panel driver may have _just_
+	 * turned on the panel and then tried to do an AUX transfer. The panel
+	 * driver has no way of knowing when the panel is ready, so it's up
+	 * to us to wait. For DP we never get into this situation so let's
+	 * avoid ever doing the extra long wait for DP.
+	 */
+	if (aux->is_edp) {
+		ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
+		if (ret) {
+			DRM_DEBUG_DP("Panel not ready for aux transactions\n");
+			goto exit;
+		}
+	}
+
+	dp_aux_update_offset_and_segment(aux, msg);
+	dp_aux_transfer_helper(aux, msg, true, true);
+
+	aux->read = msg->request & (DP_AUX_I2C_READ & DP_AUX_NATIVE_READ);
+	aux->cmd_busy = true;
+
+	if (aux->read) {
+		aux->no_send_addr = true;
+		aux->no_send_stop = false;
+	} else {
+		aux->no_send_addr = true;
+		aux->no_send_stop = true;
+	}
+
+	ret = dp_aux_cmd_fifo_tx(aux, msg, true);
+
+	/* TODO: why is fifo_rx necessary here?
+	 * Ideally fifo_rx need not be executed for an aux write
+	 */
+	ret = dp_aux_cmd_fifo_rx(aux, msg);
+
+	if (ret < 0) {
+		if (aux->native) {
+			aux->retry_cnt++;
+			if (!(aux->retry_cnt % MAX_AUX_RETRIES))
+				dp_catalog_aux_update_cfg(aux->catalog);
+		}
+		/* reset aux if link is in connected state */
+		if (dp_catalog_link_is_connected(aux->catalog))
+			dp_catalog_aux_reset(aux->catalog);
+	} else {
+		aux->retry_cnt = 0;
+		switch (aux->aux_error_num) {
+		case DP_AUX_ERR_NONE:
+			if (aux->read)
+				ret = dp_aux_cmd_fifo_rx(aux, msg);
+			msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
+			break;
+		case DP_AUX_ERR_DEFER:
+			msg->reply = aux->native ?
+				DP_AUX_NATIVE_REPLY_DEFER : DP_AUX_I2C_REPLY_DEFER;
+			break;
+		case DP_AUX_ERR_PHY:
+		case DP_AUX_ERR_ADDR:
+		case DP_AUX_ERR_NACK:
+		case DP_AUX_ERR_NACK_DEFER:
+			msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_NACK : DP_AUX_I2C_REPLY_NACK;
+			break;
+		case DP_AUX_ERR_TOUT:
+			ret = -ETIMEDOUT;
+			break;
+		}
+	}
+
+	aux->cmd_busy = false;
+
+exit:
+	mutex_unlock(&aux->mutex);
+
+	return ret;
+}
+
 /*
  * This function does the real job to process an AUX transaction.
  * It will call aux_reset() function to reset the AUX channel,
@@ -355,7 +477,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
 	}
 
 	dp_aux_update_offset_and_segment(aux, msg);
-	dp_aux_transfer_helper(aux, msg, true);
+	dp_aux_transfer_helper(aux, msg, true, false);
 
 	aux->read = msg->request & (DP_AUX_I2C_READ & DP_AUX_NATIVE_READ);
 	aux->cmd_busy = true;
@@ -368,7 +490,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
 		aux->no_send_stop = true;
 	}
 
-	ret = dp_aux_cmd_fifo_tx(aux, msg);
+	ret = dp_aux_cmd_fifo_tx(aux, msg, false);
 	if (ret < 0) {
 		if (aux->native) {
 			aux->retry_cnt++;
@@ -535,6 +657,15 @@ struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
 	aux->catalog = catalog;
 	aux->retry_cnt = 0;
 
+	/*
+	 * Use the drm_dp_aux_init() to use the aux adapter
+	 * before registering aux with the DRM device.
+	 */
+	aux->dp_aux.name = "dpu_dp_aux";
+	aux->dp_aux.dev = dev;
+	aux->dp_aux.transfer = dp_aux_transfer_init;
+	drm_dp_aux_init(&aux->dp_aux);
+
 	return &aux->dp_aux;
 }
 
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 676279d0ca8d..ccf0400176f0 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -258,6 +258,18 @@ int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog)
 				2000, 500000);
 }
 
+int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog *dp_catalog)
+{
+	u32 aux;
+	struct dp_catalog_private *catalog = container_of(dp_catalog,
+				struct dp_catalog_private, dp_catalog);
+
+	return readl_poll_timeout(catalog->io->dp_controller.ahb.base +
+				REG_DP_INTR_STATUS,
+				aux, aux & DP_INTERRUPT_STATUS1,
+				250, 250000);
+}
+
 static void dump_regs(void __iomem *base, int len)
 {
 	int i;
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
index 1f717f45c115..ad4a9e0f71f2 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.h
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
@@ -87,6 +87,7 @@ void dp_catalog_aux_enable(struct dp_catalog *dp_catalog, bool enable);
 void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog);
 int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog);
 u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
+int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog *dp_catalog);
 
 /* DP Controller APIs */
 void dp_catalog_ctrl_state_ctrl(struct dp_catalog *dp_catalog, u32 state);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index bde1a7ce442f..a5dcef040b74 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -275,13 +275,6 @@ static int dp_display_bind(struct device *dev, struct device *master,
 	dp->dp_display.drm_dev = drm;
 	priv->dp[dp->id] = &dp->dp_display;
 
-	rc = dp->parser->parse(dp->parser);
-	if (rc) {
-		DRM_ERROR("device tree parsing failed\n");
-		goto end;
-	}
-
-
 	dp->drm_dev = drm;
 	dp->aux->drm_dev = drm;
 	rc = dp_aux_register(dp->aux);
@@ -290,12 +283,6 @@ static int dp_display_bind(struct device *dev, struct device *master,
 		goto end;
 	}
 
-	rc = dp_power_client_init(dp->power);
-	if (rc) {
-		DRM_ERROR("Power client create failed\n");
-		goto end;
-	}
-
 	rc = dp_register_audio_driver(dev, dp->audio);
 	if (rc) {
 		DRM_ERROR("Audio registration Dp failed\n");
@@ -787,6 +774,12 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
 		goto error;
 	}
 
+	rc = dp->parser->parse(dp->parser);
+	if (rc) {
+		DRM_ERROR("device tree parsing failed\n");
+		goto error;
+	}
+
 	dp->catalog = dp_catalog_get(dev, &dp->parser->io);
 	if (IS_ERR(dp->catalog)) {
 		rc = PTR_ERR(dp->catalog);
@@ -803,6 +796,12 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
 		goto error;
 	}
 
+	rc = dp_power_client_init(dp->power);
+	if (rc) {
+		DRM_ERROR("Power client create failed\n");
+		goto error;
+	}
+
 	dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
 	if (IS_ERR(dp->aux)) {
 		rc = PTR_ERR(dp->aux);
@@ -1338,12 +1337,29 @@ static int dp_display_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, &dp->dp_display);
 
+	if (dp->dp_display.is_edp) {
+		dp_display_host_init(dp);
+		dp_display_host_phy_init(dp);
+		dp_catalog_ctrl_hpd_config(dp->catalog);
+
+		rc = devm_of_dp_aux_populate_bus(dp->aux, NULL);
+
+		dp_display_host_phy_exit(dp);
+		dp_display_host_deinit(dp);
+
+		if (rc) {
+			DRM_ERROR("failed to initialize panel, rc = %d\n", rc);
+			goto error;
+		}
+	}
+
 	rc = component_add(&pdev->dev, &dp_display_comp_ops);
 	if (rc) {
 		DRM_ERROR("component add failed, rc=%d\n", rc);
 		dp_display_deinit_sub_modules(dp);
 	}
 
+error:
 	return rc;
 }
 
@@ -1546,40 +1562,8 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
 {
 	int rc;
 	struct dp_display_private *dp_priv;
-	struct device_node *aux_bus;
-	struct device *dev;
 
 	dp_priv = container_of(dp, struct dp_display_private, dp_display);
-	dev = &dp_priv->pdev->dev;
-	aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
-
-	if (aux_bus && dp->is_edp) {
-		dp_display_host_init(dp_priv);
-		dp_catalog_ctrl_hpd_config(dp_priv->catalog);
-		dp_display_host_phy_init(dp_priv);
-		enable_irq(dp_priv->irq);
-
-		/*
-		 * The code below assumes that the panel will finish probing
-		 * by the time devm_of_dp_aux_populate_ep_devices() returns.
-		 * This isn't a great assumption since it will fail if the
-		 * panel driver is probed asynchronously but is the best we
-		 * can do without a bigger driver reorganization.
-		 */
-		rc = of_dp_aux_populate_bus(dp_priv->aux, NULL);
-		of_node_put(aux_bus);
-		if (rc)
-			goto error;
-
-		rc = devm_add_action_or_reset(dp->drm_dev->dev,
-						of_dp_aux_depopulate_bus_void,
-						dp_priv->aux);
-		if (rc)
-			goto error;
-	} else if (dp->is_edp) {
-		DRM_ERROR("eDP aux_bus not found\n");
-		return -ENODEV;
-	}
 
 	/*
 	 * External bridges are mandatory for eDP interfaces: one has to
@@ -1597,12 +1581,6 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
 		return 0;
 	}
 
-error:
-	if (dp->is_edp) {
-		disable_irq(dp_priv->irq);
-		dp_display_host_phy_exit(dp_priv);
-		dp_display_host_deinit(dp_priv);
-	}
 	return rc;
 }
 
-- 
2.39.0

[RFC PATCH 1/2] drm/msm/dp: enumerate edp panel during driver probe

[Sankeerth Billakanti]

The eDP panel is identified and enumerated during probe of the panel-edp
driver. The current DP driver triggers this panel-edp driver probe while
getting the panel-bridge associated with the eDP panel from the platform
driver bind. If the panel-edp probe is deferred, then the whole bunch of
MDSS parent and child drivers have to defer and roll back.

So, we want to move the panel enumeration from bind to probe so that the
probe defer is easier to handle and also both the drivers appear consistent
in panel enumeration. This change moves the DP driver panel-bridge
enumeration to probe.

Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_aux.c     | 149 ++++++++++++++++++++++++++--
 drivers/gpu/drm/msm/dp/dp_catalog.c |  12 +++
 drivers/gpu/drm/msm/dp/dp_catalog.h |   1 +
 drivers/gpu/drm/msm/dp/dp_display.c |  80 ++++++---------
 4 files changed, 182 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index cc3efed593aa..5da95dfdeede 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -110,7 +110,7 @@ static ssize_t dp_aux_write(struct dp_aux_private *aux,
 }
 
 static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
-			      struct drm_dp_aux_msg *msg)
+			      struct drm_dp_aux_msg *msg, bool poll)
 {
 	ssize_t ret;
 	unsigned long time_left;
@@ -121,10 +121,16 @@ static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
 	if (ret < 0)
 		return ret;
 
-	time_left = wait_for_completion_timeout(&aux->comp,
+	if (!poll) {
+		time_left = wait_for_completion_timeout(&aux->comp,
 						msecs_to_jiffies(250));
-	if (!time_left)
-		return -ETIMEDOUT;
+		if (!time_left)
+			return -ETIMEDOUT;
+	} else {
+		ret = dp_catalog_aux_poll_and_get_hw_interrupt(aux->catalog);
+		if (!ret)
+			dp_aux_isr(&aux->dp_aux);
+	}
 
 	return ret;
 }
@@ -238,7 +244,7 @@ static void dp_aux_update_offset_and_segment(struct dp_aux_private *aux,
  */
 static void dp_aux_transfer_helper(struct dp_aux_private *aux,
 				   struct drm_dp_aux_msg *input_msg,
-				   bool send_seg)
+				   bool send_seg, bool poll)
 {
 	struct drm_dp_aux_msg helper_msg;
 	u32 message_size = 0x10;
@@ -278,7 +284,7 @@ static void dp_aux_transfer_helper(struct dp_aux_private *aux,
 		helper_msg.address = segment_address;
 		helper_msg.buffer = &aux->segment;
 		helper_msg.size = 1;
-		dp_aux_cmd_fifo_tx(aux, &helper_msg);
+		dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
 	}
 
 	/*
@@ -292,7 +298,7 @@ static void dp_aux_transfer_helper(struct dp_aux_private *aux,
 	helper_msg.address = input_msg->address;
 	helper_msg.buffer = &aux->offset;
 	helper_msg.size = 1;
-	dp_aux_cmd_fifo_tx(aux, &helper_msg);
+	dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
 
 end:
 	aux->offset += message_size;
@@ -300,6 +306,122 @@ static void dp_aux_transfer_helper(struct dp_aux_private *aux,
 		aux->segment = 0x0; /* reset segment at end of block */
 }
 
+/*
+ * This function does the real job to process an AUX transaction.
+ * It will call aux_reset() function to reset the AUX channel,
+ * if the waiting is timeout.
+ */
+static ssize_t dp_aux_transfer_init(struct drm_dp_aux *dp_aux,
+			       struct drm_dp_aux_msg *msg)
+{
+	ssize_t ret;
+	int const aux_cmd_native_max = 16;
+	int const aux_cmd_i2c_max = 128;
+	struct dp_aux_private *aux;
+
+	aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
+
+	aux->native = msg->request & (DP_AUX_NATIVE_WRITE & DP_AUX_NATIVE_READ);
+
+	/* Ignore address only message */
+	if (msg->size == 0 || !msg->buffer) {
+		msg->reply = aux->native ?
+			DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
+		return msg->size;
+	}
+
+	/* msg sanity check */
+	if ((aux->native && msg->size > aux_cmd_native_max) ||
+	    msg->size > aux_cmd_i2c_max) {
+		DRM_ERROR("%s: invalid msg: size(%zu), request(%x)\n",
+			__func__, msg->size, msg->request);
+		return -EINVAL;
+	}
+
+	mutex_lock(&aux->mutex);
+	if (!aux->initted) {
+		ret = -EIO;
+		goto exit;
+	}
+
+	/*
+	 * For eDP it's important to give a reasonably long wait here for HPD
+	 * to be asserted. This is because the panel driver may have _just_
+	 * turned on the panel and then tried to do an AUX transfer. The panel
+	 * driver has no way of knowing when the panel is ready, so it's up
+	 * to us to wait. For DP we never get into this situation so let's
+	 * avoid ever doing the extra long wait for DP.
+	 */
+	if (aux->is_edp) {
+		ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
+		if (ret) {
+			DRM_DEBUG_DP("Panel not ready for aux transactions\n");
+			goto exit;
+		}
+	}
+
+	dp_aux_update_offset_and_segment(aux, msg);
+	dp_aux_transfer_helper(aux, msg, true, true);
+
+	aux->read = msg->request & (DP_AUX_I2C_READ & DP_AUX_NATIVE_READ);
+	aux->cmd_busy = true;
+
+	if (aux->read) {
+		aux->no_send_addr = true;
+		aux->no_send_stop = false;
+	} else {
+		aux->no_send_addr = true;
+		aux->no_send_stop = true;
+	}
+
+	ret = dp_aux_cmd_fifo_tx(aux, msg, true);
+
+	/* TODO: why is fifo_rx necessary here?
+	 * Ideally fifo_rx need not be executed for an aux write
+	 */
+	ret = dp_aux_cmd_fifo_rx(aux, msg);
+
+	if (ret < 0) {
+		if (aux->native) {
+			aux->retry_cnt++;
+			if (!(aux->retry_cnt % MAX_AUX_RETRIES))
+				dp_catalog_aux_update_cfg(aux->catalog);
+		}
+		/* reset aux if link is in connected state */
+		if (dp_catalog_link_is_connected(aux->catalog))
+			dp_catalog_aux_reset(aux->catalog);
+	} else {
+		aux->retry_cnt = 0;
+		switch (aux->aux_error_num) {
+		case DP_AUX_ERR_NONE:
+			if (aux->read)
+				ret = dp_aux_cmd_fifo_rx(aux, msg);
+			msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
+			break;
+		case DP_AUX_ERR_DEFER:
+			msg->reply = aux->native ?
+				DP_AUX_NATIVE_REPLY_DEFER : DP_AUX_I2C_REPLY_DEFER;
+			break;
+		case DP_AUX_ERR_PHY:
+		case DP_AUX_ERR_ADDR:
+		case DP_AUX_ERR_NACK:
+		case DP_AUX_ERR_NACK_DEFER:
+			msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_NACK : DP_AUX_I2C_REPLY_NACK;
+			break;
+		case DP_AUX_ERR_TOUT:
+			ret = -ETIMEDOUT;
+			break;
+		}
+	}
+
+	aux->cmd_busy = false;
+
+exit:
+	mutex_unlock(&aux->mutex);
+
+	return ret;
+}
+
 /*
  * This function does the real job to process an AUX transaction.
  * It will call aux_reset() function to reset the AUX channel,
@@ -355,7 +477,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
 	}
 
 	dp_aux_update_offset_and_segment(aux, msg);
-	dp_aux_transfer_helper(aux, msg, true);
+	dp_aux_transfer_helper(aux, msg, true, false);
 
 	aux->read = msg->request & (DP_AUX_I2C_READ & DP_AUX_NATIVE_READ);
 	aux->cmd_busy = true;
@@ -368,7 +490,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
 		aux->no_send_stop = true;
 	}
 
-	ret = dp_aux_cmd_fifo_tx(aux, msg);
+	ret = dp_aux_cmd_fifo_tx(aux, msg, false);
 	if (ret < 0) {
 		if (aux->native) {
 			aux->retry_cnt++;
@@ -535,6 +657,15 @@ struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
 	aux->catalog = catalog;
 	aux->retry_cnt = 0;
 
+	/*
+	 * Use the drm_dp_aux_init() to use the aux adapter
+	 * before registering aux with the DRM device.
+	 */
+	aux->dp_aux.name = "dpu_dp_aux";
+	aux->dp_aux.dev = dev;
+	aux->dp_aux.transfer = dp_aux_transfer_init;
+	drm_dp_aux_init(&aux->dp_aux);
+
 	return &aux->dp_aux;
 }
 
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 676279d0ca8d..ccf0400176f0 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -258,6 +258,18 @@ int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog)
 				2000, 500000);
 }
 
+int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog *dp_catalog)
+{
+	u32 aux;
+	struct dp_catalog_private *catalog = container_of(dp_catalog,
+				struct dp_catalog_private, dp_catalog);
+
+	return readl_poll_timeout(catalog->io->dp_controller.ahb.base +
+				REG_DP_INTR_STATUS,
+				aux, aux & DP_INTERRUPT_STATUS1,
+				250, 250000);
+}
+
 static void dump_regs(void __iomem *base, int len)
 {
 	int i;
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
index 1f717f45c115..ad4a9e0f71f2 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.h
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
@@ -87,6 +87,7 @@ void dp_catalog_aux_enable(struct dp_catalog *dp_catalog, bool enable);
 void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog);
 int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog);
 u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
+int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog *dp_catalog);
 
 /* DP Controller APIs */
 void dp_catalog_ctrl_state_ctrl(struct dp_catalog *dp_catalog, u32 state);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index bde1a7ce442f..a5dcef040b74 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -275,13 +275,6 @@ static int dp_display_bind(struct device *dev, struct device *master,
 	dp->dp_display.drm_dev = drm;
 	priv->dp[dp->id] = &dp->dp_display;
 
-	rc = dp->parser->parse(dp->parser);
-	if (rc) {
-		DRM_ERROR("device tree parsing failed\n");
-		goto end;
-	}
-
-
 	dp->drm_dev = drm;
 	dp->aux->drm_dev = drm;
 	rc = dp_aux_register(dp->aux);
@@ -290,12 +283,6 @@ static int dp_display_bind(struct device *dev, struct device *master,
 		goto end;
 	}
 
-	rc = dp_power_client_init(dp->power);
-	if (rc) {
-		DRM_ERROR("Power client create failed\n");
-		goto end;
-	}
-
 	rc = dp_register_audio_driver(dev, dp->audio);
 	if (rc) {
 		DRM_ERROR("Audio registration Dp failed\n");
@@ -787,6 +774,12 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
 		goto error;
 	}
 
+	rc = dp->parser->parse(dp->parser);
+	if (rc) {
+		DRM_ERROR("device tree parsing failed\n");
+		goto error;
+	}
+
 	dp->catalog = dp_catalog_get(dev, &dp->parser->io);
 	if (IS_ERR(dp->catalog)) {
 		rc = PTR_ERR(dp->catalog);
@@ -803,6 +796,12 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
 		goto error;
 	}
 
+	rc = dp_power_client_init(dp->power);
+	if (rc) {
+		DRM_ERROR("Power client create failed\n");
+		goto error;
+	}
+
 	dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
 	if (IS_ERR(dp->aux)) {
 		rc = PTR_ERR(dp->aux);
@@ -1338,12 +1337,29 @@ static int dp_display_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, &dp->dp_display);
 
+	if (dp->dp_display.is_edp) {
+		dp_display_host_init(dp);
+		dp_display_host_phy_init(dp);
+		dp_catalog_ctrl_hpd_config(dp->catalog);
+
+		rc = devm_of_dp_aux_populate_bus(dp->aux, NULL);
+
+		dp_display_host_phy_exit(dp);
+		dp_display_host_deinit(dp);
+
+		if (rc) {
+			DRM_ERROR("failed to initialize panel, rc = %d\n", rc);
+			goto error;
+		}
+	}
+
 	rc = component_add(&pdev->dev, &dp_display_comp_ops);
 	if (rc) {
 		DRM_ERROR("component add failed, rc=%d\n", rc);
 		dp_display_deinit_sub_modules(dp);
 	}
 
+error:
 	return rc;
 }
 
@@ -1546,40 +1562,8 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
 {
 	int rc;
 	struct dp_display_private *dp_priv;
-	struct device_node *aux_bus;
-	struct device *dev;
 
 	dp_priv = container_of(dp, struct dp_display_private, dp_display);
-	dev = &dp_priv->pdev->dev;
-	aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
-
-	if (aux_bus && dp->is_edp) {
-		dp_display_host_init(dp_priv);
-		dp_catalog_ctrl_hpd_config(dp_priv->catalog);
-		dp_display_host_phy_init(dp_priv);
-		enable_irq(dp_priv->irq);
-
-		/*
-		 * The code below assumes that the panel will finish probing
-		 * by the time devm_of_dp_aux_populate_ep_devices() returns.
-		 * This isn't a great assumption since it will fail if the
-		 * panel driver is probed asynchronously but is the best we
-		 * can do without a bigger driver reorganization.
-		 */
-		rc = of_dp_aux_populate_bus(dp_priv->aux, NULL);
-		of_node_put(aux_bus);
-		if (rc)
-			goto error;
-
-		rc = devm_add_action_or_reset(dp->drm_dev->dev,
-						of_dp_aux_depopulate_bus_void,
-						dp_priv->aux);
-		if (rc)
-			goto error;
-	} else if (dp->is_edp) {
-		DRM_ERROR("eDP aux_bus not found\n");
-		return -ENODEV;
-	}
 
 	/*
 	 * External bridges are mandatory for eDP interfaces: one has to
@@ -1597,12 +1581,6 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
 		return 0;
 	}
 
-error:
-	if (dp->is_edp) {
-		disable_irq(dp_priv->irq);
-		dp_display_host_phy_exit(dp_priv);
-		dp_display_host_deinit(dp_priv);
-	}
 	return rc;
 }
 
-- 
2.39.0

Re: [RFC PATCH 1/2] drm/msm/dp: enumerate edp panel during driver probe

[Dmitry Baryshkov]

On Thu, 23 Feb 2023 at 15:57, Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> The eDP panel is identified and enumerated during probe of the panel-edp
> driver. The current DP driver triggers this panel-edp driver probe while
> getting the panel-bridge associated with the eDP panel from the platform
> driver bind. If the panel-edp probe is deferred, then the whole bunch of
> MDSS parent and child drivers have to defer and roll back.

No, MDSS has not been rolled back since 5.19. Please shift your
development on top of the current msm-next.

>
> So, we want to move the panel enumeration from bind to probe so that the
> probe defer is easier to handle and also both the drivers appear consistent
> in panel enumeration. This change moves the DP driver panel-bridge
> enumeration to probe.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_aux.c     | 149 ++++++++++++++++++++++++++--
>  drivers/gpu/drm/msm/dp/dp_catalog.c |  12 +++
>  drivers/gpu/drm/msm/dp/dp_catalog.h |   1 +
>  drivers/gpu/drm/msm/dp/dp_display.c |  80 ++++++---------
>  4 files changed, 182 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index cc3efed593aa..5da95dfdeede 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -110,7 +110,7 @@ static ssize_t dp_aux_write(struct dp_aux_private *aux,
>  }
>
>  static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
> -                             struct drm_dp_aux_msg *msg)
> +                             struct drm_dp_aux_msg *msg, bool poll)

What is the reason for working in polled mode rather than always using
the interrupts?

>  {
>         ssize_t ret;
>         unsigned long time_left;
> @@ -121,10 +121,16 @@ static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
>         if (ret < 0)
>                 return ret;
>
> -       time_left = wait_for_completion_timeout(&aux->comp,
> +       if (!poll) {
> +               time_left = wait_for_completion_timeout(&aux->comp,
>                                                 msecs_to_jiffies(250));
> -       if (!time_left)
> -               return -ETIMEDOUT;
> +               if (!time_left)
> +                       return -ETIMEDOUT;
> +       } else {
> +               ret = dp_catalog_aux_poll_and_get_hw_interrupt(aux->catalog);
> +               if (!ret)
> +                       dp_aux_isr(&aux->dp_aux);
> +       }
>
>         return ret;
>  }
> @@ -238,7 +244,7 @@ static void dp_aux_update_offset_and_segment(struct dp_aux_private *aux,
>   */
>  static void dp_aux_transfer_helper(struct dp_aux_private *aux,
>                                    struct drm_dp_aux_msg *input_msg,
> -                                  bool send_seg)
> +                                  bool send_seg, bool poll)
>  {
>         struct drm_dp_aux_msg helper_msg;
>         u32 message_size = 0x10;
> @@ -278,7 +284,7 @@ static void dp_aux_transfer_helper(struct dp_aux_private *aux,
>                 helper_msg.address = segment_address;
>                 helper_msg.buffer = &aux->segment;
>                 helper_msg.size = 1;
> -               dp_aux_cmd_fifo_tx(aux, &helper_msg);
> +               dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
>         }
>
>         /*
> @@ -292,7 +298,7 @@ static void dp_aux_transfer_helper(struct dp_aux_private *aux,
>         helper_msg.address = input_msg->address;
>         helper_msg.buffer = &aux->offset;
>         helper_msg.size = 1;
> -       dp_aux_cmd_fifo_tx(aux, &helper_msg);
> +       dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
>
>  end:
>         aux->offset += message_size;
> @@ -300,6 +306,122 @@ static void dp_aux_transfer_helper(struct dp_aux_private *aux,
>                 aux->segment = 0x0; /* reset segment at end of block */
>  }
>
> +/*
> + * This function does the real job to process an AUX transaction.
> + * It will call aux_reset() function to reset the AUX channel,
> + * if the waiting is timeout.
> + */
> +static ssize_t dp_aux_transfer_init(struct drm_dp_aux *dp_aux,
> +                              struct drm_dp_aux_msg *msg)
> +{
> +       ssize_t ret;
> +       int const aux_cmd_native_max = 16;
> +       int const aux_cmd_i2c_max = 128;
> +       struct dp_aux_private *aux;
> +
> +       aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
> +
> +       aux->native = msg->request & (DP_AUX_NATIVE_WRITE & DP_AUX_NATIVE_READ);
> +
> +       /* Ignore address only message */
> +       if (msg->size == 0 || !msg->buffer) {
> +               msg->reply = aux->native ?
> +                       DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
> +               return msg->size;
> +       }
> +
> +       /* msg sanity check */
> +       if ((aux->native && msg->size > aux_cmd_native_max) ||
> +           msg->size > aux_cmd_i2c_max) {
> +               DRM_ERROR("%s: invalid msg: size(%zu), request(%x)\n",
> +                       __func__, msg->size, msg->request);
> +               return -EINVAL;
> +       }
> +
> +       mutex_lock(&aux->mutex);
> +       if (!aux->initted) {
> +               ret = -EIO;
> +               goto exit;
> +       }
> +
> +       /*
> +        * For eDP it's important to give a reasonably long wait here for HPD
> +        * to be asserted. This is because the panel driver may have _just_
> +        * turned on the panel and then tried to do an AUX transfer. The panel
> +        * driver has no way of knowing when the panel is ready, so it's up
> +        * to us to wait. For DP we never get into this situation so let's
> +        * avoid ever doing the extra long wait for DP.
> +        */
> +       if (aux->is_edp) {
> +               ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
> +               if (ret) {
> +                       DRM_DEBUG_DP("Panel not ready for aux transactions\n");
> +                       goto exit;
> +               }
> +       }
> +
> +       dp_aux_update_offset_and_segment(aux, msg);
> +       dp_aux_transfer_helper(aux, msg, true, true);
> +
> +       aux->read = msg->request & (DP_AUX_I2C_READ & DP_AUX_NATIVE_READ);
> +       aux->cmd_busy = true;
> +
> +       if (aux->read) {
> +               aux->no_send_addr = true;
> +               aux->no_send_stop = false;
> +       } else {
> +               aux->no_send_addr = true;
> +               aux->no_send_stop = true;
> +       }
> +
> +       ret = dp_aux_cmd_fifo_tx(aux, msg, true);
> +
> +       /* TODO: why is fifo_rx necessary here?
> +        * Ideally fifo_rx need not be executed for an aux write
> +        */
> +       ret = dp_aux_cmd_fifo_rx(aux, msg);
> +
> +       if (ret < 0) {
> +               if (aux->native) {
> +                       aux->retry_cnt++;
> +                       if (!(aux->retry_cnt % MAX_AUX_RETRIES))
> +                               dp_catalog_aux_update_cfg(aux->catalog);
> +               }
> +               /* reset aux if link is in connected state */
> +               if (dp_catalog_link_is_connected(aux->catalog))
> +                       dp_catalog_aux_reset(aux->catalog);
> +       } else {
> +               aux->retry_cnt = 0;
> +               switch (aux->aux_error_num) {
> +               case DP_AUX_ERR_NONE:
> +                       if (aux->read)
> +                               ret = dp_aux_cmd_fifo_rx(aux, msg);
> +                       msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
> +                       break;
> +               case DP_AUX_ERR_DEFER:
> +                       msg->reply = aux->native ?
> +                               DP_AUX_NATIVE_REPLY_DEFER : DP_AUX_I2C_REPLY_DEFER;
> +                       break;
> +               case DP_AUX_ERR_PHY:
> +               case DP_AUX_ERR_ADDR:
> +               case DP_AUX_ERR_NACK:
> +               case DP_AUX_ERR_NACK_DEFER:
> +                       msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_NACK : DP_AUX_I2C_REPLY_NACK;
> +                       break;
> +               case DP_AUX_ERR_TOUT:
> +                       ret = -ETIMEDOUT;
> +                       break;
> +               }
> +       }
> +
> +       aux->cmd_busy = false;
> +
> +exit:
> +       mutex_unlock(&aux->mutex);
> +
> +       return ret;
> +}
> +
>  /*
>   * This function does the real job to process an AUX transaction.
>   * It will call aux_reset() function to reset the AUX channel,
> @@ -355,7 +477,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>         }
>
>         dp_aux_update_offset_and_segment(aux, msg);
> -       dp_aux_transfer_helper(aux, msg, true);
> +       dp_aux_transfer_helper(aux, msg, true, false);
>
>         aux->read = msg->request & (DP_AUX_I2C_READ & DP_AUX_NATIVE_READ);
>         aux->cmd_busy = true;
> @@ -368,7 +490,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>                 aux->no_send_stop = true;
>         }
>
> -       ret = dp_aux_cmd_fifo_tx(aux, msg);
> +       ret = dp_aux_cmd_fifo_tx(aux, msg, false);
>         if (ret < 0) {
>                 if (aux->native) {
>                         aux->retry_cnt++;
> @@ -535,6 +657,15 @@ struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
>         aux->catalog = catalog;
>         aux->retry_cnt = 0;
>
> +       /*
> +        * Use the drm_dp_aux_init() to use the aux adapter
> +        * before registering aux with the DRM device.
> +        */
> +       aux->dp_aux.name = "dpu_dp_aux";
> +       aux->dp_aux.dev = dev;
> +       aux->dp_aux.transfer = dp_aux_transfer_init;
> +       drm_dp_aux_init(&aux->dp_aux);

How do you use the aux adapter? It should not be used before
aux->drm_dev is setup.

> +
>         return &aux->dp_aux;
>  }
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index 676279d0ca8d..ccf0400176f0 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -258,6 +258,18 @@ int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog)
>                                 2000, 500000);
>  }
>
> +int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog *dp_catalog)
> +{
> +       u32 aux;
> +       struct dp_catalog_private *catalog = container_of(dp_catalog,
> +                               struct dp_catalog_private, dp_catalog);
> +
> +       return readl_poll_timeout(catalog->io->dp_controller.ahb.base +
> +                               REG_DP_INTR_STATUS,
> +                               aux, aux & DP_INTERRUPT_STATUS1,
> +                               250, 250000);
> +}
> +
>  static void dump_regs(void __iomem *base, int len)
>  {
>         int i;
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
> index 1f717f45c115..ad4a9e0f71f2 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
> @@ -87,6 +87,7 @@ void dp_catalog_aux_enable(struct dp_catalog *dp_catalog, bool enable);
>  void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog);
>  int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog);
>  u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
> +int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog *dp_catalog);
>
>  /* DP Controller APIs */
>  void dp_catalog_ctrl_state_ctrl(struct dp_catalog *dp_catalog, u32 state);
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index bde1a7ce442f..a5dcef040b74 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -275,13 +275,6 @@ static int dp_display_bind(struct device *dev, struct device *master,
>         dp->dp_display.drm_dev = drm;
>         priv->dp[dp->id] = &dp->dp_display;
>
> -       rc = dp->parser->parse(dp->parser);
> -       if (rc) {
> -               DRM_ERROR("device tree parsing failed\n");
> -               goto end;
> -       }
> -
> -
>         dp->drm_dev = drm;
>         dp->aux->drm_dev = drm;
>         rc = dp_aux_register(dp->aux);
> @@ -290,12 +283,6 @@ static int dp_display_bind(struct device *dev, struct device *master,
>                 goto end;
>         }
>
> -       rc = dp_power_client_init(dp->power);
> -       if (rc) {
> -               DRM_ERROR("Power client create failed\n");
> -               goto end;
> -       }
> -
>         rc = dp_register_audio_driver(dev, dp->audio);
>         if (rc) {
>                 DRM_ERROR("Audio registration Dp failed\n");
> @@ -787,6 +774,12 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
>                 goto error;
>         }
>
> +       rc = dp->parser->parse(dp->parser);
> +       if (rc) {
> +               DRM_ERROR("device tree parsing failed\n");
> +               goto error;
> +       }
> +
>         dp->catalog = dp_catalog_get(dev, &dp->parser->io);
>         if (IS_ERR(dp->catalog)) {
>                 rc = PTR_ERR(dp->catalog);
> @@ -803,6 +796,12 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
>                 goto error;
>         }
>
> +       rc = dp_power_client_init(dp->power);
> +       if (rc) {
> +               DRM_ERROR("Power client create failed\n");
> +               goto error;
> +       }
> +
>         dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
>         if (IS_ERR(dp->aux)) {
>                 rc = PTR_ERR(dp->aux);
> @@ -1338,12 +1337,29 @@ static int dp_display_probe(struct platform_device *pdev)
>
>         platform_set_drvdata(pdev, &dp->dp_display);
>
> +       if (dp->dp_display.is_edp) {
> +               dp_display_host_init(dp);
> +               dp_display_host_phy_init(dp);
> +               dp_catalog_ctrl_hpd_config(dp->catalog);
> +
> +               rc = devm_of_dp_aux_populate_bus(dp->aux, NULL);

You should pass a real done_probing handler here, if you are going to
refactor this piece of code. The done_probing should then shut down
the resources and bind the component.

> +
> +               dp_display_host_phy_exit(dp);
> +               dp_display_host_deinit(dp);
> +
> +               if (rc) {
> +                       DRM_ERROR("failed to initialize panel, rc = %d\n", rc);
> +                       goto error;
> +               }
> +       }
> +
>         rc = component_add(&pdev->dev, &dp_display_comp_ops);
>         if (rc) {
>                 DRM_ERROR("component add failed, rc=%d\n", rc);
>                 dp_display_deinit_sub_modules(dp);
>         }
>
> +error:
>         return rc;
>  }
>
> @@ -1546,40 +1562,8 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
>  {
>         int rc;
>         struct dp_display_private *dp_priv;
> -       struct device_node *aux_bus;
> -       struct device *dev;
>
>         dp_priv = container_of(dp, struct dp_display_private, dp_display);
> -       dev = &dp_priv->pdev->dev;
> -       aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
> -
> -       if (aux_bus && dp->is_edp) {
> -               dp_display_host_init(dp_priv);
> -               dp_catalog_ctrl_hpd_config(dp_priv->catalog);
> -               dp_display_host_phy_init(dp_priv);
> -               enable_irq(dp_priv->irq);
> -
> -               /*
> -                * The code below assumes that the panel will finish probing
> -                * by the time devm_of_dp_aux_populate_ep_devices() returns.
> -                * This isn't a great assumption since it will fail if the
> -                * panel driver is probed asynchronously but is the best we
> -                * can do without a bigger driver reorganization.
> -                */
> -               rc = of_dp_aux_populate_bus(dp_priv->aux, NULL);
> -               of_node_put(aux_bus);
> -               if (rc)
> -                       goto error;
> -
> -               rc = devm_add_action_or_reset(dp->drm_dev->dev,
> -                                               of_dp_aux_depopulate_bus_void,
> -                                               dp_priv->aux);
> -               if (rc)
> -                       goto error;
> -       } else if (dp->is_edp) {
> -               DRM_ERROR("eDP aux_bus not found\n");
> -               return -ENODEV;
> -       }
>
>         /*
>          * External bridges are mandatory for eDP interfaces: one has to
> @@ -1597,12 +1581,6 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
>                 return 0;
>         }
>
> -error:
> -       if (dp->is_edp) {
> -               disable_irq(dp_priv->irq);
> -               dp_display_host_phy_exit(dp_priv);
> -               dp_display_host_deinit(dp_priv);
> -       }
>         return rc;
>  }
>
> --
> 2.39.0
>


-- 
With best wishes
Dmitry

Re: [RFC PATCH 1/2] drm/msm/dp: enumerate edp panel during driver probe

[Dmitry Baryshkov]

On Thu, 23 Feb 2023 at 15:57, Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> The eDP panel is identified and enumerated during probe of the panel-edp
> driver. The current DP driver triggers this panel-edp driver probe while
> getting the panel-bridge associated with the eDP panel from the platform
> driver bind. If the panel-edp probe is deferred, then the whole bunch of
> MDSS parent and child drivers have to defer and roll back.

No, MDSS has not been rolled back since 5.19. Please shift your
development on top of the current msm-next.

>
> So, we want to move the panel enumeration from bind to probe so that the
> probe defer is easier to handle and also both the drivers appear consistent
> in panel enumeration. This change moves the DP driver panel-bridge
> enumeration to probe.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> ---
>  drivers/gpu/drm/msm/dp/dp_aux.c     | 149 ++++++++++++++++++++++++++--
>  drivers/gpu/drm/msm/dp/dp_catalog.c |  12 +++
>  drivers/gpu/drm/msm/dp/dp_catalog.h |   1 +
>  drivers/gpu/drm/msm/dp/dp_display.c |  80 ++++++---------
>  4 files changed, 182 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index cc3efed593aa..5da95dfdeede 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -110,7 +110,7 @@ static ssize_t dp_aux_write(struct dp_aux_private *aux,
>  }
>
>  static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
> -                             struct drm_dp_aux_msg *msg)
> +                             struct drm_dp_aux_msg *msg, bool poll)

What is the reason for working in polled mode rather than always using
the interrupts?

>  {
>         ssize_t ret;
>         unsigned long time_left;
> @@ -121,10 +121,16 @@ static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
>         if (ret < 0)
>                 return ret;
>
> -       time_left = wait_for_completion_timeout(&aux->comp,
> +       if (!poll) {
> +               time_left = wait_for_completion_timeout(&aux->comp,
>                                                 msecs_to_jiffies(250));
> -       if (!time_left)
> -               return -ETIMEDOUT;
> +               if (!time_left)
> +                       return -ETIMEDOUT;
> +       } else {
> +               ret = dp_catalog_aux_poll_and_get_hw_interrupt(aux->catalog);
> +               if (!ret)
> +                       dp_aux_isr(&aux->dp_aux);
> +       }
>
>         return ret;
>  }
> @@ -238,7 +244,7 @@ static void dp_aux_update_offset_and_segment(struct dp_aux_private *aux,
>   */
>  static void dp_aux_transfer_helper(struct dp_aux_private *aux,
>                                    struct drm_dp_aux_msg *input_msg,
> -                                  bool send_seg)
> +                                  bool send_seg, bool poll)
>  {
>         struct drm_dp_aux_msg helper_msg;
>         u32 message_size = 0x10;
> @@ -278,7 +284,7 @@ static void dp_aux_transfer_helper(struct dp_aux_private *aux,
>                 helper_msg.address = segment_address;
>                 helper_msg.buffer = &aux->segment;
>                 helper_msg.size = 1;
> -               dp_aux_cmd_fifo_tx(aux, &helper_msg);
> +               dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
>         }
>
>         /*
> @@ -292,7 +298,7 @@ static void dp_aux_transfer_helper(struct dp_aux_private *aux,
>         helper_msg.address = input_msg->address;
>         helper_msg.buffer = &aux->offset;
>         helper_msg.size = 1;
> -       dp_aux_cmd_fifo_tx(aux, &helper_msg);
> +       dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
>
>  end:
>         aux->offset += message_size;
> @@ -300,6 +306,122 @@ static void dp_aux_transfer_helper(struct dp_aux_private *aux,
>                 aux->segment = 0x0; /* reset segment at end of block */
>  }
>
> +/*
> + * This function does the real job to process an AUX transaction.
> + * It will call aux_reset() function to reset the AUX channel,
> + * if the waiting is timeout.
> + */
> +static ssize_t dp_aux_transfer_init(struct drm_dp_aux *dp_aux,
> +                              struct drm_dp_aux_msg *msg)
> +{
> +       ssize_t ret;
> +       int const aux_cmd_native_max = 16;
> +       int const aux_cmd_i2c_max = 128;
> +       struct dp_aux_private *aux;
> +
> +       aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
> +
> +       aux->native = msg->request & (DP_AUX_NATIVE_WRITE & DP_AUX_NATIVE_READ);
> +
> +       /* Ignore address only message */
> +       if (msg->size == 0 || !msg->buffer) {
> +               msg->reply = aux->native ?
> +                       DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
> +               return msg->size;
> +       }
> +
> +       /* msg sanity check */
> +       if ((aux->native && msg->size > aux_cmd_native_max) ||
> +           msg->size > aux_cmd_i2c_max) {
> +               DRM_ERROR("%s: invalid msg: size(%zu), request(%x)\n",
> +                       __func__, msg->size, msg->request);
> +               return -EINVAL;
> +       }
> +
> +       mutex_lock(&aux->mutex);
> +       if (!aux->initted) {
> +               ret = -EIO;
> +               goto exit;
> +       }
> +
> +       /*
> +        * For eDP it's important to give a reasonably long wait here for HPD
> +        * to be asserted. This is because the panel driver may have _just_
> +        * turned on the panel and then tried to do an AUX transfer. The panel
> +        * driver has no way of knowing when the panel is ready, so it's up
> +        * to us to wait. For DP we never get into this situation so let's
> +        * avoid ever doing the extra long wait for DP.
> +        */
> +       if (aux->is_edp) {
> +               ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
> +               if (ret) {
> +                       DRM_DEBUG_DP("Panel not ready for aux transactions\n");
> +                       goto exit;
> +               }
> +       }
> +
> +       dp_aux_update_offset_and_segment(aux, msg);
> +       dp_aux_transfer_helper(aux, msg, true, true);
> +
> +       aux->read = msg->request & (DP_AUX_I2C_READ & DP_AUX_NATIVE_READ);
> +       aux->cmd_busy = true;
> +
> +       if (aux->read) {
> +               aux->no_send_addr = true;
> +               aux->no_send_stop = false;
> +       } else {
> +               aux->no_send_addr = true;
> +               aux->no_send_stop = true;
> +       }
> +
> +       ret = dp_aux_cmd_fifo_tx(aux, msg, true);
> +
> +       /* TODO: why is fifo_rx necessary here?
> +        * Ideally fifo_rx need not be executed for an aux write
> +        */
> +       ret = dp_aux_cmd_fifo_rx(aux, msg);
> +
> +       if (ret < 0) {
> +               if (aux->native) {
> +                       aux->retry_cnt++;
> +                       if (!(aux->retry_cnt % MAX_AUX_RETRIES))
> +                               dp_catalog_aux_update_cfg(aux->catalog);
> +               }
> +               /* reset aux if link is in connected state */
> +               if (dp_catalog_link_is_connected(aux->catalog))
> +                       dp_catalog_aux_reset(aux->catalog);
> +       } else {
> +               aux->retry_cnt = 0;
> +               switch (aux->aux_error_num) {
> +               case DP_AUX_ERR_NONE:
> +                       if (aux->read)
> +                               ret = dp_aux_cmd_fifo_rx(aux, msg);
> +                       msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
> +                       break;
> +               case DP_AUX_ERR_DEFER:
> +                       msg->reply = aux->native ?
> +                               DP_AUX_NATIVE_REPLY_DEFER : DP_AUX_I2C_REPLY_DEFER;
> +                       break;
> +               case DP_AUX_ERR_PHY:
> +               case DP_AUX_ERR_ADDR:
> +               case DP_AUX_ERR_NACK:
> +               case DP_AUX_ERR_NACK_DEFER:
> +                       msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_NACK : DP_AUX_I2C_REPLY_NACK;
> +                       break;
> +               case DP_AUX_ERR_TOUT:
> +                       ret = -ETIMEDOUT;
> +                       break;
> +               }
> +       }
> +
> +       aux->cmd_busy = false;
> +
> +exit:
> +       mutex_unlock(&aux->mutex);
> +
> +       return ret;
> +}
> +
>  /*
>   * This function does the real job to process an AUX transaction.
>   * It will call aux_reset() function to reset the AUX channel,
> @@ -355,7 +477,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>         }
>
>         dp_aux_update_offset_and_segment(aux, msg);
> -       dp_aux_transfer_helper(aux, msg, true);
> +       dp_aux_transfer_helper(aux, msg, true, false);
>
>         aux->read = msg->request & (DP_AUX_I2C_READ & DP_AUX_NATIVE_READ);
>         aux->cmd_busy = true;
> @@ -368,7 +490,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>                 aux->no_send_stop = true;
>         }
>
> -       ret = dp_aux_cmd_fifo_tx(aux, msg);
> +       ret = dp_aux_cmd_fifo_tx(aux, msg, false);
>         if (ret < 0) {
>                 if (aux->native) {
>                         aux->retry_cnt++;
> @@ -535,6 +657,15 @@ struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
>         aux->catalog = catalog;
>         aux->retry_cnt = 0;
>
> +       /*
> +        * Use the drm_dp_aux_init() to use the aux adapter
> +        * before registering aux with the DRM device.
> +        */
> +       aux->dp_aux.name = "dpu_dp_aux";
> +       aux->dp_aux.dev = dev;
> +       aux->dp_aux.transfer = dp_aux_transfer_init;
> +       drm_dp_aux_init(&aux->dp_aux);

How do you use the aux adapter? It should not be used before
aux->drm_dev is setup.

> +
>         return &aux->dp_aux;
>  }
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index 676279d0ca8d..ccf0400176f0 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -258,6 +258,18 @@ int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog)
>                                 2000, 500000);
>  }
>
> +int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog *dp_catalog)
> +{
> +       u32 aux;
> +       struct dp_catalog_private *catalog = container_of(dp_catalog,
> +                               struct dp_catalog_private, dp_catalog);
> +
> +       return readl_poll_timeout(catalog->io->dp_controller.ahb.base +
> +                               REG_DP_INTR_STATUS,
> +                               aux, aux & DP_INTERRUPT_STATUS1,
> +                               250, 250000);
> +}
> +
>  static void dump_regs(void __iomem *base, int len)
>  {
>         int i;
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
> index 1f717f45c115..ad4a9e0f71f2 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
> @@ -87,6 +87,7 @@ void dp_catalog_aux_enable(struct dp_catalog *dp_catalog, bool enable);
>  void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog);
>  int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog);
>  u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
> +int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog *dp_catalog);
>
>  /* DP Controller APIs */
>  void dp_catalog_ctrl_state_ctrl(struct dp_catalog *dp_catalog, u32 state);
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index bde1a7ce442f..a5dcef040b74 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -275,13 +275,6 @@ static int dp_display_bind(struct device *dev, struct device *master,
>         dp->dp_display.drm_dev = drm;
>         priv->dp[dp->id] = &dp->dp_display;
>
> -       rc = dp->parser->parse(dp->parser);
> -       if (rc) {
> -               DRM_ERROR("device tree parsing failed\n");
> -               goto end;
> -       }
> -
> -
>         dp->drm_dev = drm;
>         dp->aux->drm_dev = drm;
>         rc = dp_aux_register(dp->aux);
> @@ -290,12 +283,6 @@ static int dp_display_bind(struct device *dev, struct device *master,
>                 goto end;
>         }
>
> -       rc = dp_power_client_init(dp->power);
> -       if (rc) {
> -               DRM_ERROR("Power client create failed\n");
> -               goto end;
> -       }
> -
>         rc = dp_register_audio_driver(dev, dp->audio);
>         if (rc) {
>                 DRM_ERROR("Audio registration Dp failed\n");
> @@ -787,6 +774,12 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
>                 goto error;
>         }
>
> +       rc = dp->parser->parse(dp->parser);
> +       if (rc) {
> +               DRM_ERROR("device tree parsing failed\n");
> +               goto error;
> +       }
> +
>         dp->catalog = dp_catalog_get(dev, &dp->parser->io);
>         if (IS_ERR(dp->catalog)) {
>                 rc = PTR_ERR(dp->catalog);
> @@ -803,6 +796,12 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
>                 goto error;
>         }
>
> +       rc = dp_power_client_init(dp->power);
> +       if (rc) {
> +               DRM_ERROR("Power client create failed\n");
> +               goto error;
> +       }
> +
>         dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
>         if (IS_ERR(dp->aux)) {
>                 rc = PTR_ERR(dp->aux);
> @@ -1338,12 +1337,29 @@ static int dp_display_probe(struct platform_device *pdev)
>
>         platform_set_drvdata(pdev, &dp->dp_display);
>
> +       if (dp->dp_display.is_edp) {
> +               dp_display_host_init(dp);
> +               dp_display_host_phy_init(dp);
> +               dp_catalog_ctrl_hpd_config(dp->catalog);
> +
> +               rc = devm_of_dp_aux_populate_bus(dp->aux, NULL);

You should pass a real done_probing handler here, if you are going to
refactor this piece of code. The done_probing should then shut down
the resources and bind the component.

> +
> +               dp_display_host_phy_exit(dp);
> +               dp_display_host_deinit(dp);
> +
> +               if (rc) {
> +                       DRM_ERROR("failed to initialize panel, rc = %d\n", rc);
> +                       goto error;
> +               }
> +       }
> +
>         rc = component_add(&pdev->dev, &dp_display_comp_ops);
>         if (rc) {
>                 DRM_ERROR("component add failed, rc=%d\n", rc);
>                 dp_display_deinit_sub_modules(dp);
>         }
>
> +error:
>         return rc;
>  }
>
> @@ -1546,40 +1562,8 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
>  {
>         int rc;
>         struct dp_display_private *dp_priv;
> -       struct device_node *aux_bus;
> -       struct device *dev;
>
>         dp_priv = container_of(dp, struct dp_display_private, dp_display);
> -       dev = &dp_priv->pdev->dev;
> -       aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
> -
> -       if (aux_bus && dp->is_edp) {
> -               dp_display_host_init(dp_priv);
> -               dp_catalog_ctrl_hpd_config(dp_priv->catalog);
> -               dp_display_host_phy_init(dp_priv);
> -               enable_irq(dp_priv->irq);
> -
> -               /*
> -                * The code below assumes that the panel will finish probing
> -                * by the time devm_of_dp_aux_populate_ep_devices() returns.
> -                * This isn't a great assumption since it will fail if the
> -                * panel driver is probed asynchronously but is the best we
> -                * can do without a bigger driver reorganization.
> -                */
> -               rc = of_dp_aux_populate_bus(dp_priv->aux, NULL);
> -               of_node_put(aux_bus);
> -               if (rc)
> -                       goto error;
> -
> -               rc = devm_add_action_or_reset(dp->drm_dev->dev,
> -                                               of_dp_aux_depopulate_bus_void,
> -                                               dp_priv->aux);
> -               if (rc)
> -                       goto error;
> -       } else if (dp->is_edp) {
> -               DRM_ERROR("eDP aux_bus not found\n");
> -               return -ENODEV;
> -       }
>
>         /*
>          * External bridges are mandatory for eDP interfaces: one has to
> @@ -1597,12 +1581,6 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
>                 return 0;
>         }
>
> -error:
> -       if (dp->is_edp) {
> -               disable_irq(dp_priv->irq);
> -               dp_display_host_phy_exit(dp_priv);
> -               dp_display_host_deinit(dp_priv);
> -       }
>         return rc;
>  }
>
> --
> 2.39.0
>


-- 
With best wishes
Dmitry

RE: [RFC PATCH 1/2] drm/msm/dp: enumerate edp panel during driver probe

[Sankeerth Billakanti (QUIC)]

>>
>> The eDP panel is identified and enumerated during probe of the
>> panel-edp driver. The current DP driver triggers this panel-edp driver
>> probe while getting the panel-bridge associated with the eDP panel
>> from the platform driver bind. If the panel-edp probe is deferred,
>> then the whole bunch of MDSS parent and child drivers have to defer and
>roll back.
>
>No, MDSS has not been rolled back since 5.19. Please shift your development
>on top of the current msm-next.
>

Okay, I will move to the msm-next tip.

>>
>> So, we want to move the panel enumeration from bind to probe so that
>> the probe defer is easier to handle and also both the drivers appear
>> consistent in panel enumeration. This change moves the DP driver
>> panel-bridge enumeration to probe.
>>
>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
>> ---
>>  drivers/gpu/drm/msm/dp/dp_aux.c     | 149
>++++++++++++++++++++++++++--
>>  drivers/gpu/drm/msm/dp/dp_catalog.c |  12 +++
>>  drivers/gpu/drm/msm/dp/dp_catalog.h |   1 +
>>  drivers/gpu/drm/msm/dp/dp_display.c |  80 ++++++---------
>>  4 files changed, 182 insertions(+), 60 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
>> b/drivers/gpu/drm/msm/dp/dp_aux.c index cc3efed593aa..5da95dfdeede
>> 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>> @@ -110,7 +110,7 @@ static ssize_t dp_aux_write(struct dp_aux_private
>> *aux,  }
>>
>>  static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
>> -                             struct drm_dp_aux_msg *msg)
>> +                             struct drm_dp_aux_msg *msg, bool poll)
>
>What is the reason for working in polled mode rather than always using the
>interrupts?
>

The mdss interrupts will be enabled and can be used after msm_irq_install from msm_drm_bind.
We want to perform aux transactions during probe. The aux data transmission is followed by an
interrupt to indicate successful transmission status and reply information.

As interrupts are not enabled, I took this polling approach for aux interrupts during probe.

>>  {
>>         ssize_t ret;
>>         unsigned long time_left;
>> @@ -121,10 +121,16 @@ static ssize_t dp_aux_cmd_fifo_tx(struct
>dp_aux_private *aux,
>>         if (ret < 0)
>>                 return ret;
>>
>> -       time_left = wait_for_completion_timeout(&aux->comp,
>> +       if (!poll) {
>> +               time_left = wait_for_completion_timeout(&aux->comp,
>>                                                 msecs_to_jiffies(250));
>> -       if (!time_left)
>> -               return -ETIMEDOUT;
>> +               if (!time_left)
>> +                       return -ETIMEDOUT;
>> +       } else {
>> +               ret = dp_catalog_aux_poll_and_get_hw_interrupt(aux->catalog);
>> +               if (!ret)
>> +                       dp_aux_isr(&aux->dp_aux);
>> +       }
>>
>>         return ret;
>>  }
>> @@ -238,7 +244,7 @@ static void
>dp_aux_update_offset_and_segment(struct dp_aux_private *aux,
>>   */
>>  static void dp_aux_transfer_helper(struct dp_aux_private *aux,
>>                                    struct drm_dp_aux_msg *input_msg,
>> -                                  bool send_seg)
>> +                                  bool send_seg, bool poll)
>>  {
>>         struct drm_dp_aux_msg helper_msg;
>>         u32 message_size = 0x10;
>> @@ -278,7 +284,7 @@ static void dp_aux_transfer_helper(struct
>dp_aux_private *aux,
>>                 helper_msg.address = segment_address;
>>                 helper_msg.buffer = &aux->segment;
>>                 helper_msg.size = 1;
>> -               dp_aux_cmd_fifo_tx(aux, &helper_msg);
>> +               dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
>>         }
>>
>>         /*
>> @@ -292,7 +298,7 @@ static void dp_aux_transfer_helper(struct
>dp_aux_private *aux,
>>         helper_msg.address = input_msg->address;
>>         helper_msg.buffer = &aux->offset;
>>         helper_msg.size = 1;
>> -       dp_aux_cmd_fifo_tx(aux, &helper_msg);
>> +       dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
>>
>>  end:
>>         aux->offset += message_size;
>> @@ -300,6 +306,122 @@ static void dp_aux_transfer_helper(struct
>dp_aux_private *aux,
>>                 aux->segment = 0x0; /* reset segment at end of block
>> */  }
>>
>> +/*
>> + * This function does the real job to process an AUX transaction.
>> + * It will call aux_reset() function to reset the AUX channel,
>> + * if the waiting is timeout.
>> + */
>> +static ssize_t dp_aux_transfer_init(struct drm_dp_aux *dp_aux,
>> +                              struct drm_dp_aux_msg *msg) {
>> +       ssize_t ret;
>> +       int const aux_cmd_native_max = 16;
>> +       int const aux_cmd_i2c_max = 128;
>> +       struct dp_aux_private *aux;
>> +
>> +       aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
>> +
>> +       aux->native = msg->request & (DP_AUX_NATIVE_WRITE &
>> + DP_AUX_NATIVE_READ);
>> +
>> +       /* Ignore address only message */
>> +       if (msg->size == 0 || !msg->buffer) {
>> +               msg->reply = aux->native ?
>> +                       DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
>> +               return msg->size;
>> +       }
>> +
>> +       /* msg sanity check */
>> +       if ((aux->native && msg->size > aux_cmd_native_max) ||
>> +           msg->size > aux_cmd_i2c_max) {
>> +               DRM_ERROR("%s: invalid msg: size(%zu), request(%x)\n",
>> +                       __func__, msg->size, msg->request);
>> +               return -EINVAL;
>> +       }
>> +
>> +       mutex_lock(&aux->mutex);
>> +       if (!aux->initted) {
>> +               ret = -EIO;
>> +               goto exit;
>> +       }
>> +
>> +       /*
>> +        * For eDP it's important to give a reasonably long wait here for HPD
>> +        * to be asserted. This is because the panel driver may have _just_
>> +        * turned on the panel and then tried to do an AUX transfer. The panel
>> +        * driver has no way of knowing when the panel is ready, so it's up
>> +        * to us to wait. For DP we never get into this situation so let's
>> +        * avoid ever doing the extra long wait for DP.
>> +        */
>> +       if (aux->is_edp) {
>> +               ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
>> +               if (ret) {
>> +                       DRM_DEBUG_DP("Panel not ready for aux transactions\n");
>> +                       goto exit;
>> +               }
>> +       }
>> +
>> +       dp_aux_update_offset_and_segment(aux, msg);
>> +       dp_aux_transfer_helper(aux, msg, true, true);
>> +
>> +       aux->read = msg->request & (DP_AUX_I2C_READ &
>DP_AUX_NATIVE_READ);
>> +       aux->cmd_busy = true;
>> +
>> +       if (aux->read) {
>> +               aux->no_send_addr = true;
>> +               aux->no_send_stop = false;
>> +       } else {
>> +               aux->no_send_addr = true;
>> +               aux->no_send_stop = true;
>> +       }
>> +
>> +       ret = dp_aux_cmd_fifo_tx(aux, msg, true);
>> +
>> +       /* TODO: why is fifo_rx necessary here?
>> +        * Ideally fifo_rx need not be executed for an aux write
>> +        */
>> +       ret = dp_aux_cmd_fifo_rx(aux, msg);
>> +
>> +       if (ret < 0) {
>> +               if (aux->native) {
>> +                       aux->retry_cnt++;
>> +                       if (!(aux->retry_cnt % MAX_AUX_RETRIES))
>> +                               dp_catalog_aux_update_cfg(aux->catalog);
>> +               }
>> +               /* reset aux if link is in connected state */
>> +               if (dp_catalog_link_is_connected(aux->catalog))
>> +                       dp_catalog_aux_reset(aux->catalog);
>> +       } else {
>> +               aux->retry_cnt = 0;
>> +               switch (aux->aux_error_num) {
>> +               case DP_AUX_ERR_NONE:
>> +                       if (aux->read)
>> +                               ret = dp_aux_cmd_fifo_rx(aux, msg);
>> +                       msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_ACK :
>DP_AUX_I2C_REPLY_ACK;
>> +                       break;
>> +               case DP_AUX_ERR_DEFER:
>> +                       msg->reply = aux->native ?
>> +                               DP_AUX_NATIVE_REPLY_DEFER :
>DP_AUX_I2C_REPLY_DEFER;
>> +                       break;
>> +               case DP_AUX_ERR_PHY:
>> +               case DP_AUX_ERR_ADDR:
>> +               case DP_AUX_ERR_NACK:
>> +               case DP_AUX_ERR_NACK_DEFER:
>> +                       msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_NACK :
>DP_AUX_I2C_REPLY_NACK;
>> +                       break;
>> +               case DP_AUX_ERR_TOUT:
>> +                       ret = -ETIMEDOUT;
>> +                       break;
>> +               }
>> +       }
>> +
>> +       aux->cmd_busy = false;
>> +
>> +exit:
>> +       mutex_unlock(&aux->mutex);
>> +
>> +       return ret;
>> +}
>> +
>>  /*
>>   * This function does the real job to process an AUX transaction.
>>   * It will call aux_reset() function to reset the AUX channel, @@
>> -355,7 +477,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
>*dp_aux,
>>         }
>>
>>         dp_aux_update_offset_and_segment(aux, msg);
>> -       dp_aux_transfer_helper(aux, msg, true);
>> +       dp_aux_transfer_helper(aux, msg, true, false);
>>
>>         aux->read = msg->request & (DP_AUX_I2C_READ &
>DP_AUX_NATIVE_READ);
>>         aux->cmd_busy = true;
>> @@ -368,7 +490,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
>*dp_aux,
>>                 aux->no_send_stop = true;
>>         }
>>
>> -       ret = dp_aux_cmd_fifo_tx(aux, msg);
>> +       ret = dp_aux_cmd_fifo_tx(aux, msg, false);
>>         if (ret < 0) {
>>                 if (aux->native) {
>>                         aux->retry_cnt++; @@ -535,6 +657,15 @@ struct
>> drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
>>         aux->catalog = catalog;
>>         aux->retry_cnt = 0;
>>
>> +       /*
>> +        * Use the drm_dp_aux_init() to use the aux adapter
>> +        * before registering aux with the DRM device.
>> +        */
>> +       aux->dp_aux.name = "dpu_dp_aux";
>> +       aux->dp_aux.dev = dev;
>> +       aux->dp_aux.transfer = dp_aux_transfer_init;
>> +       drm_dp_aux_init(&aux->dp_aux);
>
>How do you use the aux adapter? It should not be used before
>aux->drm_dev is setup.
>

The drm_dev registration happens during the bind. But we need to use aux during probe.

The kernel doc for the drm_dp_aux_init function suggested we can use the adapter before
drm_dev registration. So, I used this function to enable the aux usage during probe.

/**
 * drm_dp_aux_init() - minimally initialise an aux channel
 * @aux: DisplayPort AUX channel
 *
 * If you need to use the drm_dp_aux's i2c adapter prior to registering it with
 * the outside world, call drm_dp_aux_init() first.

>> +
>>         return &aux->dp_aux;
>>  }
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> index 676279d0ca8d..ccf0400176f0 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> @@ -258,6 +258,18 @@ int
>dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog
>*dp_catalog)
>>                                 2000, 500000);  }
>>
>> +int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog
>> +*dp_catalog) {
>> +       u32 aux;
>> +       struct dp_catalog_private *catalog = container_of(dp_catalog,
>> +                               struct dp_catalog_private,
>> +dp_catalog);
>> +
>> +       return readl_poll_timeout(catalog->io->dp_controller.ahb.base +
>> +                               REG_DP_INTR_STATUS,
>> +                               aux, aux & DP_INTERRUPT_STATUS1,
>> +                               250, 250000); }
>> +
>>  static void dump_regs(void __iomem *base, int len)  {
>>         int i;
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h
>> b/drivers/gpu/drm/msm/dp/dp_catalog.h
>> index 1f717f45c115..ad4a9e0f71f2 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
>> @@ -87,6 +87,7 @@ void dp_catalog_aux_enable(struct dp_catalog
>> *dp_catalog, bool enable);  void dp_catalog_aux_update_cfg(struct
>> dp_catalog *dp_catalog);  int
>> dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog
>> *dp_catalog);
>>  u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
>> +int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog
>> +*dp_catalog);
>>
>>  /* DP Controller APIs */
>>  void dp_catalog_ctrl_state_ctrl(struct dp_catalog *dp_catalog, u32
>> state); diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index bde1a7ce442f..a5dcef040b74 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -275,13 +275,6 @@ static int dp_display_bind(struct device *dev, struct
>device *master,
>>         dp->dp_display.drm_dev = drm;
>>         priv->dp[dp->id] = &dp->dp_display;
>>
>> -       rc = dp->parser->parse(dp->parser);
>> -       if (rc) {
>> -               DRM_ERROR("device tree parsing failed\n");
>> -               goto end;
>> -       }
>> -
>> -
>>         dp->drm_dev = drm;
>>         dp->aux->drm_dev = drm;
>>         rc = dp_aux_register(dp->aux); @@ -290,12 +283,6 @@ static int
>> dp_display_bind(struct device *dev, struct device *master,
>>                 goto end;
>>         }
>>
>> -       rc = dp_power_client_init(dp->power);
>> -       if (rc) {
>> -               DRM_ERROR("Power client create failed\n");
>> -               goto end;
>> -       }
>> -
>>         rc = dp_register_audio_driver(dev, dp->audio);
>>         if (rc) {
>>                 DRM_ERROR("Audio registration Dp failed\n"); @@ -787,6
>> +774,12 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
>>                 goto error;
>>         }
>>
>> +       rc = dp->parser->parse(dp->parser);
>> +       if (rc) {
>> +               DRM_ERROR("device tree parsing failed\n");
>> +               goto error;
>> +       }
>> +
>>         dp->catalog = dp_catalog_get(dev, &dp->parser->io);
>>         if (IS_ERR(dp->catalog)) {
>>                 rc = PTR_ERR(dp->catalog); @@ -803,6 +796,12 @@ static
>> int dp_init_sub_modules(struct dp_display_private *dp)
>>                 goto error;
>>         }
>>
>> +       rc = dp_power_client_init(dp->power);
>> +       if (rc) {
>> +               DRM_ERROR("Power client create failed\n");
>> +               goto error;
>> +       }
>> +
>>         dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
>>         if (IS_ERR(dp->aux)) {
>>                 rc = PTR_ERR(dp->aux); @@ -1338,12 +1337,29 @@ static
>> int dp_display_probe(struct platform_device *pdev)
>>
>>         platform_set_drvdata(pdev, &dp->dp_display);
>>
>> +       if (dp->dp_display.is_edp) {
>> +               dp_display_host_init(dp);
>> +               dp_display_host_phy_init(dp);
>> +               dp_catalog_ctrl_hpd_config(dp->catalog);
>> +
>> +               rc = devm_of_dp_aux_populate_bus(dp->aux, NULL);
>
>You should pass a real done_probing handler here, if you are going to refactor
>this piece of code. The done_probing should then shut down the resources
>and bind the component.
>

I removed the resource enabling part from probe in the next patch where I implemented pm_runtime_ops.
I moved the host_init, phy_init and hpd_config into runtime_resume and calling pm_runtime_get_sync from aux_transfer.

>> +
>> +               dp_display_host_phy_exit(dp);
>> +               dp_display_host_deinit(dp);
>> +
>> +               if (rc) {
>> +                       DRM_ERROR("failed to initialize panel, rc = %d\n", rc);
>> +                       goto error;
>> +               }
>> +       }
>> +
>>         rc = component_add(&pdev->dev, &dp_display_comp_ops);
>>         if (rc) {
>>                 DRM_ERROR("component add failed, rc=%d\n", rc);
>>                 dp_display_deinit_sub_modules(dp);
>>         }
>>
>> +error:
>>         return rc;
>>  }
>>
>> @@ -1546,40 +1562,8 @@ static int dp_display_get_next_bridge(struct
>> msm_dp *dp)  {
>>         int rc;
>>         struct dp_display_private *dp_priv;
>> -       struct device_node *aux_bus;
>> -       struct device *dev;
>>
>>         dp_priv = container_of(dp, struct dp_display_private, dp_display);
>> -       dev = &dp_priv->pdev->dev;
>> -       aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
>> -
>> -       if (aux_bus && dp->is_edp) {
>> -               dp_display_host_init(dp_priv);
>> -               dp_catalog_ctrl_hpd_config(dp_priv->catalog);
>> -               dp_display_host_phy_init(dp_priv);
>> -               enable_irq(dp_priv->irq);
>> -
>> -               /*
>> -                * The code below assumes that the panel will finish probing
>> -                * by the time devm_of_dp_aux_populate_ep_devices() returns.
>> -                * This isn't a great assumption since it will fail if the
>> -                * panel driver is probed asynchronously but is the best we
>> -                * can do without a bigger driver reorganization.
>> -                */
>> -               rc = of_dp_aux_populate_bus(dp_priv->aux, NULL);
>> -               of_node_put(aux_bus);
>> -               if (rc)
>> -                       goto error;
>> -
>> -               rc = devm_add_action_or_reset(dp->drm_dev->dev,
>> -                                               of_dp_aux_depopulate_bus_void,
>> -                                               dp_priv->aux);
>> -               if (rc)
>> -                       goto error;
>> -       } else if (dp->is_edp) {
>> -               DRM_ERROR("eDP aux_bus not found\n");
>> -               return -ENODEV;
>> -       }
>>
>>         /*
>>          * External bridges are mandatory for eDP interfaces: one has
>> to @@ -1597,12 +1581,6 @@ static int dp_display_get_next_bridge(struct
>msm_dp *dp)
>>                 return 0;
>>         }
>>
>> -error:
>> -       if (dp->is_edp) {
>> -               disable_irq(dp_priv->irq);
>> -               dp_display_host_phy_exit(dp_priv);
>> -               dp_display_host_deinit(dp_priv);
>> -       }
>>         return rc;
>>  }
>>
>> --
>> 2.39.0
>>
>
>
>--
>With best wishes
>Dmitry

RE: [RFC PATCH 1/2] drm/msm/dp: enumerate edp panel during driver probe

[Sankeerth Billakanti (QUIC)]

>>
>> The eDP panel is identified and enumerated during probe of the
>> panel-edp driver. The current DP driver triggers this panel-edp driver
>> probe while getting the panel-bridge associated with the eDP panel
>> from the platform driver bind. If the panel-edp probe is deferred,
>> then the whole bunch of MDSS parent and child drivers have to defer and
>roll back.
>
>No, MDSS has not been rolled back since 5.19. Please shift your development
>on top of the current msm-next.
>

Okay, I will move to the msm-next tip.

>>
>> So, we want to move the panel enumeration from bind to probe so that
>> the probe defer is easier to handle and also both the drivers appear
>> consistent in panel enumeration. This change moves the DP driver
>> panel-bridge enumeration to probe.
>>
>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
>> ---
>>  drivers/gpu/drm/msm/dp/dp_aux.c     | 149
>++++++++++++++++++++++++++--
>>  drivers/gpu/drm/msm/dp/dp_catalog.c |  12 +++
>>  drivers/gpu/drm/msm/dp/dp_catalog.h |   1 +
>>  drivers/gpu/drm/msm/dp/dp_display.c |  80 ++++++---------
>>  4 files changed, 182 insertions(+), 60 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
>> b/drivers/gpu/drm/msm/dp/dp_aux.c index cc3efed593aa..5da95dfdeede
>> 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>> @@ -110,7 +110,7 @@ static ssize_t dp_aux_write(struct dp_aux_private
>> *aux,  }
>>
>>  static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
>> -                             struct drm_dp_aux_msg *msg)
>> +                             struct drm_dp_aux_msg *msg, bool poll)
>
>What is the reason for working in polled mode rather than always using the
>interrupts?
>

The mdss interrupts will be enabled and can be used after msm_irq_install from msm_drm_bind.
We want to perform aux transactions during probe. The aux data transmission is followed by an
interrupt to indicate successful transmission status and reply information.

As interrupts are not enabled, I took this polling approach for aux interrupts during probe.

>>  {
>>         ssize_t ret;
>>         unsigned long time_left;
>> @@ -121,10 +121,16 @@ static ssize_t dp_aux_cmd_fifo_tx(struct
>dp_aux_private *aux,
>>         if (ret < 0)
>>                 return ret;
>>
>> -       time_left = wait_for_completion_timeout(&aux->comp,
>> +       if (!poll) {
>> +               time_left = wait_for_completion_timeout(&aux->comp,
>>                                                 msecs_to_jiffies(250));
>> -       if (!time_left)
>> -               return -ETIMEDOUT;
>> +               if (!time_left)
>> +                       return -ETIMEDOUT;
>> +       } else {
>> +               ret = dp_catalog_aux_poll_and_get_hw_interrupt(aux->catalog);
>> +               if (!ret)
>> +                       dp_aux_isr(&aux->dp_aux);
>> +       }
>>
>>         return ret;
>>  }
>> @@ -238,7 +244,7 @@ static void
>dp_aux_update_offset_and_segment(struct dp_aux_private *aux,
>>   */
>>  static void dp_aux_transfer_helper(struct dp_aux_private *aux,
>>                                    struct drm_dp_aux_msg *input_msg,
>> -                                  bool send_seg)
>> +                                  bool send_seg, bool poll)
>>  {
>>         struct drm_dp_aux_msg helper_msg;
>>         u32 message_size = 0x10;
>> @@ -278,7 +284,7 @@ static void dp_aux_transfer_helper(struct
>dp_aux_private *aux,
>>                 helper_msg.address = segment_address;
>>                 helper_msg.buffer = &aux->segment;
>>                 helper_msg.size = 1;
>> -               dp_aux_cmd_fifo_tx(aux, &helper_msg);
>> +               dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
>>         }
>>
>>         /*
>> @@ -292,7 +298,7 @@ static void dp_aux_transfer_helper(struct
>dp_aux_private *aux,
>>         helper_msg.address = input_msg->address;
>>         helper_msg.buffer = &aux->offset;
>>         helper_msg.size = 1;
>> -       dp_aux_cmd_fifo_tx(aux, &helper_msg);
>> +       dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
>>
>>  end:
>>         aux->offset += message_size;
>> @@ -300,6 +306,122 @@ static void dp_aux_transfer_helper(struct
>dp_aux_private *aux,
>>                 aux->segment = 0x0; /* reset segment at end of block
>> */  }
>>
>> +/*
>> + * This function does the real job to process an AUX transaction.
>> + * It will call aux_reset() function to reset the AUX channel,
>> + * if the waiting is timeout.
>> + */
>> +static ssize_t dp_aux_transfer_init(struct drm_dp_aux *dp_aux,
>> +                              struct drm_dp_aux_msg *msg) {
>> +       ssize_t ret;
>> +       int const aux_cmd_native_max = 16;
>> +       int const aux_cmd_i2c_max = 128;
>> +       struct dp_aux_private *aux;
>> +
>> +       aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
>> +
>> +       aux->native = msg->request & (DP_AUX_NATIVE_WRITE &
>> + DP_AUX_NATIVE_READ);
>> +
>> +       /* Ignore address only message */
>> +       if (msg->size == 0 || !msg->buffer) {
>> +               msg->reply = aux->native ?
>> +                       DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
>> +               return msg->size;
>> +       }
>> +
>> +       /* msg sanity check */
>> +       if ((aux->native && msg->size > aux_cmd_native_max) ||
>> +           msg->size > aux_cmd_i2c_max) {
>> +               DRM_ERROR("%s: invalid msg: size(%zu), request(%x)\n",
>> +                       __func__, msg->size, msg->request);
>> +               return -EINVAL;
>> +       }
>> +
>> +       mutex_lock(&aux->mutex);
>> +       if (!aux->initted) {
>> +               ret = -EIO;
>> +               goto exit;
>> +       }
>> +
>> +       /*
>> +        * For eDP it's important to give a reasonably long wait here for HPD
>> +        * to be asserted. This is because the panel driver may have _just_
>> +        * turned on the panel and then tried to do an AUX transfer. The panel
>> +        * driver has no way of knowing when the panel is ready, so it's up
>> +        * to us to wait. For DP we never get into this situation so let's
>> +        * avoid ever doing the extra long wait for DP.
>> +        */
>> +       if (aux->is_edp) {
>> +               ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
>> +               if (ret) {
>> +                       DRM_DEBUG_DP("Panel not ready for aux transactions\n");
>> +                       goto exit;
>> +               }
>> +       }
>> +
>> +       dp_aux_update_offset_and_segment(aux, msg);
>> +       dp_aux_transfer_helper(aux, msg, true, true);
>> +
>> +       aux->read = msg->request & (DP_AUX_I2C_READ &
>DP_AUX_NATIVE_READ);
>> +       aux->cmd_busy = true;
>> +
>> +       if (aux->read) {
>> +               aux->no_send_addr = true;
>> +               aux->no_send_stop = false;
>> +       } else {
>> +               aux->no_send_addr = true;
>> +               aux->no_send_stop = true;
>> +       }
>> +
>> +       ret = dp_aux_cmd_fifo_tx(aux, msg, true);
>> +
>> +       /* TODO: why is fifo_rx necessary here?
>> +        * Ideally fifo_rx need not be executed for an aux write
>> +        */
>> +       ret = dp_aux_cmd_fifo_rx(aux, msg);
>> +
>> +       if (ret < 0) {
>> +               if (aux->native) {
>> +                       aux->retry_cnt++;
>> +                       if (!(aux->retry_cnt % MAX_AUX_RETRIES))
>> +                               dp_catalog_aux_update_cfg(aux->catalog);
>> +               }
>> +               /* reset aux if link is in connected state */
>> +               if (dp_catalog_link_is_connected(aux->catalog))
>> +                       dp_catalog_aux_reset(aux->catalog);
>> +       } else {
>> +               aux->retry_cnt = 0;
>> +               switch (aux->aux_error_num) {
>> +               case DP_AUX_ERR_NONE:
>> +                       if (aux->read)
>> +                               ret = dp_aux_cmd_fifo_rx(aux, msg);
>> +                       msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_ACK :
>DP_AUX_I2C_REPLY_ACK;
>> +                       break;
>> +               case DP_AUX_ERR_DEFER:
>> +                       msg->reply = aux->native ?
>> +                               DP_AUX_NATIVE_REPLY_DEFER :
>DP_AUX_I2C_REPLY_DEFER;
>> +                       break;
>> +               case DP_AUX_ERR_PHY:
>> +               case DP_AUX_ERR_ADDR:
>> +               case DP_AUX_ERR_NACK:
>> +               case DP_AUX_ERR_NACK_DEFER:
>> +                       msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_NACK :
>DP_AUX_I2C_REPLY_NACK;
>> +                       break;
>> +               case DP_AUX_ERR_TOUT:
>> +                       ret = -ETIMEDOUT;
>> +                       break;
>> +               }
>> +       }
>> +
>> +       aux->cmd_busy = false;
>> +
>> +exit:
>> +       mutex_unlock(&aux->mutex);
>> +
>> +       return ret;
>> +}
>> +
>>  /*
>>   * This function does the real job to process an AUX transaction.
>>   * It will call aux_reset() function to reset the AUX channel, @@
>> -355,7 +477,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
>*dp_aux,
>>         }
>>
>>         dp_aux_update_offset_and_segment(aux, msg);
>> -       dp_aux_transfer_helper(aux, msg, true);
>> +       dp_aux_transfer_helper(aux, msg, true, false);
>>
>>         aux->read = msg->request & (DP_AUX_I2C_READ &
>DP_AUX_NATIVE_READ);
>>         aux->cmd_busy = true;
>> @@ -368,7 +490,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
>*dp_aux,
>>                 aux->no_send_stop = true;
>>         }
>>
>> -       ret = dp_aux_cmd_fifo_tx(aux, msg);
>> +       ret = dp_aux_cmd_fifo_tx(aux, msg, false);
>>         if (ret < 0) {
>>                 if (aux->native) {
>>                         aux->retry_cnt++; @@ -535,6 +657,15 @@ struct
>> drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
>>         aux->catalog = catalog;
>>         aux->retry_cnt = 0;
>>
>> +       /*
>> +        * Use the drm_dp_aux_init() to use the aux adapter
>> +        * before registering aux with the DRM device.
>> +        */
>> +       aux->dp_aux.name = "dpu_dp_aux";
>> +       aux->dp_aux.dev = dev;
>> +       aux->dp_aux.transfer = dp_aux_transfer_init;
>> +       drm_dp_aux_init(&aux->dp_aux);
>
>How do you use the aux adapter? It should not be used before
>aux->drm_dev is setup.
>

The drm_dev registration happens during the bind. But we need to use aux during probe.

The kernel doc for the drm_dp_aux_init function suggested we can use the adapter before
drm_dev registration. So, I used this function to enable the aux usage during probe.

/**
 * drm_dp_aux_init() - minimally initialise an aux channel
 * @aux: DisplayPort AUX channel
 *
 * If you need to use the drm_dp_aux's i2c adapter prior to registering it with
 * the outside world, call drm_dp_aux_init() first.

>> +
>>         return &aux->dp_aux;
>>  }
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> index 676279d0ca8d..ccf0400176f0 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> @@ -258,6 +258,18 @@ int
>dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog
>*dp_catalog)
>>                                 2000, 500000);  }
>>
>> +int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog
>> +*dp_catalog) {
>> +       u32 aux;
>> +       struct dp_catalog_private *catalog = container_of(dp_catalog,
>> +                               struct dp_catalog_private,
>> +dp_catalog);
>> +
>> +       return readl_poll_timeout(catalog->io->dp_controller.ahb.base +
>> +                               REG_DP_INTR_STATUS,
>> +                               aux, aux & DP_INTERRUPT_STATUS1,
>> +                               250, 250000); }
>> +
>>  static void dump_regs(void __iomem *base, int len)  {
>>         int i;
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h
>> b/drivers/gpu/drm/msm/dp/dp_catalog.h
>> index 1f717f45c115..ad4a9e0f71f2 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
>> @@ -87,6 +87,7 @@ void dp_catalog_aux_enable(struct dp_catalog
>> *dp_catalog, bool enable);  void dp_catalog_aux_update_cfg(struct
>> dp_catalog *dp_catalog);  int
>> dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog
>> *dp_catalog);
>>  u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
>> +int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog
>> +*dp_catalog);
>>
>>  /* DP Controller APIs */
>>  void dp_catalog_ctrl_state_ctrl(struct dp_catalog *dp_catalog, u32
>> state); diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index bde1a7ce442f..a5dcef040b74 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -275,13 +275,6 @@ static int dp_display_bind(struct device *dev, struct
>device *master,
>>         dp->dp_display.drm_dev = drm;
>>         priv->dp[dp->id] = &dp->dp_display;
>>
>> -       rc = dp->parser->parse(dp->parser);
>> -       if (rc) {
>> -               DRM_ERROR("device tree parsing failed\n");
>> -               goto end;
>> -       }
>> -
>> -
>>         dp->drm_dev = drm;
>>         dp->aux->drm_dev = drm;
>>         rc = dp_aux_register(dp->aux); @@ -290,12 +283,6 @@ static int
>> dp_display_bind(struct device *dev, struct device *master,
>>                 goto end;
>>         }
>>
>> -       rc = dp_power_client_init(dp->power);
>> -       if (rc) {
>> -               DRM_ERROR("Power client create failed\n");
>> -               goto end;
>> -       }
>> -
>>         rc = dp_register_audio_driver(dev, dp->audio);
>>         if (rc) {
>>                 DRM_ERROR("Audio registration Dp failed\n"); @@ -787,6
>> +774,12 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
>>                 goto error;
>>         }
>>
>> +       rc = dp->parser->parse(dp->parser);
>> +       if (rc) {
>> +               DRM_ERROR("device tree parsing failed\n");
>> +               goto error;
>> +       }
>> +
>>         dp->catalog = dp_catalog_get(dev, &dp->parser->io);
>>         if (IS_ERR(dp->catalog)) {
>>                 rc = PTR_ERR(dp->catalog); @@ -803,6 +796,12 @@ static
>> int dp_init_sub_modules(struct dp_display_private *dp)
>>                 goto error;
>>         }
>>
>> +       rc = dp_power_client_init(dp->power);
>> +       if (rc) {
>> +               DRM_ERROR("Power client create failed\n");
>> +               goto error;
>> +       }
>> +
>>         dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
>>         if (IS_ERR(dp->aux)) {
>>                 rc = PTR_ERR(dp->aux); @@ -1338,12 +1337,29 @@ static
>> int dp_display_probe(struct platform_device *pdev)
>>
>>         platform_set_drvdata(pdev, &dp->dp_display);
>>
>> +       if (dp->dp_display.is_edp) {
>> +               dp_display_host_init(dp);
>> +               dp_display_host_phy_init(dp);
>> +               dp_catalog_ctrl_hpd_config(dp->catalog);
>> +
>> +               rc = devm_of_dp_aux_populate_bus(dp->aux, NULL);
>
>You should pass a real done_probing handler here, if you are going to refactor
>this piece of code. The done_probing should then shut down the resources
>and bind the component.
>

I removed the resource enabling part from probe in the next patch where I implemented pm_runtime_ops.
I moved the host_init, phy_init and hpd_config into runtime_resume and calling pm_runtime_get_sync from aux_transfer.

>> +
>> +               dp_display_host_phy_exit(dp);
>> +               dp_display_host_deinit(dp);
>> +
>> +               if (rc) {
>> +                       DRM_ERROR("failed to initialize panel, rc = %d\n", rc);
>> +                       goto error;
>> +               }
>> +       }
>> +
>>         rc = component_add(&pdev->dev, &dp_display_comp_ops);
>>         if (rc) {
>>                 DRM_ERROR("component add failed, rc=%d\n", rc);
>>                 dp_display_deinit_sub_modules(dp);
>>         }
>>
>> +error:
>>         return rc;
>>  }
>>
>> @@ -1546,40 +1562,8 @@ static int dp_display_get_next_bridge(struct
>> msm_dp *dp)  {
>>         int rc;
>>         struct dp_display_private *dp_priv;
>> -       struct device_node *aux_bus;
>> -       struct device *dev;
>>
>>         dp_priv = container_of(dp, struct dp_display_private, dp_display);
>> -       dev = &dp_priv->pdev->dev;
>> -       aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
>> -
>> -       if (aux_bus && dp->is_edp) {
>> -               dp_display_host_init(dp_priv);
>> -               dp_catalog_ctrl_hpd_config(dp_priv->catalog);
>> -               dp_display_host_phy_init(dp_priv);
>> -               enable_irq(dp_priv->irq);
>> -
>> -               /*
>> -                * The code below assumes that the panel will finish probing
>> -                * by the time devm_of_dp_aux_populate_ep_devices() returns.
>> -                * This isn't a great assumption since it will fail if the
>> -                * panel driver is probed asynchronously but is the best we
>> -                * can do without a bigger driver reorganization.
>> -                */
>> -               rc = of_dp_aux_populate_bus(dp_priv->aux, NULL);
>> -               of_node_put(aux_bus);
>> -               if (rc)
>> -                       goto error;
>> -
>> -               rc = devm_add_action_or_reset(dp->drm_dev->dev,
>> -                                               of_dp_aux_depopulate_bus_void,
>> -                                               dp_priv->aux);
>> -               if (rc)
>> -                       goto error;
>> -       } else if (dp->is_edp) {
>> -               DRM_ERROR("eDP aux_bus not found\n");
>> -               return -ENODEV;
>> -       }
>>
>>         /*
>>          * External bridges are mandatory for eDP interfaces: one has
>> to @@ -1597,12 +1581,6 @@ static int dp_display_get_next_bridge(struct
>msm_dp *dp)
>>                 return 0;
>>         }
>>
>> -error:
>> -       if (dp->is_edp) {
>> -               disable_irq(dp_priv->irq);
>> -               dp_display_host_phy_exit(dp_priv);
>> -               dp_display_host_deinit(dp_priv);
>> -       }
>>         return rc;
>>  }
>>
>> --
>> 2.39.0
>>
>
>
>--
>With best wishes
>Dmitry

Re: [RFC PATCH 1/2] drm/msm/dp: enumerate edp panel during driver probe

[Dmitry Baryshkov]

On 01/03/2023 10:13, Sankeerth Billakanti (QUIC) wrote:
>>>
>>> The eDP panel is identified and enumerated during probe of the
>>> panel-edp driver. The current DP driver triggers this panel-edp driver
>>> probe while getting the panel-bridge associated with the eDP panel
>>> from the platform driver bind. If the panel-edp probe is deferred,
>>> then the whole bunch of MDSS parent and child drivers have to defer and
>> roll back.
>>
>> No, MDSS has not been rolled back since 5.19. Please shift your development
>> on top of the current msm-next.
>>
> 
> Okay, I will move to the msm-next tip.
> 
>>>
>>> So, we want to move the panel enumeration from bind to probe so that
>>> the probe defer is easier to handle and also both the drivers appear
>>> consistent in panel enumeration. This change moves the DP driver
>>> panel-bridge enumeration to probe.
>>>
>>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/dp/dp_aux.c     | 149
>> ++++++++++++++++++++++++++--
>>>   drivers/gpu/drm/msm/dp/dp_catalog.c |  12 +++
>>>   drivers/gpu/drm/msm/dp/dp_catalog.h |   1 +
>>>   drivers/gpu/drm/msm/dp/dp_display.c |  80 ++++++---------
>>>   4 files changed, 182 insertions(+), 60 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
>>> b/drivers/gpu/drm/msm/dp/dp_aux.c index cc3efed593aa..5da95dfdeede
>>> 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>>> @@ -110,7 +110,7 @@ static ssize_t dp_aux_write(struct dp_aux_private
>>> *aux,  }
>>>
>>>   static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
>>> -                             struct drm_dp_aux_msg *msg)
>>> +                             struct drm_dp_aux_msg *msg, bool poll)
>>
>> What is the reason for working in polled mode rather than always using the
>> interrupts?
>>
> 
> The mdss interrupts will be enabled and can be used after msm_irq_install from msm_drm_bind.
> We want to perform aux transactions during probe. The aux data transmission is followed by an
> interrupt to indicate successful transmission status and reply information.

This is the price of developing on, let me guess, 5.15. Nowadays MDSS 
interrupts are enabled and can be used during dp_display_probe() and 
dp_display_bind(). If they can not for some reason, this is an issue 
that must be fixed. Please reconsider your approach after rebasing onto 
msm-next or 6.3-rc1.

> 
> As interrupts are not enabled, I took this polling approach for aux interrupts during probe.
> 
>>>   {
>>>          ssize_t ret;
>>>          unsigned long time_left;
>>> @@ -121,10 +121,16 @@ static ssize_t dp_aux_cmd_fifo_tx(struct
>> dp_aux_private *aux,
>>>          if (ret < 0)
>>>                  return ret;
>>>
>>> -       time_left = wait_for_completion_timeout(&aux->comp,
>>> +       if (!poll) {
>>> +               time_left = wait_for_completion_timeout(&aux->comp,
>>>                                                  msecs_to_jiffies(250));
>>> -       if (!time_left)
>>> -               return -ETIMEDOUT;
>>> +               if (!time_left)
>>> +                       return -ETIMEDOUT;
>>> +       } else {
>>> +               ret = dp_catalog_aux_poll_and_get_hw_interrupt(aux->catalog);
>>> +               if (!ret)
>>> +                       dp_aux_isr(&aux->dp_aux);
>>> +       }
>>>
>>>          return ret;
>>>   }
>>> @@ -238,7 +244,7 @@ static void
>> dp_aux_update_offset_and_segment(struct dp_aux_private *aux,
>>>    */
>>>   static void dp_aux_transfer_helper(struct dp_aux_private *aux,
>>>                                     struct drm_dp_aux_msg *input_msg,
>>> -                                  bool send_seg)
>>> +                                  bool send_seg, bool poll)
>>>   {
>>>          struct drm_dp_aux_msg helper_msg;
>>>          u32 message_size = 0x10;
>>> @@ -278,7 +284,7 @@ static void dp_aux_transfer_helper(struct
>> dp_aux_private *aux,
>>>                  helper_msg.address = segment_address;
>>>                  helper_msg.buffer = &aux->segment;
>>>                  helper_msg.size = 1;
>>> -               dp_aux_cmd_fifo_tx(aux, &helper_msg);
>>> +               dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
>>>          }
>>>
>>>          /*
>>> @@ -292,7 +298,7 @@ static void dp_aux_transfer_helper(struct
>> dp_aux_private *aux,
>>>          helper_msg.address = input_msg->address;
>>>          helper_msg.buffer = &aux->offset;
>>>          helper_msg.size = 1;
>>> -       dp_aux_cmd_fifo_tx(aux, &helper_msg);
>>> +       dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
>>>
>>>   end:
>>>          aux->offset += message_size;
>>> @@ -300,6 +306,122 @@ static void dp_aux_transfer_helper(struct
>> dp_aux_private *aux,
>>>                  aux->segment = 0x0; /* reset segment at end of block
>>> */  }
>>>
>>> +/*
>>> + * This function does the real job to process an AUX transaction.
>>> + * It will call aux_reset() function to reset the AUX channel,
>>> + * if the waiting is timeout.
>>> + */
>>> +static ssize_t dp_aux_transfer_init(struct drm_dp_aux *dp_aux,
>>> +                              struct drm_dp_aux_msg *msg) {
>>> +       ssize_t ret;
>>> +       int const aux_cmd_native_max = 16;
>>> +       int const aux_cmd_i2c_max = 128;
>>> +       struct dp_aux_private *aux;
>>> +
>>> +       aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
>>> +
>>> +       aux->native = msg->request & (DP_AUX_NATIVE_WRITE &
>>> + DP_AUX_NATIVE_READ);
>>> +
>>> +       /* Ignore address only message */
>>> +       if (msg->size == 0 || !msg->buffer) {
>>> +               msg->reply = aux->native ?
>>> +                       DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
>>> +               return msg->size;
>>> +       }
>>> +
>>> +       /* msg sanity check */
>>> +       if ((aux->native && msg->size > aux_cmd_native_max) ||
>>> +           msg->size > aux_cmd_i2c_max) {
>>> +               DRM_ERROR("%s: invalid msg: size(%zu), request(%x)\n",
>>> +                       __func__, msg->size, msg->request);
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       mutex_lock(&aux->mutex);
>>> +       if (!aux->initted) {
>>> +               ret = -EIO;
>>> +               goto exit;
>>> +       }
>>> +
>>> +       /*
>>> +        * For eDP it's important to give a reasonably long wait here for HPD
>>> +        * to be asserted. This is because the panel driver may have _just_
>>> +        * turned on the panel and then tried to do an AUX transfer. The panel
>>> +        * driver has no way of knowing when the panel is ready, so it's up
>>> +        * to us to wait. For DP we never get into this situation so let's
>>> +        * avoid ever doing the extra long wait for DP.
>>> +        */
>>> +       if (aux->is_edp) {
>>> +               ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
>>> +               if (ret) {
>>> +                       DRM_DEBUG_DP("Panel not ready for aux transactions\n");
>>> +                       goto exit;
>>> +               }
>>> +       }
>>> +
>>> +       dp_aux_update_offset_and_segment(aux, msg);
>>> +       dp_aux_transfer_helper(aux, msg, true, true);
>>> +
>>> +       aux->read = msg->request & (DP_AUX_I2C_READ &
>> DP_AUX_NATIVE_READ);
>>> +       aux->cmd_busy = true;
>>> +
>>> +       if (aux->read) {
>>> +               aux->no_send_addr = true;
>>> +               aux->no_send_stop = false;
>>> +       } else {
>>> +               aux->no_send_addr = true;
>>> +               aux->no_send_stop = true;
>>> +       }
>>> +
>>> +       ret = dp_aux_cmd_fifo_tx(aux, msg, true);
>>> +
>>> +       /* TODO: why is fifo_rx necessary here?
>>> +        * Ideally fifo_rx need not be executed for an aux write
>>> +        */
>>> +       ret = dp_aux_cmd_fifo_rx(aux, msg);
>>> +
>>> +       if (ret < 0) {
>>> +               if (aux->native) {
>>> +                       aux->retry_cnt++;
>>> +                       if (!(aux->retry_cnt % MAX_AUX_RETRIES))
>>> +                               dp_catalog_aux_update_cfg(aux->catalog);
>>> +               }
>>> +               /* reset aux if link is in connected state */
>>> +               if (dp_catalog_link_is_connected(aux->catalog))
>>> +                       dp_catalog_aux_reset(aux->catalog);
>>> +       } else {
>>> +               aux->retry_cnt = 0;
>>> +               switch (aux->aux_error_num) {
>>> +               case DP_AUX_ERR_NONE:
>>> +                       if (aux->read)
>>> +                               ret = dp_aux_cmd_fifo_rx(aux, msg);
>>> +                       msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_ACK :
>> DP_AUX_I2C_REPLY_ACK;
>>> +                       break;
>>> +               case DP_AUX_ERR_DEFER:
>>> +                       msg->reply = aux->native ?
>>> +                               DP_AUX_NATIVE_REPLY_DEFER :
>> DP_AUX_I2C_REPLY_DEFER;
>>> +                       break;
>>> +               case DP_AUX_ERR_PHY:
>>> +               case DP_AUX_ERR_ADDR:
>>> +               case DP_AUX_ERR_NACK:
>>> +               case DP_AUX_ERR_NACK_DEFER:
>>> +                       msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_NACK :
>> DP_AUX_I2C_REPLY_NACK;
>>> +                       break;
>>> +               case DP_AUX_ERR_TOUT:
>>> +                       ret = -ETIMEDOUT;
>>> +                       break;
>>> +               }
>>> +       }
>>> +
>>> +       aux->cmd_busy = false;
>>> +
>>> +exit:
>>> +       mutex_unlock(&aux->mutex);
>>> +
>>> +       return ret;
>>> +}
>>> +
>>>   /*
>>>    * This function does the real job to process an AUX transaction.
>>>    * It will call aux_reset() function to reset the AUX channel, @@
>>> -355,7 +477,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
>> *dp_aux,
>>>          }
>>>
>>>          dp_aux_update_offset_and_segment(aux, msg);
>>> -       dp_aux_transfer_helper(aux, msg, true);
>>> +       dp_aux_transfer_helper(aux, msg, true, false);
>>>
>>>          aux->read = msg->request & (DP_AUX_I2C_READ &
>> DP_AUX_NATIVE_READ);
>>>          aux->cmd_busy = true;
>>> @@ -368,7 +490,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
>> *dp_aux,
>>>                  aux->no_send_stop = true;
>>>          }
>>>
>>> -       ret = dp_aux_cmd_fifo_tx(aux, msg);
>>> +       ret = dp_aux_cmd_fifo_tx(aux, msg, false);
>>>          if (ret < 0) {
>>>                  if (aux->native) {
>>>                          aux->retry_cnt++; @@ -535,6 +657,15 @@ struct
>>> drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
>>>          aux->catalog = catalog;
>>>          aux->retry_cnt = 0;
>>>
>>> +       /*
>>> +        * Use the drm_dp_aux_init() to use the aux adapter
>>> +        * before registering aux with the DRM device.
>>> +        */
>>> +       aux->dp_aux.name = "dpu_dp_aux";
>>> +       aux->dp_aux.dev = dev;
>>> +       aux->dp_aux.transfer = dp_aux_transfer_init;
>>> +       drm_dp_aux_init(&aux->dp_aux);
>>
>> How do you use the aux adapter? It should not be used before
>> aux->drm_dev is setup.
>>
> 
> The drm_dev registration happens during the bind. But we need to use aux during probe.
> 
> The kernel doc for the drm_dp_aux_init function suggested we can use the adapter before
> drm_dev registration. So, I used this function to enable the aux usage during probe.

Then please also change __drm_printk() to use (drm) ? (drm->dev) : NULL 
as a first argument to dev_##level##type. Otherwise the first AUX 
transfer error before aux->drm_dev is set will oops the kernel. See how 
drm_err() is expanded.

> 
> /**
>   * drm_dp_aux_init() - minimally initialise an aux channel
>   * @aux: DisplayPort AUX channel
>   *
>   * If you need to use the drm_dp_aux's i2c adapter prior to registering it with
>   * the outside world, call drm_dp_aux_init() first.
> 
>>> +
>>>          return &aux->dp_aux;
>>>   }
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
>>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>> index 676279d0ca8d..ccf0400176f0 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>> @@ -258,6 +258,18 @@ int
>> dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog
>> *dp_catalog)
>>>                                  2000, 500000);  }
>>>
>>> +int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog
>>> +*dp_catalog) {
>>> +       u32 aux;
>>> +       struct dp_catalog_private *catalog = container_of(dp_catalog,
>>> +                               struct dp_catalog_private,
>>> +dp_catalog);
>>> +
>>> +       return readl_poll_timeout(catalog->io->dp_controller.ahb.base +
>>> +                               REG_DP_INTR_STATUS,
>>> +                               aux, aux & DP_INTERRUPT_STATUS1,
>>> +                               250, 250000); }
>>> +
>>>   static void dump_regs(void __iomem *base, int len)  {
>>>          int i;
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h
>>> b/drivers/gpu/drm/msm/dp/dp_catalog.h
>>> index 1f717f45c115..ad4a9e0f71f2 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
>>> @@ -87,6 +87,7 @@ void dp_catalog_aux_enable(struct dp_catalog
>>> *dp_catalog, bool enable);  void dp_catalog_aux_update_cfg(struct
>>> dp_catalog *dp_catalog);  int
>>> dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog
>>> *dp_catalog);
>>>   u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
>>> +int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog
>>> +*dp_catalog);
>>>
>>>   /* DP Controller APIs */
>>>   void dp_catalog_ctrl_state_ctrl(struct dp_catalog *dp_catalog, u32
>>> state); diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>> index bde1a7ce442f..a5dcef040b74 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>> @@ -275,13 +275,6 @@ static int dp_display_bind(struct device *dev, struct
>> device *master,
>>>          dp->dp_display.drm_dev = drm;
>>>          priv->dp[dp->id] = &dp->dp_display;
>>>
>>> -       rc = dp->parser->parse(dp->parser);
>>> -       if (rc) {
>>> -               DRM_ERROR("device tree parsing failed\n");
>>> -               goto end;
>>> -       }
>>> -
>>> -
>>>          dp->drm_dev = drm;
>>>          dp->aux->drm_dev = drm;
>>>          rc = dp_aux_register(dp->aux); @@ -290,12 +283,6 @@ static int
>>> dp_display_bind(struct device *dev, struct device *master,
>>>                  goto end;
>>>          }
>>>
>>> -       rc = dp_power_client_init(dp->power);
>>> -       if (rc) {
>>> -               DRM_ERROR("Power client create failed\n");
>>> -               goto end;
>>> -       }
>>> -
>>>          rc = dp_register_audio_driver(dev, dp->audio);
>>>          if (rc) {
>>>                  DRM_ERROR("Audio registration Dp failed\n"); @@ -787,6
>>> +774,12 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
>>>                  goto error;
>>>          }
>>>
>>> +       rc = dp->parser->parse(dp->parser);
>>> +       if (rc) {
>>> +               DRM_ERROR("device tree parsing failed\n");
>>> +               goto error;
>>> +       }
>>> +
>>>          dp->catalog = dp_catalog_get(dev, &dp->parser->io);
>>>          if (IS_ERR(dp->catalog)) {
>>>                  rc = PTR_ERR(dp->catalog); @@ -803,6 +796,12 @@ static
>>> int dp_init_sub_modules(struct dp_display_private *dp)
>>>                  goto error;
>>>          }
>>>
>>> +       rc = dp_power_client_init(dp->power);
>>> +       if (rc) {
>>> +               DRM_ERROR("Power client create failed\n");
>>> +               goto error;
>>> +       }
>>> +
>>>          dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
>>>          if (IS_ERR(dp->aux)) {
>>>                  rc = PTR_ERR(dp->aux); @@ -1338,12 +1337,29 @@ static
>>> int dp_display_probe(struct platform_device *pdev)
>>>
>>>          platform_set_drvdata(pdev, &dp->dp_display);
>>>
>>> +       if (dp->dp_display.is_edp) {
>>> +               dp_display_host_init(dp);
>>> +               dp_display_host_phy_init(dp);
>>> +               dp_catalog_ctrl_hpd_config(dp->catalog);
>>> +
>>> +               rc = devm_of_dp_aux_populate_bus(dp->aux, NULL);
>>
>> You should pass a real done_probing handler here, if you are going to refactor
>> this piece of code. The done_probing should then shut down the resources
>> and bind the component.
>>
> 
> I removed the resource enabling part from probe in the next patch where I implemented pm_runtime_ops.
> I moved the host_init, phy_init and hpd_config into runtime_resume and calling pm_runtime_get_sync from aux_transfer.

Next patch doesn't exist at this point. So, please, either reorder them, 
or make this patch correct per se.

Also, the key part is a call to component_add(). It should come from 
done_probing callback. AUX bus probing is asynchronous. The kernel 
registers a device and then it might take some time for the correct 
driver to be loaded, etc. We clearly expect dp_display_bind to be used 
only when the panel has been probed.

> 
>>> +
>>> +               dp_display_host_phy_exit(dp);
>>> +               dp_display_host_deinit(dp);
>>> +
>>> +               if (rc) {
>>> +                       DRM_ERROR("failed to initialize panel, rc = %d\n", rc);
>>> +                       goto error;
>>> +               }
>>> +       }
>>> +
>>>          rc = component_add(&pdev->dev, &dp_display_comp_ops);
>>>          if (rc) {
>>>                  DRM_ERROR("component add failed, rc=%d\n", rc);
>>>                  dp_display_deinit_sub_modules(dp);
>>>          }
>>>
>>> +error:
>>>          return rc;
>>>   }
>>>
>>> @@ -1546,40 +1562,8 @@ static int dp_display_get_next_bridge(struct
>>> msm_dp *dp)  {
>>>          int rc;
>>>          struct dp_display_private *dp_priv;
>>> -       struct device_node *aux_bus;
>>> -       struct device *dev;
>>>
>>>          dp_priv = container_of(dp, struct dp_display_private, dp_display);
>>> -       dev = &dp_priv->pdev->dev;
>>> -       aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
>>> -
>>> -       if (aux_bus && dp->is_edp) {
>>> -               dp_display_host_init(dp_priv);
>>> -               dp_catalog_ctrl_hpd_config(dp_priv->catalog);
>>> -               dp_display_host_phy_init(dp_priv);
>>> -               enable_irq(dp_priv->irq);
>>> -
>>> -               /*
>>> -                * The code below assumes that the panel will finish probing
>>> -                * by the time devm_of_dp_aux_populate_ep_devices() returns.
>>> -                * This isn't a great assumption since it will fail if the
>>> -                * panel driver is probed asynchronously but is the best we
>>> -                * can do without a bigger driver reorganization.
>>> -                */
>>> -               rc = of_dp_aux_populate_bus(dp_priv->aux, NULL);
>>> -               of_node_put(aux_bus);
>>> -               if (rc)
>>> -                       goto error;
>>> -
>>> -               rc = devm_add_action_or_reset(dp->drm_dev->dev,
>>> -                                               of_dp_aux_depopulate_bus_void,
>>> -                                               dp_priv->aux);
>>> -               if (rc)
>>> -                       goto error;
>>> -       } else if (dp->is_edp) {
>>> -               DRM_ERROR("eDP aux_bus not found\n");
>>> -               return -ENODEV;
>>> -       }
>>>
>>>          /*
>>>           * External bridges are mandatory for eDP interfaces: one has
>>> to @@ -1597,12 +1581,6 @@ static int dp_display_get_next_bridge(struct
>> msm_dp *dp)
>>>                  return 0;
>>>          }
>>>
>>> -error:
>>> -       if (dp->is_edp) {
>>> -               disable_irq(dp_priv->irq);
>>> -               dp_display_host_phy_exit(dp_priv);
>>> -               dp_display_host_deinit(dp_priv);
>>> -       }
>>>          return rc;
>>>   }
>>>
>>> --
>>> 2.39.0
>>>
>>
>>
>> --
>> With best wishes
>> Dmitry

-- 
With best wishes
Dmitry

Re: [RFC PATCH 1/2] drm/msm/dp: enumerate edp panel during driver probe

[Dmitry Baryshkov]

On 01/03/2023 10:13, Sankeerth Billakanti (QUIC) wrote:
>>>
>>> The eDP panel is identified and enumerated during probe of the
>>> panel-edp driver. The current DP driver triggers this panel-edp driver
>>> probe while getting the panel-bridge associated with the eDP panel
>>> from the platform driver bind. If the panel-edp probe is deferred,
>>> then the whole bunch of MDSS parent and child drivers have to defer and
>> roll back.
>>
>> No, MDSS has not been rolled back since 5.19. Please shift your development
>> on top of the current msm-next.
>>
> 
> Okay, I will move to the msm-next tip.
> 
>>>
>>> So, we want to move the panel enumeration from bind to probe so that
>>> the probe defer is easier to handle and also both the drivers appear
>>> consistent in panel enumeration. This change moves the DP driver
>>> panel-bridge enumeration to probe.
>>>
>>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/dp/dp_aux.c     | 149
>> ++++++++++++++++++++++++++--
>>>   drivers/gpu/drm/msm/dp/dp_catalog.c |  12 +++
>>>   drivers/gpu/drm/msm/dp/dp_catalog.h |   1 +
>>>   drivers/gpu/drm/msm/dp/dp_display.c |  80 ++++++---------
>>>   4 files changed, 182 insertions(+), 60 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
>>> b/drivers/gpu/drm/msm/dp/dp_aux.c index cc3efed593aa..5da95dfdeede
>>> 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>>> @@ -110,7 +110,7 @@ static ssize_t dp_aux_write(struct dp_aux_private
>>> *aux,  }
>>>
>>>   static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
>>> -                             struct drm_dp_aux_msg *msg)
>>> +                             struct drm_dp_aux_msg *msg, bool poll)
>>
>> What is the reason for working in polled mode rather than always using the
>> interrupts?
>>
> 
> The mdss interrupts will be enabled and can be used after msm_irq_install from msm_drm_bind.
> We want to perform aux transactions during probe. The aux data transmission is followed by an
> interrupt to indicate successful transmission status and reply information.

This is the price of developing on, let me guess, 5.15. Nowadays MDSS 
interrupts are enabled and can be used during dp_display_probe() and 
dp_display_bind(). If they can not for some reason, this is an issue 
that must be fixed. Please reconsider your approach after rebasing onto 
msm-next or 6.3-rc1.

> 
> As interrupts are not enabled, I took this polling approach for aux interrupts during probe.
> 
>>>   {
>>>          ssize_t ret;
>>>          unsigned long time_left;
>>> @@ -121,10 +121,16 @@ static ssize_t dp_aux_cmd_fifo_tx(struct
>> dp_aux_private *aux,
>>>          if (ret < 0)
>>>                  return ret;
>>>
>>> -       time_left = wait_for_completion_timeout(&aux->comp,
>>> +       if (!poll) {
>>> +               time_left = wait_for_completion_timeout(&aux->comp,
>>>                                                  msecs_to_jiffies(250));
>>> -       if (!time_left)
>>> -               return -ETIMEDOUT;
>>> +               if (!time_left)
>>> +                       return -ETIMEDOUT;
>>> +       } else {
>>> +               ret = dp_catalog_aux_poll_and_get_hw_interrupt(aux->catalog);
>>> +               if (!ret)
>>> +                       dp_aux_isr(&aux->dp_aux);
>>> +       }
>>>
>>>          return ret;
>>>   }
>>> @@ -238,7 +244,7 @@ static void
>> dp_aux_update_offset_and_segment(struct dp_aux_private *aux,
>>>    */
>>>   static void dp_aux_transfer_helper(struct dp_aux_private *aux,
>>>                                     struct drm_dp_aux_msg *input_msg,
>>> -                                  bool send_seg)
>>> +                                  bool send_seg, bool poll)
>>>   {
>>>          struct drm_dp_aux_msg helper_msg;
>>>          u32 message_size = 0x10;
>>> @@ -278,7 +284,7 @@ static void dp_aux_transfer_helper(struct
>> dp_aux_private *aux,
>>>                  helper_msg.address = segment_address;
>>>                  helper_msg.buffer = &aux->segment;
>>>                  helper_msg.size = 1;
>>> -               dp_aux_cmd_fifo_tx(aux, &helper_msg);
>>> +               dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
>>>          }
>>>
>>>          /*
>>> @@ -292,7 +298,7 @@ static void dp_aux_transfer_helper(struct
>> dp_aux_private *aux,
>>>          helper_msg.address = input_msg->address;
>>>          helper_msg.buffer = &aux->offset;
>>>          helper_msg.size = 1;
>>> -       dp_aux_cmd_fifo_tx(aux, &helper_msg);
>>> +       dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
>>>
>>>   end:
>>>          aux->offset += message_size;
>>> @@ -300,6 +306,122 @@ static void dp_aux_transfer_helper(struct
>> dp_aux_private *aux,
>>>                  aux->segment = 0x0; /* reset segment at end of block
>>> */  }
>>>
>>> +/*
>>> + * This function does the real job to process an AUX transaction.
>>> + * It will call aux_reset() function to reset the AUX channel,
>>> + * if the waiting is timeout.
>>> + */
>>> +static ssize_t dp_aux_transfer_init(struct drm_dp_aux *dp_aux,
>>> +                              struct drm_dp_aux_msg *msg) {
>>> +       ssize_t ret;
>>> +       int const aux_cmd_native_max = 16;
>>> +       int const aux_cmd_i2c_max = 128;
>>> +       struct dp_aux_private *aux;
>>> +
>>> +       aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
>>> +
>>> +       aux->native = msg->request & (DP_AUX_NATIVE_WRITE &
>>> + DP_AUX_NATIVE_READ);
>>> +
>>> +       /* Ignore address only message */
>>> +       if (msg->size == 0 || !msg->buffer) {
>>> +               msg->reply = aux->native ?
>>> +                       DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
>>> +               return msg->size;
>>> +       }
>>> +
>>> +       /* msg sanity check */
>>> +       if ((aux->native && msg->size > aux_cmd_native_max) ||
>>> +           msg->size > aux_cmd_i2c_max) {
>>> +               DRM_ERROR("%s: invalid msg: size(%zu), request(%x)\n",
>>> +                       __func__, msg->size, msg->request);
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       mutex_lock(&aux->mutex);
>>> +       if (!aux->initted) {
>>> +               ret = -EIO;
>>> +               goto exit;
>>> +       }
>>> +
>>> +       /*
>>> +        * For eDP it's important to give a reasonably long wait here for HPD
>>> +        * to be asserted. This is because the panel driver may have _just_
>>> +        * turned on the panel and then tried to do an AUX transfer. The panel
>>> +        * driver has no way of knowing when the panel is ready, so it's up
>>> +        * to us to wait. For DP we never get into this situation so let's
>>> +        * avoid ever doing the extra long wait for DP.
>>> +        */
>>> +       if (aux->is_edp) {
>>> +               ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
>>> +               if (ret) {
>>> +                       DRM_DEBUG_DP("Panel not ready for aux transactions\n");
>>> +                       goto exit;
>>> +               }
>>> +       }
>>> +
>>> +       dp_aux_update_offset_and_segment(aux, msg);
>>> +       dp_aux_transfer_helper(aux, msg, true, true);
>>> +
>>> +       aux->read = msg->request & (DP_AUX_I2C_READ &
>> DP_AUX_NATIVE_READ);
>>> +       aux->cmd_busy = true;
>>> +
>>> +       if (aux->read) {
>>> +               aux->no_send_addr = true;
>>> +               aux->no_send_stop = false;
>>> +       } else {
>>> +               aux->no_send_addr = true;
>>> +               aux->no_send_stop = true;
>>> +       }
>>> +
>>> +       ret = dp_aux_cmd_fifo_tx(aux, msg, true);
>>> +
>>> +       /* TODO: why is fifo_rx necessary here?
>>> +        * Ideally fifo_rx need not be executed for an aux write
>>> +        */
>>> +       ret = dp_aux_cmd_fifo_rx(aux, msg);
>>> +
>>> +       if (ret < 0) {
>>> +               if (aux->native) {
>>> +                       aux->retry_cnt++;
>>> +                       if (!(aux->retry_cnt % MAX_AUX_RETRIES))
>>> +                               dp_catalog_aux_update_cfg(aux->catalog);
>>> +               }
>>> +               /* reset aux if link is in connected state */
>>> +               if (dp_catalog_link_is_connected(aux->catalog))
>>> +                       dp_catalog_aux_reset(aux->catalog);
>>> +       } else {
>>> +               aux->retry_cnt = 0;
>>> +               switch (aux->aux_error_num) {
>>> +               case DP_AUX_ERR_NONE:
>>> +                       if (aux->read)
>>> +                               ret = dp_aux_cmd_fifo_rx(aux, msg);
>>> +                       msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_ACK :
>> DP_AUX_I2C_REPLY_ACK;
>>> +                       break;
>>> +               case DP_AUX_ERR_DEFER:
>>> +                       msg->reply = aux->native ?
>>> +                               DP_AUX_NATIVE_REPLY_DEFER :
>> DP_AUX_I2C_REPLY_DEFER;
>>> +                       break;
>>> +               case DP_AUX_ERR_PHY:
>>> +               case DP_AUX_ERR_ADDR:
>>> +               case DP_AUX_ERR_NACK:
>>> +               case DP_AUX_ERR_NACK_DEFER:
>>> +                       msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_NACK :
>> DP_AUX_I2C_REPLY_NACK;
>>> +                       break;
>>> +               case DP_AUX_ERR_TOUT:
>>> +                       ret = -ETIMEDOUT;
>>> +                       break;
>>> +               }
>>> +       }
>>> +
>>> +       aux->cmd_busy = false;
>>> +
>>> +exit:
>>> +       mutex_unlock(&aux->mutex);
>>> +
>>> +       return ret;
>>> +}
>>> +
>>>   /*
>>>    * This function does the real job to process an AUX transaction.
>>>    * It will call aux_reset() function to reset the AUX channel, @@
>>> -355,7 +477,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
>> *dp_aux,
>>>          }
>>>
>>>          dp_aux_update_offset_and_segment(aux, msg);
>>> -       dp_aux_transfer_helper(aux, msg, true);
>>> +       dp_aux_transfer_helper(aux, msg, true, false);
>>>
>>>          aux->read = msg->request & (DP_AUX_I2C_READ &
>> DP_AUX_NATIVE_READ);
>>>          aux->cmd_busy = true;
>>> @@ -368,7 +490,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
>> *dp_aux,
>>>                  aux->no_send_stop = true;
>>>          }
>>>
>>> -       ret = dp_aux_cmd_fifo_tx(aux, msg);
>>> +       ret = dp_aux_cmd_fifo_tx(aux, msg, false);
>>>          if (ret < 0) {
>>>                  if (aux->native) {
>>>                          aux->retry_cnt++; @@ -535,6 +657,15 @@ struct
>>> drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
>>>          aux->catalog = catalog;
>>>          aux->retry_cnt = 0;
>>>
>>> +       /*
>>> +        * Use the drm_dp_aux_init() to use the aux adapter
>>> +        * before registering aux with the DRM device.
>>> +        */
>>> +       aux->dp_aux.name = "dpu_dp_aux";
>>> +       aux->dp_aux.dev = dev;
>>> +       aux->dp_aux.transfer = dp_aux_transfer_init;
>>> +       drm_dp_aux_init(&aux->dp_aux);
>>
>> How do you use the aux adapter? It should not be used before
>> aux->drm_dev is setup.
>>
> 
> The drm_dev registration happens during the bind. But we need to use aux during probe.
> 
> The kernel doc for the drm_dp_aux_init function suggested we can use the adapter before
> drm_dev registration. So, I used this function to enable the aux usage during probe.

Then please also change __drm_printk() to use (drm) ? (drm->dev) : NULL 
as a first argument to dev_##level##type. Otherwise the first AUX 
transfer error before aux->drm_dev is set will oops the kernel. See how 
drm_err() is expanded.

> 
> /**
>   * drm_dp_aux_init() - minimally initialise an aux channel
>   * @aux: DisplayPort AUX channel
>   *
>   * If you need to use the drm_dp_aux's i2c adapter prior to registering it with
>   * the outside world, call drm_dp_aux_init() first.
> 
>>> +
>>>          return &aux->dp_aux;
>>>   }
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
>>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>> index 676279d0ca8d..ccf0400176f0 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>> @@ -258,6 +258,18 @@ int
>> dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog
>> *dp_catalog)
>>>                                  2000, 500000);  }
>>>
>>> +int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog
>>> +*dp_catalog) {
>>> +       u32 aux;
>>> +       struct dp_catalog_private *catalog = container_of(dp_catalog,
>>> +                               struct dp_catalog_private,
>>> +dp_catalog);
>>> +
>>> +       return readl_poll_timeout(catalog->io->dp_controller.ahb.base +
>>> +                               REG_DP_INTR_STATUS,
>>> +                               aux, aux & DP_INTERRUPT_STATUS1,
>>> +                               250, 250000); }
>>> +
>>>   static void dump_regs(void __iomem *base, int len)  {
>>>          int i;
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h
>>> b/drivers/gpu/drm/msm/dp/dp_catalog.h
>>> index 1f717f45c115..ad4a9e0f71f2 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
>>> @@ -87,6 +87,7 @@ void dp_catalog_aux_enable(struct dp_catalog
>>> *dp_catalog, bool enable);  void dp_catalog_aux_update_cfg(struct
>>> dp_catalog *dp_catalog);  int
>>> dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog
>>> *dp_catalog);
>>>   u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
>>> +int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog
>>> +*dp_catalog);
>>>
>>>   /* DP Controller APIs */
>>>   void dp_catalog_ctrl_state_ctrl(struct dp_catalog *dp_catalog, u32
>>> state); diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>> index bde1a7ce442f..a5dcef040b74 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>> @@ -275,13 +275,6 @@ static int dp_display_bind(struct device *dev, struct
>> device *master,
>>>          dp->dp_display.drm_dev = drm;
>>>          priv->dp[dp->id] = &dp->dp_display;
>>>
>>> -       rc = dp->parser->parse(dp->parser);
>>> -       if (rc) {
>>> -               DRM_ERROR("device tree parsing failed\n");
>>> -               goto end;
>>> -       }
>>> -
>>> -
>>>          dp->drm_dev = drm;
>>>          dp->aux->drm_dev = drm;
>>>          rc = dp_aux_register(dp->aux); @@ -290,12 +283,6 @@ static int
>>> dp_display_bind(struct device *dev, struct device *master,
>>>                  goto end;
>>>          }
>>>
>>> -       rc = dp_power_client_init(dp->power);
>>> -       if (rc) {
>>> -               DRM_ERROR("Power client create failed\n");
>>> -               goto end;
>>> -       }
>>> -
>>>          rc = dp_register_audio_driver(dev, dp->audio);
>>>          if (rc) {
>>>                  DRM_ERROR("Audio registration Dp failed\n"); @@ -787,6
>>> +774,12 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
>>>                  goto error;
>>>          }
>>>
>>> +       rc = dp->parser->parse(dp->parser);
>>> +       if (rc) {
>>> +               DRM_ERROR("device tree parsing failed\n");
>>> +               goto error;
>>> +       }
>>> +
>>>          dp->catalog = dp_catalog_get(dev, &dp->parser->io);
>>>          if (IS_ERR(dp->catalog)) {
>>>                  rc = PTR_ERR(dp->catalog); @@ -803,6 +796,12 @@ static
>>> int dp_init_sub_modules(struct dp_display_private *dp)
>>>                  goto error;
>>>          }
>>>
>>> +       rc = dp_power_client_init(dp->power);
>>> +       if (rc) {
>>> +               DRM_ERROR("Power client create failed\n");
>>> +               goto error;
>>> +       }
>>> +
>>>          dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
>>>          if (IS_ERR(dp->aux)) {
>>>                  rc = PTR_ERR(dp->aux); @@ -1338,12 +1337,29 @@ static
>>> int dp_display_probe(struct platform_device *pdev)
>>>
>>>          platform_set_drvdata(pdev, &dp->dp_display);
>>>
>>> +       if (dp->dp_display.is_edp) {
>>> +               dp_display_host_init(dp);
>>> +               dp_display_host_phy_init(dp);
>>> +               dp_catalog_ctrl_hpd_config(dp->catalog);
>>> +
>>> +               rc = devm_of_dp_aux_populate_bus(dp->aux, NULL);
>>
>> You should pass a real done_probing handler here, if you are going to refactor
>> this piece of code. The done_probing should then shut down the resources
>> and bind the component.
>>
> 
> I removed the resource enabling part from probe in the next patch where I implemented pm_runtime_ops.
> I moved the host_init, phy_init and hpd_config into runtime_resume and calling pm_runtime_get_sync from aux_transfer.

Next patch doesn't exist at this point. So, please, either reorder them, 
or make this patch correct per se.

Also, the key part is a call to component_add(). It should come from 
done_probing callback. AUX bus probing is asynchronous. The kernel 
registers a device and then it might take some time for the correct 
driver to be loaded, etc. We clearly expect dp_display_bind to be used 
only when the panel has been probed.

> 
>>> +
>>> +               dp_display_host_phy_exit(dp);
>>> +               dp_display_host_deinit(dp);
>>> +
>>> +               if (rc) {
>>> +                       DRM_ERROR("failed to initialize panel, rc = %d\n", rc);
>>> +                       goto error;
>>> +               }
>>> +       }
>>> +
>>>          rc = component_add(&pdev->dev, &dp_display_comp_ops);
>>>          if (rc) {
>>>                  DRM_ERROR("component add failed, rc=%d\n", rc);
>>>                  dp_display_deinit_sub_modules(dp);
>>>          }
>>>
>>> +error:
>>>          return rc;
>>>   }
>>>
>>> @@ -1546,40 +1562,8 @@ static int dp_display_get_next_bridge(struct
>>> msm_dp *dp)  {
>>>          int rc;
>>>          struct dp_display_private *dp_priv;
>>> -       struct device_node *aux_bus;
>>> -       struct device *dev;
>>>
>>>          dp_priv = container_of(dp, struct dp_display_private, dp_display);
>>> -       dev = &dp_priv->pdev->dev;
>>> -       aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
>>> -
>>> -       if (aux_bus && dp->is_edp) {
>>> -               dp_display_host_init(dp_priv);
>>> -               dp_catalog_ctrl_hpd_config(dp_priv->catalog);
>>> -               dp_display_host_phy_init(dp_priv);
>>> -               enable_irq(dp_priv->irq);
>>> -
>>> -               /*
>>> -                * The code below assumes that the panel will finish probing
>>> -                * by the time devm_of_dp_aux_populate_ep_devices() returns.
>>> -                * This isn't a great assumption since it will fail if the
>>> -                * panel driver is probed asynchronously but is the best we
>>> -                * can do without a bigger driver reorganization.
>>> -                */
>>> -               rc = of_dp_aux_populate_bus(dp_priv->aux, NULL);
>>> -               of_node_put(aux_bus);
>>> -               if (rc)
>>> -                       goto error;
>>> -
>>> -               rc = devm_add_action_or_reset(dp->drm_dev->dev,
>>> -                                               of_dp_aux_depopulate_bus_void,
>>> -                                               dp_priv->aux);
>>> -               if (rc)
>>> -                       goto error;
>>> -       } else if (dp->is_edp) {
>>> -               DRM_ERROR("eDP aux_bus not found\n");
>>> -               return -ENODEV;
>>> -       }
>>>
>>>          /*
>>>           * External bridges are mandatory for eDP interfaces: one has
>>> to @@ -1597,12 +1581,6 @@ static int dp_display_get_next_bridge(struct
>> msm_dp *dp)
>>>                  return 0;
>>>          }
>>>
>>> -error:
>>> -       if (dp->is_edp) {
>>> -               disable_irq(dp_priv->irq);
>>> -               dp_display_host_phy_exit(dp_priv);
>>> -               dp_display_host_deinit(dp_priv);
>>> -       }
>>>          return rc;
>>>   }
>>>
>>> --
>>> 2.39.0
>>>
>>
>>
>> --
>> With best wishes
>> Dmitry

-- 
With best wishes
Dmitry

RE: [RFC PATCH 1/2] drm/msm/dp: enumerate edp panel during driver probe

[Sankeerth Billakanti (QUIC)]

Hi Dmitry,

>>>>
>>>> The eDP panel is identified and enumerated during probe of the
>>>> panel-edp driver. The current DP driver triggers this panel-edp
>>>> driver probe while getting the panel-bridge associated with the eDP
>>>> panel from the platform driver bind. If the panel-edp probe is
>>>> deferred, then the whole bunch of MDSS parent and child drivers have
>>>> to defer and
>>> roll back.
>>>
>>> No, MDSS has not been rolled back since 5.19. Please shift your
>>> development on top of the current msm-next.
>>>
>>
>> Okay, I will move to the msm-next tip.
>>
>>>>
>>>> So, we want to move the panel enumeration from bind to probe so that
>>>> the probe defer is easier to handle and also both the drivers appear
>>>> consistent in panel enumeration. This change moves the DP driver
>>>> panel-bridge enumeration to probe.
>>>>
>>>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
>>>> ---
>>>>   drivers/gpu/drm/msm/dp/dp_aux.c     | 149
>>> ++++++++++++++++++++++++++--
>>>>   drivers/gpu/drm/msm/dp/dp_catalog.c |  12 +++
>>>>   drivers/gpu/drm/msm/dp/dp_catalog.h |   1 +
>>>>   drivers/gpu/drm/msm/dp/dp_display.c |  80 ++++++---------
>>>>   4 files changed, 182 insertions(+), 60 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
>>>> b/drivers/gpu/drm/msm/dp/dp_aux.c index
>cc3efed593aa..5da95dfdeede
>>>> 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>>>> @@ -110,7 +110,7 @@ static ssize_t dp_aux_write(struct
>>>> dp_aux_private *aux,  }
>>>>
>>>>   static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
>>>> -                             struct drm_dp_aux_msg *msg)
>>>> +                             struct drm_dp_aux_msg *msg, bool poll)
>>>
>>> What is the reason for working in polled mode rather than always
>>> using the interrupts?
>>>
>>
>> The mdss interrupts will be enabled and can be used after msm_irq_install
>from msm_drm_bind.
>> We want to perform aux transactions during probe. The aux data
>> transmission is followed by an interrupt to indicate successful transmission
>status and reply information.
>
>This is the price of developing on, let me guess, 5.15. Nowadays MDSS
>interrupts are enabled and can be used during dp_display_probe() and
>dp_display_bind(). If they can not for some reason, this is an issue that must
>be fixed. Please reconsider your approach after rebasing onto msm-next or
>6.3-rc1.
>

On the msm-next also, I see the mdss irq is enabled from bind (in msm_drv.c). 
msm_drm_bind -> msm_drm_init -> msm_irq_install

Currently, interrupts will not be available during the dp_display_probe.
Is there a patch series which is enabling IRQs during mdss probe?

>>
>> As interrupts are not enabled, I took this polling approach for aux interrupts
>during probe.
>>
>>>>   {
>>>>          ssize_t ret;
>>>>          unsigned long time_left;
>>>> @@ -121,10 +121,16 @@ static ssize_t dp_aux_cmd_fifo_tx(struct
>>> dp_aux_private *aux,
>>>>          if (ret < 0)
>>>>                  return ret;
>>>>
>>>> -       time_left = wait_for_completion_timeout(&aux->comp,
>>>> +       if (!poll) {
>>>> +               time_left = wait_for_completion_timeout(&aux->comp,
>>>>                                                  msecs_to_jiffies(250));
>>>> -       if (!time_left)
>>>> -               return -ETIMEDOUT;
>>>> +               if (!time_left)
>>>> +                       return -ETIMEDOUT;
>>>> +       } else {
>>>> +               ret = dp_catalog_aux_poll_and_get_hw_interrupt(aux-
>>catalog);
>>>> +               if (!ret)
>>>> +                       dp_aux_isr(&aux->dp_aux);
>>>> +       }
>>>>
>>>>          return ret;
>>>>   }
>>>> @@ -238,7 +244,7 @@ static void
>>> dp_aux_update_offset_and_segment(struct dp_aux_private *aux,
>>>>    */
>>>>   static void dp_aux_transfer_helper(struct dp_aux_private *aux,
>>>>                                     struct drm_dp_aux_msg *input_msg,
>>>> -                                  bool send_seg)
>>>> +                                  bool send_seg, bool poll)
>>>>   {
>>>>          struct drm_dp_aux_msg helper_msg;
>>>>          u32 message_size = 0x10;
>>>> @@ -278,7 +284,7 @@ static void dp_aux_transfer_helper(struct
>>> dp_aux_private *aux,
>>>>                  helper_msg.address = segment_address;
>>>>                  helper_msg.buffer = &aux->segment;
>>>>                  helper_msg.size = 1;
>>>> -               dp_aux_cmd_fifo_tx(aux, &helper_msg);
>>>> +               dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
>>>>          }
>>>>
>>>>          /*
>>>> @@ -292,7 +298,7 @@ static void dp_aux_transfer_helper(struct
>>> dp_aux_private *aux,
>>>>          helper_msg.address = input_msg->address;
>>>>          helper_msg.buffer = &aux->offset;
>>>>          helper_msg.size = 1;
>>>> -       dp_aux_cmd_fifo_tx(aux, &helper_msg);
>>>> +       dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
>>>>
>>>>   end:
>>>>          aux->offset += message_size; @@ -300,6 +306,122 @@ static
>>>> void dp_aux_transfer_helper(struct
>>> dp_aux_private *aux,
>>>>                  aux->segment = 0x0; /* reset segment at end of
>>>> block */  }
>>>>
>>>> +/*
>>>> + * This function does the real job to process an AUX transaction.
>>>> + * It will call aux_reset() function to reset the AUX channel,
>>>> + * if the waiting is timeout.
>>>> + */
>>>> +static ssize_t dp_aux_transfer_init(struct drm_dp_aux *dp_aux,
>>>> +                              struct drm_dp_aux_msg *msg) {
>>>> +       ssize_t ret;
>>>> +       int const aux_cmd_native_max = 16;
>>>> +       int const aux_cmd_i2c_max = 128;
>>>> +       struct dp_aux_private *aux;
>>>> +
>>>> +       aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
>>>> +
>>>> +       aux->native = msg->request & (DP_AUX_NATIVE_WRITE &
>>>> + DP_AUX_NATIVE_READ);
>>>> +
>>>> +       /* Ignore address only message */
>>>> +       if (msg->size == 0 || !msg->buffer) {
>>>> +               msg->reply = aux->native ?
>>>> +                       DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
>>>> +               return msg->size;
>>>> +       }
>>>> +
>>>> +       /* msg sanity check */
>>>> +       if ((aux->native && msg->size > aux_cmd_native_max) ||
>>>> +           msg->size > aux_cmd_i2c_max) {
>>>> +               DRM_ERROR("%s: invalid msg: size(%zu), request(%x)\n",
>>>> +                       __func__, msg->size, msg->request);
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>> +       mutex_lock(&aux->mutex);
>>>> +       if (!aux->initted) {
>>>> +               ret = -EIO;
>>>> +               goto exit;
>>>> +       }
>>>> +
>>>> +       /*
>>>> +        * For eDP it's important to give a reasonably long wait here for HPD
>>>> +        * to be asserted. This is because the panel driver may have _just_
>>>> +        * turned on the panel and then tried to do an AUX transfer. The
>panel
>>>> +        * driver has no way of knowing when the panel is ready, so it's up
>>>> +        * to us to wait. For DP we never get into this situation so let's
>>>> +        * avoid ever doing the extra long wait for DP.
>>>> +        */
>>>> +       if (aux->is_edp) {
>>>> +               ret = dp_catalog_aux_wait_for_hpd_connect_state(aux-
>>catalog);
>>>> +               if (ret) {
>>>> +                       DRM_DEBUG_DP("Panel not ready for aux transactions\n");
>>>> +                       goto exit;
>>>> +               }
>>>> +       }
>>>> +
>>>> +       dp_aux_update_offset_and_segment(aux, msg);
>>>> +       dp_aux_transfer_helper(aux, msg, true, true);
>>>> +
>>>> +       aux->read = msg->request & (DP_AUX_I2C_READ &
>>> DP_AUX_NATIVE_READ);
>>>> +       aux->cmd_busy = true;
>>>> +
>>>> +       if (aux->read) {
>>>> +               aux->no_send_addr = true;
>>>> +               aux->no_send_stop = false;
>>>> +       } else {
>>>> +               aux->no_send_addr = true;
>>>> +               aux->no_send_stop = true;
>>>> +       }
>>>> +
>>>> +       ret = dp_aux_cmd_fifo_tx(aux, msg, true);
>>>> +
>>>> +       /* TODO: why is fifo_rx necessary here?
>>>> +        * Ideally fifo_rx need not be executed for an aux write
>>>> +        */
>>>> +       ret = dp_aux_cmd_fifo_rx(aux, msg);
>>>> +
>>>> +       if (ret < 0) {
>>>> +               if (aux->native) {
>>>> +                       aux->retry_cnt++;
>>>> +                       if (!(aux->retry_cnt % MAX_AUX_RETRIES))
>>>> +                               dp_catalog_aux_update_cfg(aux->catalog);
>>>> +               }
>>>> +               /* reset aux if link is in connected state */
>>>> +               if (dp_catalog_link_is_connected(aux->catalog))
>>>> +                       dp_catalog_aux_reset(aux->catalog);
>>>> +       } else {
>>>> +               aux->retry_cnt = 0;
>>>> +               switch (aux->aux_error_num) {
>>>> +               case DP_AUX_ERR_NONE:
>>>> +                       if (aux->read)
>>>> +                               ret = dp_aux_cmd_fifo_rx(aux, msg);
>>>> +                       msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_ACK :
>>> DP_AUX_I2C_REPLY_ACK;
>>>> +                       break;
>>>> +               case DP_AUX_ERR_DEFER:
>>>> +                       msg->reply = aux->native ?
>>>> +                               DP_AUX_NATIVE_REPLY_DEFER :
>>> DP_AUX_I2C_REPLY_DEFER;
>>>> +                       break;
>>>> +               case DP_AUX_ERR_PHY:
>>>> +               case DP_AUX_ERR_ADDR:
>>>> +               case DP_AUX_ERR_NACK:
>>>> +               case DP_AUX_ERR_NACK_DEFER:
>>>> +                       msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_NACK :
>>> DP_AUX_I2C_REPLY_NACK;
>>>> +                       break;
>>>> +               case DP_AUX_ERR_TOUT:
>>>> +                       ret = -ETIMEDOUT;
>>>> +                       break;
>>>> +               }
>>>> +       }
>>>> +
>>>> +       aux->cmd_busy = false;
>>>> +
>>>> +exit:
>>>> +       mutex_unlock(&aux->mutex);
>>>> +
>>>> +       return ret;
>>>> +}
>>>> +
>>>>   /*
>>>>    * This function does the real job to process an AUX transaction.
>>>>    * It will call aux_reset() function to reset the AUX channel, @@
>>>> -355,7 +477,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
>>> *dp_aux,
>>>>          }
>>>>
>>>>          dp_aux_update_offset_and_segment(aux, msg);
>>>> -       dp_aux_transfer_helper(aux, msg, true);
>>>> +       dp_aux_transfer_helper(aux, msg, true, false);
>>>>
>>>>          aux->read = msg->request & (DP_AUX_I2C_READ &
>>> DP_AUX_NATIVE_READ);
>>>>          aux->cmd_busy = true;
>>>> @@ -368,7 +490,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
>>> *dp_aux,
>>>>                  aux->no_send_stop = true;
>>>>          }
>>>>
>>>> -       ret = dp_aux_cmd_fifo_tx(aux, msg);
>>>> +       ret = dp_aux_cmd_fifo_tx(aux, msg, false);
>>>>          if (ret < 0) {
>>>>                  if (aux->native) {
>>>>                          aux->retry_cnt++; @@ -535,6 +657,15 @@
>>>> struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog
>*catalog,
>>>>          aux->catalog = catalog;
>>>>          aux->retry_cnt = 0;
>>>>
>>>> +       /*
>>>> +        * Use the drm_dp_aux_init() to use the aux adapter
>>>> +        * before registering aux with the DRM device.
>>>> +        */
>>>> +       aux->dp_aux.name = "dpu_dp_aux";
>>>> +       aux->dp_aux.dev = dev;
>>>> +       aux->dp_aux.transfer = dp_aux_transfer_init;
>>>> +       drm_dp_aux_init(&aux->dp_aux);
>>>
>>> How do you use the aux adapter? It should not be used before
>>> aux->drm_dev is setup.
>>>
>>
>> The drm_dev registration happens during the bind. But we need to use aux
>during probe.
>>
>> The kernel doc for the drm_dp_aux_init function suggested we can use
>> the adapter before drm_dev registration. So, I used this function to enable
>the aux usage during probe.
>
>Then please also change __drm_printk() to use (drm) ? (drm->dev) : NULL as
>a first argument to dev_##level##type. Otherwise the first AUX transfer error
>before aux->drm_dev is set will oops the kernel. See how
>drm_err() is expanded.
>

Okay, will add this also.

>>
>> /**
>>   * drm_dp_aux_init() - minimally initialise an aux channel
>>   * @aux: DisplayPort AUX channel
>>   *
>>   * If you need to use the drm_dp_aux's i2c adapter prior to registering it
>with
>>   * the outside world, call drm_dp_aux_init() first.
>>
>>>> +
>>>>          return &aux->dp_aux;
>>>>   }
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>> index 676279d0ca8d..ccf0400176f0 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>> @@ -258,6 +258,18 @@ int
>>> dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog
>>> *dp_catalog)
>>>>                                  2000, 500000);  }
>>>>
>>>> +int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog
>>>> +*dp_catalog) {
>>>> +       u32 aux;
>>>> +       struct dp_catalog_private *catalog = container_of(dp_catalog,
>>>> +                               struct dp_catalog_private,
>>>> +dp_catalog);
>>>> +
>>>> +       return readl_poll_timeout(catalog->io->dp_controller.ahb.base +
>>>> +                               REG_DP_INTR_STATUS,
>>>> +                               aux, aux & DP_INTERRUPT_STATUS1,
>>>> +                               250, 250000); }
>>>> +
>>>>   static void dump_regs(void __iomem *base, int len)  {
>>>>          int i;
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h
>>>> b/drivers/gpu/drm/msm/dp/dp_catalog.h
>>>> index 1f717f45c115..ad4a9e0f71f2 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
>>>> @@ -87,6 +87,7 @@ void dp_catalog_aux_enable(struct dp_catalog
>>>> *dp_catalog, bool enable);  void dp_catalog_aux_update_cfg(struct
>>>> dp_catalog *dp_catalog);  int
>>>> dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog
>>>> *dp_catalog);
>>>>   u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
>>>> +int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog
>>>> +*dp_catalog);
>>>>
>>>>   /* DP Controller APIs */
>>>>   void dp_catalog_ctrl_state_ctrl(struct dp_catalog *dp_catalog, u32
>>>> state); diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> index bde1a7ce442f..a5dcef040b74 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> @@ -275,13 +275,6 @@ static int dp_display_bind(struct device *dev,
>struct
>>> device *master,
>>>>          dp->dp_display.drm_dev = drm;
>>>>          priv->dp[dp->id] = &dp->dp_display;
>>>>
>>>> -       rc = dp->parser->parse(dp->parser);
>>>> -       if (rc) {
>>>> -               DRM_ERROR("device tree parsing failed\n");
>>>> -               goto end;
>>>> -       }
>>>> -
>>>> -
>>>>          dp->drm_dev = drm;
>>>>          dp->aux->drm_dev = drm;
>>>>          rc = dp_aux_register(dp->aux); @@ -290,12 +283,6 @@ static int
>>>> dp_display_bind(struct device *dev, struct device *master,
>>>>                  goto end;
>>>>          }
>>>>
>>>> -       rc = dp_power_client_init(dp->power);
>>>> -       if (rc) {
>>>> -               DRM_ERROR("Power client create failed\n");
>>>> -               goto end;
>>>> -       }
>>>> -
>>>>          rc = dp_register_audio_driver(dev, dp->audio);
>>>>          if (rc) {
>>>>                  DRM_ERROR("Audio registration Dp failed\n"); @@ -787,6
>>>> +774,12 @@ static int dp_init_sub_modules(struct dp_display_private
>*dp)
>>>>                  goto error;
>>>>          }
>>>>
>>>> +       rc = dp->parser->parse(dp->parser);
>>>> +       if (rc) {
>>>> +               DRM_ERROR("device tree parsing failed\n");
>>>> +               goto error;
>>>> +       }
>>>> +
>>>>          dp->catalog = dp_catalog_get(dev, &dp->parser->io);
>>>>          if (IS_ERR(dp->catalog)) {
>>>>                  rc = PTR_ERR(dp->catalog); @@ -803,6 +796,12 @@ static
>>>> int dp_init_sub_modules(struct dp_display_private *dp)
>>>>                  goto error;
>>>>          }
>>>>
>>>> +       rc = dp_power_client_init(dp->power);
>>>> +       if (rc) {
>>>> +               DRM_ERROR("Power client create failed\n");
>>>> +               goto error;
>>>> +       }
>>>> +
>>>>          dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
>>>>          if (IS_ERR(dp->aux)) {
>>>>                  rc = PTR_ERR(dp->aux); @@ -1338,12 +1337,29 @@ static
>>>> int dp_display_probe(struct platform_device *pdev)
>>>>
>>>>          platform_set_drvdata(pdev, &dp->dp_display);
>>>>
>>>> +       if (dp->dp_display.is_edp) {
>>>> +               dp_display_host_init(dp);
>>>> +               dp_display_host_phy_init(dp);
>>>> +               dp_catalog_ctrl_hpd_config(dp->catalog);
>>>> +
>>>> +               rc = devm_of_dp_aux_populate_bus(dp->aux, NULL);
>>>
>>> You should pass a real done_probing handler here, if you are going to
>refactor
>>> this piece of code. The done_probing should then shut down the
>resources
>>> and bind the component.
>>>
>>
>> I removed the resource enabling part from probe in the next patch where I
>implemented pm_runtime_ops.
>> I moved the host_init, phy_init and hpd_config into runtime_resume and
>calling pm_runtime_get_sync from aux_transfer.
>
>Next patch doesn't exist at this point. So, please, either reorder them,
>or make this patch correct per se.
>
>Also, the key part is a call to component_add(). It should come from
>done_probing callback. AUX bus probing is asynchronous. The kernel
>registers a device and then it might take some time for the correct
>driver to be loaded, etc. We clearly expect dp_display_bind to be used
>only when the panel has been probed.
>


Okay, will add the done_probe function

>>
>>>> +
>>>> +               dp_display_host_phy_exit(dp);
>>>> +               dp_display_host_deinit(dp);
>>>> +
>>>> +               if (rc) {
>>>> +                       DRM_ERROR("failed to initialize panel, rc = %d\n", rc);
>>>> +                       goto error;
>>>> +               }
>>>> +       }
>>>> +
>>>>          rc = component_add(&pdev->dev, &dp_display_comp_ops);
>>>>          if (rc) {
>>>>                  DRM_ERROR("component add failed, rc=%d\n", rc);
>>>>                  dp_display_deinit_sub_modules(dp);
>>>>          }
>>>>
>>>> +error:
>>>>          return rc;
>>>>   }
>>>>
>>>> @@ -1546,40 +1562,8 @@ static int dp_display_get_next_bridge(struct
>>>> msm_dp *dp)  {
>>>>          int rc;
>>>>          struct dp_display_private *dp_priv;
>>>> -       struct device_node *aux_bus;
>>>> -       struct device *dev;
>>>>
>>>>          dp_priv = container_of(dp, struct dp_display_private, dp_display);
>>>> -       dev = &dp_priv->pdev->dev;
>>>> -       aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
>>>> -
>>>> -       if (aux_bus && dp->is_edp) {
>>>> -               dp_display_host_init(dp_priv);
>>>> -               dp_catalog_ctrl_hpd_config(dp_priv->catalog);
>>>> -               dp_display_host_phy_init(dp_priv);
>>>> -               enable_irq(dp_priv->irq);
>>>> -
>>>> -               /*
>>>> -                * The code below assumes that the panel will finish probing
>>>> -                * by the time devm_of_dp_aux_populate_ep_devices()
>returns.
>>>> -                * This isn't a great assumption since it will fail if the
>>>> -                * panel driver is probed asynchronously but is the best we
>>>> -                * can do without a bigger driver reorganization.
>>>> -                */
>>>> -               rc = of_dp_aux_populate_bus(dp_priv->aux, NULL);
>>>> -               of_node_put(aux_bus);
>>>> -               if (rc)
>>>> -                       goto error;
>>>> -
>>>> -               rc = devm_add_action_or_reset(dp->drm_dev->dev,
>>>> -                                               of_dp_aux_depopulate_bus_void,
>>>> -                                               dp_priv->aux);
>>>> -               if (rc)
>>>> -                       goto error;
>>>> -       } else if (dp->is_edp) {
>>>> -               DRM_ERROR("eDP aux_bus not found\n");
>>>> -               return -ENODEV;
>>>> -       }
>>>>
>>>>          /*
>>>>           * External bridges are mandatory for eDP interfaces: one has
>>>> to @@ -1597,12 +1581,6 @@ static int dp_display_get_next_bridge(struct
>>> msm_dp *dp)
>>>>                  return 0;
>>>>          }
>>>>
>>>> -error:
>>>> -       if (dp->is_edp) {
>>>> -               disable_irq(dp_priv->irq);
>>>> -               dp_display_host_phy_exit(dp_priv);
>>>> -               dp_display_host_deinit(dp_priv);
>>>> -       }
>>>>          return rc;
>>>>   }
>>>>
>>>> --
>>>> 2.39.0
>>>>
>>>
>>>
>>> --
>>> With best wishes
>>> Dmitry
>
>--
>With best wishes
>Dmitry

RE: [RFC PATCH 1/2] drm/msm/dp: enumerate edp panel during driver probe

[Sankeerth Billakanti (QUIC)]

Hi Dmitry,

>>>>
>>>> The eDP panel is identified and enumerated during probe of the
>>>> panel-edp driver. The current DP driver triggers this panel-edp
>>>> driver probe while getting the panel-bridge associated with the eDP
>>>> panel from the platform driver bind. If the panel-edp probe is
>>>> deferred, then the whole bunch of MDSS parent and child drivers have
>>>> to defer and
>>> roll back.
>>>
>>> No, MDSS has not been rolled back since 5.19. Please shift your
>>> development on top of the current msm-next.
>>>
>>
>> Okay, I will move to the msm-next tip.
>>
>>>>
>>>> So, we want to move the panel enumeration from bind to probe so that
>>>> the probe defer is easier to handle and also both the drivers appear
>>>> consistent in panel enumeration. This change moves the DP driver
>>>> panel-bridge enumeration to probe.
>>>>
>>>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
>>>> ---
>>>>   drivers/gpu/drm/msm/dp/dp_aux.c     | 149
>>> ++++++++++++++++++++++++++--
>>>>   drivers/gpu/drm/msm/dp/dp_catalog.c |  12 +++
>>>>   drivers/gpu/drm/msm/dp/dp_catalog.h |   1 +
>>>>   drivers/gpu/drm/msm/dp/dp_display.c |  80 ++++++---------
>>>>   4 files changed, 182 insertions(+), 60 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
>>>> b/drivers/gpu/drm/msm/dp/dp_aux.c index
>cc3efed593aa..5da95dfdeede
>>>> 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>>>> @@ -110,7 +110,7 @@ static ssize_t dp_aux_write(struct
>>>> dp_aux_private *aux,  }
>>>>
>>>>   static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
>>>> -                             struct drm_dp_aux_msg *msg)
>>>> +                             struct drm_dp_aux_msg *msg, bool poll)
>>>
>>> What is the reason for working in polled mode rather than always
>>> using the interrupts?
>>>
>>
>> The mdss interrupts will be enabled and can be used after msm_irq_install
>from msm_drm_bind.
>> We want to perform aux transactions during probe. The aux data
>> transmission is followed by an interrupt to indicate successful transmission
>status and reply information.
>
>This is the price of developing on, let me guess, 5.15. Nowadays MDSS
>interrupts are enabled and can be used during dp_display_probe() and
>dp_display_bind(). If they can not for some reason, this is an issue that must
>be fixed. Please reconsider your approach after rebasing onto msm-next or
>6.3-rc1.
>

On the msm-next also, I see the mdss irq is enabled from bind (in msm_drv.c). 
msm_drm_bind -> msm_drm_init -> msm_irq_install

Currently, interrupts will not be available during the dp_display_probe.
Is there a patch series which is enabling IRQs during mdss probe?

>>
>> As interrupts are not enabled, I took this polling approach for aux interrupts
>during probe.
>>
>>>>   {
>>>>          ssize_t ret;
>>>>          unsigned long time_left;
>>>> @@ -121,10 +121,16 @@ static ssize_t dp_aux_cmd_fifo_tx(struct
>>> dp_aux_private *aux,
>>>>          if (ret < 0)
>>>>                  return ret;
>>>>
>>>> -       time_left = wait_for_completion_timeout(&aux->comp,
>>>> +       if (!poll) {
>>>> +               time_left = wait_for_completion_timeout(&aux->comp,
>>>>                                                  msecs_to_jiffies(250));
>>>> -       if (!time_left)
>>>> -               return -ETIMEDOUT;
>>>> +               if (!time_left)
>>>> +                       return -ETIMEDOUT;
>>>> +       } else {
>>>> +               ret = dp_catalog_aux_poll_and_get_hw_interrupt(aux-
>>catalog);
>>>> +               if (!ret)
>>>> +                       dp_aux_isr(&aux->dp_aux);
>>>> +       }
>>>>
>>>>          return ret;
>>>>   }
>>>> @@ -238,7 +244,7 @@ static void
>>> dp_aux_update_offset_and_segment(struct dp_aux_private *aux,
>>>>    */
>>>>   static void dp_aux_transfer_helper(struct dp_aux_private *aux,
>>>>                                     struct drm_dp_aux_msg *input_msg,
>>>> -                                  bool send_seg)
>>>> +                                  bool send_seg, bool poll)
>>>>   {
>>>>          struct drm_dp_aux_msg helper_msg;
>>>>          u32 message_size = 0x10;
>>>> @@ -278,7 +284,7 @@ static void dp_aux_transfer_helper(struct
>>> dp_aux_private *aux,
>>>>                  helper_msg.address = segment_address;
>>>>                  helper_msg.buffer = &aux->segment;
>>>>                  helper_msg.size = 1;
>>>> -               dp_aux_cmd_fifo_tx(aux, &helper_msg);
>>>> +               dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
>>>>          }
>>>>
>>>>          /*
>>>> @@ -292,7 +298,7 @@ static void dp_aux_transfer_helper(struct
>>> dp_aux_private *aux,
>>>>          helper_msg.address = input_msg->address;
>>>>          helper_msg.buffer = &aux->offset;
>>>>          helper_msg.size = 1;
>>>> -       dp_aux_cmd_fifo_tx(aux, &helper_msg);
>>>> +       dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
>>>>
>>>>   end:
>>>>          aux->offset += message_size; @@ -300,6 +306,122 @@ static
>>>> void dp_aux_transfer_helper(struct
>>> dp_aux_private *aux,
>>>>                  aux->segment = 0x0; /* reset segment at end of
>>>> block */  }
>>>>
>>>> +/*
>>>> + * This function does the real job to process an AUX transaction.
>>>> + * It will call aux_reset() function to reset the AUX channel,
>>>> + * if the waiting is timeout.
>>>> + */
>>>> +static ssize_t dp_aux_transfer_init(struct drm_dp_aux *dp_aux,
>>>> +                              struct drm_dp_aux_msg *msg) {
>>>> +       ssize_t ret;
>>>> +       int const aux_cmd_native_max = 16;
>>>> +       int const aux_cmd_i2c_max = 128;
>>>> +       struct dp_aux_private *aux;
>>>> +
>>>> +       aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
>>>> +
>>>> +       aux->native = msg->request & (DP_AUX_NATIVE_WRITE &
>>>> + DP_AUX_NATIVE_READ);
>>>> +
>>>> +       /* Ignore address only message */
>>>> +       if (msg->size == 0 || !msg->buffer) {
>>>> +               msg->reply = aux->native ?
>>>> +                       DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
>>>> +               return msg->size;
>>>> +       }
>>>> +
>>>> +       /* msg sanity check */
>>>> +       if ((aux->native && msg->size > aux_cmd_native_max) ||
>>>> +           msg->size > aux_cmd_i2c_max) {
>>>> +               DRM_ERROR("%s: invalid msg: size(%zu), request(%x)\n",
>>>> +                       __func__, msg->size, msg->request);
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>> +       mutex_lock(&aux->mutex);
>>>> +       if (!aux->initted) {
>>>> +               ret = -EIO;
>>>> +               goto exit;
>>>> +       }
>>>> +
>>>> +       /*
>>>> +        * For eDP it's important to give a reasonably long wait here for HPD
>>>> +        * to be asserted. This is because the panel driver may have _just_
>>>> +        * turned on the panel and then tried to do an AUX transfer. The
>panel
>>>> +        * driver has no way of knowing when the panel is ready, so it's up
>>>> +        * to us to wait. For DP we never get into this situation so let's
>>>> +        * avoid ever doing the extra long wait for DP.
>>>> +        */
>>>> +       if (aux->is_edp) {
>>>> +               ret = dp_catalog_aux_wait_for_hpd_connect_state(aux-
>>catalog);
>>>> +               if (ret) {
>>>> +                       DRM_DEBUG_DP("Panel not ready for aux transactions\n");
>>>> +                       goto exit;
>>>> +               }
>>>> +       }
>>>> +
>>>> +       dp_aux_update_offset_and_segment(aux, msg);
>>>> +       dp_aux_transfer_helper(aux, msg, true, true);
>>>> +
>>>> +       aux->read = msg->request & (DP_AUX_I2C_READ &
>>> DP_AUX_NATIVE_READ);
>>>> +       aux->cmd_busy = true;
>>>> +
>>>> +       if (aux->read) {
>>>> +               aux->no_send_addr = true;
>>>> +               aux->no_send_stop = false;
>>>> +       } else {
>>>> +               aux->no_send_addr = true;
>>>> +               aux->no_send_stop = true;
>>>> +       }
>>>> +
>>>> +       ret = dp_aux_cmd_fifo_tx(aux, msg, true);
>>>> +
>>>> +       /* TODO: why is fifo_rx necessary here?
>>>> +        * Ideally fifo_rx need not be executed for an aux write
>>>> +        */
>>>> +       ret = dp_aux_cmd_fifo_rx(aux, msg);
>>>> +
>>>> +       if (ret < 0) {
>>>> +               if (aux->native) {
>>>> +                       aux->retry_cnt++;
>>>> +                       if (!(aux->retry_cnt % MAX_AUX_RETRIES))
>>>> +                               dp_catalog_aux_update_cfg(aux->catalog);
>>>> +               }
>>>> +               /* reset aux if link is in connected state */
>>>> +               if (dp_catalog_link_is_connected(aux->catalog))
>>>> +                       dp_catalog_aux_reset(aux->catalog);
>>>> +       } else {
>>>> +               aux->retry_cnt = 0;
>>>> +               switch (aux->aux_error_num) {
>>>> +               case DP_AUX_ERR_NONE:
>>>> +                       if (aux->read)
>>>> +                               ret = dp_aux_cmd_fifo_rx(aux, msg);
>>>> +                       msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_ACK :
>>> DP_AUX_I2C_REPLY_ACK;
>>>> +                       break;
>>>> +               case DP_AUX_ERR_DEFER:
>>>> +                       msg->reply = aux->native ?
>>>> +                               DP_AUX_NATIVE_REPLY_DEFER :
>>> DP_AUX_I2C_REPLY_DEFER;
>>>> +                       break;
>>>> +               case DP_AUX_ERR_PHY:
>>>> +               case DP_AUX_ERR_ADDR:
>>>> +               case DP_AUX_ERR_NACK:
>>>> +               case DP_AUX_ERR_NACK_DEFER:
>>>> +                       msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_NACK :
>>> DP_AUX_I2C_REPLY_NACK;
>>>> +                       break;
>>>> +               case DP_AUX_ERR_TOUT:
>>>> +                       ret = -ETIMEDOUT;
>>>> +                       break;
>>>> +               }
>>>> +       }
>>>> +
>>>> +       aux->cmd_busy = false;
>>>> +
>>>> +exit:
>>>> +       mutex_unlock(&aux->mutex);
>>>> +
>>>> +       return ret;
>>>> +}
>>>> +
>>>>   /*
>>>>    * This function does the real job to process an AUX transaction.
>>>>    * It will call aux_reset() function to reset the AUX channel, @@
>>>> -355,7 +477,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
>>> *dp_aux,
>>>>          }
>>>>
>>>>          dp_aux_update_offset_and_segment(aux, msg);
>>>> -       dp_aux_transfer_helper(aux, msg, true);
>>>> +       dp_aux_transfer_helper(aux, msg, true, false);
>>>>
>>>>          aux->read = msg->request & (DP_AUX_I2C_READ &
>>> DP_AUX_NATIVE_READ);
>>>>          aux->cmd_busy = true;
>>>> @@ -368,7 +490,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
>>> *dp_aux,
>>>>                  aux->no_send_stop = true;
>>>>          }
>>>>
>>>> -       ret = dp_aux_cmd_fifo_tx(aux, msg);
>>>> +       ret = dp_aux_cmd_fifo_tx(aux, msg, false);
>>>>          if (ret < 0) {
>>>>                  if (aux->native) {
>>>>                          aux->retry_cnt++; @@ -535,6 +657,15 @@
>>>> struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog
>*catalog,
>>>>          aux->catalog = catalog;
>>>>          aux->retry_cnt = 0;
>>>>
>>>> +       /*
>>>> +        * Use the drm_dp_aux_init() to use the aux adapter
>>>> +        * before registering aux with the DRM device.
>>>> +        */
>>>> +       aux->dp_aux.name = "dpu_dp_aux";
>>>> +       aux->dp_aux.dev = dev;
>>>> +       aux->dp_aux.transfer = dp_aux_transfer_init;
>>>> +       drm_dp_aux_init(&aux->dp_aux);
>>>
>>> How do you use the aux adapter? It should not be used before
>>> aux->drm_dev is setup.
>>>
>>
>> The drm_dev registration happens during the bind. But we need to use aux
>during probe.
>>
>> The kernel doc for the drm_dp_aux_init function suggested we can use
>> the adapter before drm_dev registration. So, I used this function to enable
>the aux usage during probe.
>
>Then please also change __drm_printk() to use (drm) ? (drm->dev) : NULL as
>a first argument to dev_##level##type. Otherwise the first AUX transfer error
>before aux->drm_dev is set will oops the kernel. See how
>drm_err() is expanded.
>

Okay, will add this also.

>>
>> /**
>>   * drm_dp_aux_init() - minimally initialise an aux channel
>>   * @aux: DisplayPort AUX channel
>>   *
>>   * If you need to use the drm_dp_aux's i2c adapter prior to registering it
>with
>>   * the outside world, call drm_dp_aux_init() first.
>>
>>>> +
>>>>          return &aux->dp_aux;
>>>>   }
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>> index 676279d0ca8d..ccf0400176f0 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>>>> @@ -258,6 +258,18 @@ int
>>> dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog
>>> *dp_catalog)
>>>>                                  2000, 500000);  }
>>>>
>>>> +int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog
>>>> +*dp_catalog) {
>>>> +       u32 aux;
>>>> +       struct dp_catalog_private *catalog = container_of(dp_catalog,
>>>> +                               struct dp_catalog_private,
>>>> +dp_catalog);
>>>> +
>>>> +       return readl_poll_timeout(catalog->io->dp_controller.ahb.base +
>>>> +                               REG_DP_INTR_STATUS,
>>>> +                               aux, aux & DP_INTERRUPT_STATUS1,
>>>> +                               250, 250000); }
>>>> +
>>>>   static void dump_regs(void __iomem *base, int len)  {
>>>>          int i;
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h
>>>> b/drivers/gpu/drm/msm/dp/dp_catalog.h
>>>> index 1f717f45c115..ad4a9e0f71f2 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
>>>> @@ -87,6 +87,7 @@ void dp_catalog_aux_enable(struct dp_catalog
>>>> *dp_catalog, bool enable);  void dp_catalog_aux_update_cfg(struct
>>>> dp_catalog *dp_catalog);  int
>>>> dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog
>>>> *dp_catalog);
>>>>   u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
>>>> +int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog
>>>> +*dp_catalog);
>>>>
>>>>   /* DP Controller APIs */
>>>>   void dp_catalog_ctrl_state_ctrl(struct dp_catalog *dp_catalog, u32
>>>> state); diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> index bde1a7ce442f..a5dcef040b74 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> @@ -275,13 +275,6 @@ static int dp_display_bind(struct device *dev,
>struct
>>> device *master,
>>>>          dp->dp_display.drm_dev = drm;
>>>>          priv->dp[dp->id] = &dp->dp_display;
>>>>
>>>> -       rc = dp->parser->parse(dp->parser);
>>>> -       if (rc) {
>>>> -               DRM_ERROR("device tree parsing failed\n");
>>>> -               goto end;
>>>> -       }
>>>> -
>>>> -
>>>>          dp->drm_dev = drm;
>>>>          dp->aux->drm_dev = drm;
>>>>          rc = dp_aux_register(dp->aux); @@ -290,12 +283,6 @@ static int
>>>> dp_display_bind(struct device *dev, struct device *master,
>>>>                  goto end;
>>>>          }
>>>>
>>>> -       rc = dp_power_client_init(dp->power);
>>>> -       if (rc) {
>>>> -               DRM_ERROR("Power client create failed\n");
>>>> -               goto end;
>>>> -       }
>>>> -
>>>>          rc = dp_register_audio_driver(dev, dp->audio);
>>>>          if (rc) {
>>>>                  DRM_ERROR("Audio registration Dp failed\n"); @@ -787,6
>>>> +774,12 @@ static int dp_init_sub_modules(struct dp_display_private
>*dp)
>>>>                  goto error;
>>>>          }
>>>>
>>>> +       rc = dp->parser->parse(dp->parser);
>>>> +       if (rc) {
>>>> +               DRM_ERROR("device tree parsing failed\n");
>>>> +               goto error;
>>>> +       }
>>>> +
>>>>          dp->catalog = dp_catalog_get(dev, &dp->parser->io);
>>>>          if (IS_ERR(dp->catalog)) {
>>>>                  rc = PTR_ERR(dp->catalog); @@ -803,6 +796,12 @@ static
>>>> int dp_init_sub_modules(struct dp_display_private *dp)
>>>>                  goto error;
>>>>          }
>>>>
>>>> +       rc = dp_power_client_init(dp->power);
>>>> +       if (rc) {
>>>> +               DRM_ERROR("Power client create failed\n");
>>>> +               goto error;
>>>> +       }
>>>> +
>>>>          dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
>>>>          if (IS_ERR(dp->aux)) {
>>>>                  rc = PTR_ERR(dp->aux); @@ -1338,12 +1337,29 @@ static
>>>> int dp_display_probe(struct platform_device *pdev)
>>>>
>>>>          platform_set_drvdata(pdev, &dp->dp_display);
>>>>
>>>> +       if (dp->dp_display.is_edp) {
>>>> +               dp_display_host_init(dp);
>>>> +               dp_display_host_phy_init(dp);
>>>> +               dp_catalog_ctrl_hpd_config(dp->catalog);
>>>> +
>>>> +               rc = devm_of_dp_aux_populate_bus(dp->aux, NULL);
>>>
>>> You should pass a real done_probing handler here, if you are going to
>refactor
>>> this piece of code. The done_probing should then shut down the
>resources
>>> and bind the component.
>>>
>>
>> I removed the resource enabling part from probe in the next patch where I
>implemented pm_runtime_ops.
>> I moved the host_init, phy_init and hpd_config into runtime_resume and
>calling pm_runtime_get_sync from aux_transfer.
>
>Next patch doesn't exist at this point. So, please, either reorder them,
>or make this patch correct per se.
>
>Also, the key part is a call to component_add(). It should come from
>done_probing callback. AUX bus probing is asynchronous. The kernel
>registers a device and then it might take some time for the correct
>driver to be loaded, etc. We clearly expect dp_display_bind to be used
>only when the panel has been probed.
>


Okay, will add the done_probe function

>>
>>>> +
>>>> +               dp_display_host_phy_exit(dp);
>>>> +               dp_display_host_deinit(dp);
>>>> +
>>>> +               if (rc) {
>>>> +                       DRM_ERROR("failed to initialize panel, rc = %d\n", rc);
>>>> +                       goto error;
>>>> +               }
>>>> +       }
>>>> +
>>>>          rc = component_add(&pdev->dev, &dp_display_comp_ops);
>>>>          if (rc) {
>>>>                  DRM_ERROR("component add failed, rc=%d\n", rc);
>>>>                  dp_display_deinit_sub_modules(dp);
>>>>          }
>>>>
>>>> +error:
>>>>          return rc;
>>>>   }
>>>>
>>>> @@ -1546,40 +1562,8 @@ static int dp_display_get_next_bridge(struct
>>>> msm_dp *dp)  {
>>>>          int rc;
>>>>          struct dp_display_private *dp_priv;
>>>> -       struct device_node *aux_bus;
>>>> -       struct device *dev;
>>>>
>>>>          dp_priv = container_of(dp, struct dp_display_private, dp_display);
>>>> -       dev = &dp_priv->pdev->dev;
>>>> -       aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
>>>> -
>>>> -       if (aux_bus && dp->is_edp) {
>>>> -               dp_display_host_init(dp_priv);
>>>> -               dp_catalog_ctrl_hpd_config(dp_priv->catalog);
>>>> -               dp_display_host_phy_init(dp_priv);
>>>> -               enable_irq(dp_priv->irq);
>>>> -
>>>> -               /*
>>>> -                * The code below assumes that the panel will finish probing
>>>> -                * by the time devm_of_dp_aux_populate_ep_devices()
>returns.
>>>> -                * This isn't a great assumption since it will fail if the
>>>> -                * panel driver is probed asynchronously but is the best we
>>>> -                * can do without a bigger driver reorganization.
>>>> -                */
>>>> -               rc = of_dp_aux_populate_bus(dp_priv->aux, NULL);
>>>> -               of_node_put(aux_bus);
>>>> -               if (rc)
>>>> -                       goto error;
>>>> -
>>>> -               rc = devm_add_action_or_reset(dp->drm_dev->dev,
>>>> -                                               of_dp_aux_depopulate_bus_void,
>>>> -                                               dp_priv->aux);
>>>> -               if (rc)
>>>> -                       goto error;
>>>> -       } else if (dp->is_edp) {
>>>> -               DRM_ERROR("eDP aux_bus not found\n");
>>>> -               return -ENODEV;
>>>> -       }
>>>>
>>>>          /*
>>>>           * External bridges are mandatory for eDP interfaces: one has
>>>> to @@ -1597,12 +1581,6 @@ static int dp_display_get_next_bridge(struct
>>> msm_dp *dp)
>>>>                  return 0;
>>>>          }
>>>>
>>>> -error:
>>>> -       if (dp->is_edp) {
>>>> -               disable_irq(dp_priv->irq);
>>>> -               dp_display_host_phy_exit(dp_priv);
>>>> -               dp_display_host_deinit(dp_priv);
>>>> -       }
>>>>          return rc;
>>>>   }
>>>>
>>>> --
>>>> 2.39.0
>>>>
>>>
>>>
>>> --
>>> With best wishes
>>> Dmitry
>
>--
>With best wishes
>Dmitry

Re: [RFC PATCH 1/2] drm/msm/dp: enumerate edp panel during driver probe

[Dmitry Baryshkov]

On Tue, 14 Mar 2023 at 12:23, Sankeerth Billakanti (QUIC)
<quic_sbillaka@quicinc.com> wrote:
>
> Hi Dmitry,
>
> >>>>
> >>>> The eDP panel is identified and enumerated during probe of the
> >>>> panel-edp driver. The current DP driver triggers this panel-edp
> >>>> driver probe while getting the panel-bridge associated with the eDP
> >>>> panel from the platform driver bind. If the panel-edp probe is
> >>>> deferred, then the whole bunch of MDSS parent and child drivers have
> >>>> to defer and
> >>> roll back.
> >>>
> >>> No, MDSS has not been rolled back since 5.19. Please shift your
> >>> development on top of the current msm-next.
> >>>
> >>
> >> Okay, I will move to the msm-next tip.
> >>
> >>>>
> >>>> So, we want to move the panel enumeration from bind to probe so that
> >>>> the probe defer is easier to handle and also both the drivers appear
> >>>> consistent in panel enumeration. This change moves the DP driver
> >>>> panel-bridge enumeration to probe.
> >>>>
> >>>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> >>>> ---
> >>>>   drivers/gpu/drm/msm/dp/dp_aux.c     | 149
> >>> ++++++++++++++++++++++++++--
> >>>>   drivers/gpu/drm/msm/dp/dp_catalog.c |  12 +++
> >>>>   drivers/gpu/drm/msm/dp/dp_catalog.h |   1 +
> >>>>   drivers/gpu/drm/msm/dp/dp_display.c |  80 ++++++---------
> >>>>   4 files changed, 182 insertions(+), 60 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
> >>>> b/drivers/gpu/drm/msm/dp/dp_aux.c index
> >cc3efed593aa..5da95dfdeede
> >>>> 100644
> >>>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> >>>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> >>>> @@ -110,7 +110,7 @@ static ssize_t dp_aux_write(struct
> >>>> dp_aux_private *aux,  }
> >>>>
> >>>>   static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
> >>>> -                             struct drm_dp_aux_msg *msg)
> >>>> +                             struct drm_dp_aux_msg *msg, bool poll)
> >>>
> >>> What is the reason for working in polled mode rather than always
> >>> using the interrupts?
> >>>
> >>
> >> The mdss interrupts will be enabled and can be used after msm_irq_install
> >from msm_drm_bind.
> >> We want to perform aux transactions during probe. The aux data
> >> transmission is followed by an interrupt to indicate successful transmission
> >status and reply information.
> >
> >This is the price of developing on, let me guess, 5.15. Nowadays MDSS
> >interrupts are enabled and can be used during dp_display_probe() and
> >dp_display_bind(). If they can not for some reason, this is an issue that must
> >be fixed. Please reconsider your approach after rebasing onto msm-next or
> >6.3-rc1.
> >
>
> On the msm-next also, I see the mdss irq is enabled from bind (in msm_drv.c).
> msm_drm_bind -> msm_drm_init -> msm_irq_install

MDSS IRQ domain is registered from mdss_probe() -> msm_mdss_init() ->
_msm_mdss_irq_domain_add().

Please note the difference between MDSS IRQs (handled by MDSS) vs
MDP/DPU irq (registered by msm_irq_install()).

> Currently, interrupts will not be available during the dp_display_probe.

I can not agree with you here.

> Is there a patch series which is enabling IRQs during mdss probe?

https://patchwork.freedesktop.org/series/98525/, merged for 5.19.

>
> >>
> >> As interrupts are not enabled, I took this polling approach for aux interrupts
> >during probe.
> >>
> >>>>   {
> >>>>          ssize_t ret;
> >>>>          unsigned long time_left;
> >>>> @@ -121,10 +121,16 @@ static ssize_t dp_aux_cmd_fifo_tx(struct
> >>> dp_aux_private *aux,
> >>>>          if (ret < 0)
> >>>>                  return ret;
> >>>>
> >>>> -       time_left = wait_for_completion_timeout(&aux->comp,
> >>>> +       if (!poll) {
> >>>> +               time_left = wait_for_completion_timeout(&aux->comp,
> >>>>                                                  msecs_to_jiffies(250));
> >>>> -       if (!time_left)
> >>>> -               return -ETIMEDOUT;
> >>>> +               if (!time_left)
> >>>> +                       return -ETIMEDOUT;
> >>>> +       } else {
> >>>> +               ret = dp_catalog_aux_poll_and_get_hw_interrupt(aux-
> >>catalog);
> >>>> +               if (!ret)
> >>>> +                       dp_aux_isr(&aux->dp_aux);
> >>>> +       }
> >>>>
> >>>>          return ret;
> >>>>   }
> >>>> @@ -238,7 +244,7 @@ static void
> >>> dp_aux_update_offset_and_segment(struct dp_aux_private *aux,
> >>>>    */
> >>>>   static void dp_aux_transfer_helper(struct dp_aux_private *aux,
> >>>>                                     struct drm_dp_aux_msg *input_msg,
> >>>> -                                  bool send_seg)
> >>>> +                                  bool send_seg, bool poll)
> >>>>   {
> >>>>          struct drm_dp_aux_msg helper_msg;
> >>>>          u32 message_size = 0x10;
> >>>> @@ -278,7 +284,7 @@ static void dp_aux_transfer_helper(struct
> >>> dp_aux_private *aux,
> >>>>                  helper_msg.address = segment_address;
> >>>>                  helper_msg.buffer = &aux->segment;
> >>>>                  helper_msg.size = 1;
> >>>> -               dp_aux_cmd_fifo_tx(aux, &helper_msg);
> >>>> +               dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
> >>>>          }
> >>>>
> >>>>          /*
> >>>> @@ -292,7 +298,7 @@ static void dp_aux_transfer_helper(struct
> >>> dp_aux_private *aux,
> >>>>          helper_msg.address = input_msg->address;
> >>>>          helper_msg.buffer = &aux->offset;
> >>>>          helper_msg.size = 1;
> >>>> -       dp_aux_cmd_fifo_tx(aux, &helper_msg);
> >>>> +       dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
> >>>>
> >>>>   end:
> >>>>          aux->offset += message_size; @@ -300,6 +306,122 @@ static
> >>>> void dp_aux_transfer_helper(struct
> >>> dp_aux_private *aux,
> >>>>                  aux->segment = 0x0; /* reset segment at end of
> >>>> block */  }
> >>>>
> >>>> +/*
> >>>> + * This function does the real job to process an AUX transaction.
> >>>> + * It will call aux_reset() function to reset the AUX channel,
> >>>> + * if the waiting is timeout.
> >>>> + */
> >>>> +static ssize_t dp_aux_transfer_init(struct drm_dp_aux *dp_aux,
> >>>> +                              struct drm_dp_aux_msg *msg) {
> >>>> +       ssize_t ret;
> >>>> +       int const aux_cmd_native_max = 16;
> >>>> +       int const aux_cmd_i2c_max = 128;
> >>>> +       struct dp_aux_private *aux;
> >>>> +
> >>>> +       aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
> >>>> +
> >>>> +       aux->native = msg->request & (DP_AUX_NATIVE_WRITE &
> >>>> + DP_AUX_NATIVE_READ);
> >>>> +
> >>>> +       /* Ignore address only message */
> >>>> +       if (msg->size == 0 || !msg->buffer) {
> >>>> +               msg->reply = aux->native ?
> >>>> +                       DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
> >>>> +               return msg->size;
> >>>> +       }
> >>>> +
> >>>> +       /* msg sanity check */
> >>>> +       if ((aux->native && msg->size > aux_cmd_native_max) ||
> >>>> +           msg->size > aux_cmd_i2c_max) {
> >>>> +               DRM_ERROR("%s: invalid msg: size(%zu), request(%x)\n",
> >>>> +                       __func__, msg->size, msg->request);
> >>>> +               return -EINVAL;
> >>>> +       }
> >>>> +
> >>>> +       mutex_lock(&aux->mutex);
> >>>> +       if (!aux->initted) {
> >>>> +               ret = -EIO;
> >>>> +               goto exit;
> >>>> +       }
> >>>> +
> >>>> +       /*
> >>>> +        * For eDP it's important to give a reasonably long wait here for HPD
> >>>> +        * to be asserted. This is because the panel driver may have _just_
> >>>> +        * turned on the panel and then tried to do an AUX transfer. The
> >panel
> >>>> +        * driver has no way of knowing when the panel is ready, so it's up
> >>>> +        * to us to wait. For DP we never get into this situation so let's
> >>>> +        * avoid ever doing the extra long wait for DP.
> >>>> +        */
> >>>> +       if (aux->is_edp) {
> >>>> +               ret = dp_catalog_aux_wait_for_hpd_connect_state(aux-
> >>catalog);
> >>>> +               if (ret) {
> >>>> +                       DRM_DEBUG_DP("Panel not ready for aux transactions\n");
> >>>> +                       goto exit;
> >>>> +               }
> >>>> +       }
> >>>> +
> >>>> +       dp_aux_update_offset_and_segment(aux, msg);
> >>>> +       dp_aux_transfer_helper(aux, msg, true, true);
> >>>> +
> >>>> +       aux->read = msg->request & (DP_AUX_I2C_READ &
> >>> DP_AUX_NATIVE_READ);
> >>>> +       aux->cmd_busy = true;
> >>>> +
> >>>> +       if (aux->read) {
> >>>> +               aux->no_send_addr = true;
> >>>> +               aux->no_send_stop = false;
> >>>> +       } else {
> >>>> +               aux->no_send_addr = true;
> >>>> +               aux->no_send_stop = true;
> >>>> +       }
> >>>> +
> >>>> +       ret = dp_aux_cmd_fifo_tx(aux, msg, true);
> >>>> +
> >>>> +       /* TODO: why is fifo_rx necessary here?
> >>>> +        * Ideally fifo_rx need not be executed for an aux write
> >>>> +        */
> >>>> +       ret = dp_aux_cmd_fifo_rx(aux, msg);
> >>>> +
> >>>> +       if (ret < 0) {
> >>>> +               if (aux->native) {
> >>>> +                       aux->retry_cnt++;
> >>>> +                       if (!(aux->retry_cnt % MAX_AUX_RETRIES))
> >>>> +                               dp_catalog_aux_update_cfg(aux->catalog);
> >>>> +               }
> >>>> +               /* reset aux if link is in connected state */
> >>>> +               if (dp_catalog_link_is_connected(aux->catalog))
> >>>> +                       dp_catalog_aux_reset(aux->catalog);
> >>>> +       } else {
> >>>> +               aux->retry_cnt = 0;
> >>>> +               switch (aux->aux_error_num) {
> >>>> +               case DP_AUX_ERR_NONE:
> >>>> +                       if (aux->read)
> >>>> +                               ret = dp_aux_cmd_fifo_rx(aux, msg);
> >>>> +                       msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_ACK :
> >>> DP_AUX_I2C_REPLY_ACK;
> >>>> +                       break;
> >>>> +               case DP_AUX_ERR_DEFER:
> >>>> +                       msg->reply = aux->native ?
> >>>> +                               DP_AUX_NATIVE_REPLY_DEFER :
> >>> DP_AUX_I2C_REPLY_DEFER;
> >>>> +                       break;
> >>>> +               case DP_AUX_ERR_PHY:
> >>>> +               case DP_AUX_ERR_ADDR:
> >>>> +               case DP_AUX_ERR_NACK:
> >>>> +               case DP_AUX_ERR_NACK_DEFER:
> >>>> +                       msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_NACK :
> >>> DP_AUX_I2C_REPLY_NACK;
> >>>> +                       break;
> >>>> +               case DP_AUX_ERR_TOUT:
> >>>> +                       ret = -ETIMEDOUT;
> >>>> +                       break;
> >>>> +               }
> >>>> +       }
> >>>> +
> >>>> +       aux->cmd_busy = false;
> >>>> +
> >>>> +exit:
> >>>> +       mutex_unlock(&aux->mutex);
> >>>> +
> >>>> +       return ret;
> >>>> +}
> >>>> +
> >>>>   /*
> >>>>    * This function does the real job to process an AUX transaction.
> >>>>    * It will call aux_reset() function to reset the AUX channel, @@
> >>>> -355,7 +477,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
> >>> *dp_aux,
> >>>>          }
> >>>>
> >>>>          dp_aux_update_offset_and_segment(aux, msg);
> >>>> -       dp_aux_transfer_helper(aux, msg, true);
> >>>> +       dp_aux_transfer_helper(aux, msg, true, false);
> >>>>
> >>>>          aux->read = msg->request & (DP_AUX_I2C_READ &
> >>> DP_AUX_NATIVE_READ);
> >>>>          aux->cmd_busy = true;
> >>>> @@ -368,7 +490,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
> >>> *dp_aux,
> >>>>                  aux->no_send_stop = true;
> >>>>          }
> >>>>
> >>>> -       ret = dp_aux_cmd_fifo_tx(aux, msg);
> >>>> +       ret = dp_aux_cmd_fifo_tx(aux, msg, false);
> >>>>          if (ret < 0) {
> >>>>                  if (aux->native) {
> >>>>                          aux->retry_cnt++; @@ -535,6 +657,15 @@
> >>>> struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog
> >*catalog,
> >>>>          aux->catalog = catalog;
> >>>>          aux->retry_cnt = 0;
> >>>>
> >>>> +       /*
> >>>> +        * Use the drm_dp_aux_init() to use the aux adapter
> >>>> +        * before registering aux with the DRM device.
> >>>> +        */
> >>>> +       aux->dp_aux.name = "dpu_dp_aux";
> >>>> +       aux->dp_aux.dev = dev;
> >>>> +       aux->dp_aux.transfer = dp_aux_transfer_init;
> >>>> +       drm_dp_aux_init(&aux->dp_aux);
> >>>
> >>> How do you use the aux adapter? It should not be used before
> >>> aux->drm_dev is setup.
> >>>
> >>
> >> The drm_dev registration happens during the bind. But we need to use aux
> >during probe.
> >>
> >> The kernel doc for the drm_dp_aux_init function suggested we can use
> >> the adapter before drm_dev registration. So, I used this function to enable
> >the aux usage during probe.
> >
> >Then please also change __drm_printk() to use (drm) ? (drm->dev) : NULL as
> >a first argument to dev_##level##type. Otherwise the first AUX transfer error
> >before aux->drm_dev is set will oops the kernel. See how
> >drm_err() is expanded.
> >
>
> Okay, will add this also.

Good.

>
> >>
> >> /**
> >>   * drm_dp_aux_init() - minimally initialise an aux channel
> >>   * @aux: DisplayPort AUX channel
> >>   *
> >>   * If you need to use the drm_dp_aux's i2c adapter prior to registering it
> >with
> >>   * the outside world, call drm_dp_aux_init() first.
> >>
> >>>> +
> >>>>          return &aux->dp_aux;
> >>>>   }
> >>>>
> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>>> index 676279d0ca8d..ccf0400176f0 100644
> >>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>>> @@ -258,6 +258,18 @@ int
> >>> dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog
> >>> *dp_catalog)
> >>>>                                  2000, 500000);  }
> >>>>
> >>>> +int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog
> >>>> +*dp_catalog) {
> >>>> +       u32 aux;
> >>>> +       struct dp_catalog_private *catalog = container_of(dp_catalog,
> >>>> +                               struct dp_catalog_private,
> >>>> +dp_catalog);
> >>>> +
> >>>> +       return readl_poll_timeout(catalog->io->dp_controller.ahb.base +
> >>>> +                               REG_DP_INTR_STATUS,
> >>>> +                               aux, aux & DP_INTERRUPT_STATUS1,
> >>>> +                               250, 250000); }
> >>>> +
> >>>>   static void dump_regs(void __iomem *base, int len)  {
> >>>>          int i;
> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h
> >>>> b/drivers/gpu/drm/msm/dp/dp_catalog.h
> >>>> index 1f717f45c115..ad4a9e0f71f2 100644
> >>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
> >>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
> >>>> @@ -87,6 +87,7 @@ void dp_catalog_aux_enable(struct dp_catalog
> >>>> *dp_catalog, bool enable);  void dp_catalog_aux_update_cfg(struct
> >>>> dp_catalog *dp_catalog);  int
> >>>> dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog
> >>>> *dp_catalog);
> >>>>   u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
> >>>> +int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog
> >>>> +*dp_catalog);
> >>>>
> >>>>   /* DP Controller APIs */
> >>>>   void dp_catalog_ctrl_state_ctrl(struct dp_catalog *dp_catalog, u32
> >>>> state); diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> b/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> index bde1a7ce442f..a5dcef040b74 100644
> >>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> @@ -275,13 +275,6 @@ static int dp_display_bind(struct device *dev,
> >struct
> >>> device *master,
> >>>>          dp->dp_display.drm_dev = drm;
> >>>>          priv->dp[dp->id] = &dp->dp_display;
> >>>>
> >>>> -       rc = dp->parser->parse(dp->parser);
> >>>> -       if (rc) {
> >>>> -               DRM_ERROR("device tree parsing failed\n");
> >>>> -               goto end;
> >>>> -       }
> >>>> -
> >>>> -
> >>>>          dp->drm_dev = drm;
> >>>>          dp->aux->drm_dev = drm;
> >>>>          rc = dp_aux_register(dp->aux); @@ -290,12 +283,6 @@ static int
> >>>> dp_display_bind(struct device *dev, struct device *master,
> >>>>                  goto end;
> >>>>          }
> >>>>
> >>>> -       rc = dp_power_client_init(dp->power);
> >>>> -       if (rc) {
> >>>> -               DRM_ERROR("Power client create failed\n");
> >>>> -               goto end;
> >>>> -       }
> >>>> -
> >>>>          rc = dp_register_audio_driver(dev, dp->audio);
> >>>>          if (rc) {
> >>>>                  DRM_ERROR("Audio registration Dp failed\n"); @@ -787,6
> >>>> +774,12 @@ static int dp_init_sub_modules(struct dp_display_private
> >*dp)
> >>>>                  goto error;
> >>>>          }
> >>>>
> >>>> +       rc = dp->parser->parse(dp->parser);
> >>>> +       if (rc) {
> >>>> +               DRM_ERROR("device tree parsing failed\n");
> >>>> +               goto error;
> >>>> +       }
> >>>> +
> >>>>          dp->catalog = dp_catalog_get(dev, &dp->parser->io);
> >>>>          if (IS_ERR(dp->catalog)) {
> >>>>                  rc = PTR_ERR(dp->catalog); @@ -803,6 +796,12 @@ static
> >>>> int dp_init_sub_modules(struct dp_display_private *dp)
> >>>>                  goto error;
> >>>>          }
> >>>>
> >>>> +       rc = dp_power_client_init(dp->power);
> >>>> +       if (rc) {
> >>>> +               DRM_ERROR("Power client create failed\n");
> >>>> +               goto error;
> >>>> +       }
> >>>> +
> >>>>          dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
> >>>>          if (IS_ERR(dp->aux)) {
> >>>>                  rc = PTR_ERR(dp->aux); @@ -1338,12 +1337,29 @@ static
> >>>> int dp_display_probe(struct platform_device *pdev)
> >>>>
> >>>>          platform_set_drvdata(pdev, &dp->dp_display);
> >>>>
> >>>> +       if (dp->dp_display.is_edp) {
> >>>> +               dp_display_host_init(dp);
> >>>> +               dp_display_host_phy_init(dp);
> >>>> +               dp_catalog_ctrl_hpd_config(dp->catalog);
> >>>> +
> >>>> +               rc = devm_of_dp_aux_populate_bus(dp->aux, NULL);
> >>>
> >>> You should pass a real done_probing handler here, if you are going to
> >refactor
> >>> this piece of code. The done_probing should then shut down the
> >resources
> >>> and bind the component.
> >>>
> >>
> >> I removed the resource enabling part from probe in the next patch where I
> >implemented pm_runtime_ops.
> >> I moved the host_init, phy_init and hpd_config into runtime_resume and
> >calling pm_runtime_get_sync from aux_transfer.
> >
> >Next patch doesn't exist at this point. So, please, either reorder them,
> >or make this patch correct per se.
> >
> >Also, the key part is a call to component_add(). It should come from
> >done_probing callback. AUX bus probing is asynchronous. The kernel
> >registers a device and then it might take some time for the correct
> >driver to be loaded, etc. We clearly expect dp_display_bind to be used
> >only when the panel has been probed.
> >
>
>
> Okay, will add the done_probe function

Thank you.

>
> >>
> >>>> +
> >>>> +               dp_display_host_phy_exit(dp);
> >>>> +               dp_display_host_deinit(dp);
> >>>> +
> >>>> +               if (rc) {
> >>>> +                       DRM_ERROR("failed to initialize panel, rc = %d\n", rc);
> >>>> +                       goto error;
> >>>> +               }
> >>>> +       }
> >>>> +
> >>>>          rc = component_add(&pdev->dev, &dp_display_comp_ops);
> >>>>          if (rc) {
> >>>>                  DRM_ERROR("component add failed, rc=%d\n", rc);
> >>>>                  dp_display_deinit_sub_modules(dp);
> >>>>          }
> >>>>
> >>>> +error:
> >>>>          return rc;
> >>>>   }
> >>>>
> >>>> @@ -1546,40 +1562,8 @@ static int dp_display_get_next_bridge(struct
> >>>> msm_dp *dp)  {
> >>>>          int rc;
> >>>>          struct dp_display_private *dp_priv;
> >>>> -       struct device_node *aux_bus;
> >>>> -       struct device *dev;
> >>>>
> >>>>          dp_priv = container_of(dp, struct dp_display_private, dp_display);
> >>>> -       dev = &dp_priv->pdev->dev;
> >>>> -       aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
> >>>> -
> >>>> -       if (aux_bus && dp->is_edp) {
> >>>> -               dp_display_host_init(dp_priv);
> >>>> -               dp_catalog_ctrl_hpd_config(dp_priv->catalog);
> >>>> -               dp_display_host_phy_init(dp_priv);
> >>>> -               enable_irq(dp_priv->irq);
> >>>> -
> >>>> -               /*
> >>>> -                * The code below assumes that the panel will finish probing
> >>>> -                * by the time devm_of_dp_aux_populate_ep_devices()
> >returns.
> >>>> -                * This isn't a great assumption since it will fail if the
> >>>> -                * panel driver is probed asynchronously but is the best we
> >>>> -                * can do without a bigger driver reorganization.
> >>>> -                */
> >>>> -               rc = of_dp_aux_populate_bus(dp_priv->aux, NULL);
> >>>> -               of_node_put(aux_bus);
> >>>> -               if (rc)
> >>>> -                       goto error;
> >>>> -
> >>>> -               rc = devm_add_action_or_reset(dp->drm_dev->dev,
> >>>> -                                               of_dp_aux_depopulate_bus_void,
> >>>> -                                               dp_priv->aux);
> >>>> -               if (rc)
> >>>> -                       goto error;
> >>>> -       } else if (dp->is_edp) {
> >>>> -               DRM_ERROR("eDP aux_bus not found\n");
> >>>> -               return -ENODEV;
> >>>> -       }
> >>>>
> >>>>          /*
> >>>>           * External bridges are mandatory for eDP interfaces: one has
> >>>> to @@ -1597,12 +1581,6 @@ static int dp_display_get_next_bridge(struct
> >>> msm_dp *dp)
> >>>>                  return 0;
> >>>>          }
> >>>>
> >>>> -error:
> >>>> -       if (dp->is_edp) {
> >>>> -               disable_irq(dp_priv->irq);
> >>>> -               dp_display_host_phy_exit(dp_priv);
> >>>> -               dp_display_host_deinit(dp_priv);
> >>>> -       }
> >>>>          return rc;
> >>>>   }
> >>>>
> >>>> --
> >>>> 2.39.0
> >>>>
> >>>
> >>>
> >>> --
> >>> With best wishes
> >>> Dmitry
> >
> >--
> >With best wishes
> >Dmitry
>


-- 
With best wishes
Dmitry

Re: [RFC PATCH 1/2] drm/msm/dp: enumerate edp panel during driver probe

[Dmitry Baryshkov]

On Tue, 14 Mar 2023 at 12:23, Sankeerth Billakanti (QUIC)
<quic_sbillaka@quicinc.com> wrote:
>
> Hi Dmitry,
>
> >>>>
> >>>> The eDP panel is identified and enumerated during probe of the
> >>>> panel-edp driver. The current DP driver triggers this panel-edp
> >>>> driver probe while getting the panel-bridge associated with the eDP
> >>>> panel from the platform driver bind. If the panel-edp probe is
> >>>> deferred, then the whole bunch of MDSS parent and child drivers have
> >>>> to defer and
> >>> roll back.
> >>>
> >>> No, MDSS has not been rolled back since 5.19. Please shift your
> >>> development on top of the current msm-next.
> >>>
> >>
> >> Okay, I will move to the msm-next tip.
> >>
> >>>>
> >>>> So, we want to move the panel enumeration from bind to probe so that
> >>>> the probe defer is easier to handle and also both the drivers appear
> >>>> consistent in panel enumeration. This change moves the DP driver
> >>>> panel-bridge enumeration to probe.
> >>>>
> >>>> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> >>>> ---
> >>>>   drivers/gpu/drm/msm/dp/dp_aux.c     | 149
> >>> ++++++++++++++++++++++++++--
> >>>>   drivers/gpu/drm/msm/dp/dp_catalog.c |  12 +++
> >>>>   drivers/gpu/drm/msm/dp/dp_catalog.h |   1 +
> >>>>   drivers/gpu/drm/msm/dp/dp_display.c |  80 ++++++---------
> >>>>   4 files changed, 182 insertions(+), 60 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
> >>>> b/drivers/gpu/drm/msm/dp/dp_aux.c index
> >cc3efed593aa..5da95dfdeede
> >>>> 100644
> >>>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> >>>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> >>>> @@ -110,7 +110,7 @@ static ssize_t dp_aux_write(struct
> >>>> dp_aux_private *aux,  }
> >>>>
> >>>>   static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
> >>>> -                             struct drm_dp_aux_msg *msg)
> >>>> +                             struct drm_dp_aux_msg *msg, bool poll)
> >>>
> >>> What is the reason for working in polled mode rather than always
> >>> using the interrupts?
> >>>
> >>
> >> The mdss interrupts will be enabled and can be used after msm_irq_install
> >from msm_drm_bind.
> >> We want to perform aux transactions during probe. The aux data
> >> transmission is followed by an interrupt to indicate successful transmission
> >status and reply information.
> >
> >This is the price of developing on, let me guess, 5.15. Nowadays MDSS
> >interrupts are enabled and can be used during dp_display_probe() and
> >dp_display_bind(). If they can not for some reason, this is an issue that must
> >be fixed. Please reconsider your approach after rebasing onto msm-next or
> >6.3-rc1.
> >
>
> On the msm-next also, I see the mdss irq is enabled from bind (in msm_drv.c).
> msm_drm_bind -> msm_drm_init -> msm_irq_install

MDSS IRQ domain is registered from mdss_probe() -> msm_mdss_init() ->
_msm_mdss_irq_domain_add().

Please note the difference between MDSS IRQs (handled by MDSS) vs
MDP/DPU irq (registered by msm_irq_install()).

> Currently, interrupts will not be available during the dp_display_probe.

I can not agree with you here.

> Is there a patch series which is enabling IRQs during mdss probe?

https://patchwork.freedesktop.org/series/98525/, merged for 5.19.

>
> >>
> >> As interrupts are not enabled, I took this polling approach for aux interrupts
> >during probe.
> >>
> >>>>   {
> >>>>          ssize_t ret;
> >>>>          unsigned long time_left;
> >>>> @@ -121,10 +121,16 @@ static ssize_t dp_aux_cmd_fifo_tx(struct
> >>> dp_aux_private *aux,
> >>>>          if (ret < 0)
> >>>>                  return ret;
> >>>>
> >>>> -       time_left = wait_for_completion_timeout(&aux->comp,
> >>>> +       if (!poll) {
> >>>> +               time_left = wait_for_completion_timeout(&aux->comp,
> >>>>                                                  msecs_to_jiffies(250));
> >>>> -       if (!time_left)
> >>>> -               return -ETIMEDOUT;
> >>>> +               if (!time_left)
> >>>> +                       return -ETIMEDOUT;
> >>>> +       } else {
> >>>> +               ret = dp_catalog_aux_poll_and_get_hw_interrupt(aux-
> >>catalog);
> >>>> +               if (!ret)
> >>>> +                       dp_aux_isr(&aux->dp_aux);
> >>>> +       }
> >>>>
> >>>>          return ret;
> >>>>   }
> >>>> @@ -238,7 +244,7 @@ static void
> >>> dp_aux_update_offset_and_segment(struct dp_aux_private *aux,
> >>>>    */
> >>>>   static void dp_aux_transfer_helper(struct dp_aux_private *aux,
> >>>>                                     struct drm_dp_aux_msg *input_msg,
> >>>> -                                  bool send_seg)
> >>>> +                                  bool send_seg, bool poll)
> >>>>   {
> >>>>          struct drm_dp_aux_msg helper_msg;
> >>>>          u32 message_size = 0x10;
> >>>> @@ -278,7 +284,7 @@ static void dp_aux_transfer_helper(struct
> >>> dp_aux_private *aux,
> >>>>                  helper_msg.address = segment_address;
> >>>>                  helper_msg.buffer = &aux->segment;
> >>>>                  helper_msg.size = 1;
> >>>> -               dp_aux_cmd_fifo_tx(aux, &helper_msg);
> >>>> +               dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
> >>>>          }
> >>>>
> >>>>          /*
> >>>> @@ -292,7 +298,7 @@ static void dp_aux_transfer_helper(struct
> >>> dp_aux_private *aux,
> >>>>          helper_msg.address = input_msg->address;
> >>>>          helper_msg.buffer = &aux->offset;
> >>>>          helper_msg.size = 1;
> >>>> -       dp_aux_cmd_fifo_tx(aux, &helper_msg);
> >>>> +       dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
> >>>>
> >>>>   end:
> >>>>          aux->offset += message_size; @@ -300,6 +306,122 @@ static
> >>>> void dp_aux_transfer_helper(struct
> >>> dp_aux_private *aux,
> >>>>                  aux->segment = 0x0; /* reset segment at end of
> >>>> block */  }
> >>>>
> >>>> +/*
> >>>> + * This function does the real job to process an AUX transaction.
> >>>> + * It will call aux_reset() function to reset the AUX channel,
> >>>> + * if the waiting is timeout.
> >>>> + */
> >>>> +static ssize_t dp_aux_transfer_init(struct drm_dp_aux *dp_aux,
> >>>> +                              struct drm_dp_aux_msg *msg) {
> >>>> +       ssize_t ret;
> >>>> +       int const aux_cmd_native_max = 16;
> >>>> +       int const aux_cmd_i2c_max = 128;
> >>>> +       struct dp_aux_private *aux;
> >>>> +
> >>>> +       aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
> >>>> +
> >>>> +       aux->native = msg->request & (DP_AUX_NATIVE_WRITE &
> >>>> + DP_AUX_NATIVE_READ);
> >>>> +
> >>>> +       /* Ignore address only message */
> >>>> +       if (msg->size == 0 || !msg->buffer) {
> >>>> +               msg->reply = aux->native ?
> >>>> +                       DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
> >>>> +               return msg->size;
> >>>> +       }
> >>>> +
> >>>> +       /* msg sanity check */
> >>>> +       if ((aux->native && msg->size > aux_cmd_native_max) ||
> >>>> +           msg->size > aux_cmd_i2c_max) {
> >>>> +               DRM_ERROR("%s: invalid msg: size(%zu), request(%x)\n",
> >>>> +                       __func__, msg->size, msg->request);
> >>>> +               return -EINVAL;
> >>>> +       }
> >>>> +
> >>>> +       mutex_lock(&aux->mutex);
> >>>> +       if (!aux->initted) {
> >>>> +               ret = -EIO;
> >>>> +               goto exit;
> >>>> +       }
> >>>> +
> >>>> +       /*
> >>>> +        * For eDP it's important to give a reasonably long wait here for HPD
> >>>> +        * to be asserted. This is because the panel driver may have _just_
> >>>> +        * turned on the panel and then tried to do an AUX transfer. The
> >panel
> >>>> +        * driver has no way of knowing when the panel is ready, so it's up
> >>>> +        * to us to wait. For DP we never get into this situation so let's
> >>>> +        * avoid ever doing the extra long wait for DP.
> >>>> +        */
> >>>> +       if (aux->is_edp) {
> >>>> +               ret = dp_catalog_aux_wait_for_hpd_connect_state(aux-
> >>catalog);
> >>>> +               if (ret) {
> >>>> +                       DRM_DEBUG_DP("Panel not ready for aux transactions\n");
> >>>> +                       goto exit;
> >>>> +               }
> >>>> +       }
> >>>> +
> >>>> +       dp_aux_update_offset_and_segment(aux, msg);
> >>>> +       dp_aux_transfer_helper(aux, msg, true, true);
> >>>> +
> >>>> +       aux->read = msg->request & (DP_AUX_I2C_READ &
> >>> DP_AUX_NATIVE_READ);
> >>>> +       aux->cmd_busy = true;
> >>>> +
> >>>> +       if (aux->read) {
> >>>> +               aux->no_send_addr = true;
> >>>> +               aux->no_send_stop = false;
> >>>> +       } else {
> >>>> +               aux->no_send_addr = true;
> >>>> +               aux->no_send_stop = true;
> >>>> +       }
> >>>> +
> >>>> +       ret = dp_aux_cmd_fifo_tx(aux, msg, true);
> >>>> +
> >>>> +       /* TODO: why is fifo_rx necessary here?
> >>>> +        * Ideally fifo_rx need not be executed for an aux write
> >>>> +        */
> >>>> +       ret = dp_aux_cmd_fifo_rx(aux, msg);
> >>>> +
> >>>> +       if (ret < 0) {
> >>>> +               if (aux->native) {
> >>>> +                       aux->retry_cnt++;
> >>>> +                       if (!(aux->retry_cnt % MAX_AUX_RETRIES))
> >>>> +                               dp_catalog_aux_update_cfg(aux->catalog);
> >>>> +               }
> >>>> +               /* reset aux if link is in connected state */
> >>>> +               if (dp_catalog_link_is_connected(aux->catalog))
> >>>> +                       dp_catalog_aux_reset(aux->catalog);
> >>>> +       } else {
> >>>> +               aux->retry_cnt = 0;
> >>>> +               switch (aux->aux_error_num) {
> >>>> +               case DP_AUX_ERR_NONE:
> >>>> +                       if (aux->read)
> >>>> +                               ret = dp_aux_cmd_fifo_rx(aux, msg);
> >>>> +                       msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_ACK :
> >>> DP_AUX_I2C_REPLY_ACK;
> >>>> +                       break;
> >>>> +               case DP_AUX_ERR_DEFER:
> >>>> +                       msg->reply = aux->native ?
> >>>> +                               DP_AUX_NATIVE_REPLY_DEFER :
> >>> DP_AUX_I2C_REPLY_DEFER;
> >>>> +                       break;
> >>>> +               case DP_AUX_ERR_PHY:
> >>>> +               case DP_AUX_ERR_ADDR:
> >>>> +               case DP_AUX_ERR_NACK:
> >>>> +               case DP_AUX_ERR_NACK_DEFER:
> >>>> +                       msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_NACK :
> >>> DP_AUX_I2C_REPLY_NACK;
> >>>> +                       break;
> >>>> +               case DP_AUX_ERR_TOUT:
> >>>> +                       ret = -ETIMEDOUT;
> >>>> +                       break;
> >>>> +               }
> >>>> +       }
> >>>> +
> >>>> +       aux->cmd_busy = false;
> >>>> +
> >>>> +exit:
> >>>> +       mutex_unlock(&aux->mutex);
> >>>> +
> >>>> +       return ret;
> >>>> +}
> >>>> +
> >>>>   /*
> >>>>    * This function does the real job to process an AUX transaction.
> >>>>    * It will call aux_reset() function to reset the AUX channel, @@
> >>>> -355,7 +477,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
> >>> *dp_aux,
> >>>>          }
> >>>>
> >>>>          dp_aux_update_offset_and_segment(aux, msg);
> >>>> -       dp_aux_transfer_helper(aux, msg, true);
> >>>> +       dp_aux_transfer_helper(aux, msg, true, false);
> >>>>
> >>>>          aux->read = msg->request & (DP_AUX_I2C_READ &
> >>> DP_AUX_NATIVE_READ);
> >>>>          aux->cmd_busy = true;
> >>>> @@ -368,7 +490,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
> >>> *dp_aux,
> >>>>                  aux->no_send_stop = true;
> >>>>          }
> >>>>
> >>>> -       ret = dp_aux_cmd_fifo_tx(aux, msg);
> >>>> +       ret = dp_aux_cmd_fifo_tx(aux, msg, false);
> >>>>          if (ret < 0) {
> >>>>                  if (aux->native) {
> >>>>                          aux->retry_cnt++; @@ -535,6 +657,15 @@
> >>>> struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog
> >*catalog,
> >>>>          aux->catalog = catalog;
> >>>>          aux->retry_cnt = 0;
> >>>>
> >>>> +       /*
> >>>> +        * Use the drm_dp_aux_init() to use the aux adapter
> >>>> +        * before registering aux with the DRM device.
> >>>> +        */
> >>>> +       aux->dp_aux.name = "dpu_dp_aux";
> >>>> +       aux->dp_aux.dev = dev;
> >>>> +       aux->dp_aux.transfer = dp_aux_transfer_init;
> >>>> +       drm_dp_aux_init(&aux->dp_aux);
> >>>
> >>> How do you use the aux adapter? It should not be used before
> >>> aux->drm_dev is setup.
> >>>
> >>
> >> The drm_dev registration happens during the bind. But we need to use aux
> >during probe.
> >>
> >> The kernel doc for the drm_dp_aux_init function suggested we can use
> >> the adapter before drm_dev registration. So, I used this function to enable
> >the aux usage during probe.
> >
> >Then please also change __drm_printk() to use (drm) ? (drm->dev) : NULL as
> >a first argument to dev_##level##type. Otherwise the first AUX transfer error
> >before aux->drm_dev is set will oops the kernel. See how
> >drm_err() is expanded.
> >
>
> Okay, will add this also.

Good.

>
> >>
> >> /**
> >>   * drm_dp_aux_init() - minimally initialise an aux channel
> >>   * @aux: DisplayPort AUX channel
> >>   *
> >>   * If you need to use the drm_dp_aux's i2c adapter prior to registering it
> >with
> >>   * the outside world, call drm_dp_aux_init() first.
> >>
> >>>> +
> >>>>          return &aux->dp_aux;
> >>>>   }
> >>>>
> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>>> index 676279d0ca8d..ccf0400176f0 100644
> >>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> >>>> @@ -258,6 +258,18 @@ int
> >>> dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog
> >>> *dp_catalog)
> >>>>                                  2000, 500000);  }
> >>>>
> >>>> +int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog
> >>>> +*dp_catalog) {
> >>>> +       u32 aux;
> >>>> +       struct dp_catalog_private *catalog = container_of(dp_catalog,
> >>>> +                               struct dp_catalog_private,
> >>>> +dp_catalog);
> >>>> +
> >>>> +       return readl_poll_timeout(catalog->io->dp_controller.ahb.base +
> >>>> +                               REG_DP_INTR_STATUS,
> >>>> +                               aux, aux & DP_INTERRUPT_STATUS1,
> >>>> +                               250, 250000); }
> >>>> +
> >>>>   static void dump_regs(void __iomem *base, int len)  {
> >>>>          int i;
> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h
> >>>> b/drivers/gpu/drm/msm/dp/dp_catalog.h
> >>>> index 1f717f45c115..ad4a9e0f71f2 100644
> >>>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
> >>>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
> >>>> @@ -87,6 +87,7 @@ void dp_catalog_aux_enable(struct dp_catalog
> >>>> *dp_catalog, bool enable);  void dp_catalog_aux_update_cfg(struct
> >>>> dp_catalog *dp_catalog);  int
> >>>> dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog
> >>>> *dp_catalog);
> >>>>   u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
> >>>> +int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog
> >>>> +*dp_catalog);
> >>>>
> >>>>   /* DP Controller APIs */
> >>>>   void dp_catalog_ctrl_state_ctrl(struct dp_catalog *dp_catalog, u32
> >>>> state); diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> b/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> index bde1a7ce442f..a5dcef040b74 100644
> >>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> @@ -275,13 +275,6 @@ static int dp_display_bind(struct device *dev,
> >struct
> >>> device *master,
> >>>>          dp->dp_display.drm_dev = drm;
> >>>>          priv->dp[dp->id] = &dp->dp_display;
> >>>>
> >>>> -       rc = dp->parser->parse(dp->parser);
> >>>> -       if (rc) {
> >>>> -               DRM_ERROR("device tree parsing failed\n");
> >>>> -               goto end;
> >>>> -       }
> >>>> -
> >>>> -
> >>>>          dp->drm_dev = drm;
> >>>>          dp->aux->drm_dev = drm;
> >>>>          rc = dp_aux_register(dp->aux); @@ -290,12 +283,6 @@ static int
> >>>> dp_display_bind(struct device *dev, struct device *master,
> >>>>                  goto end;
> >>>>          }
> >>>>
> >>>> -       rc = dp_power_client_init(dp->power);
> >>>> -       if (rc) {
> >>>> -               DRM_ERROR("Power client create failed\n");
> >>>> -               goto end;
> >>>> -       }
> >>>> -
> >>>>          rc = dp_register_audio_driver(dev, dp->audio);
> >>>>          if (rc) {
> >>>>                  DRM_ERROR("Audio registration Dp failed\n"); @@ -787,6
> >>>> +774,12 @@ static int dp_init_sub_modules(struct dp_display_private
> >*dp)
> >>>>                  goto error;
> >>>>          }
> >>>>
> >>>> +       rc = dp->parser->parse(dp->parser);
> >>>> +       if (rc) {
> >>>> +               DRM_ERROR("device tree parsing failed\n");
> >>>> +               goto error;
> >>>> +       }
> >>>> +
> >>>>          dp->catalog = dp_catalog_get(dev, &dp->parser->io);
> >>>>          if (IS_ERR(dp->catalog)) {
> >>>>                  rc = PTR_ERR(dp->catalog); @@ -803,6 +796,12 @@ static
> >>>> int dp_init_sub_modules(struct dp_display_private *dp)
> >>>>                  goto error;
> >>>>          }
> >>>>
> >>>> +       rc = dp_power_client_init(dp->power);
> >>>> +       if (rc) {
> >>>> +               DRM_ERROR("Power client create failed\n");
> >>>> +               goto error;
> >>>> +       }
> >>>> +
> >>>>          dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
> >>>>          if (IS_ERR(dp->aux)) {
> >>>>                  rc = PTR_ERR(dp->aux); @@ -1338,12 +1337,29 @@ static
> >>>> int dp_display_probe(struct platform_device *pdev)
> >>>>
> >>>>          platform_set_drvdata(pdev, &dp->dp_display);
> >>>>
> >>>> +       if (dp->dp_display.is_edp) {
> >>>> +               dp_display_host_init(dp);
> >>>> +               dp_display_host_phy_init(dp);
> >>>> +               dp_catalog_ctrl_hpd_config(dp->catalog);
> >>>> +
> >>>> +               rc = devm_of_dp_aux_populate_bus(dp->aux, NULL);
> >>>
> >>> You should pass a real done_probing handler here, if you are going to
> >refactor
> >>> this piece of code. The done_probing should then shut down the
> >resources
> >>> and bind the component.
> >>>
> >>
> >> I removed the resource enabling part from probe in the next patch where I
> >implemented pm_runtime_ops.
> >> I moved the host_init, phy_init and hpd_config into runtime_resume and
> >calling pm_runtime_get_sync from aux_transfer.
> >
> >Next patch doesn't exist at this point. So, please, either reorder them,
> >or make this patch correct per se.
> >
> >Also, the key part is a call to component_add(). It should come from
> >done_probing callback. AUX bus probing is asynchronous. The kernel
> >registers a device and then it might take some time for the correct
> >driver to be loaded, etc. We clearly expect dp_display_bind to be used
> >only when the panel has been probed.
> >
>
>
> Okay, will add the done_probe function

Thank you.

>
> >>
> >>>> +
> >>>> +               dp_display_host_phy_exit(dp);
> >>>> +               dp_display_host_deinit(dp);
> >>>> +
> >>>> +               if (rc) {
> >>>> +                       DRM_ERROR("failed to initialize panel, rc = %d\n", rc);
> >>>> +                       goto error;
> >>>> +               }
> >>>> +       }
> >>>> +
> >>>>          rc = component_add(&pdev->dev, &dp_display_comp_ops);
> >>>>          if (rc) {
> >>>>                  DRM_ERROR("component add failed, rc=%d\n", rc);
> >>>>                  dp_display_deinit_sub_modules(dp);
> >>>>          }
> >>>>
> >>>> +error:
> >>>>          return rc;
> >>>>   }
> >>>>
> >>>> @@ -1546,40 +1562,8 @@ static int dp_display_get_next_bridge(struct
> >>>> msm_dp *dp)  {
> >>>>          int rc;
> >>>>          struct dp_display_private *dp_priv;
> >>>> -       struct device_node *aux_bus;
> >>>> -       struct device *dev;
> >>>>
> >>>>          dp_priv = container_of(dp, struct dp_display_private, dp_display);
> >>>> -       dev = &dp_priv->pdev->dev;
> >>>> -       aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
> >>>> -
> >>>> -       if (aux_bus && dp->is_edp) {
> >>>> -               dp_display_host_init(dp_priv);
> >>>> -               dp_catalog_ctrl_hpd_config(dp_priv->catalog);
> >>>> -               dp_display_host_phy_init(dp_priv);
> >>>> -               enable_irq(dp_priv->irq);
> >>>> -
> >>>> -               /*
> >>>> -                * The code below assumes that the panel will finish probing
> >>>> -                * by the time devm_of_dp_aux_populate_ep_devices()
> >returns.
> >>>> -                * This isn't a great assumption since it will fail if the
> >>>> -                * panel driver is probed asynchronously but is the best we
> >>>> -                * can do without a bigger driver reorganization.
> >>>> -                */
> >>>> -               rc = of_dp_aux_populate_bus(dp_priv->aux, NULL);
> >>>> -               of_node_put(aux_bus);
> >>>> -               if (rc)
> >>>> -                       goto error;
> >>>> -
> >>>> -               rc = devm_add_action_or_reset(dp->drm_dev->dev,
> >>>> -                                               of_dp_aux_depopulate_bus_void,
> >>>> -                                               dp_priv->aux);
> >>>> -               if (rc)
> >>>> -                       goto error;
> >>>> -       } else if (dp->is_edp) {
> >>>> -               DRM_ERROR("eDP aux_bus not found\n");
> >>>> -               return -ENODEV;
> >>>> -       }
> >>>>
> >>>>          /*
> >>>>           * External bridges are mandatory for eDP interfaces: one has
> >>>> to @@ -1597,12 +1581,6 @@ static int dp_display_get_next_bridge(struct
> >>> msm_dp *dp)
> >>>>                  return 0;
> >>>>          }
> >>>>
> >>>> -error:
> >>>> -       if (dp->is_edp) {
> >>>> -               disable_irq(dp_priv->irq);
> >>>> -               dp_display_host_phy_exit(dp_priv);
> >>>> -               dp_display_host_deinit(dp_priv);
> >>>> -       }
> >>>>          return rc;
> >>>>   }
> >>>>
> >>>> --
> >>>> 2.39.0
> >>>>
> >>>
> >>>
> >>> --
> >>> With best wishes
> >>> Dmitry
> >
> >--
> >With best wishes
> >Dmitry
>


-- 
With best wishes
Dmitry

[RFC PATCH 2/2] drm/msm/dp: enable pm_runtime support for dp driver

[Sankeerth Billakanti]

The current DP driver directly enables or disables the necessary control
resources based on code flow. This could disable a required resource that
is needed in a different usecase. It can also lead to excessive voting of
a resource and may increase power consumption.

The pm_runtime framework can solve this problem in DP driver by monitoring
the resource enable disable calls. This change will enable support for the
pm_runtime resume and suspend operations for DP driver.

Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_aux.c     |   6 ++
 drivers/gpu/drm/msm/dp/dp_display.c | 121 ++++++++++++++++------------
 drivers/gpu/drm/msm/dp/dp_power.c   |   7 --
 3 files changed, 76 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 5da95dfdeede..45026827bf7a 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -338,6 +338,7 @@ static ssize_t dp_aux_transfer_init(struct drm_dp_aux *dp_aux,
 		return -EINVAL;
 	}
 
+	pm_runtime_get_sync(dp_aux->dev);
 	mutex_lock(&aux->mutex);
 	if (!aux->initted) {
 		ret = -EIO;
@@ -418,6 +419,8 @@ static ssize_t dp_aux_transfer_init(struct drm_dp_aux *dp_aux,
 
 exit:
 	mutex_unlock(&aux->mutex);
+	pm_runtime_mark_last_busy(dp_aux->dev);
+	pm_runtime_put_autosuspend(dp_aux->dev);
 
 	return ret;
 }
@@ -454,6 +457,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
 		return -EINVAL;
 	}
 
+	pm_runtime_get_sync(dp_aux->dev);
 	mutex_lock(&aux->mutex);
 	if (!aux->initted) {
 		ret = -EIO;
@@ -527,6 +531,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
 
 exit:
 	mutex_unlock(&aux->mutex);
+	pm_runtime_mark_last_busy(dp_aux->dev);
+	pm_runtime_put_autosuspend(dp_aux->dev);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index a5dcef040b74..a23e79e43100 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -307,8 +307,10 @@ static void dp_display_unbind(struct device *dev, struct device *master,
 	struct msm_drm_private *priv = dev_get_drvdata(master);
 
 	/* disable all HPD interrupts */
-	if (dp->core_initialized)
+	if (dp->core_initialized) {
 		dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, false);
+		pm_runtime_put_sync(dev);
+	}
 
 	kthread_stop(dp->ev_tsk);
 
@@ -1083,26 +1085,6 @@ void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp)
 	mutex_unlock(&dp_display->event_mutex);
 }
 
-static void dp_display_config_hpd(struct dp_display_private *dp)
-{
-
-	dp_display_host_init(dp);
-	dp_catalog_ctrl_hpd_config(dp->catalog);
-
-	/* Enable plug and unplug interrupts only if requested */
-	if (dp->dp_display.internal_hpd)
-		dp_catalog_hpd_config_intr(dp->catalog,
-				DP_DP_HPD_PLUG_INT_MASK |
-				DP_DP_HPD_UNPLUG_INT_MASK,
-				true);
-
-	/* Enable interrupt first time
-	 * we are leaving dp clocks on during disconnect
-	 * and never disable interrupt
-	 */
-	enable_irq(dp->irq);
-}
-
 static int hpd_event_thread(void *data)
 {
 	struct dp_display_private *dp_priv;
@@ -1163,9 +1145,6 @@ static int hpd_event_thread(void *data)
 		spin_unlock_irqrestore(&dp_priv->event_lock, flag);
 
 		switch (todo->event_id) {
-		case EV_HPD_INIT_SETUP:
-			dp_display_config_hpd(dp_priv);
-			break;
 		case EV_HPD_PLUG_INT:
 			dp_hpd_plug_handle(dp_priv, todo->data);
 			break;
@@ -1337,16 +1316,12 @@ static int dp_display_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, &dp->dp_display);
 
-	if (dp->dp_display.is_edp) {
-		dp_display_host_init(dp);
-		dp_display_host_phy_init(dp);
-		dp_catalog_ctrl_hpd_config(dp->catalog);
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+	pm_runtime_use_autosuspend(&pdev->dev);
 
+	if (dp->dp_display.is_edp) {
 		rc = devm_of_dp_aux_populate_bus(dp->aux, NULL);
-
-		dp_display_host_phy_exit(dp);
-		dp_display_host_deinit(dp);
-
 		if (rc) {
 			DRM_ERROR("failed to initialize panel, rc = %d\n", rc);
 			goto error;
@@ -1367,6 +1342,8 @@ static int dp_display_remove(struct platform_device *pdev)
 {
 	struct dp_display_private *dp = dev_get_dp_display_private(&pdev->dev);
 
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 	dp_display_deinit_sub_modules(dp);
 
 	component_del(&pdev->dev, &dp_display_comp_ops);
@@ -1375,6 +1352,42 @@ static int dp_display_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int dp_runtime_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct msm_dp *dp_display = platform_get_drvdata(pdev);
+	struct dp_display_private *dp;
+
+	dp = container_of(dp_display, struct dp_display_private, dp_display);
+	dp_display_host_phy_exit(dp);
+	dp_display_host_deinit(dp);
+
+	return 0;
+}
+
+static int dp_runtime_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct msm_dp *dp_display = platform_get_drvdata(pdev);
+	struct dp_display_private *dp;
+
+	dp = container_of(dp_display, struct dp_display_private, dp_display);
+	dp_display_host_init(dp);
+
+	if (dp->dp_display.is_edp) {
+		dp_display_host_phy_init(dp);
+	} else {
+		dp_catalog_hpd_config_intr(dp->catalog,
+				DP_DP_HPD_PLUG_INT_MASK |
+				DP_DP_HPD_UNPLUG_INT_MASK,
+				true);
+	}
+
+	dp_catalog_ctrl_hpd_config(dp->catalog);
+
+	return 0;
+}
+
 static int dp_pm_resume(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
@@ -1384,6 +1397,9 @@ static int dp_pm_resume(struct device *dev)
 
 	dp = container_of(dp_display, struct dp_display_private, dp_display);
 
+	if (pm_runtime_suspended(dev))
+		return 0;
+
 	mutex_lock(&dp->event_mutex);
 
 	drm_dbg_dp(dp->drm_dev,
@@ -1394,16 +1410,7 @@ static int dp_pm_resume(struct device *dev)
 	/* start from disconnected state */
 	dp->hpd_state = ST_DISCONNECTED;
 
-	/* turn on dp ctrl/phy */
-	dp_display_host_init(dp);
-
-	dp_catalog_ctrl_hpd_config(dp->catalog);
-
-	if (dp->dp_display.internal_hpd)
-		dp_catalog_hpd_config_intr(dp->catalog,
-				DP_DP_HPD_PLUG_INT_MASK |
-				DP_DP_HPD_UNPLUG_INT_MASK,
-				true);
+	dp_runtime_resume(dev);
 
 	if (dp_catalog_link_is_connected(dp->catalog)) {
 		/*
@@ -1452,27 +1459,29 @@ static int dp_pm_suspend(struct device *dev)
 
 	dp = container_of(dp_display, struct dp_display_private, dp_display);
 
+	if (pm_runtime_suspended(dev))
+		return 0;
+
 	mutex_lock(&dp->event_mutex);
 
 	drm_dbg_dp(dp->drm_dev,
-		"Before, type=%d core_inited=%d  phy_inited=%d power_on=%d\n",
-		dp->dp_display.connector_type, dp->core_initialized,
+		"Before, type=%d sink=%d conn=%d core_inited=%d  phy_inited=%d power_on=%d\n",
+		dp->dp_display.connector_type, dp->link->sink_count,
+		dp->dp_display.is_connected, dp->core_initialized,
 		dp->phy_initialized, dp_display->power_on);
 
 	/* mainlink enabled */
 	if (dp_power_clk_status(dp->power, DP_CTRL_PM))
 		dp_ctrl_off_link_stream(dp->ctrl);
 
-	dp_display_host_phy_exit(dp);
-
-	/* host_init will be called at pm_resume */
-	dp_display_host_deinit(dp);
+	dp_runtime_suspend(dev);
 
 	dp->hpd_state = ST_SUSPENDED;
 
 	drm_dbg_dp(dp->drm_dev,
-		"After, type=%d core_inited=%d phy_inited=%d power_on=%d\n",
-		dp->dp_display.connector_type, dp->core_initialized,
+		"After, type=%d sink=%d conn=%d core_init=%d phy_init=%d power=%d\n",
+		dp->dp_display.connector_type, dp->link->sink_count,
+		dp->dp_display.is_connected, dp->core_initialized,
 		dp->phy_initialized, dp_display->power_on);
 
 	mutex_unlock(&dp->event_mutex);
@@ -1481,6 +1490,11 @@ static int dp_pm_suspend(struct device *dev)
 }
 
 static const struct dev_pm_ops dp_pm_ops = {
+
+	SET_RUNTIME_PM_OPS(dp_runtime_suspend, dp_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+
 	.suspend = dp_pm_suspend,
 	.resume =  dp_pm_resume,
 };
@@ -1521,8 +1535,11 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
 
 	dp = container_of(dp_display, struct dp_display_private, dp_display);
 
+	/* enable host_init for HPD intr for DP */
 	if (!dp_display->is_edp)
-		dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 0);
+		pm_runtime_get_sync(&dp->pdev->dev);
+
+	enable_irq(dp->irq);
 }
 
 bool msm_dp_wide_bus_available(const struct msm_dp *dp_display)
@@ -1645,6 +1662,7 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
 		return;
 	}
 
+	pm_runtime_get_sync(&dp_display->pdev->dev);
 	if (dp->is_edp)
 		dp_hpd_plug_handle(dp_display, 0);
 
@@ -1728,6 +1746,7 @@ void dp_bridge_post_disable(struct drm_bridge *drm_bridge)
 
 	drm_dbg_dp(dp->drm_dev, "type=%d Done\n", dp->connector_type);
 	mutex_unlock(&dp_display->event_mutex);
+	pm_runtime_put_sync(&dp_display->pdev->dev);
 }
 
 void dp_bridge_mode_set(struct drm_bridge *drm_bridge,
diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
index c0aaabb03389..a736d1b0f02f 100644
--- a/drivers/gpu/drm/msm/dp/dp_power.c
+++ b/drivers/gpu/drm/msm/dp/dp_power.c
@@ -172,8 +172,6 @@ int dp_power_client_init(struct dp_power *dp_power)
 
 	power = container_of(dp_power, struct dp_power_private, dp_power);
 
-	pm_runtime_enable(&power->pdev->dev);
-
 	rc = dp_power_clk_init(power);
 	if (rc)
 		DRM_ERROR("failed to init clocks %d\n", rc);
@@ -192,7 +190,6 @@ void dp_power_client_deinit(struct dp_power *dp_power)
 
 	power = container_of(dp_power, struct dp_power_private, dp_power);
 
-	pm_runtime_disable(&power->pdev->dev);
 }
 
 int dp_power_init(struct dp_power *dp_power, bool flip)
@@ -207,8 +204,6 @@ int dp_power_init(struct dp_power *dp_power, bool flip)
 
 	power = container_of(dp_power, struct dp_power_private, dp_power);
 
-	pm_runtime_get_sync(&power->pdev->dev);
-
 	rc = dp_power_clk_enable(dp_power, DP_CORE_PM, true);
 	if (rc) {
 		DRM_ERROR("failed to enable DP core clocks, %d\n", rc);
@@ -218,7 +213,6 @@ int dp_power_init(struct dp_power *dp_power, bool flip)
 	return 0;
 
 exit:
-	pm_runtime_put_sync(&power->pdev->dev);
 	return rc;
 }
 
@@ -229,7 +223,6 @@ int dp_power_deinit(struct dp_power *dp_power)
 	power = container_of(dp_power, struct dp_power_private, dp_power);
 
 	dp_power_clk_enable(dp_power, DP_CORE_PM, false);
-	pm_runtime_put_sync(&power->pdev->dev);
 	return 0;
 }
 
-- 
2.39.0

[RFC PATCH 2/2] drm/msm/dp: enable pm_runtime support for dp driver

[Sankeerth Billakanti]

The current DP driver directly enables or disables the necessary control
resources based on code flow. This could disable a required resource that
is needed in a different usecase. It can also lead to excessive voting of
a resource and may increase power consumption.

The pm_runtime framework can solve this problem in DP driver by monitoring
the resource enable disable calls. This change will enable support for the
pm_runtime resume and suspend operations for DP driver.

Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_aux.c     |   6 ++
 drivers/gpu/drm/msm/dp/dp_display.c | 121 ++++++++++++++++------------
 drivers/gpu/drm/msm/dp/dp_power.c   |   7 --
 3 files changed, 76 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 5da95dfdeede..45026827bf7a 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -338,6 +338,7 @@ static ssize_t dp_aux_transfer_init(struct drm_dp_aux *dp_aux,
 		return -EINVAL;
 	}
 
+	pm_runtime_get_sync(dp_aux->dev);
 	mutex_lock(&aux->mutex);
 	if (!aux->initted) {
 		ret = -EIO;
@@ -418,6 +419,8 @@ static ssize_t dp_aux_transfer_init(struct drm_dp_aux *dp_aux,
 
 exit:
 	mutex_unlock(&aux->mutex);
+	pm_runtime_mark_last_busy(dp_aux->dev);
+	pm_runtime_put_autosuspend(dp_aux->dev);
 
 	return ret;
 }
@@ -454,6 +457,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
 		return -EINVAL;
 	}
 
+	pm_runtime_get_sync(dp_aux->dev);
 	mutex_lock(&aux->mutex);
 	if (!aux->initted) {
 		ret = -EIO;
@@ -527,6 +531,8 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
 
 exit:
 	mutex_unlock(&aux->mutex);
+	pm_runtime_mark_last_busy(dp_aux->dev);
+	pm_runtime_put_autosuspend(dp_aux->dev);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index a5dcef040b74..a23e79e43100 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -307,8 +307,10 @@ static void dp_display_unbind(struct device *dev, struct device *master,
 	struct msm_drm_private *priv = dev_get_drvdata(master);
 
 	/* disable all HPD interrupts */
-	if (dp->core_initialized)
+	if (dp->core_initialized) {
 		dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, false);
+		pm_runtime_put_sync(dev);
+	}
 
 	kthread_stop(dp->ev_tsk);
 
@@ -1083,26 +1085,6 @@ void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp)
 	mutex_unlock(&dp_display->event_mutex);
 }
 
-static void dp_display_config_hpd(struct dp_display_private *dp)
-{
-
-	dp_display_host_init(dp);
-	dp_catalog_ctrl_hpd_config(dp->catalog);
-
-	/* Enable plug and unplug interrupts only if requested */
-	if (dp->dp_display.internal_hpd)
-		dp_catalog_hpd_config_intr(dp->catalog,
-				DP_DP_HPD_PLUG_INT_MASK |
-				DP_DP_HPD_UNPLUG_INT_MASK,
-				true);
-
-	/* Enable interrupt first time
-	 * we are leaving dp clocks on during disconnect
-	 * and never disable interrupt
-	 */
-	enable_irq(dp->irq);
-}
-
 static int hpd_event_thread(void *data)
 {
 	struct dp_display_private *dp_priv;
@@ -1163,9 +1145,6 @@ static int hpd_event_thread(void *data)
 		spin_unlock_irqrestore(&dp_priv->event_lock, flag);
 
 		switch (todo->event_id) {
-		case EV_HPD_INIT_SETUP:
-			dp_display_config_hpd(dp_priv);
-			break;
 		case EV_HPD_PLUG_INT:
 			dp_hpd_plug_handle(dp_priv, todo->data);
 			break;
@@ -1337,16 +1316,12 @@ static int dp_display_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, &dp->dp_display);
 
-	if (dp->dp_display.is_edp) {
-		dp_display_host_init(dp);
-		dp_display_host_phy_init(dp);
-		dp_catalog_ctrl_hpd_config(dp->catalog);
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+	pm_runtime_use_autosuspend(&pdev->dev);
 
+	if (dp->dp_display.is_edp) {
 		rc = devm_of_dp_aux_populate_bus(dp->aux, NULL);
-
-		dp_display_host_phy_exit(dp);
-		dp_display_host_deinit(dp);
-
 		if (rc) {
 			DRM_ERROR("failed to initialize panel, rc = %d\n", rc);
 			goto error;
@@ -1367,6 +1342,8 @@ static int dp_display_remove(struct platform_device *pdev)
 {
 	struct dp_display_private *dp = dev_get_dp_display_private(&pdev->dev);
 
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 	dp_display_deinit_sub_modules(dp);
 
 	component_del(&pdev->dev, &dp_display_comp_ops);
@@ -1375,6 +1352,42 @@ static int dp_display_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int dp_runtime_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct msm_dp *dp_display = platform_get_drvdata(pdev);
+	struct dp_display_private *dp;
+
+	dp = container_of(dp_display, struct dp_display_private, dp_display);
+	dp_display_host_phy_exit(dp);
+	dp_display_host_deinit(dp);
+
+	return 0;
+}
+
+static int dp_runtime_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct msm_dp *dp_display = platform_get_drvdata(pdev);
+	struct dp_display_private *dp;
+
+	dp = container_of(dp_display, struct dp_display_private, dp_display);
+	dp_display_host_init(dp);
+
+	if (dp->dp_display.is_edp) {
+		dp_display_host_phy_init(dp);
+	} else {
+		dp_catalog_hpd_config_intr(dp->catalog,
+				DP_DP_HPD_PLUG_INT_MASK |
+				DP_DP_HPD_UNPLUG_INT_MASK,
+				true);
+	}
+
+	dp_catalog_ctrl_hpd_config(dp->catalog);
+
+	return 0;
+}
+
 static int dp_pm_resume(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
@@ -1384,6 +1397,9 @@ static int dp_pm_resume(struct device *dev)
 
 	dp = container_of(dp_display, struct dp_display_private, dp_display);
 
+	if (pm_runtime_suspended(dev))
+		return 0;
+
 	mutex_lock(&dp->event_mutex);
 
 	drm_dbg_dp(dp->drm_dev,
@@ -1394,16 +1410,7 @@ static int dp_pm_resume(struct device *dev)
 	/* start from disconnected state */
 	dp->hpd_state = ST_DISCONNECTED;
 
-	/* turn on dp ctrl/phy */
-	dp_display_host_init(dp);
-
-	dp_catalog_ctrl_hpd_config(dp->catalog);
-
-	if (dp->dp_display.internal_hpd)
-		dp_catalog_hpd_config_intr(dp->catalog,
-				DP_DP_HPD_PLUG_INT_MASK |
-				DP_DP_HPD_UNPLUG_INT_MASK,
-				true);
+	dp_runtime_resume(dev);
 
 	if (dp_catalog_link_is_connected(dp->catalog)) {
 		/*
@@ -1452,27 +1459,29 @@ static int dp_pm_suspend(struct device *dev)
 
 	dp = container_of(dp_display, struct dp_display_private, dp_display);
 
+	if (pm_runtime_suspended(dev))
+		return 0;
+
 	mutex_lock(&dp->event_mutex);
 
 	drm_dbg_dp(dp->drm_dev,
-		"Before, type=%d core_inited=%d  phy_inited=%d power_on=%d\n",
-		dp->dp_display.connector_type, dp->core_initialized,
+		"Before, type=%d sink=%d conn=%d core_inited=%d  phy_inited=%d power_on=%d\n",
+		dp->dp_display.connector_type, dp->link->sink_count,
+		dp->dp_display.is_connected, dp->core_initialized,
 		dp->phy_initialized, dp_display->power_on);
 
 	/* mainlink enabled */
 	if (dp_power_clk_status(dp->power, DP_CTRL_PM))
 		dp_ctrl_off_link_stream(dp->ctrl);
 
-	dp_display_host_phy_exit(dp);
-
-	/* host_init will be called at pm_resume */
-	dp_display_host_deinit(dp);
+	dp_runtime_suspend(dev);
 
 	dp->hpd_state = ST_SUSPENDED;
 
 	drm_dbg_dp(dp->drm_dev,
-		"After, type=%d core_inited=%d phy_inited=%d power_on=%d\n",
-		dp->dp_display.connector_type, dp->core_initialized,
+		"After, type=%d sink=%d conn=%d core_init=%d phy_init=%d power=%d\n",
+		dp->dp_display.connector_type, dp->link->sink_count,
+		dp->dp_display.is_connected, dp->core_initialized,
 		dp->phy_initialized, dp_display->power_on);
 
 	mutex_unlock(&dp->event_mutex);
@@ -1481,6 +1490,11 @@ static int dp_pm_suspend(struct device *dev)
 }
 
 static const struct dev_pm_ops dp_pm_ops = {
+
+	SET_RUNTIME_PM_OPS(dp_runtime_suspend, dp_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+
 	.suspend = dp_pm_suspend,
 	.resume =  dp_pm_resume,
 };
@@ -1521,8 +1535,11 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
 
 	dp = container_of(dp_display, struct dp_display_private, dp_display);
 
+	/* enable host_init for HPD intr for DP */
 	if (!dp_display->is_edp)
-		dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 0);
+		pm_runtime_get_sync(&dp->pdev->dev);
+
+	enable_irq(dp->irq);
 }
 
 bool msm_dp_wide_bus_available(const struct msm_dp *dp_display)
@@ -1645,6 +1662,7 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
 		return;
 	}
 
+	pm_runtime_get_sync(&dp_display->pdev->dev);
 	if (dp->is_edp)
 		dp_hpd_plug_handle(dp_display, 0);
 
@@ -1728,6 +1746,7 @@ void dp_bridge_post_disable(struct drm_bridge *drm_bridge)
 
 	drm_dbg_dp(dp->drm_dev, "type=%d Done\n", dp->connector_type);
 	mutex_unlock(&dp_display->event_mutex);
+	pm_runtime_put_sync(&dp_display->pdev->dev);
 }
 
 void dp_bridge_mode_set(struct drm_bridge *drm_bridge,
diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
index c0aaabb03389..a736d1b0f02f 100644
--- a/drivers/gpu/drm/msm/dp/dp_power.c
+++ b/drivers/gpu/drm/msm/dp/dp_power.c
@@ -172,8 +172,6 @@ int dp_power_client_init(struct dp_power *dp_power)
 
 	power = container_of(dp_power, struct dp_power_private, dp_power);
 
-	pm_runtime_enable(&power->pdev->dev);
-
 	rc = dp_power_clk_init(power);
 	if (rc)
 		DRM_ERROR("failed to init clocks %d\n", rc);
@@ -192,7 +190,6 @@ void dp_power_client_deinit(struct dp_power *dp_power)
 
 	power = container_of(dp_power, struct dp_power_private, dp_power);
 
-	pm_runtime_disable(&power->pdev->dev);
 }
 
 int dp_power_init(struct dp_power *dp_power, bool flip)
@@ -207,8 +204,6 @@ int dp_power_init(struct dp_power *dp_power, bool flip)
 
 	power = container_of(dp_power, struct dp_power_private, dp_power);
 
-	pm_runtime_get_sync(&power->pdev->dev);
-
 	rc = dp_power_clk_enable(dp_power, DP_CORE_PM, true);
 	if (rc) {
 		DRM_ERROR("failed to enable DP core clocks, %d\n", rc);
@@ -218,7 +213,6 @@ int dp_power_init(struct dp_power *dp_power, bool flip)
 	return 0;
 
 exit:
-	pm_runtime_put_sync(&power->pdev->dev);
 	return rc;
 }
 
@@ -229,7 +223,6 @@ int dp_power_deinit(struct dp_power *dp_power)
 	power = container_of(dp_power, struct dp_power_private, dp_power);
 
 	dp_power_clk_enable(dp_power, DP_CORE_PM, false);
-	pm_runtime_put_sync(&power->pdev->dev);
 	return 0;
 }
 
-- 
2.39.0

Re: [RFC PATCH 2/2] drm/msm/dp: enable pm_runtime support for dp driver

[kernel test robot]

Hi Sankeerth,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on drm/drm-next]
[also build test WARNING on drm-intel/for-linux-next drm-tip/drm-tip linus/master next-20230223]
[cannot apply to drm-misc/drm-misc-next drm-exynos/exynos-drm-next drm-intel/for-linux-next-fixes v6.2]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sankeerth-Billakanti/drm-msm-dp-enumerate-edp-panel-during-driver-probe/20230223-215810
base:   git://anongit.freedesktop.org/drm/drm drm-next
patch link:    https://lore.kernel.org/r/20230223135635.30659-3-quic_sbillaka%40quicinc.com
patch subject: [RFC PATCH 2/2] drm/msm/dp: enable pm_runtime support for dp driver
config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230224/202302240000.LsLyruJR-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/5351853e6b90c88348c59c9ffc02122ac2e491f8
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Sankeerth-Billakanti/drm-msm-dp-enumerate-edp-panel-during-driver-probe/20230223-215810
        git checkout 5351853e6b90c88348c59c9ffc02122ac2e491f8
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc SHELL=/bin/bash drivers/gpu/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302240000.LsLyruJR-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/msm/dp/dp_power.c: In function 'dp_power_client_deinit':
>> drivers/gpu/drm/msm/dp/dp_power.c:184:34: warning: variable 'power' set but not used [-Wunused-but-set-variable]
     184 |         struct dp_power_private *power;
         |                                  ^~~~~
   drivers/gpu/drm/msm/dp/dp_power.c: In function 'dp_power_init':
   drivers/gpu/drm/msm/dp/dp_power.c:198:34: warning: variable 'power' set but not used [-Wunused-but-set-variable]
     198 |         struct dp_power_private *power = NULL;
         |                                  ^~~~~
   drivers/gpu/drm/msm/dp/dp_power.c: In function 'dp_power_deinit':
   drivers/gpu/drm/msm/dp/dp_power.c:221:34: warning: variable 'power' set but not used [-Wunused-but-set-variable]
     221 |         struct dp_power_private *power;
         |                                  ^~~~~


vim +/power +184 drivers/gpu/drm/msm/dp/dp_power.c

c943b4948b5848 Chandan Uddaraju 2020-08-27  181  
c943b4948b5848 Chandan Uddaraju 2020-08-27  182  void dp_power_client_deinit(struct dp_power *dp_power)
c943b4948b5848 Chandan Uddaraju 2020-08-27  183  {
c943b4948b5848 Chandan Uddaraju 2020-08-27 @184  	struct dp_power_private *power;
c943b4948b5848 Chandan Uddaraju 2020-08-27  185  
c943b4948b5848 Chandan Uddaraju 2020-08-27  186  	if (!dp_power) {
c943b4948b5848 Chandan Uddaraju 2020-08-27  187  		DRM_ERROR("invalid power data\n");
c943b4948b5848 Chandan Uddaraju 2020-08-27  188  		return;
c943b4948b5848 Chandan Uddaraju 2020-08-27  189  	}
c943b4948b5848 Chandan Uddaraju 2020-08-27  190  
c943b4948b5848 Chandan Uddaraju 2020-08-27  191  	power = container_of(dp_power, struct dp_power_private, dp_power);
c943b4948b5848 Chandan Uddaraju 2020-08-27  192  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [RFC PATCH 2/2] drm/msm/dp: enable pm_runtime support for dp driver

[Bjorn Andersson]

On Thu, Feb 23, 2023 at 07:26:35PM +0530, Sankeerth Billakanti wrote:
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
[..]
> +static int dp_runtime_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct msm_dp *dp_display = platform_get_drvdata(pdev);
> +	struct dp_display_private *dp;
> +
> +	dp = container_of(dp_display, struct dp_display_private, dp_display);
> +	dp_display_host_init(dp);
> +
> +	if (dp->dp_display.is_edp) {
> +		dp_display_host_phy_init(dp);
> +	} else {
> +		dp_catalog_hpd_config_intr(dp->catalog,
> +				DP_DP_HPD_PLUG_INT_MASK |
> +				DP_DP_HPD_UNPLUG_INT_MASK,
> +				true);

I believe this is backwards.

Only in the event that there's no "downstream" HPD handler should we use
the internal HPD. This is signalled by the DRM framework by a call to
dp_bridge_hpd_enable(). So we should use that to enable/disable the
internal HPD handler.

When this happens, we have a reason for keeping power on; i.e. call
pm_runtime_get(). Once we have power/clocking, we'd call
dp_catalog_hpd_config_intr(), from dp_bridge_hpd_enable().


In the case that the internal HPD handling is not use,
dp_bridge_hpd_enable() will not be called, instead once the downstream
hpd handler switches state dp_bridge_hpd_notify() will be invoked.

In this case, we need the DP controller to be powered/clocked between
connector_status_connected and connector_status_disconnected.


I believe this should allow the DP controller(s) to stay powered down in
the case where we have external HPD handling (e.g. USB Type-C or
gpio-based dp-connector).

Regards,
Bjorn

Re: [RFC PATCH 2/2] drm/msm/dp: enable pm_runtime support for dp driver

[Bjorn Andersson]

On Thu, Feb 23, 2023 at 07:26:35PM +0530, Sankeerth Billakanti wrote:
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
[..]
> +static int dp_runtime_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct msm_dp *dp_display = platform_get_drvdata(pdev);
> +	struct dp_display_private *dp;
> +
> +	dp = container_of(dp_display, struct dp_display_private, dp_display);
> +	dp_display_host_init(dp);
> +
> +	if (dp->dp_display.is_edp) {
> +		dp_display_host_phy_init(dp);
> +	} else {
> +		dp_catalog_hpd_config_intr(dp->catalog,
> +				DP_DP_HPD_PLUG_INT_MASK |
> +				DP_DP_HPD_UNPLUG_INT_MASK,
> +				true);

I believe this is backwards.

Only in the event that there's no "downstream" HPD handler should we use
the internal HPD. This is signalled by the DRM framework by a call to
dp_bridge_hpd_enable(). So we should use that to enable/disable the
internal HPD handler.

When this happens, we have a reason for keeping power on; i.e. call
pm_runtime_get(). Once we have power/clocking, we'd call
dp_catalog_hpd_config_intr(), from dp_bridge_hpd_enable().


In the case that the internal HPD handling is not use,
dp_bridge_hpd_enable() will not be called, instead once the downstream
hpd handler switches state dp_bridge_hpd_notify() will be invoked.

In this case, we need the DP controller to be powered/clocked between
connector_status_connected and connector_status_disconnected.


I believe this should allow the DP controller(s) to stay powered down in
the case where we have external HPD handling (e.g. USB Type-C or
gpio-based dp-connector).

Regards,
Bjorn

RE: [RFC PATCH 2/2] drm/msm/dp: enable pm_runtime support for dp driver

[Sankeerth Billakanti (QUIC)]

>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
>> b/drivers/gpu/drm/msm/dp/dp_aux.c
>[..]
>> +static int dp_runtime_resume(struct device *dev) {
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct msm_dp *dp_display = platform_get_drvdata(pdev);
>> +	struct dp_display_private *dp;
>> +
>> +	dp = container_of(dp_display, struct dp_display_private, dp_display);
>> +	dp_display_host_init(dp);
>> +
>> +	if (dp->dp_display.is_edp) {
>> +		dp_display_host_phy_init(dp);
>> +	} else {
>> +		dp_catalog_hpd_config_intr(dp->catalog,
>> +				DP_DP_HPD_PLUG_INT_MASK |
>> +				DP_DP_HPD_UNPLUG_INT_MASK,
>> +				true);
>
>I believe this is backwards.
>
>Only in the event that there's no "downstream" HPD handler should we use
>the internal HPD. This is signalled by the DRM framework by a call to
>dp_bridge_hpd_enable(). So we should use that to enable/disable the
>internal HPD handler.
>
>When this happens, we have a reason for keeping power on; i.e. call
>pm_runtime_get(). Once we have power/clocking, we'd call
>dp_catalog_hpd_config_intr(), from dp_bridge_hpd_enable().
>
>
>In the case that the internal HPD handling is not use,
>dp_bridge_hpd_enable() will not be called, instead once the downstream hpd
>handler switches state dp_bridge_hpd_notify() will be invoked.
>
>In this case, we need the DP controller to be powered/clocked between
>connector_status_connected and connector_status_disconnected.
>
>
>I believe this should allow the DP controller(s) to stay powered down in the
>case where we have external HPD handling (e.g. USB Type-C or gpio-based
>dp-connector).
>
>Regards,
>Bjorn

I agree with the approach. I am moving my dev to msm-next. Will make the changes according to the HPD handling and repost

Thank you,
Sankeerth

RE: [RFC PATCH 2/2] drm/msm/dp: enable pm_runtime support for dp driver

[Sankeerth Billakanti (QUIC)]

>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
>> b/drivers/gpu/drm/msm/dp/dp_aux.c
>[..]
>> +static int dp_runtime_resume(struct device *dev) {
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct msm_dp *dp_display = platform_get_drvdata(pdev);
>> +	struct dp_display_private *dp;
>> +
>> +	dp = container_of(dp_display, struct dp_display_private, dp_display);
>> +	dp_display_host_init(dp);
>> +
>> +	if (dp->dp_display.is_edp) {
>> +		dp_display_host_phy_init(dp);
>> +	} else {
>> +		dp_catalog_hpd_config_intr(dp->catalog,
>> +				DP_DP_HPD_PLUG_INT_MASK |
>> +				DP_DP_HPD_UNPLUG_INT_MASK,
>> +				true);
>
>I believe this is backwards.
>
>Only in the event that there's no "downstream" HPD handler should we use
>the internal HPD. This is signalled by the DRM framework by a call to
>dp_bridge_hpd_enable(). So we should use that to enable/disable the
>internal HPD handler.
>
>When this happens, we have a reason for keeping power on; i.e. call
>pm_runtime_get(). Once we have power/clocking, we'd call
>dp_catalog_hpd_config_intr(), from dp_bridge_hpd_enable().
>
>
>In the case that the internal HPD handling is not use,
>dp_bridge_hpd_enable() will not be called, instead once the downstream hpd
>handler switches state dp_bridge_hpd_notify() will be invoked.
>
>In this case, we need the DP controller to be powered/clocked between
>connector_status_connected and connector_status_disconnected.
>
>
>I believe this should allow the DP controller(s) to stay powered down in the
>case where we have external HPD handling (e.g. USB Type-C or gpio-based
>dp-connector).
>
>Regards,
>Bjorn

I agree with the approach. I am moving my dev to msm-next. Will make the changes according to the HPD handling and repost

Thank you,
Sankeerth

Re: [RFC PATCH 0/2] drm/msm/dp: refactor the msm dp driver resources

[Dmitry Baryshkov]

On Thu, 23 Feb 2023 at 15:57, Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> The DP driver resources are currently enabled and disabled directly based on code flow.
> As mentioned in bug 230631602, we want to do the following:

private bug tracker

>
> 1) Refactor the dp/edp parsing code to move it to probe (it is currently done in bind).

This is good. I'd suggest splitting this into smaller chunks. First,
move all resource binding, then move the actual dp_aux handling. It
would be easier to review it this way.

> 2) Then bind all the power resources needed for AUX in pm_runtime_ops.
>
> 3) Handle EPROBE_DEFER cases of the panel-eDP aux device.

This is not handled properly. The eDP aux probing is asynchronous, so
you should move the second stage into the done_probing() part, rather
than relying on the -EPROBE_DEFER. There can be cases, when the panel
driver is not available at the DP's probe time. In such cases we
should leave the DP driver probed, then wait for the panel before
binding the component.

> 4) Verify DP functionality is unaffected.
>
> These code changes will parse the resources and get the edp panel during probe.
> All the necessary resources required for the aux transactions are moved to pm_runtime ops.
> They are enabled or disabled via get/put sync functions.
>
> This is a RFC to verify with the community if the approach we are taking is correct.
>
> https://partnerissuetracker.corp.google.com/issues/230631602

This link is useless, since its contents are not public.

>
> Sankeerth Billakanti (2):
>   drm/msm/dp: enumerate edp panel during driver probe
>   drm/msm/dp: enable pm_runtime support for dp driver
>
>  drivers/gpu/drm/msm/dp/dp_aux.c     | 155 +++++++++++++++++++++--
>  drivers/gpu/drm/msm/dp/dp_catalog.c |  12 ++
>  drivers/gpu/drm/msm/dp/dp_catalog.h |   1 +
>  drivers/gpu/drm/msm/dp/dp_display.c | 185 ++++++++++++++--------------
>  drivers/gpu/drm/msm/dp/dp_power.c   |   7 --
>  5 files changed, 250 insertions(+), 110 deletions(-)
>
> --
> 2.39.0
>


-- 
With best wishes
Dmitry

Re: [RFC PATCH 0/2] drm/msm/dp: refactor the msm dp driver resources

[Dmitry Baryshkov]

On Thu, 23 Feb 2023 at 15:57, Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> The DP driver resources are currently enabled and disabled directly based on code flow.
> As mentioned in bug 230631602, we want to do the following:

private bug tracker

>
> 1) Refactor the dp/edp parsing code to move it to probe (it is currently done in bind).

This is good. I'd suggest splitting this into smaller chunks. First,
move all resource binding, then move the actual dp_aux handling. It
would be easier to review it this way.

> 2) Then bind all the power resources needed for AUX in pm_runtime_ops.
>
> 3) Handle EPROBE_DEFER cases of the panel-eDP aux device.

This is not handled properly. The eDP aux probing is asynchronous, so
you should move the second stage into the done_probing() part, rather
than relying on the -EPROBE_DEFER. There can be cases, when the panel
driver is not available at the DP's probe time. In such cases we
should leave the DP driver probed, then wait for the panel before
binding the component.

> 4) Verify DP functionality is unaffected.
>
> These code changes will parse the resources and get the edp panel during probe.
> All the necessary resources required for the aux transactions are moved to pm_runtime ops.
> They are enabled or disabled via get/put sync functions.
>
> This is a RFC to verify with the community if the approach we are taking is correct.
>
> https://partnerissuetracker.corp.google.com/issues/230631602

This link is useless, since its contents are not public.

>
> Sankeerth Billakanti (2):
>   drm/msm/dp: enumerate edp panel during driver probe
>   drm/msm/dp: enable pm_runtime support for dp driver
>
>  drivers/gpu/drm/msm/dp/dp_aux.c     | 155 +++++++++++++++++++++--
>  drivers/gpu/drm/msm/dp/dp_catalog.c |  12 ++
>  drivers/gpu/drm/msm/dp/dp_catalog.h |   1 +
>  drivers/gpu/drm/msm/dp/dp_display.c | 185 ++++++++++++++--------------
>  drivers/gpu/drm/msm/dp/dp_power.c   |   7 --
>  5 files changed, 250 insertions(+), 110 deletions(-)
>
> --
> 2.39.0
>


-- 
With best wishes
Dmitry

RE: [RFC PATCH 0/2] drm/msm/dp: refactor the msm dp driver resources

[Sankeerth Billakanti (QUIC)]

>> The DP driver resources are currently enabled and disabled directly based
>on code flow.
>> As mentioned in bug 230631602, we want to do the following:
>
>private bug tracker
>

Will remove the reference for this.

>>
>> 1) Refactor the dp/edp parsing code to move it to probe (it is currently done
>in bind).
>
>This is good. I'd suggest splitting this into smaller chunks. First, move all
>resource binding, then move the actual dp_aux handling. It would be easier to
>review it this way.
>

Okay, will move the resource binding patch first.

>> 2) Then bind all the power resources needed for AUX in pm_runtime_ops.
>>
>> 3) Handle EPROBE_DEFER cases of the panel-eDP aux device.
>
>This is not handled properly. The eDP aux probing is asynchronous, so you
>should move the second stage into the done_probing() part, rather than
>relying on the -EPROBE_DEFER. There can be cases, when the panel driver is
>not available at the DP's probe time. In such cases we should leave the DP
>driver probed, then wait for the panel before binding the component.
>

Okay, I will explore this.

>> 4) Verify DP functionality is unaffected.
>>
>> These code changes will parse the resources and get the edp panel during
>probe.
>> All the necessary resources required for the aux transactions are moved to
>pm_runtime ops.
>> They are enabled or disabled via get/put sync functions.
>>
>> This is a RFC to verify with the community if the approach we are taking is
>correct.
>>
>> https://partnerissuetracker.corp.google.com/issues/230631602
>
>This link is useless, since its contents are not public.
>


Okay, I will remove it.

>>
>> Sankeerth Billakanti (2):
>>   drm/msm/dp: enumerate edp panel during driver probe
>>   drm/msm/dp: enable pm_runtime support for dp driver
>>
>>  drivers/gpu/drm/msm/dp/dp_aux.c     | 155 +++++++++++++++++++++--
>>  drivers/gpu/drm/msm/dp/dp_catalog.c |  12 ++
>>  drivers/gpu/drm/msm/dp/dp_catalog.h |   1 +
>>  drivers/gpu/drm/msm/dp/dp_display.c | 185 ++++++++++++++--------------
>>  drivers/gpu/drm/msm/dp/dp_power.c   |   7 --
>>  5 files changed, 250 insertions(+), 110 deletions(-)
>>
>> --
>> 2.39.0
>>
>
>
>--
>With best wishes
>Dmitry

RE: [RFC PATCH 0/2] drm/msm/dp: refactor the msm dp driver resources

[Sankeerth Billakanti (QUIC)]

>> The DP driver resources are currently enabled and disabled directly based
>on code flow.
>> As mentioned in bug 230631602, we want to do the following:
>
>private bug tracker
>

Will remove the reference for this.

>>
>> 1) Refactor the dp/edp parsing code to move it to probe (it is currently done
>in bind).
>
>This is good. I'd suggest splitting this into smaller chunks. First, move all
>resource binding, then move the actual dp_aux handling. It would be easier to
>review it this way.
>

Okay, will move the resource binding patch first.

>> 2) Then bind all the power resources needed for AUX in pm_runtime_ops.
>>
>> 3) Handle EPROBE_DEFER cases of the panel-eDP aux device.
>
>This is not handled properly. The eDP aux probing is asynchronous, so you
>should move the second stage into the done_probing() part, rather than
>relying on the -EPROBE_DEFER. There can be cases, when the panel driver is
>not available at the DP's probe time. In such cases we should leave the DP
>driver probed, then wait for the panel before binding the component.
>

Okay, I will explore this.

>> 4) Verify DP functionality is unaffected.
>>
>> These code changes will parse the resources and get the edp panel during
>probe.
>> All the necessary resources required for the aux transactions are moved to
>pm_runtime ops.
>> They are enabled or disabled via get/put sync functions.
>>
>> This is a RFC to verify with the community if the approach we are taking is
>correct.
>>
>> https://partnerissuetracker.corp.google.com/issues/230631602
>
>This link is useless, since its contents are not public.
>


Okay, I will remove it.

>>
>> Sankeerth Billakanti (2):
>>   drm/msm/dp: enumerate edp panel during driver probe
>>   drm/msm/dp: enable pm_runtime support for dp driver
>>
>>  drivers/gpu/drm/msm/dp/dp_aux.c     | 155 +++++++++++++++++++++--
>>  drivers/gpu/drm/msm/dp/dp_catalog.c |  12 ++
>>  drivers/gpu/drm/msm/dp/dp_catalog.h |   1 +
>>  drivers/gpu/drm/msm/dp/dp_display.c | 185 ++++++++++++++--------------
>>  drivers/gpu/drm/msm/dp/dp_power.c   |   7 --
>>  5 files changed, 250 insertions(+), 110 deletions(-)
>>
>> --
>> 2.39.0
>>
>
>
>--
>With best wishes
>Dmitry