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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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