Hi, On Wed, Aug 31, 2022 at 03:44:52AM +0200, Mateusz Kwiatkowski wrote: > > +#define NTSC_HFP_DURATION_TYP_NS 1500 > > +#define NTSC_HFP_DURATION_MIN_NS 1270 > > +#define NTSC_HFP_DURATION_MAX_NS 2220 > > You've defined those min/typ/max ranges, but you're not using the "typ" field > for anything other than hslen. Yeah... I've left most of them because it was so hard to find most of them, it's useful at least for documentation purposes. And it's a define so there's pretty much no downside to it as far as the final binary is involved. > The actual "typical" value is thus always the midpoint, which isn't > necessarily the best choice. > > In particular, for the standard 720px wide modes at 13.5 MHz, hsync_start > ends up being 735 for 480i and 734 for 576i, instead of 736 and 732 requested > by BT.601. That's all obviously within tolerances, but the image ends up > noticeably off-center (at least on modern TVs), especially in the 576i case. I'll try to fix that up. > > + htotal = params->line_duration_ns * pixel_clock_hz / NSEC_PER_SEC; > > You're multiplying an unsigned int and an unsigned long - both types are only > required to be 32 bit, so this is likely to overflow. You need to use a cast to > unsigned long long, and then call do_div() for 64-bit division. > > This actually overflowed on me on my Pi running ARM32 kernel, resulting in > negative horizontal porch lengths, and drm_helper_probe_add_cmdline_mode() > taking over the mode generation (badly), and a horrible mess on screen. Indeed, that's bad. > > + vfp = vfp_min + (porches_rem / 2); > > + vbp = porches - vfp; > > Relative position of the vertical sync within the VBI effectively moves the > image up and down. Adding that (porches_rem / 2) moves the image up off center > by that many pixels. I'd keep the VFP always at minimum to keep the image > centered. And you would increase the back porch only then? Maxime
Attachment:
signature.asc
Description: PGP signature
