* Re: [PATCH v2] drm/msm/dp: add logs across DP driver for ease of debugging [not found] <1625771624-11997-1-git-send-email-maitreye@codeaurora.org> @ 2021-07-09 1:27 ` Stephen Boyd 2021-07-15 21:33 ` maitreye 2021-07-09 6:27 ` kernel test robot 1 sibling, 1 reply; 3+ messages in thread From: Stephen Boyd @ 2021-07-09 1:27 UTC (permalink / raw) To: dri-devel, maitreye Cc: linux-arm-msm, freedreno, robdclark, seanpaul, nganji, aravindh, khsieh, abhinavk Quoting maitreye (2021-07-08 12:13:44) > From: Maitreyee Rao <maitreye@codeaurora.org> > > Add trace points across the MSM DP driver to help debug > interop issues. > > Changes in v2: > - Got rid of redundant log messages. > - Added %#x instead of 0x%x wherever required. > - Got rid of __func__ calls in debug messages. > - Added newline wherever missing. I think this is the new thing in v3? Adding one missing newline? > > Signed-off-by: Maitreyee Rao <maitreye@codeaurora.org> > --- > drivers/gpu/drm/msm/dp/dp_catalog.c | 8 ++++++-- > drivers/gpu/drm/msm/dp/dp_ctrl.c | 5 ++++- > drivers/gpu/drm/msm/dp/dp_display.c | 14 ++++++++++++++ > drivers/gpu/drm/msm/dp/dp_link.c | 17 ++++++++++------- > drivers/gpu/drm/msm/dp/dp_panel.c | 2 ++ > drivers/gpu/drm/msm/dp/dp_power.c | 3 +++ > 6 files changed, 39 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c > index 32f3575..292ec2c 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("enable=%d\n", 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("intr_mask=%#x config=%#x\n", 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("aux status:%#x\n", status); Unstick colon from printf specifier? > 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("pattern:%#x\n", pattern); Unstick colon from printf specifier? > switch (pattern) { > case DP_PHY_TEST_PATTERN_D10_2: > dp_write_link(catalog, REG_DP_STATE_CTRL, > @@ -745,7 +749,7 @@ void dp_catalog_ctrl_send_phy_pattern(struct dp_catalog *dp_catalog, > DP_STATE_CTRL_LINK_TRAINING_PATTERN4); > break; > default: > - DRM_DEBUG_DP("No valid test pattern requested:0x%x\n", pattern); > + DRM_DEBUG_DP("No valid test pattern requested:%#x\n", pattern); Unstick colon from printf specifier? > break; > } > } > @@ -928,7 +932,7 @@ void dp_catalog_audio_config_acr(struct dp_catalog *dp_catalog) > select = dp_catalog->audio_data; > acr_ctrl = select << 4 | BIT(31) | BIT(8) | BIT(14); > > - DRM_DEBUG_DP("select = 0x%x, acr_ctrl = 0x%x\n", select, acr_ctrl); > + DRM_DEBUG_DP("select =0x%x, acr_ctrl =0x%x\n", select, acr_ctrl); This doesn't use %#x? And then it sticks it to the equals sign but doesn't move select and acr_ctrl to them? > > dp_write_link(catalog, MMSS_DP_AUDIO_ACR_CTRL, acr_ctrl); > } > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c > index 2a8955c..21ad7d3 100644 > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c > @@ -122,7 +122,7 @@ void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl) > IDLE_PATTERN_COMPLETION_TIMEOUT_JIFFIES)) > pr_warn("PUSH_IDLE pattern timedout\n"); > > - pr_debug("mainlink off done\n"); > + DRM_DEBUG_DP("PUSH IDLE, mainlink off done\n"); I think we already get the func name as dp_ctrl_push_idle in the print so is having PUSH IDLE in all caps really useful? > } > > static void dp_ctrl_config_ctrl(struct dp_ctrl_private *ctrl) > @@ -1013,6 +1013,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("voltage level: %d emphasis level: %d\n", voltage_swing_level, > + pre_emphasis_level); > ret = dp_catalog_ctrl_update_vx_px(ctrl->catalog, > voltage_swing_level, pre_emphasis_level); > > @@ -1384,6 +1386,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("flip=%d\n", 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..45301c5 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("present=%#x sink_count=%d\n", 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("hpd=%d\n", hpd); > dp_display_send_hpd_event(&dp->dp_display); > > return 0; > @@ -369,6 +372,7 @@ static void dp_display_host_init(struct dp_display_private *dp, int reset) > { > bool flip = false; > > + DRM_DEBUG_DP("core_initialized=%d\n", dp->core_initialized); > if (dp->core_initialized) { > DRM_DEBUG_DP("DP core already initialized\n"); > return; > @@ -483,8 +487,10 @@ static int dp_display_handle_irq_hpd(struct dp_display_private *dp) > { > u32 sink_request = dp->link->sink_request; > > + DRM_DEBUG_DP("%d\n", sink_request); > if (dp->hpd_state == ST_DISCONNECTED) { > if (sink_request & DP_LINK_STATUS_UPDATED) { > + DRM_DEBUG_DP("Disconnected sink_count:%d\n", sink_request); > DRM_ERROR("Disconnected, no DP_LINK_STATUS_UPDATED\n"); > return -EINVAL; > } > @@ -509,6 +515,7 @@ static int dp_display_usbpd_attention_cb(struct device *dev) > DRM_ERROR("invalid dev\n"); > return -EINVAL; > } > + DRM_DEBUG_DP("sink_request:%d\n", sink_request); Unstick from colon? > > dp = container_of(g_dp_display, > struct dp_display_private, dp_display); > @@ -523,6 +530,7 @@ 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("hpd_state=%d sink_count=%d\n", dp->hpd_state, sink_request); > if (sink_request & DS_PORT_STATUS_CHANGED) > rc = dp_display_handle_port_ststus_changed(dp); > else > @@ -545,6 +553,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("hpd_state=%d\n", state); > if (state == ST_DISPLAY_OFF || state == ST_SUSPENDED) { > mutex_unlock(&dp->event_mutex); > return 0; > @@ -680,6 +689,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("hpd_state=%d\n", state); > /* signal the disconnect event early to ensure proper teardown */ > dp_display_handle_plugged_change(g_dp_display, false); > > @@ -738,6 +748,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("hpd_state=%d\n", state); > > mutex_unlock(&dp->event_mutex); > > @@ -882,6 +893,7 @@ static int dp_display_enable(struct dp_display_private *dp, u32 data) > > dp_display = g_dp_display; > > + DRM_DEBUG_DP("sink_count=%d\n", dp->link->sink_count); > if (dp_display->power_on) { > DRM_DEBUG_DP("Link already setup, return\n"); > return 0; > @@ -943,6 +955,7 @@ static int dp_display_disable(struct dp_display_private *dp, u32 data) > > dp_display->power_on = false; > > + DRM_DEBUG_DP("sink count:%d\n", dp->link->sink_count); > return 0; > } > > @@ -1190,6 +1203,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("hpd isr status=%#x\n", 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..316e8e6 100644 > --- a/drivers/gpu/drm/msm/dp/dp_link.c > +++ b/drivers/gpu/drm/msm/dp/dp_link.c > @@ -1036,43 +1036,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 out; > } > > ret = dp_link_process_ds_port_status_change(link); > if (!ret) { > dp_link->sink_request |= DS_PORT_STATUS_CHANGED; > - return ret; > + goto out; > } > > ret = dp_link_process_link_training_request(link); > if (!ret) { > dp_link->sink_request |= DP_TEST_LINK_TRAINING; > - return ret; > + goto out; > } > > ret = dp_link_process_phy_test_pattern_request(link); > if (!ret) { > dp_link->sink_request |= DP_TEST_LINK_PHY_TEST_PATTERN; > - return ret; > + goto out; > } > > ret = dp_link_process_link_status_update(link); > if (!ret) { > dp_link->sink_request |= DP_LINK_STATUS_UPDATED; > - return ret; > + goto out; > } > > if (dp_link_is_video_pattern_requested(link)) { > - ret = 0; > dp_link->sink_request |= DP_TEST_LINK_VIDEO_PATTERN; > + goto out; > } > > if (dp_link_is_audio_pattern_requested(link)) { > dp_link->sink_request |= DP_TEST_LINK_AUDIO_PATTERN; > - return -EINVAL; > + ret = -EINVAL; > + goto out; > } > > +out: > + DRM_DEBUG_DP("sink request=%#x", dp_link->sink_request); > return ret; I suspect this could be more readable by using if/else-if instead of goto out pattern. Can you make that change instead so we get down to the bottom of this function without using a label? > } > > 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], Drop __func__? Is this needed at all? > + 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..37c214b 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("core_clk_on=%d link_clk_on=%d stream_clk_on=%d\n", > + 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 v2] drm/msm/dp: add logs across DP driver for ease of debugging 2021-07-09 1:27 ` [PATCH v2] drm/msm/dp: add logs across DP driver for ease of debugging Stephen Boyd @ 2021-07-15 21:33 ` maitreye 0 siblings, 0 replies; 3+ messages in thread From: maitreye @ 2021-07-15 21:33 UTC (permalink / raw) To: Stephen Boyd Cc: dri-devel, linux-arm-msm, abhinavk, khsieh, seanpaul, aravindh, freedreno Hi Stephen, Thanks for reviewing my code. On 2021-07-08 18:27, Stephen Boyd wrote: > Quoting maitreye (2021-07-08 12:13:44) >> From: Maitreyee Rao <maitreye@codeaurora.org> >> >> Add trace points across the MSM DP driver to help debug >> interop issues. >> >> Changes in v2: >> - Got rid of redundant log messages. >> - Added %#x instead of 0x%x wherever required. >> - Got rid of __func__ calls in debug messages. >> - Added newline wherever missing. > > I think this is the new thing in v3? Adding one missing newline? > This was actually v2, I forgot to add the resend-patch tag. >> >> Signed-off-by: Maitreyee Rao <maitreye@codeaurora.org> >> --- >> drivers/gpu/drm/msm/dp/dp_catalog.c | 8 ++++++-- >> drivers/gpu/drm/msm/dp/dp_ctrl.c | 5 ++++- >> drivers/gpu/drm/msm/dp/dp_display.c | 14 ++++++++++++++ >> drivers/gpu/drm/msm/dp/dp_link.c | 17 ++++++++++------- >> drivers/gpu/drm/msm/dp/dp_panel.c | 2 ++ >> drivers/gpu/drm/msm/dp/dp_power.c | 3 +++ >> 6 files changed, 39 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c >> b/drivers/gpu/drm/msm/dp/dp_catalog.c >> index 32f3575..292ec2c 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("enable=%d\n", 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("intr_mask=%#x config=%#x\n", 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("aux status:%#x\n", status); > > Unstick colon from printf specifier? > Yup, will do that. >> 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("pattern:%#x\n", pattern); > > Unstick colon from printf specifier? > >> switch (pattern) { >> case DP_PHY_TEST_PATTERN_D10_2: >> dp_write_link(catalog, REG_DP_STATE_CTRL, >> @@ -745,7 +749,7 @@ void dp_catalog_ctrl_send_phy_pattern(struct >> dp_catalog *dp_catalog, >> DP_STATE_CTRL_LINK_TRAINING_PATTERN4); >> break; >> default: >> - DRM_DEBUG_DP("No valid test pattern requested:0x%x\n", >> pattern); >> + DRM_DEBUG_DP("No valid test pattern requested:%#x\n", >> pattern); > > Unstick colon from printf specifier? > Yes, Thank you will fix this. >> break; >> } >> } >> @@ -928,7 +932,7 @@ void dp_catalog_audio_config_acr(struct dp_catalog >> *dp_catalog) >> select = dp_catalog->audio_data; >> acr_ctrl = select << 4 | BIT(31) | BIT(8) | BIT(14); >> >> - DRM_DEBUG_DP("select = 0x%x, acr_ctrl = 0x%x\n", select, >> acr_ctrl); >> + DRM_DEBUG_DP("select =0x%x, acr_ctrl =0x%x\n", select, >> acr_ctrl); > > This doesn't use %#x? And then it sticks it to the equals sign but > doesn't move select and acr_ctrl to them? > You are right , will fix this. >> >> dp_write_link(catalog, MMSS_DP_AUDIO_ACR_CTRL, acr_ctrl); >> } >> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c >> b/drivers/gpu/drm/msm/dp/dp_ctrl.c >> index 2a8955c..21ad7d3 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c >> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c >> @@ -122,7 +122,7 @@ void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl) >> IDLE_PATTERN_COMPLETION_TIMEOUT_JIFFIES)) >> pr_warn("PUSH_IDLE pattern timedout\n"); >> >> - pr_debug("mainlink off done\n"); >> + DRM_DEBUG_DP("PUSH IDLE, mainlink off done\n"); > > I think we already get the func name as dp_ctrl_push_idle in the print > so is having PUSH IDLE in all caps really useful? > You are right, will remove it. >> } >> >> static void dp_ctrl_config_ctrl(struct dp_ctrl_private *ctrl) >> @@ -1013,6 +1013,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("voltage level: %d emphasis level: %d\n", >> voltage_swing_level, >> + pre_emphasis_level); >> ret = dp_catalog_ctrl_update_vx_px(ctrl->catalog, >> voltage_swing_level, pre_emphasis_level); >> >> @@ -1384,6 +1386,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("flip=%d\n", 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..45301c5 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("present=%#x sink_count=%d\n", >> 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("hpd=%d\n", hpd); >> dp_display_send_hpd_event(&dp->dp_display); >> >> return 0; >> @@ -369,6 +372,7 @@ static void dp_display_host_init(struct >> dp_display_private *dp, int reset) >> { >> bool flip = false; >> >> + DRM_DEBUG_DP("core_initialized=%d\n", dp->core_initialized); >> if (dp->core_initialized) { >> DRM_DEBUG_DP("DP core already initialized\n"); >> return; >> @@ -483,8 +487,10 @@ static int dp_display_handle_irq_hpd(struct >> dp_display_private *dp) >> { >> u32 sink_request = dp->link->sink_request; >> >> + DRM_DEBUG_DP("%d\n", sink_request); >> if (dp->hpd_state == ST_DISCONNECTED) { >> if (sink_request & DP_LINK_STATUS_UPDATED) { >> + DRM_DEBUG_DP("Disconnected sink_count:%d\n", >> sink_request); >> DRM_ERROR("Disconnected, no >> DP_LINK_STATUS_UPDATED\n"); >> return -EINVAL; >> } >> @@ -509,6 +515,7 @@ static int dp_display_usbpd_attention_cb(struct >> device *dev) >> DRM_ERROR("invalid dev\n"); >> return -EINVAL; >> } >> + DRM_DEBUG_DP("sink_request:%d\n", sink_request); > > Unstick from colon? > Yeah, will do it. >> >> dp = container_of(g_dp_display, >> struct dp_display_private, dp_display); >> @@ -523,6 +530,7 @@ 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("hpd_state=%d sink_count=%d\n", >> dp->hpd_state, sink_request); >> if (sink_request & DS_PORT_STATUS_CHANGED) >> rc = >> dp_display_handle_port_ststus_changed(dp); >> else >> @@ -545,6 +553,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("hpd_state=%d\n", state); >> if (state == ST_DISPLAY_OFF || state == ST_SUSPENDED) { >> mutex_unlock(&dp->event_mutex); >> return 0; >> @@ -680,6 +689,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("hpd_state=%d\n", state); >> /* signal the disconnect event early to ensure proper teardown >> */ >> dp_display_handle_plugged_change(g_dp_display, false); >> >> @@ -738,6 +748,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("hpd_state=%d\n", state); >> >> mutex_unlock(&dp->event_mutex); >> >> @@ -882,6 +893,7 @@ static int dp_display_enable(struct >> dp_display_private *dp, u32 data) >> >> dp_display = g_dp_display; >> >> + DRM_DEBUG_DP("sink_count=%d\n", dp->link->sink_count); >> if (dp_display->power_on) { >> DRM_DEBUG_DP("Link already setup, return\n"); >> return 0; >> @@ -943,6 +955,7 @@ static int dp_display_disable(struct >> dp_display_private *dp, u32 data) >> >> dp_display->power_on = false; >> >> + DRM_DEBUG_DP("sink count:%d\n", dp->link->sink_count); >> return 0; >> } >> >> @@ -1190,6 +1203,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("hpd isr status=%#x\n", 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..316e8e6 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_link.c >> +++ b/drivers/gpu/drm/msm/dp/dp_link.c >> @@ -1036,43 +1036,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 out; >> } >> >> ret = dp_link_process_ds_port_status_change(link); >> if (!ret) { >> dp_link->sink_request |= DS_PORT_STATUS_CHANGED; >> - return ret; >> + goto out; >> } >> >> ret = dp_link_process_link_training_request(link); >> if (!ret) { >> dp_link->sink_request |= DP_TEST_LINK_TRAINING; >> - return ret; >> + goto out; >> } >> >> ret = dp_link_process_phy_test_pattern_request(link); >> if (!ret) { >> dp_link->sink_request |= >> DP_TEST_LINK_PHY_TEST_PATTERN; >> - return ret; >> + goto out; >> } >> >> ret = dp_link_process_link_status_update(link); >> if (!ret) { >> dp_link->sink_request |= DP_LINK_STATUS_UPDATED; >> - return ret; >> + goto out; >> } >> >> if (dp_link_is_video_pattern_requested(link)) { >> - ret = 0; >> dp_link->sink_request |= DP_TEST_LINK_VIDEO_PATTERN; >> + goto out; >> } >> >> if (dp_link_is_audio_pattern_requested(link)) { >> dp_link->sink_request |= DP_TEST_LINK_AUDIO_PATTERN; >> - return -EINVAL; >> + ret = -EINVAL; >> + goto out; >> } >> >> +out: >> + DRM_DEBUG_DP("sink request=%#x", dp_link->sink_request); >> return ret; > > I suspect this could be more readable by using if/else-if instead of > goto out pattern. Can you make that change instead so we get down to > the > bottom of this function without using a label? > So after reviewing it again, It seems changing to if-else is more complicated because dp_link_process_*** functions, each have different return values and changing it to if-else and handling other cases seems more complicated. >> } >> >> 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], > > Drop __func__? Is this needed at all? > You are right this print message isn't required at all. Will get rid of it. >> + 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..37c214b 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("core_clk_on=%d link_clk_on=%d >> stream_clk_on=%d\n", >> + 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 v2] drm/msm/dp: add logs across DP driver for ease of debugging [not found] <1625771624-11997-1-git-send-email-maitreye@codeaurora.org> 2021-07-09 1:27 ` [PATCH v2] drm/msm/dp: add logs across DP driver for ease of debugging Stephen Boyd @ 2021-07-09 6:27 ` kernel test robot 1 sibling, 0 replies; 3+ messages in thread From: kernel test robot @ 2021-07-09 6:27 UTC (permalink / raw) To: maitreye, dri-devel Cc: clang-built-linux, kbuild-all, Maitreyee Rao, linux-arm-msm, abhinavk, swboyd, khsieh, seanpaul, aravindh, freedreno [-- Attachment #1: Type: text/plain, Size: 3692 bytes --] Hi maitreye, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on drm-intel/for-linux-next] [also build test WARNING on drm-tip/drm-tip drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next linus/master v5.13 next-20210709] [cannot apply to drm/drm-next] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/maitreye/drm-msm-dp-add-logs-across-DP-driver-for-ease-of-debugging/20210709-031606 base: git://anongit.freedesktop.org/drm-intel for-linux-next config: arm-randconfig-r005-20210709 (attached as .config) compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 8d69635ed9ecf36fd0ca85906bfde17949671cbe) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install arm cross compiling tool for clang build # apt-get install binutils-arm-linux-gnueabi # https://github.com/0day-ci/linux/commit/4600296e3781e89ddd188cadf6f62b587a8f4eb9 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review maitreye/drm-msm-dp-add-logs-across-DP-driver-for-ease-of-debugging/20210709-031606 git checkout 4600296e3781e89ddd188cadf6f62b587a8f4eb9 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): >> drivers/gpu/drm/msm/dp/dp_display.c:499:36: warning: variable 'sink_request' is uninitialized when used here [-Wuninitialized] DRM_DEBUG_DP("sink_request:%d\n", sink_request); ^~~~~~~~~~~~ include/drm/drm_print.h:525:31: note: expanded from macro 'DRM_DEBUG_DP' __drm_dbg(DRM_UT_DP, fmt, ## __VA_ARGS__) ^~~~~~~~~~~ drivers/gpu/drm/msm/dp/dp_display.c:492:18: note: initialize the variable 'sink_request' to silence this warning u32 sink_request; ^ = 0 drivers/gpu/drm/msm/dp/dp_display.c:1030:21: warning: variable 'drm' set but not used [-Wunused-but-set-variable] struct drm_device *drm; ^ 2 warnings generated. vim +/sink_request +499 drivers/gpu/drm/msm/dp/dp_display.c 488 489 static int dp_display_usbpd_attention_cb(struct device *dev) 490 { 491 int rc = 0; 492 u32 sink_request; 493 struct dp_display_private *dp; 494 495 if (!dev) { 496 DRM_ERROR("invalid dev\n"); 497 return -EINVAL; 498 } > 499 DRM_DEBUG_DP("sink_request:%d\n", sink_request); 500 501 dp = container_of(g_dp_display, 502 struct dp_display_private, dp_display); 503 504 /* check for any test request issued by sink */ 505 rc = dp_link_process_request(dp->link); 506 if (!rc) { 507 sink_request = dp->link->sink_request; 508 DRM_DEBUG_DP("hpd_state=%d sink_count=%d\n", dp->hpd_state, sink_request); 509 if (sink_request & DS_PORT_STATUS_CHANGED) 510 rc = dp_display_handle_port_ststus_changed(dp); 511 else 512 rc = dp_display_handle_irq_hpd(dp); 513 } 514 515 return rc; 516 } 517 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 38051 bytes --] ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-07-15 21:33 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1625771624-11997-1-git-send-email-maitreye@codeaurora.org>
2021-07-09 1:27 ` [PATCH v2] drm/msm/dp: add logs across DP driver for ease of debugging Stephen Boyd
2021-07-15 21:33 ` maitreye
2021-07-09 6:27 ` kernel test robot
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).