drm/vc4: Add support for non-standard modes in VEC

[kFYatek]

This PR adds support for non-standard composite output modelines, specifically:

• Progressive modes. These are equivalent to firmware's sdtv_mode=16/sdtv_mode=18 in terms of the resulting signal's timing, although the firmware modes employ implicit scaling (480/576 active lines from software scaled down to 240/288 lines on actual output). I added the 720x240 and 720x288 modes that are reported via vc4_vec_connector_get_modes().
• Custom interlaced modelines. Some examples of what can be done with it:
  • "720x486i" 13.5 720 734 798 858 486 491 497 525 Interlace - NTSC with all the active lines from the original spec, before the convention of cropping the outermost 6
  • "720x506i" 13.5 720 734 798 858 506 509 515 525 Interlace - NTSC mode with basically the entire VBI accessible
  • "720x610i" 13.5 720 740 804 864 610 612 618 625 Interlace - the same but for PAL; you can e.g. emit teletext by drawing in the uppermost lines
  • "720x480i" 13.5 720 740 804 864 480 486 492 525 Interlace - an approximation of the "NTSC" mode while VEC is put in the 625-line mode; allows e.g. putting SECAM color on top of that, which is a really dumb idea but it works ;)
  • Basically any interlaced mode that has horizontal timings compatible with either the 525-line or 625-line mode, and no more lines than the standard, will work and generate a sane-looking signal. I don't know how could that be useful, but if it works, then why not. Unfortunately the progressive modes are much stricter, the VEC is locked to either 262 or 312 total lines, so the only thing you can do is change the proportions between active picture and VBI.
    • "640x200" 13.5 640 694 758 858 200 223 226 262 is an example CGA-like mode with black borders.
• Support for clocking the VEC at different frequencies than 108 MHz. This also includes recalculating the color subcarrier frequencies, so that they stay within spec even in that case. I guess that this point will be the most controversial of it all, but it has some uses:
  • For fun, you can try modes such as "720x378i 8.68725 720 734 798 858 378 381 387 405 Interlace - that in theory closely approximates the early British 405-line TV system and may be able to drive such a set. Obviously I don't have hardware to check that ;)
  • More importantly though, this makes the composite output somewhat usable even without enable_tvout=1 on Pi4. Due to the clock being 0.8% slower than the spec, the ratio relationship between the line frequeny and subcarrier frequency is broken, which makes this mode cause a lot more cross-color artifacts, but the picture is still usable. Custom clock rate support has been removed, as it is nearly useless now that clk: Move vec clock to clk-raspberrypi #4639 is merged.
  • As a side note, by using a customized DT overlay that enables both VEC and HDMI at the same time, I was able to get a working dual-screen HDMI+Composite setup on Pi4. Curiously, it works both with and without enable_tvout=1 (although I don't know, maybe the HDMI timings aren't quite standard then?). So maybe having an option for this type of setup in vc4-kms-v3d-pi4.dtbo would make sense.
    • Actually, I was kind of hoping that triple screen (2xHDMI + Composite) setup would be possible, but unfortunately I get CRTC activation error if I try enabling composite while both HDMIs are enabled already ;)

[kFYatek]

I've rebased new code now that #4241 is merged.

[pelwell]

There are some niggly whitespace complaints from checkpatch about the first commit:

0001-drm-vc4-Relax-VEC-modeline-requirements-and-add-prog.patch
---------------------------------------------------------------
ERROR: code indent should use tabs where possible
#59: FILE: drivers/gpu/drm/vc4/vc4_vec.c:449:
+^I^I^I            crtc_state->mode.crtc_vdisplay < 1 ||$

ERROR: code indent should use tabs where possible
#61: FILE: drivers/gpu/drm/vc4/vc4_vec.c:451:
+^I^I^I            crtc_state->mode.crtc_vsync_start != 3 ||$

ERROR: code indent should use tabs where possible
#63: FILE: drivers/gpu/drm/vc4/vc4_vec.c:453:
+^I^I^I            crtc_state->mode.crtc_vsync_end < 4 ||$

ERROR: code indent should use tabs where possible
#88: FILE: drivers/gpu/drm/vc4/vc4_vec.c:478:
+^I^I^I            crtc_state->mode.crtc_vdisplay < 1 ||$

ERROR: code indent should use tabs where possible
#90: FILE: drivers/gpu/drm/vc4/vc4_vec.c:480:
+^I^I^I            crtc_state->mode.crtc_vsync_start != 3 ||$

ERROR: code indent should use tabs where possible
#92: FILE: drivers/gpu/drm/vc4/vc4_vec.c:482:
+^I^I^I            crtc_state->mode.crtc_vsync_end < 2 ||$

total: 6 errors, 0 warnings, 101 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

NOTE: Whitespace errors detected.
      You may wish to use scripts/cleanpatch or scripts/cleanfile

It wants the indentation to consist of as many TABs as possible, with spaces only at the end to pad out the remainder.

[6by9]

2nd and third patches have checkpatch complaints too

CHECK: Lines should not end with a '('
#128: FILE: drivers/gpu/drm/vc4/vc4_vec.c:425:
+	interlaced_mode = drm_mode_duplicate(

CHECK: Lines should not end with a '('
#131: FILE: drivers/gpu/drm/vc4/vc4_vec.c:428:
+	progressive_mode = drm_mode_duplicate(

total: 0 errors, 0 warnings, 2 checks, 146 lines checked

break the line after the =, and then align the parameters to drm_mode_duplicate

CHECK: No space is necessary after a cast
#148: FILE: drivers/gpu/drm/vc4/vc4_vec.c:687:
+	chroma_freq += (125ull * (u64) eff_clk_rate) >> 1; /* proper rounding */

That one the space isn't needed between (u64) and eff_clk_rate, and doesn't aid readability.

As long as the standard modes come out to the same numbers as before then I'm happy with the patches.

[pelwell]

The kernel submission guidelines do not specify --strict checking as a requirement. The netdev FAQ suggests it as an option, and even hwmon - the only subsystem to require it - says There should be no errors, no warnings, and few if any check messages, allowing some CHECKs.

[kFYatek]

@pelwell @6by9 I rebased this PR and resolved the style issues reported by checkpatch. Sorry that I went silent on this for so long.

[pelwell]

You're slightly outside my comfort zone with this PR, but it makes sense to me. I assume the change in vc4_vec_connector_atomic_check to use the interlace_mode.htotal is an arbitrary decision, made possible because progressive_mode .htotal is defined to be the same (the differences being in the vertical dimension)?

[kFYatek]

You're slightly outside my comfort zone with this PR, but it makes sense to me. I assume the change in vc4_vec_connector_atomic_check to use the interlace_mode.htotal is an arbitrary decision, made possible because progressive_mode .htotal is defined to be the same (the differences being in the vertical dimension)?

The VEC actually misbehaves if htotal is set to something else than 858 for the 525-line ("NTSC") or 864 for the 625-line ("PAL") mode. It generates the horizontal sync and blanking signals internally once every 858 or 864 pixels regardless of what it's being fed from the PV, and there doesn't seem to be any way of changing that.

Taking the above into account, attempting to set anything else as htotal results in an image that is skewed diagonally with a black diagonal bar in the middle (in the place where PV-generated blanking&sync are). I'm not near my development setup, so I can't make my own screenshots right now, but the effect is something like this: raspberrypi/firmware#811 (comment)

Conversely, VEC actually responds to changes in the vertical timing settings quite reasonably, at least in the interlaced mode. Manipulating this is bound to result in modes that are non-standard, but they might be useful in one way or another. But we need to ensure that htotal is set appropriately for the signal to make any sense at all.

[kFYatek]

Note: In this newest revision, aside from rebasing onto current rpi-5.10.y, I moved the mode validation code from vc4_vec_connector_atomic_check() to vc4_vec_encoder_atomic_check(), which itself replaced vc4_vec_encoder_mode_fixup(). The new version now performs the validation on adjusted_mode. This has been done to avoid doing this move in #4636.

Both approaches seem to yield equivalent results from the userland perspective.

[popcornmix]

@kFYatek can you test this firmware and this PR and check behaviour of VEC clock?

I believe previously you'd get 107.143MHz if enable_sdtv=1 was missing as firmware hadn't set up the VEC clock and the kernel code to do it was flawed. With updated fw/kernel it should get the correct 108MHz and it shouldn't be necessary to handle an inaccurate VEC clock.

[kFYatek]

@popcornmix Yes, this indeed yields clean 108 MHz VEC clock on both Pi3 and Pi4, both with and without enable_sdtv=1.

Attempting to set the clock to something else than 108 MHz still does... something, but it looks like only some very simple fractions of that clock are supported - I could get what looked like 2/3, 3/4 and 6/7 of 108 MHz (i.e., 72 MHz, 81 MHz, 92.571... MHz), but nothing in between, so attempting that is pretty much useless.

So if you intend to merge that, I'll remove that one commit that adds custom clock support.

[popcornmix]

Yes, that is expected. PLLC is set to 108 * 24, and PLLC_CHAN_CPER is a divide by 4.
so you can get clean frequencies that are integer dividers of that, but there's not a lot of control there.

I'll get the firmware and kernel patches pushed.

[kFYatek]

This newest force-push includes the following changes:

• Rebase onto current state of rpi-5.10.y
• Removal of the "drm/vc4: Make the VEC clock adjustable" commit, as it is useless after having clk: Move vec clock to clk-raspberrypi #4639 merged
• Instead, the requirement that the custom mode's clock needs to be equal to the reference one has been added in vc4_vec_encoder_atomic_check()
• reference_mode local variable has been introduced in vc4_vec_encoder_atomic_check(), to make the comparisons cleaner
• The switch for validating either 525-line-based or 625-line-based mode parameters has been changed to switch over reference_mode->vtotal so that the cases are now 525 and 625, which is probably cleaner than switching on htotal

[popcornmix]

Tested and it appears to work. The following modes gave plausible output:
video=Composite-1:720x480@60i
video=Composite-1:720x576@50i
video=Composite-1:720x240@60
video=Composite-1:720x288@50