[igt-dev] [PATCH i-g-t] lib/igt_edid: fix detailed pixel timing analog/digital
Ser, Simon
simon.ser at intel.com
Thu Apr 25 13:24:44 UTC 2019
On Thu, 2019-04-25 at 16:14 +0300, Ville Syrjälä wrote:
> 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.
Okay.
> > > > 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 at 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.
All right. Since the surrounding code is not aligning, it makes sense
not to align these.
> > > > +#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.
I have a few not-yet-submitted patches which need to represent those 2,
3 and 4-bit values. Additionally, std_timing_aspect does use them.
> 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;
... And I missed it. Since it's burried in the middle of other flags,
I'd rather make it clearer that it's not a bitfield value.
> > > >
> > > > 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.
Wikipedia names it "features".
Will revert.
> > > > } __attribute__((packed));
> > > >
> > > > struct detailed_data_string {
> > > > --
> > > > 2.21.0
> > > >
> > > > _______________________________________________
> > > > igt-dev mailing list
> > > > igt-dev at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
More information about the igt-dev
mailing list