Re: [PATCH v7 10/10] drm/amd/display: Fetch the EDID from _DDC if available for eDP

[Alex Hung <alex.hung@amd.com>]

A minor comment (see inline below).

Otherwise

Reviewed-by: Alex Hung <alex.hung@amd.com>

On 2024-09-18 15:38, Mario Limonciello wrote:
> Some manufacturers have intentionally put an EDID that differs from
> the EDID on the internal panel on laptops.
> 
> Attempt to fetch this EDID if it exists and prefer it over the EDID
> that is provided by the panel. If a user prefers to use the EDID from
> the panel, offer a DC debugging parameter that would disable this.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 62 ++++++++++++++++++-
>   drivers/gpu/drm/amd/include/amd_shared.h      |  5 ++
>   2 files changed, 66 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> index 8f4be7a1ec45..05d3e00ecce0 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> @@ -23,6 +23,8 @@
>    *
>    */
>   
> +#include <acpi/video.h>
> +
>   #include <linux/string.h>
>   #include <linux/acpi.h>
>   #include <linux/i2c.h>
> @@ -874,6 +876,60 @@ bool dm_helpers_is_dp_sink_present(struct dc_link *link)
>   	return dp_sink_present;
>   }
>   
> +static int
> +dm_helpers_probe_acpi_edid(void *data, u8 *buf, unsigned int block, size_t len)
> +{
> +	struct drm_connector *connector = data;
> +	struct acpi_device *acpidev = ACPI_COMPANION(connector->dev->dev);
> +	unsigned char start = block * EDID_LENGTH;
> +	void *edid;
> +	int r;
> +
> +	if (!acpidev)
> +		return -ENODEV;
> +
> +	/* fetch the entire edid from BIOS */
> +	r = acpi_video_get_edid(acpidev, ACPI_VIDEO_DISPLAY_LCD, -1, &edid);
> +	if (r < 0) {
> +		DRM_DEBUG_KMS("Failed to get EDID from ACPI: %d\n", r);
> +		return r;
> +	}
> +	if (len > r || start > r || start + len > r) {
> +		r = -EINVAL;
> +		goto cleanup;
> +	}
> +
> +	memcpy(buf, edid + start, len);
> +	r = 0;
> +
> +cleanup:
> +	kfree(edid);
> +
> +	return r;
> +}
> +
> +static const struct drm_edid *
> +dm_helpers_read_acpi_edid(struct amdgpu_dm_connector *aconnector)
> +{
> +	struct drm_connector *connector = &aconnector->base;
> +
> +	if (amdgpu_dc_debug_mask & DC_DISABLE_ACPI_EDID)
> +		return NULL;
> +
> +	switch (connector->connector_type) {
> +	case DRM_MODE_CONNECTOR_LVDS:
> +	case DRM_MODE_CONNECTOR_eDP:
> +		break;
> +	default:
> +		return NULL;
> +	}
> +
> +	if (connector->force == DRM_FORCE_OFF)
> +		return NULL;
> +
> +	return drm_edid_read_custom(connector, dm_helpers_probe_acpi_edid, connector);
> +}
> +
>   enum dc_edid_status dm_helpers_read_local_edid(
>   		struct dc_context *ctx,
>   		struct dc_link *link,
> @@ -896,7 +952,11 @@ enum dc_edid_status dm_helpers_read_local_edid(
>   	 * do check sum and retry to make sure read correct edid.
>   	 */
>   	do {
> -		drm_edid = drm_edid_read_ddc(connector, ddc);
> +		drm_edid = dm_helpers_read_acpi_edid(aconnector);
> +		if (drm_edid)
> +			DRM_DEBUG_KMS("Using ACPI provided EDID for %s\n", connector->name);

It is better to always output a message when ACPI's EDID is used without 
enabling any debug options. How about DRM_INFO?

> +		else
> +			drm_edid = drm_edid_read_ddc(connector, ddc);
>   		drm_edid_connector_update(connector, drm_edid);
>   		aconnector->drm_edid = drm_edid;
>   
> diff --git a/drivers/gpu/drm/amd/include/amd_shared.h b/drivers/gpu/drm/amd/include/amd_shared.h
> index 3f91926a50e9..1ec7c5e5249e 100644
> --- a/drivers/gpu/drm/amd/include/amd_shared.h
> +++ b/drivers/gpu/drm/amd/include/amd_shared.h
> @@ -337,6 +337,11 @@ enum DC_DEBUG_MASK {
>   	 * @DC_FORCE_IPS_ENABLE: If set, force enable all IPS, all the time.
>   	 */
>   	DC_FORCE_IPS_ENABLE = 0x4000,
> +	/**
> +	 * @DC_DISABLE_ACPI_EDID: If set, don't attempt to fetch EDID for
> +	 * eDP display from ACPI _DDC method.
> +	 */
> +	DC_DISABLE_ACPI_EDID = 0x8000,
>   };
>   
>   enum amd_dpm_forced_level;