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

[Maxime Ripard]

Hi,

On Sun, Oct 16, 2022 at 07:34:12PM +0200, Mateusz Kwiatkowski wrote:
> 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.

Yeah, I think it's out of scope at least for now. Also, the compositor
should probably be aware of the margins being used to put the VBI data,
so expecting userspace to come up with the mode is probably best?

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

Fixed, thanks
Maxime

Attachment: signature.asc
Description: PGP signature