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

Ser, Simon
2019-04-25T12:55:53Z

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.

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

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

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

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