Re: [PATCH v2 01/38] drm/msm/dp: split msm_dp_panel_read_sink_caps() into two parts and drop panel drm_edid

[Yongxing Mou <quic_yongmou@quicinc.com>]

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@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@quicinc.com>
>>>>>> Signed-off-by: Yongxing Mou <quic_yongmou@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..

>>>
>>
> 
>