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