Re: [PATCH v5 06/22] drm/modes: Add a function to generate analog display modes

Mateusz Kwiatkowski
2022-10-16T17:35:52Z

Hi Maxime & everyone,

Sorry for being inactive in the discussions about this patchset for the last
couple of weeks.

> +const static struct analog_parameters tv_modes_parameters[] = {
> + TV_MODE_PARAMETER(DRM_MODE_ANALOG_NTSC,
> + NTSC_LINES_NUMBER,
> + NTSC_LINE_DURATION_NS,
> + PARAM_RANGE(NTSC_HACT_DURATION_MIN_NS,
> + NTSC_HACT_DURATION_TYP_NS,
> + NTSC_HACT_DURATION_MAX_NS),
> + PARAM_RANGE(NTSC_HFP_DURATION_MIN_NS,
> + NTSC_HFP_DURATION_TYP_NS,
> + NTSC_HFP_DURATION_MAX_NS),
> + PARAM_RANGE(NTSC_HSLEN_DURATION_MIN_NS,
> + NTSC_HSLEN_DURATION_TYP_NS,
> + NTSC_HSLEN_DURATION_MAX_NS),
> + PARAM_RANGE(NTSC_HBP_DURATION_MIN_NS,
> + NTSC_HBP_DURATION_TYP_NS,
> + NTSC_HBP_DURATION_MAX_NS),
> + PARAM_RANGE(NTSC_HBLK_DURATION_MIN_NS,
> + NTSC_HBLK_DURATION_TYP_NS,
> + NTSC_HBLK_DURATION_MAX_NS),
> + 16,
> + PARAM_FIELD(3, 3),
> + PARAM_FIELD(3, 3),
> + PARAM_FIELD(16, 17)),
> + TV_MODE_PARAMETER(DRM_MODE_ANALOG_PAL,
> + PAL_LINES_NUMBER,
> + PAL_LINE_DURATION_NS,
> + PARAM_RANGE(PAL_HACT_DURATION_MIN_NS,
> + PAL_HACT_DURATION_TYP_NS,
> + PAL_HACT_DURATION_MAX_NS),
> + PARAM_RANGE(PAL_HFP_DURATION_MIN_NS,
> + PAL_HFP_DURATION_TYP_NS,
> + PAL_HFP_DURATION_MAX_NS),
> + PARAM_RANGE(PAL_HSLEN_DURATION_MIN_NS,
> + PAL_HSLEN_DURATION_TYP_NS,
> + PAL_HSLEN_DURATION_MAX_NS),
> + PARAM_RANGE(PAL_HBP_DURATION_MIN_NS,
> + PAL_HBP_DURATION_TYP_NS,
> + PAL_HBP_DURATION_MAX_NS),
> + PARAM_RANGE(PAL_HBLK_DURATION_MIN_NS,
> + PAL_HBLK_DURATION_TYP_NS,
> + PAL_HBLK_DURATION_MAX_NS),
> + 12,
> +
> + /*
> + * The front porch is actually 6 short sync
> + * pulses for the even field, and 5 for the
> + * odd field. Each sync takes half a life so
> + * the odd field front porch is shorter by
> + * half a line.
> + *
> + * In progressive, we're supposed to use 6
> + * pulses, so we're fine there
> + */
> + PARAM_FIELD(3, 2),
> +
> + /*
> + * The vsync length is 5 long sync pulses,
> + * each field taking half a line. We're
> + * shorter for both fields by half a line.
> + *
> + * In progressive, we're supposed to use 5
> + * pulses, so we're off by half
> + * a line.
> + *
> + * In interlace, we're now off by half a line
> + * for the even field and one line for the odd
> + * field.
> + */
> + PARAM_FIELD(3, 3),
> +
> + /*
> + * The back porch starts with post-equalizing
> + * pulses, consisting in 5 short sync pulses
> + * for the even field, 4 for the odd field. In
> + * progressive, it's 5 short syncs.
> + *
> + * In progressive, we thus have 2.5 lines,
> + * plus the 0.5 line we were missing
> + * previously, so we should use 3 lines.
> + *
> + * In interlace, the even field is in the
> + * exact same case than progressive. For the
> + * odd field, we should be using 2 lines but
> + * we're one line short, so we'll make up for
> + * it here by using 3.
> + *
> + * The entire blanking area is supposed to
> + * take 25 lines, so we also need to account
> + * for the rest of the blanking area that
> + * can't be in either the front porch or sync
> + * period.
> + */
> + PARAM_FIELD(19, 20)),
> +};

Nit: setting vbp limits like that makes it impossible to use
drm_analog_tv_mode() to generate modes that include the VBI for e.g. emitting
teletext.

This probably doesn't matter, as it can still be created as a custom mode from
userspace, hence I'm mentioning it as a nit.

> + * By convention, NSTC (aka 525/60) systems start with

Typo: s/NSTC/NTSC/

Best regards,
Mateusz Kwiatkowski