[PATCH v7 10/10] drm/amd/display: Fetch the EDID from _DDC if available for eDP

[Mario Limonciello]

On 9/19/2024 12:36, Hamza Mahfooz wrote:
> On 9/19/24 13:29, Alex Deucher wrote:
>> On Thu, Sep 19, 2024 at 12:06 PM Mario Limonciello
>> <mario.limonciello at amd.com> wrote:
>>>
>>> On 9/19/2024 11:03, Alex Hung wrote:
>>>> A minor comment (see inline below).
>>>>
>>>> Otherwise
>>>>
>>>> Reviewed-by: Alex Hung <alex.hung at amd.com>
>>>>
>>>> On 2024-09-18 15:38, Mario Limonciello wrote:
>>>>> Some manufacturers have intentionally put an EDID that differs from
>>>>> the EDID on the internal panel on laptops.
>>>>>
>>>>> Attempt to fetch this EDID if it exists and prefer it over the EDID
>>>>> that is provided by the panel. If a user prefers to use the EDID from
>>>>> the panel, offer a DC debugging parameter that would disable this.
>>>>>
>>>>> Signed-off-by: Mario Limonciello <mario.limonciello at amd.com>
>>>>> ---
>>>>>    .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 62 
>>>>> ++++++++++++++++++-
>>>>>    drivers/gpu/drm/amd/include/amd_shared.h      |  5 ++
>>>>>    2 files changed, 66 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>>>>> index 8f4be7a1ec45..05d3e00ecce0 100644
>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>>>>> @@ -23,6 +23,8 @@
>>>>>     *
>>>>>     */
>>>>> +#include <acpi/video.h>
>>>>> +
>>>>>    #include <linux/string.h>
>>>>>    #include <linux/acpi.h>
>>>>>    #include <linux/i2c.h>
>>>>> @@ -874,6 +876,60 @@ bool dm_helpers_is_dp_sink_present(struct dc_link
>>>>> *link)
>>>>>        return dp_sink_present;
>>>>>    }
>>>>> +static int
>>>>> +dm_helpers_probe_acpi_edid(void *data, u8 *buf, unsigned int block,
>>>>> size_t len)
>>>>> +{
>>>>> +    struct drm_connector *connector = data;
>>>>> +    struct acpi_device *acpidev = 
>>>>> ACPI_COMPANION(connector->dev->dev);
>>>>> +    unsigned char start = block * EDID_LENGTH;
>>>>> +    void *edid;
>>>>> +    int r;
>>>>> +
>>>>> +    if (!acpidev)
>>>>> +        return -ENODEV;
>>>>> +
>>>>> +    /* fetch the entire edid from BIOS */
>>>>> +    r = acpi_video_get_edid(acpidev, ACPI_VIDEO_DISPLAY_LCD, -1, 
>>>>> &edid);
>>>>> +    if (r < 0) {
>>>>> +        DRM_DEBUG_KMS("Failed to get EDID from ACPI: %d\n", r);
>>>>> +        return r;
>>>>> +    }
>>>>> +    if (len > r || start > r || start + len > r) {
>>>>> +        r = -EINVAL;
>>>>> +        goto cleanup;
>>>>> +    }
>>>>> +
>>>>> +    memcpy(buf, edid + start, len);
>>>>> +    r = 0;
>>>>> +
>>>>> +cleanup:
>>>>> +    kfree(edid);
>>>>> +
>>>>> +    return r;
>>>>> +}
>>>>> +
>>>>> +static const struct drm_edid *
>>>>> +dm_helpers_read_acpi_edid(struct amdgpu_dm_connector *aconnector)
>>>>> +{
>>>>> +    struct drm_connector *connector = &aconnector->base;
>>>>> +
>>>>> +    if (amdgpu_dc_debug_mask & DC_DISABLE_ACPI_EDID)
>>>>> +        return NULL;
>>>>> +
>>>>> +    switch (connector->connector_type) {
>>>>> +    case DRM_MODE_CONNECTOR_LVDS:
>>>>> +    case DRM_MODE_CONNECTOR_eDP:
>>>>> +        break;
>>>>> +    default:
>>>>> +        return NULL;
>>>>> +    }
>>>>> +
>>>>> +    if (connector->force == DRM_FORCE_OFF)
>>>>> +        return NULL;
>>>>> +
>>>>> +    return drm_edid_read_custom(connector,
>>>>> dm_helpers_probe_acpi_edid, connector);
>>>>> +}
>>>>> +
>>>>>    enum dc_edid_status dm_helpers_read_local_edid(
>>>>>            struct dc_context *ctx,
>>>>>            struct dc_link *link,
>>>>> @@ -896,7 +952,11 @@ enum dc_edid_status dm_helpers_read_local_edid(
>>>>>         * do check sum and retry to make sure read correct edid.
>>>>>         */
>>>>>        do {
>>>>> -        drm_edid = drm_edid_read_ddc(connector, ddc);
>>>>> +        drm_edid = dm_helpers_read_acpi_edid(aconnector);
>>>>> +        if (drm_edid)
>>>>> +            DRM_DEBUG_KMS("Using ACPI provided EDID for %s\n",
>>>>> connector->name);
>>>>
>>>> It is better to always output a message when ACPI's EDID is used 
>>>> without
>>>> enabling any debug options. How about DRM_INFO?
>>>
>>> Thanks, DRM_INFO makes sense for discoverability and will adjust it.
>>
>> I'd suggest using dev_info() or one of the newer DRM macros so we know
>> which device we are talking about if there are multiple GPUs in the
>> system.
> 
> Ya, I'd personally prefer moving amdgpu_dm over to the new(er) drm_.*
> family of logging macros.

Thanks.  I've adjusted it to:

drm_info(connector->dev, "Using ACPI provided EDID for %s\n", 
connector->name);

Also there is a debug one used above that I adjusted to:

drm_dbg(connector->dev, "Failed to get EDID from ACPI: %d\n", r);

> 
>>
>> Alex
>>
>>>
>>>>
>>>>> +        else
>>>>> +            drm_edid = drm_edid_read_ddc(connector, ddc);
>>>>>            drm_edid_connector_update(connector, drm_edid);
>>>>>            aconnector->drm_edid = drm_edid;
>>>>> diff --git a/drivers/gpu/drm/amd/include/amd_shared.h
>>>>> b/drivers/gpu/drm/amd/include/amd_shared.h
>>>>> index 3f91926a50e9..1ec7c5e5249e 100644
>>>>> --- a/drivers/gpu/drm/amd/include/amd_shared.h
>>>>> +++ b/drivers/gpu/drm/amd/include/amd_shared.h
>>>>> @@ -337,6 +337,11 @@ enum DC_DEBUG_MASK {
>>>>>         * @DC_FORCE_IPS_ENABLE: If set, force enable all IPS, all the
>>>>> time.
>>>>>         */
>>>>>        DC_FORCE_IPS_ENABLE = 0x4000,
>>>>> +    /**
>>>>> +     * @DC_DISABLE_ACPI_EDID: If set, don't attempt to fetch EDID for
>>>>> +     * eDP display from ACPI _DDC method.
>>>>> +     */
>>>>> +    DC_DISABLE_ACPI_EDID = 0x8000,
>>>>>    };
>>>>>    enum amd_dpm_forced_level;
>>>