[PATCH 0/2] Fetch EDID from ACPI _DDC method if available

[PATCH 0/2] Fetch EDID from ACPI _DDC method if available

[Mario Limonciello]

Some laptops ship an EDID in the BIOS encoded in the _DDC method that
differs than the EDID directly on the laptop panel for $REASONS.

This is the EDID that is used by the AMD Windows driver, and so sometimes
different results are found in different operating systems.

This series changes it so that when an eDP panel is found the BIOS
is checked first for an EDID and that used as a preference if found.

Mario Limonciello (2):
  ACPI: video: Handle fetching EDID that is longer than 256 bytes
  drm/amd: Fetch the EDID from _DDC if available for eDP

 drivers/acpi/acpi_video.c                     | 23 +++++++++-----
 drivers/gpu/drm/amd/amdgpu/amdgpu.h           |  2 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c      | 30 +++++++++++++++++++
 .../gpu/drm/amd/amdgpu/amdgpu_connectors.c    |  5 ++++
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  8 ++++-
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  7 +++--
 6 files changed, 65 insertions(+), 10 deletions(-)

-- 
2.34.1

[PATCH 1/2] ACPI: video: Handle fetching EDID that is longer than 256 bytes

[Mario Limonciello]

The ACPI specification allows for an EDID to be up to 512 bytes but
the _DDC EDID fetching code will only try up to 256 bytes.

Modify the code to instead start at 512 bytes and work it's way
down instead.

Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/Apx_B_Video_Extensions/output-device-specific-methods.html#ddc-return-the-edid-for-this-device
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/acpi_video.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 62f4364e4460..b3b15dd4755d 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -624,6 +624,10 @@ acpi_video_device_EDID(struct acpi_video_device *device,
 		arg0.integer.value = 1;
 	else if (length == 256)
 		arg0.integer.value = 2;
+	else if (length == 384)
+		arg0.integer.value = 3;
+	else if (length == 512)
+		arg0.integer.value = 4;
 	else
 		return -EINVAL;
 
@@ -1443,7 +1447,7 @@ int acpi_video_get_edid(struct acpi_device *device, int type, int device_id,
 
 	for (i = 0; i < video->attached_count; i++) {
 		video_device = video->attached_array[i].bind_info;
-		length = 256;
+		length = 512;
 
 		if (!video_device)
 			continue;
@@ -1478,13 +1482,18 @@ int acpi_video_get_edid(struct acpi_device *device, int type, int device_id,
 
 		if (ACPI_FAILURE(status) || !buffer ||
 		    buffer->type != ACPI_TYPE_BUFFER) {
-			length = 128;
-			status = acpi_video_device_EDID(video_device, &buffer,
-							length);
-			if (ACPI_FAILURE(status) || !buffer ||
-			    buffer->type != ACPI_TYPE_BUFFER) {
-				continue;
+			while (length) {
+				length -= 128;
+				status = acpi_video_device_EDID(video_device, &buffer,
+								length);
+				if (ACPI_FAILURE(status) || !buffer ||
+				    buffer->type != ACPI_TYPE_BUFFER) {
+					continue;
+				}
+				break;
 			}
+			if (!length)
+				continue;
 		}
 
 		*edid = buffer->buffer.pointer;
-- 
2.34.1

Re: [PATCH 1/2] ACPI: video: Handle fetching EDID that is longer than 256 bytes

[Rafael J. Wysocki]

On Fri, Jan 26, 2024 at 7:55 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> The ACPI specification allows for an EDID to be up to 512 bytes but
> the _DDC EDID fetching code will only try up to 256 bytes.
>
> Modify the code to instead start at 512 bytes and work it's way
> down instead.
>
> Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/Apx_B_Video_Extensions/output-device-specific-methods.html#ddc-return-the-edid-for-this-device
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/acpi/acpi_video.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
> index 62f4364e4460..b3b15dd4755d 100644
> --- a/drivers/acpi/acpi_video.c
> +++ b/drivers/acpi/acpi_video.c
> @@ -624,6 +624,10 @@ acpi_video_device_EDID(struct acpi_video_device *device,
>                 arg0.integer.value = 1;
>         else if (length == 256)
>                 arg0.integer.value = 2;
> +       else if (length == 384)
> +               arg0.integer.value = 3;
> +       else if (length == 512)
> +               arg0.integer.value = 4;

It looks like switch () would be somewhat better.

Or maybe even

arg0.integer.value = length / 128;

The validation could be added too:

if (arg0.integer.value > 4 || arg0.integer.value * 128 != length)
        return -EINVAL;

but it is pointless, because the caller is never passing an invalid
number to it AFAICS.

>         else
>                 return -EINVAL;
>
> @@ -1443,7 +1447,7 @@ int acpi_video_get_edid(struct acpi_device *device, int type, int device_id,
>
>         for (i = 0; i < video->attached_count; i++) {
>                 video_device = video->attached_array[i].bind_info;
> -               length = 256;
> +               length = 512;
>
>                 if (!video_device)
>                         continue;
> @@ -1478,13 +1482,18 @@ int acpi_video_get_edid(struct acpi_device *device, int type, int device_id,
>
>                 if (ACPI_FAILURE(status) || !buffer ||
>                     buffer->type != ACPI_TYPE_BUFFER) {
> -                       length = 128;
> -                       status = acpi_video_device_EDID(video_device, &buffer,
> -                                                       length);
> -                       if (ACPI_FAILURE(status) || !buffer ||
> -                           buffer->type != ACPI_TYPE_BUFFER) {
> -                               continue;
> +                       while (length) {

I would prefer a do {} while () loop here, which could include the
first invocation of acpi_video_device_EDID() too (and reduce code
duplication a bit).

> +                               length -= 128;
> +                               status = acpi_video_device_EDID(video_device, &buffer,
> +                                                               length);

No line break, please.

> +                               if (ACPI_FAILURE(status) || !buffer ||
> +                                   buffer->type != ACPI_TYPE_BUFFER) {
> +                                       continue;
> +                               }
> +                               break;
>                         }
> +                       if (!length)
> +                               continue;
>                 }
>
>                 *edid = buffer->buffer.pointer;
> --

Re: [PATCH 1/2] ACPI: video: Handle fetching EDID that is longer than 256 bytes

[Mario Limonciello]

On 1/29/2024 07:54, Rafael J. Wysocki wrote:
> On Fri, Jan 26, 2024 at 7:55 PM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>>
>> The ACPI specification allows for an EDID to be up to 512 bytes but
>> the _DDC EDID fetching code will only try up to 256 bytes.
>>
>> Modify the code to instead start at 512 bytes and work it's way
>> down instead.
>>
>> Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/Apx_B_Video_Extensions/output-device-specific-methods.html#ddc-return-the-edid-for-this-device
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/acpi/acpi_video.c | 23 ++++++++++++++++-------
>>   1 file changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
>> index 62f4364e4460..b3b15dd4755d 100644
>> --- a/drivers/acpi/acpi_video.c
>> +++ b/drivers/acpi/acpi_video.c
>> @@ -624,6 +624,10 @@ acpi_video_device_EDID(struct acpi_video_device *device,
>>                  arg0.integer.value = 1;
>>          else if (length == 256)
>>                  arg0.integer.value = 2;
>> +       else if (length == 384)
>> +               arg0.integer.value = 3;
>> +       else if (length == 512)
>> +               arg0.integer.value = 4;
> 
> It looks like switch () would be somewhat better.
> 
> Or maybe even
> 
> arg0.integer.value = length / 128;
> 
> The validation could be added too:
> 
> if (arg0.integer.value > 4 || arg0.integer.value * 128 != length)
>          return -EINVAL;
> 
> but it is pointless, because the caller is never passing an invalid
> number to it AFAICS.
> 

Thanks.  I'll swap over to one of these suggestions.

I will also split this patch separately from the other as the other will 
take some time with refactoring necessary in DRM that will take a cycle 
or two.

>>          else
>>                  return -EINVAL;
>>
>> @@ -1443,7 +1447,7 @@ int acpi_video_get_edid(struct acpi_device *device, int type, int device_id,
>>
>>          for (i = 0; i < video->attached_count; i++) {
>>                  video_device = video->attached_array[i].bind_info;
>> -               length = 256;
>> +               length = 512;
>>
>>                  if (!video_device)
>>                          continue;
>> @@ -1478,13 +1482,18 @@ int acpi_video_get_edid(struct acpi_device *device, int type, int device_id,
>>
>>                  if (ACPI_FAILURE(status) || !buffer ||
>>                      buffer->type != ACPI_TYPE_BUFFER) {
>> -                       length = 128;
>> -                       status = acpi_video_device_EDID(video_device, &buffer,
>> -                                                       length);
>> -                       if (ACPI_FAILURE(status) || !buffer ||
>> -                           buffer->type != ACPI_TYPE_BUFFER) {
>> -                               continue;
>> +                       while (length) {
> 
> I would prefer a do {} while () loop here, which could include the
> first invocation of acpi_video_device_EDID() too (and reduce code
> duplication a bit).
> 
>> +                               length -= 128;
>> +                               status = acpi_video_device_EDID(video_device, &buffer,
>> +                                                               length);
> 
> No line break, please.
> 
>> +                               if (ACPI_FAILURE(status) || !buffer ||
>> +                                   buffer->type != ACPI_TYPE_BUFFER) {
>> +                                       continue;
>> +                               }
>> +                               break;
>>                          }
>> +                       if (!length)
>> +                               continue;
>>                  }
>>
>>                  *edid = buffer->buffer.pointer;
>> --

[PATCH 2/2] drm/amd: Fetch the EDID from _DDC if available for eDP

[Mario Limonciello]

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.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h           |  2 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c      | 30 +++++++++++++++++++
 .../gpu/drm/amd/amdgpu/amdgpu_connectors.c    |  5 ++++
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  8 ++++-
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  7 +++--
 5 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index c5f3859fd682..99abe12567a4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1520,6 +1520,7 @@ int amdgpu_acpi_get_mem_info(struct amdgpu_device *adev, int xcc_id,
 
 void amdgpu_acpi_get_backlight_caps(struct amdgpu_dm_backlight_caps *caps);
 bool amdgpu_acpi_should_gpu_reset(struct amdgpu_device *adev);
+void *amdgpu_acpi_edid(struct amdgpu_device *adev, struct drm_connector *connector);
 void amdgpu_acpi_detect(void);
 void amdgpu_acpi_release(void);
 #else
@@ -1537,6 +1538,7 @@ static inline int amdgpu_acpi_get_mem_info(struct amdgpu_device *adev,
 }
 static inline void amdgpu_acpi_fini(struct amdgpu_device *adev) { }
 static inline bool amdgpu_acpi_should_gpu_reset(struct amdgpu_device *adev) { return false; }
+static inline void *amdgpu_acpi_edid(struct amdgpu_device *adev, struct drm_connector *connector) { return NULL; }
 static inline void amdgpu_acpi_detect(void) { }
 static inline void amdgpu_acpi_release(void) { }
 static inline bool amdgpu_acpi_is_power_shift_control_supported(void) { return false; }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
index e550067e5c5d..c106335f1f22 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
@@ -1380,6 +1380,36 @@ bool amdgpu_acpi_should_gpu_reset(struct amdgpu_device *adev)
 #endif
 }
 
+/**
+ * amdgpu_acpi_edid
+ * @adev: amdgpu_device pointer
+ * @connector: drm_connector pointer
+ *
+ * Returns the EDID used for the internal panel if present, NULL otherwise.
+ */
+void *
+amdgpu_acpi_edid(struct amdgpu_device *adev, struct drm_connector *connector)
+{
+	struct drm_device *ddev = adev_to_drm(adev);
+	struct acpi_device *acpidev = ACPI_COMPANION(ddev->dev);
+	void *edid;
+	int r;
+
+	if (!acpidev)
+		return NULL;
+
+	if (connector->connector_type != DRM_MODE_CONNECTOR_eDP)
+		return NULL;
+
+	r = acpi_video_get_edid(acpidev, ACPI_VIDEO_DISPLAY_LCD, -1, &edid);
+	if (r < 0) {
+		DRM_DEBUG_DRIVER("Failed to get EDID from ACPI: %d\n", r);
+		return NULL;
+	}
+
+	return kmemdup(edid, r, GFP_KERNEL);
+}
+
 /*
  * amdgpu_acpi_detect - detect ACPI ATIF/ATCS methods
  *
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
index 9caba10315a8..c7e1563a46d3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
@@ -278,6 +278,11 @@ static void amdgpu_connector_get_edid(struct drm_connector *connector)
 	struct amdgpu_device *adev = drm_to_adev(dev);
 	struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
 
+	if (amdgpu_connector->edid)
+		return;
+
+	/* if the BIOS specifies the EDID via _DDC, prefer this */
+	amdgpu_connector->edid = amdgpu_acpi_edid(adev, connector);
 	if (amdgpu_connector->edid)
 		return;
 
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index cd98b3565178..1faa21f542bd 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6562,17 +6562,23 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector)
 {
 	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
 	struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
+	struct amdgpu_device *adev = drm_to_adev(connector->dev);
 	struct dc_link *dc_link = aconnector->dc_link;
 	struct dc_sink *dc_em_sink = aconnector->dc_em_sink;
 	struct edid *edid;
 
+	/* prefer ACPI over panel for eDP */
+	edid = amdgpu_acpi_edid(adev, connector);
+
 	/*
 	 * Note: drm_get_edid gets edid in the following order:
 	 * 1) override EDID if set via edid_override debugfs,
 	 * 2) firmware EDID if set via edid_firmware module parameter
 	 * 3) regular DDC read.
 	 */
-	edid = drm_get_edid(connector, &amdgpu_connector->ddc_bus->aux.ddc);
+	if (!edid)
+		edid = drm_get_edid(connector, &amdgpu_connector->ddc_bus->aux.ddc);
+
 	if (!edid) {
 		DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
 		return;
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 e3915c4f8566..6bf2a8867e76 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
@@ -895,6 +895,7 @@ enum dc_edid_status dm_helpers_read_local_edid(
 {
 	struct amdgpu_dm_connector *aconnector = link->priv;
 	struct drm_connector *connector = &aconnector->base;
+	struct amdgpu_device *adev = drm_to_adev(connector->dev);
 	struct i2c_adapter *ddc;
 	int retry = 3;
 	enum dc_edid_status edid_status;
@@ -909,8 +910,10 @@ enum dc_edid_status dm_helpers_read_local_edid(
 	 * do check sum and retry to make sure read correct edid.
 	 */
 	do {
-
-		edid = drm_get_edid(&aconnector->base, ddc);
+		/* prefer ACPI over panel for eDP */
+		edid = amdgpu_acpi_edid(adev, connector);
+		if (!edid)
+			edid = drm_get_edid(&aconnector->base, ddc);
 
 		/* DP Compliance Test 4.2.2.6 */
 		if (link->aux_mode && connector->edid_corrupt)
-- 
2.34.1

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

[Jani Nikula]

On Fri, 26 Jan 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.
>
> Attempt to fetch this EDID if it exists and prefer it over the EDID
> that is provided by the panel.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h           |  2 ++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c      | 30 +++++++++++++++++++
>  .../gpu/drm/amd/amdgpu/amdgpu_connectors.c    |  5 ++++
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  8 ++++-
>  .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  7 +++--
>  5 files changed, 49 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index c5f3859fd682..99abe12567a4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1520,6 +1520,7 @@ int amdgpu_acpi_get_mem_info(struct amdgpu_device *adev, int xcc_id,
>  
>  void amdgpu_acpi_get_backlight_caps(struct amdgpu_dm_backlight_caps *caps);
>  bool amdgpu_acpi_should_gpu_reset(struct amdgpu_device *adev);
> +void *amdgpu_acpi_edid(struct amdgpu_device *adev, struct drm_connector *connector);
>  void amdgpu_acpi_detect(void);
>  void amdgpu_acpi_release(void);
>  #else
> @@ -1537,6 +1538,7 @@ static inline int amdgpu_acpi_get_mem_info(struct amdgpu_device *adev,
>  }
>  static inline void amdgpu_acpi_fini(struct amdgpu_device *adev) { }
>  static inline bool amdgpu_acpi_should_gpu_reset(struct amdgpu_device *adev) { return false; }
> +static inline void *amdgpu_acpi_edid(struct amdgpu_device *adev, struct drm_connector *connector) { return NULL; }
>  static inline void amdgpu_acpi_detect(void) { }
>  static inline void amdgpu_acpi_release(void) { }
>  static inline bool amdgpu_acpi_is_power_shift_control_supported(void) { return false; }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> index e550067e5c5d..c106335f1f22 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> @@ -1380,6 +1380,36 @@ bool amdgpu_acpi_should_gpu_reset(struct amdgpu_device *adev)
>  #endif
>  }
>  
> +/**
> + * amdgpu_acpi_edid
> + * @adev: amdgpu_device pointer
> + * @connector: drm_connector pointer
> + *
> + * Returns the EDID used for the internal panel if present, NULL otherwise.
> + */
> +void *
> +amdgpu_acpi_edid(struct amdgpu_device *adev, struct drm_connector *connector)
> +{
> +	struct drm_device *ddev = adev_to_drm(adev);
> +	struct acpi_device *acpidev = ACPI_COMPANION(ddev->dev);
> +	void *edid;
> +	int r;
> +
> +	if (!acpidev)
> +		return NULL;
> +
> +	if (connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> +		return NULL;
> +
> +	r = acpi_video_get_edid(acpidev, ACPI_VIDEO_DISPLAY_LCD, -1, &edid);
> +	if (r < 0) {
> +		DRM_DEBUG_DRIVER("Failed to get EDID from ACPI: %d\n", r);
> +		return NULL;
> +	}
> +
> +	return kmemdup(edid, r, GFP_KERNEL);
> +}
> +
>  /*
>   * amdgpu_acpi_detect - detect ACPI ATIF/ATCS methods
>   *
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> index 9caba10315a8..c7e1563a46d3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> @@ -278,6 +278,11 @@ static void amdgpu_connector_get_edid(struct drm_connector *connector)
>  	struct amdgpu_device *adev = drm_to_adev(dev);
>  	struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
>  
> +	if (amdgpu_connector->edid)
> +		return;
> +
> +	/* if the BIOS specifies the EDID via _DDC, prefer this */
> +	amdgpu_connector->edid = amdgpu_acpi_edid(adev, connector);

Imagine the EDID returned by acpi_video_get_edid() has edid->extensions
bigger than 4. Of course it should not, but you have no guarantees, and
it originates outside of the kernel.

The real fix is to have the function return a struct drm_edid which
tracks the allocation size separately. Unfortunately, it requires a
bunch of changes along the way. We've mostly done it in i915, and I've
sent a series to do this in drm/bridge [1].

Bottom line, we should stop using struct edid in drivers. They'll all
parse the info differently, and from what I've seen, often wrong.


BR,
Jani.


[1] https://patchwork.freedesktop.org/series/128149/


>  	if (amdgpu_connector->edid)
>  		return;
>  
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index cd98b3565178..1faa21f542bd 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6562,17 +6562,23 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector)
>  {
>  	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
>  	struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
> +	struct amdgpu_device *adev = drm_to_adev(connector->dev);
>  	struct dc_link *dc_link = aconnector->dc_link;
>  	struct dc_sink *dc_em_sink = aconnector->dc_em_sink;
>  	struct edid *edid;
>  
> +	/* prefer ACPI over panel for eDP */
> +	edid = amdgpu_acpi_edid(adev, connector);
> +
>  	/*
>  	 * Note: drm_get_edid gets edid in the following order:
>  	 * 1) override EDID if set via edid_override debugfs,
>  	 * 2) firmware EDID if set via edid_firmware module parameter
>  	 * 3) regular DDC read.
>  	 */
> -	edid = drm_get_edid(connector, &amdgpu_connector->ddc_bus->aux.ddc);
> +	if (!edid)
> +		edid = drm_get_edid(connector, &amdgpu_connector->ddc_bus->aux.ddc);
> +
>  	if (!edid) {
>  		DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
>  		return;
> 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 e3915c4f8566..6bf2a8867e76 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
> @@ -895,6 +895,7 @@ enum dc_edid_status dm_helpers_read_local_edid(
>  {
>  	struct amdgpu_dm_connector *aconnector = link->priv;
>  	struct drm_connector *connector = &aconnector->base;
> +	struct amdgpu_device *adev = drm_to_adev(connector->dev);
>  	struct i2c_adapter *ddc;
>  	int retry = 3;
>  	enum dc_edid_status edid_status;
> @@ -909,8 +910,10 @@ enum dc_edid_status dm_helpers_read_local_edid(
>  	 * do check sum and retry to make sure read correct edid.
>  	 */
>  	do {
> -
> -		edid = drm_get_edid(&aconnector->base, ddc);
> +		/* prefer ACPI over panel for eDP */
> +		edid = amdgpu_acpi_edid(adev, connector);
> +		if (!edid)
> +			edid = drm_get_edid(&aconnector->base, ddc);
>  
>  		/* DP Compliance Test 4.2.2.6 */
>  		if (link->aux_mode && connector->edid_corrupt)

-- 
Jani Nikula, Intel

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

[Mario Limonciello]

On 1/29/2024 03:39, Jani Nikula wrote:
> On Fri, 26 Jan 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.
>>
>> Attempt to fetch this EDID if it exists and prefer it over the EDID
>> that is provided by the panel.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h           |  2 ++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c      | 30 +++++++++++++++++++
>>   .../gpu/drm/amd/amdgpu/amdgpu_connectors.c    |  5 ++++
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  8 ++++-
>>   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  7 +++--
>>   5 files changed, 49 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> index c5f3859fd682..99abe12567a4 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -1520,6 +1520,7 @@ int amdgpu_acpi_get_mem_info(struct amdgpu_device *adev, int xcc_id,
>>   
>>   void amdgpu_acpi_get_backlight_caps(struct amdgpu_dm_backlight_caps *caps);
>>   bool amdgpu_acpi_should_gpu_reset(struct amdgpu_device *adev);
>> +void *amdgpu_acpi_edid(struct amdgpu_device *adev, struct drm_connector *connector);
>>   void amdgpu_acpi_detect(void);
>>   void amdgpu_acpi_release(void);
>>   #else
>> @@ -1537,6 +1538,7 @@ static inline int amdgpu_acpi_get_mem_info(struct amdgpu_device *adev,
>>   }
>>   static inline void amdgpu_acpi_fini(struct amdgpu_device *adev) { }
>>   static inline bool amdgpu_acpi_should_gpu_reset(struct amdgpu_device *adev) { return false; }
>> +static inline void *amdgpu_acpi_edid(struct amdgpu_device *adev, struct drm_connector *connector) { return NULL; }
>>   static inline void amdgpu_acpi_detect(void) { }
>>   static inline void amdgpu_acpi_release(void) { }
>>   static inline bool amdgpu_acpi_is_power_shift_control_supported(void) { return false; }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>> index e550067e5c5d..c106335f1f22 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>> @@ -1380,6 +1380,36 @@ bool amdgpu_acpi_should_gpu_reset(struct amdgpu_device *adev)
>>   #endif
>>   }
>>   
>> +/**
>> + * amdgpu_acpi_edid
>> + * @adev: amdgpu_device pointer
>> + * @connector: drm_connector pointer
>> + *
>> + * Returns the EDID used for the internal panel if present, NULL otherwise.
>> + */
>> +void *
>> +amdgpu_acpi_edid(struct amdgpu_device *adev, struct drm_connector *connector)
>> +{
>> +	struct drm_device *ddev = adev_to_drm(adev);
>> +	struct acpi_device *acpidev = ACPI_COMPANION(ddev->dev);
>> +	void *edid;
>> +	int r;
>> +
>> +	if (!acpidev)
>> +		return NULL;
>> +
>> +	if (connector->connector_type != DRM_MODE_CONNECTOR_eDP)
>> +		return NULL;
>> +
>> +	r = acpi_video_get_edid(acpidev, ACPI_VIDEO_DISPLAY_LCD, -1, &edid);
>> +	if (r < 0) {
>> +		DRM_DEBUG_DRIVER("Failed to get EDID from ACPI: %d\n", r);
>> +		return NULL;
>> +	}
>> +
>> +	return kmemdup(edid, r, GFP_KERNEL);
>> +}
>> +
>>   /*
>>    * amdgpu_acpi_detect - detect ACPI ATIF/ATCS methods
>>    *
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>> index 9caba10315a8..c7e1563a46d3 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>> @@ -278,6 +278,11 @@ static void amdgpu_connector_get_edid(struct drm_connector *connector)
>>   	struct amdgpu_device *adev = drm_to_adev(dev);
>>   	struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
>>   
>> +	if (amdgpu_connector->edid)
>> +		return;
>> +
>> +	/* if the BIOS specifies the EDID via _DDC, prefer this */
>> +	amdgpu_connector->edid = amdgpu_acpi_edid(adev, connector);
> 
> Imagine the EDID returned by acpi_video_get_edid() has edid->extensions
> bigger than 4. Of course it should not, but you have no guarantees, and
> it originates outside of the kernel.
> 
> The real fix is to have the function return a struct drm_edid which
> tracks the allocation size separately. Unfortunately, it requires a
> bunch of changes along the way. We've mostly done it in i915, and I've
> sent a series to do this in drm/bridge [1].
> 
> Bottom line, we should stop using struct edid in drivers. They'll all
> parse the info differently, and from what I've seen, often wrong.
> 
> 

Thanks for the feedback.  In that case this specific change should 
probably rebase on the Melissa's work 
https://lore.kernel.org/amd-gfx/20240126163429.56714-1-mwen@igalia.com/ 
after she takes into account the feedback.

Let me ask you this though - do you think that after that's done should 
we let all drivers get EDID from BIOS as a priority?  Or would you 
prefer that this is unique to amdgpu?

Something like:

1) If user specifies on kernel command line and puts an EDID in 
/lib/firmware use that.
2) If BIOS has EDID in _DDC and it's eDP panel, use that.
3) Get panel EDID.

> BR,
> Jani.
> 
> 
> [1] https://patchwork.freedesktop.org/series/128149/
> 
> 
>>   	if (amdgpu_connector->edid)
>>   		return;
>>   
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index cd98b3565178..1faa21f542bd 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -6562,17 +6562,23 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector)
>>   {
>>   	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
>>   	struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
>> +	struct amdgpu_device *adev = drm_to_adev(connector->dev);
>>   	struct dc_link *dc_link = aconnector->dc_link;
>>   	struct dc_sink *dc_em_sink = aconnector->dc_em_sink;
>>   	struct edid *edid;
>>   
>> +	/* prefer ACPI over panel for eDP */
>> +	edid = amdgpu_acpi_edid(adev, connector);
>> +
>>   	/*
>>   	 * Note: drm_get_edid gets edid in the following order:
>>   	 * 1) override EDID if set via edid_override debugfs,
>>   	 * 2) firmware EDID if set via edid_firmware module parameter
>>   	 * 3) regular DDC read.
>>   	 */
>> -	edid = drm_get_edid(connector, &amdgpu_connector->ddc_bus->aux.ddc);
>> +	if (!edid)
>> +		edid = drm_get_edid(connector, &amdgpu_connector->ddc_bus->aux.ddc);
>> +
>>   	if (!edid) {
>>   		DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
>>   		return;
>> 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 e3915c4f8566..6bf2a8867e76 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
>> @@ -895,6 +895,7 @@ enum dc_edid_status dm_helpers_read_local_edid(
>>   {
>>   	struct amdgpu_dm_connector *aconnector = link->priv;
>>   	struct drm_connector *connector = &aconnector->base;
>> +	struct amdgpu_device *adev = drm_to_adev(connector->dev);
>>   	struct i2c_adapter *ddc;
>>   	int retry = 3;
>>   	enum dc_edid_status edid_status;
>> @@ -909,8 +910,10 @@ enum dc_edid_status dm_helpers_read_local_edid(
>>   	 * do check sum and retry to make sure read correct edid.
>>   	 */
>>   	do {
>> -
>> -		edid = drm_get_edid(&aconnector->base, ddc);
>> +		/* prefer ACPI over panel for eDP */
>> +		edid = amdgpu_acpi_edid(adev, connector);
>> +		if (!edid)
>> +			edid = drm_get_edid(&aconnector->base, ddc);
>>   
>>   		/* DP Compliance Test 4.2.2.6 */
>>   		if (link->aux_mode && connector->edid_corrupt)
> 

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

[Jani Nikula]

On Mon, 29 Jan 2024, Mario Limonciello <mario.limonciello@amd.com> wrote:
> On 1/29/2024 03:39, Jani Nikula wrote:
>> On Fri, 26 Jan 2024, Mario Limonciello <mario.limonciello@amd.com> wrote:
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>>> index 9caba10315a8..c7e1563a46d3 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>>> @@ -278,6 +278,11 @@ static void amdgpu_connector_get_edid(struct drm_connector *connector)
>>>   	struct amdgpu_device *adev = drm_to_adev(dev);
>>>   	struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
>>>   
>>> +	if (amdgpu_connector->edid)
>>> +		return;
>>> +
>>> +	/* if the BIOS specifies the EDID via _DDC, prefer this */
>>> +	amdgpu_connector->edid = amdgpu_acpi_edid(adev, connector);
>> 
>> Imagine the EDID returned by acpi_video_get_edid() has edid->extensions
>> bigger than 4. Of course it should not, but you have no guarantees, and
>> it originates outside of the kernel.
>> 
>> The real fix is to have the function return a struct drm_edid which
>> tracks the allocation size separately. Unfortunately, it requires a
>> bunch of changes along the way. We've mostly done it in i915, and I've
>> sent a series to do this in drm/bridge [1].

Looking at it again, perhaps the ACPI code should just return a blob,
and the drm code should have a helper to wrap that around struct
drm_edid, so that the ACPI code does not have to depend on drm. Basic
idea remains.

>> Bottom line, we should stop using struct edid in drivers. They'll all
>> parse the info differently, and from what I've seen, often wrong.
>> 
>> 
>
> Thanks for the feedback.  In that case this specific change should 
> probably rebase on the Melissa's work 
> https://lore.kernel.org/amd-gfx/20240126163429.56714-1-mwen@igalia.com/ 
> after she takes into account the feedback.
>
> Let me ask you this though - do you think that after that's done should 
> we let all drivers get EDID from BIOS as a priority?  Or would you 
> prefer that this is unique to amdgpu?

If the reason for having this is that the panel EDID contains some
garbage, that's certainly not unique to amdgpu... :p

> Something like:
>
> 1) If user specifies on kernel command line and puts an EDID in 
> /lib/firmware use that.
> 2) If BIOS has EDID in _DDC and it's eDP panel, use that.

I think we should also look into this. We currently don't do this, and
it might help with some machines. However, gut feeling says it's
probably better to keep this as a per driver decision instead of trying
to bolt it into drm helpers.

BR,
Jani.


> 3) Get panel EDID.
>

-- 
Jani Nikula, Intel

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

[Mario Limonciello]

On 1/29/2024 10:46, Jani Nikula wrote:
> On Mon, 29 Jan 2024, Mario Limonciello <mario.limonciello@amd.com> wrote:
>> On 1/29/2024 03:39, Jani Nikula wrote:
>>> On Fri, 26 Jan 2024, Mario Limonciello <mario.limonciello@amd.com> wrote:
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>>>> index 9caba10315a8..c7e1563a46d3 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
>>>> @@ -278,6 +278,11 @@ static void amdgpu_connector_get_edid(struct drm_connector *connector)
>>>>    	struct amdgpu_device *adev = drm_to_adev(dev);
>>>>    	struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
>>>>    
>>>> +	if (amdgpu_connector->edid)
>>>> +		return;
>>>> +
>>>> +	/* if the BIOS specifies the EDID via _DDC, prefer this */
>>>> +	amdgpu_connector->edid = amdgpu_acpi_edid(adev, connector);
>>>
>>> Imagine the EDID returned by acpi_video_get_edid() has edid->extensions
>>> bigger than 4. Of course it should not, but you have no guarantees, and
>>> it originates outside of the kernel.
>>>
>>> The real fix is to have the function return a struct drm_edid which
>>> tracks the allocation size separately. Unfortunately, it requires a
>>> bunch of changes along the way. We've mostly done it in i915, and I've
>>> sent a series to do this in drm/bridge [1].
> 
> Looking at it again, perhaps the ACPI code should just return a blob,
> and the drm code should have a helper to wrap that around struct
> drm_edid, so that the ACPI code does not have to depend on drm. Basic
> idea remains.

I'd ideally like to split this stuff and Melissa's rework to be 
independent if possible.  I'll see if that's actually feasible.

> 
>>> Bottom line, we should stop using struct edid in drivers. They'll all
>>> parse the info differently, and from what I've seen, often wrong.
>>>
>>>
>>
>> Thanks for the feedback.  In that case this specific change should
>> probably rebase on the Melissa's work
>> https://lore.kernel.org/amd-gfx/20240126163429.56714-1-mwen@igalia.com/
>> after she takes into account the feedback.
>>
>> Let me ask you this though - do you think that after that's done should
>> we let all drivers get EDID from BIOS as a priority?  Or would you
>> prefer that this is unique to amdgpu?
> 
> If the reason for having this is that the panel EDID contains some
> garbage, that's certainly not unique to amdgpu... :p

OK; maybe a helper in DRM that wraps the ACPI code then and amdgpu will 
use the helper for this series.

I'm also thinking it makes sense to have a new /proc/cmdline setup 
option to ignore the BIOS for EDID.  I'm hoping that since Windows uses 
_DDC that BIOS will be higher quality; but you know; BIOS =)

> 
>> Something like:
>>
>> 1) If user specifies on kernel command line and puts an EDID in
>> /lib/firmware use that.
>> 2) If BIOS has EDID in _DDC and it's eDP panel, use that.
> 
> I think we should also look into this. We currently don't do this, and
> it might help with some machines. However, gut feeling says it's
> probably better to keep this as a per driver decision instead of trying
> to bolt it into drm helpers.

OK; I'll wire up the helper and if you want to use in the future you can 
too then.

> 
> BR,
> Jani.
> 
> 
>> 3) Get panel EDID.
>>
>