Re: [PATCH v2 02/38] drm/msm/dp: remove dp_display's dp_mode and use dp_panel's instead

Wed, 06 Aug 2025 02:22:49 -0700 



On 2025/6/27 20:44, Dmitry Baryshkov wrote:

On 27/06/2025 11:40, Yongxing Mou wrote:



On 2025/6/25 22:03, Dmitry Baryshkov wrote:

On Wed, Jun 25, 2025 at 08:34:18PM +0800, Yongxing Mou wrote:



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_abhin...@quicinc.com>

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.


So, if we do everything during .atomic_enable(), there would be no need
to store and/or copy anything. All the data is available and can be used
as is.


Got it. Let me confirm—can we keep msm_dp_mode or drm_display_mode in 
msm_dp_panel? Mabey debug node will use this ..



Please don't. I really dislike storing drm_atomic_state-related 
variables in a non-state structure. I think it makes it easier to 
mistakenly update or to use a stale value.



Debug code already prints modes in debugfs/dri/N/state. If we need any 
other state-related prints, they should go to the same file.



Hi, I got this point.. i go through the driver. since lots of funcs used 
msm_dp_mode cached. so maybe it is not a very small change.. I’d like to 
prioritize MST first, and then submit this patch once I got time..





This design has been in place since the first version of the DP 
driver and

has remained largely unchanged.


Yes... The point is that you are touching this piece of code anyway,
let's make it nicer.


Agree with this point.

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?


Sure... But then you have to change two places. If you optimize it
first, you have to touch only place. And it can be even submitted
separately.


Understood, that’s indeed the case. I just want to prioritize the MST 
patch and have it merged first, since it involves changes to lots of 
files. Thanks~~


Thanks~


Signed-off-by: Abhinav Kumar <quic_abhin...@quicinc.com>
Signed-off-by: Yongxing Mou <quic_yong...@quicinc.com>
---

   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..


Yep. Drop .mode_set, the callback is even described as deprecated.


Thanks, the documentation does state that.

-
-    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




































					Reply via email to