diff mbox series

[v1,20/35] drm/vc4: vec: Switch for common modes

Message ID 20220728-rpi-analog-tv-properties-v1-20-3d53ae722097@cerno.tech (mailing list archive)
State New, archived
Delegated to: Neil Armstrong
Headers show
Series drm: Analog TV Improvements | expand

Commit Message

Maxime Ripard July 29, 2022, 4:35 p.m. UTC 
Now that the core has a definition for the 525 and 625 lines analog TV
modes, let's switch to it for vc4.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Comments

Mateusz Kwiatkowski July 29, 2022, 6:12 p.m. UTC | #1 
Hi Maxime,

I'm just noting that the modelines you defined in drm_modes.c are 
different to the ones you're removing here.

The horizontal sync differences probably doesn't matter too much, VC4 
uses those only as a hint anyway and generates sync autonomously, so the 
slight differences will only cause the image to slightly shift horizontally.

But you're also changing the 480i vertical sync (vsync_start is now 488 
instead of 487, etc.). Are you sure that this won't break anything? This 
will probably shift the image by 1 line (which for the 480i might 
actually mean going out of spec), and I _think_ it might control the odd 
vs. even field first modes on some drivers. I won't be able to test this 
before Monday, but I'm just pointing out the potential issue.

BTW, I've seen a similar thing in the sun4i driver changes (patch 32/35) 
and the differences in vertical sync are even more dramatic. It's 
entirely possible that the current timings in sun4i are broken and the 
new ones are correct (the new ones certainly look saner to me), but I'd 
double-check if that driver does not have any quirks that would 
_require_ such weird settings.

Best regards,
Mateusz Kwiatkowski

W dniu 29.07.2022 o 18:35, Maxime Ripard pisze:
> Now that the core has a definition for the 525 and 625 lines analog TV
> modes, let's switch to it for vc4.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
>
> diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
> index 8f30a530b2d5..255bba563fce 100644
> --- a/drivers/gpu/drm/vc4/vc4_vec.c
> +++ b/drivers/gpu/drm/vc4/vc4_vec.c
> @@ -224,38 +224,24 @@ static const struct debugfs_reg32 vec_regs[] = {
>   	VC4_REG32(VEC_DAC_MISC),
>   };
>   
> -static const struct drm_display_mode ntsc_mode = {
> -	DRM_MODE("720x480", DRM_MODE_TYPE_DRIVER, 13500,
> -		 720, 720 + 14, 720 + 14 + 64, 720 + 14 + 64 + 60, 0,
> -		 480, 480 + 7, 480 + 7 + 6, 525, 0,
> -		 DRM_MODE_FLAG_INTERLACE)
> -};
> -
> -static const struct drm_display_mode pal_mode = {
> -	DRM_MODE("720x576", DRM_MODE_TYPE_DRIVER, 13500,
> -		 720, 720 + 20, 720 + 20 + 64, 720 + 20 + 64 + 60, 0,
> -		 576, 576 + 4, 576 + 4 + 6, 625, 0,
> -		 DRM_MODE_FLAG_INTERLACE)
> -};
> -
>   static const struct vc4_vec_tv_mode vc4_vec_tv_modes[] = {
>   	[VC4_VEC_TV_MODE_NTSC] = {
> -		.mode = &ntsc_mode,
> +		.mode = &drm_mode_480i,
>   		.config0 = VEC_CONFIG0_NTSC_STD | VEC_CONFIG0_PDEN,
>   		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
>   	},
>   	[VC4_VEC_TV_MODE_NTSC_J] = {
> -		.mode = &ntsc_mode,
> +		.mode = &drm_mode_480i,
>   		.config0 = VEC_CONFIG0_NTSC_STD,
>   		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
>   	},
>   	[VC4_VEC_TV_MODE_PAL] = {
> -		.mode = &pal_mode,
> +		.mode = &drm_mode_576i,
>   		.config0 = VEC_CONFIG0_PAL_BDGHI_STD,
>   		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
>   	},
>   	[VC4_VEC_TV_MODE_PAL_M] = {
> -		.mode = &pal_mode,
> +		.mode = &drm_mode_576i,
>   		.config0 = VEC_CONFIG0_PAL_BDGHI_STD,
>   		.config1 = VEC_CONFIG1_C_CVBS_CVBS | VEC_CONFIG1_CUSTOM_FREQ,
>   		.custom_freq = 0x223b61d1,
>
Maxime Ripard Aug. 16, 2022, 11:57 a.m. UTC | #2 
Hi,

On Fri, Jul 29, 2022 at 08:12:07PM +0200, Mateusz Kwiatkowski wrote:
> I'm just noting that the modelines you defined in drm_modes.c are different
> to the ones you're removing here.
> 
> The horizontal sync differences probably doesn't matter too much, VC4 uses
> those only as a hint anyway and generates sync autonomously, so the slight
> differences will only cause the image to slightly shift horizontally.
> 
> But you're also changing the 480i vertical sync (vsync_start is now 488
> instead of 487, etc.). Are you sure that this won't break anything? This
> will probably shift the image by 1 line (which for the 480i might actually
> mean going out of spec), and I _think_ it might control the odd vs. even
> field first modes on some drivers. I won't be able to test this before
> Monday, but I'm just pointing out the potential issue.

I didn't see any difference on both vc4 and sun4i, but you might be
right about this.

I didn't have much confidence in the vc4 modes since they were broken
before your patches, but maybe I should have used yours still.

> BTW, I've seen a similar thing in the sun4i driver changes (patch 32/35) and
> the differences in vertical sync are even more dramatic. It's entirely
> possible that the current timings in sun4i are broken and the new ones are
> correct (the new ones certainly look saner to me), but I'd double-check if
> that driver does not have any quirks that would _require_ such weird
> settings.

The only thing sun4i requires from the new mode is the line number
anyway, which stays the same.

Maxime
diff mbox series

Patch

diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
index 8f30a530b2d5..255bba563fce 100644
--- a/drivers/gpu/drm/vc4/vc4_vec.c
+++ b/drivers/gpu/drm/vc4/vc4_vec.c
@@ -224,38 +224,24 @@  static const struct debugfs_reg32 vec_regs[] = {
 	VC4_REG32(VEC_DAC_MISC),
 };
 
-static const struct drm_display_mode ntsc_mode = {
-	DRM_MODE("720x480", DRM_MODE_TYPE_DRIVER, 13500,
-		 720, 720 + 14, 720 + 14 + 64, 720 + 14 + 64 + 60, 0,
-		 480, 480 + 7, 480 + 7 + 6, 525, 0,
-		 DRM_MODE_FLAG_INTERLACE)
-};
-
-static const struct drm_display_mode pal_mode = {
-	DRM_MODE("720x576", DRM_MODE_TYPE_DRIVER, 13500,
-		 720, 720 + 20, 720 + 20 + 64, 720 + 20 + 64 + 60, 0,
-		 576, 576 + 4, 576 + 4 + 6, 625, 0,
-		 DRM_MODE_FLAG_INTERLACE)
-};
-
 static const struct vc4_vec_tv_mode vc4_vec_tv_modes[] = {
 	[VC4_VEC_TV_MODE_NTSC] = {
-		.mode = &ntsc_mode,
+		.mode = &drm_mode_480i,
 		.config0 = VEC_CONFIG0_NTSC_STD | VEC_CONFIG0_PDEN,
 		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
 	},
 	[VC4_VEC_TV_MODE_NTSC_J] = {
-		.mode = &ntsc_mode,
+		.mode = &drm_mode_480i,
 		.config0 = VEC_CONFIG0_NTSC_STD,
 		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
 	},
 	[VC4_VEC_TV_MODE_PAL] = {
-		.mode = &pal_mode,
+		.mode = &drm_mode_576i,
 		.config0 = VEC_CONFIG0_PAL_BDGHI_STD,
 		.config1 = VEC_CONFIG1_C_CVBS_CVBS,
 	},
 	[VC4_VEC_TV_MODE_PAL_M] = {
-		.mode = &pal_mode,
+		.mode = &drm_mode_576i,
 		.config0 = VEC_CONFIG0_PAL_BDGHI_STD,
 		.config1 = VEC_CONFIG1_C_CVBS_CVBS | VEC_CONFIG1_CUSTOM_FREQ,
 		.custom_freq = 0x223b61d1,