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