Received-SPF: pass (google.com: domain of linux-kernel+bounces-43061-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <ad625d5f-6b4b-41bb-a9c0-edb513697802@amd.com>
Date: Mon, 29 Jan 2024 10:12:13 -0600
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/2] ACPI: video: Handle fetching EDID that is longer than
 256 bytes
Content-Language: en-US
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: amd-gfx@lists.freedesktop.org, Alex Deucher <alexander.deucher@amd.com>,
 Harry Wentland <harry.wentland@amd.com>, Hans de Goede
 <hdegoede@redhat.com>, "open list:ACPI" <linux-acpi@vger.kernel.org>,
 open list <linux-kernel@vger.kernel.org>,
 "open list:DRM DRIVERS" <dri-devel@lists.freedesktop.org>,
 Melissa Wen <mwen@igalia.com>, Mark Pearson <mpearson-lenovo@squebb.ca>
References: <20240126184639.8187-1-mario.limonciello@amd.com>
 <20240126184639.8187-2-mario.limonciello@amd.com>
 <CAJZ5v0iX5=u5y0JS2OzYMvYNnjZBCM2YfSTsSdg3CtH4rBMyUw@mail.gmail.com>
From: Mario Limonciello <mario.limonciello@amd.com>
In-Reply-To: <CAJZ5v0iX5=u5y0JS2OzYMvYNnjZBCM2YfSTsSdg3CtH4rBMyUw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?TjhiM0hBcTJ0ZnhYcXgrbnhBaXl2M3owbTNLSHgxUEwybzNQRmFzeHk5SHps?=
 =?utf-8?B?WG5KTms5WU8wMFN6WjR3Z21Eb1dhZWFjM1lGcHZ0ZEZHV1dRY2x4RDlSaVU2?=
 =?utf-8?B?NWVFUkpaOUFTMkZsSEJ5eGhyQllIYVI4TlpTd1FybEhpNFpvRkFoNkhjZlZw?=
 =?utf-8?B?QWpQSlFETEVxMUFJUWZrcVlHc21ydmRlbXFlQkF0S2ZPaDN0K0JkTWc1SGlL?=
 =?utf-8?B?VHhuSUFkV0o1a3lXRHhyUTBJTEZnYmV3bkY5NTFsUFBLMmUzRGgwUnk3d1Nr?=
 =?utf-8?B?MFpwSDlDS3JuNnM0K3RXeHpOMnl1OVhodlc2Tytyb2h3WEl2V0xlVnFPbndw?=
 =?utf-8?B?bEdMNUZDK083L2V1L0xCdE5UN0FLZFJFMjducEVZWUxqMVVBWVJCYlhKMGQ0?=
 =?utf-8?B?OEpWa0JJN29uY2VKNjRQM05GZ3l2eW93MlBRNlFZWnZqcXdPcE5vckR5WGg0?=
 =?utf-8?B?QzczRmF5UmhsZVVNUHVmb0g1ekJjbXRBV3hWVm4rOFR5blgvbC9HWjQ0cDJO?=
 =?utf-8?B?azhLQktFY0RRYmk4NFNEaC9Ed2piVlg4WTUvVnN1aHRtS1FXZ0RBM3JnQ1pt?=
 =?utf-8?B?bG5qalR3TlNFMXE1cU1LbzluRmVaWDJnTnRrQk9oeWM1ZXhaRTd0S3B3cHc3?=
 =?utf-8?B?V3lhdTFVbFZRblhYZG9FNHlPclRWMFUwVGJsZjhoVmJXeGVqRjV0M1lHM21O?=
 =?utf-8?B?YjUrNFFZT3NBQVlsRmV4UnhIUlR3dWh6MHVCR0lWSjBlN0RhSHAwMHNaN0tx?=
 =?utf-8?B?ZC9rMGxxUW1OVCtYbDBBUEordHhhMENtanlIV0xnT0RpdFdrMFNhRFNaUVFu?=
 =?utf-8?B?VlRvMThjbUZmdTh5ZXhyNXNrNHF0SURMTXVYZlptSVl5eHZHOXZJWGxDV3p3?=
 =?utf-8?B?Y2pIV2FENm9aUm9ETENwZWhrQWFCZHpSRTg3WGpPTzVPVXJ4RWMzNlFvY25J?=
 =?utf-8?B?WHhCd2duVU0yZmp1UTAxWmFDQ2pvZVE1aUhSaXVsNnJLVUQvUVkzTjVRRVd1?=
 =?utf-8?B?RVNPSUtrYTM4QTZrd0NvL0pGd2V5VXAydjdMN09lK1I4cUhSbWRPZE9FK2xj?=
 =?utf-8?B?Y1lUUjdva2x3NDU1TWFiYzJwUS9ZOUZzZHNpc0kyVlBOclhvWDNjMGttSEtV?=
 =?utf-8?B?dXdxZE5aZlQ3eTZ2NHRQRi9qcEZGVm9zSWNPRUkrVk9RVGxlQmFDZ0JZUGJm?=
 =?utf-8?B?OFBCb29mRnlqMmxxQS9sdjlRZTR4OWRwWDhCU25LL1dOL3JrS3g1eGpXQUJV?=
 =?utf-8?B?SDUzd1lyaDRDcDNIZXZiNFk3bGVPT0VxZ0dLN3FYdmEwVVJ3SjRNbHM3aVJw?=
 =?utf-8?B?SHRqdjFnZE92Y3ZXbVpoVGpaSC83V05XemhvRGhmanZBaU1scTlydmZ4QzUz?=
 =?utf-8?B?aSszTGN3Sjd3Z3F4S2Y0VXFSdTNCcWpCRk0vUzBSUmdTeTM4ekg4alRoZ0Vs?=
 =?utf-8?B?VHZZSUxvNGRVVndqTmphd0JHRCtORi9Ga1YvU29kSjgzRkZzOGVjWmpTVGgx?=
 =?utf-8?B?U3JvRVUxOWxVRkJkUzZlZzdpOGVPaVlUWEo4ZGxsMVVnVWpJQm1LaStDYjdh?=
 =?utf-8?B?VGpOSWh6Q0RLTFdEVTRtT0xManhqRkFSL1N3aE9hRWpiK2ttNG9uRCtwZFlI?=
 =?utf-8?B?dk5OV3VESzF5TSs2UVYyeWdhMDJEcXhnZWczbGNlRytUWnRDZmh1YnlsUGNN?=
 =?utf-8?B?RW5jU1cxM25BbHhqR3hVS2xKQW9NZUVUb2paNDg1WFlJTmE4R3BKWnl1WGFE?=
 =?utf-8?B?dis4WUVseU1zZ1E4bDNLbEU2UDliYm1KdUx4aFlCQ0dnL29leXM0UWJnS1pF?=
 =?utf-8?B?cXE3NENuR21jdGR6NDlEc2JUNVZkQjAxQzhxekM3L1JBZWNhSFhXQ2l6cjM1?=
 =?utf-8?B?VUtob3pEcTRNTWoyS2c5RXFMMktYQUNWSXJqOEF5Zk1hYTIzVnVaN0hUS0Ex?=
 =?utf-8?B?M2F0NlZpOWVabDhQQWRVZnR0Zm9XOHE0aThCVmx1M1JlL0Q4L0RIVGsyQ2hi?=
 =?utf-8?B?SzAySi9wTG04bzVPSjkrTjRkb3VMUGxpOU1zUGhCYlZnRkU2eXJKcVJZOWwr?=
 =?utf-8?B?aG1BUCtvMHluVFAyUWNtbjlZUURMY0V4SkF4YTltVDl2WElqTFBKMjRDdHA0?=
 =?utf-8?Q?iwuuXjqpv43BlbTHe5zIhZKDo?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 711d9a4b-14a7-485c-fde9-08dc20e50e99
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:12:14.5492
 (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: b4myuZIPKU1Kcuip4bj6+eD3uAlhYGPv4GXvCSIzeDBX1taSN11iY95B39x4keeoCszQV+vnh5xKgGlSFLc3DQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR12MB6789

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