Re: [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: [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: [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
[PATCH v10 05/19] drm/connector: Add TV standard property
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)
+
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
+ * - saturation
+ * - select subconnector
+ * - subconnector
+ */
+/**
+ * DOC: Analog TV Connector Properties
+ *
+ * TV Mode:
+ * Indicates the TV Mode used on an analog TV connector. The value
+ * of this property can be one of the following:
+ *
+ * NTSC:
+ * TV Mode is CCIR System M (aka 525-lines) together with
+ * the NTSC Color Encoding.
+ *
+ * NTSC-443:
+ *
+ *
