Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] drm/msm/dp: recoverable warning converted to fatal error in pixel clk enable - [Low] drm/msm/dp: debug logs hardcode DP_STREAM_0 for stream_clks status --
commit 7e53604a5503e211e31129f82d37f5fe82b91f63 Author: Abhinav Kumar <[email protected]> drm/msm/dp: introduce stream_id for each DP panel With MST, each DP controller can handle multiple streams. There shall be one dp_panel for each stream but the dp_display object shall be shared among them. To represent this abstraction, add a stream_id field to struct msm_dp_panel and introduce the msm_dp_stream_id enum. For SST, the field is initialized to DP_STREAM_0. > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c > b/drivers/gpu/drm/msm/dp/dp_ctrl.c > index e7bb9558c0b41..67f1da46b973f 100644 > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c > @@ -1750,7 +1751,7 @@ int msm_dp_ctrl_core_clk_enable(struct msm_dp_ctrl > *msm_dp_ctrl) > > drm_dbg_dp(ctrl->drm_dev, "enable core clocks \n"); > drm_dbg_dp(ctrl->drm_dev, "stream_clks:%s link_clks:%s core_clks:%s\n", > - str_on_off(ctrl->stream_clks_on), > + str_on_off(ctrl->stream_clks_on[DP_STREAM_0]), [Severity: Low] Will hardcoding DP_STREAM_0 here obscure the state of streams 1 through 3 in MST topologies? Since the core clocks are shared, printing only the stream 0 state could be misleading when multiple streams might be active. > str_on_off(ctrl->link_clks_on), > str_on_off(ctrl->core_clks_on)); > [ ... ] > @@ -1800,7 +1801,7 @@ static int msm_dp_ctrl_link_clk_enable(struct > msm_dp_ctrl *msm_dp_ctrl) > > drm_dbg_dp(ctrl->drm_dev, "enable link clocks\n"); > drm_dbg_dp(ctrl->drm_dev, "stream_clks:%s link_clks:%s core_clks:%s\n", > - str_on_off(ctrl->stream_clks_on), > + str_on_off(ctrl->stream_clks_on[DP_STREAM_0]), [Severity: Low] Does this similarly hide the status of the other stream clocks when enabling the link clocks? > str_on_off(ctrl->link_clks_on), > str_on_off(ctrl->core_clks_on)); > [ ... ] > @@ -2190,38 +2191,39 @@ static bool msm_dp_ctrl_send_phy_test_pattern(struct > msm_dp_ctrl_private *ctrl) > return success; > } > > -static int msm_dp_ctrl_on_pixel_clk(struct msm_dp_ctrl_private *ctrl, > unsigned long pixel_rate) > +static int msm_dp_ctrl_on_pixel_clk(struct msm_dp_ctrl_private *ctrl, > unsigned long pixel_rate, > + enum msm_dp_stream_id stream_id) > { > int ret; > > - ret = clk_set_rate(ctrl->pixel_clk, pixel_rate * 1000); > + ret = clk_set_rate(ctrl->pixel_clk[stream_id], pixel_rate * 1000); > if (ret) { > DRM_ERROR("Failed to set pixel clock rate. ret=%d\n", ret); > return ret; > } > > - if (WARN_ON_ONCE(ctrl->stream_clks_on)) > - return 0; > + if (WARN_ON_ONCE(ctrl->stream_clks_on[stream_id])) > + return -EINVAL; [Severity: Medium] Does returning -EINVAL instead of 0 here turn a recoverable state mismatch into a hard failure? Previously in msm_dp_ctrl_on_pixel_clk(), if the stream clock was already on, it would trigger a warning but return 0, allowing the driver to skip the enable step and proceed. By returning -EINVAL, the initialization sequence now aborts, potentially leaving the display blank if the internal state gets desynced. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
