| 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: <326bbd02-f414-48e3-a396-4b94f19054f7@quicinc.com>
Date: Wed, 25 Jun 2025 16:43:55 +0800
From: Yongxing Mou <quic_yongmou@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.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 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.
>> +
>> + if (!drm_edid)
>> + return;
>> +
>> /* FIXME: get rid of drm_edid_raw() */
>> - const struct edid *edid = drm_edid_raw(msm_dp_panel->drm_edid);
>> + const struct edid *edid = drm_edid_raw(drm_edid);
>> u8 checksum;
>>
>> if (edid)
>> @@ -515,11 +528,3 @@ struct msm_dp_panel *msm_dp_panel_get(struct device *dev, struct drm_dp_aux *aux
>>
>> return msm_dp_panel;
>> }
>> -
>> -void msm_dp_panel_put(struct msm_dp_panel *msm_dp_panel)
>> -{
>> - if (!msm_dp_panel)
>> - return;
>> -
>> - drm_edid_free(msm_dp_panel->drm_edid);
>> -}
>
> Too many changes to be stuffed under the hood of "Also perform foo"
>
Got it, thanks , will split this patch and refactor the commit msg.
>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
>> index 4906f4f09f2451cfed3c1007f38b4db7dfdb1d90..7f139478e1012d5b8f1f745f0de5fc3943745428 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
>> @@ -32,7 +32,6 @@ struct msm_dp_panel {
>> u8 downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
>>
>> struct msm_dp_link_info link_info;
>> - const struct drm_edid *drm_edid;
>> struct drm_connector *connector;
>> struct msm_dp_display_mode msm_dp_mode;
>> struct msm_dp_panel_psr psr_cap;
>> @@ -51,7 +50,9 @@ int msm_dp_panel_timing_cfg(struct msm_dp_panel *msm_dp_panel);
>> int msm_dp_panel_read_sink_caps(struct msm_dp_panel *msm_dp_panel,
>> struct drm_connector *connector);
>> u32 msm_dp_panel_get_mode_bpp(struct msm_dp_panel *msm_dp_panel, u32 mode_max_bpp,
>> - u32 mode_pclk_khz);
>> + u32 mode_pclk_khz);
>> +int msm_dp_panel_read_link_caps(struct msm_dp_panel *dp_panel);
>> +int msm_dp_panel_read_edid(struct msm_dp_panel *dp_panel, struct drm_connector *connector);
>> int msm_dp_panel_get_modes(struct msm_dp_panel *msm_dp_panel,
>> struct drm_connector *connector);
>> void msm_dp_panel_handle_sink_request(struct msm_dp_panel *msm_dp_panel);
>> @@ -86,5 +87,4 @@ static inline bool is_lane_count_valid(u32 lane_count)
>>
>> struct msm_dp_panel *msm_dp_panel_get(struct device *dev, struct drm_dp_aux *aux,
>> struct msm_dp_link *link, struct msm_dp_catalog *catalog);
>> -void msm_dp_panel_put(struct msm_dp_panel *msm_dp_panel);
>> #endif /* _DP_PANEL_H_ */
>>
>> --
>> 2.34.1
>>
>
Powered by blists - more mailing lists