Re: [Intel-gfx] [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 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"
property is supposed to do.
>> Also added some comments explain
Re: [Intel-gfx] [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_NTSC_J,
>
Re: [Intel-gfx] [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_START0x144 > #define
Re: [Intel-gfx] [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_NTSC, /* 525 lines, 60Hz */
>
Re: [Intel-gfx] [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: [Intel-gfx] [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..d82aef168075 100644
> --- a/drivers/gpu/drm/
Re: [Intel-gfx] [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(>base,
> +
> dev->mode_config.tv_mode_property,
> + _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: [Intel-gfx] [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 = _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: [Intel-gfx] [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 to bother a lot of people" bit too much.
Here's why:
- Analog TV output is a relatively obscure feature in thi
Re: [Intel-gfx] [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 = _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
Re: [Intel-gfx] [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 = _state->adjusted_mode;
> const struct vc4_vec_tv_mode *vec_mode;
>
> vec_mode = _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, _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
[Intel-gfx] [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 = _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 = >connector;
struct drm_connector_state *conn_state =
drm_atomic_get_new_connector_state(state, connector);
+ struct drm_display_mode *adjusted_mode =
+ >crtc->state->adjusted_mode;
const struct vc4_vec_tv_mode *tv_mode;
int idx, ret;
if (!drm_dev_enter(drm, ))
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: [Intel-gfx] [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: [Intel-gfx] [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 = _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 = _vec_tv_modes[conn_state->tv.legacy_mode];
> -
> - if (conn_state->crtc &&
> - !drm_mode_equal(vec_mode->mode, _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: [Intel-gfx] [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: [Intel-gfx] [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
> = _state->adjusted_mode; > const struct vc4_vec_tv_mode *vec_mode; > >
> vec_mode =
_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, _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: [Intel-gfx] [PATCH v5 06/22] drm/modes: Add a function to generate analog display modes
e 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: [Intel-gfx] [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 = >connector;
> struct drm_connector_state *conn_state =
> drm_atomic_get_new_connector_state(state, connector);
> - const struct vc4_vec_tv_mode *tv_mode =
> - _vec_tv_modes[conn_state->tv.legacy_mode];
> + const struct vc4_vec_tv_mode *tv_mode;
> int idx, ret;
>
> if (!drm_dev_enter(drm, ))
> return;
>
> + tv_mode = vc4_vec_tv_mode_lookup(conn_state->tv.mode);
> + if (!tv_mode)
> + goto err_dev_exit;
> +
> ret = pm_runtime_get_sync(>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: [Intel-gfx] [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: [Intel-gfx] [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: [Intel-gfx] [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 that in turn causes the image >> to be significantly shifted upwards. >> >> I hope thi
Re: [Intel-gfx] [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 "NTSC-J" stuck as a universally recognized na
Re: [Intel-gfx] [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: [Intel-gfx] [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 for those.
My feelings about the PAL-60 issue are not that s
Re: [Intel-gfx] [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: [Intel-gfx] [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: [Intel-gfx] [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: [Intel-gfx] [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: [Intel-gfx] [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: [Intel-gfx] [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 = _vec_tv_modes[conn_state->tv.mode];
> -
> - if (conn_state->crtc &&
> - !drm_mode_equal(vec_mode->mode, _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: [Intel-gfx] [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
