Re: [PATCH v8 02/15] drm/msm/dp: drop deprecated .mode_set() and use .atomic_enable

[sashiko-bot@kernel.org]

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

New issues:
- [High] Moving mode state updates to `.atomic_enable` causes the DPU encoder to read stale wide bus configuration.

Pre-existing issues:
- [High] `mode_valid` incorrectly evaluates wide bus availability based on the currently active mode's state rather than the candidate mode, causing valid modes to be rejected.
--

--- Patch [2]: [PATCH v8 02/15] drm/msm/dp: drop deprecated .mode_set() and use .atomic_enable ---
commit f8a619167d17d8d750e695eca53077efafe8ab7e
Author: Yongxing Mou <yongxing.mou@oss.qualcomm.com>

drm/msm/dp: drop deprecated .mode_set() and use .atomic_enable

The bridge .mode_set() callback is deprecated. Remove it and move all
mode setup logic to .atomic_enable(), where the adjusted_mode is
available from the atomic CRTC state.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index bce165bac106b..57eafd4724627 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c

[ ... ]

> @@ -1309,7 +1325,7 @@ bool msm_dp_wide_bus_available(const struct msm_dp *msm_dp_display)
>  
>  	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
>  
> -	if (dp->msm_dp_mode.out_fmt_is_yuv_420)
> +	if (dp->panel->msm_dp_mode.out_fmt_is_yuv_420)
>  		return false;
>  
>  	return dp->wide_bus_supported;

[Severity: High]
This is a pre-existing issue, but does msm_dp_wide_bus_available() evaluate
the candidate mode correctly during mode probing?

This function checks the currently active mode's format (out_fmt_is_yuv_420)
instead of the candidate mode's format.

When msm_dp_bridge_mode_valid() (or msm_edp_bridge_mode_valid()) calls this
during mode probing, won't it incorrectly reject valid high-resolution RGB
modes if the user is currently running a lower-resolution YUV420 mode?

[ ... ]

> @@ -1386,7 +1406,7 @@ void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
>  	if (msm_dp_display->link->sink_count == 0)
>  		return;
>  
> -	rc = msm_dp_display_set_mode(dp, &msm_dp_display->msm_dp_mode);
> +	rc = msm_dp_display_set_mode(dp, &crtc_state->adjusted_mode, msm_dp_display->panel);
>  	if (rc) {
>  		DRM_ERROR("Failed to perform a mode set, rc=%d\n", rc);
>  		return;

[Severity: High]
Does moving this mode setup into .atomic_enable() cause the DPU encoder
to read a stale wide bus configuration?

In the DRM atomic commit flow, the encoder's .atomic_enable() (which executes
dpu_encoder_virt_atomic_enable()) is called before the bridge's
.atomic_enable().

Because msm_dp_display_set_mode() updates the format state 
(msm_dp_panel->msm_dp_mode.out_fmt_is_yuv_420), the DPU encoder will query
msm_dp_wide_bus_available() before this new mode's format has been stored.

Won't this cause the hardware to be programmed using the wide bus state from
the previously active mode, potentially causing display corruption or a blank
screen when switching between RGB and YUV420?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-dp_mstclean-v8-0-bea261bf4e7d@oss.qualcomm.com?part=2