On Thu, 13 Feb 2025, Egor Vorontsov <sdoregor@xxxxxxxx> wrote: > On Wed, 2025-02-12 at 11:35 +0200, Jani Nikula wrote: >> > + /* TODO: support video-optimized refresh rate */ >> > + if (timings->flags & (1 << 4)) >> > + return NULL; >> >> Mmh. I'm not sure I'd go this far. The bit indicates *two* timings, one >> for which the below *is* correct, and another additional one with >> vrefresh * (1000/1001). >> >> We could just add a drm_dbg_kms(dev, "<message>") here about missing >> fractional refresh rate, and proceed with the one non-fractional rate? >> Or just have the TODO comment with no checks. > I'll go with the former, for now. > >> Either way, >> Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx> > Thank you. ... But! > >> Are you up for the follow-ups too? And since you've got the hang of it, >> maybe fix struct displayid_formula_timings_9 hactive/vactive to __be16 >> as well? > ... at this moment I realised that both the specs and the legacy code > actually indicate it's indeed *little*-endian shorts! > I.e. `x[0] | x[1] << 8' -- that's LSB-first. > > Also, virtually no code in `drm_edid.c' uses big-endian. Yes, I *obviously* meant __be16 and be16_to_cpu(). ;D Good catch, and sorry about that, quite the *facepalm* for me. > Thus, I'm fixing both my code and `displayid_detailed_timings_1' (I > suppose you meant this struct instead) to use __le16. Indeed. Thanks, Jani. -- Jani Nikula, Intel
