Nouveau - Sep 2022 - [PATCH v2 10/41] drm/modes: Add a function to generate analog display modes

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20220909/cc223aa2/attachment.sig>

Hi Maxime,

W dniu 9.09.2022 o 15:54, Maxime Ripard pisze:
> Hi,
>
> On Wed, Sep 07, 2022 at 11:31:21PM +0200, Mateusz Kwiatkowski wrote:
> [...]
>> 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.
Well, it's probably the job of the CRTC driver to split the values from
drm_display_mode back into separate values for odd and even fields. That's
how
it's done in the vc4 driver, anyway.
>
>> - 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?
Yes, lines 0-23 is the entire blanking area. And the "back porch" in
this
context is everything from the start of the sync pulse to the start of active
video. It's not just the equalizing pulses.

The equalizing pulses have no equivalent in DRM terms. VC4/VEC inserts those
automatically and there's no direct control over them, I'm not sure
about other
encoders.

The equalizing pulses are also not essential for the composite video to work.
The spec requires them, but most TVs will tolerate them not being there (and
early systems like the British 405-line system didn't have any).
>> 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?
Yes, I meant adding 48 lines to both porches, and I meant
"proportionately" as
"split equally". Maybe that was an unfortunate choice of words.
>> 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.
If they're doing that, they probably want to have vbp in the framebuffer,
like VBIT2.

Best regards,
Mateusz Kwiatkowski