Message-ID: <d2e118a85bac5ce2be7882b6339ebce563d37953.camel@mediatek.com>
Date: Mon, 4 Aug 2025 05:32:49 +0000
From: CK Hu (胡俊光) <ck.hu@...iatek.com>
To: "towwy321@...il.com" <towwy321@...il.com>, "simona@...ll.ch"
	<simona@...ll.ch>, "chunkuang.hu@...nel.org" <chunkuang.hu@...nel.org>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	"airlied@...il.com" <airlied@...il.com>, "granquet@...libre.com"
	<granquet@...libre.com>, "msp@...libre.com" <msp@...libre.com>,
	"p.zabel@...gutronix.de" <p.zabel@...gutronix.de>, "matthias.bgg@...il.com"
	<matthias.bgg@...il.com>
CC: "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-mediatek@...ts.infradead.org"
	<linux-mediatek@...ts.infradead.org>, "dri-devel@...ts.freedesktop.org"
	<dri-devel@...ts.freedesktop.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, Rex-BC Chen (陳柏辰)
	<Rex-BC.Chen@...iatek.com>
Subject: Re: [PATCH] drm/mediatek: dp: Fix suspend/resume training failure

On Sun, 2025-08-03 at 18:32 +0800, Haru Zheng wrote:
> External email : Please do not click links or open attachments until you have verified the sender or the content.
> 
> 
> From 4919bd858cd9cba8e4aadba7c3d1fd434ef3b09e Mon Sep 17 00:00:00 2001
> From: Haru Zheng <towwy321@...il.com>
> Date: Wed, 18 Jun 2025 11:28:37 +0800
> Subject: [PATCH] drm/mediatek: dp: Fix suspend/resume training failure
> 
> When suspending and resuming DisplayPort via Type-C,
> link training may fail.
> 
> This patch backports the software IRQ handling for DP,
> as eDP uses hardware IRQ while DP uses software IRQ.
> Additionally, cable_plugged_in is flipped in
> mtk_dp_hpd_event to ensure correct hotplug detection
> during resume.
> 
> These changes fix the DP training failure after suspend/resume.
> 
> Fixes: f70ac097a2cf ("drm/mediatek: Add MT8195 Embedded DisplayPort driver")
> Tested-on: Genio G700 EVK
> Signed-off-by: Haru Zheng <towwy321@...il.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dp.c     | 100 ++++++++++++++++++++++++--
>  drivers/gpu/drm/mediatek/mtk_dp_reg.h |   3 +
>  2 files changed, 96 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c
> b/drivers/gpu/drm/mediatek/mtk_dp.c
> index bef6eeb30d3e..0cfe792bc36d 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
> @@ -1012,6 +1012,16 @@ static u32 mtk_dp_swirq_get_clear(struct mtk_dp *mtk_dp)
>   return irq_status;
>  }
> 
> +static void mtk_dp_swirq_enable(struct mtk_dp *mtk_dp, bool enable)
> +{
> + if (enable)

Use 'tab' instead of white space for indent.

> + mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_35C4, 0,
> +    SW_IRQ_FINAL_STATUS_DP_TRANS_P0_MASK);
> + else
> + mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_35C4, 0xFFFF,
> +    SW_IRQ_FINAL_STATUS_DP_TRANS_P0_MASK);

Simplify this as:

mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_35C4, enable ? 0 : 0xffff,
		   SW_IRQ_FINAL_STATUS_DP_TRANS_P0_MASK);

> +}
> +
>  static u32 mtk_dp_hwirq_get_clear(struct mtk_dp *mtk_dp)
>  {
>   u32 irq_status = (mtk_dp_read(mtk_dp, MTK_DP_TRANS_P0_3418) &
> @@ -1751,6 +1761,8 @@ static int mtk_dp_parse_capabilities(struct
> mtk_dp *mtk_dp)
>       mtk_dp->train_info.sink_ssc)
>   return 0;
> 
> + memset(mtk_dp->rx_cap, 0, DP_RECEIVER_CAP_SIZE);

This is not necessary. Drop it.

> +
>   ret = drm_dp_read_dpcd_caps(&mtk_dp->aux, mtk_dp->rx_cap);
>   if (ret < 0)
>   return ret;
> @@ -2031,8 +2043,8 @@ static irqreturn_t mtk_dp_hpd_event(int hpd, void *dev)
>   spin_unlock_irqrestore(&mtk_dp->irq_thread_lock, flags);
> 
>   if (cable_sta_chg) {
> - if (!!(mtk_dp_read(mtk_dp, MTK_DP_TRANS_P0_3414) &
> -        HPD_DB_DP_TRANS_P0_MASK))
> + if (!(mtk_dp_read(mtk_dp, MTK_DP_TRANS_P0_3414) &
> +       HPD_DB_DP_TRANS_P0_MASK))

Why the definition is changed?

>   mtk_dp->train_info.cable_plugged_in = true;
>   else
>   mtk_dp->train_info.cable_plugged_in = false;
> @@ -2265,6 +2277,41 @@ static ssize_t mtk_dp_aux_transfer(struct
> drm_dp_aux *mtk_aux,
>   return ret;
>  }
> 
> +static void mtk_dp_swirq_hpd(struct mtk_dp *mtk_dp, u8 conn)
> +{
> + u32 data;
> +
> + data = mtk_dp_read(mtk_dp, MTK_DP_TRANS_P0_3414);
> +
> + mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_3414,
> +    HPD_OVR_EN_DP_TRANS_P0_MASK,
> +    HPD_OVR_EN_DP_TRANS_P0_MASK);
> +
> + if (conn)
> + mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_3414,
> +    HPD_SET_DP_TRANS_P0_MASK,
> +    HPD_SET_DP_TRANS_P0_MASK);
> + else
> + mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_3414,
> +    0,
> +    HPD_SET_DP_TRANS_P0_MASK);
> +}
> +
> +static void mtk_dp_swirq_hpd_interrupt_set(struct mtk_dp *mtk_dp, u8 status)
> +{
> + dev_info(mtk_dp->dev, "[DPTX] status:%d [2:DISCONNECT, 4:CONNECT]\n", status);

Use dev_dbg() instead of dev_info().
I do not like many log when nothing wrong.
Log would cost CPU usage and reduce the performance.

> +
> + if (status == MTK_DP_HPD_CONNECT) {
> + mtk_dp_init_port(mtk_dp);

In mtk_dp_resume(), mtk_dp_init_port() is already called.
Do not call it again here.

> + mtk_dp_swirq_hpd(mtk_dp, TRUE);
> + } else {
> + mtk_dp_swirq_hpd(mtk_dp, FALSE);
> + }
> +
> + mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_35C0, status,
> +    SW_IRQ_SET_DP_TRANS_P0_MASK);
> +}
> +
>  static int mtk_dp_poweron(struct mtk_dp *mtk_dp)
>  {
>   int ret;
> @@ -2534,7 +2581,7 @@ static int mtk_dp_bridge_atomic_check(struct
> drm_bridge *bridge,
> 
>   dev_dbg(mtk_dp->dev, "input format 0x%04x, output format 0x%04x\n",
>   bridge_state->input_bus_cfg.format,
> - bridge_state->output_bus_cfg.format);
> + bridge_state->output_bus_cfg.format);
> 
>   if (input_bus_format == MEDIA_BUS_FMT_YUYV8_1X16)
>   mtk_dp->info.format = DP_PIXELFORMAT_YUV422;
> @@ -2552,6 +2599,30 @@ static int mtk_dp_bridge_atomic_check(struct
> drm_bridge *bridge,
>   return 0;
>  }
> 
> +static void mtk_dp_bridge_hpd_notify(struct drm_bridge *bridge,
> +      enum drm_connector_status status)

mtk_dp_bridge_hpd_notify() is useless. Drop it.

> +{
> + struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge);
> + struct mtk_dp_train_info *train_info = &mtk_dp->train_info;
> +
> + if (mtk_dp->bridge.type != DRM_MODE_CONNECTOR_eDP) {
> + if (mtk_dp->hpd_state != status) {
> + if (status == connector_status_disconnected) {
> + train_info->cable_plugged_in = false;
> + } else {
> + mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_3414,
> +    HPD_OVR_EN_DP_TRANS_P0_MASK,
> +    HPD_OVR_EN_DP_TRANS_P0_MASK);
> + mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_3414,
> +    HPD_SET_DP_TRANS_P0_MASK,
> +    HPD_SET_DP_TRANS_P0_MASK);
> + train_info->cable_plugged_in = true;
> + }
> + mtk_dp->hpd_state = status;
> + }
> + }
> +}
> +
>  static const struct drm_bridge_funcs mtk_dp_bridge_funcs = {
>   .atomic_check = mtk_dp_bridge_atomic_check,
>   .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> @@ -2801,7 +2872,8 @@ static int mtk_dp_probe(struct platform_device *pdev)
>   mtk_dp_power_enable(mtk_dp);
> 
>   /* Disable HW interrupts: we don't need any for eDP */
> - mtk_dp_hwirq_enable(mtk_dp, false);
> + mtk_dp_hwirq_enable(mtk_dp, true);

eDP does not need IRQ. Why do you enable it?

> + mtk_dp_swirq_enable(mtk_dp, false);
> 
>   /*
>   * Power on the AUX to allow reading the EDID from aux-bus:
> @@ -2829,6 +2901,9 @@ static int mtk_dp_probe(struct platform_device *pdev)
>   }
>   }
>   } else {
> + mtk_dp_swirq_enable(mtk_dp, false);
> + mtk_dp_hwirq_enable(mtk_dp, false);
> + mtk_dp_swirq_enable(mtk_dp, true);

For DP, irq enable when bridge attach.
Do not enable here.

>   mtk_dp->bridge.ops = DRM_BRIDGE_OP_DETECT |
>        DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_HPD;
>   ret = devm_drm_bridge_add(dev, &mtk_dp->bridge);
> @@ -2861,10 +2936,15 @@ static int mtk_dp_suspend(struct device *dev)
>   struct mtk_dp *mtk_dp = dev_get_drvdata(dev);
> 
>   mtk_dp_power_disable(mtk_dp);
> - if (mtk_dp->bridge.type != DRM_MODE_CONNECTOR_eDP)
> +
> + if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP) {
>   mtk_dp_hwirq_enable(mtk_dp, false);

This patch is for dp not edp, right?
Why do you change behavior of edp?

> - pm_runtime_put_sync(dev);
> + } else {
> + mtk_dp_swirq_hpd_interrupt_set(mtk_dp, MTK_DP_HPD_DISCONNECT);
> + mtk_dp_swirq_enable(mtk_dp, false);
> + }
> 
> + pm_runtime_put_sync(dev);
>   return 0;
>  }
> 
> @@ -2874,8 +2954,14 @@ static int mtk_dp_resume(struct device *dev)
> 
>   pm_runtime_get_sync(dev);
>   mtk_dp_init_port(mtk_dp);
> - if (mtk_dp->bridge.type != DRM_MODE_CONNECTOR_eDP)
> +
> + if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP) {
>   mtk_dp_hwirq_enable(mtk_dp, true);

This patch is for dp not edp, right?
Why do you change behavior of edp?

> + } else {
> + mtk_dp_swirq_hpd_interrupt_set(mtk_dp, MTK_DP_HPD_CONNECT);
> + mtk_dp_swirq_enable(mtk_dp, true);

It seems that you want to notify connection when resume.
You could notify in software flow and do not need an IRQ to do this.
Try to do it in software flow.
If you have any problem, let's discuss.

Regards,
CK

> + }
> +
>   mtk_dp_power_enable(mtk_dp);
> 
>   return 0;
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp_reg.h
> b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
> index 8ad7a9cc259e..7c97e230be50 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp_reg.h
> +++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
> @@ -286,7 +286,10 @@
>  #define POST_MISC_DATA_LANE1_OV_DP_TRANS_P0_MASK BIT(9)
>  #define POST_MISC_DATA_LANE2_OV_DP_TRANS_P0_MASK BIT(10)
>  #define POST_MISC_DATA_LANE3_OV_DP_TRANS_P0_MASK BIT(11)
> +#define MTK_DP_TRANS_P0_35C0 0x35c0
> +#define MTK_DP_TRANS_P0_35C4 0x35c4
>  #define MTK_DP_TRANS_P0_35C8 0x35c8
> +#define SW_IRQ_SET_DP_TRANS_P0_MASK GENMASK(15, 0)
>  #define SW_IRQ_CLR_DP_TRANS_P0_MASK GENMASK(15, 0)
>  #define SW_IRQ_STATUS_DP_TRANS_P0_MASK GENMASK(15, 0)
>  #define MTK_DP_TRANS_P0_35D0 0x35d0
> --
> 2.45.2

Powered by blists - more mailing lists