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

Mario Limonciello mario.limonciello at amd.com
Mon Jan 29 16:09:35 UTC 2024


On 1/29/2024 03:39, Jani Nikula wrote:
> On Fri, 26 Jan 2024, Mario Limonciello <mario.limonciello at 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 at 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)
> 



More information about the dri-devel mailing list