Re: [PATCH] drm/vc4: vec: Add support for PAL-60

[Mateusz Kwiatkowski <kfyatek@gmail.com>]

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