Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Silent inversion of hardware initialization sequence (pixel clock and link retraining) - [Medium] API split fails to support inserting MST payload programming as claimed Pre-existing issues: - [High] Unpowered hardware register access causes kernel panic on error path -- --- Patch [8]: [PATCH v9 08/14] drm/msm/dp: break up dp_display_enable into two parts --- commit 722ccac6521827b0527a34404c0521cf56234857 Author: Abhinav Kumar <[email protected]> drm/msm/dp: break up dp_display_enable into two parts Split dp_display_enable into prepare and enable APIs. This aims to support MST bridges_enable inserting MST payload functions between enabling stream clocks and programming registers. > dp_display_enable() currently re-trains the link if needed and then > enables the pixel clock, programs the controller to start sending the > pixel stream. Split these two parts into prepare/enable APIs, to support > MST bridges_enable insert the MST payloads funcs between enable > stream_clks and program register. [Severity: Medium] Does this split actually allow inserting MST payload functions between enabling stream clocks and programming the registers? Looking at the implementation of msm_dp_ctrl_on_stream() below, the call to msm_dp_ctrl_on_pixel_clk() and the subsequent register programming are still tightly coupled in the same function. It doesn't seem possible for an external caller to inject payload programming between them. > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c > b/drivers/gpu/drm/msm/dp/dp_ctrl.c > index 068d4f8b673f6..43094dda34349 100644 > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c [ ... ] > -int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl, bool > force_link_train) > +int msm_dp_ctrl_prepare_stream_on(struct msm_dp_ctrl *msm_dp_ctrl, bool > force_link_train) > { > int ret = 0; [ ... ] > - ret = msm_dp_ctrl_on_pixel_clk(ctrl, pixel_rate); > - if (ret) > - return ret; > - > if (force_link_train || !msm_dp_ctrl_channel_eq_ok(ctrl)) > msm_dp_ctrl_link_retrain(ctrl); [Severity: High] Does this change invert the hardware initialization sequence? Before this patch, msm_dp_ctrl_on_stream() called msm_dp_ctrl_on_pixel_clk() to enable the pixel clock before performing the link retraining. With the new split, msm_dp_ctrl_prepare_stream_on() performs link retraining, but the pixel clock isn't enabled until msm_dp_ctrl_on_stream() is called later. Could this cause link training failures or black screens by retraining before the pixel clock is ready? [ ... ] > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > b/drivers/gpu/drm/msm/dp/dp_display.c > index 2a84a6ffedbb5..d094d484474cd 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c [ ... ] > -static int msm_dp_display_enable(struct msm_dp_display_private *dp, bool > force_link_train) > +static int msm_dp_display_prepare_link(struct msm_dp_display_private *dp) > +{ > + struct msm_dp *msm_dp_display = &dp->msm_dp_display; > + int rc = 0; > + bool force_link_train = false; > + > + drm_dbg_dp(dp->drm_dev, "sink_count=%d\n", dp->link->sink_count); > + > + if (msm_dp_display->is_edp) > + msm_dp_hpd_plug_handle(dp); > + > + rc = pm_runtime_resume_and_get(&msm_dp_display->pdev->dev); > + if (rc) { > + DRM_ERROR("failed to pm_runtime_resume\n"); > + return rc; > + } [Severity: High] This is a pre-existing issue, but if pm_runtime_resume_and_get() fails here, the device is left unpowered while the DRM state considers it enabled. Since msm_dp_bridge_atomic_enable() returns void and cannot abort the overall atomic commit if this fails, msm_dp_bridge_atomic_disable() will unconditionally be called later during teardown. That disable path eventually calls msm_dp_ctrl_push_idle() which accesses unclocked and unpowered DP hardware registers, leading to a synchronous external abort (kernel panic). Should there be a way to prevent hardware access during disable if the device never successfully resumed? [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
