From: Stephen Boyd <swboyd@chromium.org>
To: dri-devel@lists.freedesktop.org, maitreye <maitreye@codeaurora.org>
Cc: linux-arm-msm@vger.kernel.org, freedreno@lists.freedesktop.org,
robdclark@gmail.com, seanpaul@chromium.org,
nganji@codeaurora.org, aravindh@codeaurora.org,
khsieh@codeaurora.org, abhinavk@codeaurora.org
Subject: Re: [PATCH] drm/msm/dp: add logs across DP driver for ease of debugging
Date: Thu, 17 Jun 2021 15:33:22 -0700 [thread overview]
Message-ID: <CAE-0n51UCvxCbB0MTznyAiZ+qoi3_fe6FJoW3+NZ0QL-P+6u4w@mail.gmail.com> (raw)
In-Reply-To: <1623892134-20447-1-git-send-email-maitreye@codeaurora.org>
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;
>
WARNING: multiple messages have this Message-ID (diff)
From: Stephen Boyd <swboyd@chromium.org>
To: dri-devel@lists.freedesktop.org, maitreye <maitreye@codeaurora.org>
Cc: linux-arm-msm@vger.kernel.org, abhinavk@codeaurora.org,
khsieh@codeaurora.org, robdclark@gmail.com,
seanpaul@chromium.org, aravindh@codeaurora.org,
freedreno@lists.freedesktop.org
Subject: Re: [PATCH] drm/msm/dp: add logs across DP driver for ease of debugging
Date: Thu, 17 Jun 2021 15:33:22 -0700 [thread overview]
Message-ID: <CAE-0n51UCvxCbB0MTznyAiZ+qoi3_fe6FJoW3+NZ0QL-P+6u4w@mail.gmail.com> (raw)
In-Reply-To: <1623892134-20447-1-git-send-email-maitreye@codeaurora.org>
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;
>
next prev parent reply other threads:[~2021-06-17 22:33 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2021-06-17 22:33 ` Stephen Boyd
2021-06-28 17:14 ` maitreye
2021-06-28 17:14 ` maitreye
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAE-0n51UCvxCbB0MTznyAiZ+qoi3_fe6FJoW3+NZ0QL-P+6u4w@mail.gmail.com \
--to=swboyd@chromium.org \
--cc=abhinavk@codeaurora.org \
--cc=aravindh@codeaurora.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=freedreno@lists.freedesktop.org \
--cc=khsieh@codeaurora.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=maitreye@codeaurora.org \
--cc=nganji@codeaurora.org \
--cc=robdclark@gmail.com \
--cc=seanpaul@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.