[v5,1/3] 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 <mario.limonciello@amd.com>
---
v1->v2:
 * Split code from previous amdgpu specific helper to generic drm helper.
v2->v3:
 * Add an extra select to fix a variety of randconfig errors found from
   LKP robot.
v3->v4:
 * Return struct drm_edid
v4->v5:
 * Rename to drm_edid_read_acpi
 * Drop selects
---
 drivers/gpu/drm/Kconfig     |   7 +++
 drivers/gpu/drm/drm_edid.c  | 113 +++++++++++++++++++++++++++++++++---
 include/drm/drm_connector.h |   6 ++
 include/drm/drm_edid.h      |   1 +
 4 files changed, 119 insertions(+), 8 deletions(-)
  

On 2/10/2024 23:50, 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 <mario.limonciello@amd.com>
> ---
> v1->v2:
>   * Split code from previous amdgpu specific helper to generic drm helper.
> v2->v3:
>   * Add an extra select to fix a variety of randconfig errors found from
>     LKP robot.
> v3->v4:
>   * Return struct drm_edid
> v4->v5:
>   * Rename to drm_edid_read_acpi
>   * Drop selects
> ---
>   drivers/gpu/drm/Kconfig     |   7 +++
>   drivers/gpu/drm/drm_edid.c  | 113 +++++++++++++++++++++++++++++++++---
>   include/drm/drm_connector.h |   6 ++
>   include/drm/drm_edid.h      |   1 +
>   4 files changed, 119 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 2520db0b776e..a49740c528b9 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -103,6 +103,13 @@ config DRM_KMS_HELPER
>   	help
>   	  CRTC helpers for KMS drivers.
>   
> +config DRM_ACPI_HELPER
> +	tristate "ACPI support in DRM"
> +	depends on DRM
> +	depends on (ACPI_VIDEO || ACPI_VIDEO=n)
> +	help
> +	  ACPI helpers for DRM drivers.
> +

Unfortunately in my wider testing this still fails with a few combinations.

This combination fails to link:

CONFIG_ACPI_VIDEO=m
CONFIG_DRM_ACPI_HELPER=y
CONFIG_DRM=y

This combination links but doesn't work because the IS_REACHABLE() fails 
(-EOPNOTSUPP):

CONFIG_ACPI_VIDEO=m
CONFIG_DRM_ACPI_HELPER=M
CONFIG_DRM=y

I'm tempted to split off all of drm_edid to it's own module instead  of 
CONFIG_DRM_ACPI_HELPER which has a depends on (ACPI_VIDEO || ACPI_VIDEIO=n).

Or Daniel, any better ideas?

>   config DRM_DEBUG_DP_MST_TOPOLOGY_REFS
>           bool "Enable refcount backtrace history in the DP MST helpers"
>   	depends on STACKTRACE_SUPPORT
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 69c68804023f..096c278b6f66 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,62 @@ 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 */
> +	if (IS_REACHABLE(CONFIG_DRM_ACPI_HELPER)) {
> +		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;
> +		}
> +	} else {
> +		r = -EOPNOTSUPP;
> +	}
> +	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 +2678,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 +2687,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)
> +{
> +	const struct drm_edid *drm_edid;
> +
> +	if (connector->force == DRM_FORCE_OFF)
> +		return NULL;
> +
> +	drm_edid = drm_edid_read_custom(connector, drm_do_probe_acpi_edid, connector);
> +
> +	/* Note: Do *not* call connector updates here. */
> +
> +	return drm_edid;
> +}
> +EXPORT_SYMBOL(drm_edid_read_acpi);
> +
>   /**
>    * drm_edid_read_custom - Read EDID data using given EDID block read function
>    * @connector: Connector to use
> @@ -2727,10 +2815,11 @@ const struct drm_edid *drm_edid_read_ddc(struct drm_connector *connector,
>   EXPORT_SYMBOL(drm_edid_read_ddc);
>   
>   /**
> - * drm_edid_read - Read EDID data using connector's I2C adapter
> + * drm_edid_read - Read EDID data using BIOS or connector's I2C adapter
>    * @connector: Connector to use
>    *
> - * Read EDID using the connector's I2C adapter.
> + * Read EDID from BIOS if allowed by connector or by using the connector's
> + * I2C adapter.
>    *
>    * The EDID may be overridden using debugfs override_edid or firmware EDID
>    * (drm_edid_load_firmware() and drm.edid_firmware parameter), in this priority
> @@ -2742,10 +2831,18 @@ EXPORT_SYMBOL(drm_edid_read_ddc);
>    */
>   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)
> +		drm_edid = drm_edid_read_acpi(connector);
> +
> +	if (!drm_edid)
> +		drm_edid = drm_edid_read_ddc(connector, connector->ddc);
> +
> +	return drm_edid;
>   }
>   EXPORT_SYMBOL(drm_edid_read);
>   
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index fe88d7fc6b8f..74ed47f37a69 100644
> --- 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;
>   };
>   
>   #define obj_to_connector(x) container_of(x, struct drm_connector, base)
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 518d1b8106c7..38b5e1b5c773 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -463,5 +463,6 @@ bool drm_edid_is_digital(const struct drm_edid *drm_edid);
>   
>   const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
>   				  int ext_id, int *ext_index);
> +const struct drm_edid *drm_edid_read_acpi(struct drm_connector *connector);
>   
>   #endif /* __DRM_EDID_H__ */
  

On 2/12/2024 10:31, Mario Limonciello wrote:
> On 2/10/2024 23:50, 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 <mario.limonciello@amd.com>
>> ---
>> v1->v2:
>>   * Split code from previous amdgpu specific helper to generic drm 
>> helper.
>> v2->v3:
>>   * Add an extra select to fix a variety of randconfig errors found from
>>     LKP robot.
>> v3->v4:
>>   * Return struct drm_edid
>> v4->v5:
>>   * Rename to drm_edid_read_acpi
>>   * Drop selects
>> ---
>>   drivers/gpu/drm/Kconfig     |   7 +++
>>   drivers/gpu/drm/drm_edid.c  | 113 +++++++++++++++++++++++++++++++++---
>>   include/drm/drm_connector.h |   6 ++
>>   include/drm/drm_edid.h      |   1 +
>>   4 files changed, 119 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index 2520db0b776e..a49740c528b9 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -103,6 +103,13 @@ config DRM_KMS_HELPER
>>       help
>>         CRTC helpers for KMS drivers.
>> +config DRM_ACPI_HELPER
>> +    tristate "ACPI support in DRM"
>> +    depends on DRM
>> +    depends on (ACPI_VIDEO || ACPI_VIDEO=n)
>> +    help
>> +      ACPI helpers for DRM drivers.
>> +
> 
> Unfortunately in my wider testing this still fails with a few combinations.
> 
> This combination fails to link:
> 
> CONFIG_ACPI_VIDEO=m
> CONFIG_DRM_ACPI_HELPER=y
> CONFIG_DRM=y
> 
> This combination links but doesn't work because the IS_REACHABLE() fails 
> (-EOPNOTSUPP):
> 
> CONFIG_ACPI_VIDEO=m
> CONFIG_DRM_ACPI_HELPER=M
> CONFIG_DRM=y
> 
> I'm tempted to split off all of drm_edid to it's own module instead  of 
> CONFIG_DRM_ACPI_HELPER which has a depends on (ACPI_VIDEO || 
> ACPI_VIDEIO=n).
> 
> Or Daniel, any better ideas?

I've come up with a solution that undoes all the select mess in 
preceding patches.

This patch will be swapped around to

--- 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

I'll wait a little for any other feedback and then post the updated 
series.  None of the other patches change in any material way.

> 
>>   config DRM_DEBUG_DP_MST_TOPOLOGY_REFS
>>           bool "Enable refcount backtrace history in the DP MST helpers"
>>       depends on STACKTRACE_SUPPORT
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 69c68804023f..096c278b6f66 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,62 @@ 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 */
>> +    if (IS_REACHABLE(CONFIG_DRM_ACPI_HELPER)) {
>> +        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;
>> +        }
>> +    } else {
>> +        r = -EOPNOTSUPP;
>> +    }
>> +    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 +2678,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 +2687,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)
>> +{
>> +    const struct drm_edid *drm_edid;
>> +
>> +    if (connector->force == DRM_FORCE_OFF)
>> +        return NULL;
>> +
>> +    drm_edid = drm_edid_read_custom(connector, 
>> drm_do_probe_acpi_edid, connector);
>> +
>> +    /* Note: Do *not* call connector updates here. */
>> +
>> +    return drm_edid;
>> +}
>> +EXPORT_SYMBOL(drm_edid_read_acpi);
>> +
>>   /**
>>    * drm_edid_read_custom - Read EDID data using given EDID block read 
>> function
>>    * @connector: Connector to use
>> @@ -2727,10 +2815,11 @@ const struct drm_edid 
>> *drm_edid_read_ddc(struct drm_connector *connector,
>>   EXPORT_SYMBOL(drm_edid_read_ddc);
>>   /**
>> - * drm_edid_read - Read EDID data using connector's I2C adapter
>> + * drm_edid_read - Read EDID data using BIOS or connector's I2C adapter
>>    * @connector: Connector to use
>>    *
>> - * Read EDID using the connector's I2C adapter.
>> + * Read EDID from BIOS if allowed by connector or by using the 
>> connector's
>> + * I2C adapter.
>>    *
>>    * The EDID may be overridden using debugfs override_edid or 
>> firmware EDID
>>    * (drm_edid_load_firmware() and drm.edid_firmware parameter), in 
>> this priority
>> @@ -2742,10 +2831,18 @@ EXPORT_SYMBOL(drm_edid_read_ddc);
>>    */
>>   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)
>> +        drm_edid = drm_edid_read_acpi(connector);
>> +
>> +    if (!drm_edid)
>> +        drm_edid = drm_edid_read_ddc(connector, connector->ddc);
>> +
>> +    return drm_edid;
>>   }
>>   EXPORT_SYMBOL(drm_edid_read);
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index fe88d7fc6b8f..74ed47f37a69 100644
>> --- 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;
>>   };
>>   #define obj_to_connector(x) container_of(x, struct drm_connector, base)
>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> index 518d1b8106c7..38b5e1b5c773 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -463,5 +463,6 @@ bool drm_edid_is_digital(const struct drm_edid 
>> *drm_edid);
>>   const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
>>                     int ext_id, int *ext_index);
>> +const struct drm_edid *drm_edid_read_acpi(struct drm_connector 
>> *connector);
>>   #endif /* __DRM_EDID_H__ */
>
  

On Sat, 10 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 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 <mario.limonciello@amd.com>
> ---
> v1->v2:
>  * Split code from previous amdgpu specific helper to generic drm helper.
> v2->v3:
>  * Add an extra select to fix a variety of randconfig errors found from
>    LKP robot.
> v3->v4:
>  * Return struct drm_edid
> v4->v5:
>  * Rename to drm_edid_read_acpi
>  * Drop selects
> ---
>  drivers/gpu/drm/Kconfig     |   7 +++
>  drivers/gpu/drm/drm_edid.c  | 113 +++++++++++++++++++++++++++++++++---
>  include/drm/drm_connector.h |   6 ++
>  include/drm/drm_edid.h      |   1 +
>  4 files changed, 119 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 2520db0b776e..a49740c528b9 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -103,6 +103,13 @@ config DRM_KMS_HELPER
>  	help
>  	  CRTC helpers for KMS drivers.
>  
> +config DRM_ACPI_HELPER
> +	tristate "ACPI support in DRM"
> +	depends on DRM
> +	depends on (ACPI_VIDEO || ACPI_VIDEO=n)
> +	help
> +	  ACPI helpers for DRM drivers.
> +
>  config DRM_DEBUG_DP_MST_TOPOLOGY_REFS
>          bool "Enable refcount backtrace history in the DP MST helpers"
>  	depends on STACKTRACE_SUPPORT
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 69c68804023f..096c278b6f66 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,62 @@ 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 */
> +	if (IS_REACHABLE(CONFIG_DRM_ACPI_HELPER)) {
> +		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;
> +		}
> +	} else {
> +		r = -EOPNOTSUPP;
> +	}
> +	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 +2678,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 +2687,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);
> +	}

Hmm. So do you want the presence of ACPI EDID to determine whether the
display is there or not?

Note how the override and firmware EDID mechanisms only override the
EDID *contents*. They are orthogonal from connector forcing. So you can
override the EDID, but still rely on the DDC probe to determine if the
display is there.

The question is, how likely is it that the ACPI EDID is used not just
because the actual EDID is bogus, but also because the display just
doesn't respond to DDC at all?

If possible, I'd like ACPI EDID to follow the override/firmware EDID
semantics on this.

>  
> -	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)
> +{
> +	const struct drm_edid *drm_edid;
> +
> +	if (connector->force == DRM_FORCE_OFF)
> +		return NULL;

If the caller handled all the connector->force stuff, this could be
dropped.

> +
> +	drm_edid = drm_edid_read_custom(connector, drm_do_probe_acpi_edid, connector);
> +
> +	/* Note: Do *not* call connector updates here. */
> +
> +	return drm_edid;
> +}
> +EXPORT_SYMBOL(drm_edid_read_acpi);
> +
>  /**
>   * drm_edid_read_custom - Read EDID data using given EDID block read function
>   * @connector: Connector to use
> @@ -2727,10 +2815,11 @@ const struct drm_edid *drm_edid_read_ddc(struct drm_connector *connector,
>  EXPORT_SYMBOL(drm_edid_read_ddc);
>  
>  /**
> - * drm_edid_read - Read EDID data using connector's I2C adapter
> + * drm_edid_read - Read EDID data using BIOS or connector's I2C adapter
>   * @connector: Connector to use
>   *
> - * Read EDID using the connector's I2C adapter.
> + * Read EDID from BIOS if allowed by connector or by using the connector's
> + * I2C adapter.
>   *
>   * The EDID may be overridden using debugfs override_edid or firmware EDID
>   * (drm_edid_load_firmware() and drm.edid_firmware parameter), in this priority
> @@ -2742,10 +2831,18 @@ EXPORT_SYMBOL(drm_edid_read_ddc);
>   */
>  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)
> +		drm_edid = drm_edid_read_acpi(connector);
> +
> +	if (!drm_edid)
> +		drm_edid = drm_edid_read_ddc(connector, connector->ddc);

This should be in drm_edid_read_ddc() not drm_edid_read(). Please let's
not make the two behave any different. It would be really weird if one
had an ACPI check and the other not.

> +
> +	return drm_edid;
>  }
>  EXPORT_SYMBOL(drm_edid_read);
>  
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index fe88d7fc6b8f..74ed47f37a69 100644
> --- 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;
>  };
>  
>  #define obj_to_connector(x) container_of(x, struct drm_connector, base)
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 518d1b8106c7..38b5e1b5c773 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -463,5 +463,6 @@ bool drm_edid_is_digital(const struct drm_edid *drm_edid);
>  
>  const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
>  				  int ext_id, int *ext_index);
> +const struct drm_edid *drm_edid_read_acpi(struct drm_connector *connector);
>  
>  #endif /* __DRM_EDID_H__ */
  

On 2/15/2024 08:01, Jani Nikula wrote:
> On Sat, 10 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 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 <mario.limonciello@amd.com>
>> ---
>> v1->v2:
>>   * Split code from previous amdgpu specific helper to generic drm helper.
>> v2->v3:
>>   * Add an extra select to fix a variety of randconfig errors found from
>>     LKP robot.
>> v3->v4:
>>   * Return struct drm_edid
>> v4->v5:
>>   * Rename to drm_edid_read_acpi
>>   * Drop selects
>> ---
>>   drivers/gpu/drm/Kconfig     |   7 +++
>>   drivers/gpu/drm/drm_edid.c  | 113 +++++++++++++++++++++++++++++++++---
>>   include/drm/drm_connector.h |   6 ++
>>   include/drm/drm_edid.h      |   1 +
>>   4 files changed, 119 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index 2520db0b776e..a49740c528b9 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -103,6 +103,13 @@ config DRM_KMS_HELPER
>>   	help
>>   	  CRTC helpers for KMS drivers.
>>   
>> +config DRM_ACPI_HELPER
>> +	tristate "ACPI support in DRM"
>> +	depends on DRM
>> +	depends on (ACPI_VIDEO || ACPI_VIDEO=n)
>> +	help
>> +	  ACPI helpers for DRM drivers.
>> +
>>   config DRM_DEBUG_DP_MST_TOPOLOGY_REFS
>>           bool "Enable refcount backtrace history in the DP MST helpers"
>>   	depends on STACKTRACE_SUPPORT
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 69c68804023f..096c278b6f66 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,62 @@ 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 */
>> +	if (IS_REACHABLE(CONFIG_DRM_ACPI_HELPER)) {
>> +		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;
>> +		}
>> +	} else {
>> +		r = -EOPNOTSUPP;
>> +	}
>> +	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 +2678,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 +2687,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);
>> +	}
> 
> Hmm. So do you want the presence of ACPI EDID to determine whether the
> display is there or not?
> 
> Note how the override and firmware EDID mechanisms only override the
> EDID *contents*. They are orthogonal from connector forcing. So you can
> override the EDID, but still rely on the DDC probe to determine if the
> display is there.
> 
> The question is, how likely is it that the ACPI EDID is used not just
> because the actual EDID is bogus, but also because the display just
> doesn't respond to DDC at all?
> 

The cases that prompted this the panel "responds" to DDC fine, but the 
EDID data doesn't match what is expected to be used which leads to other 
unexpected functional problems.

> If possible, I'd like ACPI EDID to follow the override/firmware EDID
> semantics on this.
> 

So basically probe with DDC but if the driver specifies on the connector 
to use ACPI if present then try that (similar to how override is only 
used if specified by the user)?

>>   
>> -	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)
>> +{
>> +	const struct drm_edid *drm_edid;
>> +
>> +	if (connector->force == DRM_FORCE_OFF)
>> +		return NULL;
> 
> If the caller handled all the connector->force stuff, this could be
> dropped.
> 
>> +
>> +	drm_edid = drm_edid_read_custom(connector, drm_do_probe_acpi_edid, connector);
>> +
>> +	/* Note: Do *not* call connector updates here. */
>> +
>> +	return drm_edid;
>> +}
>> +EXPORT_SYMBOL(drm_edid_read_acpi);
>> +
>>   /**
>>    * drm_edid_read_custom - Read EDID data using given EDID block read function
>>    * @connector: Connector to use
>> @@ -2727,10 +2815,11 @@ const struct drm_edid *drm_edid_read_ddc(struct drm_connector *connector,
>>   EXPORT_SYMBOL(drm_edid_read_ddc);
>>   
>>   /**
>> - * drm_edid_read - Read EDID data using connector's I2C adapter
>> + * drm_edid_read - Read EDID data using BIOS or connector's I2C adapter
>>    * @connector: Connector to use
>>    *
>> - * Read EDID using the connector's I2C adapter.
>> + * Read EDID from BIOS if allowed by connector or by using the connector's
>> + * I2C adapter.
>>    *
>>    * The EDID may be overridden using debugfs override_edid or firmware EDID
>>    * (drm_edid_load_firmware() and drm.edid_firmware parameter), in this priority
>> @@ -2742,10 +2831,18 @@ EXPORT_SYMBOL(drm_edid_read_ddc);
>>    */
>>   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)
>> +		drm_edid = drm_edid_read_acpi(connector);
>> +
>> +	if (!drm_edid)
>> +		drm_edid = drm_edid_read_ddc(connector, connector->ddc);
> 
> This should be in drm_edid_read_ddc() not drm_edid_read(). Please let's
> not make the two behave any different. It would be really weird if one
> had an ACPI check and the other not.

OK, I'll move things around.

> 
>> +
>> +	return drm_edid;
>>   }
>>   EXPORT_SYMBOL(drm_edid_read);
>>   
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index fe88d7fc6b8f..74ed47f37a69 100644
>> --- 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;
>>   };
>>   
>>   #define obj_to_connector(x) container_of(x, struct drm_connector, base)
>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> index 518d1b8106c7..38b5e1b5c773 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -463,5 +463,6 @@ bool drm_edid_is_digital(const struct drm_edid *drm_edid);
>>   
>>   const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
>>   				  int ext_id, int *ext_index);
>> +const struct drm_edid *drm_edid_read_acpi(struct drm_connector *connector);
>>   
>>   #endif /* __DRM_EDID_H__ */
>