Re: [PATCH v1] drm/msm/dp: Disable stream clock after video-ready timeout

On Tue, Jul 07, 2026 at 06:05:53PM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Disabling the pixel clock while the DP controller is actively in the SEND_VIDEO state violates the teardown sequence and can lock up the hardware.
> --

Could you please comment on this Sashiko comment?

> 
> --- Patch [1]: [PATCH v1] drm/msm/dp: Disable stream clock after video-ready timeout ---
> commit d51f59f75198361a5116081d3f135fd586dbcb66
> Author: Yuho Choi <[email protected]>
> 
> drm/msm/dp: Disable stream clock after video-ready timeout
> 
> This commit routes the video-ready timeout path in msm_dp_ctrl_on_stream() 
> through a cleanup path. It disables the stream pixel clock and clears the
> stream_clks_on flag if the clock was enabled during the call.
> 
> > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > index 86ef8c89ad443..139e9fa25d6a7 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> [ ... ]
> > @@ -2543,13 +2545,17 @@ int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl, bool force_link_train
> >  
> >  	ret = msm_dp_ctrl_wait4video_ready(ctrl);
> >  	if (ret)
> > -		return ret;
> > +		goto end;
> >  
> >  	mainlink_ready = msm_dp_ctrl_mainlink_ready(ctrl);
> >  	drm_dbg_dp(ctrl->drm_dev,
> >  		"mainlink %s\n", mainlink_ready ? "READY" : "NOT READY");
> >  
> >  end:
> > +	if (stream_clk_enabled && ret) {
> > +		clk_disable_unprepare(ctrl->pixel_clk);
> > +		ctrl->stream_clks_on = false;
> > +	}
> 
> [Severity: High]
> Does this error path leave the DP controller in an active state without
> reverting it before disabling the pixel clock?
> 
> Earlier in this function, the controller is programmed to send video:
> 
> 	msm_dp_write_link(ctrl, REG_DP_STATE_CTRL, DP_STATE_CTRL_SEND_VIDEO);
> 
> 	ret = msm_dp_ctrl_wait4video_ready(ctrl);
> 
> If wait4video_ready times out, the code jumps to the end label and disables
> the pixel clock. However, if the caller ignores this failure and DRM later
> disables the bridge, the teardown sequence will invoke msm_dp_ctrl_push_idle().
> 
> Since the controller was left in DP_STATE_CTRL_SEND_VIDEO but its pixel
> clock is now disabled, could writing DP_STATE_CTRL_PUSH_IDLE cause a
> timeout or wedge the hardware because it cannot process the state transition
> without the clock?
> 
> >  	return ret;
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

-- 
With best wishes
Dmitry