* [PATCH] drm/msm/dp: add logs across DP driver for ease of debugging
@ 2021-06-17 1:08 ` maitreye
0 siblings, 0 replies; 6+ 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] 6+ messages in thread
* [PATCH] drm/msm/dp: add logs across DP driver for ease of debugging
@ 2021-06-17 1:08 ` maitreye
0 siblings, 0 replies; 6+ messages in thread
From: maitreye @ 2021-06-17 1:08 UTC (permalink / raw)
To: dri-devel
Cc: Maitreyee Rao, linux-arm-msm, abhinavk, swboyd, khsieh,
robdclark, seanpaul, aravindh, freedreno
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] 6+ messages in thread
* Re: [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
-1 siblings, 0 replies; 6+ 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] 6+ messages in thread
* Re: [PATCH] drm/msm/dp: add logs across DP driver for ease of debugging
@ 2021-06-17 22:33 ` Stephen Boyd
0 siblings, 0 replies; 6+ messages in thread
From: Stephen Boyd @ 2021-06-17 22:33 UTC (permalink / raw)
To: dri-devel, maitreye
Cc: linux-arm-msm, abhinavk, khsieh, robdclark, seanpaul, aravindh,
freedreno
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] 6+ 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
-1 siblings, 0 replies; 6+ 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] 6+ messages in thread
* Re: [PATCH] drm/msm/dp: add logs across DP driver for ease of debugging
@ 2021-06-28 17:14 ` maitreye
0 siblings, 0 replies; 6+ messages in thread
From: maitreye @ 2021-06-28 17:14 UTC (permalink / raw)
To: Stephen Boyd
Cc: linux-arm-msm, abhinavk, khsieh, seanpaul, dri-devel, 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] 6+ messages in thread
end of thread, other threads:[~2021-06-28 17:15 UTC | newest]
Thread overview: 6+ 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 1:08 ` maitreye
2021-06-17 22:33 ` Stephen Boyd
2021-06-17 22:33 ` Stephen Boyd
2021-06-28 17:14 ` maitreye
2021-06-28 17:14 ` maitreye
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.