diff mbox series

drm/vc4: vec: Add support for PAL-60

Message ID 93bf9fcc-c645-b042-011f-8f1fc957af48@gmail.com (mailing list archive)
State New, archived
Headers show
Series drm/vc4: vec: Add support for PAL-60 | expand

Commit Message

Mateusz Kwiatkowski Oct. 16, 2022, 7:46 p.m. UTC 
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 <kfyatek+publicgit@gmail.com>
---
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-d841cc64fe4b@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(-)


base-commit: e16415e3ddae9abb14a00793554a162403f9af6d

Comments

Maxime Ripard Oct. 18, 2022, 8:31 a.m. UTC | #1 
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.

Maxime
Mateusz Kwiatkowski Oct. 18, 2022, 8:57 p.m. UTC | #2 
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
Maxime Ripard Oct. 20, 2022, 3:34 p.m. UTC | #3 
On Tue, Oct 18, 2022 at 10:57:04PM +0200, Mateusz Kwiatkowski wrote:
> 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.

That's the one I would expect, and I assume we could just do that by
selecting the 480i mode + PAL TV Mode property, right?

> 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.

But it's not really clear to me why we should support both.

> 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.

OK

Thanks!
Maxime
diff mbox series

Patch

diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
index 88b4330bfa39..bbc41e502cc3 100644
--- a/drivers/gpu/drm/vc4/vc4_vec.c
+++ b/drivers/gpu/drm/vc4/vc4_vec.c
@@ -235,6 +235,7 @@  enum vc4_vec_tv_mode_id {
 
 struct vc4_vec_tv_mode {
 	unsigned int mode;
+	u16 expected_htotal;
 	u32 config0;
 	u32 config1;
 	u32 custom_freq;
@@ -270,37 +271,52 @@  static const struct debugfs_reg32 vec_regs[] = {
 static const struct vc4_vec_tv_mode vc4_vec_tv_modes[] = {
 	{
 		.mode = DRM_MODE_TV_MODE_NTSC,
+		.expected_htotal = 858,
 		.config0 = VEC_CONFIG0_NTSC_STD | VEC_CONFIG0_PDEN,
 		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
 	},
 	{
 		.mode = DRM_MODE_TV_MODE_NTSC_443,
+		.expected_htotal = 858,
 		.config0 = VEC_CONFIG0_NTSC_STD,
 		.config1 = VEC_CONFIG1_C_CVBS_CVBS | VEC_CONFIG1_CUSTOM_FREQ,
 		.custom_freq = 0x2a098acb,
 	},
 	{
 		.mode = DRM_MODE_TV_MODE_NTSC_J,
+		.expected_htotal = 858,
 		.config0 = VEC_CONFIG0_NTSC_STD,
 		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
 	},
 	{
 		.mode = DRM_MODE_TV_MODE_PAL,
+		.expected_htotal = 864,
 		.config0 = VEC_CONFIG0_PAL_BDGHI_STD,
 		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
 	},
+	{
+		/* PAL-60 */
+		.mode = DRM_MODE_TV_MODE_PAL,
+		.expected_htotal = 858,
+		.config0 = VEC_CONFIG0_PAL_M_STD,
+		.config1 = VEC_CONFIG1_C_CVBS_CVBS | VEC_CONFIG1_CUSTOM_FREQ,
+		.custom_freq = 0x2a098acb,
+	},
 	{
 		.mode = DRM_MODE_TV_MODE_PAL_M,
+		.expected_htotal = 858,
 		.config0 = VEC_CONFIG0_PAL_M_STD,
 		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
 	},
 	{
 		.mode = DRM_MODE_TV_MODE_PAL_N,
+		.expected_htotal = 864,
 		.config0 = VEC_CONFIG0_PAL_N_STD,
 		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
 	},
 	{
 		.mode = DRM_MODE_TV_MODE_SECAM,
+		.expected_htotal = 864,
 		.config0 = VEC_CONFIG0_SECAM_STD,
 		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
 		.custom_freq = 0x29c71c72,
@@ -308,14 +324,15 @@  static const struct vc4_vec_tv_mode vc4_vec_tv_modes[] = {
 };
 
 static inline const struct vc4_vec_tv_mode *
-vc4_vec_tv_mode_lookup(unsigned int mode)
+vc4_vec_tv_mode_lookup(unsigned int mode, u16 htotal)
 {
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(vc4_vec_tv_modes); i++) {
 		const struct vc4_vec_tv_mode *tv_mode = &vc4_vec_tv_modes[i];
 
-		if (tv_mode->mode == mode)
+		if (tv_mode->mode == mode &&
+		    tv_mode->expected_htotal == htotal)
 			return tv_mode;
 	}
 
@@ -394,6 +411,7 @@  vc4_vec_connector_set_property(struct drm_connector *connector,
 		break;
 
 	case VC4_VEC_TV_MODE_PAL:
+	case VC4_VEC_TV_MODE_PAL_60:
 		state->tv.mode = DRM_MODE_TV_MODE_PAL;
 		break;
 
@@ -551,13 +569,16 @@  static void vc4_vec_encoder_enable(struct drm_encoder *encoder,
 	struct drm_connector *connector = &vec->connector;
 	struct drm_connector_state *conn_state =
 		drm_atomic_get_new_connector_state(state, connector);
+	struct drm_display_mode *adjusted_mode =
+		&encoder->crtc->state->adjusted_mode;
 	const struct vc4_vec_tv_mode *tv_mode;
 	int idx, ret;
 
 	if (!drm_dev_enter(drm, &idx))
 		return;
 
-	tv_mode = vc4_vec_tv_mode_lookup(conn_state->tv.mode);
+	tv_mode = vc4_vec_tv_mode_lookup(conn_state->tv.mode,
+					 adjusted_mode->htotal);
 	if (!tv_mode)
 		goto err_dev_exit;