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

2024-01-29 Thread Mario Limonciello 

On 1/29/2024 07:54, Rafael J. Wysocki wrote:

On Fri, Jan 26, 2024 at 7:55 PM Mario Limonciello
 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 
---
  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, ,
-   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, 
,
+   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

2024-01-29 Thread Rafael J. Wysocki 
On Fri, Jan 26, 2024 at 7:55 PM Mario Limonciello
 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 
> ---
>  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, ,
> -   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, 
> ,
> +   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 1/2] ACPI: video: Handle fetching EDID that is longer than 256 bytes

2024-01-26 Thread 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 
---
 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, ,
-   length);
-   if (ACPI_FAILURE(status) || !buffer ||
-   buffer->type != ACPI_TYPE_BUFFER) {
-   continue;
+   while (length) {
+   length -= 128;
+   status = acpi_video_device_EDID(video_device, 
,
+   length);
+   if (ACPI_FAILURE(status) || !buffer ||
+   buffer->type != ACPI_TYPE_BUFFER) {
+   continue;
+   }
+   break;
}
+   if (!length)
+   continue;
}
 
*edid = buffer->buffer.pointer;
-- 
2.34.1