Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] Spurious WARN_ON_ONCE triggered on display enable retry due to unhandled clock state on error path. - [Low] Incomplete refactoring: msm_dp_ctrl_off_link_stream() still uses open-coded pixel clock disablement. --
--- Patch [7]: [PATCH v8 07/15] drm/msm/dp: move the pixel clock control to its own API --- commit 56bf43c0164f110e1bba46e5b2fdcdcbaa97bf82 Author: Yongxing Mou <[email protected]> drm/msm/dp: move the pixel clock control to its own API Enable/Disable of DP pixel clock happens in multiple code paths leading to code duplication. Move it into individual helpers so that the helpers can be called wherever necessary. > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c > b/drivers/gpu/drm/msm/dp/dp_ctrl.c > index 711579de899d8..a79d9bb93cd06 100644 > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c [ ... ] > +static int msm_dp_ctrl_on_pixel_clk(struct msm_dp_ctrl_private *ctrl, > unsigned long pixel_rate) > +{ > + int ret; > + > + ret = clk_set_rate(ctrl->pixel_clk, 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; [Severity: Medium] Can this WARN_ON_ONCE be triggered by a recoverable hardware timeout? If msm_dp_ctrl_wait4video_ready() fails in msm_dp_ctrl_on_stream(), it returns directly without disabling the pixel clock, leaving ctrl->stream_clks_on set to true. Since the display enable failed, msm_dp_display->power_on remains false. A subsequent cleanup call to msm_dp_display_disable() returns immediately because power_on is false, which prevents it from cleaning up the clock state. When the display is retried, msm_dp_ctrl_on_pixel_clk() will be called again and hit this new warning. > + > + ret = clk_prepare_enable(ctrl->pixel_clk); > + if (ret) { > + DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret); > + return ret; > + } > + ctrl->stream_clks_on = true; > + > + return ret; > +} > + > +static void msm_dp_ctrl_off_pixel_clk(struct msm_dp_ctrl *msm_dp_ctrl) > +{ > + struct msm_dp_ctrl_private *ctrl; > + > + ctrl = container_of(msm_dp_ctrl, struct msm_dp_ctrl_private, > msm_dp_ctrl); > + > + if (ctrl->stream_clks_on) { > + clk_disable_unprepare(ctrl->pixel_clk); > + ctrl->stream_clks_on = false; > + } > +} [Severity: Low] This isn't a bug, but could this new helper also be used in msm_dp_ctrl_off_link_stream() to replace the open-coded check? It looks like msm_dp_ctrl_off_link_stream() still retains the duplicated pixel clock disablement logic: if (ctrl->stream_clks_on) { clk_disable_unprepare(ctrl->pixel_clk); ctrl->stream_clks_on = false; } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
