Re: [PATCH v7 04/35] drm/msm/dp: introduce stream_id for each DP panel

[sashiko-bot@kernel.org]

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 <quic_abhinavk@quicinc.com>

    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/20260929-msm-dp-mst-v7-0-4b81473185e7@oss.qualcomm.com?part=4