Re: [PATCH v2 10/41] drm/modes: Add a function to generate analog display modes

[Mateusz Kwiatkowski]

Hi Maxime,

Wow. That's an enormous amount of effort put into this patch.

But I'm tempted to say that this is actually overengineered quite a bit :D
Considering that there's no way to access all these calculations from user
space, and I can't imagine anybody using anything else than those standard
480i/576i (and maybe 240p/288p) modes at 13.5 MHz any time soon... I'm not
sure if we actually need all this.

But anyway, I'm not the maintainer of this subsystem, so I'm not the one to
decide.

> +enum drm_mode_analog {
> +    DRM_MODE_ANALOG_NTSC,
> +    DRM_MODE_ANALOG_PAL,
> +};

Using "NTSC" and "PAL" to describe the 50Hz and 60Hz analog TV modes is common,
but strictly speaking a misnomer. Those are color encoding systems, and your
patchset fully supports lesser used, but standard encodings for those (e.g.
PAL-M for 60Hz and SECAM for 50Hz). I'd propose switching to some more neutral
naming scheme. Some ideas:

- DRM_MODE_ANALOG_60_HZ / DRM_MODE_ANALOG_50_HZ (after standard refresh rate)
- DRM_MODE_ANALOG_525_LINES / DRM_MODE_ANALOG_625_LINES (after standard line
  count)
- DRM_MODE_ANALOG_JM / DRM_MODE_ANALOG_BDGHIKLN (after corresponding ITU System
  Letter Designations)

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

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

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

Best regards,
Mateusz Kwiatkowski