[igt-dev] [PATCH i-g-t] lib/igt_edid: fix detailed pixel timing analog/digital

[igt-dev] [PATCH i-g-t] lib/igt_edid: fix detailed pixel timing analog/digital

[Simon Ser]

The generated EDIDs were wrongly indicating that they are analog screens. Fixup
the detailed timings flags to advertise a digital screen 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.

Signed-off-by: Simon Ser <simon.ser@intel.com>
Fixes: a2fd0489c87a4d647c339f98057e6a1550e0e2f5
---
 lib/igt_edid.c |  6 +++---
 lib/igt_edid.h | 10 ++++++----
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/lib/igt_edid.c b/lib/igt_edid.c
index 52e66ab2..13a5b53e 100644
--- a/lib/igt_edid.c
+++ b/lib/igt_edid.c
@@ -110,11 +110,11 @@ 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->features = EDID_PT_DIGITAL_SEPARATE;
 	if (mode->flags & DRM_MODE_FLAG_PHSYNC)
-		pt->misc |= EDID_PT_HSYNC_POSITIVE;
+		pt->features |= EDID_PT_HSYNC_POSITIVE;
 	if (mode->flags & DRM_MODE_FLAG_PVSYNC)
-		pt->misc |= EDID_PT_VSYNC_POSITIVE;
+		pt->features |= EDID_PT_VSYNC_POSITIVE;
 }
 
 /**
diff --git a/lib/igt_edid.h b/lib/igt_edid.h
index bbcb939a..860a8531 100644
--- a/lib/igt_edid.h
+++ b/lib/igt_edid.h
@@ -54,9 +54,11 @@ struct std_timing {
 
 #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)
+#define EDID_PT_SEPARATE_SYNC (3 << 3)
+#define EDID_PT_STEREO (1 << 5)
+#define EDID_PT_INTERLACED (1 << 7)
+#define EDID_PT_DIGITAL_COMPOSITE (0b10 << 3)
+#define EDID_PT_DIGITAL_SEPARATE (0b11 << 3)
 
 struct detailed_pixel_timing {
 	uint8_t hactive_lo;
@@ -74,7 +76,7 @@ struct detailed_pixel_timing {
 	uint8_t width_height_mm_hi;
 	uint8_t hborder;
 	uint8_t vborder;
-	uint8_t misc;
+	uint8_t features;
 } __attribute__((packed));
 
 struct detailed_data_string {
-- 
2.21.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] lib/igt_edid: fix detailed pixel timing analog/digital

[Ville Syrjälä]

On Thu, Apr 25, 2019 at 03:12:00PM +0300, Simon Ser wrote:
> The generated EDIDs were wrongly indicating that they are analog screens. Fixup
> the detailed timings flags to advertise a digital screen instead.

This commit message is a bit confusing. The patch only touching the sync
signal stuff, which doesn't indicate whether the video signal itself is
analog or digital.

> 
> 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.
> 
> Signed-off-by: Simon Ser <simon.ser@intel.com>
> Fixes: a2fd0489c87a4d647c339f98057e6a1550e0e2f5
> ---
>  lib/igt_edid.c |  6 +++---
>  lib/igt_edid.h | 10 ++++++----
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/igt_edid.c b/lib/igt_edid.c
> index 52e66ab2..13a5b53e 100644
> --- a/lib/igt_edid.c
> +++ b/lib/igt_edid.c
> @@ -110,11 +110,11 @@ 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->features = EDID_PT_DIGITAL_SEPARATE;
>  	if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> -		pt->misc |= EDID_PT_HSYNC_POSITIVE;
> +		pt->features |= EDID_PT_HSYNC_POSITIVE;
>  	if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> -		pt->misc |= EDID_PT_VSYNC_POSITIVE;
> +		pt->features |= EDID_PT_VSYNC_POSITIVE;
>  }
>  
>  /**
> diff --git a/lib/igt_edid.h b/lib/igt_edid.h
> index bbcb939a..860a8531 100644
> --- a/lib/igt_edid.h
> +++ b/lib/igt_edid.h
> @@ -54,9 +54,11 @@ struct std_timing {
>  
>  #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)
> +#define EDID_PT_SEPARATE_SYNC (3 << 3)
> +#define EDID_PT_STEREO (1 << 5)
> +#define EDID_PT_INTERLACED (1 << 7)

Why the whitespace changes?

> +#define EDID_PT_DIGITAL_COMPOSITE (0b10 << 3)
> +#define EDID_PT_DIGITAL_SEPARATE (0b11 << 3)

Binary literals look strange to me. Also inconsistent with
the rest of the bit definitions, so I would not use them at
this time.

>  
>  struct detailed_pixel_timing {
>  	uint8_t hactive_lo;
> @@ -74,7 +76,7 @@ struct detailed_pixel_timing {
>  	uint8_t width_height_mm_hi;
>  	uint8_t hborder;
>  	uint8_t vborder;
> -	uint8_t misc;
> +	uint8_t features;

Why rename it? It no longer matches drm_edid.c.

>  } __attribute__((packed));
>  
>  struct detailed_data_string {
> -- 
> 2.21.0
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] lib/igt_edid: fix detailed pixel timing analog/digital

[Ser, Simon]

On Thu, 2019-04-25 at 15:31 +0300, Ville Syrjälä wrote:
> On Thu, Apr 25, 2019 at 03:12:00PM +0300, Simon Ser wrote:
> > The generated EDIDs were wrongly indicating that they are analog screens. Fixup
> > the detailed timings flags to advertise a digital screen instead.
> 
> This commit message is a bit confusing. The patch only touching the sync
> signal stuff, which doesn't indicate whether the video signal itself is
> analog or digital.

That's fair. A more accurate description would be:

  The generated EDIDs were containing analog pixel timings. Fix them up
  so that they appear as digital pixel timings.

> > 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.
> > 
> > Signed-off-by: Simon Ser <simon.ser@intel.com>
> > Fixes: a2fd0489c87a4d647c339f98057e6a1550e0e2f5
> > ---
> >  lib/igt_edid.c |  6 +++---
> >  lib/igt_edid.h | 10 ++++++----
> >  2 files changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/lib/igt_edid.c b/lib/igt_edid.c
> > index 52e66ab2..13a5b53e 100644
> > --- a/lib/igt_edid.c
> > +++ b/lib/igt_edid.c
> > @@ -110,11 +110,11 @@ 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->features = EDID_PT_DIGITAL_SEPARATE;
> >  	if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> > -		pt->misc |= EDID_PT_HSYNC_POSITIVE;
> > +		pt->features |= EDID_PT_HSYNC_POSITIVE;
> >  	if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> > -		pt->misc |= EDID_PT_VSYNC_POSITIVE;
> > +		pt->features |= EDID_PT_VSYNC_POSITIVE;
> >  }
> >  
> >  /**
> > diff --git a/lib/igt_edid.h b/lib/igt_edid.h
> > index bbcb939a..860a8531 100644
> > --- a/lib/igt_edid.h
> > +++ b/lib/igt_edid.h
> > @@ -54,9 +54,11 @@ struct std_timing {
> >  
> >  #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)
> > +#define EDID_PT_SEPARATE_SYNC (3 << 3)
> > +#define EDID_PT_STEREO (1 << 5)
> > +#define EDID_PT_INTERLACED (1 << 7)
> 
> Why the whitespace changes?

I don't align those. I could find a lot of different styles for
define/enum alignment in the current codebase, so I just picked the one
I'm used to.

> > +#define EDID_PT_DIGITAL_COMPOSITE (0b10 << 3)
> > +#define EDID_PT_DIGITAL_SEPARATE (0b11 << 3)
> 
> Binary literals look strange to me. Also inconsistent with
> the rest of the bit definitions, so I would not use them at
> this time.

These are used on purpose, since they aren't bitfields. EDIDs sometimes
have two or three bits values, and I think binary literals are a lot
more readable for those than hex.

> >  
> >  struct detailed_pixel_timing {
> >  	uint8_t hactive_lo;
> > @@ -74,7 +76,7 @@ struct detailed_pixel_timing {
> >  	uint8_t width_height_mm_hi;
> >  	uint8_t hborder;
> >  	uint8_t vborder;
> > -	uint8_t misc;
> > +	uint8_t features;
> 
> Why rename it? It no longer matches drm_edid.c.

I don't think sticking to drm_edid.c is a goal per se. Because IGT
needs to generate EDIDs, it will diverge from the kernel's code anyway.

The reason for the change is that "features" is a better name than
"misc" for this field. Not that it matters a lot, I can change it back.

> >  } __attribute__((packed));
> >  
> >  struct detailed_data_string {
> > -- 
> > 2.21.0
> > 
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.BAT: success for lib/igt_edid: fix detailed pixel timing analog/digital

[Patchwork]

== Series Details ==

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

== Summary ==

CI Bug Log - changes from IGT_4965 -> IGTPW_2918
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Suppressed ####

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

  * igt@gem_workarounds@basic-read:
    - {fi-cml-u}:         [SKIP][1] ([fdo#109271]) -> [SKIP][2] +72 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/fi-cml-u/igt@gem_workarounds@basic-read.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/fi-cml-u/igt@gem_workarounds@basic-read.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-blb-e6850:       [PASS][3] -> [INCOMPLETE][4] ([fdo#107718])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/fi-blb-e6850/igt@gem_exec_suspend@basic-s3.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/fi-blb-e6850/igt@gem_exec_suspend@basic-s3.html

  * igt@i915_selftest@live_hangcheck:
    - fi-skl-iommu:       [PASS][5] -> [INCOMPLETE][6] ([fdo#108602] / [fdo#108744])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/fi-skl-iommu/igt@i915_selftest@live_hangcheck.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/fi-skl-iommu/igt@i915_selftest@live_hangcheck.html

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-kbl-7567u:       [PASS][7] -> [DMESG-WARN][8] ([fdo#108566])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/fi-kbl-7567u/igt@kms_chamelium@common-hpd-after-suspend.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/fi-kbl-7567u/igt@kms_chamelium@common-hpd-after-suspend.html

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-kbl-7500u:       [DMESG-WARN][9] ([fdo#105128] / [fdo#107139]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/fi-kbl-7500u/igt@gem_exec_suspend@basic-s4-devices.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/fi-kbl-7500u/igt@gem_exec_suspend@basic-s4-devices.html

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

  [fdo#105128]: https://bugs.freedesktop.org/show_bug.cgi?id=105128
  [fdo#107139]: https://bugs.freedesktop.org/show_bug.cgi?id=107139
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108602]: https://bugs.freedesktop.org/show_bug.cgi?id=108602
  [fdo#108744]: https://bugs.freedesktop.org/show_bug.cgi?id=108744
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#110514]: https://bugs.freedesktop.org/show_bug.cgi?id=110514


Participating hosts (34 -> 34)
------------------------------

  Additional (11): fi-bxt-dsi fi-hsw-peppy fi-skl-6770hq fi-skl-6260u fi-whl-u fi-kbl-x1275 fi-bxt-j4205 fi-icl-y fi-byt-n2820 fi-bsw-kefka fi-kbl-r 
  Missing    (11): fi-kbl-soraka fi-ilk-m540 fi-skl-guc fi-byt-squawks fi-bsw-cyan fi-snb-2520m fi-ctg-p8600 fi-gdg-551 fi-bdw-samus fi-byt-clapper fi-skl-6700k2 


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

  * IGT: IGT_4965 -> IGTPW_2918

  CI_DRM_5999: bde6790dd103ba3c4c127bbc061b90ea3f406762 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2918: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/
  IGT_4965: 640684403327250392107f027e5cd2fb2c76f7d9 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] lib/igt_edid: fix detailed pixel timing analog/digital

[Ville Syrjälä]

On Thu, Apr 25, 2019 at 12:55:50PM +0000, Ser, Simon wrote:
> On Thu, 2019-04-25 at 15:31 +0300, Ville Syrjälä wrote:
> > On Thu, Apr 25, 2019 at 03:12:00PM +0300, Simon Ser wrote:
> > > The generated EDIDs were wrongly indicating that they are analog screens. Fixup
> > > the detailed timings flags to advertise a digital screen instead.
> > 
> > This commit message is a bit confusing. The patch only touching the sync
> > signal stuff, which doesn't indicate whether the video signal itself is
> > analog or digital.
> 
> That's fair. A more accurate description would be:
> 
>   The generated EDIDs were containing analog pixel timings. Fix them up
>   so that they appear as digital pixel timings.

I think the original with basically s/screen/sync/ would be more clear.

> 
> > > 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.
> > > 
> > > Signed-off-by: Simon Ser <simon.ser@intel.com>
> > > Fixes: a2fd0489c87a4d647c339f98057e6a1550e0e2f5
> > > ---
> > >  lib/igt_edid.c |  6 +++---
> > >  lib/igt_edid.h | 10 ++++++----
> > >  2 files changed, 9 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/lib/igt_edid.c b/lib/igt_edid.c
> > > index 52e66ab2..13a5b53e 100644
> > > --- a/lib/igt_edid.c
> > > +++ b/lib/igt_edid.c
> > > @@ -110,11 +110,11 @@ 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->features = EDID_PT_DIGITAL_SEPARATE;
> > >  	if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> > > -		pt->misc |= EDID_PT_HSYNC_POSITIVE;
> > > +		pt->features |= EDID_PT_HSYNC_POSITIVE;
> > >  	if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> > > -		pt->misc |= EDID_PT_VSYNC_POSITIVE;
> > > +		pt->features |= EDID_PT_VSYNC_POSITIVE;
> > >  }
> > >  
> > >  /**
> > > diff --git a/lib/igt_edid.h b/lib/igt_edid.h
> > > index bbcb939a..860a8531 100644
> > > --- a/lib/igt_edid.h
> > > +++ b/lib/igt_edid.h
> > > @@ -54,9 +54,11 @@ struct std_timing {
> > >  
> > >  #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)
> > > +#define EDID_PT_SEPARATE_SYNC (3 << 3)
> > > +#define EDID_PT_STEREO (1 << 5)
> > > +#define EDID_PT_INTERLACED (1 << 7)
> > 
> > Why the whitespace changes?
> 
> I don't align those. I could find a lot of different styles for
> define/enum alignment in the current codebase, so I just picked the one
> I'm used to.

The non-controversial approach is to stick to the style used in the
surrounding code. That way things stay looking mostly consistent.
If everyone adds their own favorite style to every change we'll end
up in a mess.

> 
> > > +#define EDID_PT_DIGITAL_COMPOSITE (0b10 << 3)
> > > +#define EDID_PT_DIGITAL_SEPARATE (0b11 << 3)
> > 
> > Binary literals look strange to me. Also inconsistent with
> > the rest of the bit definitions, so I would not use them at
> > this time.
> 
> These are used on purpose, since they aren't bitfields. EDIDs sometimes
> have two or three bits values, and I think binary literals are a lot
> more readable for those than hex.

The existing defintions don't use them so the end result looks
a bit strange. I wouldn't mind so much if we were consistent
in their use throughout the file.

Hmm. Looks like we already have the digital separate sync definiton
under the name EDID_PT_SEPARATE_SYNC. So now we end up with a
duplicate name for that one. So looks like the minimal fix
would have been just:

- pt->misc = 0;
+ pt->misc = EDID_PT_SEPARATE_SYNC;

> 
> > >  
> > >  struct detailed_pixel_timing {
> > >  	uint8_t hactive_lo;
> > > @@ -74,7 +76,7 @@ struct detailed_pixel_timing {
> > >  	uint8_t width_height_mm_hi;
> > >  	uint8_t hborder;
> > >  	uint8_t vborder;
> > > -	uint8_t misc;
> > > +	uint8_t features;
> > 
> > Why rename it? It no longer matches drm_edid.c.
> 
> I don't think sticking to drm_edid.c is a goal per se. Because IGT
> needs to generate EDIDs, it will diverge from the kernel's code anyway.
> 
> The reason for the change is that "features" is a better name than
> "misc" for this field. Not that it matters a lot, I can change it back.

The spec doesn't give it a name AFAICS. So sticking to existing
conventions means less cognitive load trying to figure out what
is what.

> 
> > >  } __attribute__((packed));
> > >  
> > >  struct detailed_data_string {
> > > -- 
> > > 2.21.0
> > > 
> > > _______________________________________________
> > > igt-dev mailing list
> > > igt-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] lib/igt_edid: fix detailed pixel timing analog/digital

[Ser, Simon]

On Thu, 2019-04-25 at 16:14 +0300, Ville Syrjälä wrote:
> On Thu, Apr 25, 2019 at 12:55:50PM +0000, Ser, Simon wrote:
> > On Thu, 2019-04-25 at 15:31 +0300, Ville Syrjälä wrote:
> > > On Thu, Apr 25, 2019 at 03:12:00PM +0300, Simon Ser wrote:
> > > > The generated EDIDs were wrongly indicating that they are
> > > > analog screens. Fixup
> > > > the detailed timings flags to advertise a digital screen
> > > > instead.
> > > 
> > > This commit message is a bit confusing. The patch only touching
> > > the sync
> > > signal stuff, which doesn't indicate whether the video signal
> > > itself is
> > > analog or digital.
> > 
> > That's fair. A more accurate description would be:
> > 
> >   The generated EDIDs were containing analog pixel timings. Fix
> > them up
> >   so that they appear as digital pixel timings.
> 
> I think the original with basically s/screen/sync/ would be more
> clear.

Okay.

> > > > 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.
> > > > 
> > > > Signed-off-by: Simon Ser <simon.ser@intel.com>
> > > > Fixes: a2fd0489c87a4d647c339f98057e6a1550e0e2f5
> > > > ---
> > > >  lib/igt_edid.c |  6 +++---
> > > >  lib/igt_edid.h | 10 ++++++----
> > > >  2 files changed, 9 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/lib/igt_edid.c b/lib/igt_edid.c
> > > > index 52e66ab2..13a5b53e 100644
> > > > --- a/lib/igt_edid.c
> > > > +++ b/lib/igt_edid.c
> > > > @@ -110,11 +110,11 @@ 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->features = EDID_PT_DIGITAL_SEPARATE;
> > > >  	if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> > > > -		pt->misc |= EDID_PT_HSYNC_POSITIVE;
> > > > +		pt->features |= EDID_PT_HSYNC_POSITIVE;
> > > >  	if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> > > > -		pt->misc |= EDID_PT_VSYNC_POSITIVE;
> > > > +		pt->features |= EDID_PT_VSYNC_POSITIVE;
> > > >  }
> > > >  
> > > >  /**
> > > > diff --git a/lib/igt_edid.h b/lib/igt_edid.h
> > > > index bbcb939a..860a8531 100644
> > > > --- a/lib/igt_edid.h
> > > > +++ b/lib/igt_edid.h
> > > > @@ -54,9 +54,11 @@ struct std_timing {
> > > >  
> > > >  #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)
> > > > +#define EDID_PT_SEPARATE_SYNC (3 << 3)
> > > > +#define EDID_PT_STEREO (1 << 5)
> > > > +#define EDID_PT_INTERLACED (1 << 7)
> > > 
> > > Why the whitespace changes?
> > 
> > I don't align those. I could find a lot of different styles for
> > define/enum alignment in the current codebase, so I just picked the
> > one
> > I'm used to.
> 
> The non-controversial approach is to stick to the style used in the
> surrounding code. That way things stay looking mostly consistent.
> If everyone adds their own favorite style to every change we'll end
> up in a mess.

All right. Since the surrounding code is not aligning, it makes sense
not to align these.

> > > > +#define EDID_PT_DIGITAL_COMPOSITE (0b10 << 3)
> > > > +#define EDID_PT_DIGITAL_SEPARATE (0b11 << 3)
> > > 
> > > Binary literals look strange to me. Also inconsistent with
> > > the rest of the bit definitions, so I would not use them at
> > > this time.
> > 
> > These are used on purpose, since they aren't bitfields. EDIDs
> > sometimes
> > have two or three bits values, and I think binary literals are a
> > lot
> > more readable for those than hex.
> 
> The existing defintions don't use them so the end result looks
> a bit strange. I wouldn't mind so much if we were consistent
> in their use throughout the file.

I have a few not-yet-submitted patches which need to represent those 2,
3 and 4-bit values. Additionally, std_timing_aspect does use them.

> Hmm. Looks like we already have the digital separate sync definiton
> under the name EDID_PT_SEPARATE_SYNC. So now we end up with a
> duplicate name for that one. So looks like the minimal fix
> would have been just:
> 
> - pt->misc = 0;
> + pt->misc = EDID_PT_SEPARATE_SYNC;

... And I missed it. Since it's burried in the middle of other flags,
I'd rather make it clearer that it's not a bitfield value.

> > > >  
> > > >  struct detailed_pixel_timing {
> > > >  	uint8_t hactive_lo;
> > > > @@ -74,7 +76,7 @@ struct detailed_pixel_timing {
> > > >  	uint8_t width_height_mm_hi;
> > > >  	uint8_t hborder;
> > > >  	uint8_t vborder;
> > > > -	uint8_t misc;
> > > > +	uint8_t features;
> > > 
> > > Why rename it? It no longer matches drm_edid.c.
> > 
> > I don't think sticking to drm_edid.c is a goal per se. Because IGT
> > needs to generate EDIDs, it will diverge from the kernel's code
> > anyway.
> > 
> > The reason for the change is that "features" is a better name than
> > "misc" for this field. Not that it matters a lot, I can change it
> > back.
> 
> The spec doesn't give it a name AFAICS. So sticking to existing
> conventions means less cognitive load trying to figure out what
> is what.

Wikipedia names it "features".

Will revert.

> > > >  } __attribute__((packed));
> > > >  
> > > >  struct detailed_data_string {
> > > > -- 
> > > > 2.21.0
> > > > 
> > > > _______________________________________________
> > > > igt-dev mailing list
> > > > igt-dev@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.IGT: success for lib/igt_edid: fix detailed pixel timing analog/digital

[Patchwork]

== Series Details ==

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

== Summary ==

CI Bug Log - changes from IGT_4965_full -> IGTPW_2918_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_tiled_swapping@non-threaded:
    - shard-iclb:         [PASS][1] -> [INCOMPLETE][2] ([fdo#107713] / [fdo#108686])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/shard-iclb5/igt@gem_tiled_swapping@non-threaded.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/shard-iclb2/igt@gem_tiled_swapping@non-threaded.html
    - shard-apl:          [PASS][3] -> [INCOMPLETE][4] ([fdo#103927])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/shard-apl1/igt@gem_tiled_swapping@non-threaded.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/shard-apl5/igt@gem_tiled_swapping@non-threaded.html
    - shard-snb:          [PASS][5] -> [FAIL][6] ([fdo#108686])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/shard-snb1/igt@gem_tiled_swapping@non-threaded.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/shard-snb4/igt@gem_tiled_swapping@non-threaded.html

  * igt@gem_workarounds@suspend-resume-fd:
    - shard-kbl:          [PASS][7] -> [DMESG-WARN][8] ([fdo#108566]) +1 similar issue
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/shard-kbl6/igt@gem_workarounds@suspend-resume-fd.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/shard-kbl5/igt@gem_workarounds@suspend-resume-fd.html

  * igt@i915_pm_rpm@i2c:
    - shard-iclb:         [PASS][9] -> [DMESG-WARN][10] ([fdo#109982])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/shard-iclb4/igt@i915_pm_rpm@i2c.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/shard-iclb2/igt@i915_pm_rpm@i2c.html

  * igt@i915_suspend@debugfs-reader:
    - shard-apl:          [PASS][11] -> [DMESG-WARN][12] ([fdo#108566]) +5 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/shard-apl7/igt@i915_suspend@debugfs-reader.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/shard-apl6/igt@i915_suspend@debugfs-reader.html

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-apl:          [PASS][13] -> [FAIL][14] ([fdo#102887] / [fdo#105363])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/shard-apl7/igt@kms_flip@flip-vs-expired-vblank.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/shard-apl3/igt@kms_flip@flip-vs-expired-vblank.html

  * igt@kms_flip@flip-vs-suspend:
    - shard-snb:          [PASS][15] -> [INCOMPLETE][16] ([fdo#105411])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/shard-snb4/igt@kms_flip@flip-vs-suspend.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/shard-snb1/igt@kms_flip@flip-vs-suspend.html

  * igt@kms_flip_tiling@flip-to-x-tiled:
    - shard-iclb:         [PASS][17] -> [FAIL][18] ([fdo#108134])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/shard-iclb6/igt@kms_flip_tiling@flip-to-x-tiled.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/shard-iclb1/igt@kms_flip_tiling@flip-to-x-tiled.html

  * igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-indfb-plflip-blt:
    - shard-glk:          [PASS][19] -> [FAIL][20] ([fdo#103167])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/shard-glk2/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-indfb-plflip-blt.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/shard-glk8/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-indfb-plflip-blt.html

  * igt@kms_frontbuffer_tracking@fbc-stridechange:
    - shard-iclb:         [PASS][21] -> [FAIL][22] ([fdo#103167]) +4 similar issues
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/shard-iclb6/igt@kms_frontbuffer_tracking@fbc-stridechange.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/shard-iclb8/igt@kms_frontbuffer_tracking@fbc-stridechange.html

  * igt@kms_plane@pixel-format-pipe-a-planes-source-clamping:
    - shard-glk:          [PASS][23] -> [SKIP][24] ([fdo#109271])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/shard-glk9/igt@kms_plane@pixel-format-pipe-a-planes-source-clamping.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/shard-glk8/igt@kms_plane@pixel-format-pipe-a-planes-source-clamping.html

  * igt@kms_plane_lowres@pipe-a-tiling-x:
    - shard-iclb:         [PASS][25] -> [FAIL][26] ([fdo#103166])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/shard-iclb2/igt@kms_plane_lowres@pipe-a-tiling-x.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/shard-iclb7/igt@kms_plane_lowres@pipe-a-tiling-x.html

  * igt@kms_psr@psr2_no_drrs:
    - shard-iclb:         [PASS][27] -> [SKIP][28] ([fdo#109441]) +3 similar issues
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/shard-iclb2/igt@kms_psr@psr2_no_drrs.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/shard-iclb1/igt@kms_psr@psr2_no_drrs.html

  
#### Possible fixes ####

  * igt@gem_ctx_isolation@vcs1-s3:
    - shard-kbl:          [DMESG-WARN][29] ([fdo#108566]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/shard-kbl6/igt@gem_ctx_isolation@vcs1-s3.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/shard-kbl3/igt@gem_ctx_isolation@vcs1-s3.html

  * igt@kms_cursor_crc@cursor-64x64-suspend:
    - shard-kbl:          [FAIL][31] ([fdo#103232]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/shard-kbl5/igt@kms_cursor_crc@cursor-64x64-suspend.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/shard-kbl2/igt@kms_cursor_crc@cursor-64x64-suspend.html

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-apl:          [DMESG-WARN][33] ([fdo#108566]) -> [PASS][34] +1 similar issue
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/shard-apl4/igt@kms_flip@flip-vs-suspend-interruptible.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/shard-apl3/igt@kms_flip@flip-vs-suspend-interruptible.html

  * igt@kms_frontbuffer_tracking@fbcpsr-rgb565-draw-render:
    - shard-iclb:         [FAIL][35] ([fdo#103167]) -> [PASS][36] +1 similar issue
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/shard-iclb8/igt@kms_frontbuffer_tracking@fbcpsr-rgb565-draw-render.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/shard-iclb4/igt@kms_frontbuffer_tracking@fbcpsr-rgb565-draw-render.html

  * igt@kms_plane@pixel-format-pipe-c-planes-source-clamping:
    - shard-glk:          [SKIP][37] ([fdo#109271]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/shard-glk3/igt@kms_plane@pixel-format-pipe-c-planes-source-clamping.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/shard-glk9/igt@kms_plane@pixel-format-pipe-c-planes-source-clamping.html

  * igt@kms_plane_scaling@pipe-a-scaler-with-pixel-format:
    - shard-glk:          [SKIP][39] ([fdo#109271] / [fdo#109278]) -> [PASS][40] +1 similar issue
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/shard-glk5/igt@kms_plane_scaling@pipe-a-scaler-with-pixel-format.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/shard-glk9/igt@kms_plane_scaling@pipe-a-scaler-with-pixel-format.html

  * igt@kms_psr@psr2_primary_mmap_gtt:
    - shard-iclb:         [SKIP][41] ([fdo#109441]) -> [PASS][42] +1 similar issue
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/shard-iclb8/igt@kms_psr@psr2_primary_mmap_gtt.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/shard-iclb2/igt@kms_psr@psr2_primary_mmap_gtt.html

  * igt@kms_rotation_crc@multiplane-rotation-cropping-top:
    - shard-kbl:          [FAIL][43] ([fdo#109016]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/shard-kbl4/igt@kms_rotation_crc@multiplane-rotation-cropping-top.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/shard-kbl3/igt@kms_rotation_crc@multiplane-rotation-cropping-top.html

  * igt@kms_sysfs_edid_timing:
    - shard-iclb:         [FAIL][45] ([fdo#100047]) -> [PASS][46]
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_4965/shard-iclb2/igt@kms_sysfs_edid_timing.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/shard-iclb7/igt@kms_sysfs_edid_timing.html

  
  [fdo#100047]: https://bugs.freedesktop.org/show_bug.cgi?id=100047
  [fdo#102887]: https://bugs.freedesktop.org/show_bug.cgi?id=102887
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#108134]: https://bugs.freedesktop.org/show_bug.cgi?id=108134
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108686]: https://bugs.freedesktop.org/show_bug.cgi?id=108686
  [fdo#109016]: https://bugs.freedesktop.org/show_bug.cgi?id=109016
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109982]: https://bugs.freedesktop.org/show_bug.cgi?id=109982


Participating hosts (7 -> 6)
------------------------------

  Missing    (1): shard-skl 


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

  * IGT: IGT_4965 -> IGTPW_2918

  CI_DRM_5999: bde6790dd103ba3c4c127bbc061b90ea3f406762 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2918: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/
  IGT_4965: 640684403327250392107f027e5cd2fb2c76f7d9 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2918/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev