Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] Newly introduced sleeping function msleep() is used in an atomic context path via bridge atomic_disable. - [High] The SDP_DP13_EN_DP_ENC1_P0 bit is added to the value but omitted from the mask, preventing SDP from actually being enabled. - [High] NULL pointer dereference when parsing calibration data due to missing efuse_fmt. Pre-existing issues: - [High] Preexisting sleeping functions used in atomic context via atomic enable/disable callbacks. -- commit c515b507239926f8609226c47569e9b95e171972 Author: AngeloGioacchino Del Regno <[email protected]> drm/mediatek: mtk_dp: Add support for eDP1.5 IPs and MT8196 SoC This commit adds support for MediaTek's Embedded DisplayPort (eDP) v1.5 IP as found in the MT8196 SoC and other variants. It introduces new data structures, modifies encoder paths, and encloses register writes in platform data booleans. > diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c > b/drivers/gpu/drm/mediatek/mtk_dp.c > index 2d58eacb3d3e6..605ead7fa9bf6 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dp.c > +++ b/drivers/gpu/drm/mediatek/mtk_dp.c [ ... ] > @@ -647,7 +668,16 @@ static void mtk_dp_setup_encoder(struct mtk_dp *mtk_dp) > mtk_dp_update_bits(mtk_dp, MTK_DP_ENC1_P0_3364, > FIFO_READ_START_POINT_DP_ENC1_P0_VAL << 12, > FIFO_READ_START_POINT_DP_ENC1_P0_MASK); > - mtk_dp_write(mtk_dp, MTK_DP_ENC1_P0_3368, DP_ENC1_P0_3368_VAL); > + > + mask = VIDEO_SRAM_FIFO_CNT_RESET_SEL_DP_ENC1_P0 | > VIDEO_STABLE_CNT_THRD_DP_ENC1_P0; > + mask |= BS2BS_MODE_DP_ENC1_P0; > + > + val = mask; > + > + if (mtk_dp->data->sdp_encoder1_enable) > + val |= SDP_DP13_EN_DP_ENC1_P0; > + > + mtk_dp_update_bits(mtk_dp, MTK_DP_ENC1_P0_3368, val, mask); > } [Severity: High] Does this code successfully enable the Secondary Data Packet (SDP)? The SDP_DP13_EN_DP_ENC1_P0 bit is added to the val variable, but it does not appear to be added to the mask. Since mtk_dp_update_bits() applies the mask before writing, won't this omit the bit and prevent SDP from actually being enabled? [ ... ] > @@ -1106,6 +1136,72 @@ static void mtk_dp_hwirq_enable(struct mtk_dp *mtk_dp, > bool enable) > IRQ_MASK_DP_TRANS_P0_MASK); > } > > +static void mtk_dp_encoder_force_bias_power_on(struct mtk_dp *mtk_dp) > +{ > + /* Force MCU to enable BIAS */ > + mtk_dp_update_bits(mtk_dp, REG_3F44_DP_ENC_P0_3, > + ENC_PHY_PWR_OVERWRITE_EN, ENC_PHY_PWR_OVERWRITE_EN); > + mtk_dp_update_bits(mtk_dp, REG_3F44_DP_ENC_P0_3, > + FIELD_PREP_CONST(ENC_PHY_PWR_STATE_OVERWRITE_MASK, > + ENC_PHY_BIAS_PWR_ON), > + ENC_PHY_PWR_STATE_OVERWRITE_MASK); > + > + /* Wait for the MCU to get LDO ready in PHYD */ > + msleep(50); > + > + mtk_dp_update_bits(mtk_dp, REG_3F44_DP_ENC_P0_3, > + 0, ENC_PHY_PWR_OVERWRITE_EN); > +} [Severity: High] Is it safe to use msleep() in this context? This function is called from mtk_dp_bridge_atomic_disable(), which runs as part of the atomic commit tail. According to DRM subsystem guidelines, atomic commit callbacks execute in an atomic context where sleeping functions are unsafe and can lead to warnings or system instability. [ ... ] > @@ -2583,6 +2796,9 @@ static void mtk_dp_bridge_atomic_disable(struct > drm_bridge *bridge, > /* SDP path reset sw*/ > mtk_dp_sdp_path_reset(mtk_dp); > > + 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 is it safe to have msleep(20) here? The mtk_dp_bridge_atomic_disable() callback executes in an atomic context during display modeset or disable. Calling sleeping functions like msleep() or usleep_range() (inside the preceding mtk_dp_sdp_path_reset() call) from an atomic context violates core DRM atomic context rules and can cause system instability. [ ... ] > @@ -3107,6 +3325,19 @@ static const struct mtk_dp_data mt8195_dp_data = { > .audio_supported = true, > .audio_m_div2_bit = MT8195_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2, > .hw_max_link_rate = DP_LINK_BW_5_4, > + .sdp_encoder1_enable = true, > +}; > + > +static const struct mtk_dp_data mt8196_edp_data = { > + .bridge_type = DRM_MODE_CONNECTOR_eDP, > + .smc_cmd = MTK_DP_SIP_ATF_EDP_SET_VID_MUTE, > + .audio_supported = false, > + .audio_m_div2_bit = MT8195_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2, > + .hw_max_link_rate = DP_LINK_BW_8_1, > + .hwip_v1p4 = true, > + .aux_hpd_supported = true, > + .force_pwr_states = true, > + .psr_supported = true, > }; [Severity: High] Can this structure lead to a NULL pointer dereference? The mt8196_edp_data definition lacks the efuse_fmt field. If the devicetree node for the MT8196 eDP lacks a phys property, the driver will fall back to its legacy PHY registration path. The legacy path calls mtk_dp_get_calibration_data() which attempts to iterate over the array: for (i = 0; i < MTK_DP_CAL_MAX; i++) { fmt = &mtk_dp->data->efuse_fmt[i]; ... } Because efuse_fmt is NULL, wouldn't this cause a kernel panic during probe? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=10
