[v4] drm/msm/dp: add logs across DP driver for ease of debugging

Message ID	1626909581-2887-1-git-send-email-maitreye@codeaurora.org (mailing list archive)
State	Superseded
Headers	show Return-Path: <linux-arm-msm-owner@kernel.org> Date: Subject: Cc: To: From: Sender; bh=qW7+aGb27WgSrS+4SpFtcHSdTczASjKRcKc+fgJwpNY=; b=RZNes5USt12tE81PC3hZvhPEJ5mHUmq8PkGJ0UDvWTSnhrkQOUDXMRi1oxpDXjZddZHRnFDO zAzK3ida5zU3Xm8jEnBSVxHvu9cNbWdfej6+Yx917BNtRYNJc1xDLRZLwzYgp1Xma598bQT1 CBs7iWMe/itf3PKoCZ0WFYFZtqs= Sender: maitreye=codeaurora.org@mg.codeaurora.org sender: maitreye) by smtp.codeaurora.org (Postfix) with ESMTPSA id 0600AC433D3; Wed, 21 Jul 2021 23:19:49 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 0600AC433D3 From: maitreye <maitreye@codeaurora.org> To: dri-devel@lists.freedesktop.org Cc: Maitreyee Rao <maitreye@codeaurora.org>, linux-arm-msm@vger.kernel.org, freedreno@lists.freedesktop.org, robdclark@gmail.com, seanpaul@chromium.org, swboyd@chromium.org, nganji@codeaurora.org, aravindh@codeaurora.org, khsieh@codeaurora.org, abhinavk@codeaurora.org Subject: [PATCH v4] drm/msm/dp: add logs across DP driver for ease of debugging Date: Wed, 21 Jul 2021 16:19:40 -0700 Message-Id: <1626909581-2887-1-git-send-email-maitreye@codeaurora.org> Precedence: bulk
Series	[v4] drm/msm/dp: add logs across DP driver for ease of debugging \| expand [v4] drm/msm/dp: add logs across DP driver for ease of debugging

Quoting maitreye@codeaurora.org (2021-07-22 20:53:37) > On 2021-07-22 15:09, Stephen Boyd wrote: > Thank you for the comments . > > Quoting maitreye@codeaurora.org (2021-07-22 14:33:43) > >> Thank you Stephen. > >> > >> On 2021-07-22 13:31, Stephen Boyd wrote: > >> > Quoting maitreye (2021-07-21 16:19:40) > >> >> From: Maitreyee Rao <maitreye@codeaurora.org> > >> >> > >> >> Add trace points across the MSM DP driver to help debug > >> >> interop issues. > >> >> > >> >> Changes in v4: > >> >> - Changed goto statement and used if else-if > >> > > >> > I think drm likes to see all the changelog here to see patch evolution. > >> > > >> Yes, I will fix this > >> >> > >> >> Signed-off-by: Maitreyee Rao <maitreye@codeaurora.org> > >> >> --- > >> >> diff --git a/drivers/gpu/drm/msm/dp/dp_link.c > >> >> b/drivers/gpu/drm/msm/dp/dp_link.c > >> >> index be986da..8c98ab7 100644 > >> >> --- a/drivers/gpu/drm/msm/dp/dp_link.c > >> >> +++ b/drivers/gpu/drm/msm/dp/dp_link.c > >> >> @@ -1036,43 +1036,28 @@ 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; > >> >> } > >> >> - > >> >> - ret = dp_link_process_ds_port_status_change(link); > >> >> - if (!ret) { > >> >> + else if (!(ret = dp_link_process_ds_port_status_change(link))) > >> >> { > >> >> dp_link->sink_request |= DS_PORT_STATUS_CHANGED; > >> >> - return ret; > >> >> } > >> >> - > >> >> - ret = dp_link_process_link_training_request(link); > >> >> - if (!ret) { > >> >> + else if (!(ret = dp_link_process_link_training_request(link))) > >> >> { > >> >> dp_link->sink_request |= DP_TEST_LINK_TRAINING; > >> >> - return ret; > >> >> } > >> >> - > >> >> - ret = dp_link_process_phy_test_pattern_request(link); > >> >> - if (!ret) { > >> >> + else if (!(ret = > >> >> dp_link_process_phy_test_pattern_request(link))) { > >> >> dp_link->sink_request |= > >> >> DP_TEST_LINK_PHY_TEST_PATTERN; > >> >> - return ret; > >> >> - } > >> >> - > >> >> - ret = dp_link_process_link_status_update(link); > >> >> - if (!ret) { > >> >> + } > >> >> + else if (!(ret = dp_link_process_link_status_update(link))) { > >> > > >> > The kernel coding style is to leave the brackets on the same line > >> > > >> > if (condition) { > >> > > >> > } else if (conditon) { > >> > > >> > } > >> > > >> > See Documentation/process/coding-style.rst > >> > > >> Yes, I will fix this > >> > >> > Also, the if (!(ret = dp_link_...)) style is really hard to read. Maybe > >> > apply this patch before? > >> > > >> > ----8<---- > >> > diff --git a/drivers/gpu/drm/msm/dp/dp_link.c > >> > b/drivers/gpu/drm/msm/dp/dp_link.c > >> > index 1195044a7a3b..408cddd90f0f 100644 > >> > --- a/drivers/gpu/drm/msm/dp/dp_link.c > >> > +++ b/drivers/gpu/drm/msm/dp/dp_link.c > >> > @@ -1027,41 +1027,22 @@ 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; > >> > - } > >> > - > >> > - ret = dp_link_process_ds_port_status_change(link); > >> > - if (!ret) { > >> > + } else if (!dp_link_process_ds_port_status_change(link)) { > >> > dp_link->sink_request |= DS_PORT_STATUS_CHANGED; > >> > - return ret; > >> > - } > >> > - > >> > - ret = dp_link_process_link_training_request(link); > >> > - if (!ret) { > >> > + } else if (!dp_link_process_link_training_request(link)) { > >> > dp_link->sink_request |= DP_TEST_LINK_TRAINING; > >> > - return ret; > >> > - } > >> > - > >> > - ret = dp_link_process_phy_test_pattern_request(link); > >> > - if (!ret) { > >> > + } else if (!dp_link_process_phy_test_pattern_request(link)) { > >> > dp_link->sink_request |= DP_TEST_LINK_PHY_TEST_PATTERN; > >> > - return ret; > >> > - } > >> > - > >> > - ret = dp_link_process_link_status_update(link); > >> > - if (!ret) { > >> > + } else if (!dp_link_process_link_status_update(link)) { > >> > dp_link->sink_request |= DP_LINK_STATUS_UPDATED; > >> > - return ret; > >> > - } > >> > - > >> > - if (dp_link_is_video_pattern_requested(link)) { > >> > - ret = 0; > >> > - dp_link->sink_request |= DP_TEST_LINK_VIDEO_PATTERN; > >> > - } > >> > + } else { > >> > + if (dp_link_is_video_pattern_requested(link)) > >> > + dp_link->sink_request |= DP_TEST_LINK_VIDEO_PATTERN; > >> > > >> > - if (dp_link_is_audio_pattern_requested(link)) { > >> > - dp_link->sink_request |= DP_TEST_LINK_AUDIO_PATTERN; > >> > - return -EINVAL; > >> > + if (dp_link_is_audio_pattern_requested(link)) { > >> > + dp_link->sink_request |= DP_TEST_LINK_AUDIO_PATTERN; > >> > + ret = -EINVAL; > >> > + } > >> > } > >> > > >> > return ret; > >> The reason I did this was to preserve the value of ret as the caller > >> of > >> the function checks it. Some functions return -EINVAl like in here: > >> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/dp/dp_link.c#L972 > >> , so to check that it would be necessary to get the ret value. > > > > ret is overwritten multiple times. The logic seems to be if ret is > > not-zero, reassign it, until we get to the end. How about this > > > > ----8<---- > > diff --git a/drivers/gpu/drm/msm/dp/dp_link.c > > b/drivers/gpu/drm/msm/dp/dp_link.c > > index 1195044a7a3b..e59138566c0a 100644 > > --- a/drivers/gpu/drm/msm/dp/dp_link.c > > +++ b/drivers/gpu/drm/msm/dp/dp_link.c > > @@ -1027,41 +1027,27 @@ 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; > > - } > > - > > - ret = dp_link_process_ds_port_status_change(link); > > - if (!ret) { > > + } else if (!dp_link_process_ds_port_status_change(link)) { > > dp_link->sink_request |= DS_PORT_STATUS_CHANGED; > > - return ret; > > - } > > - > > - ret = dp_link_process_link_training_request(link); > > - if (!ret) { > > + } else if (!dp_link_process_link_training_request(link)) { > > dp_link->sink_request |= DP_TEST_LINK_TRAINING; > > - return ret; > > - } > > - > > - ret = dp_link_process_phy_test_pattern_request(link); > > - if (!ret) { > > + } else if (!dp_link_process_phy_test_pattern_request(link)) { > > dp_link->sink_request |= DP_TEST_LINK_PHY_TEST_PATTERN; > > - return ret; > > - } > > - > > - ret = dp_link_process_link_status_update(link); > > - if (!ret) { > > - dp_link->sink_request |= DP_LINK_STATUS_UPDATED; > > - return ret; > > - } > > - > > - if (dp_link_is_video_pattern_requested(link)) { > > - ret = 0; > > - dp_link->sink_request |= DP_TEST_LINK_VIDEO_PATTERN; > > - } > > - > > - if (dp_link_is_audio_pattern_requested(link)) { > > - dp_link->sink_request |= DP_TEST_LINK_AUDIO_PATTERN; > > - return -EINVAL; > > + } else { > > + ret = dp_link_process_link_status_update(link); > > + if (!ret) { > > + dp_link->sink_request |= DP_LINK_STATUS_UPDATED; > > + } else { > > + if (dp_link_is_video_pattern_requested(link)) { > > + ret = 0; > > + dp_link->sink_request |= DP_TEST_LINK_VIDEO_PATTERN; > > + } > > + > > + if (dp_link_is_audio_pattern_requested(link)) { > > + dp_link->sink_request |= DP_TEST_LINK_AUDIO_PATTERN; > > + ret = -EINVAL; > > + } > > + } > > } > > > > return ret; > I do agree, this will solve the issue for over writing ret variable. But > I can see two potential problems with this, if we get two events at the > same time or one of the other we might end up processing the two events, > which won't be the expected behavior. The code isn't written to handle this. Does it need to handle it? If so, please update it for that new feature in a different patch than this one that adds logging. >For example, if we get > dp_link_process_link_status_update and it gives a non zero result and we > go to the else statement, and get the event for video pattern request, > we will end up processing two events. > The second problem, is currently > we only have APIs that return 0 or EINVAL as return values, but if we > ever in future add an API with a different return value it wont be easy > to add the behavior. Agreed. Fortunately it's just code and it can easily be rewritten in the future when it changes. > But if these issues seem like they can be ignored , > then we can go ahead with your suggestions. I think they can be ignored if we're not supposed to handle two events at the same time.

diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index 32f3575..958d3fa3 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); 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); 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); 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: %#x, acr_ctrl: %#x\n", select, acr_ctrl); 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..72de71a 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("mainlink off done\n"); } 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..f0a81f7 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); 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..8c98ab7 100644 --- a/drivers/gpu/drm/msm/dp/dp_link.c +++ b/drivers/gpu/drm/msm/dp/dp_link.c @@ -1036,43 +1036,28 @@ 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; } - - ret = dp_link_process_ds_port_status_change(link); - if (!ret) { + else if (!(ret = dp_link_process_ds_port_status_change(link))) { dp_link->sink_request |= DS_PORT_STATUS_CHANGED; - return ret; } - - ret = dp_link_process_link_training_request(link); - if (!ret) { + else if (!(ret = dp_link_process_link_training_request(link))) { dp_link->sink_request |= DP_TEST_LINK_TRAINING; - return ret; } - - ret = dp_link_process_phy_test_pattern_request(link); - if (!ret) { + else if (!(ret = dp_link_process_phy_test_pattern_request(link))) { dp_link->sink_request |= DP_TEST_LINK_PHY_TEST_PATTERN; - return ret; - } - - ret = dp_link_process_link_status_update(link); - if (!ret) { + } + else if (!(ret = dp_link_process_link_status_update(link))) { dp_link->sink_request |= DP_LINK_STATUS_UPDATED; - return ret; } - - if (dp_link_is_video_pattern_requested(link)) { - ret = 0; + else if (dp_link_is_video_pattern_requested(link)) { dp_link->sink_request |= DP_TEST_LINK_VIDEO_PATTERN; } - - if (dp_link_is_audio_pattern_requested(link)) { + else if (dp_link_is_audio_pattern_requested(link)) { dp_link->sink_request |= DP_TEST_LINK_AUDIO_PATTERN; - return -EINVAL; + ret = -EINVAL; } - + + DRM_DEBUG_DP("sink request=%#x", dp_link->sink_request); return ret; } 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;

[v4] drm/msm/dp: add logs across DP driver for ease of debugging

Commit Message

Comments

Patch