[PATCH 0/2] Docs/EDID: Fixed and improved EDID documentation

[PATCH 0/2] Docs/EDID: Fixed and improved EDID documentation

[Christoph Niedermaier]

A problem was found when EDID data sets for displays other
than the provided samples were generated. The patch series has
no effect on the provided samples that still match the data
used in drivers/gpu/drm/drm_edid_load.c.
The provided samples use small values for XOFFSET, XPULSE,
YOFFSET and YPULSE, where the error doesn't occur. This fix
corrects the use of that values in case of high values, because
the most significant bits were treated incorrectly.

The previous version made it necessary to first generate an
EDID data set without correct CRC and then to fix the CRC in
a second step. This patch series adds the CRC calculation to the
makefile in such a way that a correct EDID data set is generated
in a single build step.

---

Christoph Niedermaier (2):
  Docs/EDID: Fixed erroneous bits of XOFFSET, XPULSE, YOFFSET and YPULSE
  Docs/EDID: Calculate CRC while building the code

 Documentation/EDID/1024x768.S  |  5 ++---
 Documentation/EDID/1280x1024.S |  5 ++---
 Documentation/EDID/1600x1200.S |  5 ++---
 Documentation/EDID/1680x1050.S |  5 ++---
 Documentation/EDID/1920x1080.S |  5 ++---
 Documentation/EDID/800x600.S   |  5 ++---
 Documentation/EDID/HOWTO.txt   | 13 ++-----------
 Documentation/EDID/Makefile    | 15 +++++++++++++--
 Documentation/EDID/edid.S      | 10 ++++++----
 9 files changed, 33 insertions(+), 35 deletions(-)

-- 
2.1.4

[PATCH 1/2] Docs/EDID: Fixed erroneous bits of XOFFSET, XPULSE, YOFFSET and YPULSE

[Christoph Niedermaier]

The problem was found when EDID data sets for displays other
than the provided samples were generated. The patch has
no effect on the provided samples that still match the data
used in drivers/gpu/drm/drm_edid_load.c.

The provided samples use small values for XOFFSET, XPULSE,
YOFFSET and YPULSE, where the error doesn't occur. This fix
corrects the use of that values in case of high values, because
the most significant bits were treated incorrectly.

So in edid.S msbs4 should use bit 8 and 9 of XOFFSET and XPULS.
For YOFFSET and YPULSE msbs4 should use bit 4 and 5.

lsbs2 was introduced for a better overview, without
functional change.

Removing also the useless value 63 of all files, because
it is added in the *.S description files and then it is
subtracted in edid.S.

Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.de>
Reviewed-by: Carsten Emde <C.Emde@osadl.org>
---
 Documentation/EDID/1024x768.S  |  4 ++--
 Documentation/EDID/1280x1024.S |  4 ++--
 Documentation/EDID/1600x1200.S |  4 ++--
 Documentation/EDID/1680x1050.S |  4 ++--
 Documentation/EDID/1920x1080.S |  4 ++--
 Documentation/EDID/800x600.S   |  4 ++--
 Documentation/EDID/HOWTO.txt   |  4 ++--
 Documentation/EDID/edid.S      | 10 ++++++----
 8 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/Documentation/EDID/1024x768.S b/Documentation/EDID/1024x768.S
index 6f3e4b7..ff4013e 100644
--- a/Documentation/EDID/1024x768.S
+++ b/Documentation/EDID/1024x768.S
@@ -31,8 +31,8 @@
 #define YBLANK 38
 #define XOFFSET 8
 #define XPULSE 144
-#define YOFFSET (63+3)
-#define YPULSE (63+6)
+#define YOFFSET 3
+#define YPULSE 6
 #define DPI 72
 #define VFREQ 60 /* Hz */
 #define TIMING_NAME "Linux XGA"
diff --git a/Documentation/EDID/1280x1024.S b/Documentation/EDID/1280x1024.S
index bd9bef2a..ce0e85b 100644
--- a/Documentation/EDID/1280x1024.S
+++ b/Documentation/EDID/1280x1024.S
@@ -31,8 +31,8 @@
 #define YBLANK 42
 #define XOFFSET 48
 #define XPULSE 112
-#define YOFFSET (63+1)
-#define YPULSE (63+3)
+#define YOFFSET 1
+#define YPULSE 3
 #define DPI 72
 #define VFREQ 60 /* Hz */
 #define TIMING_NAME "Linux SXGA"
diff --git a/Documentation/EDID/1600x1200.S b/Documentation/EDID/1600x1200.S
index a45101c..5eeb751 100644
--- a/Documentation/EDID/1600x1200.S
+++ b/Documentation/EDID/1600x1200.S
@@ -31,8 +31,8 @@
 #define YBLANK 50
 #define XOFFSET 64
 #define XPULSE 192
-#define YOFFSET (63+1)
-#define YPULSE (63+3)
+#define YOFFSET 1
+#define YPULSE 3
 #define DPI 72
 #define VFREQ 60 /* Hz */
 #define TIMING_NAME "Linux UXGA"
diff --git a/Documentation/EDID/1680x1050.S b/Documentation/EDID/1680x1050.S
index b0d7c69..ec67950 100644
--- a/Documentation/EDID/1680x1050.S
+++ b/Documentation/EDID/1680x1050.S
@@ -31,8 +31,8 @@
 #define YBLANK 39
 #define XOFFSET 104
 #define XPULSE 176
-#define YOFFSET (63+3)
-#define YPULSE (63+6)
+#define YOFFSET 3
+#define YPULSE 6
 #define DPI 96
 #define VFREQ 60 /* Hz */
 #define TIMING_NAME "Linux WSXGA"
diff --git a/Documentation/EDID/1920x1080.S b/Documentation/EDID/1920x1080.S
index 3084355e..e0657af 100644
--- a/Documentation/EDID/1920x1080.S
+++ b/Documentation/EDID/1920x1080.S
@@ -31,8 +31,8 @@
 #define YBLANK 45
 #define XOFFSET 88
 #define XPULSE 44
-#define YOFFSET (63+4)
-#define YPULSE (63+5)
+#define YOFFSET 4
+#define YPULSE 5
 #define DPI 96
 #define VFREQ 60 /* Hz */
 #define TIMING_NAME "Linux FHD"
diff --git a/Documentation/EDID/800x600.S b/Documentation/EDID/800x600.S
index 6644e26..b6853b2 100644
--- a/Documentation/EDID/800x600.S
+++ b/Documentation/EDID/800x600.S
@@ -28,8 +28,8 @@
 #define YBLANK 28
 #define XOFFSET 40
 #define XPULSE 128
-#define YOFFSET (63+1)
-#define YPULSE (63+4)
+#define YOFFSET 1
+#define YPULSE 4
 #define DPI 72
 #define VFREQ 60 /* Hz */
 #define TIMING_NAME "Linux SVGA"
diff --git a/Documentation/EDID/HOWTO.txt b/Documentation/EDID/HOWTO.txt
index 835db33..7d05a7d 100644
--- a/Documentation/EDID/HOWTO.txt
+++ b/Documentation/EDID/HOWTO.txt
@@ -45,8 +45,8 @@ EDID:
 
 #define YPIX vdisp
 #define YBLANK vtotal-vdisp
-#define YOFFSET (63+(vsyncstart-vdisp))
-#define YPULSE (63+(vsyncend-vsyncstart))
+#define YOFFSET vsyncstart-vdisp
+#define YPULSE vsyncend-vsyncstart
 
 The CRC value in the last line
   #define CRC 0x55
diff --git a/Documentation/EDID/edid.S b/Documentation/EDID/edid.S
index ef082dc..c3d1381 100644
--- a/Documentation/EDID/edid.S
+++ b/Documentation/EDID/edid.S
@@ -47,9 +47,11 @@
 #define mfgname2id(v1,v2,v3) \
 	((((v1-'@')&0x1f)<<10)+(((v2-'@')&0x1f)<<5)+((v3-'@')&0x1f))
 #define swap16(v1) ((v1>>8)+((v1&0xff)<<8))
+#define lsbs2(v1,v2) (((v1&0x0f)<<4)+(v2&0x0f))
 #define msbs2(v1,v2) ((((v1>>8)&0x0f)<<4)+((v2>>8)&0x0f))
 #define msbs4(v1,v2,v3,v4) \
-	(((v1&0x03)>>2)+((v2&0x03)>>4)+((v3&0x03)>>6)+((v4&0x03)>>8))
+	((((v1>>8)&0x03)<<6)+(((v2>>8)&0x03)<<4)+\
+	(((v3>>4)&0x03)<<2)+((v4>>4)&0x03))
 #define pixdpi2mm(pix,dpi) ((pix*25)/dpi)
 #define xsize pixdpi2mm(XPIX,DPI)
 #define ysize pixdpi2mm(YPIX,DPI)
@@ -200,9 +202,9 @@ y_msbs:		.byte	msbs2(YPIX,YBLANK)
 x_snc_off_lsb:	.byte	XOFFSET&0xff
 /* Horizontal sync pulse width pixels 8 lsbits (0-1023) */
 x_snc_pls_lsb:	.byte	XPULSE&0xff
-/* Bits 7-4 	Vertical sync offset lines 4 lsbits -63)
-   Bits 3-0 	Vertical sync pulse width lines 4 lsbits -63) */
-y_snc_lsb:	.byte	((YOFFSET-63)<<4)+(YPULSE-63)
+/* Bits 7-4 	Vertical sync offset lines 4 lsbits (0-63)
+   Bits 3-0 	Vertical sync pulse width lines 4 lsbits (0-63) */
+y_snc_lsb:	.byte	lsbs2(YOFFSET, YPULSE)
 /* Bits 7-6 	Horizontal sync offset pixels 2 msbits
    Bits 5-4 	Horizontal sync pulse width pixels 2 msbits
    Bits 3-2 	Vertical sync offset lines 2 msbits
-- 
2.1.4

[PATCH 2/2] Docs/EDID: Calculate CRC while building the code

[Christoph Niedermaier]

The previous version made it necessary to first generate an
EDID data set without correct CRC and then to fix the CRC in
a second step. This patch adds the CRC calculation to the
makefile in such a way that a correct EDID data set is generated
in a single build step. Successfully tested with all existing
and a couple of new data sets.

Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.de>
Reviewed-by: Carsten Emde <C.Emde@osadl.org>
---
 Documentation/EDID/1024x768.S  |  1 -
 Documentation/EDID/1280x1024.S |  1 -
 Documentation/EDID/1600x1200.S |  1 -
 Documentation/EDID/1680x1050.S |  1 -
 Documentation/EDID/1920x1080.S |  1 -
 Documentation/EDID/800x600.S   |  1 -
 Documentation/EDID/HOWTO.txt   |  9 ---------
 Documentation/EDID/Makefile    | 15 +++++++++++++--
 8 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/Documentation/EDID/1024x768.S b/Documentation/EDID/1024x768.S
index ff4013e..4aed3f9 100644
--- a/Documentation/EDID/1024x768.S
+++ b/Documentation/EDID/1024x768.S
@@ -39,6 +39,5 @@
 #define ESTABLISHED_TIMING2_BITS 0x08 /* Bit 3 -> 1024x768 @60 Hz */
 #define HSYNC_POL 0
 #define VSYNC_POL 0
-#define CRC 0x55
 
 #include "edid.S"
diff --git a/Documentation/EDID/1280x1024.S b/Documentation/EDID/1280x1024.S
index ce0e85b..b26dd42 100644
--- a/Documentation/EDID/1280x1024.S
+++ b/Documentation/EDID/1280x1024.S
@@ -39,6 +39,5 @@
 /* No ESTABLISHED_TIMINGx_BITS */
 #define HSYNC_POL 1
 #define VSYNC_POL 1
-#define CRC 0xa0
 
 #include "edid.S"
diff --git a/Documentation/EDID/1600x1200.S b/Documentation/EDID/1600x1200.S
index 5eeb751..0d091b2 100644
--- a/Documentation/EDID/1600x1200.S
+++ b/Documentation/EDID/1600x1200.S
@@ -39,6 +39,5 @@
 /* No ESTABLISHED_TIMINGx_BITS */
 #define HSYNC_POL 1
 #define VSYNC_POL 1
-#define CRC 0x9d
 
 #include "edid.S"
diff --git a/Documentation/EDID/1680x1050.S b/Documentation/EDID/1680x1050.S
index ec67950..7dfed9a 100644
--- a/Documentation/EDID/1680x1050.S
+++ b/Documentation/EDID/1680x1050.S
@@ -39,6 +39,5 @@
 /* No ESTABLISHED_TIMINGx_BITS */
 #define HSYNC_POL 1
 #define VSYNC_POL 1
-#define CRC 0x26
 
 #include "edid.S"
diff --git a/Documentation/EDID/1920x1080.S b/Documentation/EDID/1920x1080.S
index e0657af..d6ffbba 100644
--- a/Documentation/EDID/1920x1080.S
+++ b/Documentation/EDID/1920x1080.S
@@ -39,6 +39,5 @@
 /* No ESTABLISHED_TIMINGx_BITS */
 #define HSYNC_POL 1
 #define VSYNC_POL 1
-#define CRC 0x05
 
 #include "edid.S"
diff --git a/Documentation/EDID/800x600.S b/Documentation/EDID/800x600.S
index b6853b2..a561658 100644
--- a/Documentation/EDID/800x600.S
+++ b/Documentation/EDID/800x600.S
@@ -36,6 +36,5 @@
 #define ESTABLISHED_TIMING1_BITS 0x01 /* Bit 0: 800x600 @ 60Hz */
 #define HSYNC_POL 1
 #define VSYNC_POL 1
-#define CRC 0xc2
 
 #include "edid.S"
diff --git a/Documentation/EDID/HOWTO.txt b/Documentation/EDID/HOWTO.txt
index 7d05a7d..539871c 100644
--- a/Documentation/EDID/HOWTO.txt
+++ b/Documentation/EDID/HOWTO.txt
@@ -47,12 +47,3 @@ EDID:
 #define YBLANK vtotal-vdisp
 #define YOFFSET vsyncstart-vdisp
 #define YPULSE vsyncend-vsyncstart
-
-The CRC value in the last line
-  #define CRC 0x55
-also is a bit tricky. After a first version of the binary data set is
-created, it must be checked with the "edid-decode" utility which will
-most probably complain about a wrong CRC. Fortunately, the utility also
-displays the correct CRC which must then be inserted into the source
-file. After the make procedure is repeated, the EDID data set is ready
-to be used.
diff --git a/Documentation/EDID/Makefile b/Documentation/EDID/Makefile
index 17763ca..85a927d 100644
--- a/Documentation/EDID/Makefile
+++ b/Documentation/EDID/Makefile
@@ -15,10 +15,21 @@ clean:
 %.o:	%.S
 	@cc -c $^
 
-%.bin:	%.o
+%.bin.nocrc:	%.o
 	@objcopy -Obinary $^ $@
 
-%.bin.ihex:	%.o
+%.crc:	%.bin.nocrc
+	@list=$$(for i in `seq 1 127`; do head -c$$i $^ | tail -c1 \
+		| hexdump -v -e '/1 "%02X+"'; done); \
+		echo "ibase=16;100-($${list%?})%100" | bc >$@
+
+%.p:	%.crc %.S
+	@cc -c -DCRC="$$(cat $*.crc)" -o $@ $*.S
+
+%.bin:	%.p
+	@objcopy -Obinary $^ $@
+
+%.bin.ihex:	%.p
 	@objcopy -Oihex $^ $@
 	@dos2unix $@ 2>/dev/null
 
-- 
2.1.4

Re: [PATCH 0/2] Docs/EDID: Fixed and improved EDID documentation

[Jonathan Corbet]

On Mon, 5 Nov 2018 09:48:33 +0100
Christoph Niedermaier <cniedermaier@dh-electronics.de> wrote:

> A problem was found when EDID data sets for displays other
> than the provided samples were generated. The patch series has
> no effect on the provided samples that still match the data
> used in drivers/gpu/drm/drm_edid_load.c.
> The provided samples use small values for XOFFSET, XPULSE,
> YOFFSET and YPULSE, where the error doesn't occur. This fix
> corrects the use of that values in case of high values, because
> the most significant bits were treated incorrectly.
> 
> The previous version made it necessary to first generate an
> EDID data set without correct CRC and then to fix the CRC in
> a second step. This patch series adds the CRC calculation to the
> makefile in such a way that a correct EDID data set is generated
> in a single build step.

This seems reasonable, I guess; I've applied both.  It seems to me, though,
that this stuff is in the wrong place.  Perhaps we should go one step
further and move it to tools/ ?

Thanks,

jon

Re: [PATCH 0/2] Docs/EDID: Fixed and improved EDID documentation

[Jani Nikula]

On Tue, 06 Nov 2018, Jonathan Corbet <corbet@lwn.net> wrote:
> On Mon, 5 Nov 2018 09:48:33 +0100
> Christoph Niedermaier <cniedermaier@dh-electronics.de> wrote:
>
>> A problem was found when EDID data sets for displays other
>> than the provided samples were generated. The patch series has
>> no effect on the provided samples that still match the data
>> used in drivers/gpu/drm/drm_edid_load.c.
>> The provided samples use small values for XOFFSET, XPULSE,
>> YOFFSET and YPULSE, where the error doesn't occur. This fix
>> corrects the use of that values in case of high values, because
>> the most significant bits were treated incorrectly.
>> 
>> The previous version made it necessary to first generate an
>> EDID data set without correct CRC and then to fix the CRC in
>> a second step. This patch series adds the CRC calculation to the
>> makefile in such a way that a correct EDID data set is generated
>> in a single build step.
>
> This seems reasonable, I guess; I've applied both.  It seems to me, though,
> that this stuff is in the wrong place.  Perhaps we should go one step
> further and move it to tools/ ?

And then the next step further would be to write a tool in a high level
language to generate the data rather than assemble the binary. Such a
tool would, of course, catch errors like the ones fixed by this patch.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

RE: [PATCH 0/2] Docs/EDID: Fixed and improved EDID documentation

[Christoph Niedermaier]

On Tue, 13 Nov 2018, Jani Nikula wrote:
> On Tue, 06 Nov 2018, Jonathan Corbet <corbet@lwn.net> wrote:
>> On Mon, 5 Nov 2018 09:48:33 +0100
>> Christoph Niedermaier <cniedermaier@dh-electronics.de> wrote:
>>
>>> A problem was found when EDID data sets for displays other than the 
>>> provided samples were generated. The patch series has no effect on 
>>> the provided samples that still match the data used in 
>>> drivers/gpu/drm/drm_edid_load.c.
>>> The provided samples use small values for XOFFSET, XPULSE, YOFFSET 
>>> and YPULSE, where the error doesn't occur. This fix corrects the use 
>>> of that values in case of high values, because the most significant 
>>> bits were treated incorrectly.
>>> 
>>> The previous version made it necessary to first generate an EDID data 
>>> set without correct CRC and then to fix the CRC in a second step. 
>>> This patch series adds the CRC calculation to the makefile in such a 
>>> way that a correct EDID data set is generated in a single build step.
>>
>> This seems reasonable, I guess; I've applied both.  It seems to me, 
>> though, that this stuff is in the wrong place.  Perhaps we should go 
>> one step further and move it to tools/ ?
>
> And then the next step further would be to write a tool in a high level language to generate the data rather than assemble the binary. Such a tool would, of course, catch errors like the ones fixed by this patch.

It would be nice, but for now it works.
Speaking for me such a task has low priority,
because in my opinion this code is not often used and
it is more intended for documentation purpose.

Best regards,
Christoph

Re: [PATCH 0/2] Docs/EDID: Fixed and improved EDID documentation

[Christoph Niedermaier]

Hi Jon,

On Tue, Nov 6, 2018, Jonathan Corbet wrote:
> This seems reasonable, I guess; I've applied both.  It seems to me, though,
> that this stuff is in the wrong place.  Perhaps we should go one step
> further and move it to tools/ ?

I spoke to Carsten and his intention apparently was to merely document
how the somewhat cryptic data structures in drivers/gpu/drm/drm_edid_load.c
were derived. The code needed to be run only once to generate the data and
never again. So it not really is a tool that someone may want to run from time
to time for a particular purpose. So we would propose to leave the EDID directory
where it currently is. However, should you feel that it would better be placed
in the tools directory then you certainly may do that.

Best regards,
Christoph