Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Secondary stream panels fail to have their `hw_revision` initialized, silently breaking controller-specific hardware workarounds in MST setups. - [High] Stream-specific panels are incorrectly passed to mainlink APIs, corrupting shared link state and misconfiguring the link based on leaf monitor capabilities instead of the MST hub. --
--- Patch [10]: [PATCH v8 10/15] drm/msm/dp: allow dp_ctrl stream APIs to use any panel passed to it --- commit 07d3f4b80ae5875a958a8aceb22c9ffab07db108 Author: Abhinav Kumar <[email protected]> drm/msm/dp: allow dp_ctrl stream APIs to use any panel passed to it With MST, multiple sinks share a single DP controller, so a cached panel in msm_dp_ctrl_private can no longer represent the per-stream sink. Drop the cache and pass panel explicitly to all stream-related dp_ctrl APIs. > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c > b/drivers/gpu/drm/msm/dp/dp_ctrl.c > index ae79cbec26d19..207643fa73063 100644 > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c > @@ -219,7 +219,7 @@ void msm_dp_ctrl_reset(struct msm_dp_ctrl *msm_dp_ctrl) > > if (!ctrl->hw_revision) { > ctrl->hw_revision = msm_dp_read_ahb(ctrl, REG_DP_HW_VERSION); > - ctrl->panel->hw_revision = ctrl->hw_revision; > + panel->hw_revision = ctrl->hw_revision; > } > } [Severity: High] Does this leave the hw_revision uninitialized for secondary stream panels in an MST setup? Since ctrl->hw_revision is populated during the host initialization for the primary panel, any subsequent stream panels passed to this function will bypass the assignment. Could this silently break hardware workarounds that depend on panel->hw_revision being set correctly? [ ... ] > @@ -414,10 +414,11 @@ static void msm_dp_ctrl_config_ctrl_streams(struct > msm_dp_ctrl_private *ctrl, > msm_dp_write_link(ctrl, REG_DP_CONFIGURATION_CTRL, config); > } > > -static void msm_dp_ctrl_config_ctrl_link(struct msm_dp_ctrl_private *ctrl) > +static void msm_dp_ctrl_config_ctrl_link(struct msm_dp_ctrl_private *ctrl, > + struct msm_dp_panel *panel) > { > u32 config = 0; > - const u8 *dpcd = ctrl->panel->dpcd; > + const u8 *dpcd = panel->dpcd; > > /* Default-> LSCLK DIV: 1/4 LCLK */ > config |= (2 << DP_CONFIGURATION_CTRL_LSCLK_DIV_SHIFT); [Severity: High] Can the direct write to REG_DP_CONFIGURATION_CTRL at the end of this function erase stream-specific configurations established by previously enabled streams? Since msm_dp_ctrl_config_ctrl_link() is called on every stream enable from msm_dp_ctrl_on_stream(), it seems this might overwrite the shared register and corrupt the state of active streams. [ ... ] > @@ -1810,11 +1821,12 @@ static void msm_dp_ctrl_link_clk_disable(struct > msm_dp_ctrl *msm_dp_ctrl) > str_on_off(ctrl->core_clks_on)); > } > > -static int msm_dp_ctrl_enable_mainlink_clocks(struct msm_dp_ctrl_private > *ctrl) > +static int msm_dp_ctrl_enable_mainlink_clocks(struct msm_dp_ctrl_private > *ctrl, > + struct msm_dp_panel *panel) > { > int ret = 0; > struct phy *phy = ctrl->phy; > - const u8 *dpcd = ctrl->panel->dpcd; > + const u8 *dpcd = panel->dpcd; > > ctrl->phy_opts.dp.lanes = ctrl->link->link_params.num_lanes; > ctrl->phy_opts.dp.link_rate = ctrl->link->link_params.rate / 100; [Severity: High] Is it correct to use the stream-specific (leaf) DPCD to configure shared mainlink properties here and in msm_dp_ctrl_link_train()? This appears to misconfigure the link based on leaf monitor capabilities instead of the MST hub. If a leaf monitor doesn't support features like enhanced framing or downspread, but the hub does, could this break the link to the hub? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=10
