linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm/msm/dp: add logs across DP driver for ease of debugging
@ 2021-06-17  1:08 maitreye
  2021-06-17 22:33 ` Stephen Boyd
  0 siblings, 1 reply; 3+ messages in thread
From: maitreye @ 2021-06-17  1:08 UTC (permalink / raw)
  To: dri-devel
  Cc: Maitreyee Rao, linux-arm-msm, freedreno, robdclark, seanpaul,
	swboyd, nganji, aravindh, khsieh, abhinavk

From: Maitreyee Rao <maitreye@codeaurora.org>

Add trace points across the MSM DP driver to help debug
interop issues.

Signed-off-by: Maitreyee Rao <maitreye@codeaurora.org>
---
 drivers/gpu/drm/msm/dp/dp_aux.c     |  5 +++--
 drivers/gpu/drm/msm/dp/dp_catalog.c |  4 ++++
 drivers/gpu/drm/msm/dp/dp_ctrl.c    |  7 +++++++
 drivers/gpu/drm/msm/dp/dp_display.c | 16 ++++++++++++++++
 drivers/gpu/drm/msm/dp/dp_link.c    | 20 +++++++++++++-------
 drivers/gpu/drm/msm/dp/dp_panel.c   |  2 ++
 drivers/gpu/drm/msm/dp/dp_power.c   |  3 +++
 7 files changed, 48 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 4a3293b..5fdff18d 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -121,9 +121,10 @@ static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
 
 	time_left = wait_for_completion_timeout(&aux->comp,
 						msecs_to_jiffies(250));
-	if (!time_left)
+	if (!time_left) {
+		DRM_DEBUG_DP("%s aux timeout error timeout:%lu\n", __func__, time_left);
 		return -ETIMEDOUT;
-
+	}
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 32f3575..5de5dcd 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -372,6 +372,7 @@ void dp_catalog_ctrl_mainlink_ctrl(struct dp_catalog *dp_catalog,
 	struct dp_catalog_private *catalog = container_of(dp_catalog,
 				struct dp_catalog_private, dp_catalog);
 
+	DRM_DEBUG_DP("%s enable=0x%x\n", __func__, enable);
 	if (enable) {
 		/*
 		 * To make sure link reg writes happens before other operation,
@@ -580,6 +581,7 @@ void dp_catalog_hpd_config_intr(struct dp_catalog *dp_catalog,
 
 	config = (en ? config | intr_mask : config & ~intr_mask);
 
+	DRM_DEBUG_DP("%s intr_mask=0x%x config=0x%x\n", __func__, intr_mask, config);
 	dp_write_aux(catalog, REG_DP_DP_HPD_INT_MASK,
 				config & DP_DP_HPD_INT_MASK);
 }
@@ -610,6 +612,7 @@ u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog)
 	u32 status;
 
 	status = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
+	DRM_DEBUG_DP("%s aux status:0x%x\n", __func__, status);
 	status >>= DP_DP_HPD_STATE_STATUS_BITS_SHIFT;
 	status &= DP_DP_HPD_STATE_STATUS_BITS_MASK;
 
@@ -685,6 +688,7 @@ void dp_catalog_ctrl_send_phy_pattern(struct dp_catalog *dp_catalog,
 	/* Make sure to clear the current pattern before starting a new one */
 	dp_write_link(catalog, REG_DP_STATE_CTRL, 0x0);
 
+	DRM_DEBUG_DP("%s pattern:0x%x\n", __func__, pattern);
 	switch (pattern) {
 	case DP_PHY_TEST_PATTERN_D10_2:
 		dp_write_link(catalog, REG_DP_STATE_CTRL,
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 2a8955c..7fd1e3f 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -99,6 +99,7 @@ static int dp_aux_link_configure(struct drm_dp_aux *aux,
 	values[0] = drm_dp_link_rate_to_bw_code(link->rate);
 	values[1] = link->num_lanes;
 
+	DRM_DEBUG_DP("%s value0:0x%x value1:0x%x\n", __func__, values[0], values[1]);
 	if (link->capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
 		values[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
 
@@ -122,6 +123,7 @@ void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl)
 			IDLE_PATTERN_COMPLETION_TIMEOUT_JIFFIES))
 		pr_warn("PUSH_IDLE pattern timedout\n");
 
+	DRM_DEBUG_DP("PUSH IDLE\n");
 	pr_debug("mainlink off done\n");
 }
 
@@ -1013,6 +1015,8 @@ static int dp_ctrl_update_vx_px(struct dp_ctrl_private *ctrl)
 	u32 voltage_swing_level = link->phy_params.v_level;
 	u32 pre_emphasis_level = link->phy_params.p_level;
 
+	DRM_DEBUG_DP("%s: voltage level:%d emphasis level:%d\n", __func__,
+			voltage_swing_level, pre_emphasis_level);
 	ret = dp_catalog_ctrl_update_vx_px(ctrl->catalog,
 		voltage_swing_level, pre_emphasis_level);
 
@@ -1112,6 +1116,8 @@ static int dp_ctrl_link_train_1(struct dp_ctrl_private *ctrl,
 		cr->lane_0_1 = link_status[0];
 		cr->lane_2_3 = link_status[1];
 
+		DRM_DEBUG_DP("link status:0x%x 0x%x 0x%x 0x%x 0x%x\n", link_status[0],
+				link_status[1], link_status[2], link_status[3], link_status[4]);
 		if (drm_dp_clock_recovery_ok(link_status,
 			ctrl->link->link_params.num_lanes)) {
 			return 0;
@@ -1384,6 +1390,7 @@ int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, bool flip, bool reset)
 	if (reset)
 		dp_catalog_ctrl_reset(ctrl->catalog);
 
+	DRM_DEBUG_DP("%s Flip:%d\n", __func__, flip);
 	dp_catalog_ctrl_phy_reset(ctrl->catalog);
 	phy_init(phy);
 	dp_catalog_ctrl_enable_irq(ctrl->catalog, true);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index cf9c645..b471fe4 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -275,6 +275,8 @@ static bool dp_display_is_ds_bridge(struct dp_panel *panel)
 
 static bool dp_display_is_sink_count_zero(struct dp_display_private *dp)
 {
+	DRM_DEBUG_DP("%s present=0x%x sink_count=%d\n", __func__,
+			dp->panel->dpcd[DP_DOWNSTREAMPORT_PRESENT], dp->link->sink_count);
 	return dp_display_is_ds_bridge(dp->panel) &&
 		(dp->link->sink_count == 0);
 }
@@ -320,6 +322,7 @@ static int dp_display_send_hpd_notification(struct dp_display_private *dp,
 
 	dp->dp_display.is_connected = hpd;
 
+	DRM_DEBUG_DP("%s hpd=%d\n", __func__, hpd);
 	dp_display_send_hpd_event(&dp->dp_display);
 
 	return 0;
@@ -369,6 +372,8 @@ static void dp_display_host_init(struct dp_display_private *dp, int reset)
 {
 	bool flip = false;
 
+	DRM_DEBUG_DP("%s core_initialized=%d", __func__, dp->core_initialized);
+
 	if (dp->core_initialized) {
 		DRM_DEBUG_DP("DP core already initialized\n");
 		return;
@@ -483,8 +488,10 @@ static int dp_display_handle_irq_hpd(struct dp_display_private *dp)
 {
 	u32 sink_request = dp->link->sink_request;
 
+	DRM_DEBUG_DP("%s %d\n", __func__, sink_request);
 	if (dp->hpd_state == ST_DISCONNECTED) {
 		if (sink_request & DP_LINK_STATUS_UPDATED) {
+			DRM_DEBUG_DP("%s:Disconnected sink_count:%d\n", __func__, sink_request);
 			DRM_ERROR("Disconnected, no DP_LINK_STATUS_UPDATED\n");
 			return -EINVAL;
 		}
@@ -509,6 +516,7 @@ static int dp_display_usbpd_attention_cb(struct device *dev)
 		DRM_ERROR("invalid dev\n");
 		return -EINVAL;
 	}
+	DRM_DEBUG_DP("%s sink_request:%d\n", __func__, sink_request);
 
 	dp = container_of(g_dp_display,
 			struct dp_display_private, dp_display);
@@ -523,6 +531,8 @@ static int dp_display_usbpd_attention_cb(struct device *dev)
 	rc = dp_link_process_request(dp->link);
 	if (!rc) {
 		sink_request = dp->link->sink_request;
+		DRM_DEBUG_DP("%s hpd_state=%d sink_count=%d\n", __func__,
+				dp->hpd_state, sink_request);
 		if (sink_request & DS_PORT_STATUS_CHANGED)
 			rc = dp_display_handle_port_ststus_changed(dp);
 		else
@@ -545,6 +555,7 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data)
 	mutex_lock(&dp->event_mutex);
 
 	state =  dp->hpd_state;
+	DRM_DEBUG_DP("%s hpd_state=%d\n", __func__, state);
 	if (state == ST_DISPLAY_OFF || state == ST_SUSPENDED) {
 		mutex_unlock(&dp->event_mutex);
 		return 0;
@@ -680,6 +691,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
 	/* start sentinel checking in case of missing uevent */
 	dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
 
+	DRM_DEBUG_DP("%s hpd_state=%d\n", __func__, state);
 	/* signal the disconnect event early to ensure proper teardown */
 	dp_display_handle_plugged_change(g_dp_display, false);
 
@@ -738,6 +750,7 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp, u32 data)
 	if (ret == -ECONNRESET) { /* cable unplugged */
 		dp->core_initialized = false;
 	}
+	DRM_DEBUG_DP("%s hpd_state=%d\n", __func__, state);
 
 	mutex_unlock(&dp->event_mutex);
 
@@ -882,6 +895,7 @@ static int dp_display_enable(struct dp_display_private *dp, u32 data)
 
 	dp_display = g_dp_display;
 
+	DRM_DEBUG_DP("%s sink_count=%d\n", __func__, dp->link->sink_count);
 	if (dp_display->power_on) {
 		DRM_DEBUG_DP("Link already setup, return\n");
 		return 0;
@@ -943,6 +957,7 @@ static int dp_display_disable(struct dp_display_private *dp, u32 data)
 
 	dp_display->power_on = false;
 
+	DRM_DEBUG_DP("%s:  sink count:%d\n", __func__, dp->link->sink_count);
 	return 0;
 }
 
@@ -1190,6 +1205,7 @@ static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
 
 	hpd_isr_status = dp_catalog_hpd_get_intr_status(dp->catalog);
 
+	DRM_DEBUG_DP("%s: hpd isr status:%x\n", __func__, hpd_isr_status);
 	if (hpd_isr_status & 0x0F) {
 		/* hpd related interrupts */
 		if (hpd_isr_status & DP_DP_HPD_PLUG_INT_MASK ||
diff --git a/drivers/gpu/drm/msm/dp/dp_link.c b/drivers/gpu/drm/msm/dp/dp_link.c
index be986da..f858a8c 100644
--- a/drivers/gpu/drm/msm/dp/dp_link.c
+++ b/drivers/gpu/drm/msm/dp/dp_link.c
@@ -973,6 +973,9 @@ static int dp_link_process_link_status_update(struct dp_link_private *link)
  */
 static int dp_link_process_ds_port_status_change(struct dp_link_private *link)
 {
+	DRM_DEBUG_DP("link status 0:0x%x 1:0x%x 2:0x%x 3:0x%x 4:0x%x", link->link_status[0],
+			link->link_status[1], link->link_status[2],
+			link->link_status[3], link->link_status[4]);
 	if (get_link_status(link->link_status, DP_LANE_ALIGN_STATUS_UPDATED) &
 					DP_DOWNSTREAM_PORT_STATUS_CHANGED)
 		goto reset;
@@ -1036,43 +1039,46 @@ int dp_link_process_request(struct dp_link *dp_link)
 
 	if (link->request.test_requested == DP_TEST_LINK_EDID_READ) {
 		dp_link->sink_request |= DP_TEST_LINK_EDID_READ;
-		return ret;
+		goto error;
 	}
 
 	ret = dp_link_process_ds_port_status_change(link);
 	if (!ret) {
 		dp_link->sink_request |= DS_PORT_STATUS_CHANGED;
-		return ret;
+		goto error;
 	}
 
 	ret = dp_link_process_link_training_request(link);
 	if (!ret) {
 		dp_link->sink_request |= DP_TEST_LINK_TRAINING;
-		return ret;
+		goto error;
 	}
 
 	ret = dp_link_process_phy_test_pattern_request(link);
 	if (!ret) {
 		dp_link->sink_request |= DP_TEST_LINK_PHY_TEST_PATTERN;
-		return ret;
+		goto error;
 	}
 
 	ret = dp_link_process_link_status_update(link);
 	if (!ret) {
 		dp_link->sink_request |= DP_LINK_STATUS_UPDATED;
-		return ret;
+		goto error;
 	}
 
 	if (dp_link_is_video_pattern_requested(link)) {
-		ret = 0;
 		dp_link->sink_request |= DP_TEST_LINK_VIDEO_PATTERN;
+		goto error;
 	}
 
 	if (dp_link_is_audio_pattern_requested(link)) {
 		dp_link->sink_request |= DP_TEST_LINK_AUDIO_PATTERN;
-		return -EINVAL;
+		ret = -EINVAL;
+		goto error;
 	}
 
+error:
+	DRM_DEBUG_DP("%s sink request:%x", __func__, dp_link->sink_request);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
index 88196f7..71db071 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -66,6 +66,8 @@ static int dp_panel_read_dpcd(struct dp_panel *dp_panel)
 		goto end;
 	}
 
+	DRM_DEBUG_DP("%s 0x%x 0x%x 0x%x 0x%x 0x%x\n", __func__, dpcd[0],
+			dpcd[1], dpcd[2], dpcd[3], dpcd[4]);
 	link_info->revision = dpcd[DP_DPCD_REV];
 	major = (link_info->revision >> 4) & 0x0f;
 	minor = link_info->revision & 0x0f;
diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
index 3961ba4..2271941 100644
--- a/drivers/gpu/drm/msm/dp/dp_power.c
+++ b/drivers/gpu/drm/msm/dp/dp_power.c
@@ -208,6 +208,9 @@ static int dp_power_clk_set_rate(struct dp_power_private *power,
 
 int dp_power_clk_status(struct dp_power *dp_power, enum dp_pm_type pm_type)
 {
+	DRM_DEBUG_DP("%s core_clk_on=%d link_clk_on%d stream_clk_on=%d\n", __func__,
+			dp_power->core_clks_on, dp_power->link_clks_on, dp_power->stream_clks_on);
+
 	if (pm_type == DP_CORE_PM)
 		return dp_power->core_clks_on;
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


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

* Re: [PATCH] drm/msm/dp: add logs across DP driver for ease of debugging
  2021-06-17  1:08 [PATCH] drm/msm/dp: add logs across DP driver for ease of debugging maitreye
@ 2021-06-17 22:33 ` Stephen Boyd
  2021-06-28 17:14   ` maitreye
  0 siblings, 1 reply; 3+ messages in thread
From: Stephen Boyd @ 2021-06-17 22:33 UTC (permalink / raw)
  To: dri-devel, maitreye
  Cc: linux-arm-msm, freedreno, robdclark, seanpaul, nganji, aravindh,
	khsieh, abhinavk

Quoting maitreye (2021-06-16 18:08:54)
> From: Maitreyee Rao <maitreye@codeaurora.org>
>
> Add trace points across the MSM DP driver to help debug
> interop issues.
>
> Signed-off-by: Maitreyee Rao <maitreye@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/dp/dp_aux.c     |  5 +++--
>  drivers/gpu/drm/msm/dp/dp_catalog.c |  4 ++++
>  drivers/gpu/drm/msm/dp/dp_ctrl.c    |  7 +++++++
>  drivers/gpu/drm/msm/dp/dp_display.c | 16 ++++++++++++++++
>  drivers/gpu/drm/msm/dp/dp_link.c    | 20 +++++++++++++-------
>  drivers/gpu/drm/msm/dp/dp_panel.c   |  2 ++
>  drivers/gpu/drm/msm/dp/dp_power.c   |  3 +++
>  7 files changed, 48 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 4a3293b..5fdff18d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -121,9 +121,10 @@ static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
>
>         time_left = wait_for_completion_timeout(&aux->comp,
>                                                 msecs_to_jiffies(250));
> -       if (!time_left)
> +       if (!time_left) {
> +               DRM_DEBUG_DP("%s aux timeout error timeout:%lu\n", __func__, time_left);

This will always print 0 for "no time left". Is that useful to know? I'd
rather we just drop that. Also, __func__ shouldn't be needed given that
__drm_dbg() uses builtin_return_address(). And then, I believe the DP
aux core code already adds logs on the transfer to indicate how it
failed, so probably this whole line can be dropped.

>                 return -ETIMEDOUT;
> -
> +       }
>         return ret;
>  }
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index 32f3575..5de5dcd 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -372,6 +372,7 @@ void dp_catalog_ctrl_mainlink_ctrl(struct dp_catalog *dp_catalog,
>         struct dp_catalog_private *catalog = container_of(dp_catalog,
>                                 struct dp_catalog_private, dp_catalog);
>
> +       DRM_DEBUG_DP("%s enable=0x%x\n", __func__, enable);

Again, drop __func__. 'enable' is a bool, why is printed in hex format?

>         if (enable) {
>                 /*
>                  * To make sure link reg writes happens before other operation,
> @@ -580,6 +581,7 @@ void dp_catalog_hpd_config_intr(struct dp_catalog *dp_catalog,
>
>         config = (en ? config | intr_mask : config & ~intr_mask);
>
> +       DRM_DEBUG_DP("%s intr_mask=0x%x config=0x%x\n", __func__, intr_mask, config);
>         dp_write_aux(catalog, REG_DP_DP_HPD_INT_MASK,
>                                 config & DP_DP_HPD_INT_MASK);
>  }
> @@ -610,6 +612,7 @@ u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog)
>         u32 status;
>
>         status = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
> +       DRM_DEBUG_DP("%s aux status:0x%x\n", __func__, status);
>         status >>= DP_DP_HPD_STATE_STATUS_BITS_SHIFT;
>         status &= DP_DP_HPD_STATE_STATUS_BITS_MASK;
>
> @@ -685,6 +688,7 @@ void dp_catalog_ctrl_send_phy_pattern(struct dp_catalog *dp_catalog,
>         /* Make sure to clear the current pattern before starting a new one */
>         dp_write_link(catalog, REG_DP_STATE_CTRL, 0x0);
>
> +       DRM_DEBUG_DP("%s pattern:0x%x\n", __func__, pattern);
>         switch (pattern) {
>         case DP_PHY_TEST_PATTERN_D10_2:
>                 dp_write_link(catalog, REG_DP_STATE_CTRL,
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 2a8955c..7fd1e3f 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -99,6 +99,7 @@ static int dp_aux_link_configure(struct drm_dp_aux *aux,
>         values[0] = drm_dp_link_rate_to_bw_code(link->rate);
>         values[1] = link->num_lanes;
>
> +       DRM_DEBUG_DP("%s value0:0x%x value1:0x%x\n", __func__, values[0], values[1]);

The drm_dp_dpcd_write() soon after should tell us what this is, so is
this necessary?

>         if (link->capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
>                 values[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
>
> @@ -122,6 +123,7 @@ void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl)
>                         IDLE_PATTERN_COMPLETION_TIMEOUT_JIFFIES))
>                 pr_warn("PUSH_IDLE pattern timedout\n");
>
> +       DRM_DEBUG_DP("PUSH IDLE\n");
>         pr_debug("mainlink off done\n");

Can these two printks be combined into one DRM_DEBUG_DP()?

>  }
>
> @@ -1013,6 +1015,8 @@ static int dp_ctrl_update_vx_px(struct dp_ctrl_private *ctrl)
>         u32 voltage_swing_level = link->phy_params.v_level;
>         u32 pre_emphasis_level = link->phy_params.p_level;
>
> +       DRM_DEBUG_DP("%s: voltage level:%d emphasis level:%d\n", __func__,

Can we unstick the colon : from the printk format?

	voltage level: %d emphasis level: %d

> +                       voltage_swing_level, pre_emphasis_level);
>         ret = dp_catalog_ctrl_update_vx_px(ctrl->catalog,
>                 voltage_swing_level, pre_emphasis_level);
>
> @@ -1112,6 +1116,8 @@ static int dp_ctrl_link_train_1(struct dp_ctrl_private *ctrl,
>                 cr->lane_0_1 = link_status[0];
>                 cr->lane_2_3 = link_status[1];
>
> +               DRM_DEBUG_DP("link status:0x%x 0x%x 0x%x 0x%x 0x%x\n", link_status[0],
> +                               link_status[1], link_status[2], link_status[3], link_status[4]);

Again, the drm_dp_dpcd_read_link_status() code will print this for us so
this is redundant.

>                 if (drm_dp_clock_recovery_ok(link_status,
>                         ctrl->link->link_params.num_lanes)) {
>                         return 0;
> @@ -1384,6 +1390,7 @@ int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, bool flip, bool reset)
>         if (reset)
>                 dp_catalog_ctrl_reset(ctrl->catalog);
>
> +       DRM_DEBUG_DP("%s Flip:%d\n", __func__, flip);

Maybe

	"%s", flip ? "flipped" : "not flipped"

or

	"flip=%d", flip

>         dp_catalog_ctrl_phy_reset(ctrl->catalog);
>         phy_init(phy);
>         dp_catalog_ctrl_enable_irq(ctrl->catalog, true);
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index cf9c645..b471fe4 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -275,6 +275,8 @@ static bool dp_display_is_ds_bridge(struct dp_panel *panel)
>
>  static bool dp_display_is_sink_count_zero(struct dp_display_private *dp)
>  {
> +       DRM_DEBUG_DP("%s present=0x%x sink_count=%d\n", __func__,

We can use %#x for the 0x prefix.

> +                       dp->panel->dpcd[DP_DOWNSTREAMPORT_PRESENT], dp->link->sink_count);
>         return dp_display_is_ds_bridge(dp->panel) &&
>                 (dp->link->sink_count == 0);
>  }
> @@ -320,6 +322,7 @@ static int dp_display_send_hpd_notification(struct dp_display_private *dp,
>
>         dp->dp_display.is_connected = hpd;
>
> +       DRM_DEBUG_DP("%s hpd=%d\n", __func__, hpd);
>         dp_display_send_hpd_event(&dp->dp_display);
>
>         return 0;
> @@ -369,6 +372,8 @@ static void dp_display_host_init(struct dp_display_private *dp, int reset)
>  {
>         bool flip = false;
>
> +       DRM_DEBUG_DP("%s core_initialized=%d", __func__, dp->core_initialized);

Missing newline.

> +
>         if (dp->core_initialized) {
>                 DRM_DEBUG_DP("DP core already initialized\n");
>                 return;
> @@ -483,8 +488,10 @@ static int dp_display_handle_irq_hpd(struct dp_display_private *dp)
>  {
>         u32 sink_request = dp->link->sink_request;
>
> +       DRM_DEBUG_DP("%s %d\n", __func__, sink_request);
>         if (dp->hpd_state == ST_DISCONNECTED) {
>                 if (sink_request & DP_LINK_STATUS_UPDATED) {
> +                       DRM_DEBUG_DP("%s:Disconnected sink_count:%d\n", __func__, sink_request);
>                         DRM_ERROR("Disconnected, no DP_LINK_STATUS_UPDATED\n");
>                         return -EINVAL;
>                 }
> @@ -509,6 +516,7 @@ static int dp_display_usbpd_attention_cb(struct device *dev)
>                 DRM_ERROR("invalid dev\n");
>                 return -EINVAL;
>         }
> +       DRM_DEBUG_DP("%s sink_request:%d\n", __func__, sink_request);
>
>         dp = container_of(g_dp_display,
>                         struct dp_display_private, dp_display);
> @@ -523,6 +531,8 @@ static int dp_display_usbpd_attention_cb(struct device *dev)
>         rc = dp_link_process_request(dp->link);
>         if (!rc) {
>                 sink_request = dp->link->sink_request;
> +               DRM_DEBUG_DP("%s hpd_state=%d sink_count=%d\n", __func__,
> +                               dp->hpd_state, sink_request);
>                 if (sink_request & DS_PORT_STATUS_CHANGED)
>                         rc = dp_display_handle_port_ststus_changed(dp);
>                 else
> @@ -545,6 +555,7 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data)
>         mutex_lock(&dp->event_mutex);
>
>         state =  dp->hpd_state;
> +       DRM_DEBUG_DP("%s hpd_state=%d\n", __func__, state);
>         if (state == ST_DISPLAY_OFF || state == ST_SUSPENDED) {
>                 mutex_unlock(&dp->event_mutex);
>                 return 0;
> @@ -680,6 +691,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
>         /* start sentinel checking in case of missing uevent */
>         dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
>
> +       DRM_DEBUG_DP("%s hpd_state=%d\n", __func__, state);
>         /* signal the disconnect event early to ensure proper teardown */
>         dp_display_handle_plugged_change(g_dp_display, false);
>
> @@ -738,6 +750,7 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp, u32 data)
>         if (ret == -ECONNRESET) { /* cable unplugged */
>                 dp->core_initialized = false;
>         }
> +       DRM_DEBUG_DP("%s hpd_state=%d\n", __func__, state);
>
>         mutex_unlock(&dp->event_mutex);
>
> @@ -882,6 +895,7 @@ static int dp_display_enable(struct dp_display_private *dp, u32 data)
>
>         dp_display = g_dp_display;
>
> +       DRM_DEBUG_DP("%s sink_count=%d\n", __func__, dp->link->sink_count);
>         if (dp_display->power_on) {
>                 DRM_DEBUG_DP("Link already setup, return\n");
>                 return 0;
> @@ -943,6 +957,7 @@ static int dp_display_disable(struct dp_display_private *dp, u32 data)
>
>         dp_display->power_on = false;
>
> +       DRM_DEBUG_DP("%s:  sink count:%d\n", __func__, dp->link->sink_count);
>         return 0;
>  }
>
> @@ -1190,6 +1205,7 @@ static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
>
>         hpd_isr_status = dp_catalog_hpd_get_intr_status(dp->catalog);
>
> +       DRM_DEBUG_DP("%s: hpd isr status:%x\n", __func__, hpd_isr_status);

This one could have %#x

>         if (hpd_isr_status & 0x0F) {
>                 /* hpd related interrupts */
>                 if (hpd_isr_status & DP_DP_HPD_PLUG_INT_MASK ||
> diff --git a/drivers/gpu/drm/msm/dp/dp_link.c b/drivers/gpu/drm/msm/dp/dp_link.c
> index be986da..f858a8c 100644
> --- a/drivers/gpu/drm/msm/dp/dp_link.c
> +++ b/drivers/gpu/drm/msm/dp/dp_link.c
> @@ -973,6 +973,9 @@ static int dp_link_process_link_status_update(struct dp_link_private *link)
>   */
>  static int dp_link_process_ds_port_status_change(struct dp_link_private *link)
>  {
> +       DRM_DEBUG_DP("link status 0:0x%x 1:0x%x 2:0x%x 3:0x%x 4:0x%x", link->link_status[0],
> +                       link->link_status[1], link->link_status[2],
> +                       link->link_status[3], link->link_status[4]);

Is it useful to have the link status before it is gotten in the line
below? Also, get_link_status() seems to subtract a value and return it
vs. care about 5 elements.


>         if (get_link_status(link->link_status, DP_LANE_ALIGN_STATUS_UPDATED) &
>                                         DP_DOWNSTREAM_PORT_STATUS_CHANGED)
>                 goto reset;
> @@ -1036,43 +1039,46 @@ int dp_link_process_request(struct dp_link *dp_link)
>
>         if (link->request.test_requested == DP_TEST_LINK_EDID_READ) {
>                 dp_link->sink_request |= DP_TEST_LINK_EDID_READ;
> -               return ret;
> +               goto error;
>         }
>
>         ret = dp_link_process_ds_port_status_change(link);
>         if (!ret) {
>                 dp_link->sink_request |= DS_PORT_STATUS_CHANGED;
> -               return ret;
> +               goto error;
>         }
>
>         ret = dp_link_process_link_training_request(link);
>         if (!ret) {
>                 dp_link->sink_request |= DP_TEST_LINK_TRAINING;
> -               return ret;
> +               goto error;
>         }
>
>         ret = dp_link_process_phy_test_pattern_request(link);
>         if (!ret) {
>                 dp_link->sink_request |= DP_TEST_LINK_PHY_TEST_PATTERN;
> -               return ret;
> +               goto error;
>         }
>
>         ret = dp_link_process_link_status_update(link);
>         if (!ret) {
>                 dp_link->sink_request |= DP_LINK_STATUS_UPDATED;
> -               return ret;
> +               goto error;
>         }
>
>         if (dp_link_is_video_pattern_requested(link)) {
> -               ret = 0;

ret is not zero here, right? But now we dropped it?

>                 dp_link->sink_request |= DP_TEST_LINK_VIDEO_PATTERN;
> +               goto error;
>         }
>
>         if (dp_link_is_audio_pattern_requested(link)) {
>                 dp_link->sink_request |= DP_TEST_LINK_AUDIO_PATTERN;
> -               return -EINVAL;
> +               ret = -EINVAL;
> +               goto error;
>         }
>
> +error:

Is it an error? More like "out".

> +       DRM_DEBUG_DP("%s sink request:%x", __func__, dp_link->sink_request);
>         return ret;
>  }
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
> index 88196f7..71db071 100644
> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> @@ -66,6 +66,8 @@ static int dp_panel_read_dpcd(struct dp_panel *dp_panel)
>                 goto end;
>         }
>
> +       DRM_DEBUG_DP("%s 0x%x 0x%x 0x%x 0x%x 0x%x\n", __func__, dpcd[0],
> +                       dpcd[1], dpcd[2], dpcd[3], dpcd[4]);

Please drop as drm_dp_dpcd_read() should already print it.

>         link_info->revision = dpcd[DP_DPCD_REV];
>         major = (link_info->revision >> 4) & 0x0f;
>         minor = link_info->revision & 0x0f;
> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
> index 3961ba4..2271941 100644
> --- a/drivers/gpu/drm/msm/dp/dp_power.c
> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
> @@ -208,6 +208,9 @@ static int dp_power_clk_set_rate(struct dp_power_private *power,
>
>  int dp_power_clk_status(struct dp_power *dp_power, enum dp_pm_type pm_type)
>  {
> +       DRM_DEBUG_DP("%s core_clk_on=%d link_clk_on%d stream_clk_on=%d\n", __func__,

Missing = on link_clk_on?

> +                       dp_power->core_clks_on, dp_power->link_clks_on, dp_power->stream_clks_on);
> +
>         if (pm_type == DP_CORE_PM)
>                 return dp_power->core_clks_on;
>

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

* Re: [PATCH] drm/msm/dp: add logs across DP driver for ease of debugging
  2021-06-17 22:33 ` Stephen Boyd
@ 2021-06-28 17:14   ` maitreye
  0 siblings, 0 replies; 3+ messages in thread
From: maitreye @ 2021-06-28 17:14 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: dri-devel, linux-arm-msm, abhinavk, khsieh, robdclark, seanpaul,
	aravindh, freedreno

Hi Stephen,
Thanks for the review.
On 2021-06-17 15:33, Stephen Boyd wrote:
> Quoting maitreye (2021-06-16 18:08:54)
>> From: Maitreyee Rao <maitreye@codeaurora.org>
>> 
>> Add trace points across the MSM DP driver to help debug
>> interop issues.
>> 
>> Signed-off-by: Maitreyee Rao <maitreye@codeaurora.org>
>> ---
>>  drivers/gpu/drm/msm/dp/dp_aux.c     |  5 +++--
>>  drivers/gpu/drm/msm/dp/dp_catalog.c |  4 ++++
>>  drivers/gpu/drm/msm/dp/dp_ctrl.c    |  7 +++++++
>>  drivers/gpu/drm/msm/dp/dp_display.c | 16 ++++++++++++++++
>>  drivers/gpu/drm/msm/dp/dp_link.c    | 20 +++++++++++++-------
>>  drivers/gpu/drm/msm/dp/dp_panel.c   |  2 ++
>>  drivers/gpu/drm/msm/dp/dp_power.c   |  3 +++
>>  7 files changed, 48 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c 
>> b/drivers/gpu/drm/msm/dp/dp_aux.c
>> index 4a3293b..5fdff18d 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
>> @@ -121,9 +121,10 @@ static ssize_t dp_aux_cmd_fifo_tx(struct 
>> dp_aux_private *aux,
>> 
>>         time_left = wait_for_completion_timeout(&aux->comp,
>>                                                 
>> msecs_to_jiffies(250));
>> -       if (!time_left)
>> +       if (!time_left) {
>> +               DRM_DEBUG_DP("%s aux timeout error timeout:%lu\n", 
>> __func__, time_left);
> 
> This will always print 0 for "no time left". Is that useful to know? 
> I'd
> rather we just drop that. Also, __func__ shouldn't be needed given that
> __drm_dbg() uses builtin_return_address(). And then, I believe the DP
> aux core code already adds logs on the transfer to indicate how it
> failed, so probably this whole line can be dropped.
> 
Will fix this
>>                 return -ETIMEDOUT;
>> -
>> +       }
>>         return ret;
>>  }
>> 
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c 
>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> index 32f3575..5de5dcd 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> @@ -372,6 +372,7 @@ void dp_catalog_ctrl_mainlink_ctrl(struct 
>> dp_catalog *dp_catalog,
>>         struct dp_catalog_private *catalog = container_of(dp_catalog,
>>                                 struct dp_catalog_private, 
>> dp_catalog);
>> 
>> +       DRM_DEBUG_DP("%s enable=0x%x\n", __func__, enable);
> 
> Again, drop __func__. 'enable' is a bool, why is printed in hex format?
> 
Will fix this
>>         if (enable) {
>>                 /*
>>                  * To make sure link reg writes happens before other 
>> operation,
>> @@ -580,6 +581,7 @@ void dp_catalog_hpd_config_intr(struct dp_catalog 
>> *dp_catalog,
>> 
>>         config = (en ? config | intr_mask : config & ~intr_mask);
>> 
>> +       DRM_DEBUG_DP("%s intr_mask=0x%x config=0x%x\n", __func__, 
>> intr_mask, config);
>>         dp_write_aux(catalog, REG_DP_DP_HPD_INT_MASK,
>>                                 config & DP_DP_HPD_INT_MASK);
>>  }
>> @@ -610,6 +612,7 @@ u32 dp_catalog_link_is_connected(struct dp_catalog 
>> *dp_catalog)
>>         u32 status;
>> 
>>         status = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
>> +       DRM_DEBUG_DP("%s aux status:0x%x\n", __func__, status);
>>         status >>= DP_DP_HPD_STATE_STATUS_BITS_SHIFT;
>>         status &= DP_DP_HPD_STATE_STATUS_BITS_MASK;
>> 
>> @@ -685,6 +688,7 @@ void dp_catalog_ctrl_send_phy_pattern(struct 
>> dp_catalog *dp_catalog,
>>         /* Make sure to clear the current pattern before starting a 
>> new one */
>>         dp_write_link(catalog, REG_DP_STATE_CTRL, 0x0);
>> 
>> +       DRM_DEBUG_DP("%s pattern:0x%x\n", __func__, pattern);
>>         switch (pattern) {
>>         case DP_PHY_TEST_PATTERN_D10_2:
>>                 dp_write_link(catalog, REG_DP_STATE_CTRL,
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c 
>> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index 2a8955c..7fd1e3f 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -99,6 +99,7 @@ static int dp_aux_link_configure(struct drm_dp_aux 
>> *aux,
>>         values[0] = drm_dp_link_rate_to_bw_code(link->rate);
>>         values[1] = link->num_lanes;
>> 
>> +       DRM_DEBUG_DP("%s value0:0x%x value1:0x%x\n", __func__, 
>> values[0], values[1]);
> 
> The drm_dp_dpcd_write() soon after should tell us what this is, so is
> this necessary?
You are right, will fix this.
> 
>>         if (link->capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
>>                 values[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
>> 
>> @@ -122,6 +123,7 @@ void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl)
>>                         IDLE_PATTERN_COMPLETION_TIMEOUT_JIFFIES))
>>                 pr_warn("PUSH_IDLE pattern timedout\n");
>> 
>> +       DRM_DEBUG_DP("PUSH IDLE\n");
>>         pr_debug("mainlink off done\n");
> 
> Can these two printks be combined into one DRM_DEBUG_DP()?
> 
Sure, will do that.
>>  }
>> 
>> @@ -1013,6 +1015,8 @@ static int dp_ctrl_update_vx_px(struct 
>> dp_ctrl_private *ctrl)
>>         u32 voltage_swing_level = link->phy_params.v_level;
>>         u32 pre_emphasis_level = link->phy_params.p_level;
>> 
>> +       DRM_DEBUG_DP("%s: voltage level:%d emphasis level:%d\n", 
>> __func__,
> 
> Can we unstick the colon : from the printk format?
Yes, will fix it.
> 
> 	voltage level: %d emphasis level: %d
> 
>> +                       voltage_swing_level, pre_emphasis_level);
>>         ret = dp_catalog_ctrl_update_vx_px(ctrl->catalog,
>>                 voltage_swing_level, pre_emphasis_level);
>> 
>> @@ -1112,6 +1116,8 @@ static int dp_ctrl_link_train_1(struct 
>> dp_ctrl_private *ctrl,
>>                 cr->lane_0_1 = link_status[0];
>>                 cr->lane_2_3 = link_status[1];
>> 
>> +               DRM_DEBUG_DP("link status:0x%x 0x%x 0x%x 0x%x 0x%x\n", 
>> link_status[0],
>> +                               link_status[1], link_status[2], 
>> link_status[3], link_status[4]);
> 
> Again, the drm_dp_dpcd_read_link_status() code will print this for us 
> so
> this is redundant.
Yes, you are right will fix it.
> 
>>                 if (drm_dp_clock_recovery_ok(link_status,
>>                         ctrl->link->link_params.num_lanes)) {
>>                         return 0;
>> @@ -1384,6 +1390,7 @@ int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, 
>> bool flip, bool reset)
>>         if (reset)
>>                 dp_catalog_ctrl_reset(ctrl->catalog);
>> 
>> +       DRM_DEBUG_DP("%s Flip:%d\n", __func__, flip);
> 
> Maybe
> 
> 	"%s", flip ? "flipped" : "not flipped"
> 
> or
> 
> 	"flip=%d", flip
> 
Yes, will fix it.
>>         dp_catalog_ctrl_phy_reset(ctrl->catalog);
>>         phy_init(phy);
>>         dp_catalog_ctrl_enable_irq(ctrl->catalog, true);
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index cf9c645..b471fe4 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -275,6 +275,8 @@ static bool dp_display_is_ds_bridge(struct 
>> dp_panel *panel)
>> 
>>  static bool dp_display_is_sink_count_zero(struct dp_display_private 
>> *dp)
>>  {
>> +       DRM_DEBUG_DP("%s present=0x%x sink_count=%d\n", __func__,
> 
> We can use %#x for the 0x prefix.
Thanks , will fix this.
> 
>> +                       dp->panel->dpcd[DP_DOWNSTREAMPORT_PRESENT], 
>> dp->link->sink_count);
>>         return dp_display_is_ds_bridge(dp->panel) &&
>>                 (dp->link->sink_count == 0);
>>  }
>> @@ -320,6 +322,7 @@ static int dp_display_send_hpd_notification(struct 
>> dp_display_private *dp,
>> 
>>         dp->dp_display.is_connected = hpd;
>> 
>> +       DRM_DEBUG_DP("%s hpd=%d\n", __func__, hpd);
>>         dp_display_send_hpd_event(&dp->dp_display);
>> 
>>         return 0;
>> @@ -369,6 +372,8 @@ static void dp_display_host_init(struct 
>> dp_display_private *dp, int reset)
>>  {
>>         bool flip = false;
>> 
>> +       DRM_DEBUG_DP("%s core_initialized=%d", __func__, 
>> dp->core_initialized);
> 
> Missing newline.
Thanks, will add it
> 
>> +
>>         if (dp->core_initialized) {
>>                 DRM_DEBUG_DP("DP core already initialized\n");
>>                 return;
>> @@ -483,8 +488,10 @@ static int dp_display_handle_irq_hpd(struct 
>> dp_display_private *dp)
>>  {
>>         u32 sink_request = dp->link->sink_request;
>> 
>> +       DRM_DEBUG_DP("%s %d\n", __func__, sink_request);
>>         if (dp->hpd_state == ST_DISCONNECTED) {
>>                 if (sink_request & DP_LINK_STATUS_UPDATED) {
>> +                       DRM_DEBUG_DP("%s:Disconnected 
>> sink_count:%d\n", __func__, sink_request);
>>                         DRM_ERROR("Disconnected, no 
>> DP_LINK_STATUS_UPDATED\n");
>>                         return -EINVAL;
>>                 }
>> @@ -509,6 +516,7 @@ static int dp_display_usbpd_attention_cb(struct 
>> device *dev)
>>                 DRM_ERROR("invalid dev\n");
>>                 return -EINVAL;
>>         }
>> +       DRM_DEBUG_DP("%s sink_request:%d\n", __func__, sink_request);
>> 
>>         dp = container_of(g_dp_display,
>>                         struct dp_display_private, dp_display);
>> @@ -523,6 +531,8 @@ static int dp_display_usbpd_attention_cb(struct 
>> device *dev)
>>         rc = dp_link_process_request(dp->link);
>>         if (!rc) {
>>                 sink_request = dp->link->sink_request;
>> +               DRM_DEBUG_DP("%s hpd_state=%d sink_count=%d\n", 
>> __func__,
>> +                               dp->hpd_state, sink_request);
>>                 if (sink_request & DS_PORT_STATUS_CHANGED)
>>                         rc = 
>> dp_display_handle_port_ststus_changed(dp);
>>                 else
>> @@ -545,6 +555,7 @@ static int dp_hpd_plug_handle(struct 
>> dp_display_private *dp, u32 data)
>>         mutex_lock(&dp->event_mutex);
>> 
>>         state =  dp->hpd_state;
>> +       DRM_DEBUG_DP("%s hpd_state=%d\n", __func__, state);
>>         if (state == ST_DISPLAY_OFF || state == ST_SUSPENDED) {
>>                 mutex_unlock(&dp->event_mutex);
>>                 return 0;
>> @@ -680,6 +691,7 @@ static int dp_hpd_unplug_handle(struct 
>> dp_display_private *dp, u32 data)
>>         /* start sentinel checking in case of missing uevent */
>>         dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, 
>> DP_TIMEOUT_5_SECOND);
>> 
>> +       DRM_DEBUG_DP("%s hpd_state=%d\n", __func__, state);
>>         /* signal the disconnect event early to ensure proper teardown 
>> */
>>         dp_display_handle_plugged_change(g_dp_display, false);
>> 
>> @@ -738,6 +750,7 @@ static int dp_irq_hpd_handle(struct 
>> dp_display_private *dp, u32 data)
>>         if (ret == -ECONNRESET) { /* cable unplugged */
>>                 dp->core_initialized = false;
>>         }
>> +       DRM_DEBUG_DP("%s hpd_state=%d\n", __func__, state);
>> 
>>         mutex_unlock(&dp->event_mutex);
>> 
>> @@ -882,6 +895,7 @@ static int dp_display_enable(struct 
>> dp_display_private *dp, u32 data)
>> 
>>         dp_display = g_dp_display;
>> 
>> +       DRM_DEBUG_DP("%s sink_count=%d\n", __func__, 
>> dp->link->sink_count);
>>         if (dp_display->power_on) {
>>                 DRM_DEBUG_DP("Link already setup, return\n");
>>                 return 0;
>> @@ -943,6 +957,7 @@ static int dp_display_disable(struct 
>> dp_display_private *dp, u32 data)
>> 
>>         dp_display->power_on = false;
>> 
>> +       DRM_DEBUG_DP("%s:  sink count:%d\n", __func__, 
>> dp->link->sink_count);
>>         return 0;
>>  }
>> 
>> @@ -1190,6 +1205,7 @@ static irqreturn_t dp_display_irq_handler(int 
>> irq, void *dev_id)
>> 
>>         hpd_isr_status = dp_catalog_hpd_get_intr_status(dp->catalog);
>> 
>> +       DRM_DEBUG_DP("%s: hpd isr status:%x\n", __func__, 
>> hpd_isr_status);
> 
> This one could have %#x
yes will fix this.
> 
>>         if (hpd_isr_status & 0x0F) {
>>                 /* hpd related interrupts */
>>                 if (hpd_isr_status & DP_DP_HPD_PLUG_INT_MASK ||
>> diff --git a/drivers/gpu/drm/msm/dp/dp_link.c 
>> b/drivers/gpu/drm/msm/dp/dp_link.c
>> index be986da..f858a8c 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_link.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_link.c
>> @@ -973,6 +973,9 @@ static int 
>> dp_link_process_link_status_update(struct dp_link_private *link)
>>   */
>>  static int dp_link_process_ds_port_status_change(struct 
>> dp_link_private *link)
>>  {
>> +       DRM_DEBUG_DP("link status 0:0x%x 1:0x%x 2:0x%x 3:0x%x 4:0x%x", 
>> link->link_status[0],
>> +                       link->link_status[1], link->link_status[2],
>> +                       link->link_status[3], link->link_status[4]);
> 
> Is it useful to have the link status before it is gotten in the line
> below? Also, get_link_status() seems to subtract a value and return it
> vs. care about 5 elements.
Yes , I checked again and the dp_link_parse_status_field() already calls 
  drm_dp_dpcd_read_link_status()  so we do not need this and I will 
remove it.
> 
> 
>>         if (get_link_status(link->link_status, 
>> DP_LANE_ALIGN_STATUS_UPDATED) &
>>                                         
>> DP_DOWNSTREAM_PORT_STATUS_CHANGED)
>>                 goto reset;
>> @@ -1036,43 +1039,46 @@ int dp_link_process_request(struct dp_link 
>> *dp_link)
>> 
>>         if (link->request.test_requested == DP_TEST_LINK_EDID_READ) {
>>                 dp_link->sink_request |= DP_TEST_LINK_EDID_READ;
>> -               return ret;
>> +               goto error;
>>         }
>> 
>>         ret = dp_link_process_ds_port_status_change(link);
>>         if (!ret) {
>>                 dp_link->sink_request |= DS_PORT_STATUS_CHANGED;
>> -               return ret;
>> +               goto error;
>>         }
>> 
>>         ret = dp_link_process_link_training_request(link);
>>         if (!ret) {
>>                 dp_link->sink_request |= DP_TEST_LINK_TRAINING;
>> -               return ret;
>> +               goto error;
>>         }
>> 
>>         ret = dp_link_process_phy_test_pattern_request(link);
>>         if (!ret) {
>>                 dp_link->sink_request |= 
>> DP_TEST_LINK_PHY_TEST_PATTERN;
>> -               return ret;
>> +               goto error;
>>         }
>> 
>>         ret = dp_link_process_link_status_update(link);
>>         if (!ret) {
>>                 dp_link->sink_request |= DP_LINK_STATUS_UPDATED;
>> -               return ret;
>> +               goto error;
>>         }
>> 
>>         if (dp_link_is_video_pattern_requested(link)) {
>> -               ret = 0;
> 
> ret is not zero here, right? But now we dropped it?
ret is defaulted to zero at the beginning of the function and so this is 
redundant and I got rid of it.
> 
>>                 dp_link->sink_request |= DP_TEST_LINK_VIDEO_PATTERN;
>> +               goto error;
>>         }
>> 
>>         if (dp_link_is_audio_pattern_requested(link)) {
>>                 dp_link->sink_request |= DP_TEST_LINK_AUDIO_PATTERN;
>> -               return -EINVAL;
>> +               ret = -EINVAL;
>> +               goto error;
>>         }
>> 
>> +error:
> 
> Is it an error? More like "out".
Yes, will make that change.
> 
>> +       DRM_DEBUG_DP("%s sink request:%x", __func__, 
>> dp_link->sink_request);
>>         return ret;
>>  }
>> 
>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c 
>> b/drivers/gpu/drm/msm/dp/dp_panel.c
>> index 88196f7..71db071 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
>> @@ -66,6 +66,8 @@ static int dp_panel_read_dpcd(struct dp_panel 
>> *dp_panel)
>>                 goto end;
>>         }
>> 
>> +       DRM_DEBUG_DP("%s 0x%x 0x%x 0x%x 0x%x 0x%x\n", __func__, 
>> dpcd[0],
>> +                       dpcd[1], dpcd[2], dpcd[3], dpcd[4]);
> 
> Please drop as drm_dp_dpcd_read() should already print it.
yes, you are right, will fix this.
> 
>>         link_info->revision = dpcd[DP_DPCD_REV];
>>         major = (link_info->revision >> 4) & 0x0f;
>>         minor = link_info->revision & 0x0f;
>> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c 
>> b/drivers/gpu/drm/msm/dp/dp_power.c
>> index 3961ba4..2271941 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_power.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
>> @@ -208,6 +208,9 @@ static int dp_power_clk_set_rate(struct 
>> dp_power_private *power,
>> 
>>  int dp_power_clk_status(struct dp_power *dp_power, enum dp_pm_type 
>> pm_type)
>>  {
>> +       DRM_DEBUG_DP("%s core_clk_on=%d link_clk_on%d 
>> stream_clk_on=%d\n", __func__,
> 
> Missing = on link_clk_on?
Yes, thank you, will add it.
> 
>> +                       dp_power->core_clks_on, 
>> dp_power->link_clks_on, dp_power->stream_clks_on);
>> +
>>         if (pm_type == DP_CORE_PM)
>>                 return dp_power->core_clks_on;
>> 

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

end of thread, other threads:[~2021-06-28 17:14 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-17  1:08 [PATCH] drm/msm/dp: add logs across DP driver for ease of debugging maitreye
2021-06-17 22:33 ` Stephen Boyd
2021-06-28 17:14   ` maitreye

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