| 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: <buvgxzf5u5wkj2nxd6rquvcktjmxoclwrkkmxeih6pnikubqe3@yoytvnayvxtv>
Date: Wed, 25 Jun 2025 16:32:55 +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 01/38] drm/msm/dp: split msm_dp_panel_read_sink_caps()
into two parts and drop panel drm_edid
On Wed, Jun 25, 2025 at 04:43:55PM +0800, Yongxing Mou wrote:
>
>
> On 2025/6/9 20:41, Dmitry Baryshkov wrote:
> > On Mon, Jun 09, 2025 at 08:21:20PM +0800, Yongxing Mou wrote:
> > > From: Abhinav Kumar <quic_abhinavk@...cinc.com>
> > >
> > > In preparation of DP MST where link caps are read for the
> > > immediate downstream device and the edid is read through
> >
> > EDID, not edid. Please review all your patches for up/down case.
> >
> Got it. Thanks~
> > > sideband messaging, split the msm_dp_panel_read_sink_caps() into
> > > two parts which read the link parameters and the edid parts
> > > respectively. Also drop the panel drm_edid cached as we actually
> > > don't need it.
> >
> > Also => separate change.
> >
> Got it.
> > >
> > > 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 | 13 +++++----
> > > drivers/gpu/drm/msm/dp/dp_panel.c | 55 ++++++++++++++++++++-----------------
> > > drivers/gpu/drm/msm/dp/dp_panel.h | 6 ++--
> > > 3 files changed, 40 insertions(+), 34 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > > index 6f05a939ce9e648e9601597155999b6f85adfcff..4a9b65647cdef1ed6c3bb851f93df0db8be977af 100644
> > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > @@ -389,7 +389,11 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
> > > dp->link->lttpr_count = msm_dp_display_lttpr_init(dp, dpcd);
> > > - rc = msm_dp_panel_read_sink_caps(dp->panel, connector);
> > > + rc = msm_dp_panel_read_link_caps(dp->panel);
> > > + if (rc)
> > > + goto end;
> > > +
> > > + rc = msm_dp_panel_read_edid(dp->panel, connector);
> > > if (rc)
> > > goto end;
> > > @@ -720,7 +724,6 @@ static int msm_dp_irq_hpd_handle(struct msm_dp_display_private *dp, u32 data)
> > > static void msm_dp_display_deinit_sub_modules(struct msm_dp_display_private *dp)
> > > {
> > > msm_dp_audio_put(dp->audio);
> > > - msm_dp_panel_put(dp->panel);
> > > msm_dp_aux_put(dp->aux);
> > > }
> > > @@ -783,7 +786,7 @@ static int msm_dp_init_sub_modules(struct msm_dp_display_private *dp)
> > > rc = PTR_ERR(dp->ctrl);
> > > DRM_ERROR("failed to initialize ctrl, rc = %d\n", rc);
> > > dp->ctrl = NULL;
> > > - goto error_ctrl;
> > > + goto error_link;
> > > }
> > > dp->audio = msm_dp_audio_get(dp->msm_dp_display.pdev, dp->catalog);
> > > @@ -791,13 +794,11 @@ static int msm_dp_init_sub_modules(struct msm_dp_display_private *dp)
> > > rc = PTR_ERR(dp->audio);
> > > pr_err("failed to initialize audio, rc = %d\n", rc);
> > > dp->audio = NULL;
> > > - goto error_ctrl;
> > > + goto error_link;
> > > }
> > > return rc;
> > > -error_ctrl:
> > > - msm_dp_panel_put(dp->panel);
> > > error_link:
> > > msm_dp_aux_put(dp->aux);
> > > error:
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
> > > index 4e8ab75c771b1e3a2d62f75e9993e1062118482b..d9041e235104a74b3cc50ff2e307eae0c4301ef3 100644
> > > --- a/drivers/gpu/drm/msm/dp/dp_panel.c
> > > +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> > > @@ -118,14 +118,13 @@ static u32 msm_dp_panel_get_supported_bpp(struct msm_dp_panel *msm_dp_panel,
> > > return min_supported_bpp;
> > > }
> > > -int msm_dp_panel_read_sink_caps(struct msm_dp_panel *msm_dp_panel,
> > > - struct drm_connector *connector)
> > > +int msm_dp_panel_read_link_caps(struct msm_dp_panel *msm_dp_panel)
> > > {
> > > int rc, bw_code;
> > > int count;
> > > struct msm_dp_panel_private *panel;
> > > - if (!msm_dp_panel || !connector) {
> > > + if (!msm_dp_panel) {
> > > DRM_ERROR("invalid input\n");
> > > return -EINVAL;
> > > }
> > > @@ -160,26 +159,29 @@ int msm_dp_panel_read_sink_caps(struct msm_dp_panel *msm_dp_panel,
> > > rc = drm_dp_read_downstream_info(panel->aux, msm_dp_panel->dpcd,
> > > msm_dp_panel->downstream_ports);
> > > - if (rc)
> > > - return rc;
> > > + return rc;
> > > +}
> > > - drm_edid_free(msm_dp_panel->drm_edid);
> > > +int msm_dp_panel_read_edid(struct msm_dp_panel *msm_dp_panel, struct drm_connector *connector)
> > > +{
> > > + struct msm_dp_panel_private *panel;
> > > + const struct drm_edid *drm_edid;
> > > +
> > > + panel = container_of(msm_dp_panel, struct msm_dp_panel_private, msm_dp_panel);
> > > - msm_dp_panel->drm_edid = drm_edid_read_ddc(connector, &panel->aux->ddc);
> > > + drm_edid = drm_edid_read_ddc(connector, &panel->aux->ddc);
> > > - drm_edid_connector_update(connector, msm_dp_panel->drm_edid);
> > > + drm_edid_connector_update(connector, drm_edid);
> > > - if (!msm_dp_panel->drm_edid) {
> > > + if (!drm_edid) {
> > > DRM_ERROR("panel edid read failed\n");
> > > /* check edid read fail is due to unplug */
> > > if (!msm_dp_catalog_link_is_connected(panel->catalog)) {
> > > - rc = -ETIMEDOUT;
> > > - goto end;
> > > + return -ETIMEDOUT;
> > > }
> > > }
> > > -end:
> > > - return rc;
> > > + return 0;
> > > }
> > > u32 msm_dp_panel_get_mode_bpp(struct msm_dp_panel *msm_dp_panel,
> > > @@ -208,15 +210,20 @@ u32 msm_dp_panel_get_mode_bpp(struct msm_dp_panel *msm_dp_panel,
> > > int msm_dp_panel_get_modes(struct msm_dp_panel *msm_dp_panel,
> > > struct drm_connector *connector)
> > > {
> > > + struct msm_dp_panel_private *panel;
> > > + const struct drm_edid *drm_edid;
> > > +
> > > if (!msm_dp_panel) {
> > > DRM_ERROR("invalid input\n");
> > > return -EINVAL;
> > > }
> > > - if (msm_dp_panel->drm_edid)
> > > - return drm_edid_connector_add_modes(connector);
> > > + panel = container_of(msm_dp_panel, struct msm_dp_panel_private, msm_dp_panel);
> > > +
> > > + drm_edid = drm_edid_read_ddc(connector, &panel->aux->ddc);
> > > + drm_edid_connector_update(connector, drm_edid);
> >
> > If EDID has been read and processed after HPD high event, why do we need
> > to re-read it again? Are we expecting that EDID will change?
> >
> Here we indeed don't need to read the EDID again, so we can directly call
> drm_edid_connector_add_modes. Thanks.
> > > - return 0;
> > > + return drm_edid_connector_add_modes(connector);
> > > }
> > > static u8 msm_dp_panel_get_edid_checksum(const struct edid *edid)
> > > @@ -229,6 +236,7 @@ static u8 msm_dp_panel_get_edid_checksum(const struct edid *edid)
> > > void msm_dp_panel_handle_sink_request(struct msm_dp_panel *msm_dp_panel)
> > > {
> > > struct msm_dp_panel_private *panel;
> > > + const struct drm_edid *drm_edid;
> > > if (!msm_dp_panel) {
> > > DRM_ERROR("invalid input\n");
> > > @@ -238,8 +246,13 @@ void msm_dp_panel_handle_sink_request(struct msm_dp_panel *msm_dp_panel)
> > > panel = container_of(msm_dp_panel, struct msm_dp_panel_private, msm_dp_panel);
> > > if (panel->link->sink_request & DP_TEST_LINK_EDID_READ) {
> > > + drm_edid = drm_edid_read_ddc(msm_dp_panel->connector, &panel->aux->ddc);
> >
> > And again....
> >
> Here we need the struct edid,since we drop the cached drm_edid, so we need
> to read it again. Or we can return the drm_edid from msm_dp_panel_read_edid
> and pass it to msm_dp_panel_handle_sink_request, then we don't need to read
> drm_edid here. Emm, I'm still a bit curious why we can't cache the drm_edid?
> It would help us to access it when needed. Emm, i see other drivers also
> cache it.
Yes, they can cache EDID. However, in this case we don't even need it at
all. This piece needs to be rewritten to use
drm_dp_send_real_edid_checksum(), connector->real_edid_checksum.
Corresponding changes can be submitted separately.
--
With best wishes
Dmitry
Powered by blists - more mailing lists