On Thu, Sep 19, 2024 at 12:06 PM Mario Limonciello
<mario.limonciello@xxxxxxx> wrote:
>
> On 9/19/2024 11:03, Alex Hung wrote:
> > A minor comment (see inline below).
> >
> > Otherwise
> >
> > Reviewed-by: Alex Hung <alex.hung@xxxxxxx>
> >
> > 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@xxxxxxx>
> >> ---
> >> .../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.
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;
>
