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 <[email protected]>
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
Hi Mateusz,
On Fri, Sep 2, 2022 at 12:00 AM Mateusz Kwiatkowski <[email protected]> wrote:
> 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.
Exactly. Anything outputting RGB (e.g. through a SCART or VGA connector)
doesn't care about the color subcarrier or modulator parts. Likewise,
anything outputting CVBS doesn't care about the modulator part.
Perhaps "generic" variants of NSTC and PAL/SECAM should be added, which
would really just mean 525/60 resp. 625/50.
Alternatively, the tv_mode field could be split in two parts (either
two separate fields, or bitwise), to maintain a clear separation between
lines/fields versus color encoding and RF modulation (with zero for the
latter meaning a generic version)? That would also keep the door open
for TV_MODE_405_50, TV_MODE_819_50, TV_MODE_750_50, TV_MODE_750_60, ...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
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 <[email protected]>
>
>
>
> 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);
>
Hi,
On Fri, Sep 02, 2022 at 12:00:33AM +0200, Mateusz Kwiatkowski wrote:
> 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.
Ack. What would be the list of standards we'd absolutely need? NSTC-M,
NTSC-J, PAL-60, PAL-B, PAL-M, SECAM-60 and SECAM-B?
> > +/**
> > + * 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.
The argument doc mentions that it's a "Bitmask of TV modes supported
(See DRM_MODE_TV_MODE_*)", how would you improve it?
Thanks!
Maxime
On Fri, Sep 02, 2022 at 09:35:20AM +0200, Geert Uytterhoeven wrote:
> On Fri, Sep 2, 2022 at 12:00 AM Mateusz Kwiatkowski <[email protected]> wrote:
> > 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.
>
> Exactly. Anything outputting RGB (e.g. through a SCART or VGA connector)
> doesn't care about the color subcarrier or modulator parts. Likewise,
> anything outputting CVBS doesn't care about the modulator part.
>
> Perhaps "generic" variants of NSTC and PAL/SECAM should be added, which
> would really just mean 525/60 resp. 625/50.
>
> Alternatively, the tv_mode field could be split in two parts (either
> two separate fields, or bitwise), to maintain a clear separation between
> lines/fields versus color encoding and RF modulation (with zero for the
> latter meaning a generic version)? That would also keep the door open
> for TV_MODE_405_50, TV_MODE_819_50, TV_MODE_750_50, TV_MODE_750_60, ...
Again, that property is only about color encoding and RF modulation. The
lines numbers and whether it's interlaced or not is encoded in the mode,
not here. So what you suggest is totally doable today.
Maxime
Hi Maxime,
W dniu 7.09.2022 o 14:10, Maxime Ripard pisze:
> Hi,
>
> On Fri, Sep 02, 2022 at 12:00:33AM +0200, Mateusz Kwiatkowski wrote:
>> 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.
>
> Ack. What would be the list of standards we'd absolutely need? NSTC-M,
> NTSC-J, PAL-60, PAL-B, PAL-M, SECAM-60 and SECAM-B?
The "essential list" IMO is NTSC, NTSC-J, NTSC-443, PAL, PAL-M, PAL-N and SECAM.
Note that:
- I intentionally propose "NTSC", "PAL" and "SECAM" without an ITU system
designation. If we only consider composite signals, there's no difference
between e.g. PAL-B, PAL-D and PAL-I, so it's better to label it as a generic
mode, IMO.
* PAL-M and PAL-N are different, because those unique color encodings were
only ever used with Systems M and N, respectively.
* NTSC-J is also different, because "System J" doesn't exist anywhere in ITU
documents. Japan technically uses System M with a non-standard black level.
But "NTSC-J" stuck as a universally recognized name for that variant.
- I intentionally did not list PAL-60 or SECAM-60. TBH... PAL-60 is just
regular PAL paired with 480i60 modeline. Most if not all displays that
accept PAL-60 input will just label it as "PAL". If we are not introducing
strict modeline validation, then maybe separating PAL and PAL-60 isn't really
necessary? Same goes for SECAM vs. SECAM-60.
...and same goes for NTSC vs. NTSC-50 a.k.a NTSC-N, which is a very exotic
mode, but known to exist at least in the Atari ST world, see also:
https://en.wikipedia.org/wiki/NTSC#NTSC-N/NTSC50
Combining PAL and PAL-60 into a single setting would complicate the vc4 driver
a little bit, though, as the registers need to be set up differently for those.
My feelings about the PAL-60 issue are not that strong, though. Merging PAL
and PAL-60 in this context is just a loose suggestion, I won't even try to
argue if you disagree.
>>> +/**
>>> + * 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.
>
> The argument doc mentions that it's a "Bitmask of TV modes supported
> (See DRM_MODE_TV_MODE_*)", how would you improve it?
Maybe something like "Bitwise OR of BIT(DRM_MODE_TV_MODE_*) values"? Or maybe
just add a little usage example?
> Thanks!
> Maxime
Best regards,
Mateusz Kwiatkowski
On Wed, Sep 07, 2022 at 09:52:09PM +0200, Mateusz Kwiatkowski wrote:
> W dniu 7.09.2022 o 14:10, Maxime Ripard pisze:
> > Hi,
> >
> > On Fri, Sep 02, 2022 at 12:00:33AM +0200, Mateusz Kwiatkowski wrote:
> >> 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.
> >
> > Ack. What would be the list of standards we'd absolutely need? NSTC-M,
> > NTSC-J, PAL-60, PAL-B, PAL-M, SECAM-60 and SECAM-B?
>
> The "essential list" IMO is NTSC, NTSC-J, NTSC-443, PAL, PAL-M, PAL-N and SECAM.
> Note that:
>
> - I intentionally propose "NTSC", "PAL" and "SECAM" without an ITU system
> ? designation. If we only consider composite signals, there's no difference
> ? between e.g. PAL-B, PAL-D and PAL-I, so it's better to label it as a generic
> ? mode, IMO.
>
> ? * PAL-M and PAL-N are different, because those unique color encodings were
> ??? only ever used with Systems M and N, respectively.
>
> ? * NTSC-J is also different, because "System J" doesn't exist anywhere in ITU
> ??? documents. Japan technically uses System M with a non-standard black level.
> ??? But "NTSC-J" stuck as a universally recognized name for that variant.
>
> - I intentionally did not list PAL-60 or SECAM-60. TBH... PAL-60 is just
> ? regular PAL paired with 480i60 modeline. Most if not all displays that
> ? accept PAL-60 input will just label it as "PAL". If we are not introducing
> ? strict modeline validation, then maybe separating PAL and PAL-60 isn't really
> ? necessary? Same goes for SECAM vs. SECAM-60.
> ?
> ? ...and same goes for NTSC vs. NTSC-50 a.k.a NTSC-N, which is a very exotic
> ? mode, but known to exist at least in the Atari ST world, see also:
> ? https://en.wikipedia.org/wiki/NTSC#NTSC-N/NTSC50
>
> Combining PAL and PAL-60 into a single setting would complicate the vc4 driver
> a little bit, though, as the registers need to be set up differently for those.
>
> My feelings about the PAL-60 issue are not that strong, though. Merging PAL
> and PAL-60 in this context is just a loose suggestion, I won't even try to
> argue if you disagree.
Ack
> >>> +/**
> >>> + * 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.
> >
> > The argument doc mentions that it's a "Bitmask of TV modes supported
> > (See DRM_MODE_TV_MODE_*)", how would you improve it?
>
> Maybe something like "Bitwise OR of BIT(DRM_MODE_TV_MODE_*) values"? Or maybe
> just add a little usage example?
This is the way we're usually documenting it in DRM:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_plane.c#L357
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_crtc.c#L861
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_blend.c#L546
So I'd rather keep it consistent
Maxime
W dniu 9.09.2022 o 11:46, Maxime Ripard pisze:
> On Wed, Sep 07, 2022 at 09:52:09PM +0200, Mateusz Kwiatkowski wrote:
>> W dniu 7.09.2022 o 14:10, Maxime Ripard pisze:
>>> Hi,
>>>
>>> On Fri, Sep 02, 2022 at 12:00:33AM +0200, Mateusz Kwiatkowski wrote:
>>>> 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.
>>>
>>> Ack. What would be the list of standards we'd absolutely need? NSTC-M,
>>> NTSC-J, PAL-60, PAL-B, PAL-M, SECAM-60 and SECAM-B?
>>
>> The "essential list" IMO is NTSC, NTSC-J, NTSC-443, PAL, PAL-M, PAL-N and SECAM.
>> Note that:
>>
>> - I intentionally propose "NTSC", "PAL" and "SECAM" without an ITU system
>> designation. If we only consider composite signals, there's no difference
>> between e.g. PAL-B, PAL-D and PAL-I, so it's better to label it as a generic
>> mode, IMO.
>>
>> * PAL-M and PAL-N are different, because those unique color encodings were
>> only ever used with Systems M and N, respectively.
>>
>> * NTSC-J is also different, because "System J" doesn't exist anywhere in ITU
>> documents. Japan technically uses System M with a non-standard black level.
>> But "NTSC-J" stuck as a universally recognized name for that variant.
>>
>> - I intentionally did not list PAL-60 or SECAM-60. TBH... PAL-60 is just
>> regular PAL paired with 480i60 modeline. Most if not all displays that
>> accept PAL-60 input will just label it as "PAL". If we are not introducing
>> strict modeline validation, then maybe separating PAL and PAL-60 isn't really
>> necessary? Same goes for SECAM vs. SECAM-60.
>>
>> ...and same goes for NTSC vs. NTSC-50 a.k.a NTSC-N, which is a very exotic
>> mode, but known to exist at least in the Atari ST world, see also:
>> https://en.wikipedia.org/wiki/NTSC#NTSC-N/NTSC50
>>
>> Combining PAL and PAL-60 into a single setting would complicate the vc4 driver
>> a little bit, though, as the registers need to be set up differently for those.
>>
>> My feelings about the PAL-60 issue are not that strong, though. Merging PAL
>> and PAL-60 in this context is just a loose suggestion, I won't even try to
>> argue if you disagree.
>
> Ack
>
>>>>> +/**
>>>>> + * 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.
>>>
>>> The argument doc mentions that it's a "Bitmask of TV modes supported
>>> (See DRM_MODE_TV_MODE_*)", how would you improve it?
>>
>> Maybe something like "Bitwise OR of BIT(DRM_MODE_TV_MODE_*) values"? Or maybe
>> just add a little usage example?
>
> This is the way we're usually documenting it in DRM:
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_plane.c#L357
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_crtc.c#L861
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_blend.c#L546
>
> So I'd rather keep it consistent
>
> Maxime
Ack, OK then.