Nouveau - Sep 2022 - [PATCH v2 09/41] 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 bitmask 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.

We'll then be able to phase out the older tv mode property.

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

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 4e4fbc9e0049..b1fcacd150e8 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -980,6 +980,29 @@ 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_443, "NTSC-443" },
+	{ DRM_MODE_TV_MODE_NTSC_J, "NTSC-J" },
+	{ DRM_MODE_TV_MODE_NTSC_M, "NTSC-M" },
+	{ DRM_MODE_TV_MODE_PAL_60, "PAL-60" },
+	{ DRM_MODE_TV_MODE_PAL_B, "PAL-B" },
+	{ DRM_MODE_TV_MODE_PAL_D, "PAL-D" },
+	{ DRM_MODE_TV_MODE_PAL_G, "PAL-G" },
+	{ DRM_MODE_TV_MODE_PAL_H, "PAL-H" },
+	{ DRM_MODE_TV_MODE_PAL_I, "PAL-I" },
+	{ DRM_MODE_TV_MODE_PAL_M, "PAL-M" },
+	{ DRM_MODE_TV_MODE_PAL_N, "PAL-N" },
+	{ DRM_MODE_TV_MODE_PAL_NC, "PAL-Nc" },
+	{ DRM_MODE_TV_MODE_SECAM_60, "SECAM-60" },
+	{ DRM_MODE_TV_MODE_SECAM_B, "SECAM-B" },
+	{ DRM_MODE_TV_MODE_SECAM_D, "SECAM-D" },
+	{ DRM_MODE_TV_MODE_SECAM_G, "SECAM-G" },
+	{ DRM_MODE_TV_MODE_SECAM_K, "SECAM-K" },
+	{ DRM_MODE_TV_MODE_SECAM_K1, "SECAM-K1" },
+	{ DRM_MODE_TV_MODE_SECAM_L, "SECAM-L" },
+};
+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 */
@@ -1645,6 +1668,10 @@ EXPORT_SYMBOL(drm_mode_create_tv_margin_properties);
  * responsible for allocating a list of format names and passing them to
  * this routine.
  *
+ * NOTE: This functions registers the deprecated "mode" connector
+ * property to select the analog TV mode (ie, NTSC, PAL, etc.). New
+ * drivers must use drm_mode_create_tv_properties() instead.
+ *
  * Returns:
  * 0 on success or a negative error code on failure.
  */
@@ -1686,7 +1713,6 @@ int drm_mode_create_tv_properties_legacy(struct drm_device
*dev,
 	if (drm_mode_create_tv_margin_properties(dev))
 		goto nomem;
 
-
 	if (num_modes) {
 		dev->mode_config.legacy_tv_mode_property  			drm_property_create(dev,
DRM_MODE_PROP_ENUM,
@@ -1735,6 +1761,46 @@ int drm_mode_create_tv_properties_legacy(struct
drm_device *dev,
 }
 EXPORT_SYMBOL(drm_mode_create_tv_properties_legacy);
 
+/**
+ * drm_mode_create_tv_properties - create TV specific connector properties
+ * @dev: DRM device
+ * @supported_tv_modes: Bitmask of TV modes supported (See DRM_MODE_TV_MODE_*)
+
+ * Called by a driver's TV initialization routine, this function creates
+ * the TV specific connector properties for a given device.  Caller is
+ * responsible for allocating a list of format names and passing them to
+ * this routine.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+int drm_mode_create_tv_properties(struct drm_device *dev,
+				  unsigned int supported_tv_modes)
+{
+	struct drm_prop_enum_list tv_mode_list[DRM_MODE_TV_MODE_MAX];
+	struct drm_property *tv_mode;
+	unsigned int i, len = 0;
+
+	for (i = 0; i < DRM_MODE_TV_MODE_MAX; i++) {
+		if (!(supported_tv_modes & BIT(i)))
+			continue;
+
+		tv_mode_list[len].type = i;
+		tv_mode_list[len].name = drm_get_tv_mode_name(i);
+		len++;
+	}
+
+	tv_mode = drm_property_create_enum(dev, 0, "TV mode",
+					   tv_mode_list, len);
+	if (!tv_mode)
+		return -ENOMEM;
+
+	dev->mode_config.tv_mode_property = tv_mode;
+
+	return drm_mode_create_tv_properties_legacy(dev, 0, NULL);
+}
+EXPORT_SYMBOL(drm_mode_create_tv_properties);
+
 /**
  * drm_mode_create_scaling_mode_property - create scaling mode property
  * @dev: DRM device
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index d566b4a4709c..7d3881f35e7c 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -143,6 +143,144 @@ enum subpixel_order {
 
 };
 
+/**
+ * enum drm_connector_tv_mode - Analog TV output mode
+ *
+ * This enum is used to indicate the TV output mode used on an analog TV
+ * connector.
+ *
+ * WARNING: The values of this enum is uABI since they're exposed in the
+ * "TV mode" connector property.
+ */
+enum drm_connector_tv_mode {
+	/**
+	 * @DRM_MODE_TV_MODE_NTSC_443: Variant of
+	 * @DRM_MODE_TV_MODE_NTSC_M. Uses a color subcarrier frequency
+	 * of 4.43 MHz.
+	 */
+	DRM_MODE_TV_MODE_NTSC_443 = 0,
+
+	/**
+	 * @DRM_MODE_TV_MODE_NTSC_J: Variant of @DRM_MODE_TV_MODE_NTSC_M
+	 * used in Japan. Uses a black level equals to the blanking
+	 * level.
+	 */
+	DRM_MODE_TV_MODE_NTSC_J,
+
+	/**
+	 * @DRM_MODE_TV_MODE_NTSC_M: CCIR System M (aka 525-lines)
+	 * together with the NTSC Color Encoding.
+	 */
+	DRM_MODE_TV_MODE_NTSC_M,
+
+	/**
+	 * @DRM_MODE_TV_MODE_PAL_60: CCIR System M (aka 525-lines)
+	 * together with the PAL color encoding and color subcarrier
+	 * frequency.
+	 */
+	DRM_MODE_TV_MODE_PAL_60,
+
+	/**
+	 * @DRM_MODE_TV_MODE_PAL_B: CCIR System B together with the PAL
+	 * color system.
+	 */
+	DRM_MODE_TV_MODE_PAL_B,
+
+	/**
+	 * @DRM_MODE_TV_MODE_PAL_D: CCIR System D together with the PAL
+	 * color system.
+	 */
+	DRM_MODE_TV_MODE_PAL_D,
+
+	/**
+	 * @DRM_MODE_TV_MODE_PAL_G: CCIR System G together with the PAL
+	 * color system. Similar to @DRM_MODE_TV_MODE_PAL_B but uses a
+	 * different channel bandwidth and transmission band.
+	 */
+	DRM_MODE_TV_MODE_PAL_G,
+
+	/**
+	 * @DRM_MODE_TV_MODE_PAL_H: CCIR System H together with the PAL
+	 * color system. Similar to @DRM_MODE_TV_MODE_PAL_G but uses a
+	 * narrower lower side band.
+	 */
+	DRM_MODE_TV_MODE_PAL_H,
+
+	/**
+	 * @DRM_MODE_TV_MODE_PAL_I: CCIR System I together with the PAL
+	 * color system. Similar to @DRM_MODE_TV_MODE_PAL_B, but with a
+	 * different audio encoding.
+	 */
+	DRM_MODE_TV_MODE_PAL_I,
+
+	/**
+	 * @DRM_MODE_TV_MODE_PAL_M: CCIR System M (aka 525-lines)
+	 * together with the PAL color encoding
+	 */
+	DRM_MODE_TV_MODE_PAL_M,
+
+	/**
+	 * @DRM_MODE_TV_MODE_PAL_N: CCIR System N together with the PAL
+	 * color encoding. It uses 625 lines, but has a color subcarrier
+	 * frequency of 3.58MHz, the SECAM color space, and narrower
+	 * channels compared to most of the other PAL variants.
+	 */
+	DRM_MODE_TV_MODE_PAL_N,
+
+	/**
+	 * @DRM_MODE_TV_MODE_PAL_NC: Seems equivalent to
+	 * @DRM_MODE_TV_MODE_PAL_N.
+	 */
+	DRM_MODE_TV_MODE_PAL_NC,
+
+	/**
+	 * @DRM_MODE_TV_MODE_SECAM_60: CCIR System M (aka 525-lines)
+	 * together with the SECAM color system.
+	 */
+	DRM_MODE_TV_MODE_SECAM_60,
+
+	/**
+	 * @DRM_MODE_TV_MODE_SECAM_B: CCIR System B together with the
+	 * SECAM color system.
+	 */
+	DRM_MODE_TV_MODE_SECAM_B,
+
+	/**
+	 * @DRM_MODE_TV_MODE_SECAM_D: CCIR System D together with the
+	 * SECAM color system.
+	 */
+	DRM_MODE_TV_MODE_SECAM_D,
+
+	/**
+	 * @DRM_MODE_TV_MODE_SECAM_G: CCIR System G together with the
+	 * SECAM color system. Similar to @DRM_MODE_TV_MODE_SECAM_B but
+	 * uses a different channel bandwidth and transmission band.
+	 */
+	DRM_MODE_TV_MODE_SECAM_G,
+
+	/**
+	 * @DRM_MODE_TV_MODE_SECAM_K: CCIR System G together with the
+	 * SECAM color system. Similar to @DRM_MODE_TV_MODE_SECAM_G but
+	 * with different channels.
+	 */
+	DRM_MODE_TV_MODE_SECAM_K,
+
+	/**
+	 * @DRM_MODE_TV_MODE_SECAM_K1: CCIR System G together with the
+	 * SECAM color system. Similar to @DRM_MODE_TV_MODE_SECAM_G and
+	 * @DRM_MODE_TV_MODE_SECAM_K but with different channels.
+	 */
+	DRM_MODE_TV_MODE_SECAM_K1,
+
+	/**
+	 * @DRM_MODE_TV_MODE_SECAM_L: CCIR System L together with the
+	 * SECAM color system.
+	 */
+	DRM_MODE_TV_MODE_SECAM_L,
+
+	DRM_MODE_TV_MODE_MAX,
+};
+
 /**
  * struct drm_scrambling: sink's scrambling support.
  */
@@ -696,6 +834,7 @@ struct drm_connector_tv_margins {
  * @subconnector: detected subconnector
  * @margins: TV margins
  * @legacy_mode: Legacy TV mode, driver specific value
+ * @mode: TV mode
  * @brightness: brightness in percent
  * @contrast: contrast in percent
  * @flicker_reduction: flicker reduction in percent
@@ -708,6 +847,7 @@ struct drm_tv_connector_state {
 	enum drm_mode_subconnector subconnector;
 	struct drm_connector_tv_margins margins;
 	unsigned int legacy_mode;
+	unsigned int mode;
 	unsigned int brightness;
 	unsigned int contrast;
 	unsigned int flicker_reduction;
@@ -1789,6 +1929,7 @@ const char *drm_get_subpixel_order_name(enum
subpixel_order order);
 const char *drm_get_dpms_name(int val);
 const char *drm_get_dvi_i_subconnector_name(int val);
 const char *drm_get_dvi_i_select_name(int val);
+const char *drm_get_tv_mode_name(int val);
 const char *drm_get_tv_subconnector_name(int val);
 const char *drm_get_tv_select_name(int val);
 const char *drm_get_dp_subconnector_name(int val);
@@ -1802,6 +1943,8 @@ int drm_mode_create_tv_margin_properties(struct drm_device
*dev);
 int drm_mode_create_tv_properties_legacy(struct drm_device *dev,
 					 unsigned int num_modes,
 					 const char * const modes[]);
+int drm_mode_create_tv_properties(struct drm_device *dev,
+				  unsigned int supported_tv_modes);
 void drm_connector_attach_tv_margin_properties(struct drm_connector *conn);
 int drm_mode_create_scaling_mode_property(struct drm_device *dev);
 int drm_connector_attach_content_type_property(struct drm_connector *dev);
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 35a827175c24..10a6f7d1df0d 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -718,9 +718,17 @@ struct drm_mode_config {
 	/**
 	 * @legacy_tv_mode_property: Optional TV property to select
 	 * the output TV mode.
+	 *
+	 * Superseeded by @tv_mode_property
 	 */
 	struct drm_property *legacy_tv_mode_property;
 
+	/**
+	 * @tv_mode_property: Optional TV property to select the TV
+	 * standard output on the connector.
+	 */
+	struct drm_property *tv_mode_property;
+
 	/**
 	 * @tv_left_margin_property: Optional TV property to set the left
 	 * margin (expressed in pixels).

-- 
b4 0.10.0-dev-65ba7

Hi Maxime,

W dniu 29.08.2022 o 15:11, Maxime Ripard pisze:
> 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 bitmask 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.
This is not a bitmask property anymore, you've just changed it to an enum.
The commit message is now misleading.
> +static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = {
> +?? ?{ DRM_MODE_TV_MODE_NTSC_443, "NTSC-443" },
> +?? ?{ DRM_MODE_TV_MODE_NTSC_J, "NTSC-J" },
> +?? ?{ DRM_MODE_TV_MODE_NTSC_M, "NTSC-M" },
> +?? ?{ DRM_MODE_TV_MODE_PAL_60, "PAL-60" },
> +?? ?{ DRM_MODE_TV_MODE_PAL_B, "PAL-B" },
> +?? ?{ DRM_MODE_TV_MODE_PAL_D, "PAL-D" },
> +?? ?{ DRM_MODE_TV_MODE_PAL_G, "PAL-G" },
> +?? ?{ DRM_MODE_TV_MODE_PAL_H, "PAL-H" },
> +?? ?{ DRM_MODE_TV_MODE_PAL_I, "PAL-I" },
> +?? ?{ DRM_MODE_TV_MODE_PAL_M, "PAL-M" },
> +?? ?{ DRM_MODE_TV_MODE_PAL_N, "PAL-N" },
> +?? ?{ DRM_MODE_TV_MODE_PAL_NC, "PAL-Nc" },
> +?? ?{ DRM_MODE_TV_MODE_SECAM_60, "SECAM-60" },
> +?? ?{ DRM_MODE_TV_MODE_SECAM_B, "SECAM-B" },
> +?? ?{ DRM_MODE_TV_MODE_SECAM_D, "SECAM-D" },
> +?? ?{ DRM_MODE_TV_MODE_SECAM_G, "SECAM-G" },
> +?? ?{ DRM_MODE_TV_MODE_SECAM_K, "SECAM-K" },
> +?? ?{ DRM_MODE_TV_MODE_SECAM_K1, "SECAM-K1" },
> +?? ?{ DRM_MODE_TV_MODE_SECAM_L, "SECAM-L" },
> +};
I did not comment on it the last time, but this list looks a little bit random.

Compared to the standards defined by V4L2, you also define SECAM-60 (a good
thing to define, because why not), but don't define PAL-B1, PAL-D1, PAL-K,
SECAM-H, SECAM-LC (whatever that is - probably just another name for SECAM-L,
see my comment about PAL-Nc below), or NTSC-M-KR (a Korean variant of NTSC).

Like I mentioned previously, I'm personally not a fan of including all those
CCIR/ITU system variants, as they don't mean any difference to the output
unless
there is an RF modulator involved. But I get it that they have already been used
and regressing probably wouldn't be a very good idea. But in that case
keeping
it consistent with the set of values used by V4L2 would be wise, I think.
> +/**
> + * drm_mode_create_tv_properties - create TV specific connector properties
> + * @dev: DRM device
> + * @supported_tv_modes: Bitmask of TV modes supported (See
DRM_MODE_TV_MODE_*)
> +
> + * Called by a driver's TV initialization routine, this function
creates
> + * the TV specific connector properties for a given device.? Caller is
> + * responsible for allocating a list of format names and passing them to
> + * this routine.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +int drm_mode_create_tv_properties(struct drm_device *dev,
> +?? ??? ??? ??? ?? unsigned int supported_tv_modes)
supported_tv_modes is supposed to be a bitmask of BIT(DRM_MODE_TV_MODE_*)
(or (1<<DRM_MODE_TV_MODE_*)) rather than DRM_MODE_TV_MODE_* directly, but
this
is not said explicitly anywhere in this doc comment.
> +?? ?/**
> +?? ? * @DRM_MODE_TV_MODE_PAL_NC: Seems equivalent to
> +?? ? * @DRM_MODE_TV_MODE_PAL_N.
> +?? ? */
> +?? ?DRM_MODE_TV_MODE_PAL_NC,
AFAIK, the entire reason that "PAL-Nc" is ever mentioned as something
separate
from PAL-N is a result of a misunderstanding or misreading of the CCIR/ITU
documents. See also the posting signed as Alchaemist here:
https://en.wikipedia.org/wiki/Talk:PAL#PAL-N_versus_PAL-Nc

That being said, we probably want to keep it if we want to remaing compatible
with the loads of software and drivers which enumerate those as separate
systems. But from a technical standpoint, PAL-N and PAL-Nc (and N/PAL, PAL-CN
etc.) are just different "spellings" referring to exactly the same
system.
> +?? ?/**
> +?? ? * @DRM_MODE_TV_MODE_SECAM_K: CCIR System G together with the
> +?? ? * SECAM color system. Similar to @DRM_MODE_TV_MODE_SECAM_G but
> +?? ? * with different channels.
> +?? ? */
> +?? ?DRM_MODE_TV_MODE_SECAM_K,
> +
> +?? ?/**
> +?? ? * @DRM_MODE_TV_MODE_SECAM_K1: CCIR System G together with the
> +?? ? * SECAM color system. Similar to @DRM_MODE_TV_MODE_SECAM_G and
> +?? ? * @DRM_MODE_TV_MODE_SECAM_K but with different channels.
> +?? ? */
> +?? ?DRM_MODE_TV_MODE_SECAM_K1,
Typos: you meant CCIR Systems K and K1, not System G.

Best regards,
Mateusz Kwiatkowski

Den 29.08.2022 15.11, 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 bitmask 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.
> 
> 
> 
> We'll then be able to phase out the older tv mode property.
> 
> 
> 
> Signed-off-by: Maxime Ripard <maxime at cerno.tech>
> 
> 
> 
> diff --git a/drivers/gpu/drm/drm_connector.c
b/drivers/gpu/drm/drm_connector.c
> +/**
> 
> + * drm_mode_create_tv_properties - create TV specific connector properties
> 
> + * @dev: DRM device
> 
> + * @supported_tv_modes: Bitmask of TV modes supported (See
DRM_MODE_TV_MODE_*)
> 
> +
> 
> + * Called by a driver's TV initialization routine, this function
creates
> 
> + * the TV specific connector properties for a given device.  Caller is
> 
> + * responsible for allocating a list of format names and passing them to
> 
> + * this routine.
> 
> + *
> 
> + * Returns:
> 
> + * 0 on success or a negative error code on failure.
> 
> + */
> 
> +int drm_mode_create_tv_properties(struct drm_device *dev,
> 
> +				  unsigned int supported_tv_modes)
> 
> +{
> 
> +	struct drm_prop_enum_list tv_mode_list[DRM_MODE_TV_MODE_MAX];
> 
> +	struct drm_property *tv_mode;
> 
> +	unsigned int i, len = 0;
> 
> +
> 
Can you add a check here like in the legacy version:

	if (dev->mode_config.tv_mode_property)
		return 0;

This way it's possible to call this multiple times. Like in drm/gud
during connector init if there are multiple TV connectors or if a device
with multiple IP blocks should show up.

Noralf.
> +	for (i = 0; i < DRM_MODE_TV_MODE_MAX; i++) {
> 
> +		if (!(supported_tv_modes & BIT(i)))
> 
> +			continue;
> 
> +
> 
> +		tv_mode_list[len].type = i;
> 
> +		tv_mode_list[len].name = drm_get_tv_mode_name(i);
> 
> +		len++;
> 
> +	}
> 
> +
> 
> +	tv_mode = drm_property_create_enum(dev, 0, "TV mode",
> 
> +					   tv_mode_list, len);
> 
> +	if (!tv_mode)
> 
> +		return -ENOMEM;
> 
> +
> 
> +	dev->mode_config.tv_mode_property = tv_mode;
> 
> +
> 
> +	return drm_mode_create_tv_properties_legacy(dev, 0, NULL);
> 
> +}
> 
> +EXPORT_SYMBOL(drm_mode_create_tv_properties);
>