Re: [PATCH v10 05/19] drm/connector: Add TV standard property

[Maxime Ripard <maxime@cerno.tech>]

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

On Thu, Nov 17, 2022 at 03:35:57PM +0100, Mauro Carvalho Chehab wrote:
> On Thu, 17 Nov 2022 10:28:48 +0100
> Maxime Ripard <maxime@cerno.tech> wrote:
> 
> > 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 enum 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, and the property
> > creation function will filter out the modes not supported.
> > 
> > We'll then be able to phase out the older tv mode property.
> > 
> > Tested-by: Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > 
> > ---
> > Changes in v10:
> > - Fix checkpatch warning
> > 
> > Changes in v5:
> > - Create an analog TV properties documentation section, and document TV
> >   Mode there instead of the csv file
> > 
> > Changes in v4:
> > - Add property documentation to kms-properties.csv
> > - Fix documentation
> > ---
> >  Documentation/gpu/drm-kms.rst     |   6 ++
> >  drivers/gpu/drm/drm_atomic_uapi.c |   4 ++
> >  drivers/gpu/drm/drm_connector.c   | 122 +++++++++++++++++++++++++++++++++++++-
> >  include/drm/drm_connector.h       |  64 ++++++++++++++++++++
> >  include/drm/drm_mode_config.h     |   8 +++
> >  5 files changed, 203 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> > index b4377a545425..321f2f582c64 100644
> > --- a/Documentation/gpu/drm-kms.rst
> > +++ b/Documentation/gpu/drm-kms.rst
> > @@ -520,6 +520,12 @@ HDMI Specific Connector Properties
> >  .. kernel-doc:: drivers/gpu/drm/drm_connector.c
> >     :doc: HDMI connector properties
> >  
> > +Analog TV Specific Connector Properties
> > +----------------------------------
> > +
> > +.. kernel-doc:: drivers/gpu/drm/drm_connector.c
> > +   :doc: Analog TV Connector Properties
> > +
> >  Standard CRTC Properties
> >  ------------------------
> >  
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index 7f2b9a07fbdf..d867e7f9f2cd 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -700,6 +700,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
> >  		state->tv.margins.bottom = val;
> >  	} else if (property == config->legacy_tv_mode_property) {
> >  		state->tv.legacy_mode = val;
> > +	} else if (property == config->tv_mode_property) {
> > +		state->tv.mode = val;
> >  	} else if (property == config->tv_brightness_property) {
> >  		state->tv.brightness = val;
> >  	} else if (property == config->tv_contrast_property) {
> > @@ -810,6 +812,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
> >  		*val = state->tv.margins.bottom;
> >  	} else if (property == config->legacy_tv_mode_property) {
> >  		*val = state->tv.legacy_mode;
> > +	} else if (property == config->tv_mode_property) {
> > +		*val = state->tv.mode;
> >  	} else if (property == config->tv_brightness_property) {
> >  		*val = state->tv.brightness;
> >  	} else if (property == config->tv_contrast_property) {
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index 06e737ed15f5..07d449736956 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -984,6 +984,17 @@ static const struct drm_prop_enum_list drm_dvi_i_subconnector_enum_list[] = {
> >  DRM_ENUM_NAME_FN(drm_get_dvi_i_subconnector_name,
> >  		 drm_dvi_i_subconnector_enum_list)
> >  
> > +static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = {
> > +	{ DRM_MODE_TV_MODE_NTSC, "NTSC" },
> > +	{ DRM_MODE_TV_MODE_NTSC_443, "NTSC-443" },
> > +	{ DRM_MODE_TV_MODE_NTSC_J, "NTSC-J" },
> > +	{ DRM_MODE_TV_MODE_PAL, "PAL" },
> > +	{ DRM_MODE_TV_MODE_PAL_M, "PAL-M" },
> > +	{ DRM_MODE_TV_MODE_PAL_N, "PAL-N" },
> > +	{ DRM_MODE_TV_MODE_SECAM, "SECAM" },
> > +};
> 
> Nack. It sounds a very bad idea to have standards as generic as 
> NTSC, PAL, SECAM. 
> 
> If you take a look at the CCIR/ITU-R specs that define video standards, 
> you'll see that the standard has actually two components:
> 
> 1. the composite color TV signal: PAL, NTSC, SECAM, defined in ITU-R BT1700[1]
> 
> 2. and the conventional analogue TV (the "monochromatic" part),
> as defined in ITU-R BT.1701[2], which is, basically, a letter from A to N
> (with some country-specific variants, like Nc). Two of those standards
> (M and J) are used on Countries with a power grid of 60Hz, as they have
> a frame rate of either 30fps or 29.997fps.
> 
> [1] https://www.itu.int/rec/R-REC-BT.1700-0-200502-I/en
> [2] https://www.itu.int/rec/R-REC-BT.1701-1-200508-I/en
> 
> The actual combination is defined within Country-specific laws, which
> selects a conventional analogue signal with a composite color one.
> 
> So, for instance, US uses NTSC/M (because it uses a 60Hz power grid).
> There is a 50Hz variant, called NTSC/443 (not used on any Country, but
> present on some European VCR equipments capable of recording at 25fps,
> using NTSC).
> 
> Btw, some VCR equipments in US may also have PAL/60 with has the
> same timings as NTSC, but uses PAL instead.
> 
> What happens is that, in Europe, different PAL standards got used, but:
> 
> - most TV sets and their chipsets were developed to auto-detect and
>   support the differences between different systems PAL/B, PAL/G, PAL/D,...
> - several of those standards have a difference only at the audio
>   sub-carriers. So, they look identical for the video decoding part.
> - standards may have a different inter-channel space (it can vary from
>   5 to 8 MHz) to minimize cross-signal interference.

We've had that discussion already, at v3:
https://lore.kernel.org/dri-devel/20220728-rpi-analog-tv-properties-v2-9-459522d653a7@cerno.tech/

AFAICS, we can easily add the extra standards to the properties list if
and when needed.

So unless you can come up with some practical issues that can't be
addressed by the current design without a major rework, I don't intend
to change that.

Maxime

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