diff mbox series

[05/11] edid-decode: fix standard timing vertical pixels

Message ID 20210914121129.51451-6-joevt@shaw.ca (mailing list archive)
State New, archived
Headers show
Series edid-decode: bug fixes, additions, changes | expand

Commit Message

joevt Sept. 14, 2021, 12:11 p.m. UTC 
Don't do ceiling to nearest 8 pixels for active vertical lines. See examples elo-4600l-hdmi and kogan-kaled24144f-hdmi.

Section 3.9 and 3.10.3.6 of EDID 1.4 does not say vertical lines must be a multiple of 8.  This line of code appears to have been added to satisfy the 3rd example in VTB-EXT spec but that example has an incorrect HAP indicator decimal value so it cannot be trusted. Also, all 3 examples have an incorrect vertical refresh value as noted in parse-vtb-ext-block.cpp. The VESA DMT spec has the following examples that are not a multiple of 8 lines which support this change:
1400x1050 4:3
1440x900 16:10
1600x900 16:9
1680x1050 16:10

Signed-off-by: Joe van Tunen <joevt@shaw.ca>
---
 parse-base-block.cpp | 1 -
 1 file changed, 1 deletion(-)

Comments

Hans Verkuil Sept. 15, 2021, 10:08 a.m. UTC | #1 
On 14/09/2021 14:11, joevt wrote:
> Don't do ceiling to nearest 8 pixels for active vertical lines. See examples elo-4600l-hdmi and kogan-kaled24144f-hdmi.
> 
> Section 3.9 and 3.10.3.6 of EDID 1.4 does not say vertical lines must be a multiple of 8.  This line of code appears to have been added to satisfy the 3rd example in VTB-EXT spec but that example has an incorrect HAP indicator decimal value so it cannot be trusted. Also, all 3 examples have an incorrect vertical refresh value as noted in parse-vtb-ext-block.cpp. The VESA DMT spec has the following examples that are not a multiple of 8 lines which support this change:
> 1400x1050 4:3
> 1440x900 16:10
> 1600x900 16:9
> 1680x1050 16:10
> 
> Signed-off-by: Joe van Tunen <joevt@shaw.ca>
> ---
>  parse-base-block.cpp | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/parse-base-block.cpp b/parse-base-block.cpp
> index e2901a6..32d2079 100644
> --- a/parse-base-block.cpp
> +++ b/parse-base-block.cpp
> @@ -573,7 +573,6 @@ void edid_state::print_standard_timing(const char *prefix, unsigned char b1, uns
>  		break;
>  	}
>  	vact = (double)hact * vratio / hratio;
> -	vact = 8 * ((vact + 7) / 8);

I need to look closer at this. I think it was added to help with 1360x768, which without
this line maps to 1360x765.

I'll get back to you on this.

Regards,

	Hans

>  	refresh = (b2 & 0x3f) + 60;
>  
>  	formula.hact = hact;
>
Hans Verkuil Sept. 15, 2021, 11:10 a.m. UTC | #2 
On 15/09/2021 12:08, Hans Verkuil wrote:
> On 14/09/2021 14:11, joevt wrote:
>> Don't do ceiling to nearest 8 pixels for active vertical lines. See examples elo-4600l-hdmi and kogan-kaled24144f-hdmi.
>>
>> Section 3.9 and 3.10.3.6 of EDID 1.4 does not say vertical lines must be a multiple of 8.  This line of code appears to have been added to satisfy the 3rd example in VTB-EXT spec but that example has an incorrect HAP indicator decimal value so it cannot be trusted. Also, all 3 examples have an incorrect vertical refresh value as noted in parse-vtb-ext-block.cpp. The VESA DMT spec has the following examples that are not a multiple of 8 lines which support this change:
>> 1400x1050 4:3
>> 1440x900 16:10
>> 1600x900 16:9
>> 1680x1050 16:10
>>
>> Signed-off-by: Joe van Tunen <joevt@shaw.ca>
>> ---
>>  parse-base-block.cpp | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/parse-base-block.cpp b/parse-base-block.cpp
>> index e2901a6..32d2079 100644
>> --- a/parse-base-block.cpp
>> +++ b/parse-base-block.cpp
>> @@ -573,7 +573,6 @@ void edid_state::print_standard_timing(const char *prefix, unsigned char b1, uns
>>  		break;
>>  	}
>>  	vact = (double)hact * vratio / hratio;
>> -	vact = 8 * ((vact + 7) / 8);
> 
> I need to look closer at this. I think it was added to help with 1360x768, which without
> this line maps to 1360x765.
> 
> I'll get back to you on this.

I did some more reading on this, and appendix D of the EDID 1.4 spec
says this (D-8):

"If calculated aspect ratio is not 16 : 10 AR, 4 : 3 AR, 5 : 4 AR or 16 : 9 AR
 what timing description should be used?"

"Ref.: Section 3.9 (E-EDID Standard Release A, Revision 2)
 The Standard Timings Identification code may not be used to
 identify timings which do not match one of these standard aspect
 ratios. Support for such timings must be indicated elsewhere,
 e.g., by use of a Detailed Timing Descriptor."

So you are correct with your change, but I think it would make
sense to add a new check:

// See also Ref. D-8 in the EDID-1.4 spec
if (vact & 1)
	warn("Standard Timing %ux%u has a dubious odd vertical resolution.\n", hact, vact);

So that way an attempt to use ST to describe 1360x768 will result in a warning.

Regards,

	Hans

> 
> Regards,
> 
> 	Hans
> 
>>  	refresh = (b2 & 0x3f) + 60;
>>  
>>  	formula.hact = hact;
>>
>
joevt Sept. 15, 2021, 6:28 p.m. UTC | #3 
Good find. I agree with that warning.

On 2021-09-15, 4:10 AM, "Hans Verkuil" <hverkuil@xs4all.nl> wrote:

    On 15/09/2021 12:08, Hans Verkuil wrote:
    > On 14/09/2021 14:11, joevt wrote:
    >> Don't do ceiling to nearest 8 pixels for active vertical lines. See examples elo-4600l-hdmi and kogan-kaled24144f-hdmi.
    >>
    >> Section 3.9 and 3.10.3.6 of EDID 1.4 does not say vertical lines must be a multiple of 8.  This line of code appears to have been added to satisfy the 3rd example in VTB-EXT spec but that example has an incorrect HAP indicator decimal value so it cannot be trusted. Also, all 3 examples have an incorrect vertical refresh value as noted in parse-vtb-ext-block.cpp. The VESA DMT spec has the following examples that are not a multiple of 8 lines which support this change:
    >> 1400x1050 4:3
    >> 1440x900 16:10
    >> 1600x900 16:9
    >> 1680x1050 16:10
    >>
    >> Signed-off-by: Joe van Tunen <joevt@shaw.ca>
    >> ---
    >>  parse-base-block.cpp | 1 -
    >>  1 file changed, 1 deletion(-)
    >>
    >> diff --git a/parse-base-block.cpp b/parse-base-block.cpp
    >> index e2901a6..32d2079 100644
    >> --- a/parse-base-block.cpp
    >> +++ b/parse-base-block.cpp
    >> @@ -573,7 +573,6 @@ void edid_state::print_standard_timing(const char *prefix, unsigned char b1, uns
    >>  		break;
    >>  	}
    >>  	vact = (double)hact * vratio / hratio;
    >> -	vact = 8 * ((vact + 7) / 8);
    > 
    > I need to look closer at this. I think it was added to help with 1360x768, which without
    > this line maps to 1360x765.
    > 
    > I'll get back to you on this.

    I did some more reading on this, and appendix D of the EDID 1.4 spec
    says this (D-8):

    "If calculated aspect ratio is not 16 : 10 AR, 4 : 3 AR, 5 : 4 AR or 16 : 9 AR
     what timing description should be used?"

    "Ref.: Section 3.9 (E-EDID Standard Release A, Revision 2)
     The Standard Timings Identification code may not be used to
     identify timings which do not match one of these standard aspect
     ratios. Support for such timings must be indicated elsewhere,
     e.g., by use of a Detailed Timing Descriptor."

    So you are correct with your change, but I think it would make
    sense to add a new check:

    // See also Ref. D-8 in the EDID-1.4 spec
    if (vact & 1)
    	warn("Standard Timing %ux%u has a dubious odd vertical resolution.\n", hact, vact);

    So that way an attempt to use ST to describe 1360x768 will result in a warning.

    Regards,

    	Hans

    > 
    > Regards,
    > 
    > 	Hans
    > 
    >>  	refresh = (b2 & 0x3f) + 60;
    >>  
    >>  	formula.hact = hact;
    >>
    >
diff mbox series

Patch

diff --git a/parse-base-block.cpp b/parse-base-block.cpp
index e2901a6..32d2079 100644
--- a/parse-base-block.cpp
+++ b/parse-base-block.cpp
@@ -573,7 +573,6 @@  void edid_state::print_standard_timing(const char *prefix, unsigned char b1, uns
 		break;
 	}
 	vact = (double)hact * vratio / hratio;
-	vact = 8 * ((vact + 7) / 8);
 	refresh = (b2 & 0x3f) + 60;
 
 	formula.hact = hact;