Re: [PATCH 09/13] drm/msm/dp: Track output bit depth in bridge atomic state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm/dp: Atomic semantics violation by deferring bandwidth check
--

commit 90a8ee3f0617fe6a0415caee72e5905ba2709743
Author: Xilin Wu <sophon@radxa.com>
drm/msm/dp: Track output bit depth in bridge atomic state

This commit tracks the output bit depth in the bridge atomic state to expose
max bpc on DP and eDP, and selects a supported component depth. It reads
cached capabilities under a lock and defers bandwidth checks if they are not
yet valid, rechecking the selected depth against the trained link before
enabling video.

> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 16c9165b5f311..f41924e75854c 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -2620,6 +2621,20 @@ int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl, struct msm_dp_panel *
>  
>  	ctrl = container_of(msm_dp_ctrl, struct msm_dp_ctrl_private, msm_dp_ctrl);
>  
> +	/* Link training may have reduced the available bandwidth. */
> +	if (!panel->video_test) {
> +		u32 clock = panel->msm_dp_mode.drm_mode.clock;
> +
> +		if (panel->msm_dp_mode.out_fmt_is_yuv_420)
> +			clock /= 2;
> +		ret = msm_dp_utils_select_bpp(panel->msm_dp_mode.bpp / 3, 10,
> +					      clock, ctrl->link->link_params.rate,
> +					      ctrl->link->link_params.num_lanes);
> +		if (ret < 0)
> +			return ret;

[Severity: High]
Does this error path abort the stream setup during the atomic_enable phase?

Userspace can trigger this by requesting a modeset via the atomic API with a
mode or bpp that exceeds the hardware DP link rate while the panel is off.
Because the validation was bypassed in msm_dp_display_atomic_check(),
msm_dp_ctrl_on_stream() now re-evaluates the bandwidth using the actual
trained rate. 

If the requested mode exceeds the trained link bandwidth, this returns
-EINVAL during atomic_enable, which aborts the stream setup. Bypassing
validation in atomic_check and failing during atomic_enable violates the
fundamental guarantee of DRM atomic modesetting, which requires that
atomic_check rejects all unsupported configurations so that atomic_enable is
infallible. This can result in state desynchronization between the DRM core
and the driver, leading to a blank screen and potential vblank timeouts.

> +		panel->msm_dp_mode.bpp = ret;
> +	}
> +
>  	pixel_rate_orig = panel->msm_dp_mode.drm_mode.clock;
>  	pixel_rate = pixel_rate_orig;
>  
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 9c5b1cd200f85..af6bc179723cc 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
> @@ -682,6 +684,34 @@ static int msm_dp_display_set_mode(struct msm_dp *msm_dp_display,
>  	return 0;
>  }
>  
> +int msm_dp_display_atomic_check(struct msm_dp *msm_dp_display,
> +				struct msm_dp_bridge_state *state,
> +				const struct drm_display_mode *mode,
> +				const struct drm_connector_state *conn_state)
> +{
> +	const struct drm_display_info *info = &conn_state->connector->display_info;
> +	struct msm_dp_display_private *dp;
> +	unsigned int clock = mode->clock;
> +	int bpp;
> +
> +	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
> +
> +	if (drm_mode_is_420_only(info, mode))
> +		clock /= 2;
> +
> +	guard(mutex)(&dp->plugged_lock);
> +
> +	bpp = msm_dp_utils_select_bpp(info->bpc, conn_state->max_requested_bpc,
> +				      clock, dp->panel->link_caps_valid ?
> +				      dp->panel->link_info.rate : 0,
> +				      dp->panel->link_info.num_lanes);

[Severity: High]
Can this defer the bandwidth check to the enable phase and violate DRM atomic
semantics?

When an eDP panel is powered down and its link capabilities have not yet been
probed (link_caps_valid is false), this passes 0 for the rate to
msm_dp_utils_select_bpp(). This causes the bandwidth verification to be
skipped, allowing the commit to be accepted by the kernel.

> +	if (bpp < 0)
> +		return bpp;
> +
> +	state->bpp = bpp;
> +	return 0;
> +}
> +
>  static int msm_dp_display_prepare_link(struct msm_dp_display_private *dp)
>  {
>  	struct msm_dp *msm_dp_display = &dp->msm_dp_display;
> diff --git a/drivers/gpu/drm/msm/dp/dp_utils.c b/drivers/gpu/drm/msm/dp/dp_utils.c
> index 72bae48a1fe0b..3e49854d73fb9 100644
> --- a/drivers/gpu/drm/msm/dp/dp_utils.c
> +++ b/drivers/gpu/drm/msm/dp/dp_utils.c
[ ... ]
> @@ -89,6 +89,26 @@ void msm_dp_utils_pack_sdp_header(const struct dp_sdp_header *sdp_header,
>  		FIELD_PREP(PARITY_3_MASK, msm_dp_utils_calculate_parity(sdp_header->HB3));
>  }
>  
> +/* A zero link rate means that a powered-down eDP panel has not been probed. */
> +int msm_dp_utils_select_bpp(unsigned int bpc, unsigned int max_bpc,
> +			    unsigned int clock, unsigned int rate,
> +			    unsigned int lanes)
> +{
> +	unsigned int bpp;
> +
> +	bpc = min3(bpc ? bpc : 8, max_bpc, 10U);
> +	bpc = round_down(bpc, 2);
> +	if (bpc < 6)
> +		return -EINVAL;
> +
> +	for (bpp = bpc * 3; bpp >= 18; bpp -= 6) {
> +		if (!rate || (u64)clock * bpp <= (u64)rate * lanes * 8)
> +			return bpp;

[Severity: High]
Does this code allow bypassing the bandwidth validation?

When rate is 0, the check automatically bypasses bandwidth validation,
returning success unconditionally.

> +	}
> +
> +	return -EINVAL;
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-msm-dp-hdr10-v1-0-dbbf8b2b42bc@radxa.com?part=9