Fix composite output video norms #4241
Conversation
|
Thanks for the patches. I need to spend a little time looking into them and testing. There will be a request for all the commit texts to be updated as they should start It's also a little confusing that you add all these *_mode_set functions for the new standards, then rip them all out again and move the config that they add into struct vc4_vec_tv_mode. It would be nicer to make that move first, and then add the extra modes. With regard |
|
NOTE: There are no changes in the force-push above yet. I just rebased everything onto the current main branch. I will now proceed with addressing your comments. |
Well, that line is just e.g. |
|
NOTE: As far as code is concerned, the only changes compared to the previous version are slight wording improvements in some comments. No actual code has been changed, this is all about reorganizing the commits.
Sure, done.
Sure, done as well. The original PR contained the commits "raw", mostly how I actually developed these changes. I don't know much about the etiquette of the kernel repo, so sorry about that. I now rewrote the history so that the commits make more sense on their own, and are more concise as well. |
There was a problem hiding this comment.
I'm not sure we need to read-out the hardware state. The command line parser explicitly supports named modes, so the firmware could just set video=TV-1:NTSC-J or whatever and the initial framebuffer will use that mode.
Switching back and forth between modes is more problematic though. The userspace is free to build whatever mode it wants, and / or the framebuffer emulation (and thus the initial setup) can be disabled.
In such a case, you don't really have a way to differentiate between variants that use the same mode (like between NSTC-J and PAL-M). Once again, you can only hope that the name will be filled and compare the name to the modes you support, and fallback on PAL or NTSC
There was a problem hiding this comment.
Sorry it's taken me a while to get around to looking at this.
Generally looks fine, but the PV setup changes in vc4_crtc.c have me a little concerned.
Seeing as I don't believe it holds any secrets, the firmware config code for the PV when driving the VEC is:
if (use_625_line)
{
vec_state.pixelvalve_setup.horizontal_sync_width_in_pixels = 64;
vec_state.pixelvalve_setup.horizontal_frontporch_in_pixels = 20;
vec_state.pixelvalve_setup.horizontal_backporch_in_pixels = 60;
vec_state.pixelvalve_setup.horizontal_active_in_pixels = 720;
vec_state.pixelvalve_setup.vertical_sync_width_in_lines = 3;
vec_state.pixelvalve_setup.vertical_frontporch_in_lines = 2;
vec_state.pixelvalve_setup.vertical_backporch_in_lines = use_progressive ? 19 : 20;
vec_state.pixelvalve_setup.vertical_active_in_lines = 576/2;
vec_state.pixelvalve_setup.vertical_even_sync_width_in_lines = vec_state.pixelvalve_setup.vertical_sync_width_in_lines;
vec_state.pixelvalve_setup.vertical_even_frontporch_in_lines = vec_state.pixelvalve_setup.vertical_frontporch_in_lines;
vec_state.pixelvalve_setup.vertical_even_backporch_in_lines = vec_state.pixelvalve_setup.vertical_backporch_in_lines - 1;
vec_state.pixelvalve_setup.vertical_even_active_in_lines = vec_state.pixelvalve_setup.vertical_active_in_lines;
vec_state.pixelvalve_setup.first_field_odd = 0;
}
else
{
vec_state.pixelvalve_setup.horizontal_sync_width_in_pixels = 64;
vec_state.pixelvalve_setup.horizontal_frontporch_in_pixels = 14;
vec_state.pixelvalve_setup.horizontal_backporch_in_pixels = 60;
vec_state.pixelvalve_setup.horizontal_active_in_pixels = 720;
vec_state.pixelvalve_setup.vertical_sync_width_in_lines = 3;
vec_state.pixelvalve_setup.vertical_frontporch_in_lines = 3;
vec_state.pixelvalve_setup.vertical_backporch_in_lines = 16;
vec_state.pixelvalve_setup.vertical_active_in_lines = 480/2;
vec_state.pixelvalve_setup.vertical_even_sync_width_in_lines = vec_state.pixelvalve_setup.vertical_sync_width_in_lines;
vec_state.pixelvalve_setup.vertical_even_frontporch_in_lines = vec_state.pixelvalve_setup.vertical_frontporch_in_lines + 1;
vec_state.pixelvalve_setup.vertical_even_backporch_in_lines = vec_state.pixelvalve_setup.vertical_backporch_in_lines;
vec_state.pixelvalve_setup.vertical_even_active_in_lines = vec_state.pixelvalve_setup.vertical_active_in_lines;
vec_state.pixelvalve_setup.first_field_odd = 1;
}
uint32_t half_line = (vec_state.pixelvalve_setup.horizontal_sync_width_in_pixels +
vec_state.pixelvalve_setup.horizontal_frontporch_in_pixels +
vec_state.pixelvalve_setup.horizontal_backporch_in_pixels +
vec_state.pixelvalve_setup.horizontal_active_in_pixels) >> 1;
vec_state.pixelvalve_setup.second_field_offset = half_line + 1;
if (pal_mode)
vec_state.pixelvalve_setup.mode = PIXELVALVE_MODE_VEC_PAL;
else
vec_state.pixelvalve_setup.mode = PIXELVALVE_MODE_VEC_NTSC;
The values are written without modification into the relevant PV registers.
Note that all the vertical timings have already been divided by 2 due to interlacing. That happens in the magic of drm_mode_set_crtcinfo for DRM, but that does have the potential for rounding errors on odd values.
drivers/gpu/drm/vc4/vc4_crtc.c
Outdated
| mode->crtc_vsync_end - 1, | ||
| VC4_SET_FIELD(mode->crtc_vtotal + vtotal_fixup - | ||
| mode->crtc_vsync_end - | ||
| (ntsc ? 0 : 1), |
There was a problem hiding this comment.
This one feels odd.
vtotal_fixup = (vec_interlaced && !ntsc) ? 1 : 0
but here we're doing - (ntsc ? 0 : 1), and we're in a condition where we're interlaced, so effectively ```(interlaced && ntsc) ? 0 : 1).
Doesn't that end up with the two operations cancelling each other out?
There was a problem hiding this comment.
Not exactly.
- Progressive or HDMI interlace:
vtotal_fixup == 0,ntsc == 0, total fixup here:-1 - VEC 525-line ("NTSC"):
vtotal_fixup == 0,ntsc == 1, total fixup here:0 - VEC 625-line ("PAL"):
vtotal_fixup == 1,ntsc == 0, total fixup here:0
I guess this vtotal_fixup may be confusing after all, and using vec_interlaced and ntsc directly at those two places these are used will be cleaner.
drivers/gpu/drm/vc4/vc4_crtc.c
Outdated
| @@ -343,6 +343,10 @@ static void vc4_crtc_config_pv(struct drm_crtc *crtc) | |||
| bool is_dsi1 = vc4_encoder->type == VC4_ENCODER_TYPE_DSI1; | |||
| u32 format = is_dsi1 ? PV_CONTROL_FORMAT_DSIV_24 : PV_CONTROL_FORMAT_24; | |||
| u8 ppc = pv_data->pixels_per_clock; | |||
| bool vec_interlaced = (vc4_encoder->type == VC4_ENCODER_TYPE_VEC && | |||
| interlace); | |||
| bool ntsc = (vec_interlaced && mode->htotal == 858); | |||
There was a problem hiding this comment.
This way of detecting NTSC does feel a little fragile, but I'm not sure that there is a better solution.
There was a problem hiding this comment.
I now realize that this may even be confusing, as this actually detects 525/625-line modes, not NTSC per se (i.e. PAL-M is "NTSC" in this sense and that's intended), so renaming these variables is probably the right thing to do.
drivers/gpu/drm/vc4/vc4_vec.c
Outdated
| @@ -283,8 +283,8 @@ static void vc4_vec_pal_m_mode_set(struct vc4_vec *vec) | |||
|
|
|||
| static const struct drm_display_mode pal_mode = { | |||
| DRM_MODE("720x576", DRM_MODE_TYPE_DRIVER, 13500, | |||
| 720, 720 + 20, 720 + 20 + 64, 720 + 20 + 64 + 60, 0, | |||
| 576, 576 + 2, 576 + 2 + 3, 576 + 2 + 3 + 20, 0, | |||
| 720, 720 + 13, 720 + 13 + 64, 720 + 13 + 64 + 67, 0, | |||
There was a problem hiding this comment.
Has this been tested on a Pi4? Some of the PVs on 2711 operate at 2 pixels/clock, which means that odd values in horizontal timings cause issues as it can't signal the sync event between the two pixels.
I'm not convinced by the centering anyway as it will cause different behaviour compared to the firmware, and it also should be a separate patch to that correcting the timings.
There was a problem hiding this comment.
Fair enough, keeping the timing consistent with the firmware is a fair argument.
But odd values for front and back porch lengths do indeed work on Pi4.
drivers/gpu/drm/vc4/vc4_crtc.c
Outdated
| PV_VERTB_VFP) | | ||
| VC4_SET_FIELD(mode->crtc_vdisplay, PV_VERTB_VACTIVE)); | ||
| VC4_SET_FIELD(mode->crtc_vdisplay, | ||
| PV_VERTB_VACTIVE)); |
There was a problem hiding this comment.
All of these fixups become quite hard to read and understand what exactly is going on.
My gut feeling is that it'd be clearer to add local variables at least for the vertical active, fp, sw, and bp values, and then have explicit clauses for NTSC and PAL when assigning to the registers. At least that way the fixups are obvious.
There was a problem hiding this comment.
OK, I'll try to simplify things a bit.
Well, unfortunately the current versions of the firmware only pass information such as I agree that what I wrote is a bit of a kludge, and I agree that if the firmware could pass the colour system settings on the kernel's command line, that would be much better. I'm happy to update my code once it does, but for now, reading the VEC register state found at boot is the only way to get this information. |
|
Note; the force-push above is just a rebase, with no changes to the actual code. I'll amend the diff with the next update. |
It looks like the firmware has a struct I'm currently failing to find the kernel side of the parsing for @mripard Are you believing that the parsing for this is already present and will set the tv_mode_property, or is this a further change that is needed? |
kFYatek
force-pushed
the
fix-composite
branch
2 times, most recently
from
5050b04
to
1fd7b63
Compare
|
Note: the update from 7 minutes ago updates the first commit in this PR according to the review notes. The second update from just now contains no code changes, it just updates the commit message of that commit to remove information about the shift by 7 pixels, which has been removed. I forgot to update the commit message the first time. |
|
I updated the PR with the new logic. The new I added additional logic so if that option is absent, but the One important note is that if the |
|
The above force-push rebased this PR onto current The conflict resolution was ("old" is the previously pushed revision, "new" is the current one): --- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -362,7 +349,7 @@ static void vc4_crtc_config_pv(struct drm_crtc *crtc, struct drm_atomic_state *s
if (interlace) {
bool odd_field_first = false;
- u32 field_delay = mode->htotal * pixel_rep / 2;
+ u32 field_delay = mode->htotal * pixel_rep / (2 * ppc);
u16 vert_bp_even = vert_bp;
u16 vert_fp_even = vert_fp;
@@ -372,16 +359,14 @@ static void vc4_crtc_config_pv(struct drm_crtc *crtc, struct drm_atomic_state *s
if (mode->htotal == 858) {
/* 525-line mode (NTSC or PAL-M) */
odd_field_first = true;
- ++vert_fp_even;
- } else {
- /* 625-line mode (PAL or SECAM) */
- ++vert_bp;
}
- } else {
- /* HDMI */
- --vert_bp_even;
}
+ if (odd_field_first)
+ ++vert_fp_even;
+ else
+ ++vert_bp;
+
CRTC_WRITE(PV_VERTA_EVEN,
VC4_SET_FIELD(vert_bp_even, PV_VERTA_VBP) |
VC4_SET_FIELD(vert_sync, PV_VERTA_VSYNC)); |
drivers/gpu/drm/vc4/vc4_vec.c
Outdated
| }; | ||
|
|
||
| enum vc4_vec_tv_mode_id | ||
| vc4_vec_get_default_mode(struct drm_connector *connector) { |
There was a problem hiding this comment.
Function start braces must go on separate lines.
drivers/gpu/drm/vc4/vc4_vec.c
Outdated
| (!connector->cmdline_mode.refresh_specified && | ||
| (connector->cmdline_mode.yres == 288 || | ||
| connector->cmdline_mode.yres == 576)))) { | ||
| /* no explicitly specified TV norm; use PAL if a mode that |
There was a problem hiding this comment.
Kernel multi-line comments must be of the form:
/*
* blah blah
* blah blah blah
*/
There was a problem hiding this comment.
The drm subsystem is happy with
/* blah
* blah
*/
but the closing */ does have to be on a separate line.
|
This isn't really my field, so apologies that I can only contribute nit-picky cosmetic feedback, but this looks like an impressive patch set. |
There was a problem hiding this comment.
Thanks for the patchset.
I need to test it out to satisfy myself that it actually does the right thing, but other than the minor niggles below it looks good.
I have asked mripard over passing the tv standard via the kernel command line video= entry. Seeing as he wrote most of that code he's a pretty good person to ask :-) I don't see any way that it sets the tv_mode property, so the module parameter seems a good option at present.
The only other driver I really see handling tv_mode is i915, and it just always defaults to NTSC.
| @@ -68,6 +68,7 @@ | |||
| #define VEC_CONFIG0_STD_MASK GENMASK(1, 0) | |||
| #define VEC_CONFIG0_NTSC_STD 0 | |||
| #define VEC_CONFIG0_PAL_BDGHI_STD 1 | |||
| #define VEC_CONFIG0_PAL_M_STD 2 | |||
There was a problem hiding this comment.
Can I ask where you got that define from? The datasheet I have lists the values as
0 - NTSC
1 - PAL_BDGHI
2 - reserved
3 - PAL_N
The firmware only selects from modes 0, 1, and 3, so I'm wondering where your mapping for 2 has come from.
There was a problem hiding this comment.
It's been a while since I found this, so my memory is not exactly clear, but AFAIR it was mostly experimentation. I was just messing with the registers and seeing what happens. I set what's defined here as VEC_CONFIG0_STD_MASK to 2 and poof, a standards-compliant PAL-M mode appeared.
I wrote https://github.com/kFYatek/tweakvec some time earlier to mess with these registers from user space. The effects of that have been discussed a bit in raspberrypi/firmware#811, where @Sobakin76 confirmed that this works on, let's call it, "period-accurate" displays. Here's a video they made that shows it: https://youtu.be/299IORo56lM (note, however, that they have their Pi set to 240p progressive mode). PAL-M support is uncommon on devices from my region, so I couldn't do that personally, but I did check it on modern-ish devices (particularly the Fushicai USBTV007 grabber with Windows drivers, and a Samsung UE40E6800S TV - both are curiously compatible with pretty much every TV norm ever).
There was a problem hiding this comment.
I've now checked the 2711 register definitions, and those values are listed as reserved there too.
I'll see if I can get anyone at Broadcom to confirm your findings.
| * PAL (4433618.75 Hz) - 0x2A098ACB | ||
| * PAL-M (3575611.[888111] Hz) - 0x21E6EFE3 | ||
| * PAL-N (3582056.25 Hz) - 0x21F69446 | ||
| * |
There was a problem hiding this comment.
Datasheet says colour subcarrier frequency = 13.5MHz * FREQ / 2^31.
However the firmware code for setting up PAL_M (the only option which uses a custom subcarrier) is 0x223b61d1, which differs from yours. Computing the values, I agree with your numbers though.
There was a problem hiding this comment.
0x223b61d1, which translates to 3610402.169 Hz, is a long-running bug in the firmware. I'm not aware of a TV norm that would use this frequency ever existing. sdtv_mode=3 indeed configures that frequency (with VEC_CONFIG0_STD_MASK set to 0 for NTSC), but that mode is pretty much unusable. Here's how sdtv_mode=3 looks on my Fushicai USBTV007 grabber. See also raspberrypi/firmware#756, there are pictures of @Sobakin76's monitors showing the same thing there.
| * | ||
| * NOTE: For SECAM, it is used as the Dr center frequency, | ||
| * regardless of whether VEC_CONFIG1_CUSTOM_FREQ is enabled or not; | ||
| * that is specified as 4406250 Hz, which corresponds to 0x29C71C72. |
There was a problem hiding this comment.
That 0x29C71C72 corresponds to the default value by the looks of it.
| * Db center frequency for SECAM; the clock for this is the same as for | ||
| * VEC_FREQ3_2/VEC_FREQ1_0, which is used for Dr center frequency. | ||
| * | ||
| * This is specified as 4250000 Hz, which corresponds to 0x284BDA13. |
There was a problem hiding this comment.
0x284BDA13 is the default value for the register. Probably worth mentioning that as otherwise there is no other reference to it, and VEC_FCW_SECAM_B is never set.
| @@ -45,6 +45,7 @@ | |||
| #define VEC_CONFIG0_YDEL(x) ((x) << 26) | |||
| #define VEC_CONFIG0_CDEL_MASK GENMASK(25, 24) | |||
| #define VEC_CONFIG0_CDEL(x) ((x) << 24) | |||
| #define VEC_CONFIG0_SECAM_STD BIT(21) | |||
There was a problem hiding this comment.
More magic fields. CONFIG0 bits 23:19 are defined a reserved, write as 0. Then again it also doesn't say how to select SECAM mode!
There was a problem hiding this comment.
Well, that's another product of me just messing around with this register. I read this header, saw that VEC_FCW_SECAM_B and VEC_SECAM_GAIN_VAL registers and was intrigued, especially since nothing else referred to SECAM and I don't think support for SECAM on the Pi was ever seen before. I started flipping bits in this register and when I set this one, again, poof, SECAM appeared.
I empirically confirmed that this mode is detected as SECAM and looks right on a couple of modern devices:
- Fushicai USBTV007 grabber
- Samsung UE40E6800S
- Panasonic TX-L42ET5E
- Samsung UE65MU8002 (through an RF modulator, this TV does not have any baseband analog inputs)
Again, @Sobakin76 also confirmed this working on their more period-accurate Sony PVM-14N5MDE CRT: raspberrypi/firmware#811 (comment)
EDIT: Note that the artifacts on their photo are a known issue with the SECAM system itself; this is basically what happens on it instead of PAL/NTSC-style "dot crawl" when the image has too much sharp edges across the horizontal axis (exactly like the fine text on that picture screenshot).
drivers/gpu/drm/vc4/vc4_vec.c
Outdated
| (!connector->cmdline_mode.refresh_specified && | ||
| (connector->cmdline_mode.yres == 288 || | ||
| connector->cmdline_mode.yres == 576)))) { | ||
| /* no explicitly specified TV norm; use PAL if a mode that |
There was a problem hiding this comment.
The drm subsystem is happy with
/* blah
* blah
*/
but the closing */ does have to be on a separate line.
| drm_atomic_helper_connector_reset(connector); | ||
| /* preserve TV standard */ | ||
| if (connector->state) | ||
| connector->state->tv.mode = vc4_vec_get_default_mode(connector); |
There was a problem hiding this comment.
Do we want to reset to the default (as defined by the module parameter), or retain the old setting?
If you change the module parameter (it is permissions 0600) then on the next reset call don't we have the same scenario as at boot where the mode may not match the standard?
There was a problem hiding this comment.
You're right about the comment - checkpatch says "WARNING: Block comments use a trailing */ on a separate line".
There was a problem hiding this comment.
reset is only called once at boot time, it's what creates the first initial commit. Any state after that is the union of the previous state and whatever has been changed in the various commits that occurred.
There was a problem hiding this comment.
Uh, this is actually one of the places where I don't exactly know what I'm doing. From what I understand, this function is called when a new connector "state" object is created. By default, it is more or less just zeroed out, which means also setting tv.mode field to 0, which is NTSC in our case. The default value set through drm_object_attach_property() is not respected for some reason, so without this code, we would end up in NTSC mode at boot regardless of what was set there.
Setting this value later in userspace (xrandr --set mode PAL etc.) still works.
Actually the only other driver in the entire kernel codebase that uses the tv.mode field is i915 - which defaults to NTSC anyway, as pointed out in #4241 (review). nouveau and ch7006 just keep track of it in a separate private variable and use that instead.
I don't know, maybe a cleaner solution would be to patch the DRM subsystem itself so that it would pull the default value from whatever has been set through drm_object_attach_property() - because not respecting that even though the tv_mode_property is automatically mapped to that field seems like a bug. Maybe that should be done directly in vc4_vec_connector_reset() or something, although I'm not sure if that's even possible.
My main issue with reading out the hardware state is that it means that users won't be able to change the TV norm without rebooting, and it has weird interactions with other parts of the system, for example:
The tv_mode parameter you introduced addresses most of these concerns, but it's redundant with the same property that is already set. The command line parser can parse arbitrary properties, so it would make more sense to extend this part: Adding another switch statement that fills a new field in drm_cmdline_mode that would store the mode, and then init the tv_mode property to that value here: would work quite nicely without any ad-hoc solution. |
This is actually not true. Switching between 525-line (NTSC/NTSC-J/NTSC-443/PAL-M/PAL60) and 625-line (PAL/PAL-N/SECAM) modes requires adding the missing mode via |
|
None of it feels totally clean. Adding an extra property to It's also slightly ugly that the cmdline parsing appears to be called from |
|
I've grabbed the Kramer VP719xl that I have access to which can supposedly handle NTSC, NTSC4.43, PAL, PAL-N, PAL-M, SECAM. So it nominally misses NTSC-J and PAL-60, but should be able to tell me about the rest. Now to find my A/V lead to connect it up.... |
|
@6by9 If it supports NTSC and PAL, it almost certainly supports PAL-60. It will probably just label it as "PAL", while still syncing at 60Hz. NTSC-J and regular NTSC are also identical except for the black level; i.e., if you set NTSC-J and never go below about 5% gray level, you'll end up with a compliant NTSC(-M) signal. So I think your display should handle all the modes. |
|
Tested against my Kramer (it's a presentation switcher/scaler rather than a display). I had expected changing mode (at least between those with the same resolution) wouldn't require a manual disable/reconfigure/enable. That should achievable by setting It may be this setup, but it looks like it's working at a very reduced bit depth on the VEC output. A quick hack to the overlay so that HDMI is active at the same time and the images are significantly different (I'll try and attach a photo in a bit). I'd be interested to know if you see the same. The desktop looks OK, but a JPEG is quantised. |
I refactored the However, Xorg does not check the ioctl return value when setting properties, so if you try to set an invalid value (e.g.
I haven't noticed anything like that. What JPEG did you use and what program did you use to display it? In my experience, there is a bit of banding, but nothing too distracting, and importantly it seems to be more or less the same as in the firmware framebuffer. I did some screenshots with my USB grabber, showing the gradient.png file from rtings.com rendered in Chromium in fullscreen mode (F11). |
|
Thanks for your perseverance on this. The refactor to use connector_atomic_check is almost exactly what I'd ended up with. Looks fine. I'd just viewed a JPEG (an unfocused capture from an HQ camera module) and there is a significant difference between the HDMI output and VEC. See this image where the front monitor is on HDMI, and the back one is the VEC fed through the Kramer to give VGA out. |
|
Whoah. That looks bad indeed. But as you can see on my screenshots linked above, I saw nothing like this. I found some time ago, however, that the Pi (Pi 4 especially) outputs composite signals that are not exactly the standard voltage. When I used an RF modulator (just a cheapo thing from Amazon) to connect the Pi's composite output to my modern Samsung TV that only accepts analog signals via RF, I found that the brightest parts of the image got clipped. I was able to get the picture to look right by putting a potentiometer between the Pi and the modulator and setting it slightly down from "100%". I can make pictures and measure the actual resistance if you want. I found that modulator is based on the MC44BS373CA chip, with expects 1 V peak-to-peak video input (see page 7 of the datasheet, among others). The Pi's signal is not exactly that, and it has a nonzero DC component (again, this is especially true on Pi4; on Pi4, I could not find any VEC register tweaks that could fix this issue), which confuses that modulator. All other devices I tested employ some kind of AGC and thus have no problems with its output. My wild guess is that maybe your Kramer also doesn't like the voltages output by the Pi for whatever reason. Although this weird green tint seems to affect neither the highlights or the shadows, but rather the middle of the brightness range, which is particularly odd and not at all consistent with the voltage mismatch theory... |
|
Say the word when you think this is ready for merging. |
|
@pelwell I'm not sure who you're asking. I considered it ready from the beginning, and updates were just addressing the reviews ;) With the catch that I expanded on ideas from this PR in #4406, which refactors some code refactored here again - so you might want to take a look there as well. I was under the impression that we're waiting for acceptance from @mripard and, mostly, @6by9. |
|
Understood - I was addressing the reviewers as well. |
|
Sorry, juggling too many things. One last checkpatch complaint (sorry) - patch author doesn't match the Signed-Off-By If you've updated your standard author email address, then |
This commit fixes vertical timings of the VEC (composite output) modes
to accurately represent the 525-line ("NTSC") and 625-line ("PAL") ITU-R
standards.
Previous timings were actually defined as 502 and 601 lines, resulting
in non-standard 62.69 Hz and 52 Hz signals being generated,
respectively.
Changes to vc4_crtc.c have also been made, to make the PixelValve
vertical timings accurately correspond to the DRM modeline in interlaced
modes. The resulting VERTA/VERTB register values have been verified
against the reference values set by the Raspberry Pi firmware.
Signed-off-by: Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>
Change the mode_set function pointer logic to declarative config0, config1 and custom_freq fields, to make TV mode setting logic more concise and uniform. Additionally, remove the superfluous tv_mode field, which was redundant with the mode field in struct drm_tv_connector_state. Signed-off-by: Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>
PAL-M is a Brazilian analog TV standard that uses a PAL-style chroma subcarrier at 3.575611[888111] MHz on top of 525-line (480i60) timings. This commit makes the driver actually use the proper VEC preset for this mode instead of just changing PAL subcarrier frequency. DRM mode constant names have also been changed, as they no longer correspond to the "NTSC" or "PAL" terms. Signed-off-by: Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>
Add support for the following composite output modes (all of them are somewhat more obscure than the previously defined ones): - NTSC_443 - NTSC-style signal with the chroma subcarrier shifted to 4.43361875 MHz (the PAL subcarrier frequency). Never used for broadcasting, but sometimes used as a hack to play NTSC content in PAL regions (e.g. on VCRs). - PAL_N - PAL with alternative chroma subcarrier frequency, 3.58205625 MHz. Used as a broadcast standard in Argentina, Paraguay and Uruguay to fit 576i50 with colour in 6 MHz channel raster. - PAL60 - 480i60 signal with PAL-style color at normal European PAL frequency. Another non-standard, non-broadcast mode, used in similar contexts as NTSC_443. Some displays support one but not the other. - SECAM - French frequency-modulated analog color standard; also have been broadcast in Eastern Europe and various parts of Africa and Asia. Uses the same 576i50 timings as PAL. Also added some comments explaining color subcarrier frequency registers. Signed-off-by: Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>
Similar to the ch7006 and nouveau drivers, introduce a "tv_mode" module parameter that allow setting the TV norm by specifying vc4.tv_norm= on the kernel command line. If that is not specified, try inferring one of the most popular norms (PAL or NTSC) from the video mode specified on the command line. On Raspberry Pis, this causes the most common cases of the sdtv_mode setting in config.txt to be respected. Signed-off-by: Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>
Replace drm_encoder_helper_funcs::atomic_check with drm_connector_helper_funcs::atomic_check - the former is not called during drm_mode_obj_set_property_ioctl(). Set crtc_state->mode_changed if TV norm changes even without explicit mode change. This makes things like "xrandr --output Composite-1 --set mode PAL-M" work properly. Signed-off-by: Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>
|
No problem. I believe it should be OK now. |
|
Thank you for fixing those up. Checkpatch --strict still has one final whinge of but I'm happy to ignore that one as otherwise the LGTM. |
kernel: Fix composite output video norms See: raspberrypi/linux#4241
kernel: Fix composite output video norms See: raspberrypi/linux#4241
This PR makes the Full-KMS video output over VEC use the same timings as the firmware (with one difference regarding 625-line modes, see below), fixes PAL modes, and adds support for additional TV standards.
Some remarks:
vc4_crtc.care somewhat messy, but I made them to align the PixelValve settings (theHORZA/HORZB/VERTA/VERTBregisters in particular) perfectly with what the firmware does, while keeping the modeline truthful. For example, I needed thatvtotal_fixupto make the total number of lines per field accurate (263/313 lines) while keepingvtotalin mode definition at 525/625 lines. I could setvtotalin mode definition to 526/626, but that's not the actual number of lines, plus it messes up the refresh rates inxrandrand such.if (vec_interlaced) ++field_delay;thing, but that makes the image cleaner. Compare: before vs. after. Tweaking this value indeed moves the fields relative to each other, but whyhtotal / 2 + 1in particular is the proper value rather thanhtotal / 2, I have no idea.VEC_CONFIG3register - setting theHORIZ_LEN_MPEG1_SIFmakes it even narrower, whileSHAPE_NON_LINEARchanges the gradient at the edges a little bit. Anyway, positioning the sync pulse 7 pixels earlier than the default centers the image relative to this mask. Compare: before vs. after.vc4_vec_get_current_mode()belongs in the source like I put it, but I wrote it so that thesdtv_modesetting inconfig.txtis respected to some extent by the Full-KMS driver as the default mode at boot time. If there is a better way to do it, please tell me.NTSC-443,PAL-M,PAL60andSECAMmodes are configured in the same way as in TweakVec. The settings have been verified to work for other users in Support for progressive PAL60 in config.txt/sdtv_mode firmware#811sdtv_mode=0; see below for how to do that) causes the ability to switch virtual terminals (Ctrl+Alt+F1) to be lost, unless you switch the TV norm back first. This is how this driver has behaved before my changes. I don't know if this is how it should behave - perhaps it should switch back to e.g. NTSC (or whatever the last 525-line norm was) when doing so? I wrote some (non-public for now) code that does just that, so I can post a supplementary PR or update this one if that's the requested behaviour.I tested norm switching under X with the following one-liner:
720x480works for NTSC, NTSC-J, NTSC-443, PAL-M and PAL60.720x576works for PAL, PAL-N and SECAM.To switch to a mode incompatible with the one set at boot time, it has to be defined first using:
or
Modelines that are not exactly one of the two above, are intentionally rejected, at least for now, and this behaviour is carried over from the existing code. I will probably post another PR later that relaxes this requirement a bit, and adds support for progressive (240p/288p) modes.