| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4jrpa7iyygciuy2k4ydk7cpm5isdrddclljf6gbyvkiqc645tx@idyds4tkstkx>
Date: Wed, 25 Jun 2025 17:03:31 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Yongxing Mou <quic_yongmou@...cinc.com>
Cc: Rob Clark <robin.clark@....qualcomm.com>,
Abhinav Kumar <abhinav.kumar@...ux.dev>,
Jessica Zhang <jessica.zhang@....qualcomm.com>,
Sean Paul <sean@...rly.run>,
Marijn Suijten <marijn.suijten@...ainline.org>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
linux-arm-msm@...r.kernel.org, dri-devel@...ts.freedesktop.org,
freedreno@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
Abhinav Kumar <quic_abhinavk@...cinc.com>
Subject: Re: [PATCH v2 02/38] drm/msm/dp: remove dp_display's dp_mode and use
dp_panel's instead
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_abhinavk@...cinc.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.
>
> 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.
> 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.
>
> Thanks~
> > >
> > > Signed-off-by: Abhinav Kumar <quic_abhinavk@...cinc.com>
> > > Signed-off-by: Yongxing Mou <quic_yongmou@...cinc.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.
> > > -
> > > - 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
> > >
> >
>
--
With best wishes
Dmitry
Powered by blists - more mailing lists