| Message ID | 20240918213845.158293-11-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amd/display: Use drm_edid for more code | expand |
A minor comment (see inline below). Otherwise Reviewed-by: Alex Hung <alex.hung@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@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? > + 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;
On 9/19/2024 11:03, Alex Hung wrote: > A minor comment (see inline below). > > Otherwise > > Reviewed-by: Alex Hung <alex.hung@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@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. > >> + 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;
On Thu, Sep 19, 2024 at 12:06 PM Mario Limonciello <mario.limonciello@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@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@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. 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; >
On 9/19/24 13:29, Alex Deucher wrote: > On Thu, Sep 19, 2024 at 12:06 PM Mario Limonciello > <mario.limonciello@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@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@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; >>
On 9/19/2024 12:36, Hamza Mahfooz wrote: > On 9/19/24 13:29, Alex Deucher wrote: >> On Thu, Sep 19, 2024 at 12:06 PM Mario Limonciello >> <mario.limonciello@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@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@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. Thanks. I've adjusted it to: drm_info(connector->dev, "Using ACPI provided EDID for %s\n", connector->name); Also there is a debug one used above that I adjusted to: drm_dbg(connector->dev, "Failed to get EDID from ACPI: %d\n", r); > >> >> 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; >>>
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); + 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;
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@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(-)