* 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
[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
* 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
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).