Re: [PATCH v6 3/5] drm: Add support to get EDID from ACPI
Hi Mario, On Thu, Feb 15, 2024 at 8:04 PM Mario Limonciello wrote: > On 2/15/2024 12:47, Ville Syrjälä wrote: > > On Thu, Feb 15, 2024 at 12:20:56PM -0600, Mario Limonciello wrote: > >> On 2/14/2024 17:13, Ville Syrjälä wrote: > >>> On Wed, Feb 14, 2024 at 03:57:54PM -0600, Mario Limonciello wrote: > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -1886,6 +1886,12 @@ struct drm_connector { > > /** @hdr_sink_metadata: HDR Metadata Information read from > sink */ > struct hdr_sink_metadata hdr_sink_metadata; > + > + /** > + * @acpi_edid_allowed: Get the EDID from the BIOS, if available. > + * This is only applicable to eDP and LVDS displays. > + */ > + bool acpi_edid_allowed; > >>> > >>> Aren't there other bools/small stuff in there for tighter packing? > >> > >> Does the compiler automatically do the packing if you put bools nearby > >> in a struct? If so; TIL. > > > > Yes. Well, depends on the types and their alignment requirements > > of course, and/or whether you specified __packed or not. > > > > You can use 'pahole' to find the holes in structures. > > Thanks! I don't see a __packed attribute on struct drm_connector, but > I'll put it near by other bools in case that changes in the future. FTR, don't add __packed unless you have a very good reason to do so. With __packed, the compiler will emit multiple byte-accesses to access multi-byte integrals on platforms that do not support unaligned memory access. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH v6 3/5] drm: Add support to get EDID from ACPI
On 2/15/2024 12:47, Ville Syrjälä wrote: On Thu, Feb 15, 2024 at 12:20:56PM -0600, Mario Limonciello wrote: On 2/14/2024 17:13, Ville Syrjälä wrote: On Wed, Feb 14, 2024 at 03:57:54PM -0600, Mario Limonciello wrote: Some manufacturers have intentionally put an EDID that differs from the EDID on the internal panel on laptops. Drivers that prefer to fetch this EDID can set a bit on the drm_connector to indicate that the DRM EDID helpers should try to fetch it and it is preferred if it's present. Signed-off-by: Mario Limonciello --- drivers/gpu/drm/Kconfig | 1 + drivers/gpu/drm/drm_edid.c | 109 +--- include/drm/drm_connector.h | 6 ++ include/drm/drm_edid.h | 1 + 4 files changed, 109 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 872edb47bb53..3db89e6af01d 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -8,6 +8,7 @@ menuconfig DRM tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI support)" depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA + depends on (ACPI_VIDEO || ACPI_VIDEO=n) select DRM_PANEL_ORIENTATION_QUIRKS select DRM_KMS_HELPER if DRM_FBDEV_EMULATION select FB_CORE if DRM_FBDEV_EMULATION diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 923c4423151c..cdc30c6d05d5 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -28,6 +28,7 @@ * DEALINGS IN THE SOFTWARE. */ +#include #include #include #include @@ -2188,6 +2189,58 @@ 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_connector + * @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_connector *connector = data; + struct drm_device *ddev = connector->dev; + struct acpi_device *acpidev = ACPI_COMPANION(ddev->dev); + unsigned char start = block * EDID_LENGTH; + void *edid; + int r; + + if (!acpidev) + return -ENODEV; + + switch (connector->connector_type) { + case DRM_MODE_CONNECTOR_LVDS: + case DRM_MODE_CONNECTOR_eDP: + break; + default: + return -EINVAL; + } We could have other types of connectors that want this too. I don't see any real benefit in having this check tbh. Drivers should simply notset the flag on connectors where it won't work, and only the driver can really know that. Ack. + /* fetch the entire edid from BIOS */ + r = acpi_video_get_edid(acpidev, ACPI_VIDEO_DISPLAY_LCD, -1, ); + 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 void connector_bad_edid(struct drm_connector *connector, const struct edid *edid, int num_blocks) { @@ -2621,7 +2674,8 @@ EXPORT_SYMBOL(drm_probe_ddc); * @connector: connector we're probing * @adapter: I2C adapter to use for DDC * - * Poke the given I2C channel to grab EDID data if possible. If found, + * If the connector allows it, try to fetch EDID data using ACPI. If not found + * poke the given I2C channel to grab EDID data if possible. If found, * attach it to the connector. * * Return: Pointer to valid EDID or NULL if we couldn't find any. @@ -2629,20 +2683,50 @@ EXPORT_SYMBOL(drm_probe_ddc); struct edid *drm_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { - struct edid *edid; + struct edid *edid = NULL; if (connector->force == DRM_FORCE_OFF) return NULL; - if (connector->force == DRM_FORCE_UNSPECIFIED && !drm_probe_ddc(adapter)) - return NULL; + if (connector->acpi_edid_allowed) + edid = _drm_do_get_edid(connector, drm_do_probe_acpi_edid, connector, NULL); + + if (!edid) { + if (connector->force == DRM_FORCE_UNSPECIFIED && !drm_probe_ddc(adapter)) + return NULL; + edid = _drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter, NULL); + } - edid = _drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter, NULL);
Re: [PATCH v6 3/5] drm: Add support to get EDID from ACPI
On Thu, Feb 15, 2024 at 12:20:56PM -0600, Mario Limonciello wrote: > On 2/14/2024 17:13, Ville Syrjälä wrote: > > On Wed, Feb 14, 2024 at 03:57:54PM -0600, Mario Limonciello wrote: > >> Some manufacturers have intentionally put an EDID that differs from > >> the EDID on the internal panel on laptops. Drivers that prefer to > >> fetch this EDID can set a bit on the drm_connector to indicate that > >> the DRM EDID helpers should try to fetch it and it is preferred if > >> it's present. > >> > >> Signed-off-by: Mario Limonciello > >> --- > >> drivers/gpu/drm/Kconfig | 1 + > >> drivers/gpu/drm/drm_edid.c | 109 +--- > >> include/drm/drm_connector.h | 6 ++ > >> include/drm/drm_edid.h | 1 + > >> 4 files changed, 109 insertions(+), 8 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > >> index 872edb47bb53..3db89e6af01d 100644 > >> --- a/drivers/gpu/drm/Kconfig > >> +++ b/drivers/gpu/drm/Kconfig > >> @@ -8,6 +8,7 @@ > >> menuconfig DRM > >>tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI > >> support)" > >>depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA > >> + depends on (ACPI_VIDEO || ACPI_VIDEO=n) > >>select DRM_PANEL_ORIENTATION_QUIRKS > >>select DRM_KMS_HELPER if DRM_FBDEV_EMULATION > >>select FB_CORE if DRM_FBDEV_EMULATION > >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > >> index 923c4423151c..cdc30c6d05d5 100644 > >> --- a/drivers/gpu/drm/drm_edid.c > >> +++ b/drivers/gpu/drm/drm_edid.c > >> @@ -28,6 +28,7 @@ > >>* DEALINGS IN THE SOFTWARE. > >>*/ > >> > >> +#include > >> #include > >> #include > >> #include > >> @@ -2188,6 +2189,58 @@ 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_connector > >> + * @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_connector *connector = data; > >> + struct drm_device *ddev = connector->dev; > >> + struct acpi_device *acpidev = ACPI_COMPANION(ddev->dev); > >> + unsigned char start = block * EDID_LENGTH; > >> + void *edid; > >> + int r; > >> + > >> + if (!acpidev) > >> + return -ENODEV; > >> + > >> + switch (connector->connector_type) { > >> + case DRM_MODE_CONNECTOR_LVDS: > >> + case DRM_MODE_CONNECTOR_eDP: > >> + break; > >> + default: > >> + return -EINVAL; > >> + } > > > > We could have other types of connectors that want this too. > > I don't see any real benefit in having this check tbh. Drivers > > should simply notset the flag on connectors where it won't work, > > and only the driver can really know that. > > Ack. > > > > >> + /* fetch the entire edid from BIOS */ > >> + r = acpi_video_get_edid(acpidev, ACPI_VIDEO_DISPLAY_LCD, -1, ); > >> + 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 void connector_bad_edid(struct drm_connector *connector, > >> const struct edid *edid, int num_blocks) > >> { > >> @@ -2621,7 +2674,8 @@ EXPORT_SYMBOL(drm_probe_ddc); > >>* @connector: connector we're probing > >>* @adapter: I2C adapter to use for DDC > >>* > >> - * Poke the given I2C channel to grab EDID data if possible. If found, > >> + * If the connector allows it, try to fetch EDID data using ACPI. If not > >> found > >> + * poke the given I2C channel to grab EDID data if possible. If found, > >>* attach it to the connector. > >>* > >>* Return: Pointer to valid EDID or NULL if we couldn't find any. > >> @@ -2629,20 +2683,50 @@ EXPORT_SYMBOL(drm_probe_ddc); > >> struct edid *drm_get_edid(struct drm_connector *connector, > >> struct i2c_adapter *adapter) > >> { > >> - struct edid *edid; > >> + struct edid *edid = NULL; > >> > >>if (connector->force == DRM_FORCE_OFF) > >>return NULL; > >> > >> - if (connector->force == DRM_FORCE_UNSPECIFIED && > >> !drm_probe_ddc(adapter)) > >> - return NULL; > >> + if (connector->acpi_edid_allowed) > >> + edid = _drm_do_get_edid(connector,
Re: [PATCH v6 3/5] drm: Add support to get EDID from ACPI
On 2/14/2024 17:13, Ville Syrjälä wrote: On Wed, Feb 14, 2024 at 03:57:54PM -0600, Mario Limonciello wrote: Some manufacturers have intentionally put an EDID that differs from the EDID on the internal panel on laptops. Drivers that prefer to fetch this EDID can set a bit on the drm_connector to indicate that the DRM EDID helpers should try to fetch it and it is preferred if it's present. Signed-off-by: Mario Limonciello --- drivers/gpu/drm/Kconfig | 1 + drivers/gpu/drm/drm_edid.c | 109 +--- include/drm/drm_connector.h | 6 ++ include/drm/drm_edid.h | 1 + 4 files changed, 109 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 872edb47bb53..3db89e6af01d 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -8,6 +8,7 @@ menuconfig DRM tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI support)" depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA + depends on (ACPI_VIDEO || ACPI_VIDEO=n) select DRM_PANEL_ORIENTATION_QUIRKS select DRM_KMS_HELPER if DRM_FBDEV_EMULATION select FB_CORE if DRM_FBDEV_EMULATION diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 923c4423151c..cdc30c6d05d5 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -28,6 +28,7 @@ * DEALINGS IN THE SOFTWARE. */ +#include #include #include #include @@ -2188,6 +2189,58 @@ 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_connector + * @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_connector *connector = data; + struct drm_device *ddev = connector->dev; + struct acpi_device *acpidev = ACPI_COMPANION(ddev->dev); + unsigned char start = block * EDID_LENGTH; + void *edid; + int r; + + if (!acpidev) + return -ENODEV; + + switch (connector->connector_type) { + case DRM_MODE_CONNECTOR_LVDS: + case DRM_MODE_CONNECTOR_eDP: + break; + default: + return -EINVAL; + } We could have other types of connectors that want this too. I don't see any real benefit in having this check tbh. Drivers should simply notset the flag on connectors where it won't work, and only the driver can really know that. Ack. + /* fetch the entire edid from BIOS */ + r = acpi_video_get_edid(acpidev, ACPI_VIDEO_DISPLAY_LCD, -1, ); + 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 void connector_bad_edid(struct drm_connector *connector, const struct edid *edid, int num_blocks) { @@ -2621,7 +2674,8 @@ EXPORT_SYMBOL(drm_probe_ddc); * @connector: connector we're probing * @adapter: I2C adapter to use for DDC * - * Poke the given I2C channel to grab EDID data if possible. If found, + * If the connector allows it, try to fetch EDID data using ACPI. If not found + * poke the given I2C channel to grab EDID data if possible. If found, * attach it to the connector. * * Return: Pointer to valid EDID or NULL if we couldn't find any. @@ -2629,20 +2683,50 @@ EXPORT_SYMBOL(drm_probe_ddc); struct edid *drm_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) { - struct edid *edid; + struct edid *edid = NULL; if (connector->force == DRM_FORCE_OFF) return NULL; - if (connector->force == DRM_FORCE_UNSPECIFIED && !drm_probe_ddc(adapter)) - return NULL; + if (connector->acpi_edid_allowed) + edid = _drm_do_get_edid(connector, drm_do_probe_acpi_edid, connector, NULL); + + if (!edid) { + if (connector->force == DRM_FORCE_UNSPECIFIED && !drm_probe_ddc(adapter)) + return NULL; + edid = _drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter, NULL); + } - edid = _drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter, NULL); drm_connector_update_edid_property(connector, edid); return edid; } EXPORT_SYMBOL(drm_get_edid); +/** + *
Re: [PATCH v6 3/5] drm: Add support to get EDID from ACPI
On Thu, 15 Feb 2024, Ville Syrjälä wrote: > On Wed, Feb 14, 2024 at 03:57:54PM -0600, Mario Limonciello wrote: >> +static int >> +drm_do_probe_acpi_edid(void *data, u8 *buf, unsigned int block, size_t len) >> +{ >> +struct drm_connector *connector = data; >> +struct drm_device *ddev = connector->dev; >> +struct acpi_device *acpidev = ACPI_COMPANION(ddev->dev); >> +unsigned char start = block * EDID_LENGTH; >> +void *edid; >> +int r; >> + >> +if (!acpidev) >> +return -ENODEV; >> + >> +switch (connector->connector_type) { >> +case DRM_MODE_CONNECTOR_LVDS: >> +case DRM_MODE_CONNECTOR_eDP: >> +break; >> +default: >> +return -EINVAL; >> +} > > We could have other types of connectors that want this too. > I don't see any real benefit in having this check tbh. Drivers > should simply notset the flag on connectors where it won't work, > and only the driver can really know that. Agreed. >> const struct drm_edid *drm_edid_read(struct drm_connector *connector) >> { >> +const struct drm_edid *drm_edid = NULL; >> + >> if (drm_WARN_ON(connector->dev, !connector->ddc)) >> return NULL; >> >> -return drm_edid_read_ddc(connector, connector->ddc); >> +if (connector->acpi_edid_allowed) > > That should probably be called 'prefer_acpi_edid' or something > since it's the first choice when the flag is set. > > But I'm not so sure there's any real benefit in having this > flag at all. You anyway have to modify the driver to use this, > so why not just have the driver do the call directly instead of > adding this extra detour via the flag? Heh, round and round we go [1]. BR, Jani. [1] https://lore.kernel.org/r/87sf23auxv@intel.com -- Jani Nikula, Intel
Re: [PATCH v6 3/5] drm: Add support to get EDID from ACPI
On Wed, 14 Feb 2024, Mario Limonciello wrote: > Some manufacturers have intentionally put an EDID that differs from > the EDID on the internal panel on laptops. Drivers that prefer to > fetch this EDID can set a bit on the drm_connector to indicate that > the DRM EDID helpers should try to fetch it and it is preferred if > it's present. I just replied to a previous version of the patch [1]. Looks like all the comments there still hold. BR, Jani. [1] https://lore.kernel.org/r/87eddd6d41@intel.com > > Signed-off-by: Mario Limonciello > --- > drivers/gpu/drm/Kconfig | 1 + > drivers/gpu/drm/drm_edid.c | 109 +--- > include/drm/drm_connector.h | 6 ++ > include/drm/drm_edid.h | 1 + > 4 files changed, 109 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 872edb47bb53..3db89e6af01d 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -8,6 +8,7 @@ > menuconfig DRM > tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI > support)" > depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA > + depends on (ACPI_VIDEO || ACPI_VIDEO=n) > select DRM_PANEL_ORIENTATION_QUIRKS > select DRM_KMS_HELPER if DRM_FBDEV_EMULATION > select FB_CORE if DRM_FBDEV_EMULATION > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 923c4423151c..cdc30c6d05d5 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -28,6 +28,7 @@ > * DEALINGS IN THE SOFTWARE. > */ > > +#include > #include > #include > #include > @@ -2188,6 +2189,58 @@ 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_connector > + * @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_connector *connector = data; > + struct drm_device *ddev = connector->dev; > + struct acpi_device *acpidev = ACPI_COMPANION(ddev->dev); > + unsigned char start = block * EDID_LENGTH; > + void *edid; > + int r; > + > + if (!acpidev) > + return -ENODEV; > + > + switch (connector->connector_type) { > + case DRM_MODE_CONNECTOR_LVDS: > + case DRM_MODE_CONNECTOR_eDP: > + break; > + default: > + return -EINVAL; > + } > + > + /* fetch the entire edid from BIOS */ > + r = acpi_video_get_edid(acpidev, ACPI_VIDEO_DISPLAY_LCD, -1, ); > + 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 void connector_bad_edid(struct drm_connector *connector, > const struct edid *edid, int num_blocks) > { > @@ -2621,7 +2674,8 @@ EXPORT_SYMBOL(drm_probe_ddc); > * @connector: connector we're probing > * @adapter: I2C adapter to use for DDC > * > - * Poke the given I2C channel to grab EDID data if possible. If found, > + * If the connector allows it, try to fetch EDID data using ACPI. If not > found > + * poke the given I2C channel to grab EDID data if possible. If found, > * attach it to the connector. > * > * Return: Pointer to valid EDID or NULL if we couldn't find any. > @@ -2629,20 +2683,50 @@ EXPORT_SYMBOL(drm_probe_ddc); > struct edid *drm_get_edid(struct drm_connector *connector, > struct i2c_adapter *adapter) > { > - struct edid *edid; > + struct edid *edid = NULL; > > if (connector->force == DRM_FORCE_OFF) > return NULL; > > - if (connector->force == DRM_FORCE_UNSPECIFIED && > !drm_probe_ddc(adapter)) > - return NULL; > + if (connector->acpi_edid_allowed) > + edid = _drm_do_get_edid(connector, drm_do_probe_acpi_edid, > connector, NULL); > + > + if (!edid) { > + if (connector->force == DRM_FORCE_UNSPECIFIED && > !drm_probe_ddc(adapter)) > + return NULL; > + edid = _drm_do_get_edid(connector, drm_do_probe_ddc_edid, > adapter, NULL); > + } > > - edid = _drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter, > NULL); > drm_connector_update_edid_property(connector, edid); > return edid; > } > EXPORT_SYMBOL(drm_get_edid); >
Re: [PATCH v6 3/5] drm: Add support to get EDID from ACPI
On Wed, Feb 14, 2024 at 03:57:54PM -0600, Mario Limonciello wrote: > Some manufacturers have intentionally put an EDID that differs from > the EDID on the internal panel on laptops. Drivers that prefer to > fetch this EDID can set a bit on the drm_connector to indicate that > the DRM EDID helpers should try to fetch it and it is preferred if > it's present. > > Signed-off-by: Mario Limonciello > --- > drivers/gpu/drm/Kconfig | 1 + > drivers/gpu/drm/drm_edid.c | 109 +--- > include/drm/drm_connector.h | 6 ++ > include/drm/drm_edid.h | 1 + > 4 files changed, 109 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 872edb47bb53..3db89e6af01d 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -8,6 +8,7 @@ > menuconfig DRM > tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI > support)" > depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA > + depends on (ACPI_VIDEO || ACPI_VIDEO=n) > select DRM_PANEL_ORIENTATION_QUIRKS > select DRM_KMS_HELPER if DRM_FBDEV_EMULATION > select FB_CORE if DRM_FBDEV_EMULATION > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 923c4423151c..cdc30c6d05d5 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -28,6 +28,7 @@ > * DEALINGS IN THE SOFTWARE. > */ > > +#include > #include > #include > #include > @@ -2188,6 +2189,58 @@ 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_connector > + * @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_connector *connector = data; > + struct drm_device *ddev = connector->dev; > + struct acpi_device *acpidev = ACPI_COMPANION(ddev->dev); > + unsigned char start = block * EDID_LENGTH; > + void *edid; > + int r; > + > + if (!acpidev) > + return -ENODEV; > + > + switch (connector->connector_type) { > + case DRM_MODE_CONNECTOR_LVDS: > + case DRM_MODE_CONNECTOR_eDP: > + break; > + default: > + return -EINVAL; > + } We could have other types of connectors that want this too. I don't see any real benefit in having this check tbh. Drivers should simply notset the flag on connectors where it won't work, and only the driver can really know that. > + /* fetch the entire edid from BIOS */ > + r = acpi_video_get_edid(acpidev, ACPI_VIDEO_DISPLAY_LCD, -1, ); > + 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 void connector_bad_edid(struct drm_connector *connector, > const struct edid *edid, int num_blocks) > { > @@ -2621,7 +2674,8 @@ EXPORT_SYMBOL(drm_probe_ddc); > * @connector: connector we're probing > * @adapter: I2C adapter to use for DDC > * > - * Poke the given I2C channel to grab EDID data if possible. If found, > + * If the connector allows it, try to fetch EDID data using ACPI. If not > found > + * poke the given I2C channel to grab EDID data if possible. If found, > * attach it to the connector. > * > * Return: Pointer to valid EDID or NULL if we couldn't find any. > @@ -2629,20 +2683,50 @@ EXPORT_SYMBOL(drm_probe_ddc); > struct edid *drm_get_edid(struct drm_connector *connector, > struct i2c_adapter *adapter) > { > - struct edid *edid; > + struct edid *edid = NULL; > > if (connector->force == DRM_FORCE_OFF) > return NULL; > > - if (connector->force == DRM_FORCE_UNSPECIFIED && > !drm_probe_ddc(adapter)) > - return NULL; > + if (connector->acpi_edid_allowed) > + edid = _drm_do_get_edid(connector, drm_do_probe_acpi_edid, > connector, NULL); > + > + if (!edid) { > + if (connector->force == DRM_FORCE_UNSPECIFIED && > !drm_probe_ddc(adapter)) > + return NULL; > + edid = _drm_do_get_edid(connector, drm_do_probe_ddc_edid, > adapter, NULL); > + } > > - edid = _drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter, > NULL); >