lib/igt_edid: fix detailed pixel timing analog/digital

== Series Details ==

Series: lib/igt_edid: fix detailed pixel timing analog/digital (rev2)
URL : https://patchwork.freedesktop.org/series/59938/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_6002 -> IGTPW_2925
====================================================

Summary
-------

**FAILURE**

Serious unknown changes coming with IGTPW_2925 absolutely need to be verified manually.

If you think the reported changes have nothing to do with the changes introduced in IGTPW_2925, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI.

External URL: https://patchwork.freedesktop.org/api/1.0/series/59938/revisions/2/mbox/

Possible new issues
-------------------

Here are the unknown changes that may have been introduced in IGTPW_2925:

### IGT changes ###

#### Possible regressions ####

* igt@gem_close_race@basic-threads:
- fi-skl-6770hq: [PASS][1] -> [INCOMPLETE][2]
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6002/fi-skl-6770hq/igt@[email protected]
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2925/fi-skl-6770hq/igt@[email protected]

* igt@gem_cpu_reloc@basic:
- fi-kbl-r: [PASS][3] -> [INCOMPLETE][4]
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6002/fi-kbl-r/igt@[email protected]
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2925/fi-kbl-r/igt@[email protected]

* igt@gem_ctx_create@basic-files:
- fi-kbl-7500u: [PASS][5] -> [INCOMPLETE][6]
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6002/fi-kbl-7500u/igt@[email protected]
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2925/fi-kbl-7500u/igt@[email protected]
- fi-skl-lmem: [PASS][7] -> [INCOMPLETE][8]
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6002/fi-skl-lmem/igt@[email protected]
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2925/fi-skl-lmem/igt@[email protected]

#### Suppressed ####

The following results come from untrusted machines, tests, or statuses. They do not affect the overall result.

* igt@kms_chamelium@dp-hpd-fast:
- {fi-icl-u2}: NOTRUN -> [FAIL][9] +3 similar issues
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2925/fi-icl-u2/igt@[email protected]

Known issues
------------

Here are the changes found in IGTPW_2925 that come from known issues:

### IGT changes ###

#### Possible fixes ####

* igt@kms_busy@basic-flip-a:
- fi-kbl-7567u: [SKIP][10] ([fdo#109271] / [fdo#109278]) -> [PASS][11] +2 similar issues
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6002/fi-kbl-7567u/igt@[email protected]
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2925/fi-kbl-7567u/igt@[email protected]

{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).

[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278

Participating hosts (36 -> 38)
------------------------------

Additional (11): fi-bxt-dsi fi-icl-u2 fi-snb-2520m fi-byt-clapper fi-kbl-x1275 fi-icl-u3 fi-pnv-d510 fi-icl-y fi-byt-n2820 fi-bsw-kefka fi-skl-6700k2
Missing (9): fi-kbl-soraka fi-ilk-m540 fi-skl-gvtdvm fi-ilk-650 fi-ctg-p8600 fi-elk-e7500 fi-bdw-samus fi-skl-6600u fi-snb-2600

Build changes
-------------

* IGT: IGT_4966 -> IGTPW_2925

CI_DRM_6002: 8e38b2c2f198640d840047e426ac009b59977633 @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_2925: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2925/
IGT_4966: a75429544f5721316b04a36551c57573e0c79486 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2925/
_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

The generated EDIDs were wrongly indicating that they support analog sync. Fixup the detailed timings flags to advertise digital sync instead.

Currently the Linux kernel seems to ignore this completely. However I'd prefer to fix this anyway to make sure we don't run into issues if an EDID consumer actually cares about it.

The header definitions for EDID_PT_* values has been re-organized to make it clearer in which situations the flags are relevant.

Signed-off-by Simon Ser <[email protected]>

Fixes a2fd0489c87a4d647c339f98057e6a1550e0e2f5

---

Changes from v1 to v2:
- Fix misleading commit message
- Revert the "misc" → "features" rename
- Re-organize EDID_PT_* definitions to make them clearer

 lib/igt_edid.c |  2 +-
 lib/igt_edid.h | 14 ++++++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)

lib/igt_edid.c +1 -1

index 52e66ab2..9d604b13 100644
@@ -110,7 +110,7 @@ void detailed_timing_set_mode(struct detailed_timing *dt, drmModeModeInfo *mode,
 	pt->width_height_mm_hi = (width_mm & 0xF00) >> 4
 				 | (height_mm & 0xF00) >> 8;
 
-	pt->misc = 0;
+	pt->misc = EDID_PT_SYNC_DIGITAL_SEPARATE;
 	if (mode->flags & DRM_MODE_FLAG_PHSYNC)
 		pt->misc |= EDID_PT_HSYNC_POSITIVE;
 	if (mode->flags & DRM_MODE_FLAG_PVSYNC)

lib/igt_edid.h +0 -0

index bbcb939a..d0963033 100644

lib/igt_edid.h +8 -4

+
+/* Sync type */
+#define EDID_PT_SYNC_ANALOG 0
+#define EDID_PT_SYNC_DIGITAL_COMPOSITE (0b10 << 3)
+#define EDID_PT_SYNC_DIGITAL_SEPARATE (0b11 << 3)
+
+/* Applies to EDID_PT_SYNC_DIGITAL_SEPARATE only */
 #define EDID_PT_HSYNC_POSITIVE (1 << 1)
 #define EDID_PT_VSYNC_POSITIVE (1 << 2)
-#define EDID_PT_SEPARATE_SYNC  (3 << 3)
-#define EDID_PT_STEREO         (1 << 5)
-#define EDID_PT_INTERLACED     (1 << 7)
 
 struct detailed_pixel_timing {
 	uint8_t hactive_lo;

@@ -74,7 +80,7 @@ struct detailed_pixel_timing {
 	uint8_t width_height_mm_hi;
 	uint8_t hborder;
 	uint8_t vborder;
-	uint8_t misc;
+	uint8_t misc; /* EDID_PT_* */
 } __attribute__((packed));
 
 struct detailed_data_string {

Signature · 1 line

2.21.0

Quoted 7 lines · Simon Ser

The generated EDIDs were wrongly indicating that they support analog sync. Fixup the detailed timings flags to advertise digital sync instead.

Currently the Linux kernel seems to ignore this completely. However I'd prefer to fix this anyway to make sure we don't run into issues if an EDID consumer actually cares about it.

Quoted 5 lines

The header definitions for EDID_PT_* values has been re-organized to make it clearer in which situations the flags are relevant.

Signed-off-by Simon Ser <[email protected]>

Fixes a2fd0489c87a4d647c339f98057e6a1550e0e2f5

I think the custom is to use the kernel style for these. The kernel docs have the magic git config incantation listed somewhere.

Quoted 6 lines

---

Changes from v1 to v2:
- Fix misleading commit message
- Revert the "misc" → "features" rename
- Re-organize EDID_PT_* definitions to make them clearer

And this should be included in the commit message proper.

Quoted 23 lines

 lib/igt_edid.c |  2 +-
 lib/igt_edid.h | 14 ++++++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)

lib/igt_edid.c +1 -1

index 52e66ab2..9d604b13 100644
@@ -110,7 +110,7 @@ void detailed_timing_set_mode(struct detailed_timing *dt, drmModeModeInfo *mode,
 	pt->width_height_mm_hi = (width_mm & 0xF00) >> 4
 				 | (height_mm & 0xF00) >> 8;
 
-	pt->misc = 0;
+	pt->misc = EDID_PT_SYNC_DIGITAL_SEPARATE;
 	if (mode->flags & DRM_MODE_FLAG_PHSYNC)
 		pt->misc |= EDID_PT_HSYNC_POSITIVE;
 	if (mode->flags & DRM_MODE_FLAG_PVSYNC)

lib/igt_edid.h +0 -0

index bbcb939a..d0963033 100644
@@ -52,11 +52,17 @@ struct std_timing {
 

lib/igt_edid.h struct std_timing { · line 56

Quoted 4 lines

lib/igt_edid.h +2 -0

 #define DETAILED_TIMINGS_LEN 4
 
+#define EDID_PT_STEREO (1 << 5)
+#define EDID_PT_INTERLACED (1 << 7)

The 5,7,3,1,2 order is making my ocd tingle. Would be nice to stick to a consistent order.

Anyways, apart from the style issues the meat of the patch looks good to me:

Reviewed-by Ville Syrjälä <[email protected]>

Quoted 23 lines

lib/igt_edid.h +8 -4

+
+/* Sync type */
+#define EDID_PT_SYNC_ANALOG 0
+#define EDID_PT_SYNC_DIGITAL_COMPOSITE (0b10 << 3)
+#define EDID_PT_SYNC_DIGITAL_SEPARATE (0b11 << 3)
+
+/* Applies to EDID_PT_SYNC_DIGITAL_SEPARATE only */
 #define EDID_PT_HSYNC_POSITIVE (1 << 1)
 #define EDID_PT_VSYNC_POSITIVE (1 << 2)
-#define EDID_PT_SEPARATE_SYNC  (3 << 3)
-#define EDID_PT_STEREO         (1 << 5)
-#define EDID_PT_INTERLACED     (1 << 7)
 
 struct detailed_pixel_timing {
 	uint8_t hactive_lo;

@@ -74,7 +80,7 @@ struct detailed_pixel_timing {
 	uint8_t width_height_mm_hi;
 	uint8_t hborder;
 	uint8_t vborder;
-	uint8_t misc;
+	uint8_t misc; /* EDID_PT_* */
 } __attribute__((packed));
 

Quoted 3 lines

lib/igt_edid.h +0 -0

 struct detailed_data_string {

Signature · 1 line

2.21.0

Hi,

Thanks for your reply.

Quoted 8 lines · Ville Syrjälä

Quoted 7 lines · Simon Ser

The generated EDIDs were wrongly indicating that they support analog sync. Fixup the detailed timings flags to advertise digital sync instead.

Currently the Linux kernel seems to ignore this completely. However I'd prefer to fix this anyway to make sure we don't run into issues if an EDID consumer actually cares about it.

Quoted 8 lines

Quoted 5 lines

The header definitions for EDID_PT_* values has been re-organized to make it clearer in which situations the flags are relevant.

Signed-off-by Simon Ser <[email protected]>

Fixes a2fd0489c87a4d647c339f98057e6a1550e0e2f5

I think the custom is to use the kernel style for these. The kernel docs have the magic git config incantation listed somewhere.

Ah, right.

Quoted 7 lines

Quoted 6 lines

---

Changes from v1 to v2:
- Fix misleading commit message
- Revert the "misc" → "features" rename
- Re-organize EDID_PT_* definitions to make them clearer

Quoted 1 line

And this should be included in the commit message proper.

Do you mean the whole "changes from v1 to v2" paragraph should be included?

Quoted 24 lines

Quoted 24 lines

 lib/igt_edid.c |  2 +-
 lib/igt_edid.h | 14 ++++++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)

lib/igt_edid.c +1 -1

index 52e66ab2..9d604b13 100644
@@ -110,7 +110,7 @@ void detailed_timing_set_mode(struct detailed_timing *dt, drmModeModeInfo *mode,
 	pt->width_height_mm_hi = (width_mm & 0xF00) >> 4
 				 | (height_mm & 0xF00) >> 8;
 
-	pt->misc = 0;
+	pt->misc = EDID_PT_SYNC_DIGITAL_SEPARATE;
 	if (mode->flags & DRM_MODE_FLAG_PHSYNC)
 		pt->misc |= EDID_PT_HSYNC_POSITIVE;
 	if (mode->flags & DRM_MODE_FLAG_PVSYNC)

lib/igt_edid.h +0 -0

index bbcb939a..d0963033 100644
@@ -52,11 +52,17 @@ struct std_timing {
 

Quoted 7 lines

lib/igt_edid.h struct std_timing { · line 56

Quoted 4 lines

lib/igt_edid.h +2 -0

 #define DETAILED_TIMINGS_LEN 4
 
+#define EDID_PT_STEREO (1 << 5)
+#define EDID_PT_INTERLACED (1 << 7)

The 5,7,3,1,2 order is making my ocd tingle. Would be nice to stick to a consistent order.

The intent is to group them by usage:

- First flags that are always relevant
- Then flags that indicate the sync type
- Then flags that only work for a particular sync type

I think it would make it less clearer to sort them by bitshift.

Quoted 31 lines

Anyways, apart from the style issues the meat of the patch looks good to me:

Reviewed-by Ville Syrjälä <[email protected]>

Quoted 26 lines

lib/igt_edid.h +8 -4

+
+/* Sync type */
+#define EDID_PT_SYNC_ANALOG 0
+#define EDID_PT_SYNC_DIGITAL_COMPOSITE (0b10 << 3)
+#define EDID_PT_SYNC_DIGITAL_SEPARATE (0b11 << 3)
+
+/* Applies to EDID_PT_SYNC_DIGITAL_SEPARATE only */
 #define EDID_PT_HSYNC_POSITIVE (1 << 1)
 #define EDID_PT_VSYNC_POSITIVE (1 << 2)
-#define EDID_PT_SEPARATE_SYNC  (3 << 3)
-#define EDID_PT_STEREO         (1 << 5)
-#define EDID_PT_INTERLACED     (1 << 7)
 
 struct detailed_pixel_timing {
 	uint8_t hactive_lo;

@@ -74,7 +80,7 @@ struct detailed_pixel_timing {
 	uint8_t width_height_mm_hi;
 	uint8_t hborder;
 	uint8_t vborder;
-	uint8_t misc;
+	uint8_t misc; /* EDID_PT_* */
 } __attribute__((packed));
 
 struct detailed_data_string {

Signature · 1 line

2.21.0