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

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

[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.

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 <simon.ser@intel.com>
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(-)

diff --git a/lib/igt_edid.c b/lib/igt_edid.c
index 52e66ab2..9d604b13 100644
--- a/lib/igt_edid.c
+++ b/lib/igt_edid.c
@@ -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)
diff --git a/lib/igt_edid.h b/lib/igt_edid.h
index bbcb939a..d0963033 100644
--- a/lib/igt_edid.h
+++ b/lib/igt_edid.h
@@ -52,11 +52,17 @@ struct std_timing {
 
 #define DETAILED_TIMINGS_LEN 4
 
+#define EDID_PT_STEREO (1 << 5)
+#define EDID_PT_INTERLACED (1 << 7)
+
+/* 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 {
-- 
2.21.0

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

[igt-dev] ✗ Fi.CI.BAT: failure for lib/igt_edid: fix detailed pixel timing analog/digital (rev2)

[Patchwork]

== 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@gem_close_race@basic-threads.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2925/fi-skl-6770hq/igt@gem_close_race@basic-threads.html

  * 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@gem_cpu_reloc@basic.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2925/fi-kbl-r/igt@gem_cpu_reloc@basic.html

  * 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@gem_ctx_create@basic-files.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2925/fi-kbl-7500u/igt@gem_ctx_create@basic-files.html
    - fi-skl-lmem:        [PASS][7] -> [INCOMPLETE][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6002/fi-skl-lmem/igt@gem_ctx_create@basic-files.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2925/fi-skl-lmem/igt@gem_ctx_create@basic-files.html

  
#### 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@kms_chamelium@dp-hpd-fast.html

  
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@kms_busy@basic-flip-a.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2925/fi-kbl-7567u/igt@kms_busy@basic-flip-a.html

  
  {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
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] ✗ Fi.CI.BAT: failure for lib/igt_edid: fix detailed pixel timing analog/digital (rev2)

[Arkadiusz Hiler]

On Fri, Apr 26, 2019 at 08:41:45AM +0000, Patchwork wrote:
> == 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 ####

Ccing Martin, as those have nothing to do with the change

>   * 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@gem_close_race@basic-threads.html
>    [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2925/fi-skl-6770hq/igt@gem_close_race@basic-threads.html
> 
>   * 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@gem_cpu_reloc@basic.html
>    [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2925/fi-kbl-r/igt@gem_cpu_reloc@basic.html
> 
>   * 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@gem_ctx_create@basic-files.html
>    [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2925/fi-kbl-7500u/igt@gem_ctx_create@basic-files.html
>     - fi-skl-lmem:        [PASS][7] -> [INCOMPLETE][8]
>    [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6002/fi-skl-lmem/igt@gem_ctx_create@basic-files.html
>    [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2925/fi-skl-lmem/igt@gem_ctx_create@basic-files.html
> 
>   
> #### 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@kms_chamelium@dp-hpd-fast.html
> 
>   
> 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@kms_busy@basic-flip-a.html
>    [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2925/fi-kbl-7567u/igt@kms_busy@basic-flip-a.html
> 
>   
>   {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
> 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

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

[Ville Syrjälä]

On Fri, Apr 26, 2019 at 11:00:18AM +0300, Simon Ser wrote:
> 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 <simon.ser@intel.com>
> 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.

> ---
> 
> 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.

> 
>  lib/igt_edid.c |  2 +-
>  lib/igt_edid.h | 14 ++++++++++----
>  2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/igt_edid.c b/lib/igt_edid.c
> index 52e66ab2..9d604b13 100644
> --- a/lib/igt_edid.c
> +++ b/lib/igt_edid.c
> @@ -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)
> diff --git a/lib/igt_edid.h b/lib/igt_edid.h
> index bbcb939a..d0963033 100644
> --- a/lib/igt_edid.h
> +++ b/lib/igt_edid.h
> @@ -52,11 +52,17 @@ struct std_timing {
>  
>  #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ä <ville.syrjala@linux.intel.com>

> +
> +/* 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 {
> -- 
> 2.21.0

-- 
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 v2] lib/igt_edid: fix detailed pixel timing analog/digital

[Ser, Simon]

Hi,

Thanks for your reply.

On Tue, 2019-04-30 at 20:21 +0300, Ville Syrjälä wrote:
> On Fri, Apr 26, 2019 at 11:00:18AM +0300, Simon Ser wrote:
> > 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 <simon.ser@intel.com>
> > 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.

> > ---
> > 
> > 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.

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

> >  lib/igt_edid.c |  2 +-
> >  lib/igt_edid.h | 14 ++++++++++----
> >  2 files changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/lib/igt_edid.c b/lib/igt_edid.c
> > index 52e66ab2..9d604b13 100644
> > --- a/lib/igt_edid.c
> > +++ b/lib/igt_edid.c
> > @@ -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)
> > diff --git a/lib/igt_edid.h b/lib/igt_edid.h
> > index bbcb939a..d0963033 100644
> > --- a/lib/igt_edid.h
> > +++ b/lib/igt_edid.h
> > @@ -52,11 +52,17 @@ struct std_timing {
> >  
> >  #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.

> Anyways, apart from the style issues the meat of the patch
> looks good to me:
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> > +
> > +/* 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 {
> > -- 
> > 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 v2] lib/igt_edid: fix detailed pixel timing analog/digital

[Ville Syrjälä]

On Mon, May 06, 2019 at 10:51:07AM +0000, Ser, Simon wrote:
> Hi,
> 
> Thanks for your reply.
> 
> On Tue, 2019-04-30 at 20:21 +0300, Ville Syrjälä wrote:
> > On Fri, Apr 26, 2019 at 11:00:18AM +0300, Simon Ser wrote:
> > > 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 <simon.ser@intel.com>
> > > 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.
> 
> > > ---
> > > 
> > > 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.
> 
> Do you mean the whole "changes from v1 to v2" paragraph should be
> included?

Yes. There should be plenty of examples in git log showing people
including the patch changelog in the commit msg.

> 
> > >  lib/igt_edid.c |  2 +-
> > >  lib/igt_edid.h | 14 ++++++++++----
> > >  2 files changed, 11 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/lib/igt_edid.c b/lib/igt_edid.c
> > > index 52e66ab2..9d604b13 100644
> > > --- a/lib/igt_edid.c
> > > +++ b/lib/igt_edid.c
> > > @@ -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)
> > > diff --git a/lib/igt_edid.h b/lib/igt_edid.h
> > > index bbcb939a..d0963033 100644
> > > --- a/lib/igt_edid.h
> > > +++ b/lib/igt_edid.h
> > > @@ -52,11 +52,17 @@ struct std_timing {
> > >  
> > >  #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.

7,5,3,2,1 seems to satisfy both.

> 
> > Anyways, apart from the style issues the meat of the patch
> > looks good to me:
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > > +
> > > +/* 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 {
> > > -- 
> > > 2.21.0

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