Messages in this thread | ![/](/images/icornerl.gif) | | Date | Fri, 9 Feb 2024 09:34:13 -0600 | Subject | Re: [PATCH v4 1/3] drm: Add drm_get_acpi_edid() helper | From | Mario Limonciello <> |
| |
On 2/9/2024 05:07, Daniel Vetter wrote: > On Thu, Feb 08, 2024 at 11:57:11AM +0200, Jani Nikula wrote: >> On Wed, 07 Feb 2024, Mario Limonciello <mario.limonciello@amd.com> wrote: >>> Some manufacturers have intentionally put an EDID that differs from >>> the EDID on the internal panel on laptops. Drivers can call this >>> helper to attempt to fetch the EDID from the BIOS's ACPI _DDC method. >>> >>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >>> --- >>> drivers/gpu/drm/Kconfig | 5 +++ >>> drivers/gpu/drm/drm_edid.c | 77 ++++++++++++++++++++++++++++++++++++++ >>> include/drm/drm_edid.h | 1 + >>> 3 files changed, 83 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig >>> index 6ec33d36f3a4..ec2bb71e8b36 100644 >>> --- a/drivers/gpu/drm/Kconfig >>> +++ b/drivers/gpu/drm/Kconfig >>> @@ -21,6 +21,11 @@ menuconfig DRM >>> select KCMP >>> select VIDEO_CMDLINE >>> select VIDEO_NOMODESET >>> + select ACPI_VIDEO if ACPI >>> + select BACKLIGHT_CLASS_DEVICE if ACPI >>> + select INPUT if ACPI >>> + select X86_PLATFORM_DEVICES if ACPI && X86 >>> + select ACPI_WMI if ACPI && X86 >> >> I think I'll defer to drm maintainers on whether this is okay or >> something to be avoided. > > Uh yeah this is a bit much, and select just messes with everything. Just > #ifdef this in the code with a dummy alternative, if users configure their > kernel without acpi but need it, they get to keep all the pieces. > > Alternatively make a DRM_ACPI_HELPERS symbol, but imo a Kconfig for every > function is also not great. And just using #ifdef in the code also works > for CONFIG_OF, which is exactly the same thing for platforms using dt to > describe hw. > > Also I'd expect ACPI code to already provide dummy functions if ACPI is > provided, so you probably dont even need all that much #ifdef in the code. > > What we defo cant do is select platform/hw stuff just because you enable > CONFIG_DRM. > -Sima
The problem was with linking. I'll experiment with #ifdef for the next version.
> >> >> >>> help >>> Kernel-level support for the Direct Rendering Infrastructure (DRI) >>> introduced in XFree86 4.0. If you say Y here, you need to select >>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c >>> index 923c4423151c..c649b4f9fd8e 100644 >>> --- a/drivers/gpu/drm/drm_edid.c >>> +++ b/drivers/gpu/drm/drm_edid.c >>> @@ -28,6 +28,7 @@ >>> * DEALINGS IN THE SOFTWARE. >>> */ >>> >>> +#include <acpi/video.h> >>> #include <linux/bitfield.h> >>> #include <linux/cec.h> >>> #include <linux/hdmi.h> >>> @@ -2188,6 +2189,49 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len) >>> return ret == xfers ? 0 : -1; >>> } >>> >>> +/** >>> + * drm_do_probe_acpi_edid() - get EDID information via ACPI _DDC >>> + * @data: struct drm_device >>> + * @buf: EDID data buffer to be filled >>> + * @block: 128 byte EDID block to start fetching from >>> + * @len: EDID data buffer length to fetch >>> + * >>> + * Try to fetch EDID information by calling acpi_video_get_edid() function. >>> + * >>> + * Return: 0 on success or error code on failure. >>> + */ >>> +static int >>> +drm_do_probe_acpi_edid(void *data, u8 *buf, unsigned int block, size_t len) >>> +{ >>> + struct drm_device *ddev = data; >>> + struct acpi_device *acpidev = ACPI_COMPANION(ddev->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 -EINVAL; >>> + } >>> + 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 void connector_bad_edid(struct drm_connector *connector, >>> const struct edid *edid, int num_blocks) >>> { >>> @@ -2643,6 +2687,39 @@ struct edid *drm_get_edid(struct drm_connector *connector, >>> } >>> EXPORT_SYMBOL(drm_get_edid); >>> >>> +/** >>> + * drm_get_acpi_edid - get EDID data, if available >> >> I'd prefer all the new EDID API to be named drm_edid_*. Makes a clean >> break from the old API, and is more consistent. >> >> So perhaps drm_edid_read_acpi() to be in line with all the other struct >> drm_edid based EDID reading functions. >> >>> + * @connector: connector we're probing >>> + * >>> + * Use the BIOS to attempt to grab EDID data if possible. >>> + * >>> + * The returned pointer must be freed using drm_edid_free(). >>> + * >>> + * Return: Pointer to valid EDID or NULL if we couldn't find any. >>> + */ >>> +const struct drm_edid *drm_get_acpi_edid(struct drm_connector *connector) >>> +{ >>> + const struct drm_edid *drm_edid; >>> + >>> + 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; >>> + >>> + drm_edid = drm_edid_read_custom(connector, drm_do_probe_acpi_edid, connector->dev); >>> + >>> + /* Note: Do *not* call connector updates here. */ >>> + >>> + return drm_edid; >>> +} >>> +EXPORT_SYMBOL(drm_get_acpi_edid); >>> + >>> /** >>> * drm_edid_read_custom - Read EDID data using given EDID block read function >>> * @connector: Connector to use >>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h >>> index 7923bc00dc7a..ca41be289fc6 100644 >>> --- a/include/drm/drm_edid.h >>> +++ b/include/drm/drm_edid.h >>> @@ -410,6 +410,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, >>> void *data); >>> struct edid *drm_get_edid(struct drm_connector *connector, >>> struct i2c_adapter *adapter); >>> +const struct drm_edid *drm_get_acpi_edid(struct drm_connector *connector); >> >> There's a comment >> >> /* Interface based on struct drm_edid */ >> >> towards the end of the file, gathering all the new API under it. >> >> Other than that, LGTM, >> >> BR, >> Jani. >> >>> u32 drm_edid_get_panel_id(struct i2c_adapter *adapter); >>> struct edid *drm_get_edid_switcheroo(struct drm_connector *connector, >>> struct i2c_adapter *adapter); >> >> -- >> Jani Nikula, Intel >
| ![\](/images/icornerr.gif) |