lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <d0aa557c16b9fab64253a2a52c2eb66a76029b4b.camel@mediatek.com>
Date: Tue, 5 Aug 2025 03:52:32 +0000
From: CK Hu (胡俊光) <ck.hu@...iatek.com>
To: "towwy321@...il.com" <towwy321@...il.com>, "chunkuang.hu@...nel.org"
	<chunkuang.hu@...nel.org>, "simona@...ll.ch" <simona@...ll.ch>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	"airlied@...il.com" <airlied@...il.com>, "msp@...libre.com"
	<msp@...libre.com>, "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
	"matthias.bgg@...il.com" <matthias.bgg@...il.com>, "granquet@...libre.com"
	<granquet@...libre.com>
CC: Rex-BC Chen (陳柏辰) <Rex-BC.Chen@...iatek.com>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] drm/mediatek: dp: Fix suspend/resume training failure

On Tue, 2025-08-05 at 00:03 +0800, Haru Zheng wrote:
> External email : Please do not click links or open attachments until you have verified the sender or the content.
> 
> 
> When suspending and resuming DisplayPort via Type-C,
> link training will be 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")
> Signed-off-by: Haru Zheng <towwy321@...il.com>
> 
> ---
> 
> Changes since v1:
> - Fixed indentation to use tabs
> - Simplified swirq_enable() logic with ternary
> - Removed unnecessary memset()
> - Replaced dev_info() with dev_dbg()
> - Add mtk_dp_bridge_hpd_notify() declaration to struct drm_bridge_funcs mtk_dp_bridge_funcs
> - Removed IRQ enable from probe() to avoid enabling IRQ for eDP
> - Corrected HW/SW IRQ logic:
>   * eDP uses hardware IRQ, DP uses software IRQ
>   * Previously some logic was reversed causing issues
> - Fixed hotplug detection logic in mtk_dp_hpd_event:
>   * cable_plugged_in flag inverted to ensure correct detection on resume
> 
> Code flow:
> - On suspend, DP disables training and IRQs accordingly.
> - On resume, IRQs for DP are re-enabled via software IRQ handler.
> - HPD events are processed with correct plug/unplug state, ensuring training succeeds.
> 
> ---
>  drivers/gpu/drm/mediatek/mtk_dp.c     | 94 ++++++++++++++++++++++++---
>  drivers/gpu/drm/mediatek/mtk_dp_reg.h |  3 +
>  2 files changed, 87 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index bef6eeb30d3e..b0f96c7c279e 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
> @@ -1012,6 +1012,12 @@ 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)
> +{
> +       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) &
> @@ -2031,8 +2037,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 do you change this.
Original code works normally.
This change may cause problem.

>                         mtk_dp->train_info.cable_plugged_in = true;
>                 else
>                         mtk_dp->train_info.cable_plugged_in = false;
> @@ -2252,7 +2258,7 @@ static ssize_t mtk_dp_aux_transfer(struct drm_dp_aux *mtk_aux,
>                                              to_access, &msg->reply);
> 
>                 if (ret) {
> -                       dev_info(mtk_dp->dev,
> +                       dev_dbg(mtk_dp->dev,
>                                  "Failed to do AUX transfer: %d\n", ret);

This is error case. Do not use dev_dbg().

>                         goto err;
>                 }
> @@ -2265,6 +2271,35 @@ 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);

data is useless. Drop it.

> +
> +       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);

Where do you define 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);

Where do you define 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_dbg(mtk_dp->dev, "[DPTX] status:%d [2:DISCONNECT, 4:CONNECT]\n", status);
> +
> +       mtk_dp_swirq_hpd(mtk_dp, status == MTK_DP_HPD_CONNECT ? TRUE : 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;
> @@ -2326,7 +2361,13 @@ static int mtk_dp_bridge_attach(struct drm_bridge *bridge,
>         if (mtk_dp->bridge.type != DRM_MODE_CONNECTOR_eDP) {
>                 irq_clear_status_flags(mtk_dp->irq, IRQ_NOAUTOEN);
>                 enable_irq(mtk_dp->irq);
> +               /* eDP does use HW IRQs */

'mtk_dp->bridge.type != DRM_MODE_CONNECTOR_eDP' means here is DP.
The comment is wrong.

>                 mtk_dp_hwirq_enable(mtk_dp, true);
> +               mtk_dp_swirq_enable(mtk_dp, false);
> +       } else {
> +               /* DP does use SW IRQs */

Here is eDP.
eDP does not need IRQ.
If you want to use IRQ, add comment to describe why you need.

> +               mtk_dp_hwirq_enable(mtk_dp, false);
> +               mtk_dp_swirq_enable(mtk_dp, true);
>         }
> 
>         return 0;
> @@ -2534,7 +2575,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);

This is a cleanup modification.
This is not related to the title.
If you want to modify this, collect all cleanup in this file and separate to another patch.

> 
>         if (input_bus_format == MEDIA_BUS_FMT_YUYV8_1X16)
>                 mtk_dp->info.format = DP_PIXELFORMAT_YUV422;
> @@ -2552,6 +2593,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)

Why add this.
Original code could work normally.
If this is not related to the title, separate this to another patch.

> +{
> +       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,
> @@ -2566,6 +2631,7 @@ static const struct drm_bridge_funcs mtk_dp_bridge_funcs = {
>         .mode_valid = mtk_dp_bridge_mode_valid,
>         .edid_read = mtk_dp_edid_read,
>         .detect = mtk_dp_bdg_detect,
> +       .hpd_notify = mtk_dp_bridge_hpd_notify,
>  };
> 
>  static void mtk_dp_debounce_timer(struct timer_list *t)
> @@ -2800,9 +2866,6 @@ static int mtk_dp_probe(struct platform_device *pdev)
>                 mtk_dp_initialize_aux_settings(mtk_dp);
>                 mtk_dp_power_enable(mtk_dp);
> 
> -               /* Disable HW interrupts: we don't need any for eDP */
> -               mtk_dp_hwirq_enable(mtk_dp, false);
> -
>                 /*
>                  * Power on the AUX to allow reading the EDID from aux-bus:
>                  * please note that it is necessary to call power off in the
> @@ -2861,10 +2924,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) {

eDP does not need IRQ.
If you want to use IRQ, add comment to describe why you need.

>                 mtk_dp_hwirq_enable(mtk_dp, false);
> -       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 +2942,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);

eDP does not need IRQ.
If you want to use IRQ, add comment to describe why you need.

Regards,
CK

> +       } else {
> +               mtk_dp_swirq_hpd_interrupt_set(mtk_dp, MTK_DP_HPD_CONNECT);
> +               mtk_dp_swirq_enable(mtk_dp, true);
> +       }
> +
>         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.23.0
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ