Re: [PATCH v7 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 setup to `.atomic_enable` causes the upstream DPU encoder to read stale wide-bus configuration data during its own enablement.

Pre-existing issues:
- [High] Mode validation callback incorrectly evaluates wide bus availability based on the state of the currently active mode rather than the candidate mode.
--

commit 1b0de5b490fbc6b7e19b00f71948fa7d36954fe7
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)

[Severity: High]
This isn't a bug introduced by this patch, but doesn't this function
check the currently active mode's format rather than the candidate mode's
format?

During mode validation in msm_dp_display_mode_valid(), the driver calls
msm_dp_wide_bus_available() to adjust pixel clock limits. Since this reads
the active mode's YUV420 state, wouldn't it incorrectly assume wide bus is
unavailable for a candidate RGB mode if the currently active mode happens
to be YUV420?

This might cause valid high-resolution RGB modes to be rejected because
their clock exceeds DP_MAX_PIXEL_CLK_KHZ. Additionally, isn't this reading
a state field that could be locklessly mutated by concurrent atomic commits?

>  		return false;
>  
>  	return dp->wide_bus_supported;

[ ... ]

> @@ -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);

[Severity: High]
By moving the mode setup here into the bridge's .atomic_enable callback,
could this cause the upstream DPU encoder to read stale wide-bus
configuration data?

The DRM atomic helper sequence executes the encoder's .atomic_enable hook
before the bridge's .atomic_enable hook. In the encoder's enable sequence:

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c:dpu_encoder_virt_atomic_enable()
    dpu_enc->wide_bus_en = dpu_encoder_is_widebus_enabled(drm_enc);

This calls down to msm_dp_wide_bus_available(), which checks the
out_fmt_is_yuv_420 flag.

Since the bridge hasn't updated its mode state yet via
msm_dp_display_set_mode(), won't the encoder read the stale value from the
previous modeset?

This seems like it could incorrectly enable or disable wide bus support
when switching between RGB and YUV420 modes, potentially leading to a
hardware configuration mismatch and display corruption.

>  	if (rc) {
>  		DRM_ERROR("Failed to perform a mode set, rc=%d\n", rc);
>  		return;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-dp_mstclean-v7-0-ea04113e8233@oss.qualcomm.com?part=2