[PATCH v2 01/38] drm/msm/dp: split msm_dp_panel_read_sink_caps() into two parts and drop panel drm_edid
Dmitry Baryshkov
dmitry.baryshkov at oss.qualcomm.com
Wed Aug 6 10:39:52 UTC 2025
On 06/08/2025 14:33, Yongxing Mou wrote:
>
>
> On 2025/6/27 20:40, Dmitry Baryshkov wrote:
>> On 27/06/2025 10:49, Yongxing Mou wrote:
>>>
>>>
>>> On 2025/6/25 21:32, Dmitry Baryshkov wrote:
>>>> 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 at quicinc.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 at quicinc.com>
>>>>>>> Signed-off-by: Yongxing Mou <quic_yongmou at quicinc.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.
>>>>
>>> Got it, thanks, will separate this patch from MST patches.. Even if
>>> we use drm_dp_send_real_edid_checksum to send connector-
>>> >real_edid_checksum, that’s only when the EDID state is incorrect.
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
>>> tree/ drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c?
>>> h=v6.16-rc3#n1020
>>> When the EDID is read correctly, it should send edid->checksum
>>> instead.
>>
>> I wonder if we should fix the drm_edid to always set
>> real_edid_checksum instead.
>>
> Emm, can i understand that is another issue exist in the currently DRM..
Currently it's more of a design issue.
--
With best wishes
Dmitry
More information about the dri-devel
mailing list