| 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: <63c60424-1b2d-4912-81b2-7c7ead4c8289@amd.com>
Date: Mon, 29 Jan 2024 10:09:35 -0600
From: Mario Limonciello <mario.limonciello@....com>
To: Jani Nikula <jani.nikula@...ux.intel.com>, amd-gfx@...ts.freedesktop.org,
Alex Deucher <alexander.deucher@....com>,
Harry Wentland <harry.wentland@....com>,
"Rafael J . Wysocki" <rafael@...nel.org>, Hans de Goede <hdegoede@...hat.com>
Cc: open list <linux-kernel@...r.kernel.org>,
"open list:DRM DRIVERS" <dri-devel@...ts.freedesktop.org>,
Melissa Wen <mwen@...lia.com>, "open list:ACPI"
<linux-acpi@...r.kernel.org>, Mark Pearson <mpearson-lenovo@...ebb.ca>
Subject: Re: [PATCH 2/2] drm/amd: Fetch the EDID from _DDC if available for
eDP
On 1/29/2024 03:39, Jani Nikula wrote:
> On Fri, 26 Jan 2024, Mario Limonciello <mario.limonciello@....com> 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.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 ++
>> drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 30 +++++++++++++++++++
>> .../gpu/drm/amd/amdgpu/amdgpu_connectors.c | 5 ++++
>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 ++++-
>> .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 7 +++--
>> 5 files changed, 49 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> index c5f3859fd682..99abe12567a4 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -1520,6 +1520,7 @@ int amdgpu_acpi_get_mem_info(struct amdgpu_device *adev, int xcc_id,
>>
>> void amdgpu_acpi_get_backlight_caps(struct amdgpu_dm_backlight_caps *caps);
>> bool amdgpu_acpi_should_gpu_reset(struct amdgpu_device *adev);
>> +void *amdgpu_acpi_edid(struct amdgpu_device *adev, struct drm_connector *connector);
>> void amdgpu_acpi_detect(void);
>> void amdgpu_acpi_release(void);
>> #else
>> @@ -1537,6 +1538,7 @@ static inline int amdgpu_acpi_get_mem_info(struct amdgpu_device *adev,
>> }
>> static inline void amdgpu_acpi_fini(struct amdgpu_device *adev) { }
>> static inline bool amdgpu_acpi_should_gpu_reset(struct amdgpu_device *adev) { return false; }
>> +static inline void *amdgpu_acpi_edid(struct amdgpu_device *adev, struct drm_connector *connector) { return NULL; }
>> static inline void amdgpu_acpi_detect(void) { }
>> static inline void amdgpu_acpi_release(void) { }
>> static inline bool amdgpu_acpi_is_power_shift_control_supported(void) { return false; }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>> index e550067e5c5d..c106335f1f22 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>> @@ -1380,6 +1380,36 @@ bool amdgpu_acpi_should_gpu_reset(struct amdgpu_device *adev)
>> #endif
>> }
>>
>> +/**
>> + * amdgpu_acpi_edid
>> + * @adev: amdgpu_device pointer
>> + * @connector: drm_connector pointer
>> + *
>> + * Returns the EDID used for the internal panel if present, NULL otherwise.
>> + */
>> +void *
>> +amdgpu_acpi_edid(struct amdgpu_device *adev, struct drm_connector *connector)
>> +{
>> + struct drm_device *ddev = adev_to_drm(adev);
>> + struct acpi_device *acpidev = ACPI_COMPANION(ddev->dev);
>> + void *edid;
>> + int r;
>> +
>> + if (!acpidev)
>> + return NULL;
>> +
>> + if (connector->connector_type != DRM_MODE_CONNECTOR_eDP)
>> + return NULL;
>> +
>> + r = acpi_video_get_edid(acpidev, ACPI_VIDEO_DISPLAY_LCD, -1, &edid);
>> + if (r < 0) {
>> + DRM_DEBUG_DRIVER("Failed to get EDID from ACPI: %d\n", r);
>> + return NULL;
>> + }
>> +
>> + return kmemdup(edid, r, GFP_KERNEL);
>> +}
>> +
>> /*
>> * amdgpu_acpi_detect - detect ACPI ATIF/ATCS methods
>> *
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>> index 9caba10315a8..c7e1563a46d3 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>> @@ -278,6 +278,11 @@ static void amdgpu_connector_get_edid(struct drm_connector *connector)
>> struct amdgpu_device *adev = drm_to_adev(dev);
>> struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
>>
>> + if (amdgpu_connector->edid)
>> + return;
>> +
>> + /* if the BIOS specifies the EDID via _DDC, prefer this */
>> + amdgpu_connector->edid = amdgpu_acpi_edid(adev, connector);
>
> Imagine the EDID returned by acpi_video_get_edid() has edid->extensions
> bigger than 4. Of course it should not, but you have no guarantees, and
> it originates outside of the kernel.
>
> The real fix is to have the function return a struct drm_edid which
> tracks the allocation size separately. Unfortunately, it requires a
> bunch of changes along the way. We've mostly done it in i915, and I've
> sent a series to do this in drm/bridge [1].
>
> Bottom line, we should stop using struct edid in drivers. They'll all
> parse the info differently, and from what I've seen, often wrong.
>
>
Thanks for the feedback. In that case this specific change should
probably rebase on the Melissa's work
https://lore.kernel.org/amd-gfx/20240126163429.56714-1-mwen@igalia.com/
after she takes into account the feedback.
Let me ask you this though - do you think that after that's done should
we let all drivers get EDID from BIOS as a priority? Or would you
prefer that this is unique to amdgpu?
Something like:
1) If user specifies on kernel command line and puts an EDID in
/lib/firmware use that.
2) If BIOS has EDID in _DDC and it's eDP panel, use that.
3) Get panel EDID.
> BR,
> Jani.
>
>
> [1] https://patchwork.freedesktop.org/series/128149/
>
>
>> if (amdgpu_connector->edid)
>> return;
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index cd98b3565178..1faa21f542bd 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -6562,17 +6562,23 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector)
>> {
>> struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
>> struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
>> + struct amdgpu_device *adev = drm_to_adev(connector->dev);
>> struct dc_link *dc_link = aconnector->dc_link;
>> struct dc_sink *dc_em_sink = aconnector->dc_em_sink;
>> struct edid *edid;
>>
>> + /* prefer ACPI over panel for eDP */
>> + edid = amdgpu_acpi_edid(adev, connector);
>> +
>> /*
>> * Note: drm_get_edid gets edid in the following order:
>> * 1) override EDID if set via edid_override debugfs,
>> * 2) firmware EDID if set via edid_firmware module parameter
>> * 3) regular DDC read.
>> */
>> - edid = drm_get_edid(connector, &amdgpu_connector->ddc_bus->aux.ddc);
>> + if (!edid)
>> + edid = drm_get_edid(connector, &amdgpu_connector->ddc_bus->aux.ddc);
>> +
>> if (!edid) {
>> DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
>> return;
>> 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 e3915c4f8566..6bf2a8867e76 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
>> @@ -895,6 +895,7 @@ enum dc_edid_status dm_helpers_read_local_edid(
>> {
>> struct amdgpu_dm_connector *aconnector = link->priv;
>> struct drm_connector *connector = &aconnector->base;
>> + struct amdgpu_device *adev = drm_to_adev(connector->dev);
>> struct i2c_adapter *ddc;
>> int retry = 3;
>> enum dc_edid_status edid_status;
>> @@ -909,8 +910,10 @@ enum dc_edid_status dm_helpers_read_local_edid(
>> * do check sum and retry to make sure read correct edid.
>> */
>> do {
>> -
>> - edid = drm_get_edid(&aconnector->base, ddc);
>> + /* prefer ACPI over panel for eDP */
>> + edid = amdgpu_acpi_edid(adev, connector);
>> + if (!edid)
>> + edid = drm_get_edid(&aconnector->base, ddc);
>>
>> /* DP Compliance Test 4.2.2.6 */
>> if (link->aux_mode && connector->edid_corrupt)
>
Powered by blists - more mailing lists