On Fri, 26 Jan 2024, Mario Limonciello <mario.limonciello@xxxxxxx> 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@xxxxxxx>
> ---
> 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.
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)
--
Jani Nikula, Intel
