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);
>
[PATCH v6 3/5] drm: Add support to get EDID from ACPI
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;
+ }
+
+ /* 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);
+/**
+ * drm_edid_read_acpi - get EDID data, if available
+ * @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_edid_read_acpi(struct drm_connector *connector)
+{
+
