[PATCH v7 10/10] drm/amd/display: Fetch the EDID from _DDC if available for eDP
Hamza Mahfooz
hamza.mahfooz at amd.com
Thu Sep 19 17:36:31 UTC 2024
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.
>
> 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;
>>
--
Hamza
More information about the dri-devel
mailing list