Re: [PATCH v10 18/19] drm/vc4: vec: Add support for more analog TV standards
Hi Mauro, As the author of the original version of this commit, and also a person who argued quite a bit on these descriptions and decisions, let me chip in a bit. W dniu 17.11.2022 o 16:49, Mauro Carvalho Chehab pisze: > On Thu, 17 Nov 2022 10:29:01 +0100 > Maxime Ripard wrote: > >> From: Mateusz Kwiatkowski >> >> 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. > > That's not right. Argentina uses a different standard than Paraguay and > Uruguai. > > See, there are two variants of PAL/N. The original one and PAL/N' - also > called PAL/NC or PAL/CN (Combination N). Some of the timings are > different on /NC variant. > > As far as I'm aware, PAL/Nc is used in Argentina, while > PAL/N is used in Paraguai and Uruguai, but I may be wrong on that, > as it has been a long time since had to touch on this. If you say so - maybe that's true. But I tried to find any differences between PAL-N and PAL-Nc many times and haven't found anything concrete. The only authoritative source where System N and "Combination N/PAL" seem to be mentioned as separate entities is BT.1701 <https://www.itu.int/rec/R-REC-BT.1701-1-200508-I/en>. However: a) the differences are very subtle (with "combination N/PAL" being just a tad stricter than what's mentioned for System N) b) "Combination N/PAL" can be understood as just "System N combined with PAL color", as opposed to "raw", black&white System N. This intepretation is also what the user calling themselves "Alcahemist" suggests here: https://en.wikipedia.org/wiki/Talk:PAL#PAL-N_versus_PAL-Nc This is of course far from an authoritative source. If you have a definitive source for PAL-N and PAL-Nc being different, or concrete information on what is different between them specifically, then so be it. But I tried and haven't found anything conclusive. >> - 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. > > This is also wrong. just like PAL, there are several variants of SECAM, > one used in France, and a different one in France overseas and on > previous France colonies in Africa and Asia. Eastern Europe also used > different variants of SECAM. This is true. However, those differed only in RF modulation. For example, French SECAM-L used positive video modulation and AM sound, while Eastern European SECAM-D/K used negative video modulation and FM sound. But the baseband composite signals were identical. There were several other variants of SECAM, like early SECAM/V vs. SECAM/H ("Field identification" vs. "Line identification") which moved the color identification signals from VBI to HBI. But that's a change that all SECAM regions, including both France and Eastern Europe did in the 1980s to acommodate for teletext. Again, authoritative sources are scarce, but see e.g. https://web.archive.org/web/20160303232903/http://www.pembers.freeserve.co.uk/World-TV-Standards/Colour-Standards.html (search for "Synchronisation of SECAM colour transmissions" on the page). There's also MESECAM, but that only applies to encoding on VHS and Betamax tapes, not the signals themselves. There was also SECAM-M for 525-line (480i) signals, but I haven't found any conclusive evidence that it was ever used for broadcast anywhere So yeah, SECAM can be a bit confusing, but AFAIK there's only one standard if we're talking about the composite video layer. -- Some *really* old (like, 1960s old) versions of CCIR documents also listed more substantial differences between various 625-line systems, including the number of active lines varying from 571 to 589. But all revisions from 1974 onward list the modern value of 575 active lines for all the variants, making them differ only in RF modulation details. Which is beyond the scope of what the "TV mode" proper
Re: [PATCH v7 22/23] drm/vc4: vec: Add support for more analog TV standards
Hi Maxime, I ran your v7 patchset on my Pi with Xorg, and the mode switching, as well as the preferred mode handling, all work really well now! I just noted that the downstream version of the vc4 driver still has inaccurate field delays in vc4_crtc.c, which causes vertical lines to appear jagged (like here: https://user-images.githubusercontent.com/4499762/112738569-385c3280-8f64-11eb-83c4-d671537af209.png). This has been fixed downstream in https://github.com/raspberrypi/linux/pull/4241/commits/bc093f27bf2613ec93524fdc19e922dd7dd3d800, but I guess that should be upstreamed separately...? Anyway, it's unrelated to the changes made in this patchset, so... I'm not sure if I'm qualified or allowed to do these, but just in case: Tested-by: Mateusz Kwiatkowski (that pretty much applies to parts 19-22 in general, I can respond to those messages as well if you wish) Best regards, Mateusz Kwiatkowski W dniu 7.11.2022 o 15:16, Maxime Ripard pisze: > From: Mateusz Kwiatkowski > > 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. > > Acked-by: Noralf Trønnes > Signed-off-by: Mateusz Kwiatkowski > Signed-off-by: Maxime Ripard > > --- > Changes in v6: > - Support PAL60 again > --- > drivers/gpu/drm/vc4/vc4_vec.c | 111 > -- > 1 file changed, 107 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c > index a828fc6fb776..d23dbad3cbf6 100644 > --- a/drivers/gpu/drm/vc4/vc4_vec.c > +++ b/drivers/gpu/drm/vc4/vc4_vec.c > @@ -46,6 +46,7 @@ > #define VEC_CONFIG0_YDEL(x) ((x) << 26) > #define VEC_CONFIG0_CDEL_MASKGENMASK(25, 24) > #define VEC_CONFIG0_CDEL(x) ((x) << 24) > +#define VEC_CONFIG0_SECAM_STDBIT(21) > #define VEC_CONFIG0_PBPR_FIL BIT(18) > #define VEC_CONFIG0_CHROMA_GAIN_MASK GENMASK(17, 16) > #define VEC_CONFIG0_CHROMA_GAIN_UNITY(0 << 16) > @@ -76,6 +77,27 @@ > #define VEC_SOFT_RESET 0x10c > #define VEC_CLMP0_START 0x144 > #define VEC_CLMP0_END0x148 > + > +/* > + * These set the color subcarrier frequency > + * if VEC_CONFIG1_CUSTOM_FREQ is enabled. > + * > + * VEC_FREQ1_0 contains the most significant 16-bit half-word, > + * VEC_FREQ3_2 contains the least significant 16-bit half-word. > + * 0x8000 seems to be equivalent to the pixel clock > + * (which itself is the VEC clock divided by 8). > + * > + * Reference values (with the default pixel clock of 13.5 MHz): > + * > + * NTSC (3579545.[45] Hz) - 0x21F07C1F > + * PAL (4433618.75 Hz) - 0x2A098ACB > + * PAL-M (3575611.[888111] Hz) - 0x21E6EFE3 > + * PAL-N (3582056.25 Hz) - 0x21F69446 > + * > + * 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. > + */ > #define VEC_FREQ3_2 0x180 > #define VEC_FREQ1_0 0x184 > > @@ -118,6 +140,14 @@ > > #define VEC_INTERRUPT_CONTROL0x190 > #define VEC_INTERRUPT_STATUS 0x194 > + > +/* > + * 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 425 Hz, which corresponds to 0x284BDA13. > + * That is also the default value, so no need to set it explicitly. > + */ > #define VEC_FCW_SECAM_B 0x198 > #define VEC_SECAM_GAIN_VAL 0x19c > > @@ -197,10 +227,15 @@ enum vc4_vec_tv_mode_id { > VC4_VEC_TV_MODE_NTS
Re: [Nouveau] [PATCH v7 22/23] drm/vc4: vec: Add support for more analog TV standards
Hi Lukas, Maxime and everyone, W dniu 8.11.2022 o 14:17, Lukas Satin pisze: > They are important for retrogaming and connecting TV out to CRT TV or using > emulator. > > I have PS1 that is using PAL-60 for example. > > Can you add 240p and 288p non-interlaced modes for NTSC and PAL, please? To add progressive mode support, at least for the VC4/VEC device that's used on the Raspberry Pi, all that's necessary is a patch like: --- a/drivers/gpu/drm/vc4/vc4_vec.c +++ b/drivers/gpu/drm/vc4/vc4_vec.c @@ -623,7 +623,9 @@ static void vc4_vec_encoder_enable(struct drm_encoder *encoder, VEC_WRITE(VEC_CLMP0_START, 0xac); VEC_WRITE(VEC_CLMP0_END, 0xec); VEC_WRITE(VEC_CONFIG2, - VEC_CONFIG2_UV_DIG_DIS | VEC_CONFIG2_RGB_DIG_DIS); + VEC_CONFIG2_UV_DIG_DIS | + VEC_CONFIG2_RGB_DIG_DIS | + ((adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) ? 0 : VEC_CONFIG2_PROG_SCAN)); VEC_WRITE(VEC_CONFIG3, VEC_CONFIG3_HORIZ_LEN_STD); VEC_WRITE(VEC_DAC_CONFIG, vec->variant->dac_config); and then you can just add custom modes, for example within Xorg: xrandr --newmode 720x240 13.5 720 736 800 858 240 243 246 262 xrandr --newmode 720x288 13.5 720 740 804 864 288 290 293 312 Note that the pixel aspect ratio will be all over the place - unfortunately this is necessary at the driver level, because VC4's VEC does not support pixel clocks other than 13.5 MHz. However, you can fix it by running something like "xrandr --scale-from 320x240" or "xrandr --scale-from 384x288". Other (non-X) applications would need to be adapted to similarly configure DRM scaling. I'm not sure if Maxime wants to introduce any more code like the patch above to facilitate progressive scan support, though (@Maxime: feel free to grab the code from above or anything else from https://github.com/raspberrypi/linux/pull/4406 if you do, however!). We talked recently that the priority is to finally merge existing functionality first, see this message: https://lore.kernel.org/dri-devel/20221027115822.5vd3fqlcpy4gfq5v@houat/ I'm willing to post a couple of follow-up patches to improve things like support for progressive modes or exotic TV norms (such as PAL-M-50 or PAL-N-60) within the VC4 driver once this patchset lands - but I agree with Maxime's point to focus on merging existing functionality first. > Lukas Best regards, Mateusz Kwiatkowski > On Mon, Nov 7, 2022 at 3:19 PM Maxime Ripard wrote: > > From: Mateusz Kwiatkowski > > 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. > > Acked-by: Noralf Trønnes > Signed-off-by: Mateusz Kwiatkowski > Signed-off-by: Maxime Ripard > > --- > Changes in v6: > - Support PAL60 again > --- > drivers/gpu/drm/vc4/vc4_vec.c | 111 > -- > 1 file changed, 107 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c > index a828fc6fb776..d23dbad3cbf6 100644 > --- a/drivers/gpu/drm/vc4/vc4_vec.c > +++ b/drivers/gpu/drm/vc4/vc4_vec.c > @@ -46,6 +46,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) > #define VEC_CONFIG0_PBPR_FIL BIT(18) > #define VEC_CONFIG0_CHROMA_GAIN_MASK GENMASK(17, 16) > #define VEC_CONFIG0_CHROMA_GAIN_UNITY (0 << 16) > @@ -76,6 +77,27 @@ > #define VEC_SOFT_RESET 0x10c > #define VEC_CLMP0_START
Re: [Nouveau] [PATCH v7 06/23] drm/modes: Add a function to generate analog display modes
Hi Lukas, W dniu 8.11.2022 o 14:28, Lukas Satin pisze: > Hi, your statement: > > "However, analog display usually have fairly loose timings requirements, > the only discrete parameters being the total number of lines and pixel > clock frequency." > > Please do not make it as a rule. You said yourself: "usually". Arcade CRT > have more loose timings, but professional broadcast TV's such as Sony PVM, > Sony BVM, JVC. These cost tens of thousand dollars back in the day. Now they > are affordable for gamers. I just solved issue in Retroarch, CRT Switchres > library here: https://github.com/antonioginer/switchres/issues/96 I think I'm partially to blame for this wording. See this message and the surrounding thread: https://lore.kernel.org/dri-devel/6d1dfaad-7310-a596-34dd-4a6d9aa95...@gmail.com/ A lot of composite video equipment routinely violated the reference spec. For example, CGA and Apple II output singal that had 15699.76 Hz horizontal sync and 59.92 Hz vertical sync instead of the regular 15734.26 Hz / 59.94 Hz. Values for Famicom/NES are 15745.98 Hz / 60.10 Hz (and the last line is slightly shorter than all other ones at that). And I'm pretty sure these will display just fine on a PVM. Of course you can't just output 14 kHz / 70 Hz and expect it to work, there are constraints of display capabilities. But the point of that discussion, which culminated in the wording you see in the code, was that it does not need to be precise down to every clock cycle as you would normally expect in the digital world. > This model is quite common among retrogamers and on Reddit. > > Some developers do not test it properly. > > This model requires exact number of lines. > > For Switchres we came up with these ranges: > crt_range0 15625-15750, 49.50-65.00, 2.000, 4.700, 8.000, 0.064, > 0.192, 1.024, 1, 1, 192, 288, 0, 0 > crt_range1 15625.00-15625.00, 50.00-50.00, 1.500, 4.700, 5.800, > 0.064, 0.160, 1.056, 1, 1, 0, 0, 448, 576 > crt_range2 15734.26-15734.26, 59.94-59.94, 1.500, 4.700, 4.700, > 0.191, 0.191, 0.953, 1, 1, 0, 0, 448, 480 > crt_range0 is default, more loose definition for MAME emulators. crt_range1 > is PAL and crt_range2 is NTSC. > > Yes, this model does support both NTSC and PAL. > > Does your driver or library support that? > > For example old driver in Windows 7 with NVIDIA 2007 driver on Geforce 7600 > can support both NTSC and PAL and these are being switched automatically by > the resolution you choose. So in desktop properties, you change to 640x480 > and it will switch TV chipset to NTSC 480i. Then you change to 720x576 and it > will switch TV chipset to PAL 576i. > > It would be preferred if advanced users could set up these numbers from a > commandline during a runtime, so it would depend on the app being used. As far as I understand the patch, this is exactly how it works right now. The function you're commenting on is only used for generating the "default" modes. > Lukas Best regards, Mateusz Kwiatkowski > On Mon, Nov 7, 2022 at 3:17 PM Maxime Ripard wrote: > > Multiple drivers (meson, vc4, sun4i) define analog TV 525-lines and > 625-lines modes in their drivers. > > Since those modes are fairly standard, and that we'll need to use them > in more places in the future, it makes sense to move their definition > into the core framework. > > However, analog display usually have fairly loose timings requirements, > the only discrete parameters being the total number of lines and pixel > clock frequency. Thus, we created a function that will create a display > mode from the standard, the pixel frequency and the active area. > > Signed-off-by: Maxime Ripard > > --- > Changes in v6: > - Fix typo > > Changes in v4: > - Reworded the line length check comment > - Switch to HZ_PER_KHZ in tests > - Use previous timing to fill our mode > - Move the number of lines check earlier > --- > drivers/gpu/drm/drm_modes.c| 474 > + > drivers/gpu/drm/tests/Makefile | 1 + > drivers/gpu/drm/tests/drm_modes_test.c | 144 ++ > include/drm/drm_modes.h| 17 ++ > 4 files changed, 636 insertions(+) > > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c > index 5d4ac79381c4..71c050c3ee6b 100644 > --- a/drivers/gpu/drm/drm_modes.c > +++ b/drivers/gpu/drm/drm_modes.c > @@ -116,6 +116,480 @@ void drm_mode_probed_add(struct drm_connector > *connector, > } > EXPORT_SYMBOL(drm_mode_probed_add); > > +enum drm_mode_analog { > + DRM_MODE_ANALOG_NTS
Re: [PATCH v7 15/23] drm/modes: Introduce more named modes
Hi Noralf, W dniu 8.11.2022 o 10:38, Noralf Trønnes pisze: > > Den 07.11.2022 19.03, skrev Noralf Trønnes: >> >> Den 07.11.2022 15.16, skrev Maxime Ripard: >>> Now that we can easily extend the named modes list, let's add a few more >>> analog TV modes that were used in the wild, and some unit tests to make >>> sure it works as intended. >>> >>> Signed-off-by: Maxime Ripard >>> >>> --- >>> Changes in v6: >>> - Renamed the tests to follow DRM test naming convention >>> >>> Changes in v5: >>> - Switched to KUNIT_ASSERT_NOT_NULL >>> --- >>> drivers/gpu/drm/drm_modes.c | 2 + >>> drivers/gpu/drm/tests/drm_client_modeset_test.c | 54 >>> + >>> 2 files changed, 56 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c >>> index 49441cabdd9d..17c5b6108103 100644 >>> --- a/drivers/gpu/drm/drm_modes.c >>> +++ b/drivers/gpu/drm/drm_modes.c >>> @@ -2272,7 +2272,9 @@ struct drm_named_mode { >>> >>> static const struct drm_named_mode drm_named_modes[] = { >>> NAMED_MODE("NTSC", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE, >>> DRM_MODE_TV_MODE_NTSC), >>> + NAMED_MODE("NTSC-J", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE, >>> DRM_MODE_TV_MODE_NTSC_J), >>> NAMED_MODE("PAL", 13500, 720, 576, DRM_MODE_FLAG_INTERLACE, >>> DRM_MODE_TV_MODE_PAL), >>> + NAMED_MODE("PAL-M", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE, >>> DRM_MODE_TV_MODE_PAL_M), >>> }; >> I'm now having second thoughts about the tv_mode commandline option. Can >> we just add all the variants to this table and drop the tv_mode option? >> IMO this will be more user friendly and less confusing. >> > One downside of this is that it's not possible to force connector status > when using named modes, but I think it would be better to have a force > option than a tv_mode option. A lot of userspace treats unknown status > as disconnected. > > Anyone know if it's possible to set the connector status sysfs file > using a udev rule? > > Noralf. I think that leaving named modes only would be a bit limiting. There are use cases for custom modes, e.g. we might want progressive 240p "NTSC" (like 80s/90s home computers and video game consoles) or the modes with non-13.5MHz pixel clock that Geert requested with Amiga in mind. I'm not sure if the current cmdline-to-drm_mode conversion is flexible enough to meaningfully facilitate those, but we're at least getting the syntax down. Best regards, Mateusz Kwiatkowski
Re: [PATCH v6 22/23] drm/vc4: vec: Add support for more analog TV standards
Hi Maxime, I've seen that you've incorporated my PAL60 patch. Thanks! I still yet need to test your v6 changes, but looking at this code with just my mental static analysis, it seems to me that the vc4_vec_encoder_atomic_check() should have the tv_mode validation. I should've added it to the PAL60 patch, but it somehow slipped my mind then. Anyway, I mentioned it previously here: https://lore.kernel.org/dri-devel/0f2beec2-ae8e-5579-f0b6-a73d9dae1...@gmail.com/ It would look something like this, inside vc4_vec_encoder_atomic_check(): + const struct vc4_vec_tv_mode *tv_mode = + vc4_vec_tv_mode_lookup(conn_state->tv.mode); + + if (!tv_mode) + return -EINVAL; Without this, it's possible to set e.g. 480i mode and SECAM, which will fail - but with the current version it will only fail in vc4_vec_encoder_enable(), which cannot return an error, and in my experience that causes a rather lengthy lockup. But, like I said, I still need to actually test that with this version. Anyway, I was also thinking about adding support for the more "exotic" non-standard modes. NTSC-50 is, unfortunately, impossible with VEC, but PAL-N-60 and PAL-M-50 should work. The necessary vc4_vec_tv_modes entries would look something like: @@ -325,12 +325,28 @@ static const struct vc4_vec_tv_mode vc4_vec_tv_modes[] = { .config0 = VEC_CONFIG0_PAL_M_STD, .config1 = VEC_CONFIG1_C_CVBS_CVBS, }, + { + /* PAL-M-50 */ + .mode = DRM_MODE_TV_MODE_PAL, + .expected_htotal = 864, + .config0 = VEC_CONFIG0_PAL_BDGHI_STD, + .config1 = VEC_CONFIG1_C_CVBS_CVBS | VEC_CONFIG1_CUSTOM_FREQ, + .custom_freq = 0x21e6efe3, + }, { .mode = DRM_MODE_TV_MODE_PAL_N, .expected_htotal = 864, .config0 = VEC_CONFIG0_PAL_N_STD, .config1 = VEC_CONFIG1_C_CVBS_CVBS, }, + { + /* PAL-N-60 */ + .mode = DRM_MODE_TV_MODE_PAL_N, + .expected_htotal = 858, + .config0 = VEC_CONFIG0_PAL_M_STD, + .config1 = VEC_CONFIG1_C_CVBS_CVBS | VEC_CONFIG1_CUSTOM_FREQ, + .custom_freq = 0x21f69446, + }, { .mode = DRM_MODE_TV_MODE_SECAM, .expected_htotal = 864, I'm not sure if we actually want to add that. The two arguments for doing so I can think of is 1. it should work, so "why not", 2. it means that more modes will result in _some_ kind of a valid signal, rather than erroring out, which is always a plus in my book. I can also think of a hypothetical use case, like someone in South America with an old PAL-N-only set that would nevertheless still sync at 60 Hz (perhaps with the help of messing with vertical hold knob), who would like to play retro games at 60 Hz in color. But on the other hand, I admit that this scenario is likely a stretch and the number of people who would actually use it is probably close to the proverbial two ;) So it's your call, I'm just leaving those settings here just in case. I'll get back in a couple of days when I do some testing of this v6 patchset. Best regards, Mateusz Kwiatkowski W dniu 26.10.2022 o 17:33, max...@cerno.tech pisze: > From: Mateusz Kwiatkowski > > 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. > > Acked-by: Noralf Trønnes > Signed-off-by: Mateusz Kwiatkowski > Signed-off-by: Maxime Ripard > > --- > Changes in v6: > - Support PAL60 again > --- > drivers/gpu/drm/vc4/vc4_vec.c | 111 > -- > 1 file changed, 107 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c > index 1dda451c8def..d82aef1
Re: [PATCH v6 16/23] drm/probe-helper: Provide a TV get_modes helper
Hi Maxime, First of all, nice idea with the helper function that can be reused by different drivers. This is neat! But looking at this function, it feels a bit overcomplicated. You're creating the two modes, then checking which one is the default, then set the preferred one and possibly reorder them. Maybe it can be simplified somehow? Although when I tried to refactor it myself, I ended up with something that's not better at all. Maybe it needs to be complicated, after all :( Anyway, the current version seems to have a couple of bugs: > + if (tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL) || > + tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL_N) || > + tv_mode_supported(connector, DRM_MODE_TV_MODE_SECAM)) { > + mode = drm_mode_analog_pal_576i(connector->dev); > + if (!mode) > + return 0; > + > + tv_modes[count++] = mode; > + } If the 480i mode has been created properly, but there's an error creating the 576i one (we enter the if (!mode) clause), the 480i mode will leak. > + if (count == 1) { You're handling the count == 1 case specially, but if count == 0, the rest of the code will assume that two modes exist and probably segfault in the process. > + ret = drm_object_property_get_default_value(&connector->base, > + > dev->mode_config.tv_mode_property, > + &default_mode); > + if (ret) > + return 0; > + > + if (cmdline->tv_mode_specified) > + default_mode = cmdline->tv_mode; In case of an error (ret != 0), the modes created so far in the tv_modes array will leak. Also, I wonder if maybe the if (cmdline->tv_mode_specified) clause should go first? If we're going to use the default from cmdline, there's no point in even querying the property default value. Best regards, Mateusz Kwiatkowski
Re: [PATCH v6 14/23] drm/modes: Properly generate a drm_display_mode from a named mode
Hi Maxime, +static struct drm_display_mode *drm_named_mode(struct drm_device *dev, + struct drm_cmdline_mode *cmd) +{ + struct drm_display_mode *mode; + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(drm_named_modes); i++) { + const struct drm_named_mode *named_mode = &drm_named_modes[i]; + + if (strcmp(cmd->name, named_mode->name)) + continue; + + if (!named_mode->tv_mode) + continue; + + mode = drm_analog_tv_mode(dev, + named_mode->tv_mode, + named_mode->pixel_clock_khz * 1000, + named_mode->xres, + named_mode->yres, + named_mode->flags & DRM_MODE_FLAG_INTERLACE); + if (!mode) + return NULL; + + return mode; + } + + return NULL; +} + You didn't add tv_mode_specified to struct drm_named_mode, and left the if (!named_mode->tv_mode) condition here. This will break on NTSC. Best regards, Mateusz Kwiatkowski
Re: [PATCH v5 20/22] drm/vc4: vec: Convert to the new TV mode property
Hi Maxime, W dniu 18.10.2022 o 12:00, Maxime Ripard pisze: > On Mon, Oct 17, 2022 at 12:31:31PM +0200, Noralf Trønnes wrote: >> Den 16.10.2022 20.52, skrev Mateusz Kwiatkowski: >>>> static int vc4_vec_connector_get_modes(struct drm_connector *connector) >>>> { >>>> - struct drm_connector_state *state = connector->state; >>>>struct drm_display_mode *mode; >>>> >>>> - mode = drm_mode_duplicate(connector->dev, >>>> -vc4_vec_tv_modes[state->tv.legacy_mode].mode); >>>> + mode = drm_mode_analog_ntsc_480i(connector->dev); >>>>if (!mode) { >>>>DRM_ERROR("Failed to create a new display mode\n"); >>>>return -ENOMEM; >>>>} >>>> >>>> + mode->type |= DRM_MODE_TYPE_PREFERRED; >>>>drm_mode_probed_add(connector, mode); >>>> >>>> - return 1; >>>> + mode = drm_mode_analog_pal_576i(connector->dev); >>>> + if (!mode) { >>>> + DRM_ERROR("Failed to create a new display mode\n"); >>>> + return -ENOMEM; >>>> + } >>>> + >>>> + drm_mode_probed_add(connector, mode); >>>> + >>>> + return 2; >>>> +} >>> >>> Referencing those previous discussions: >>> - >>> https://lore.kernel.org/dri-devel/0255f7c6-0484-6456-350d-cf24f3fee...@tronnes.org/ >>> - >>> https://lore.kernel.org/dri-devel/c8f8015a-75da-afa8-ca7f-b2b134cac...@gmail.com/ >>> >>> Unconditionally setting the 480i mode as DRM_MODE_TYPE_PREFERRED causes Xorg >>> (at least on current Raspberry Pi OS) to display garbage when >>> video=Composite1:PAL is specified on the command line, so I'm afraid this >>> won't >>> do. >>> >>> As I see it, there are three viable solutions for this issue: >>> >>> a) Somehow query the video= command line option from this function, and set >>>DRM_MODE_TYPE_PREFERRED appropriately. This would break the abstraction >>>provided by global DRM code, but should work fine. >>> >>> b) Modify drm_helper_probe_add_cmdline_mode() so that it sets >>>DRM_MODE_TYPE_PREFERRED in addition to DRM_MODE_TYPE_USERDEF. This seems >>>pretty robust, but affects the entire DRM subsystem, which may break >>>userspace in different ways. >>> >>>- Maybe this could be mitigated by adding some additional conditions, >>> e.g. >>> setting the PREFERRED flag only if no modes are already flagged as such >>> and/or only if the cmdline mode is a named one (~= analog TV mode) >>> >>> c) Forcing userspace (Xorg / Raspberry Pi OS) to get fixed and honor the >>> USERDEF >>>flag. >>> >>> Either way, hardcoding 480i as PREFERRED does not seem right. >>> >> >> My solution for this is to look at tv.mode to know which mode to mark as >> preferred. Maxime didn't like this since it changes things behind >> userspace's back. I don't see how that can cause any problems for userspace. >> >> If userspace uses atomic and sets tv_mode, it has to know which mode to >> use before hand, so it doesn't look at the preferreded flag. >> >> If it uses legacy and sets tv_mode, it can end up with a stale preferred >> flag, but no worse than not having the flag or that ntsc is always >> preferred. >> >> If it doesn't change tv_mode, there's no problem, the preferred flag >> doesn't change. > > I don't like it because I just see no way to make this reliable. When we > set tv_mode, we're not only going to change the preferred flag, but also > the order of the modes to make the preferred mode first. > > Since we just changed the mode lists, we also want to send a hotplug > event to userspace so that it gets notified of it. It will pick up the > new preferred mode, great. > > But what if it doesn't? There's no requirement for userspace to handle > hotplug events, and Kodi won't for example. So we just changed the TV > mode but not the actual mode, and that's it. It's just as broken for > Kodi as it is for X11 right now. > > If we can't get a bullet-proof solution, then I'm not convinced it's > worth addressing. Especially since it's already the current state, and > it doesn't seem to bother a lot of people. I wouldn't rely on the "doesn't seem t
Re: [PATCH] drm/vc4: vec: Add support for PAL-60
Hi Maxime, W dniu 18.10.2022 o 10:31, Maxime Ripard pisze: > Hi, > > On Sun, Oct 16, 2022 at 09:46:49PM +0200, Mateusz Kwiatkowski wrote: >> @@ -308,14 +324,15 @@ static const struct vc4_vec_tv_mode vc4_vec_tv_modes[] >> = { >> }; >> >> static inline const struct vc4_vec_tv_mode * >> -vc4_vec_tv_mode_lookup(unsigned int mode) >> +vc4_vec_tv_mode_lookup(unsigned int mode, u16 htotal) >> { >> unsigned int i; >> >> for (i = 0; i < ARRAY_SIZE(vc4_vec_tv_modes); i++) { >> const struct vc4_vec_tv_mode *tv_mode = &vc4_vec_tv_modes[i]; >> >> -if (tv_mode->mode == mode) >> +if (tv_mode->mode == mode && >> +tv_mode->expected_htotal == htotal) >> return tv_mode; > > Is there any reason we're not using the refresh rate to filter this? It > seems more natural to me. Let me give you an example first. There are actually two ways to configure PAL-60-ish mode on VC4/VEC: a) Modeline 13.5 720 734 798 858 480 487 493 525 Interlace, standard registers set to VEC_CONFIG0_PAL_M_STD, custom frequency enabled and set to 0x2a098acb; Setting the standard registers to "PAL-M" puts the VEC in true 59.94 Hz mode, so the video timings are identical as for NTSC (or PAL-M), and the custom frequency makes the color subcarrier compatible with regular PAL receivers. This is the "true" PAL-60, thanks to the true System M timings. a) Modeline 13.5 720 740 804 864 480 486 492 525 Interlace, standards registers set to VEC_CONFIG0_PAL with standard frequency; This is a "fake" PAL-60 mode, the refresh rate is actually ~59.524 Hz. Most "NTSC" sets will be able to sync with this mode no problem, but the VEC is actually operating in its 50 Hz mode - it's just the "premature" vertical sync signal causes it to output something that is similar to the 525-line system, however strictly speaking non-standard due to lower horizontal sync frequency. This comes down to the fact that: - When VEC's standard registers are set to VEC_CONFIG0_NTSC_STD or VEC_CONFIG0_PAL_M_STD, it operates in the "CCIR System M" mode, expects htotal to be exactly 858 pixels (and it will generate horizontal sync pulse every 858 pixels on its own regardless of what comes out of the PV - so there will be garbage on screen if you set it to anything else), and vtotal to be 525 lines. It will not accept vtotal that's any higher (it will generate its own vertical sync as demanded by System M if not triggered by the PV), but it can be lower - resulting in modes that are non-standard, but otherwise valid. - Likewise, when the registers are set to VEC_CONFIG0_PAL_BDGHI_STD, VEC_CONFIG0_PAL_N_STD or VEC_CONFIG0_SECAM_STD (SECAM is a bit special, but that's irrelevant here), it operates in the "CCIR System B/D/G/H/I/N" mode, and likewise, expects htotal to be exactly 864 pixels (garbage on screen otherwise), vtotal limit is 625 lines, etc. Checking for the refresh rate would only work for standard-compliant modes and have the potential of completely breaking on any custom modes. Conversely, checking for htotal aligns perfectly with the limitations of the hardware, and allows the user to set any modeline that the hardware is able to output with any level of sanity. Footnote: all this information on VEC's behavior comes from my own experimentation, messing around with its registers and seeing what happens (both on screen and on an oscilloscope). I've never seen any Broadcom docs on this chip, so it's by no means official. Best regards, Mateusz Kwiatkowski
[PATCH] drm/vc4: vec: Add support for PAL-60
Add support for the PAL-60 mode. Because there is no separate TV mode property value for PAL-60, this requires matching the settings based on the modeline in addition to just that property alone. Signed-off-by: Mateusz Kwiatkowski --- This patch depends on patch '[PATCH v5 21/22] drm/vc4: vec: Add support for more analog TV standards' submitted by Maxime Ripard (https://lore.kernel.org/dri-devel/20220728-rpi-analog-tv-properties-v5-21-d841cc64f...@cerno.tech/). To Maxime: if you decide to post v6, feel free to include this in your patchset instead if you want. --- drivers/gpu/drm/vc4/vc4_vec.c | 27 --- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c index 88b4330bfa39..bbc41e502cc3 100644 --- a/drivers/gpu/drm/vc4/vc4_vec.c +++ b/drivers/gpu/drm/vc4/vc4_vec.c @@ -235,6 +235,7 @@ enum vc4_vec_tv_mode_id { struct vc4_vec_tv_mode { unsigned int mode; + u16 expected_htotal; u32 config0; u32 config1; u32 custom_freq; @@ -270,37 +271,52 @@ static const struct debugfs_reg32 vec_regs[] = { static const struct vc4_vec_tv_mode vc4_vec_tv_modes[] = { { .mode = DRM_MODE_TV_MODE_NTSC, + .expected_htotal = 858, .config0 = VEC_CONFIG0_NTSC_STD | VEC_CONFIG0_PDEN, .config1 = VEC_CONFIG1_C_CVBS_CVBS, }, { .mode = DRM_MODE_TV_MODE_NTSC_443, + .expected_htotal = 858, .config0 = VEC_CONFIG0_NTSC_STD, .config1 = VEC_CONFIG1_C_CVBS_CVBS | VEC_CONFIG1_CUSTOM_FREQ, .custom_freq = 0x2a098acb, }, { .mode = DRM_MODE_TV_MODE_NTSC_J, + .expected_htotal = 858, .config0 = VEC_CONFIG0_NTSC_STD, .config1 = VEC_CONFIG1_C_CVBS_CVBS, }, { .mode = DRM_MODE_TV_MODE_PAL, + .expected_htotal = 864, .config0 = VEC_CONFIG0_PAL_BDGHI_STD, .config1 = VEC_CONFIG1_C_CVBS_CVBS, }, + { + /* PAL-60 */ + .mode = DRM_MODE_TV_MODE_PAL, + .expected_htotal = 858, + .config0 = VEC_CONFIG0_PAL_M_STD, + .config1 = VEC_CONFIG1_C_CVBS_CVBS | VEC_CONFIG1_CUSTOM_FREQ, + .custom_freq = 0x2a098acb, + }, { .mode = DRM_MODE_TV_MODE_PAL_M, + .expected_htotal = 858, .config0 = VEC_CONFIG0_PAL_M_STD, .config1 = VEC_CONFIG1_C_CVBS_CVBS, }, { .mode = DRM_MODE_TV_MODE_PAL_N, + .expected_htotal = 864, .config0 = VEC_CONFIG0_PAL_N_STD, .config1 = VEC_CONFIG1_C_CVBS_CVBS, }, { .mode = DRM_MODE_TV_MODE_SECAM, + .expected_htotal = 864, .config0 = VEC_CONFIG0_SECAM_STD, .config1 = VEC_CONFIG1_C_CVBS_CVBS, .custom_freq = 0x29c71c72, @@ -308,14 +324,15 @@ static const struct vc4_vec_tv_mode vc4_vec_tv_modes[] = { }; static inline const struct vc4_vec_tv_mode * -vc4_vec_tv_mode_lookup(unsigned int mode) +vc4_vec_tv_mode_lookup(unsigned int mode, u16 htotal) { unsigned int i; for (i = 0; i < ARRAY_SIZE(vc4_vec_tv_modes); i++) { const struct vc4_vec_tv_mode *tv_mode = &vc4_vec_tv_modes[i]; - if (tv_mode->mode == mode) + if (tv_mode->mode == mode && + tv_mode->expected_htotal == htotal) return tv_mode; } @@ -394,6 +411,7 @@ vc4_vec_connector_set_property(struct drm_connector *connector, break; case VC4_VEC_TV_MODE_PAL: + case VC4_VEC_TV_MODE_PAL_60: state->tv.mode = DRM_MODE_TV_MODE_PAL; break; @@ -551,13 +569,16 @@ static void vc4_vec_encoder_enable(struct drm_encoder *encoder, struct drm_connector *connector = &vec->connector; struct drm_connector_state *conn_state = drm_atomic_get_new_connector_state(state, connector); + struct drm_display_mode *adjusted_mode = + &encoder->crtc->state->adjusted_mode; const struct vc4_vec_tv_mode *tv_mode; int idx, ret; if (!drm_dev_enter(drm, &idx)) return; - tv_mode = vc4_vec_tv_mode_lookup(conn_state->tv.mode); + tv_mode = vc4_vec_tv_mode_lookup(conn_state->tv.mode, +adjusted_mode->htotal); if (!tv_mode) goto err_dev_exit; base-commit: e16415e3ddae9abb14a00793554a162403f9af6d -- 2.34.1
Re: [PATCH v5 21/22] drm/vc4: vec: Add support for more analog TV standards
Hi Maxime, W dniu 13.10.2022 o 15:19, Maxime Ripard pisze: > From: Mateusz Kwiatkowski > > 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. The current version actually does not support PAL-60. Proper PAL-60 output from VEC requires configuring it differently than for regular PAL. We have unified the PAL and PAL-60 modes for the "TV mode" property, but the code here has not been adjusted appropriately. I'll try to submit an additional patch that fixes this shortly. > - 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. > > Acked-by: Noralf Trønnes > Signed-off-by: Mateusz Kwiatkowski > Signed-off-by: Maxime Ripard > --- (snip) Best regards, Mateusz Kwiatkowski
Re: [PATCH v5 20/22] drm/vc4: vec: Convert to the new TV mode property
Hi Maxime, Urgh. I cannot send e-mails apparently today, as I removed the second half of the previous message. Here goes: > @@ -454,13 +563,6 @@ static int vc4_vec_encoder_atomic_check(struct > drm_encoder *encoder, > struct drm_connector_state *conn_state) > { > const struct drm_display_mode *mode = &crtc_state->adjusted_mode; You could add here something like: + const struct vc4_vec_tv_mode *tv_mode = + vc4_vec_tv_mode_lookup(conn_state->tv.mode); + + if (!tv_mode) + return -EINVAL; This should explicitly make it impossible to enter the equivalent condition in vc4_vec_encoder_enable() that causes the problem mentioned in the previous e-mail. This is probably basically impossible already, but I triggered that when testing a follow-up change I'd like to post shortly. > - const struct vc4_vec_tv_mode *vec_mode; > - > - vec_mode = &vc4_vec_tv_modes[conn_state->tv.legacy_mode]; > - > - if (conn_state->crtc && > - !drm_mode_equal(vec_mode->mode, &crtc_state->adjusted_mode)) > - return -EINVAL; If you're removing the reference mode, then I think you should at least add checks that the crtc_clock is set to 13.5 MHz (it's otherwise ignored) and that crtc_htotal is either 858 or 864 (using a switch over reference_mode->htotal as I proposed in my comment to patch 19/22 would double as such check), as all other values causes VEC to output garbage. Best regards, Mateusz Kwiatkowski
Re: [PATCH v5 20/22] drm/vc4: vec: Convert to the new TV mode property
Hi Maxime, > static int vc4_vec_connector_get_modes(struct drm_connector *connector) > { > - struct drm_connector_state *state = connector->state; > struct drm_display_mode *mode; > > - mode = drm_mode_duplicate(connector->dev, > - vc4_vec_tv_modes[state->tv.legacy_mode].mode); > + mode = drm_mode_analog_ntsc_480i(connector->dev); > if (!mode) { > DRM_ERROR("Failed to create a new display mode\n"); > return -ENOMEM; > } > > + mode->type |= DRM_MODE_TYPE_PREFERRED; > drm_mode_probed_add(connector, mode); > > - return 1; > + mode = drm_mode_analog_pal_576i(connector->dev); > + if (!mode) { > + DRM_ERROR("Failed to create a new display mode\n"); > + return -ENOMEM; > + } > + > + drm_mode_probed_add(connector, mode); > + > + return 2; > +} Referencing those previous discussions: - https://lore.kernel.org/dri-devel/0255f7c6-0484-6456-350d-cf24f3fee...@tronnes.org/ - https://lore.kernel.org/dri-devel/c8f8015a-75da-afa8-ca7f-b2b134cac...@gmail.com/ Unconditionally setting the 480i mode as DRM_MODE_TYPE_PREFERRED causes Xorg (at least on current Raspberry Pi OS) to display garbage when video=Composite1:PAL is specified on the command line, so I'm afraid this won't do. As I see it, there are three viable solutions for this issue: a) Somehow query the video= command line option from this function, and set DRM_MODE_TYPE_PREFERRED appropriately. This would break the abstraction provided by global DRM code, but should work fine. b) Modify drm_helper_probe_add_cmdline_mode() so that it sets DRM_MODE_TYPE_PREFERRED in addition to DRM_MODE_TYPE_USERDEF. This seems pretty robust, but affects the entire DRM subsystem, which may break userspace in different ways. - Maybe this could be mitigated by adding some additional conditions, e.g. setting the PREFERRED flag only if no modes are already flagged as such and/or only if the cmdline mode is a named one (~= analog TV mode) c) Forcing userspace (Xorg / Raspberry Pi OS) to get fixed and honor the USERDEF flag. Either way, hardcoding 480i as PREFERRED does not seem right. Note: this also applies to the sun4i version (patch 22/22). > @@ -366,13 +472,16 @@ static void vc4_vec_encoder_enable(struct drm_encoder > *encoder, > struct drm_connector *connector = &vec->connector; > struct drm_connector_state *conn_state = > drm_atomic_get_new_connector_state(state, connector); > - const struct vc4_vec_tv_mode *tv_mode = > - &vc4_vec_tv_modes[conn_state->tv.legacy_mode]; > + const struct vc4_vec_tv_mode *tv_mode; > int idx, ret; > > if (!drm_dev_enter(drm, &idx)) > return; > > + tv_mode = vc4_vec_tv_mode_lookup(conn_state->tv.mode); > + if (!tv_mode) > + goto err_dev_exit; > + > ret = pm_runtime_get_sync(&vec->pdev->dev); > if (ret < 0) { > DRM_ERROR("Failed to retain power domain: %d\n", ret); If this (!tv_mode) condition is somehow triggered, the power management goes somewhat crazy. vc4_vec_encoder_enable() cannot return an error, so when vc4_vec_encoder_disable() is eventually called after a failed enable, it hangs in pm_runtime_put() for quite a bit. At least I think that's what's happening. Anyway, to solve this, I'd propose this thing below: Best regards, Mateusz Kwiatkowski
Re: [PATCH v5 19/22] drm/vc4: vec: Check for VEC output constraints
Hi Maxime, Sorry about the mess that happened to the previous message. I hope this one will be delivered more cleanly. W dniu 13.10.2022 o 15:19, Maxime Ripard pisze: > From: Mateusz Kwiatkowski > > The VEC can accept pretty much any relatively reasonable mode, but still > has a bunch of constraints to meet. > > Let's create an atomic_check() implementation that will make sure we > don't end up accepting a non-functional mode. > > Acked-by: Noralf Trønnes > Signed-off-by: Mateusz Kwiatkowski > Signed-off-by: Maxime Ripard > --- > drivers/gpu/drm/vc4/vc4_vec.c | 48 >+++ > 1 file changed, 48 insertions(+) > > diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c > index 90e375a8a8f9..1fcb7baf874e 100644 > --- a/drivers/gpu/drm/vc4/vc4_vec.c > +++ b/drivers/gpu/drm/vc4/vc4_vec.c > @@ -453,6 +453,7 @@ static int vc4_vec_encoder_atomic_check(struct > drm_encoder *encoder, > struct drm_crtc_state *crtc_state, > struct drm_connector_state *conn_state) > { > + const struct drm_display_mode *mode = &crtc_state->adjusted_mode; > const struct vc4_vec_tv_mode *vec_mode; > > vec_mode = &vc4_vec_tv_modes[conn_state->tv.legacy_mode]; > @@ -461,6 +462,53 @@ static int vc4_vec_encoder_atomic_check(struct > drm_encoder *encoder, > !drm_mode_equal(vec_mode->mode, &crtc_state->adjusted_mode)) > return -EINVAL; > > + if (mode->crtc_hdisplay % 4) > + return -EINVAL; > + > + if (!(mode->crtc_hsync_end - mode->crtc_hsync_start)) > + return -EINVAL; > + > + switch (mode->vtotal) { > + case 525: > + if (mode->crtc_vtotal > 262) > + return -EINVAL; > + > + if (mode->crtc_vdisplay < 1 || mode->crtc_vdisplay > 253) > + return -EINVAL; > + > + if (!(mode->crtc_vsync_start - mode->crtc_vdisplay)) > + return -EINVAL; > + > + if ((mode->crtc_vsync_end - mode->crtc_vsync_start) != 3) > + return -EINVAL; > + > + if ((mode->crtc_vtotal - mode->crtc_vsync_end) < 4) > + return -EINVAL; > + > + break; > + > + case 625: > + if (mode->crtc_vtotal > 312) > + return -EINVAL; > + > + if (mode->crtc_vdisplay < 1 || mode->crtc_vdisplay > 305) > + return -EINVAL; > + > + if (!(mode->crtc_vsync_start - mode->crtc_vdisplay)) > + return -EINVAL; > + > + if ((mode->crtc_vsync_end - mode->crtc_vsync_start) != 3) > + return -EINVAL; > + > + if ((mode->crtc_vtotal - mode->crtc_vsync_end) < 2) > + return -EINVAL; > + > + break; > + > + default: > + return -EINVAL; > + } > + > return 0; > } > > In my original version of this function (https://github.com/raspberrypi/linux/pull/4406/files) the switch is over reference_mode->vtotal, not mode->vtotal. This was intended to explicitly allow a different value of mode->vtotal, to support non-standard modes, such as "fake" 525 lines with SECAM encoding, or the progressive modes. You're switching over mode->vtotal, which makes specifying those impossible. I don't think we should limit the users like that. We're removing reference_mode in patch 20/22, so adding a switch over reference_mode->vtotal is probably not a good idea -- in that case I'd switch over mode->htotal instead: 858 for "NTSC" and 864 for "PAL". This may seem a bit weird, but any other value of htotal causes the VEC to output garbage anyway. Best regards, Mateusz Kwiatkowski
Re: [PATCH v5 19/22] drm/vc4: vec: Check for VEC output constraints
Hi Maxime, W dniu 13.10.2022 o 15:19, Maxime Ripard pisze: > From: Mateusz Kwiatkowski > > The VEC can accept pretty much any relatively > reasonable mode, but still > has a bunch of constraints to meet. > > Let's > create an atomic_check() implementation that will make sure we > don't end up > accepting a non-functional mode. > > Acked-by: Noralf Trønnes > > Signed-off-by: Mateusz Kwiatkowski > Signed-off-by: Maxime Ripard > --- > > drivers/gpu/drm/vc4/vc4_vec.c | 48 > +++ > 1 file changed, 48 > insertions(+) > > diff --git a/drivers/gpu/drm/vc4/vc4_vec.c > b/drivers/gpu/drm/vc4/vc4_vec.c > index 90e375a8a8f9..1fcb7baf874e 100644 > > --- a/drivers/gpu/drm/vc4/vc4_vec.c > +++ b/drivers/gpu/drm/vc4/vc4_vec.c > > @@ -453,6 +453,7 @@ static int vc4_vec_encoder_atomic_check(struct > drm_encoder *encoder, > struct drm_crtc_state *crtc_state, > struct > drm_connector_state *conn_state) > { > + const struct drm_display_mode *mode > = &crtc_state->adjusted_mode; > const struct vc4_vec_tv_mode *vec_mode; > > > vec_mode = &vc4_vec_tv_modes[conn_state->tv.legacy_mode]; > @@ -461,6 +462,53 @@ static int vc4_vec_encoder_atomic_check(struct drm_encoder *encoder, > !drm_mode_equal(vec_mode->mode, &crtc_state->adjusted_mode)) > return -EINVAL; > > + if (mode->crtc_hdisplay % 4) > + return -EINVAL; > + > + if (!(mode->crtc_hsync_end - mode->crtc_hsync_start)) > + return -EINVAL; > + > + switch (mode->vtotal) { > + case 525: > + if (mode->crtc_vtotal > 262) > + return -EINVAL; > + > + if (mode->crtc_vdisplay < 1 || mode->crtc_vdisplay > 253) > + return -EINVAL; > + > + if (!(mode->crtc_vsync_start - mode->crtc_vdisplay)) > + return -EINVAL; > + > + if ((mode->crtc_vsync_end - mode->crtc_vsync_start) != 3) > + return -EINVAL; > + > + if ((mode->crtc_vtotal - mode->crtc_vsync_end) < 4) > + return -EINVAL; > + > + break; > + > + case 625: > + if (mode->crtc_vtotal > 312) > + return -EINVAL; > + > + if (mode->crtc_vdisplay < 1 || mode->crtc_vdisplay > 305) > + return -EINVAL; > + > + if (!(mode->crtc_vsync_start - mode->crtc_vdisplay)) > + return -EINVAL; > + > + if ((mode->crtc_vsync_end - mode->crtc_vsync_start) != 3) > + return -EINVAL; > + > + if ((mode->crtc_vtotal - mode->crtc_vsync_end) < 2) > + return -EINVAL; > + > + break; > + > + default: > + return -EINVAL; > + } > + > return 0; > } In my original version of this function (https://github.com/raspberrypi/linux/pull/4406/files) the switch is over reference_mode->vtotal, not mode->vtotal. This was intended to explicitly allow a different value of mode->vtotal, to support non-standard modes, such as "fake" 525 lines with SECAM encoding, or the progressive modes. You're switching over mode->vtotal, which makes specifying those impossible. I don't think we should limit the users like that. We're removing reference_mode in patch 20/22, so adding a switch over reference_mode->vtotal is probably not a good idea -- in that case I'd switch over mode->htotal instead: 858 for "NTSC" and 864 for "PAL". This may seem a bit weird, but any other value of htotal causes the VEC to output garbage anyway. Best regards, Mateusz Kwiatkowski
Re: [PATCH v5 13/22] drm/modes: Introduce the tv_mode property as a command-line option
Hi Maxime, Noralf & everyone, I'd like to address Noralf here in particular, and refer to these discussions from the past: - https://lore.kernel.org/linux-arm-kernel/2f607c7d-6da1-c8df-1c02-8dd344a92...@gmail.com/ - https://lore.kernel.org/linux-arm-kernel/9e76a508-f469-a54d-ecd7-b5868ca99...@tronnes.org/ > @@ -2230,20 +2256,22 @@ struct drm_named_mode { > unsigned int xres; > unsigned int yres; > unsigned int flags; > + unsigned int tv_mode; > }; I saw that you (Noralf) opposed my suggestion about the DRM_MODE_TV_MODE_NONE enum value in enum drm drm_connector_tv_mode. I get your argumentation, and I'm not gonna argue, but I still don't like the fact that struct drm_named_mode now includes a field that is only relevant for analog TV modes, has no "none" value, and yet the type is supposed to be generic enough to be usable for other types of outputs as well. It's true that it can just be ignored (as Maxime mentioned in his response to my e-mail linked above), and now the value of 0 corresponds to DRM_MODE_TV_MODE_NTSC, which is a rather sane default, but it still feels messy to me. I'm not gonna force my opinion here, but I wanted to bring your attention to this issue, maybe you have some other solution in mind for this problem. Or if you don't see that as a problem at all, that's fine, too. Best regards, Mateusz Kwiatkowski
Re: [PATCH v5 06/22] drm/modes: Add a function to generate analog display modes
gt; +* we're one line short, so we'll make up for > +* it here by using 3. > +* > +* The entire blanking area is supposed to > +* take 25 lines, so we also need to account > +* for the rest of the blanking area that > +* can't be in either the front porch or sync > +* period. > +*/ > + PARAM_FIELD(19, 20)), > +}; Nit: setting vbp limits like that makes it impossible to use drm_analog_tv_mode() to generate modes that include the VBI for e.g. emitting teletext. This probably doesn't matter, as it can still be created as a custom mode from userspace, hence I'm mentioning it as a nit. > + * By convention, NSTC (aka 525/60) systems start with Typo: s/NSTC/NTSC/ Best regards, Mateusz Kwiatkowski
Re: [PATCH v2 10/41] drm/modes: Add a function to generate analog display modes
> 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. I meant "from the end of the sync pulse", obviously. Sorry about the slipup.
Re: [PATCH v2 10/41] drm/modes: Add a function to generate analog display modes
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
Re: [PATCH v2 09/41] drm/connector: Add TV standard property
W dniu 9.09.2022 o 11:46, Maxime Ripard pisze: > On Wed, Sep 07, 2022 at 09:52:09PM +0200, Mateusz Kwiatkowski wrote: >> W dniu 7.09.2022 o 14:10, Maxime Ripard pisze: >>> Hi, >>> >>> On Fri, Sep 02, 2022 at 12:00:33AM +0200, Mateusz Kwiatkowski wrote: >>>> W dniu 29.08.2022 o 15:11, Maxime Ripard pisze: >>>>> The TV mode property has been around for a while now to select and get the >>>>> current TV mode output on an analog TV connector. >>>>> >>>>> Despite that property name being generic, its content isn't and has been >>>>> driver-specific which makes it hard to build any generic behaviour on top >>>>> of it, both in kernel and user-space. >>>>> >>>>> Let's create a new bitmask tv norm property, that can contain any of the >>>>> analog TV standards currently supported by kernel drivers. Each driver can >>>>> then pass in a bitmask of the modes it supports. >>>> >>>> This is not a bitmask property anymore, you've just changed it to an enum. >>>> The commit message is now misleading. >>>> >>>>> +static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = { >>>>> + { DRM_MODE_TV_MODE_NTSC_443, "NTSC-443" }, >>>>> + { DRM_MODE_TV_MODE_NTSC_J, "NTSC-J" }, >>>>> + { DRM_MODE_TV_MODE_NTSC_M, "NTSC-M" }, >>>>> + { DRM_MODE_TV_MODE_PAL_60, "PAL-60" }, >>>>> + { DRM_MODE_TV_MODE_PAL_B, "PAL-B" }, >>>>> + { DRM_MODE_TV_MODE_PAL_D, "PAL-D" }, >>>>> + { DRM_MODE_TV_MODE_PAL_G, "PAL-G" }, >>>>> + { DRM_MODE_TV_MODE_PAL_H, "PAL-H" }, >>>>> + { DRM_MODE_TV_MODE_PAL_I, "PAL-I" }, >>>>> + { DRM_MODE_TV_MODE_PAL_M, "PAL-M" }, >>>>> + { DRM_MODE_TV_MODE_PAL_N, "PAL-N" }, >>>>> + { DRM_MODE_TV_MODE_PAL_NC, "PAL-Nc" }, >>>>> + { DRM_MODE_TV_MODE_SECAM_60, "SECAM-60" }, >>>>> + { DRM_MODE_TV_MODE_SECAM_B, "SECAM-B" }, >>>>> + { DRM_MODE_TV_MODE_SECAM_D, "SECAM-D" }, >>>>> + { DRM_MODE_TV_MODE_SECAM_G, "SECAM-G" }, >>>>> + { DRM_MODE_TV_MODE_SECAM_K, "SECAM-K" }, >>>>> + { DRM_MODE_TV_MODE_SECAM_K1, "SECAM-K1" }, >>>>> + { DRM_MODE_TV_MODE_SECAM_L, "SECAM-L" }, >>>>> +}; >>>> >>>> I did not comment on it the last time, but this list looks a little bit >>>> random. >>>> >>>> Compared to the standards defined by V4L2, you also define SECAM-60 (a good >>>> thing to define, because why not), but don't define PAL-B1, PAL-D1, PAL-K, >>>> SECAM-H, SECAM-LC (whatever that is - probably just another name for >>>> SECAM-L, >>>> see my comment about PAL-Nc below), or NTSC-M-KR (a Korean variant of >>>> NTSC). >>>> >>>> Like I mentioned previously, I'm personally not a fan of including all >>>> those >>>> CCIR/ITU system variants, as they don't mean any difference to the output >>>> unless >>>> there is an RF modulator involved. But I get it that they have already >>>> been used >>>> and regressing probably wouldn't be a very good idea. But in that case >>>> keeping >>>> it consistent with the set of values used by V4L2 would be wise, I think. >>> >>> Ack. What would be the list of standards we'd absolutely need? NSTC-M, >>> NTSC-J, PAL-60, PAL-B, PAL-M, SECAM-60 and SECAM-B? >> >> The "essential list" IMO is NTSC, NTSC-J, NTSC-443, PAL, PAL-M, PAL-N and >> SECAM. >> Note that: >> >> - I intentionally propose "NTSC", "PAL" and "SECAM" without an ITU system >> designation. If we only consider composite signals, there's no difference >> between e.g. PAL-B, PAL-D and PAL-I, so it's better to label it as a >>generic >> mode, IMO. >> >> * PAL-M and PAL-N are different, because those unique color encodings were >> only ever used with Systems M and N, respectively. >> >> * NTSC-J is also different, because "System J" doesn't exist anywhere in >>ITU >> documents. Japan technically uses System M with a non-standard black >>level. >> But
Re: [PATCH v2 32/41] drm/vc4: vec: Convert to the new TV mode property
Hi Maxime, W dniu 08.09.2022 o 13:23, Maxime Ripard pisze: > Hi Noralf, > > On Tue, Aug 30, 2022 at 09:01:08PM +0200, Noralf Trønnes wrote: >>> +static const struct drm_prop_enum_list tv_mode_names[] = { >> >> Maybe call it legacy_tv_mode_enums? >> >>> >>> + { VC4_VEC_TV_MODE_NTSC, "NTSC", }, >>> >>> + { VC4_VEC_TV_MODE_NTSC_J, "NTSC-J", }, >>> >>> + { VC4_VEC_TV_MODE_PAL, "PAL", }, >>> >>> + { VC4_VEC_TV_MODE_PAL_M, "PAL-M", }, >> >> If you use DRM_MODE_TV_MODE_* here you don't need to translate the value >> using the switch statement in get/set property, you can use the value >> directly to get/set tv.mode. > > I'm sorry, I'm not quite sure what you mean by that. If we expose the > DRM_MODE_TV_MODE_* properties there, won't that change the values the > userspace will need to use to set that property? I'd just like to point out that if numerical values of these enums are your concern, then you're (or perhaps I am ;) already breaking this by adding new modes in patch 33/41 in this series. And the values (and names!) added by that patch (33/41) don't match those currently used by the downstream version (https://github.com/raspberrypi/linux/blob/rpi-5.15.y/drivers/gpu/drm/vc4/vc4_vec.c). If any userspace software is manipulating this property, it's most likely targeting the downstream code. But since you're not aiming for consistency with that, I was under the impression that compatibility isn't a concern. Best regards, Mateusz Kwiatkowski
Re: [PATCH v2 10/41] drm/modes: Add a function to generate analog display modes
Hi Maxime, 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 - 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 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). 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. Best regards, Mateusz Kwiatkowski
Re: [PATCH v2 09/41] drm/connector: Add TV standard property
Hi Maxime, W dniu 7.09.2022 o 14:10, Maxime Ripard pisze: > Hi, > > On Fri, Sep 02, 2022 at 12:00:33AM +0200, Mateusz Kwiatkowski wrote: >> W dniu 29.08.2022 o 15:11, Maxime Ripard pisze: >>> The TV mode property has been around for a while now to select and get the >>> current TV mode output on an analog TV connector. >>> >>> Despite that property name being generic, its content isn't and has been >>> driver-specific which makes it hard to build any generic behaviour on top >>> of it, both in kernel and user-space. >>> >>> Let's create a new bitmask tv norm property, that can contain any of the >>> analog TV standards currently supported by kernel drivers. Each driver can >>> then pass in a bitmask of the modes it supports. >> >> This is not a bitmask property anymore, you've just changed it to an enum. >> The commit message is now misleading. >> >>> +static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = { >>> + { DRM_MODE_TV_MODE_NTSC_443, "NTSC-443" }, >>> + { DRM_MODE_TV_MODE_NTSC_J, "NTSC-J" }, >>> + { DRM_MODE_TV_MODE_NTSC_M, "NTSC-M" }, >>> + { DRM_MODE_TV_MODE_PAL_60, "PAL-60" }, >>> + { DRM_MODE_TV_MODE_PAL_B, "PAL-B" }, >>> + { DRM_MODE_TV_MODE_PAL_D, "PAL-D" }, >>> + { DRM_MODE_TV_MODE_PAL_G, "PAL-G" }, >>> + { DRM_MODE_TV_MODE_PAL_H, "PAL-H" }, >>> + { DRM_MODE_TV_MODE_PAL_I, "PAL-I" }, >>> + { DRM_MODE_TV_MODE_PAL_M, "PAL-M" }, >>> + { DRM_MODE_TV_MODE_PAL_N, "PAL-N" }, >>> + { DRM_MODE_TV_MODE_PAL_NC, "PAL-Nc" }, >>> + { DRM_MODE_TV_MODE_SECAM_60, "SECAM-60" }, >>> + { DRM_MODE_TV_MODE_SECAM_B, "SECAM-B" }, >>> + { DRM_MODE_TV_MODE_SECAM_D, "SECAM-D" }, >>> + { DRM_MODE_TV_MODE_SECAM_G, "SECAM-G" }, >>> + { DRM_MODE_TV_MODE_SECAM_K, "SECAM-K" }, >>> + { DRM_MODE_TV_MODE_SECAM_K1, "SECAM-K1" }, >>> + { DRM_MODE_TV_MODE_SECAM_L, "SECAM-L" }, >>> +}; >> >> I did not comment on it the last time, but this list looks a little bit >> random. >> >> Compared to the standards defined by V4L2, you also define SECAM-60 (a good >> thing to define, because why not), but don't define PAL-B1, PAL-D1, PAL-K, >> SECAM-H, SECAM-LC (whatever that is - probably just another name for SECAM-L, >> see my comment about PAL-Nc below), or NTSC-M-KR (a Korean variant of NTSC). >> >> Like I mentioned previously, I'm personally not a fan of including all those >> CCIR/ITU system variants, as they don't mean any difference to the output >> unless >> there is an RF modulator involved. But I get it that they have already been >> used >> and regressing probably wouldn't be a very good idea. But in that case >> keeping >> it consistent with the set of values used by V4L2 would be wise, I think. > > Ack. What would be the list of standards we'd absolutely need? NSTC-M, > NTSC-J, PAL-60, PAL-B, PAL-M, SECAM-60 and SECAM-B? The "essential list" IMO is NTSC, NTSC-J, NTSC-443, PAL, PAL-M, PAL-N and SECAM. Note that: - I intentionally propose "NTSC", "PAL" and "SECAM" without an ITU system designation. If we only consider composite signals, there's no difference between e.g. PAL-B, PAL-D and PAL-I, so it's better to label it as a generic mode, IMO. * PAL-M and PAL-N are different, because those unique color encodings were only ever used with Systems M and N, respectively. * NTSC-J is also different, because "System J" doesn't exist anywhere in ITU documents. Japan technically uses System M with a non-standard black level. But "NTSC-J" stuck as a universally recognized name for that variant. - I intentionally did not list PAL-60 or SECAM-60. TBH... PAL-60 is just regular PAL paired with 480i60 modeline. Most if not all displays that accept PAL-60 input will just label it as "PAL". If we are not introducing strict modeline validation, then maybe separating PAL and PAL-60 isn't really necessary? Same goes for SECAM vs. SECAM-60. ...and same goes for NTSC vs. NTSC-50 a.k.a NTSC-N, which is a very exotic mode, but known to exist at least in the Atari ST world, see also: https://en.wikipedia.org/wiki/NTSC#NTSC-N/NTSC50 Combining PAL and PAL-60 into a single setting would complicate the vc4 driver a little bit, though, as the registers need to be set up differently f
Re: [PATCH v2 10/41] drm/modes: Add a function to generate analog display modes
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") - If even more active lines are specified, once the limit for vfp is hit, then decrease vbp only - the resulting image will definitely be off-center, but there's no other way I hope this makes sense for you as well. Best regards, Mateusz Kwiatkowski
Re: [PATCH v2 10/41] drm/modes: Add a function to generate analog display modes
Hi Maxime, W dniu 5.09.2022 o 15:32, Maxime Ripard pisze: > Hi, > > On Wed, Aug 31, 2022 at 10:14:28AM +0200, Geert Uytterhoeven wrote: >>>> +enum drm_mode_analog { >>>> + DRM_MODE_ANALOG_NTSC, >>>> + DRM_MODE_ANALOG_PAL, >>>> +}; >>> >>> Using "NTSC" and "PAL" to describe the 50Hz and 60Hz analog TV modes is >>> common, >>> but strictly speaking a misnomer. Those are color encoding systems, and your >>> patchset fully supports lesser used, but standard encodings for those (e.g. >>> PAL-M for 60Hz and SECAM for 50Hz). I'd propose switching to some more >>> neutral >>> naming scheme. Some ideas: >>> >>> - DRM_MODE_ANALOG_60_HZ / DRM_MODE_ANALOG_50_HZ (after standard refresh >>> rate) >>> - DRM_MODE_ANALOG_525_LINES / DRM_MODE_ANALOG_625_LINES (after standard line >>> count) >> >> IMHO these are bad names, as e.g. VGA640x480@60 is also analog, using >> 60 Hz and 525 lines. Add "TV" to the name? >> >>> - DRM_MODE_ANALOG_JM / DRM_MODE_ANALOG_BDGHIKLN (after corresponding ITU >>> System >>> Letter Designations) >> >> Or DRM_MODE_ITU_*? >> But given the long list of letters, this looks fragile to me. > > Does it matter at all? It's an internal API that isn't exposed at all. > I'd rather have a common name that everyone can understand in this case > rather than a *perfect* name where most will scratch their head > wondering what it's about. You may have a point. But in that case, maybe it'd make sense to at least add a short comment explaining what do you mean by "NTSC" and "PAL" in this context? Best regards, Mateusz Kwiatkowski
Re: [PATCH v2 20/41] drm/modes: Properly generate a drm_display_mode from a named mode
Hi Maxime, > + if (!named_mode->tv_mode) > + continue; As mentioned in the previous email replying to 19/41, this makes it impossible to specify DRM_MODE_TV_MODE_NTSC_443 as currently defined in the named mode successfully. Best regards, Mateusz Kwiatkowski
Re: [PATCH v2 19/41] drm/modes: Introduce the tv_mode property as a command-line option
Hi Maxime, > @@ -2212,20 +2239,22 @@ struct drm_named_mode { > unsigned int xres; > unsigned int yres; > unsigned int flags; > + unsigned int tv_mode; > }; Are _all_ named modes supposed to be about analog TV? If so, then probably this structure should be renamed drm_named_analog_tv_mode or something. If not, then including tv_mode in all of them sounds almost dangrous. 0 is a valid value for enum drm_connector_tv_mode, corresponding to DRM_MODE_TV_MODE_NTSC_443. This is a very weird default (maybe it shouldn't be the one that has a numeric value of 0?) and if there ever is a named mode that is not related to analog TV, it looks that it will refer to NTSC-443. Not sure where could that actually propagate, and maybe what I'm saying can't happen, but I'm imagining weird scenarios where a GPU that has both a VGA/HDMI/whatever output, and a composite output, switches to NTSC-443 on the composite output by default because a named mode for the modern output is selected. Maybe something like DRM_MODE_TV_MODE_NONE = 0 would make sense? Maybe not. This is not an actual suggestion, just "thinking out loud". Best regards, Mateusz Kwiatkowski
Re: [PATCH v2 09/41] drm/connector: Add TV standard property
Hi Maxime, W dniu 29.08.2022 o 15:11, Maxime Ripard pisze: > The TV mode property has been around for a while now to select and get the > current TV mode output on an analog TV connector. > > Despite that property name being generic, its content isn't and has been > driver-specific which makes it hard to build any generic behaviour on top > of it, both in kernel and user-space. > > Let's create a new bitmask tv norm property, that can contain any of the > analog TV standards currently supported by kernel drivers. Each driver can > then pass in a bitmask of the modes it supports. This is not a bitmask property anymore, you've just changed it to an enum. The commit message is now misleading. > +static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = { > + { DRM_MODE_TV_MODE_NTSC_443, "NTSC-443" }, > + { DRM_MODE_TV_MODE_NTSC_J, "NTSC-J" }, > + { DRM_MODE_TV_MODE_NTSC_M, "NTSC-M" }, > + { DRM_MODE_TV_MODE_PAL_60, "PAL-60" }, > + { DRM_MODE_TV_MODE_PAL_B, "PAL-B" }, > + { DRM_MODE_TV_MODE_PAL_D, "PAL-D" }, > + { DRM_MODE_TV_MODE_PAL_G, "PAL-G" }, > + { DRM_MODE_TV_MODE_PAL_H, "PAL-H" }, > + { DRM_MODE_TV_MODE_PAL_I, "PAL-I" }, > + { DRM_MODE_TV_MODE_PAL_M, "PAL-M" }, > + { DRM_MODE_TV_MODE_PAL_N, "PAL-N" }, > + { DRM_MODE_TV_MODE_PAL_NC, "PAL-Nc" }, > + { DRM_MODE_TV_MODE_SECAM_60, "SECAM-60" }, > + { DRM_MODE_TV_MODE_SECAM_B, "SECAM-B" }, > + { DRM_MODE_TV_MODE_SECAM_D, "SECAM-D" }, > + { DRM_MODE_TV_MODE_SECAM_G, "SECAM-G" }, > + { DRM_MODE_TV_MODE_SECAM_K, "SECAM-K" }, > + { DRM_MODE_TV_MODE_SECAM_K1, "SECAM-K1" }, > + { DRM_MODE_TV_MODE_SECAM_L, "SECAM-L" }, > +}; I did not comment on it the last time, but this list looks a little bit random. Compared to the standards defined by V4L2, you also define SECAM-60 (a good thing to define, because why not), but don't define PAL-B1, PAL-D1, PAL-K, SECAM-H, SECAM-LC (whatever that is - probably just another name for SECAM-L, see my comment about PAL-Nc below), or NTSC-M-KR (a Korean variant of NTSC). Like I mentioned previously, I'm personally not a fan of including all those CCIR/ITU system variants, as they don't mean any difference to the output unless there is an RF modulator involved. But I get it that they have already been used and regressing probably wouldn't be a very good idea. But in that case keeping it consistent with the set of values used by V4L2 would be wise, I think. > +/** > + * drm_mode_create_tv_properties - create TV specific connector properties > + * @dev: DRM device > + * @supported_tv_modes: Bitmask of TV modes supported (See > DRM_MODE_TV_MODE_*) > + > + * Called by a driver's TV initialization routine, this function creates > + * the TV specific connector properties for a given device. Caller is > + * responsible for allocating a list of format names and passing them to > + * this routine. > + * > + * Returns: > + * 0 on success or a negative error code on failure. > + */ > +int drm_mode_create_tv_properties(struct drm_device *dev, > + unsigned int supported_tv_modes) supported_tv_modes is supposed to be a bitmask of BIT(DRM_MODE_TV_MODE_*) (or (1< + /** > + * @DRM_MODE_TV_MODE_PAL_NC: Seems equivalent to > + * @DRM_MODE_TV_MODE_PAL_N. > + */ > + DRM_MODE_TV_MODE_PAL_NC, AFAIK, the entire reason that "PAL-Nc" is ever mentioned as something separate from PAL-N is a result of a misunderstanding or misreading of the CCIR/ITU documents. See also the posting signed as Alchaemist here: https://en.wikipedia.org/wiki/Talk:PAL#PAL-N_versus_PAL-Nc That being said, we probably want to keep it if we want to remaing compatible with the loads of software and drivers which enumerate those as separate systems. But from a technical standpoint, PAL-N and PAL-Nc (and N/PAL, PAL-CN etc.) are just different "spellings" referring to exactly the same system. > + /** > + * @DRM_MODE_TV_MODE_SECAM_K: CCIR System G together with the > + * SECAM color system. Similar to @DRM_MODE_TV_MODE_SECAM_G but > + * with different channels. > + */ > + DRM_MODE_TV_MODE_SECAM_K, > + > + /** > + * @DRM_MODE_TV_MODE_SECAM_K1: CCIR System G together with the > + * SECAM color system. Similar to @DRM_MODE_TV_MODE_SECAM_G and > + * @DRM_MODE_TV_MODE_SECAM_K but with different channels. > + */ > + DRM_MODE_TV_MODE_SECAM_K1, Typos: you meant CCIR Systems K and K1, not System G. Best regards, Mateusz Kwiatkowski
Re: [PATCH v2 32/41] drm/vc4: vec: Convert to the new TV mode property
Hi Maxime, I tested your patchset on my Pi and it mostly works. Good work! However, I noticed a couple of issues. > -static int vc4_vec_encoder_atomic_check(struct drm_encoder *encoder, > - struct drm_crtc_state *crtc_state, > - struct drm_connector_state *conn_state) > -{ > - const struct vc4_vec_tv_mode *vec_mode; > - > - vec_mode = &vc4_vec_tv_modes[conn_state->tv.mode]; > - > - if (conn_state->crtc && > - !drm_mode_equal(vec_mode->mode, &crtc_state->adjusted_mode)) > - return -EINVAL; > - > - return 0; > -} I may have said it myself that we should allow custom modelines without too much validation. The VC4 and VEC, however, have some considerable limitations when it comes to the modelines that they can reliably output. In particular, attempting to use "50 Hz" timings in NTSC/PAL-M modes (or "60 Hz" in PAL/SECAM modes) results in a weirdly skewed image. Here's how it may look like: https://user-images.githubusercontent.com/4499762/187575940-736e7262-c82d-42f3-a2d8-f309cbd51139.png This is because if the CRTC does not trigger the sync pulses within an acceptable time window, the VEC apparently generates them itself. This causes the VEC sync pulses (which go onto the wire) not quite line up with the ones from the modeline, which results in what you see on the screenshot. I once wrote a validation function based on extensive testing of what produces a sensible output and what doesn't. You can find it here: https://github.com/raspberrypi/linux/pull/4406/commits/15c0c51. I think it might be a good idea to include something like that - even though I know it's somewhat ugly. (BTW, those %2 checks on vertical timings in that linked commit can be ignored; those values are divided by 2 for interlaced modes anyway. Those checks were intended to ensure proper odd-first or even-first timings; I'm not sure if your code calculates those in the same way) > static int vc4_vec_connector_get_modes(struct drm_connector *connector) > { > - struct drm_connector_state *state = connector->state; > struct drm_display_mode *mode; > + int count = 0; > > - mode = drm_mode_duplicate(connector->dev, > - vc4_vec_tv_modes[state->tv.mode].mode); > + mode = drm_mode_analog_ntsc_480i(connector->dev); > if (!mode) { > DRM_ERROR("Failed to create a new display mode\n"); > return -ENOMEM; > } > > drm_mode_probed_add(connector, mode); > + count += 1; > > - return 1; > + mode = drm_mode_analog_pal_576i(connector->dev); > + if (!mode) { > + DRM_ERROR("Failed to create a new display mode\n"); > + return -ENOMEM; > + } > + > + drm_mode_probed_add(connector, mode); > + count += 1; > + > + return count; > +} Xorg is pretty confused by these modes being reported like that. The 576i mode is *always* preferred, presumably because of the higher resolution. If the NTSC mode is set (via the kernel cmdline or just due to it being the default), this results in a mess on the screen - exactly the same thing as on the screenshot linked above. Note that drm_helper_probe_add_cmdline_mode() *does* add the DRM_MODE_TYPE_USERDEF flag to the 480i mode, having detected it as preferred on the command line - but Xorg does not seem to care about that. I remember Noralf suggesting setting DRM_MODE_TYPE_PREFERRED for the mode that corresponds to the currently chosen tv_mode - that would fix the problem. An alternative would be to _not_ add the "opposite" mode at all, like the current default Raspberry Pi OS kernel behaves. Note that if you decide to add the modeline validation like I suggested in the comment above, then without setting the preferred mode properly, Xorg will just give up and sit on a blank screen until you run xrandr from another terminal if tv_mode incompatible with 576i is selected. Best regards, Mateusz Kwiatkowski
Re: [PATCH v2 10/41] drm/modes: Add a function to generate analog display modes
Hi Maxime, Wow. That's an enormous amount of effort put into this patch. But I'm tempted to say that this is actually overengineered quite a bit :D Considering that there's no way to access all these calculations from user space, and I can't imagine anybody using anything else than those standard 480i/576i (and maybe 240p/288p) modes at 13.5 MHz any time soon... I'm not sure if we actually need all this. But anyway, I'm not the maintainer of this subsystem, so I'm not the one to decide. > +enum drm_mode_analog { > + DRM_MODE_ANALOG_NTSC, > + DRM_MODE_ANALOG_PAL, > +}; Using "NTSC" and "PAL" to describe the 50Hz and 60Hz analog TV modes is common, but strictly speaking a misnomer. Those are color encoding systems, and your patchset fully supports lesser used, but standard encodings for those (e.g. PAL-M for 60Hz and SECAM for 50Hz). I'd propose switching to some more neutral naming scheme. Some ideas: - DRM_MODE_ANALOG_60_HZ / DRM_MODE_ANALOG_50_HZ (after standard refresh rate) - DRM_MODE_ANALOG_525_LINES / DRM_MODE_ANALOG_625_LINES (after standard line count) - DRM_MODE_ANALOG_JM / DRM_MODE_ANALOG_BDGHIKLN (after corresponding ITU System Letter Designations) > +#define NTSC_HFP_DURATION_TYP_NS 1500 > +#define NTSC_HFP_DURATION_MIN_NS 1270 > +#define NTSC_HFP_DURATION_MAX_NS 2220 You've defined those min/typ/max ranges, but you're not using the "typ" field for anything other than hslen. The actual "typical" value is thus always the midpoint, which isn't necessarily the best choice. In particular, for the standard 720px wide modes at 13.5 MHz, hsync_start ends up being 735 for 480i and 734 for 576i, instead of 736 and 732 requested by BT.601. That's all obviously within tolerances, but the image ends up noticeably off-center (at least on modern TVs), especially in the 576i case. > + htotal = params->line_duration_ns * pixel_clock_hz / NSEC_PER_SEC; You're multiplying an unsigned int and an unsigned long - both types are only required to be 32 bit, so this is likely to overflow. You need to use a cast to unsigned long long, and then call do_div() for 64-bit division. This actually overflowed on me on my Pi running ARM32 kernel, resulting in negative horizontal porch lengths, and drm_helper_probe_add_cmdline_mode() taking over the mode generation (badly), and a horrible mess on screen. > + 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. Best regards, Mateusz Kwiatkowski
Re: [PATCH v1 00/35] drm: Analog TV Improvements
Hi Maxime, Noralf and everyone, Just as a quick update. I isolated the issue to the power management subsystem. Unfortunately I know very little about the power management subsystem so I don't think I can help. There are two alternative dirty ad-hoc hacks that get things working. - Commenting out the pm_runtime_put() / pm_runtime_get_sync() calls in vc4_vec.c - Reverting this PR by Dom Cobley a.k.a. popcornmix: https://github.com/raspberrypi/linux/pull/4639 Either of these approaches makes VEC mode switching work again. Obviously neither is appropriate for a permanent solution. I tried some random code permutations that came to my mind, like using the vc4_encoder callbacks (e.g. post_crtc_powerdown) instead of the standard enable/disable encoder callbacks, but to no avail. Since the clocks and power management seem to be delegated to the firmware now, my guess is that this might be a firmware issue, but I can't really check all the firmware versions. It certainly crashes on the version currently available in Raspberry Pi OS repos, and on this one: https://github.com/raspberrypi/rpi-firmware/commit/4dde751. My Pi4 doesn't boot using any newer firmware, at least not from USB - I might try some SD card after the weekend. Best regards, Mateusz Kwiatkowski W dniu 25.08.2022 o 18:17, Mateusz Kwiatkowski pisze: Hi Maxime, W dniu 25.08.2022 o 17:55, Maxime Ripard pisze: Hi Mateusz, On Mon, Aug 22, 2022 at 10:57:26AM +0200, Mateusz Kwiatkowski wrote: Hi Maxime, I tried testing and reviewing your changes properly over the last weekend, but ultimately ran into this ("flip_done timed out" etc.) issue and was unable to mitigate it, at least so far. This seems to pop up every time I try to change modes in any way (either change the TV norm, or just try doing "xrandr --output Composite-1 --off" followed by bringing it back on; it also means that the Pi goes unusable when the DE's screen saving routine kicks in). I'm using a Pi 4, and it works with the rpi-5.13.y branch https://github.com/raspberrypi/linux, but seemingly nothing newer. I specifically tried rpi-5.14.y, rpi-5.15.y and rpi-5.19.y - rpi-5.15.y, which is the current main branch in Raspberry Pi OS, seems to be broken since forever; at least since my patches (originally written for 5.10) landed there. I'll try identifying the issue further, possibly later today, and maybe check the rpi-6.0.y branch as well. I've tried it this, and I can't reproduce it here. But I'm curious, how did you apply this series? rpi-5.13.y is probably full of conflicts everywhere? I applied your patches onto rpi-5.15.y. There were conflicts, but they seemed relatively minor. I'm not sure if I did a good job at resolving them - I ran into various problems trying to test your changes, but I chose not to criticize you before making sure that it's really due to your changes, or without some remarks more constructive than "doesn't work" ;-) I can push my rebase onto some Github fork if you're interested. I was able to work around some of those problems, and also saw that some of them were already mentioned by other reviewers (such as the generic modes not matching due to the aspect ratio setting). Ultimately I got stuck on this bug, so I put testing this series on hold in favor of debugging the bigger issue. So far unfortunately no luck with it. I did not try rebasing your changes onto 5.13 or older. Maxime Best regards, Mateusz Kwiatkowski
Re: [PATCH v1 00/35] drm: Analog TV Improvements
Hi Maxime, W dniu 25.08.2022 o 17:55, Maxime Ripard pisze: > Hi Mateusz, > > On Mon, Aug 22, 2022 at 10:57:26AM +0200, Mateusz Kwiatkowski wrote: >> Hi Maxime, >> >> I tried testing and reviewing your changes properly over the last weekend, >> but >> ultimately ran into this ("flip_done timed out" etc.) issue and was unable to >> mitigate it, at least so far. This seems to pop up every time I try to change >> modes in any way (either change the TV norm, or just try doing >> "xrandr --output Composite-1 --off" followed by bringing it back on; it also >> means that the Pi goes unusable when the DE's screen saving routine kicks >> in). >> >> I'm using a Pi 4, and it works with the rpi-5.13.y branch >> https://github.com/raspberrypi/linux, but seemingly nothing newer. >> I specifically tried rpi-5.14.y, rpi-5.15.y and rpi-5.19.y - rpi-5.15.y, >> which is the current main branch in Raspberry Pi OS, seems to be broken since >> forever; at least since my patches (originally written for 5.10) landed >> there. >> >> I'll try identifying the issue further, possibly later today, and maybe check >> the rpi-6.0.y branch as well. > > I've tried it this, and I can't reproduce it here. But I'm curious, how > did you apply this series? rpi-5.13.y is probably full of conflicts > everywhere? I applied your patches onto rpi-5.15.y. There were conflicts, but they seemed relatively minor. I'm not sure if I did a good job at resolving them - I ran into various problems trying to test your changes, but I chose not to criticize you before making sure that it's really due to your changes, or without some remarks more constructive than "doesn't work" ;-) I can push my rebase onto some Github fork if you're interested. I was able to work around some of those problems, and also saw that some of them were already mentioned by other reviewers (such as the generic modes not matching due to the aspect ratio setting). Ultimately I got stuck on this bug, so I put testing this series on hold in favor of debugging the bigger issue. So far unfortunately no luck with it. I did not try rebasing your changes onto 5.13 or older. > Maxime Best regards, Mateusz Kwiatkowski
Re: [PATCH v1 34/35] drm/modes: Introduce the tv_mode property as a command-line option
Hi Maxime & Noralf, W dniu 24.08.2022 o 17:45, Maxime Ripard pisze: > Hi Noralf, > > On Sat, Aug 20, 2022 at 10:18:47PM +0200, Noralf Trønnes wrote: >> Den 29.07.2022 18.35, skrev Maxime Ripard: >>> Our new tv mode option allows to specify the TV mode from a property. >>> However, it can still be useful, for example to avoid any boot time >>> artifact, to set that property directly from the kernel command line. >>> >>> Let's add some code to allow it, and some unit tests to exercise that code. >>> >>> Signed-off-by: Maxime Ripard >>> >> >> In the subject it says "tv_mode property", but the property is called >> "tv norm", so the option should be tv_norm? > > Yeah... I don't know. mode is taken but it's obviously the best name. So > I went with norm to avoid the (internal) conflict but I left mode for > the user facing property. > > I'm not sure what's best here, or maybe we can pick another name entirely? I think "standard" and "system" are also fairly common names for this property. I once had an old multi-standard Sharp CRT TV that allowed manually selecting the expected color encoding (PAL/SECAM/NTSC/NTSC-443), and the relevant button on the remote was labeled "SYSTEM". V4L2 calls this "standard", see include/uapi/linux/videodev2.h, compare with v4l2_std_id, V4L2_STD_* etc. BTW, maybe reusing those V4L2 constants, or aligning the numerical values, would make some sense? > > Maxime Best regards, Mateusz Kwiatkowski
Re: [PATCH v1 24/35] drm/vc4: vec: Add support for more analog TV standards
Hi Maxime, W dniu 15.08.2022 o 10:37, Maxime Ripard pisze: > Hi, > > On Fri, Jul 29, 2022 at 07:55:30PM +0200, Mateusz Kwiatkowski wrote: >> Hi Maxime, >> >> I think that declaring PAL-B and SECAM-B as the only supported 576i >> norms is a bit random. > > Starting with this patch, PAL-N should be supported as well, right? Oh, sure. I forgot about it. My brain was too focused on the "standard PAL" modes, which excludes PAL-N. > >> Norms B, D, G, H, I, K, K1 and L (for both PAL and SECAM) are >> essentially identical if we're talking about baseband signals, AFAIK >> they only differ when those are modulated as RF signals. I'm not sure >> if there's a point to differentiating those (that's more about patch >> 05/35) unless we need to deal with some device that actually features >> an RF modulator. > > What I was aiming for is to have all the cases we have in all the > drivers covered so that we can make that property generic. i915 > declares and uses all those variants: > > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/i915/display/intel_sdvo.c#L68 > > Especially since it's i915 and it's pretty much the standard as far as > the uAPI goes, I'd rather avoid any regression there. OK, if there are already drivers that differentiate those, then it doesn't make sense to introduce regressions. And yes, there is plenty of software already out there that differentiate between those modes in the context of composite video. It still doesn't make much sense from the engineering point of view, though. > >> But if we do want to have all those norms separate, then I'd say that >> VC4 should declare support for all of those, and all should map to the >> same VEC settings. Some users from e.g. the UK might think that they >> won't get proper picture if PAL-I is not on the list of supported >> norms. Same goes for e.g. SECAM-D/K in the former Soviet territories, >> and so on. > > I'd be open to it, but we can always extend vc4 to support those modes > later on. The work you did to make that easier should make it trivial. Doing that in the future is OK as well. I just wanted to point out that PAL-B/D/G/H/I/K/K1/L (and same for SECAM) is the same exact thing as far as baseband composite video is concerned, so declaring only one of those as supported is potentially misleading for anybody who is not aware of that fact. > > Maxime Best regards, Mateusz Kwiatkowski
Re: [PATCH v1 04/35] drm/modes: Introduce 480i and 576i modes
lines for the 59.94 Hz standard. Or 576/486, depending on how you count. The topmost line of those 576/486 starts at half the screen, and the bottommost line ends at half the screen - so they are often combined when counting and given as 575/485. The digital 576i50 standard includes those half-lines. In the 59.94 Hz regions, 480 active digial lines ended up the norm, because 486 does not have nice dividers, and also some of the outermost lines which were always overscanned anyway, ended up used for things like closed captioning over the years. - Speaking of closed captioning... a lot of different stuff were put in the blanking interval over the years. Like teletext in Europe. There are projects like VBIT2 <https://github.com/peterkvt80/vbit2> which intentionally reconfigure the Raspberry Pi composite output to include the blanking interval in the framebuffer so that teletext can be output by drawing on the edge of the "screen" (from the computer point of view). - A lot of equipment outside the broadcast industry willingly violated those standards, and there are real world use cases for that. Film studios used very slightly modified TVs to make them sync with 24fps cameras - in that variant, "NTSC" could have e.g. 655 lines so that the TV would refresh at 48 Hz with the same line frequency. Home computers and video game consoles output progressive 262/312-line modes instead of interlaced 525/625 lines. And often changed the line frequency slightly as well, for various reasons. Those progressive modes are still favored by retro gaming and emulation enthusiasts, because they incur a specific look on CRT displays. Even playing back video from a tape (especially home-grade, like VHS) could cause timings to go wildly out of spec, because of mechanical imprecisions. - There were multitude of standards predating the ubiquitous 525/60 and 625/50 modes. The British 405-line and French 819-line standards are the most notorious, having lasted well into the 1980s, but there were also a lot of wildly varying pre-WW2 television systems. And there are enthusiasts dedicated to preserving those. My point is that the norms for analog TV are rather loose, and I think we shouldn't limit the drivers to only accepting the "proper" modes as defined in the spec. Those should of course be the default, but if non-standard modelines can be generated - there are legitimate use cases why people might want those. > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- > ge...@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like > that. > -- Linus Torvalds Best regards, Mateusz Kwiatkowski
Re: [PATCH v1 00/35] drm: Analog TV Improvements
Hi Maxime, I tried testing and reviewing your changes properly over the last weekend, but ultimately ran into this ("flip_done timed out" etc.) issue and was unable to mitigate it, at least so far. This seems to pop up every time I try to change modes in any way (either change the TV norm, or just try doing "xrandr --output Composite-1 --off" followed by bringing it back on; it also means that the Pi goes unusable when the DE's screen saving routine kicks in). I'm using a Pi 4, and it works with the rpi-5.13.y branch https://github.com/raspberrypi/linux, but seemingly nothing newer. I specifically tried rpi-5.14.y, rpi-5.15.y and rpi-5.19.y - rpi-5.15.y, which is the current main branch in Raspberry Pi OS, seems to be broken since forever; at least since my patches (originally written for 5.10) landed there. I'll try identifying the issue further, possibly later today, and maybe check the rpi-6.0.y branch as well. Best regards, Mateusz Kwiatkowski W dniu 22.08.2022 o 09:48, Maxime Ripard pisze: > Hi, > > On Sun, Aug 21, 2022 at 06:33:12PM +0200, Noralf Trønnes wrote: >> Den 29.07.2022 18.34, skrev Maxime Ripard: >>> Hi, >>> >>> Here's a series aiming at improving the command line named modes support, >>> and more importantly how we deal with all the analog TV variants. >>> >>> The named modes support were initially introduced to allow to specify the >>> analog TV mode to be used. >>> >>> However, this was causing multiple issues: >>> >>> * The mode name parsed on the command line was passed directly to the >>> driver, which had to figure out which mode it was suppose to match; >>> >>> * Figuring that out wasn't really easy, since the video= argument or what >>> the userspace might not even have a name in the first place, but >>> instead could have passed a mode with the same timings; >>> >>> * The fallback to matching on the timings was mostly working as long as >>> we were supporting one 525 lines (most likely NSTC) and one 625 lines >>> (PAL), but couldn't differentiate between two modes with the same >>> timings (NTSC vs PAL-M vs NSTC-J for example); >>> >>> * There was also some overlap with the tv mode property registered by >>> drm_mode_create_tv_properties(), but named modes weren't interacting >>> with that property at all. >>> >>> * Even though that property was generic, its possible values were >>> specific to each drivers, which made some generic support difficult. >>> >>> Thus, I chose to tackle in multiple steps: >>> >>> * A new TV norm property was introduced, with generic values, each driver >>> reporting through a bitmask what standard it supports to the userspace; >>> >>> * This option was added to the command line parsing code to be able to >>> specify it on the kernel command line, and new atomic_check and reset >>> helpers were created to integrate properly into atomic KMS; >>> >>> * The named mode parsing code is now creating a proper display mode for >>> the given named mode, and the TV standard will thus be part of the >>> connector state; >>> >>> * Two drivers were converted and tested for now (vc4 and sun4i), with >>> some backward compatibility code to translate the old TV mode to the >>> new TV mode; >>> >>> Unit tests were created along the way. Nouveau, ch7006 and gud are >>> currently broken for now since I expect that work to be reworked fairly >>> significantly. I'm also not entirely sure about how to migrate GUD to the >>> new property. >>> >>> Let me know what you think, >>> Maxime >>> >> I don't know if it's related to this patchset or not, but I do get this: >> >> pi@pi4t:~ $ sudo dmesg -C && sudo modprobe -r vc4 && sudo modprobe vc4 >> && dmesg >> [ 430.066211] Console: switching to colour dummy device 80x30 >> [ 431.294788] vc4-drm gpu: bound fe40.hvs (ops vc4_hvs_ops [vc4]) >> [ 431.295115] vc4-drm gpu: bound fec13000.vec (ops vc4_vec_ops [vc4]) >> [ 431.295467] vc4-drm gpu: bound fe004000.txp (ops vc4_txp_ops [vc4]) >> [ 431.295804] vc4-drm gpu: bound fec12000.pixelvalve (ops vc4_crtc_ops >> [vc4]) >> [ 431.298895] [drm] Initialized vc4 0.0.0 20140616 for gpu on minor 0 >> [ 441.444250] vc4-drm gpu: [drm] *ERROR* [CRTC:68:crtc-1] flip_done >> timed out >> [ 441.446529] Console: switching to colo
Re: [PATCH v1 24/35] drm/vc4: vec: Add support for more analog TV standards
Hi Maxime, I think that declaring PAL-B and SECAM-B as the only supported 576i norms is a bit random. Norms B, D, G, H, I, K, K1 and L (for both PAL and SECAM) are essentially identical if we're talking about baseband signals, AFAIK they only differ when those are modulated as RF signals. I'm not sure if there's a point to differentiating those (that's more about patch 05/35) unless we need to deal with some device that actually features an RF modulator. But if we do want to have all those norms separate, then I'd say that VC4 should declare support for all of those, and all should map to the same VEC settings. Some users from e.g. the UK might think that they won't get proper picture if PAL-I is not on the list of supported norms. Same goes for e.g. SECAM-D/K in the former Soviet territories, and so on. Best regards, Mateusz Kwiatkowski W dniu 29.07.2022 o 18:35, Maxime Ripard pisze: From: Mateusz Kwiatkowski 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 Signed-off-by: Maxime Ripard diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c index e40b55de1b3c..91d343238b0f 100644 --- a/drivers/gpu/drm/vc4/vc4_vec.c +++ b/drivers/gpu/drm/vc4/vc4_vec.c @@ -46,6 +46,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) #define VEC_CONFIG0_PBPR_FIL BIT(18) #define VEC_CONFIG0_CHROMA_GAIN_MASK GENMASK(17, 16) #define VEC_CONFIG0_CHROMA_GAIN_UNITY (0 << 16) @@ -76,6 +77,27 @@ #define VEC_SOFT_RESET0x10c #define VEC_CLMP0_START 0x144 #define VEC_CLMP0_END 0x148 + +/* + * These set the color subcarrier frequency + * if VEC_CONFIG1_CUSTOM_FREQ is enabled. + * + * VEC_FREQ1_0 contains the most significant 16-bit half-word, + * VEC_FREQ3_2 contains the least significant 16-bit half-word. + * 0x8000 seems to be equivalent to the pixel clock + * (which itself is the VEC clock divided by 8). + * + * Reference values (with the default pixel clock of 13.5 MHz): + * + * NTSC (3579545.[45] Hz) - 0x21F07C1F + * PAL (4433618.75 Hz) - 0x2A098ACB + * PAL-M (3575611.[888111] Hz) - 0x21E6EFE3 + * PAL-N (3582056.25 Hz) - 0x21F69446 + * + * 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. + */ #define VEC_FREQ3_2 0x180 #define VEC_FREQ1_0 0x184 @@ -118,6 +140,14 @@ #define VEC_INTERRUPT_CONTROL 0x190 #define VEC_INTERRUPT_STATUS 0x194 + +/* + * 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 425 Hz, which corresponds to 0x284BDA13. + * That is also the default value, so no need to set it explicitly. + */ #define VEC_FCW_SECAM_B 0x198 #define VEC_SECAM_GAIN_VAL0x19c @@ -194,9 +224,13 @@ connector_to_vc4_vec(struct drm_connector *connector) enum vc4_vec_tv_mode_id { VC4_VEC_TV_MODE_NTSC, + VC4_VEC_TV_MODE_NTSC_443, VC4_VEC_TV_MODE_NTSC_J, VC4_VEC_TV_MODE_PAL, + VC4_VEC_TV_MODE_PAL_60, VC4_VEC_TV_MODE_PAL_M, + VC4_VEC_TV_MODE_PAL_N, + VC4_VEC_TV_MODE_SECAM, }; struct vc4_vec_tv_mode { @@ -234,6 +268,12 @@ static const struct debugfs_reg32 vec_regs[] = { }; static const struct vc4_vec_tv_mode vc4_vec_tv_modes[] = { + { + .mode = DRM_MODE_TV_NORM_NTSC_443, + .config0 = VEC_CONFIG0_NTSC_STD, + .config1 = VEC_CONFIG1_C_CVBS_CVBS | VEC_CONFIG1_CUSTOM_FREQ, + .custom_freq = 0x2a09
Re: [PATCH v1 20/35] drm/vc4: vec: Switch for common modes
Hi Maxime, I'm just noting that the modelines you defined in drm_modes.c are different to the ones you're removing here. The horizontal sync differences probably doesn't matter too much, VC4 uses those only as a hint anyway and generates sync autonomously, so the slight differences will only cause the image to slightly shift horizontally. But you're also changing the 480i vertical sync (vsync_start is now 488 instead of 487, etc.). Are you sure that this won't break anything? This will probably shift the image by 1 line (which for the 480i might actually mean going out of spec), and I _think_ it might control the odd vs. even field first modes on some drivers. I won't be able to test this before Monday, but I'm just pointing out the potential issue. BTW, I've seen a similar thing in the sun4i driver changes (patch 32/35) and the differences in vertical sync are even more dramatic. It's entirely possible that the current timings in sun4i are broken and the new ones are correct (the new ones certainly look saner to me), but I'd double-check if that driver does not have any quirks that would _require_ such weird settings. Best regards, Mateusz Kwiatkowski W dniu 29.07.2022 o 18:35, Maxime Ripard pisze: Now that the core has a definition for the 525 and 625 lines analog TV modes, let's switch to it for vc4. Signed-off-by: Maxime Ripard diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c index 8f30a530b2d5..255bba563fce 100644 --- a/drivers/gpu/drm/vc4/vc4_vec.c +++ b/drivers/gpu/drm/vc4/vc4_vec.c @@ -224,38 +224,24 @@ static const struct debugfs_reg32 vec_regs[] = { VC4_REG32(VEC_DAC_MISC), }; -static const struct drm_display_mode ntsc_mode = { - DRM_MODE("720x480", DRM_MODE_TYPE_DRIVER, 13500, -720, 720 + 14, 720 + 14 + 64, 720 + 14 + 64 + 60, 0, -480, 480 + 7, 480 + 7 + 6, 525, 0, -DRM_MODE_FLAG_INTERLACE) -}; - -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 + 4, 576 + 4 + 6, 625, 0, -DRM_MODE_FLAG_INTERLACE) -}; - static const struct vc4_vec_tv_mode vc4_vec_tv_modes[] = { [VC4_VEC_TV_MODE_NTSC] = { - .mode = &ntsc_mode, + .mode = &drm_mode_480i, .config0 = VEC_CONFIG0_NTSC_STD | VEC_CONFIG0_PDEN, .config1 = VEC_CONFIG1_C_CVBS_CVBS, }, [VC4_VEC_TV_MODE_NTSC_J] = { - .mode = &ntsc_mode, + .mode = &drm_mode_480i, .config0 = VEC_CONFIG0_NTSC_STD, .config1 = VEC_CONFIG1_C_CVBS_CVBS, }, [VC4_VEC_TV_MODE_PAL] = { - .mode = &pal_mode, + .mode = &drm_mode_576i, .config0 = VEC_CONFIG0_PAL_BDGHI_STD, .config1 = VEC_CONFIG1_C_CVBS_CVBS, }, [VC4_VEC_TV_MODE_PAL_M] = { - .mode = &pal_mode, + .mode = &drm_mode_576i, .config0 = VEC_CONFIG0_PAL_BDGHI_STD, .config1 = VEC_CONFIG1_C_CVBS_CVBS | VEC_CONFIG1_CUSTOM_FREQ, .custom_freq = 0x223b61d1,
Re: [PATCH v1 14/35] drm/atomic-helper: Add an analog TV atomic_check implementation
Hi Maxime, I'm pretty sure that PAL-60 and SECAM-60 should be tied to the 480i mode. Those are non-standard "norms" that use 60 Hz sync (which is largely synonymous with 480i in the analog TV world) with PAL/SECAM color encoding. Best regards, Mateusz Kwiatkowski W dniu 29.07.2022 o 18:34, Maxime Ripard pisze: The analog TV connector drivers share some atomic_check logic, and the new TV standard property have created a bunch of new constraints that needs to be shared across drivers too. Let's create an atomic_check helper for those use cases. Signed-off-by: Maxime Ripard diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c index 6d14cb0c64b1..fce5569bd66a 100644 --- a/drivers/gpu/drm/drm_atomic_state_helper.c +++ b/drivers/gpu/drm/drm_atomic_state_helper.c @@ -552,6 +552,93 @@ void drm_atomic_helper_connector_tv_reset(struct drm_connector *connector) } EXPORT_SYMBOL(drm_atomic_helper_connector_tv_reset); +/** + * @drm_atomic_helper_connector_tv_check: Validate an analog TV connector state + * @connector: DRM Connector + * @state: the DRM State object + * + * Checks the state object to see if the requested state is valid for an + * analog TV connector. + * + * Returns: + * Zero for success, a negative error code on error. + */ +int drm_atomic_helper_connector_tv_check(struct drm_connector *connector, +struct drm_atomic_state *state) +{ + struct drm_connector_state *old_conn_state = + drm_atomic_get_old_connector_state(state, connector); + struct drm_connector_state *new_conn_state = + drm_atomic_get_new_connector_state(state, connector); + const struct drm_display_mode *mode; + struct drm_crtc_state *crtc_state; + struct drm_crtc *crtc; + + crtc = new_conn_state->crtc; + if (!crtc) + return 0; + + crtc_state = drm_atomic_get_new_crtc_state(state, crtc); + if (!crtc_state) + return -EINVAL; + + switch (new_conn_state->tv.norm) { + case DRM_MODE_TV_NORM_NTSC_443: + fallthrough; + case DRM_MODE_TV_NORM_NTSC_J: + fallthrough; + case DRM_MODE_TV_NORM_NTSC_M: + fallthrough; + case DRM_MODE_TV_NORM_PAL_M: + mode = &drm_mode_480i; + break; + + case DRM_MODE_TV_NORM_PAL_60: + fallthrough; + case DRM_MODE_TV_NORM_PAL_B: + fallthrough; + case DRM_MODE_TV_NORM_PAL_D: + fallthrough; + case DRM_MODE_TV_NORM_PAL_G: + fallthrough; + case DRM_MODE_TV_NORM_PAL_H: + fallthrough; + case DRM_MODE_TV_NORM_PAL_I: + fallthrough; + case DRM_MODE_TV_NORM_PAL_N: + fallthrough; + case DRM_MODE_TV_NORM_PAL_NC: + fallthrough; + case DRM_MODE_TV_NORM_SECAM_60: + fallthrough; + case DRM_MODE_TV_NORM_SECAM_B: + fallthrough; + case DRM_MODE_TV_NORM_SECAM_D: + fallthrough; + case DRM_MODE_TV_NORM_SECAM_G: + fallthrough; + case DRM_MODE_TV_NORM_SECAM_K: + fallthrough; + case DRM_MODE_TV_NORM_SECAM_K1: + fallthrough; + case DRM_MODE_TV_NORM_SECAM_L: + mode = &drm_mode_576i; + break; + + default: + return -EINVAL; + } + + if (!drm_mode_equal(mode, &crtc_state->mode)) + return -EINVAL; + + if (old_conn_state->tv.norm != new_conn_state->tv.norm) + crtc_state->mode_changed = true; + + return 0; +} +EXPORT_SYMBOL(drm_atomic_helper_connector_tv_check); + /** * __drm_atomic_helper_connector_duplicate_state - copy atomic connector state * @connector: connector object diff --git a/include/drm/drm_atomic_state_helper.h b/include/drm/drm_atomic_state_helper.h index c8fbce795ee7..b9740edb2658 100644 --- a/include/drm/drm_atomic_state_helper.h +++ b/include/drm/drm_atomic_state_helper.h @@ -26,6 +26,7 @@ #include +struct drm_atomic_state; struct drm_bridge; struct drm_bridge_state; struct drm_crtc; @@ -71,6 +72,8 @@ void __drm_atomic_helper_connector_reset(struct drm_connector *connector, struct drm_connector_state *conn_state); void drm_atomic_helper_connector_reset(struct drm_connector *connector); void drm_atomic_helper_connector_tv_reset(struct drm_connector *connector); +int drm_atomic_helper_connector_tv_check(struct drm_connector *connector, +struct drm_atomic_state *state); void drm_atomic_helper_connector_tv_margins_reset(struct drm_connector *connector); void __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,