Hi,

On Wed, Sep 07, 2022 at 11:31:21PM +0200, Mateusz Kwiatkowski wrote:
> W dniu 7.09.2022 o 16:34, Maxime Ripard pisze:
> > On Mon, Sep 05, 2022 at 06:44:42PM +0200, Mateusz Kwiatkowski wrote:
> >> Hi Maxime,
> >>
> >> W dniu 5.09.2022 o 15:37, Maxime Ripard pisze:
> >>>>> +    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?
> >>
> >> Well, increasing vbp only gives a centered image with the default 480i/576i
> >> resolutions. However, only ever changing vbp will cause the image to be always
> >> at the bottom of the screen when the active line count is decreased (e.g.
> >> setting the resolution to 720x480 but for 50Hz "PAL" - like many game consoles
> >> did back in the day).
> >>
> >> I believe that the perfect solution would:
> >>
> >> - Use the canonical / standard-defined blanking line counts for the standard
> >>   vertical resolutions (480/486/576)
> >> - Increase vfp and vbp from there by the same number if a smaller number of
> >>   active lines is specified, so that the resulting image is centered
> >> - Likewise, decrease vfp and vbp by the same number if the active line number
> >>   is larger and there is still leeway (this should allow for seamless handling
> >>   of 480i vs. 486i for 60 Hz "NTSC")
> >
> > I'm not sure I understand how that's any different than the code you
> > initially commented on.
> >
> > I would start by taking the entire blanking area, remove the sync
> > period. We only have the two porches now, and I'm starting from the
> > minimum, adding as many pixels in both (unless it's not an even number,
> > in which case the backporch will have the extra pixel).
> >
> > Isn't it the same thing?
> > [...]
> > Unless you only want me to consider the front porch maximum?
> 
> I think you're confusing the "post-equalizing pulses" with the "vertical back
> porch" a little bit. The "vertical back porch" includes both the post-equalizing
> pulses and the entire rest of the VBI period, for the standard resolutions at
> least.
> 
> The "canonical" modelines (at least for vc4's VEC, see the notes below):
> 
> - (vfp==4, vsync==6, vbp==39) for 576i
> - (vfp==7, vsync==6, vbp==32) for 480i
> - (vfp==5, vsync==6, vbp==28) for 486i (full frame NTSC as originally specified)
> 
> The numbers for vfp don't exactly match the theoretical values, because:
> 
> - VEC actually adds a half-line pulse on top of VFP_ODD, and in the 625-line
>   mode also on top of VFP_EVEN (not always, but let's not dwell too much)
> - Conversely, VEC subtracts the half-line pulse from VSYNC_ODD and VSYNC_EVEN
>   in the 625-line mode
> - SMPTE S170M (see https://www.itu.int/rec/R-REC-BT.1700-0-200502-I/en) defines
>   that active picture for NTSC is on lines 21-263 and 283-525; 263 and 283 are
>   half-lines that are represented as full lines in the "486i" spec

It's going to be a bit difficult to match that into a drm_display_mode,
since as far I understand it, all the timings are the sum of the timings
of both fields in interlaced. I guess we'll have to be close enough.

> - SMPTE 314M, which is the spec for DV, defines the 480 active lines as lines
>   23-262 and 285-524; see Table 20 on page 26 in
>   https://last.hit.bme.hu/download/firtha/video/SMPTE/SMPTE-314M%20DV25-50.pdf;
>   this means that the standard 480i frame shaves off four topmost and two
>   bottommost lines (2 and 1 per field) of the 486i full frame

I'm still struggling a bit to match that into front porch, sync period
and back porch. I guess the sync period is easy since it's pretty much
fixed. That line 0-23 is the entire blanking area though, right?

> Note that the half-line pulses in vfp/vsync may be generated in a different way
> on encoders other than vc4's VEC. Maybe we should define some concrete
> semantics for vfp/vsync in analog TV modes, and compensate for that in the
> drivers. But anyway, that's a separate issue.
> 
> My point is that, to get a centered image, you can then proportionately add
> values to those "canonical" vfp/vbp values. For example if someone specifies
> 720x480 frame, but 50 Hz PAL, you should set (vfp==52, vsync==6, vbp==87).

In this case, you add 48 both front porches, right? How is that
proportionate?

> 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?

Maxime