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

Ville Syrjälä
2019-04-30T17:21:40Z

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