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

Dmitry Baryshkov
2026-04-11T17:23:17Z

On Fri, Apr 10, 2026 at 05:33:37PM +0800, Yongxing Mou wrote:
> 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.
>
> Drop msm_dp_mode from msm_dp_display_private and store the mode directly
> in the panel, as it was only used as a temporary cache. Both changes are
> limited to msm_dp_display_set_mode and are kept in a single patch.
>
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> Signed-off-by: Yongxing Mou <yongxing.mou@oss.qualcomm.com>
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 81 ++++++++++++++-----------------------
> drivers/gpu/drm/msm/dp/dp_drm.c | 2 -
> drivers/gpu/drm/msm/dp/dp_drm.h | 3 --
> 3 files changed, 31 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index e28cc1bbb5b1..e9f0b96c3ebd 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -63,7 +63,6 @@ struct msm_dp_display_private {
> struct msm_dp_panel *panel;
> struct msm_dp_ctrl *ctrl;
>
> - struct msm_dp_display_mode msm_dp_mode;
> struct msm_dp msm_dp_display;
>
> /* wait for audio signaling */
> @@ -593,16 +592,33 @@ static int msm_dp_init_sub_modules(struct msm_dp_display_private *dp)
> }
>
> static int msm_dp_display_set_mode(struct msm_dp *msm_dp_display,
> - struct msm_dp_display_mode *mode)
> + const struct drm_display_mode *adjusted_mode,
> + struct msm_dp_panel *msm_dp_panel)
> {
> struct msm_dp_display_private *dp;
> + u32 bpp;
>
> dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
>
> - drm_mode_copy(&dp->panel->msm_dp_mode.drm_mode, &mode->drm_mode);
> - dp->panel->msm_dp_mode.bpp = mode->bpp;
> - dp->panel->msm_dp_mode.out_fmt_is_yuv_420 = mode->out_fmt_is_yuv_420;
> - msm_dp_panel_init_panel_info(dp->panel);
> + drm_mode_copy(&msm_dp_panel->msm_dp_mode.drm_mode, adjusted_mode);
> + if (msm_dp_display_check_video_test(msm_dp_display))
> + bpp = msm_dp_display_get_test_bpp(msm_dp_display);
> + else
> + bpp = msm_dp_panel->connector->display_info.bpc * 3;
> +
> + msm_dp_panel->msm_dp_mode.bpp = bpp ? bpp : 24; /* Default bpp */
> + msm_dp_panel->msm_dp_mode.v_active_low =
> + !!(adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC);
> + msm_dp_panel->msm_dp_mode.h_active_low =
> + !!(adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC);
> + msm_dp_panel->msm_dp_mode.out_fmt_is_yuv_420 =
> + drm_mode_is_420_only(&msm_dp_panel->connector->display_info, adjusted_mode) &&
> + msm_dp_panel->vsc_sdp_supported;

We need to stop poking into the msm_dp_panel internals. would you mind
changing the code to pass the mode to msm_dp_panel_init_panel_info()? It
can be the next patch in the series.

Nevertheless,

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

> + msm_dp_panel_init_panel_info(msm_dp_panel);
> +
> + /* populate wide_bus_support to different layers */
> + dp->ctrl->wide_bus_en =
> + msm_dp_panel->msm_dp_mode.out_fmt_is_yuv_420 ? false : dp->wide_bus_supported;
> return 0;
> }
>
> @@ -1305,7 +1321,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;
> @@ -1361,15 +1377,19 @@ void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
> {
> struct msm_dp_bridge *msm_dp_bridge = to_dp_bridge(drm_bridge);
> struct msm_dp *dp = msm_dp_bridge->msm_dp_display;
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *crtc_state;
> int rc = 0;
> struct msm_dp_display_private *msm_dp_display;
> bool force_link_train = false;
>
> msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
> - if (!msm_dp_display->msm_dp_mode.drm_mode.clock) {
> - DRM_ERROR("invalid params\n");
> +
> + crtc = drm_atomic_get_new_crtc_for_encoder(state,
> + drm_bridge->encoder);
> + if (!crtc)
> return;
> - }
> + crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>
> if (dp->is_edp)
> msm_dp_hpd_plug_handle(msm_dp_display);
> @@ -1382,7 +1402,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;
> @@ -1440,45 +1460,6 @@ void msm_dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
> pm_runtime_put_sync(&dp->pdev->dev);
> }
>
> -void msm_dp_bridge_mode_set(struct drm_bridge *drm_bridge,
> - const struct drm_display_mode *mode,
> - const struct drm_display_mode *adjusted_mode)
> -{
> - struct msm_dp_bridge *msm_dp_bridge = to_dp_bridge(drm_bridge);
> - struct msm_dp *dp = msm_dp_bridge->msm_dp_display;
> - struct msm_dp_display_private *msm_dp_display;
> - struct msm_dp_panel *msm_dp_panel;
> -
> - msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
> - msm_dp_panel = msm_dp_display->panel;
> -
> - memset(&msm_dp_display->msm_dp_mode, 0x0, sizeof(struct msm_dp_display_mode));
> -
> - if (msm_dp_display_check_video_test(dp))
> - msm_dp_display->msm_dp_mode.bpp = msm_dp_display_get_test_bpp(dp);
> - else /* Default num_components per pixel = 3 */
> - msm_dp_display->msm_dp_mode.bpp = dp->connector->display_info.bpc * 3;
> -
> - if (!msm_dp_display->msm_dp_mode.bpp)
> - msm_dp_display->msm_dp_mode.bpp = 24; /* Default bpp */
> -
> - drm_mode_copy(&msm_dp_display->msm_dp_mode.drm_mode, adjusted_mode);
> -
> - msm_dp_display->msm_dp_mode.v_active_low =
> - !!(msm_dp_display->msm_dp_mode.drm_mode.flags & DRM_MODE_FLAG_NVSYNC);
> -
> - msm_dp_display->msm_dp_mode.h_active_low =
> - !!(msm_dp_display->msm_dp_mode.drm_mode.flags & DRM_MODE_FLAG_NHSYNC);
> -
> - msm_dp_display->msm_dp_mode.out_fmt_is_yuv_420 =
> - drm_mode_is_420_only(&dp->connector->display_info, adjusted_mode) &&
> - msm_dp_panel->vsc_sdp_supported;
> -
> - /* populate wide_bus_support to different layers */
> - msm_dp_display->ctrl->wide_bus_en =
> - msm_dp_display->msm_dp_mode.out_fmt_is_yuv_420 ? false : msm_dp_display->wide_bus_supported;
> -}
> -
> void msm_dp_bridge_hpd_enable(struct drm_bridge *bridge)
> {
> struct msm_dp_bridge *msm_dp_bridge = to_dp_bridge(bridge);
> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
> index 8dc0dabd275c..af3d3e3a2d84 100644
> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> @@ -56,7 +56,6 @@ static const struct drm_bridge_funcs msm_dp_bridge_ops = {
> .atomic_enable = msm_dp_bridge_atomic_enable,
> .atomic_disable = msm_dp_bridge_atomic_disable,
> .atomic_post_disable = msm_dp_bridge_atomic_post_disable,
> - .mode_set = msm_dp_bridge_mode_set,
> .mode_valid = msm_dp_bridge_mode_valid,
> .get_modes = msm_dp_bridge_get_modes,
> .detect = msm_dp_bridge_detect,
> @@ -233,7 +232,6 @@ static const struct drm_bridge_funcs msm_edp_bridge_ops = {
> .atomic_enable = msm_edp_bridge_atomic_enable,
> .atomic_disable = msm_edp_bridge_atomic_disable,
> .atomic_post_disable = msm_edp_bridge_atomic_post_disable,
> - .mode_set = msm_dp_bridge_mode_set,
> .mode_valid = msm_edp_bridge_mode_valid,
> .atomic_reset = drm_atomic_helper_bridge_reset,
> .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h b/drivers/gpu/drm/msm/dp/dp_drm.h
> index 6c0426803d78..6d4cbb9f3918 100644
> --- a/drivers/gpu/drm/msm/dp/dp_drm.h
> +++ b/drivers/gpu/drm/msm/dp/dp_drm.h
> @@ -36,9 +36,6 @@ void msm_dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
> enum drm_mode_status msm_dp_bridge_mode_valid(struct drm_bridge *bridge,
> const struct drm_display_info *info,
> const struct drm_display_mode *mode);
> -void msm_dp_bridge_mode_set(struct drm_bridge *drm_bridge,
> - const struct drm_display_mode *mode,
> - const struct drm_display_mode *adjusted_mode);
> void msm_dp_bridge_hpd_enable(struct drm_bridge *bridge);
> void msm_dp_bridge_hpd_disable(struct drm_bridge *bridge);
> void msm_dp_bridge_hpd_notify(struct drm_bridge *bridge,
>
> --
> 2.43.0
>

--
With best wishes
Dmitry