On 7/9/26 14:11, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 3 potential issue(s)
to consider:
New issues:
- [High] Flawed hardware reset sequence and register mask conflict for the new
IP (hwip_v1p4) in mtk_dp_encoder_reset().
- [High] Uninitialized calibration data is applied to PHY registers on MT8196
if a legacy device tree is used.
Pre-existing issues:
- [High] Sleeping functions called from atomic context in bridge atomic
callbacks.
None of those are valid, if you want more context, check below.
--
--- Patch [11]: [PATCH v4 11/12] drm/mediatek: mtk_dp: Add support for eDP1.5
IPs and MT8196 SoC ---
commit 21e6cb76ec5d736e41c62351d0dcc4f501bc644f
Author: AngeloGioacchino Del Regno <[email protected]>
drm/mediatek: mtk_dp: Add support for eDP1.5 IPs and MT8196 SoC
Add support for MediaTek's Embedded DisplayPort (eDP) v1.5 IP as
found in the MT8196 SoC and other variants, and enclose some of
the new register writes in specific platform data booleans.
--- a/drivers/gpu/drm/mediatek/mtk_dp_reg.h
+++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
[ ... ]
+#define REG_3FF8_DP_ENC_P0_3 0x1ff8
+#define ENC_PHY_STATE_RESET_ALL_MASK GENMASK(7, 0)
+#define PHY_STATE_W_1_DP_ENC_P0_3 BIT(6)
+#define DP_ENCODER_DVO_ON_W_1 BIT(5)
+#define DP_ENCODER_PSR_XTAL_FREQ_MHZ_MASK GENMASK(13, 9)
[ ... ]
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
[ ... ]
+static void mtk_dp_encoder_reset(struct mtk_dp *mtk_dp)
+{
/* dp tx encoder reset all sw */
mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3004,
DP_TX_ENCODER_4P_RESET_SW_DP_ENC0_P0,
DP_TX_ENCODER_4P_RESET_SW_DP_ENC0_P0);
+ if (mtk_dp->data->hwip_v1p4) {
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3004, 0,
+ DP_TX_ENCODER_4P_RESET_SW_DP_ENC0_P0);
+ mtk_dp_update_bits(mtk_dp, REG_3FF8_DP_ENC_P0_3,
+ ENC_PHY_STATE_RESET_ALL_MASK,
+ ENC_PHY_STATE_RESET_ALL_MASK);
+ }
+
/* Wait for sw reset to complete */
usleep_range(1000, 5000);
- mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3004,
- 0, DP_TX_ENCODER_4P_RESET_SW_DP_ENC0_P0);
+
+ if (!mtk_dp->data->hwip_v1p4)
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3004,
+ 0, DP_TX_ENCODER_4P_RESET_SW_DP_ENC0_P0);
+}
[Severity: High]
Does ENC_PHY_STATE_RESET_ALL_MASK overlap with functional state bits
like PHY_STATE_W_1_DP_ENC_P0_3 and DP_ENCODER_DVO_ON_W_1 defined in
mtk_dp_reg.h?
That's trolling!!! :-P
Yes, and that's on purpose.
Also, could the new hwip_v1p4 sequence in mtk_dp_encoder_reset() leave the
encoder PHY stuck in reset? The code asserts ENC_PHY_STATE_RESET_ALL_MASK
before the wait, but there does not appear to be a step to deassert it after
the sleep finishes.
It's ok, because it's auto-cleared by HW.
@@ -2860,7 +3079,8 @@ static int mtk_dp_register_phy(struct mtk_dp *mtk_dp)
[ ... ]
/* PHY calibration data is in mtk_dp only for legacy devicetree */
- mtk_dp_get_calibration_data(mtk_dp);
+ if (mtk_dp->data->efuse_fmt)
+ mtk_dp_get_calibration_data(mtk_dp);
mtk_dp->phy = devm_phy_get(&mtk_dp->phy_dev->dev, "dp");
[ ... ]
@@ -1359,8 +1550,10 @@ static int mtk_dp_phy_configure(struct mtk_dp *mtk_dp,
[ ... ]
/*
* For legacy, deprecated strategy, set partial PHY calibration here.
* New-style will set all PHY calibrations with phy ops instead.
*/
if (mtk_dp->phy_dev)
mtk_dp_set_calibration_data(mtk_dp);
[Severity: High]
If a legacy device tree is used on an MT8196 device, could this configuration
apply uninitialized zeros to the PHY hardware?
If a legacy device tree is used on MT8196, this MediaTek maintainer (me) needs
a big vacation :-D
This is an impossible scenario.
In mtk_dp_register_phy(), mtk_dp_get_calibration_data() is skipped because
efuse_fmt is NULL for mt8196_edp_data. However, mtk_dp_phy_configure() will
still call mtk_dp_set_calibration_data() if the legacy phy_dev was created,
potentially writing an unpopulated cal_data array directly to the registers.
The legacy phy_dev can't be created, unless the DT is very-very-very wrong, and
if it is, this driver can't get a magic 8 ball.
+static void mtk_dp_encoder_force_bias_power_on(struct mtk_dp *mtk_dp)
+{
[ ... ]
+ /* Wait for the MCU to get LDO ready in PHYD */
+ msleep(50);
[ ... ]
+}
[ ... ]
@@ -2580,6 +2795,10 @@ static void mtk_dp_bridge_atomic_disable(struct
drm_bridge *bridge,
[ ... ]
/* SDP path reset sw*/
mtk_dp_sdp_path_reset(mtk_dp);
+ /* Make sure BIAS is powered on after reset even when disabled */
+ if (mtk_dp->data->force_pwr_states)
+ mtk_dp_encoder_force_bias_power_on(mtk_dp);
+
/* Ensure the sink is muted */
msleep(20);
}
[Severity: High]
This is a pre-existing issue, but does this introduce an additional sleeping
function into an atomic context path?
Since mtk_dp_bridge_atomic_disable() operates in an atomic context during
modesetting, calling mtk_dp_encoder_force_bias_power_on() uses msleep(50),
which could result in kernel warnings or system instability.
It's not atomic context....
