Re: [PATCH v6 22/23] drm/vc4: vec: Add support for more analog TV standards

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1: Type: text/plain, Size: 3333 bytes --]

Hi,

On Thu, Oct 27, 2022 at 12:32:50AM +0200, Mateusz Kwiatkowski wrote:
> 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-a73d9dae1af4@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.

ACK, I'll add it.

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

This series is already pretty massive and is difficult to merge, so I'd
rather avoid to add new stuff in at every version. The changes look easy
enough to be a follow-up patch, so I'd prefer to do it that way.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]