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

[Maxime Ripard]

Hi,

On Sun, Sep 11, 2022 at 06:48:50AM +0200, Mateusz Kwiatkowski wrote:
> >> Those extra vbp lines will be treated as a black bar at the top of the frame,
> >> and extra vfp lines will be at the bottom of the frame.
> >>
> >> However if someone specifies e.g. 720x604, there's nothing more you could
> >> remove from vfp, so your only option is to reduce vbp compared to the standard
> >> mode, so you'll end up with (vfp==4, vsync==6, vbp==11). The image will not be
> >> centered, the topmost lines will get cropped out, but that's the best we can do
> >> and if someone is requesting such resolution, they most likely want to actually
> >> access the VBI to e.g. emit teletext.
> >>
> >> Your current code always starts at (vfp==5 or 6, vsync=6, vbp==6) and then
> >> increases both vfp and vbp proportionately. This puts vsync dead center in the
> >> VBI, which is not how it's supposed to be - and that in turn causes the image
> >> to be significantly shifted upwards.
> >>
> >> I hope this makes more sense to you now.
> >
> > I'm really struggling with this, so thanks for explaining this further
> > (and patiently ;))
> >
> > If I get this right, what you'd like to change is this part of the
> > calculus (simplified a bit, and using PAL, 576i):
> >
> >   vfp_min = params->vfp_lines.even + params->vfp_lines.odd; // 5
> >   vbp_min = params->vbp_lines.even + params->vbp_lines.odd; // 6
> >   vslen = params->vslen_lines.even + params->vslen_lines.odd; // 6
> >
> >   porches = params->num_lines - vactive - vslen; // 43
> >   porches_rem = porches - vfp_min - vbp_min; // 32
> >
> >   vfp = vfp_min + (porches_rem / 2); // 21
> >   vbp = porches - vfp; // 22
> >
> > Which is indeed having sync centered.
> >
> > I initially changed it to:
> >
> >   vfp = vfp_min; // 6
> >   vbp = num_lines - vactive - vslen - vfp; // 38
> >
> > Which is close enough for 576i, but at 480i/50Hz would end up with 134,
> > so still fairly far off.
> >
> > I guess your suggestion would be along the line of:
> >
> >   vfp_min = params->vfp_lines.even + params->vfp_lines.odd; // 5
> >   vbp_min = params->vbp_lines.even + params->vbp_lines.odd; // 38
> >   vslen = params->vslen_lines.even + params->vslen_lines.odd; // 6
> >
> >   porches = params->num_lines - vactive - vslen; // 0
> >   porches_rem = porches - vfp_min - vbp_min; // 0
> >
> >   vfp = vfp_min + (porches_rem / 2); // 5
> >   vbp = porches - vfp; // 38
> >
> > Which is still close enough for 576i, but for 480i would end up with:
> >
> >   porches = params->num_lines - vactive - vslen; // 139
> >   porches_rem = porches - vfp_min - vbp_min; // 96
> >
> >   vfp = vfp_min + (porches_rem / 2); // 53
> >   vbp = porches - vfp; // 86
> >
> > Right?
> 
> Yes. And if that's supposed to mean 480i in 50 Hz "PAL" mode, that's also
> "close enough" to the values I suggested above.
> 
> If you substitute values for true 60 Hz "NTSC" 480i, you should also get values
> that are "close enough" to the official spec.
> 
> The only thing I'd conceptually change is that the 38 lines is not really
> "vbp_min". It's more like "vbp_typ". As I mentioned above, we may want to lower
> this value if someone wants more active lines than the official 486/576.

porches_rem is an int, so if vactive > (num_lines + vslen + vfp_min +
vbp_min), porches_rem is going to be negative and we'll remove equally
between vfp and vbp to match what's been asked

So I believe this should work fine?

Maxime

Attachment: signature.asc
Description: PGP signature