Received-SPF: pass (google.com: domain of linux-kernel+bounces-43058-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <63c60424-1b2d-4912-81b2-7c7ead4c8289@amd.com>
Date: Mon, 29 Jan 2024 10:09:35 -0600
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 2/2] drm/amd: Fetch the EDID from _DDC if available for
 eDP
To: Jani Nikula <jani.nikula@linux.intel.com>, amd-gfx@lists.freedesktop.org,
 Alex Deucher <alexander.deucher@amd.com>,
 Harry Wentland <harry.wentland@amd.com>,
 "Rafael J . Wysocki" <rafael@kernel.org>, Hans de Goede <hdegoede@redhat.com>
Cc: open list <linux-kernel@vger.kernel.org>,
 "open list:DRM DRIVERS" <dri-devel@lists.freedesktop.org>,
 Melissa Wen <mwen@igalia.com>, "open list:ACPI"
 <linux-acpi@vger.kernel.org>, Mark Pearson <mpearson-lenovo@squebb.ca>
References: <20240126184639.8187-1-mario.limonciello@amd.com>
 <20240126184639.8187-3-mario.limonciello@amd.com> <87le88jx63.fsf@intel.com>
Content-Language: en-US
From: Mario Limonciello <mario.limonciello@amd.com>
In-Reply-To: <87le88jx63.fsf@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?bmErM0dWYWdkVWhjMzJkbFpQRXAveU9kK3RBbytWbEIzeE5pUUxPZVovOTJz?=
 =?utf-8?B?Q0Ira0o2WENwWVVpMjVEZ1hUc0pib29KWm5Kd25EKzNITVJneXpJdjZmYk91?=
 =?utf-8?B?NGhOT3ltbHY1M1Z5ZXpVc1lmN0xRSVFWa0dDcGlqVDVkUUQ5Ni9PRGFQZTUx?=
 =?utf-8?B?M1V5T3FRUUtNb0dFT3dKWTQxL0ZnMkEyR04vcVdhaFp0NnRsMTZ1dExmWEZB?=
 =?utf-8?B?cVkwa1VvZkxoK1hyRDZlbTBsdHNJbjhRNnYvUmJmK3pTMGpPYVFVclhnZi8w?=
 =?utf-8?B?R2lTL0VpdDF1M3BiSVk4bFM5Yi9lT2Y4VTVEeE5UMVUvaUpLK3dvVmp2bllp?=
 =?utf-8?B?MW1SOGRBVTRVYm1FTDBtby9CQ1BXV0FGRHZiaUVnNjg1VGplaERzdFpMUG1S?=
 =?utf-8?B?bFFGQ29pV2pRUHQwQ3NBY2VBeTVtVHgvNHgrU0txcld5c1d6TFlraDFXbFdz?=
 =?utf-8?B?MnBIRlV6bFdEZTd3QVQvbHdUT2d6OXdjKzVsNWU2Z2djU1BqN2JoVDhKbUI3?=
 =?utf-8?B?M01QWVJSbVJZdkhPNHZ2ZXlOVTUxZHBKWHRseEUwWHIxd0pGcGhJSDBEUGlM?=
 =?utf-8?B?NG4wSHpOWTAxd3VHUGs1RHI3b0hud05heHB0Q0ptSUJWQ1l1UHpROVd4TWFr?=
 =?utf-8?B?K1k0Tk5RdER5MlVNK0Q2amZZUC81dER2SHluUVRyQlB4Unh2MEUyWkcwSkJ1?=
 =?utf-8?B?akx5T0VWanhnYnY0NWIxVUdSL0xHUGl1RFRoWk10VDBnanJBbG1hUU9wNTNn?=
 =?utf-8?B?TTVJcWpNL3JvZ3Y2OHhNaUpnSlI4cjJ0TkNXWlVyVzBXcWdKNHh4V0RYV3Np?=
 =?utf-8?B?dHlBZnBNNHVxVzJoVG9odHE0NFk0V2w5c0gzYUhkelordGZneVZvKy9OVm1k?=
 =?utf-8?B?U3hnTDdiNlN2NWJNL0wyaTNteFhqeUYxTytXNXpBMzFFKzRWdklXMVB4SnhH?=
 =?utf-8?B?UUxwQWNORDR1UXRYaHhLNlFESmxCUlhiYjNkWUhjRTlxcHhDQzAyMDA0SmJk?=
 =?utf-8?B?RzRRUytvMEJVeHMvc2VhTmpXRTVoMlArNHZGck8xQlpKQ2lYaCtQdjZWaHdQ?=
 =?utf-8?B?MGRzM2Nkb3JZUVdwTXQ3UnA3ekEvOENpR0J3UFRDLytBdlRGZmVxZmx2R3lI?=
 =?utf-8?B?YTJaSitaWXMwYjRtRmtzcENQSjhLZTFGZFQ1NjRYUDhkbmFTVFlLSjRxei9L?=
 =?utf-8?B?RU5PNzFEZ2NCaHJ2Ni9RN25DbXRkc09SQUF6NHJ5ajdldWpObU9IbVNrNHJO?=
 =?utf-8?B?UFMwd2dCTjVUZTVhMktuMjRscnpYZEFUVzJRWXRHMGNzTE9wRThXVDhGbENx?=
 =?utf-8?B?ZjEra3RsQzllZDNxYzEyUmFuZDhTb2NMWFFaK09jZDNMTTdEN1JHYi9lN1h5?=
 =?utf-8?B?akZVRHhWU1JXdVkwTG56a1lTbVdKMTVvUUlTQzZPMjI4eWtIZGUwZzhWUVQ4?=
 =?utf-8?B?emFUVC8zbFRpbnZVUUtTN1FEdXhMSjNDTmJlN0duejZ3MHBoM2VjcUxaVHJW?=
 =?utf-8?B?TmplZ0hxL1VUUnNRWFF6WjNzUEhxeEdVREoxR0J3a0pNOS9hcWNtQWNnb1Vt?=
 =?utf-8?B?TUtEMkNXRzNEeEFveFdSbEJtRmxkRGJiV0VTNzd3R3BLT0xNTEVGanB5WURi?=
 =?utf-8?B?WUE4Z1V3SkU5SzRrWWR0dzYwcmNMdHprMFZIVG45dXE5TkwxUktudmdXWmRt?=
 =?utf-8?B?MjVGamNPVWJ4ZjVmWm16RCtqOTZHdnVlQ3o5TDl2QjBtTkE4UFJJTUg5MUhX?=
 =?utf-8?B?eWwwRUxsWFVmazVvYm9UQ3RLSHE5TTNaQ2grVFZLK1c0RDU2UGFpMVR4enJo?=
 =?utf-8?B?M29JQ01oUmliNnkzc0RxM0Fjb2VWY2J2NDhtRTdGcmVHQmhoWmJ3NkJ3WEJh?=
 =?utf-8?B?VnE4cTZlTC9OaEJ6aEZXQTQ3K0JCakFleDlqSjRKNWQ3NkJPTzFISmVxOWNq?=
 =?utf-8?B?akF5eWk4a2FXU3EvcEdib2duc3YvZ2FYdVV4TjBaY05ZTGFwdUdUSDJ1Z1Nn?=
 =?utf-8?B?Vkd3T1U2MlNmL2lIN3hkR1V1SjhmWkExVlUyd3FXUzA5czZPMlBqOHl1Mm1Z?=
 =?utf-8?B?TU5ad0UvMEYzRjd0L1RTUlZndnFpaXE1MFJoOE9IaEpxdTZxZHVJMzlZa2FC?=
 =?utf-8?Q?bESUlCXujnfaljGAyWFFHej/R?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 98872086-6283-4ca0-3aec-08dc20e4b026
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Jan 2024 16:09:36.1247
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 4UwIeUhUHSJupKIfaAP1ape61E3auzz9UClmrcT7bCpMO5/XX0YVc8Vm+15apCRYYN1wRw6CB9Ccd5b3tvh+ZQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR12MB7633

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