Re: [PATCH 01/24] drm/msm/dp: reject YUV420-only modes without VSC SDP support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The patch relies on mode filtering to replace explicit VSC SDP support checks, but fails to apply this filtering to eDP, causing a DPU/DP state mismatch.

Pre-existing issues:
- [High] msm_dp_wide_bus_available() incorrectly uses the currently active hardware state to determine if wide bus can be used for a proposed mode, potentially rejecting valid high-bandwidth modes.
--

--- Patch [1]: [PATCH 01/24] drm/msm/dp: reject YUV420-only modes without VSC SDP support ---
commit 978b7d550fcfcaed1db8ec32849e99ffcd2880a7
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/msm/dp: reject YUV420-only modes without VSC SDP support

This patch updates DP mode validation to explicitly reject YUV420-only
modes if the panel lacks VSC SDP support. By filtering these out, the
DPU encoder logic is simplified to assume any validated 420-only mode
has VSC SDP support.

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 778e231d49677..1f20695f81e35 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -710,8 +710,7 @@ void dpu_encoder_update_topology(struct drm_encoder *drm_enc,
>  		if (fb && MSM_FORMAT_IS_YUV(msm_framebuffer_format(fb)))
>  			topology->num_cdm++;
>  	} else if (disp_info->intf_type == INTF_DP) {
> -		if (msm_dp_is_yuv_420_enabled(priv->kms->dp[disp_info->h_tile_instance[0]],
> -					      adj_mode))
> +		if (drm_mode_is_420_only(&connector->display_info, adj_mode))
>  			topology->num_cdm++;

[Severity: High]
Does this change break mode validation for eDP displays by assuming VSC SDP
support is always present for 420-only modes?

This patch adds mode filtering to msm_dp_bridge_mode_valid() to reject modes
without VSC SDP, so that dpu_encoder_update_topology() can assume all
420-only modes are fully supported.

However, the eDP validation function, msm_edp_bridge_mode_valid(), does not
appear to have the same filtering logic applied.

If an eDP sink advertises a YUV420-only mode without VSC SDP support, will it
pass validation, causing dpu_encoder_update_topology() to allocate a CDM
block (expecting YUV420) while the DP controller is still configured for RGB?

>  	}
>  }
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index dc6f33809ca5f..e0c44eef3abab 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
> @@ -709,9 +710,16 @@ enum drm_mode_status msm_dp_bridge_mode_valid(struct drm_bridge *bridge,
>  	msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
>  	link_info = &msm_dp_display->panel->link_info;
>  
> -	if ((drm_mode_is_420_only(&dp->connector->display_info, mode) &&
> -	     msm_dp_display->panel->vsc_sdp_supported) ||
> -	     msm_dp_wide_bus_available(dp))
> +	is_yuv_420 = drm_mode_is_420_only(&dp->connector->display_info, mode);
> +
> +	/*
> +	 * YUV 420 is carried over DP by signalling the colorimetry through a
> +	 * VSC SDP, so a 420-only mode cannot be driven without VSC SDP support.
> +	 */
> +	if (is_yuv_420 && !msm_dp_display->panel->vsc_sdp_supported)
> +		return MODE_NO_420;
> +
> +	if (is_yuv_420 || msm_dp_wide_bus_available(dp))
>  		mode_pclk_khz /= 2;

[Severity: High]
This is a pre-existing issue, but does msm_dp_wide_bus_available() check
the active hardware state rather than the proposed mode?

During mode validation in msm_dp_bridge_mode_valid(), the code checks:
	if (is_yuv_420 || msm_dp_wide_bus_available(dp))

However, msm_dp_wide_bus_available() evaluates the currently active modeset
instead of the proposed mode:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_wide_bus_available() {
	...
	if (dp->msm_dp_mode.out_fmt_is_yuv_420)
		return false;
	...
}

If userspace probes a high-bandwidth RGB mode while the current display state
is YUV420, could this incorrectly restrict hardware capability based on
the transient state and improperly reject valid modes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-drm-msm-display-interface-v1-0-368c10fe62fd@oss.qualcomm.com?part=1