Re: [Intel-gfx] [PATCH v10 05/19] drm/connector: Add TV standard property
Den 17.11.2022 10.28, skrev Maxime Ripard:
> 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
> Signed-off-by: Maxime Ripard
>
> ---
> 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" },
> +};
> +DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list)
> +
This patch looks good but since I'm no TV standards expert I can't say
if the content of this list is a good choice for reflecting the world of
TV standards.
Acked-by: Noralf Trønnes
> static const struct drm_prop_enum_list drm_tv_select_enum_list[] = {
> { DRM_MODE_SUBCONNECTOR_Automatic, "Automatic" }, /* DVI-I and TV-out */
> { DRM_MODE_SUBCONNECTOR_Composite, "Composite" }, /* TV-out */
> @@ -1552,6 +1563,71 @@
> EXPORT_SYMBOL(drm_connector_attach_dp_subconnector_property);
> * infoframe values is done through drm_hdmi_avi_infoframe_content_type().
> */
>
> +/*
> + * TODO: Document the properties:
> + * - left margin
> + * - right margin
> + * - top margin
> + * - bottom margin
> + * - brightness
> + * - contrast
> + * - flicker reduction
> + * - hue
> + * - mode
> + * - overscan
> + *
Re: [Intel-gfx] [PATCH v10 05/19] drm/connector: Add TV standard property
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 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
> > Signed-off-by: Maxime Ripard
> >
> > ---
> > 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]
Re: [Intel-gfx] [PATCH v10 05/19] drm/connector: Add TV standard property
On Thu, 17 Nov 2022 10:28:48 +0100
Maxime Ripard 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
> Signed-off-by: Maxime Ripard
>
> ---
> 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
