Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] msm_dp_display_post_enable() hardcodes root dp->panel, breaking stream-specific PSR config --
commit ce954c1f87fec9b4ffb2b11bf4184d722dd05ced Author: Abhinav Kumar <[email protected]> drm/msm/dp: factor out _helper variants of bridge ops accepting a panel The atomic bridge callbacks (set_mode / enable / disable / post_disable) on dp_display currently hard-code dp->panel. This commit introduces helper variants that take a struct msm_dp_panel pointer to allow the upcoming DP MST encoder to drive link-level prepare and unprepare uniformly. > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > b/drivers/gpu/drm/msm/dp/dp_display.c > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c [ ... ] > +void msm_dp_display_enable_helper(struct msm_dp *msm_dp_display, > + struct msm_dp_panel *msm_dp_panel) > +{ > + int rc = 0; > + struct msm_dp_display_private *dp; > + > + dp = container_of(msm_dp_display, struct msm_dp_display_private, > msm_dp_display); > + > + rc = msm_dp_display_enable(dp, msm_dp_panel); > + if (rc) > + DRM_ERROR("DP display enable failed, rc=%d\n", rc); > + > + rc = msm_dp_display_post_enable(msm_dp_display); [Severity: Medium] Does this call correctly configure PSR for MST streams? This helper correctly accepts a stream-specific msm_dp_panel, but msm_dp_display_post_enable() was not refactored to take a panel argument. Looking at msm_dp_display_post_enable(): static int msm_dp_display_post_enable(struct msm_dp *msm_dp_display) { ... if (msm_dp_display->psr_supported) msm_dp_ctrl_config_psr(dp->ctrl, dp->panel); ... } It still hardcodes the root dp->panel instead of the active stream's panel. Could this misconfiguration lead to missing or incorrect PSR setup on MST streams? > + if (rc) { > + DRM_ERROR("DP display post enable failed, rc=%d\n", rc); > + msm_dp_display_disable(dp, msm_dp_panel); > + } > + > + drm_dbg_dp(msm_dp_display->drm_dev, "type=%d Done\n", > msm_dp_display->connector_type); > +} [ ... ] > +void msm_dp_display_disable_helper(struct msm_dp *msm_dp_display, > + struct msm_dp_panel *msm_dp_panel) > +{ > + struct msm_dp_display_private *dp; > + > + dp = container_of(msm_dp_display, struct msm_dp_display_private, > msm_dp_display); > > /* > * If .atomic_enable() bailed out - link training failure is the common > @@ -1521,17 +1542,28 @@ void msm_dp_display_atomic_disable(struct msm_dp *dp) > * TrustZone-level SOCCP/ADSP force-stop and a silent SoC reset. > * There is nothing to push idle, so leave it alone. > */ > - if (!dp->power_on) > + if (!msm_dp_display->active_stream_cnt) > return; [Severity: High] This is a pre-existing issue, but could this safety guard be bypassed if a stream fails to enable? Looking at msm_dp_display_enable(), active_stream_cnt is incremented unconditionally even if msm_dp_ctrl_on_stream() fails: static int msm_dp_display_enable(struct msm_dp_display_private *dp, struct msm_dp_panel *msm_dp_panel) { rc = msm_dp_ctrl_on_stream(dp->ctrl, msm_dp_panel, ...); msm_dp_display->active_stream_cnt++; return rc; } If msm_dp_ctrl_on_stream() returns an error during standard atomic commit paths (e.g., due to a transient hardware failure like pixel clock enablement), active_stream_cnt is artificially greater than zero. Later, when the stream is disabled, this check evaluates to true. Pushing idle patterns to a controller that failed to enable will time out, which as documented here can cause a TrustZone-level SOCCP/ADSP force-stop and a silent SoC reset. Can we avoid incrementing active_stream_cnt when msm_dp_ctrl_on_stream() fails? > - msm_dp_ctrl_push_vcpf(msm_dp_display->ctrl, msm_dp_display->panel); > - msm_dp_ctrl_mst_timeslot_setup(msm_dp_display->ctrl); > - msm_dp_ctrl_mst_send_act(msm_dp_display->ctrl, msm_dp_display->panel); > + msm_dp_ctrl_push_vcpf(dp->ctrl, msm_dp_panel); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=15
