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