Re: [PATCH v2 02/38] drm/msm/dp: remove dp_display's dp_mode and use dp_panel's instead
From: Yongxing Mou
Date: Wed Jun 25 2025 - 08:35:19 EST
On 2025/6/9 20:48, Dmitry Baryshkov wrote:
On Mon, Jun 09, 2025 at 08:21:21PM +0800, Yongxing Mou wrote:
From: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
dp_display caches the current display mode and then passes it onto
the panel to be used for programming the panel params. Remove this
two level passing and directly populated the panel's dp_display_mode
instead.
- Why do we need to cache / copy it anyway? Can't we just pass the
corresponding drm_atomic_state / drm_crtc_state / drm_display_mode ?
This part works as follows: .mode_set() copies the adjusted_mode into
msm_dp_display_private->msm_dp_display_mode, and also parses and stores
variables such as v_active_low/h_active_low/out_fmt_is_yuv_420 and ...
When @drm_bridge_funcs.atomic_enable() is called, it copies
msm_dp_display->msm_dp_mode into dp_panel->msm_dp_mode and initializes
panel_info in msm_dp_display_set_mode(). Then when go to
msm_dp_ctrl_on_stream(), the parameters are updated into the
corresponding hardware registers.
This design has been in place since the first version of the DP driver
and has remained largely unchanged. Originally, the drm_mode would be
passed in two stages: from msm_dp_display->msm_dp_mode to
dp_panel->msm_dp_mode. Since in MST mode each stream requires its own
drm_mode and stored in dp_panel, we simplified the two-stage transfer
into a single step (.mode_set() do all things and store in
msm_dp_panel). Meanwhile we modified the msm_dp_display_set_mode
function to accept a msm_dp_panel parameter, allowing the MST bridge
funcs' mode_set() to reuse this part code.
The following patches:
https://patchwork.freedesktop.org/patch/657573/?series=142207&rev=2 and
https://patchwork.freedesktop.org/patch/657593/?series=142207&rev=2,
introduce msm_dp_display_*_helper functions to help reuse common code
across MST/SST/eDP drm_bridge_funcs.
If we drop msm_dp_mode from dp_panel and use drm_display_mode, it might
introduce a large number of changes that are not directly related to
MST. Actually i think the presence of msm_dp_display_mode seems to
simplify the work in msm_dp_panel_timing_cfg(), this patch series we
want to focus on MST parts, so would we consider optimizing them later?
Thanks~
Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
Signed-off-by: Yongxing Mou <quic_yongmou@xxxxxxxxxxx>
---
drivers/gpu/drm/msm/dp/dp_display.c | 76 ++++++++++++++-----------------------
1 file changed, 29 insertions(+), 47 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 4a9b65647cdef1ed6c3bb851f93df0db8be977af..9d2db9cbd2552470a36a63f70f517c35436f7280 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -92,7 +92,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 */
@@ -806,16 +805,29 @@ 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(&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;
+
+ 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;
- 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);
+ msm_dp_panel_init_panel_info(msm_dp_panel);
return 0;
}
@@ -1431,10 +1443,13 @@ bool msm_dp_needs_periph_flush(const struct msm_dp *msm_dp_display,
bool msm_dp_wide_bus_available(const struct msm_dp *msm_dp_display)
{
struct msm_dp_display_private *dp;
+ struct msm_dp_panel *dp_panel;
dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
- if (dp->msm_dp_mode.out_fmt_is_yuv_420)
+ dp_panel = dp->panel;
+
+ if (dp_panel->msm_dp_mode.out_fmt_is_yuv_420)
return false;
return dp->wide_bus_supported;
@@ -1496,10 +1511,6 @@ void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
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");
- return;
- }
if (dp->is_edp)
msm_dp_hpd_plug_handle(msm_dp_display, 0);
@@ -1517,15 +1528,6 @@ void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
return;
}
- rc = msm_dp_display_set_mode(dp, &msm_dp_display->msm_dp_mode);
- if (rc) {
- DRM_ERROR("Failed to perform a mode set, rc=%d\n", rc);
- mutex_unlock(&msm_dp_display->event_mutex);
- return;
- }
It should be done other way around: keep this call and drop
msm_dp_bridge_mode_set().
Emm as reply in last comments..
-
- hpd_state = msm_dp_display->hpd_state;
-
if (hpd_state == ST_CONNECTED && !dp->power_on) {
msm_dp_display_host_phy_init(msm_dp_display);
force_link_train = true;
@@ -1604,33 +1606,13 @@ void msm_dp_bridge_mode_set(struct drm_bridge *drm_bridge,
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;
+ msm_dp_display_set_mode(dp, adjusted_mode, msm_dp_panel);
/* 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;
- msm_dp_display->catalog->wide_bus_en =
- msm_dp_display->msm_dp_mode.out_fmt_is_yuv_420 ? false : msm_dp_display->wide_bus_supported;
+ msm_dp_display->ctrl->wide_bus_en = msm_dp_panel->msm_dp_mode.out_fmt_is_yuv_420 ?
+ false : msm_dp_display->wide_bus_supported;
+ msm_dp_display->catalog->wide_bus_en = msm_dp_panel->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)
--
2.34.1