| Message ID | 20220728-rpi-analog-tv-properties-v1-5-3d53ae722097@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Analog TV Improvements | expand |
Den 29.07.2022 18.34, 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@cerno.tech> > Please also update Documentation/gpu/kms-properties.csv Requirements for adding a new property is found in Documentation/gpu/drm-kms.rst > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > index c06d0639d552..d7ff6c644c2f 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->tv_mode_property) { > state->tv.mode = val; > + } else if (property == config->tv_norm_property) { > + state->tv.norm = 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->tv_mode_property) { > *val = state->tv.mode; > + } else if (property == config->tv_norm_property) { > + *val = state->tv.norm; > } 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 e3142c8142b3..68a4e47f85a9 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -1637,6 +1637,7 @@ EXPORT_SYMBOL(drm_mode_create_tv_margin_properties); > /** > * drm_mode_create_tv_properties - create TV specific connector properties > * @dev: DRM device > + * @supported_tv_norms: Bitmask of TV norms supported (See DRM_MODE_TV_NORM_*) > * @num_modes: number of different TV formats (modes) supported > * @modes: array of pointers to strings containing name of each format > * > @@ -1649,11 +1650,40 @@ EXPORT_SYMBOL(drm_mode_create_tv_margin_properties); > * 0 on success or a negative error code on failure. > */ > int drm_mode_create_tv_properties(struct drm_device *dev, > + unsigned int supported_tv_norms, > unsigned int num_modes, > const char * const modes[]) > { > + static const struct drm_prop_enum_list tv_norm_values[] = { > + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_443) - 1, "NTSC-443" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_J) - 1, "NTSC-J" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_M) - 1, "NTSC-M" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_60) - 1, "PAL-60" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_B) - 1, "PAL-B" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_D) - 1, "PAL-D" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_G) - 1, "PAL-G" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_H) - 1, "PAL-H" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_I) - 1, "PAL-I" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_M) - 1, "PAL-M" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_N) - 1, "PAL-N" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_NC) - 1, "PAL-Nc" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_60) - 1, "SECAM-60" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_B) - 1, "SECAM-B" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_D) - 1, "SECAM-D" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_G) - 1, "SECAM-G" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_K) - 1, "SECAM-K" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_K1) - 1, "SECAM-K1" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_L) - 1, "SECAM-L" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_HD480I) - 1, "hd480i" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_HD480P) - 1, "hd480p" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_HD576I) - 1, "hd576i" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_HD576P) - 1, "hd576p" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_HD720P) - 1, "hd720p" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_HD1080I) - 1, "hd1080i" }, > + }; > struct drm_property *tv_selector; > struct drm_property *tv_subconnector; > + struct drm_property *tv_norm; > unsigned int i; > > if (dev->mode_config.tv_select_subconnector_property) > @@ -1686,6 +1716,13 @@ int drm_mode_create_tv_properties(struct drm_device *dev, > if (drm_mode_create_tv_margin_properties(dev)) > goto nomem; > > + tv_norm = drm_property_create_bitmask(dev, 0, "tv norm", > + tv_norm_values, ARRAY_SIZE(tv_norm_values), > + supported_tv_norms); I expected this to be an enum, why a bitmask? Userspace can set multiple bits in a bitmask. Noralf. > + if (!tv_norm) > + goto nomem; > + dev->mode_config.tv_norm_property = tv_norm; > + > dev->mode_config.tv_mode_property = > drm_property_create(dev, DRM_MODE_PROP_ENUM, > "mode", num_modes); > diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c > index 4a788c1c9058..457529e5d857 100644 > --- a/drivers/gpu/drm/vc4/vc4_vec.c > +++ b/drivers/gpu/drm/vc4/vc4_vec.c > @@ -573,7 +573,9 @@ static int vc4_vec_bind(struct device *dev, struct device *master, void *data) > struct vc4_vec *vec; > int ret; > > - ret = drm_mode_create_tv_properties(drm, ARRAY_SIZE(tv_mode_names), > + ret = drm_mode_create_tv_properties(drm, > + 0, > + ARRAY_SIZE(tv_mode_names), > tv_mode_names); > if (ret) > return ret; > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index 1e9996b33cc8..78275e68ff66 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -143,6 +143,32 @@ enum subpixel_order { > > }; > > +#define DRM_MODE_TV_NORM_NTSC_443 (1 << 0) > +#define DRM_MODE_TV_NORM_NTSC_J (1 << 1) > +#define DRM_MODE_TV_NORM_NTSC_M (1 << 2) > +#define DRM_MODE_TV_NORM_PAL_60 (1 << 3) > +#define DRM_MODE_TV_NORM_PAL_B (1 << 4) > +#define DRM_MODE_TV_NORM_PAL_D (1 << 5) > +#define DRM_MODE_TV_NORM_PAL_G (1 << 6) > +#define DRM_MODE_TV_NORM_PAL_H (1 << 7) > +#define DRM_MODE_TV_NORM_PAL_I (1 << 8) > +#define DRM_MODE_TV_NORM_PAL_M (1 << 9) > +#define DRM_MODE_TV_NORM_PAL_N (1 << 10) > +#define DRM_MODE_TV_NORM_PAL_NC (1 << 11) > +#define DRM_MODE_TV_NORM_SECAM_60 (1 << 12) > +#define DRM_MODE_TV_NORM_SECAM_B (1 << 13) > +#define DRM_MODE_TV_NORM_SECAM_D (1 << 14) > +#define DRM_MODE_TV_NORM_SECAM_G (1 << 15) > +#define DRM_MODE_TV_NORM_SECAM_K (1 << 16) > +#define DRM_MODE_TV_NORM_SECAM_K1 (1 << 17) > +#define DRM_MODE_TV_NORM_SECAM_L (1 << 18) > +#define DRM_MODE_TV_NORM_HD480I (1 << 19) > +#define DRM_MODE_TV_NORM_HD480P (1 << 20) > +#define DRM_MODE_TV_NORM_HD576I (1 << 21) > +#define DRM_MODE_TV_NORM_HD576P (1 << 22) > +#define DRM_MODE_TV_NORM_HD720P (1 << 23) > +#define DRM_MODE_TV_NORM_HD1080I (1 << 24) > + > /** > * struct drm_scrambling: sink's scrambling support. > */ > @@ -687,6 +713,7 @@ struct drm_tv_connector_state { > enum drm_mode_subconnector subconnector; > struct drm_connector_tv_margins margins; > unsigned int mode; > + unsigned int norm; > unsigned int brightness; > unsigned int contrast; > unsigned int flicker_reduction; > @@ -1779,6 +1806,7 @@ void drm_connector_attach_dp_subconnector_property(struct drm_connector *connect > > int drm_mode_create_tv_margin_properties(struct drm_device *dev); > int drm_mode_create_tv_properties(struct drm_device *dev, > + unsigned int supported_tv_norms, > unsigned int num_modes, > const char * const modes[]); > void drm_connector_attach_tv_margin_properties(struct drm_connector *conn); > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > index 6b5e01295348..d9e79def8b92 100644 > --- a/include/drm/drm_mode_config.h > +++ b/include/drm/drm_mode_config.h > @@ -704,6 +704,12 @@ struct drm_mode_config { > */ > struct drm_property *dp_subconnector_property; > > + /** > + * @tv_norm_property: Optional TV property to select the TV > + * standard output on the connector. > + */ > + struct drm_property *tv_norm_property; > + > /** > * @tv_subconnector_property: Optional TV property to differentiate > * between different TV connector types. >
Hi Maxime, On Fri, Jul 29, 2022 at 6:35 PM Maxime Ripard <maxime@cerno.tech> wrote: > The TV mode property has been around for a while now to select and get the > current TV mode output on an analog TV connector. > > Despite that property name being generic, its content isn't and has been > driver-specific which makes it hard to build any generic behaviour on top > of it, both in kernel and user-space. > > Let's create a new 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@cerno.tech> Thanks for your patch! > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -1649,11 +1650,40 @@ EXPORT_SYMBOL(drm_mode_create_tv_margin_properties); > * 0 on success or a negative error code on failure. > */ > int drm_mode_create_tv_properties(struct drm_device *dev, > + unsigned int supported_tv_norms, > unsigned int num_modes, > const char * const modes[]) > { > + static const struct drm_prop_enum_list tv_norm_values[] = { > + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_443) - 1, "NTSC-443" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_J) - 1, "NTSC-J" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_M) - 1, "NTSC-M" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_60) - 1, "PAL-60" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_B) - 1, "PAL-B" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_D) - 1, "PAL-D" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_G) - 1, "PAL-G" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_H) - 1, "PAL-H" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_I) - 1, "PAL-I" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_M) - 1, "PAL-M" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_N) - 1, "PAL-N" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_NC) - 1, "PAL-Nc" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_60) - 1, "SECAM-60" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_B) - 1, "SECAM-B" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_D) - 1, "SECAM-D" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_G) - 1, "SECAM-G" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_K) - 1, "SECAM-K" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_K1) - 1, "SECAM-K1" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_L) - 1, "SECAM-L" }, The above are analog standards, with a variable horizontal resolution. > + { __builtin_ffs(DRM_MODE_TV_NORM_HD480I) - 1, "hd480i" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_HD480P) - 1, "hd480p" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_HD576I) - 1, "hd576i" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_HD576P) - 1, "hd576p" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_HD720P) - 1, "hd720p" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_HD1080I) - 1, "hd1080i" }, The above are digital standards, with a fixed resolution. You seem to have missed "hd1080p"? In addition, "hd720p", "hd080i", and "hd1080p" are available in both 50 and 60 (actually 59.94) Hz, while "hd1080p" can also use 24 or 25 Hz. Either you have to add them here (e.g. "hd720p50" and "hd720p60"), or handle them through "@<refresh>". The latter would impact "[PATCH v1 09/35] drm/modes: Move named modes parsing to a separate function", as currently a named mode and a refresh rate can't be specified both. As "[PATCH v1 34/35] drm/modes: Introduce the tv_mode property as a command-line option" uses a separate "tv_mode" option, and not the main mode name, I think you want to add them here. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
Hi, On Mon, Aug 08, 2022 at 02:44:56PM +0200, Noralf Trønnes wrote: > Den 29.07.2022 18.34, 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@cerno.tech> > > > > Please also update Documentation/gpu/kms-properties.csv > > Requirements for adding a new property is found in > Documentation/gpu/drm-kms.rst I knew this was going to be raised at some point, so I'm glad it's that early :) I really don't know what to do there. If we stick by our usual rules, then we can't have any of that work merged. However, I think the status quo is not really satisfactory either. Indeed, we have a property, that doesn't follow those requirements either, with a driver-specific content, and that stands in the way of fixes and further improvements at both the core framework and driver levels. So having that new property still seems like a net improvement at the driver, framework and uAPI levels, even if it's not entirely following the requirements we have in place. Even more so since, realistically, those kind of interfaces will never get any new development on the user-space side of things, it's considered by everyone as legacy. This also is something we need to support at some point if we want to completely deprecate the fbdev drivers and provide decent alternatives in KMS. So yeah, strictly speaking, we would not qualify for our requirements there. I still think we have a strong case for an exception though. > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > > index c06d0639d552..d7ff6c644c2f 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->tv_mode_property) { > > state->tv.mode = val; > > + } else if (property == config->tv_norm_property) { > > + state->tv.norm = 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->tv_mode_property) { > > *val = state->tv.mode; > > + } else if (property == config->tv_norm_property) { > > + *val = state->tv.norm; > > } 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 e3142c8142b3..68a4e47f85a9 100644 > > --- a/drivers/gpu/drm/drm_connector.c > > +++ b/drivers/gpu/drm/drm_connector.c > > @@ -1637,6 +1637,7 @@ EXPORT_SYMBOL(drm_mode_create_tv_margin_properties); > > /** > > * drm_mode_create_tv_properties - create TV specific connector properties > > * @dev: DRM device > > + * @supported_tv_norms: Bitmask of TV norms supported (See DRM_MODE_TV_NORM_*) > > * @num_modes: number of different TV formats (modes) supported > > * @modes: array of pointers to strings containing name of each format > > * > > @@ -1649,11 +1650,40 @@ EXPORT_SYMBOL(drm_mode_create_tv_margin_properties); > > * 0 on success or a negative error code on failure. > > */ > > int drm_mode_create_tv_properties(struct drm_device *dev, > > + unsigned int supported_tv_norms, > > unsigned int num_modes, > > const char * const modes[]) > > { > > + static const struct drm_prop_enum_list tv_norm_values[] = { > > + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_443) - 1, "NTSC-443" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_J) - 1, "NTSC-J" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_M) - 1, "NTSC-M" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_60) - 1, "PAL-60" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_B) - 1, "PAL-B" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_D) - 1, "PAL-D" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_G) - 1, "PAL-G" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_H) - 1, "PAL-H" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_I) - 1, "PAL-I" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_M) - 1, "PAL-M" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_N) - 1, "PAL-N" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_NC) - 1, "PAL-Nc" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_60) - 1, "SECAM-60" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_B) - 1, "SECAM-B" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_D) - 1, "SECAM-D" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_G) - 1, "SECAM-G" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_K) - 1, "SECAM-K" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_K1) - 1, "SECAM-K1" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_L) - 1, "SECAM-L" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD480I) - 1, "hd480i" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD480P) - 1, "hd480p" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD576I) - 1, "hd576i" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD576P) - 1, "hd576p" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD720P) - 1, "hd720p" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD1080I) - 1, "hd1080i" }, > > + }; > > struct drm_property *tv_selector; > > struct drm_property *tv_subconnector; > > + struct drm_property *tv_norm; > > unsigned int i; > > > > if (dev->mode_config.tv_select_subconnector_property) > > @@ -1686,6 +1716,13 @@ int drm_mode_create_tv_properties(struct drm_device *dev, > > if (drm_mode_create_tv_margin_properties(dev)) > > goto nomem; > > > > + tv_norm = drm_property_create_bitmask(dev, 0, "tv norm", > > + tv_norm_values, ARRAY_SIZE(tv_norm_values), > > + supported_tv_norms); > > I expected this to be an enum, why a bitmask? Userspace can set multiple > bits in a bitmask. I went for a bitmask since it allowed to report the capabilities of a driver, but I just realised that you can do that with an enum too, like we do for color encodings. I'll switch for an enum, thanks! Maxime
Den 16.08.2022 10.26, skrev Maxime Ripard: > Hi, > > On Mon, Aug 08, 2022 at 02:44:56PM +0200, Noralf Trønnes wrote: >> Den 29.07.2022 18.34, 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@cerno.tech> >>> >> >> Please also update Documentation/gpu/kms-properties.csv >> >> Requirements for adding a new property is found in >> Documentation/gpu/drm-kms.rst > > I knew this was going to be raised at some point, so I'm glad it's that > early :) > > I really don't know what to do there. If we stick by our usual rules, > then we can't have any of that work merged. > > However, I think the status quo is not really satisfactory either. > Indeed, we have a property, that doesn't follow those requirements > either, with a driver-specific content, and that stands in the way of > fixes and further improvements at both the core framework and driver > levels. > > So having that new property still seems like a net improvement at the > driver, framework and uAPI levels, even if it's not entirely following > the requirements we have in place. > > Even more so since, realistically, those kind of interfaces will never > get any new development on the user-space side of things, it's > considered by everyone as legacy. > > This also is something we need to support at some point if we want to > completely deprecate the fbdev drivers and provide decent alternatives > in KMS. > > So yeah, strictly speaking, we would not qualify for our requirements > there. I still think we have a strong case for an exception though. > Which requirements would that be? The only one I can see is the documentation and maybe an IGT test. Noralf. >>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c >>> index c06d0639d552..d7ff6c644c2f 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->tv_mode_property) { >>> state->tv.mode = val; >>> + } else if (property == config->tv_norm_property) { >>> + state->tv.norm = 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->tv_mode_property) { >>> *val = state->tv.mode; >>> + } else if (property == config->tv_norm_property) { >>> + *val = state->tv.norm; >>> } 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 e3142c8142b3..68a4e47f85a9 100644 >>> --- a/drivers/gpu/drm/drm_connector.c >>> +++ b/drivers/gpu/drm/drm_connector.c >>> @@ -1637,6 +1637,7 @@ EXPORT_SYMBOL(drm_mode_create_tv_margin_properties); >>> /** >>> * drm_mode_create_tv_properties - create TV specific connector properties >>> * @dev: DRM device >>> + * @supported_tv_norms: Bitmask of TV norms supported (See DRM_MODE_TV_NORM_*) >>> * @num_modes: number of different TV formats (modes) supported >>> * @modes: array of pointers to strings containing name of each format >>> * >>> @@ -1649,11 +1650,40 @@ EXPORT_SYMBOL(drm_mode_create_tv_margin_properties); >>> * 0 on success or a negative error code on failure. >>> */ >>> int drm_mode_create_tv_properties(struct drm_device *dev, >>> + unsigned int supported_tv_norms, >>> unsigned int num_modes, >>> const char * const modes[]) >>> { >>> + static const struct drm_prop_enum_list tv_norm_values[] = { >>> + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_443) - 1, "NTSC-443" }, >>> + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_J) - 1, "NTSC-J" }, >>> + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_M) - 1, "NTSC-M" }, >>> + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_60) - 1, "PAL-60" }, >>> + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_B) - 1, "PAL-B" }, >>> + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_D) - 1, "PAL-D" }, >>> + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_G) - 1, "PAL-G" }, >>> + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_H) - 1, "PAL-H" }, >>> + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_I) - 1, "PAL-I" }, >>> + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_M) - 1, "PAL-M" }, >>> + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_N) - 1, "PAL-N" }, >>> + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_NC) - 1, "PAL-Nc" }, >>> + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_60) - 1, "SECAM-60" }, >>> + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_B) - 1, "SECAM-B" }, >>> + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_D) - 1, "SECAM-D" }, >>> + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_G) - 1, "SECAM-G" }, >>> + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_K) - 1, "SECAM-K" }, >>> + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_K1) - 1, "SECAM-K1" }, >>> + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_L) - 1, "SECAM-L" }, >>> + { __builtin_ffs(DRM_MODE_TV_NORM_HD480I) - 1, "hd480i" }, >>> + { __builtin_ffs(DRM_MODE_TV_NORM_HD480P) - 1, "hd480p" }, >>> + { __builtin_ffs(DRM_MODE_TV_NORM_HD576I) - 1, "hd576i" }, >>> + { __builtin_ffs(DRM_MODE_TV_NORM_HD576P) - 1, "hd576p" }, >>> + { __builtin_ffs(DRM_MODE_TV_NORM_HD720P) - 1, "hd720p" }, >>> + { __builtin_ffs(DRM_MODE_TV_NORM_HD1080I) - 1, "hd1080i" }, >>> + }; >>> struct drm_property *tv_selector; >>> struct drm_property *tv_subconnector; >>> + struct drm_property *tv_norm; >>> unsigned int i; >>> >>> if (dev->mode_config.tv_select_subconnector_property) >>> @@ -1686,6 +1716,13 @@ int drm_mode_create_tv_properties(struct drm_device *dev, >>> if (drm_mode_create_tv_margin_properties(dev)) >>> goto nomem; >>> >>> + tv_norm = drm_property_create_bitmask(dev, 0, "tv norm", >>> + tv_norm_values, ARRAY_SIZE(tv_norm_values), >>> + supported_tv_norms); >> >> I expected this to be an enum, why a bitmask? Userspace can set multiple >> bits in a bitmask. > > I went for a bitmask since it allowed to report the capabilities of a > driver, but I just realised that you can do that with an enum too, like > we do for color encodings. > > I'll switch for an enum, thanks! > Maxime
On Tue, Aug 16, 2022 at 11:42:20AM +0200, Noralf Trønnes wrote: > > > Den 16.08.2022 10.26, skrev Maxime Ripard: > > Hi, > > > > On Mon, Aug 08, 2022 at 02:44:56PM +0200, Noralf Trønnes wrote: > >> Den 29.07.2022 18.34, 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@cerno.tech> > >>> > >> > >> Please also update Documentation/gpu/kms-properties.csv > >> > >> Requirements for adding a new property is found in > >> Documentation/gpu/drm-kms.rst > > > > I knew this was going to be raised at some point, so I'm glad it's that > > early :) > > > > I really don't know what to do there. If we stick by our usual rules, > > then we can't have any of that work merged. > > > > However, I think the status quo is not really satisfactory either. > > Indeed, we have a property, that doesn't follow those requirements > > either, with a driver-specific content, and that stands in the way of > > fixes and further improvements at both the core framework and driver > > levels. > > > > So having that new property still seems like a net improvement at the > > driver, framework and uAPI levels, even if it's not entirely following > > the requirements we have in place. > > > > Even more so since, realistically, those kind of interfaces will never > > get any new development on the user-space side of things, it's > > considered by everyone as legacy. > > > > This also is something we need to support at some point if we want to > > completely deprecate the fbdev drivers and provide decent alternatives > > in KMS. > > > > So yeah, strictly speaking, we would not qualify for our requirements > > there. I still think we have a strong case for an exception though. > > Which requirements would that be? The only one I can see is the > documentation and maybe an IGT test. This is the one I had in mind https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements I overlooked yours obviously, so I'll update my patches to fix it. Maxime
Hi Geert, On Fri, Aug 12, 2022 at 03:25:18PM +0200, Geert Uytterhoeven wrote: > > --- a/drivers/gpu/drm/drm_connector.c > > +++ b/drivers/gpu/drm/drm_connector.c > > @@ -1649,11 +1650,40 @@ EXPORT_SYMBOL(drm_mode_create_tv_margin_properties); > > * 0 on success or a negative error code on failure. > > */ > > int drm_mode_create_tv_properties(struct drm_device *dev, > > + unsigned int supported_tv_norms, > > unsigned int num_modes, > > const char * const modes[]) > > { > > + static const struct drm_prop_enum_list tv_norm_values[] = { > > + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_443) - 1, "NTSC-443" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_J) - 1, "NTSC-J" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_M) - 1, "NTSC-M" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_60) - 1, "PAL-60" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_B) - 1, "PAL-B" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_D) - 1, "PAL-D" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_G) - 1, "PAL-G" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_H) - 1, "PAL-H" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_I) - 1, "PAL-I" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_M) - 1, "PAL-M" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_N) - 1, "PAL-N" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_NC) - 1, "PAL-Nc" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_60) - 1, "SECAM-60" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_B) - 1, "SECAM-B" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_D) - 1, "SECAM-D" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_G) - 1, "SECAM-G" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_K) - 1, "SECAM-K" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_K1) - 1, "SECAM-K1" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_L) - 1, "SECAM-L" }, > > The above are analog standards, with a variable horizontal resolution. > > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD480I) - 1, "hd480i" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD480P) - 1, "hd480p" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD576I) - 1, "hd576i" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD576P) - 1, "hd576p" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD720P) - 1, "hd720p" }, > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD1080I) - 1, "hd1080i" }, > > The above are digital standards, with a fixed resolution. Are they? It's not clear to me from looking at nouveau, but I was under the impression that they were modes for a component output, so CEA 770.3. I don't have the spec though, so I can't check. > You seem to have missed "hd1080p"? Nobody is using it. If we ever have a driver that uses it I think we can add it. > In addition, "hd720p", "hd080i", and "hd1080p" are available in both 50 > and 60 (actually 59.94) Hz, while "hd1080p" can also use 24 or 25 Hz. It looks like nouveau only exposes modes for 480p at 59.94Hz, 576p at 50Hz, 720p at 60Hz, 1080i at 30Hz. > Either you have to add them here (e.g. "hd720p50" and "hd720p60"), or > handle them through "@<refresh>". The latter would impact "[PATCH v1 > 09/35] drm/modes: Move named modes parsing to a separate function", as > currently a named mode and a refresh rate can't be specified both. I think the former would make more sense. It simplifies a bit the parser, and we're going to use a named mode anyway. > As "[PATCH v1 34/35] drm/modes: Introduce the tv_mode property as a > command-line option" uses a separate "tv_mode" option, and not the main > mode name, I think you want to add them here. It's a separate story I think, we could have a named mode hd720p50, which would be equivalent to 1280x720,tv_mode=hd720p Maxime
Hi Maxime, On Tue, Aug 16, 2022 at 3:20 PM Maxime Ripard <maxime@cerno.tech> wrote: > On Fri, Aug 12, 2022 at 03:25:18PM +0200, Geert Uytterhoeven wrote: > > > --- a/drivers/gpu/drm/drm_connector.c > > > +++ b/drivers/gpu/drm/drm_connector.c > > > @@ -1649,11 +1650,40 @@ EXPORT_SYMBOL(drm_mode_create_tv_margin_properties); > > > * 0 on success or a negative error code on failure. > > > */ > > > int drm_mode_create_tv_properties(struct drm_device *dev, > > > + unsigned int supported_tv_norms, > > > unsigned int num_modes, > > > const char * const modes[]) > > > { > > > + static const struct drm_prop_enum_list tv_norm_values[] = { > > > + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_443) - 1, "NTSC-443" }, > > > + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_J) - 1, "NTSC-J" }, > > > + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_M) - 1, "NTSC-M" }, > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_60) - 1, "PAL-60" }, > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_B) - 1, "PAL-B" }, > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_D) - 1, "PAL-D" }, > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_G) - 1, "PAL-G" }, > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_H) - 1, "PAL-H" }, > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_I) - 1, "PAL-I" }, > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_M) - 1, "PAL-M" }, > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_N) - 1, "PAL-N" }, > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_NC) - 1, "PAL-Nc" }, > > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_60) - 1, "SECAM-60" }, > > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_B) - 1, "SECAM-B" }, > > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_D) - 1, "SECAM-D" }, > > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_G) - 1, "SECAM-G" }, > > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_K) - 1, "SECAM-K" }, > > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_K1) - 1, "SECAM-K1" }, > > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_L) - 1, "SECAM-L" }, > > > > The above are analog standards, with a variable horizontal resolution. > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD480I) - 1, "hd480i" }, > > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD480P) - 1, "hd480p" }, > > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD576I) - 1, "hd576i" }, > > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD576P) - 1, "hd576p" }, > > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD720P) - 1, "hd720p" }, > > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD1080I) - 1, "hd1080i" }, > > > > The above are digital standards, with a fixed resolution. > > Are they? > > It's not clear to me from looking at nouveau, but I was under the > impression that they were modes for a component output, so CEA 770.3. I > don't have the spec though, so I can't check. Oh right, I forgot about analog HD over component, where you can use other pixel clocks than in the digital standard. > > You seem to have missed "hd1080p"? > > Nobody is using it. If we ever have a driver that uses it I think we can > add it. The PS3 supports 1080p over component https://manuals.playstation.net/document/en/ps3/current/settings/videooutput.html > > In addition, "hd720p", "hd080i", and "hd1080p" are available in both 50 > > and 60 (actually 59.94) Hz, while "hd1080p" can also use 24 or 25 Hz. > > It looks like nouveau only exposes modes for 480p at 59.94Hz, 576p at > 50Hz, 720p at 60Hz, 1080i at 30Hz. PS3 supports both 50 and 60 Hz (same link above). > > Either you have to add them here (e.g. "hd720p50" and "hd720p60"), or > > handle them through "@<refresh>". The latter would impact "[PATCH v1 > > 09/35] drm/modes: Move named modes parsing to a separate function", as > > currently a named mode and a refresh rate can't be specified both. > > I think the former would make more sense. It simplifies a bit the > parser, and we're going to use a named mode anyway. > > > As "[PATCH v1 34/35] drm/modes: Introduce the tv_mode property as a > > command-line option" uses a separate "tv_mode" option, and not the main > > mode name, I think you want to add them here. > > It's a separate story I think, we could have a named mode hd720p50, > which would be equivalent to 1280x720,tv_mode=hd720p So where's the field rate in "1280x720,tv_mode=hd720p"? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
On Tue, Aug 16, 2022 at 03:29:07PM +0200, Geert Uytterhoeven wrote: > Hi Maxime, > > On Tue, Aug 16, 2022 at 3:20 PM Maxime Ripard <maxime@cerno.tech> wrote: > > On Fri, Aug 12, 2022 at 03:25:18PM +0200, Geert Uytterhoeven wrote: > > > > --- a/drivers/gpu/drm/drm_connector.c > > > > +++ b/drivers/gpu/drm/drm_connector.c > > > > @@ -1649,11 +1650,40 @@ EXPORT_SYMBOL(drm_mode_create_tv_margin_properties); > > > > * 0 on success or a negative error code on failure. > > > > */ > > > > int drm_mode_create_tv_properties(struct drm_device *dev, > > > > + unsigned int supported_tv_norms, > > > > unsigned int num_modes, > > > > const char * const modes[]) > > > > { > > > > + static const struct drm_prop_enum_list tv_norm_values[] = { > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_443) - 1, "NTSC-443" }, > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_J) - 1, "NTSC-J" }, > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_M) - 1, "NTSC-M" }, > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_60) - 1, "PAL-60" }, > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_B) - 1, "PAL-B" }, > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_D) - 1, "PAL-D" }, > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_G) - 1, "PAL-G" }, > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_H) - 1, "PAL-H" }, > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_I) - 1, "PAL-I" }, > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_M) - 1, "PAL-M" }, > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_N) - 1, "PAL-N" }, > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_NC) - 1, "PAL-Nc" }, > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_60) - 1, "SECAM-60" }, > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_B) - 1, "SECAM-B" }, > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_D) - 1, "SECAM-D" }, > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_G) - 1, "SECAM-G" }, > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_K) - 1, "SECAM-K" }, > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_K1) - 1, "SECAM-K1" }, > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_L) - 1, "SECAM-L" }, > > > > > > The above are analog standards, with a variable horizontal resolution. > > > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD480I) - 1, "hd480i" }, > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD480P) - 1, "hd480p" }, > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD576I) - 1, "hd576i" }, > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD576P) - 1, "hd576p" }, > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD720P) - 1, "hd720p" }, > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD1080I) - 1, "hd1080i" }, > > > > > > The above are digital standards, with a fixed resolution. > > > > Are they? > > > > It's not clear to me from looking at nouveau, but I was under the > > impression that they were modes for a component output, so CEA 770.3. I > > don't have the spec though, so I can't check. > > Oh right, I forgot about analog HD over component, where you can use > other pixel clocks than in the digital standard. > > > > You seem to have missed "hd1080p"? > > > > Nobody is using it. If we ever have a driver that uses it I think we can > > add it. > > The PS3 supports 1080p over component > https://manuals.playstation.net/document/en/ps3/current/settings/videooutput.html Yeah, and iirc the Xbox360 did too, but what I meant by nobody is using it is that there's no driver using it currently. > > > In addition, "hd720p", "hd080i", and "hd1080p" are available in both 50 > > > and 60 (actually 59.94) Hz, while "hd1080p" can also use 24 or 25 Hz. > > > > It looks like nouveau only exposes modes for 480p at 59.94Hz, 576p at > > 50Hz, 720p at 60Hz, 1080i at 30Hz. > > PS3 supports both 50 and 60 Hz (same link above). I'm probably wary on this, but I'd rather stay at feature parity for this series. There's already plenty of occasion to screw up something that I'd rather not introduce new stuff I haven't been able to test :) Provided we can easily extend it to support these additional features of course :) > > > Either you have to add them here (e.g. "hd720p50" and "hd720p60"), or > > > handle them through "@<refresh>". The latter would impact "[PATCH v1 > > > 09/35] drm/modes: Move named modes parsing to a separate function", as > > > currently a named mode and a refresh rate can't be specified both. > > > > I think the former would make more sense. It simplifies a bit the > > parser, and we're going to use a named mode anyway. > > > > > As "[PATCH v1 34/35] drm/modes: Introduce the tv_mode property as a > > > command-line option" uses a separate "tv_mode" option, and not the main > > > mode name, I think you want to add them here. > > > > It's a separate story I think, we could have a named mode hd720p50, > > which would be equivalent to 1280x720,tv_mode=hd720p > > So where's the field rate in "1280x720,tv_mode=hd720p"? Yeah, sorry I meant 1280x720@50,tv_mode=hd720p Maxime
Hi Maxime, On Tue, Aug 16, 2022 at 4:11 PM Maxime Ripard <maxime@cerno.tech> wrote: > On Tue, Aug 16, 2022 at 03:29:07PM +0200, Geert Uytterhoeven wrote: > > On Tue, Aug 16, 2022 at 3:20 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > On Fri, Aug 12, 2022 at 03:25:18PM +0200, Geert Uytterhoeven wrote: > > > > > --- a/drivers/gpu/drm/drm_connector.c > > > > > +++ b/drivers/gpu/drm/drm_connector.c > > > > > @@ -1649,11 +1650,40 @@ EXPORT_SYMBOL(drm_mode_create_tv_margin_properties); > > > > > * 0 on success or a negative error code on failure. > > > > > */ > > > > > int drm_mode_create_tv_properties(struct drm_device *dev, > > > > > + unsigned int supported_tv_norms, > > > > > unsigned int num_modes, > > > > > const char * const modes[]) > > > > > { > > > > > + static const struct drm_prop_enum_list tv_norm_values[] = { > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_443) - 1, "NTSC-443" }, > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_J) - 1, "NTSC-J" }, > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_M) - 1, "NTSC-M" }, > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_60) - 1, "PAL-60" }, > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_B) - 1, "PAL-B" }, > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_D) - 1, "PAL-D" }, > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_G) - 1, "PAL-G" }, > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_H) - 1, "PAL-H" }, > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_I) - 1, "PAL-I" }, > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_M) - 1, "PAL-M" }, > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_N) - 1, "PAL-N" }, > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_NC) - 1, "PAL-Nc" }, > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_60) - 1, "SECAM-60" }, > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_B) - 1, "SECAM-B" }, > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_D) - 1, "SECAM-D" }, > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_G) - 1, "SECAM-G" }, > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_K) - 1, "SECAM-K" }, > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_K1) - 1, "SECAM-K1" }, > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_L) - 1, "SECAM-L" }, > > > > > > > > The above are analog standards, with a variable horizontal resolution. > > > > > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD480I) - 1, "hd480i" }, > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD480P) - 1, "hd480p" }, > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD576I) - 1, "hd576i" }, > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD576P) - 1, "hd576p" }, > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD720P) - 1, "hd720p" }, > > > > > + { __builtin_ffs(DRM_MODE_TV_NORM_HD1080I) - 1, "hd1080i" }, > > > > > > > > The above are digital standards, with a fixed resolution. > > > > > > Are they? > > > > > > It's not clear to me from looking at nouveau, but I was under the > > > impression that they were modes for a component output, so CEA 770.3. I > > > don't have the spec though, so I can't check. > > > > Oh right, I forgot about analog HD over component, where you can use > > other pixel clocks than in the digital standard. > > > > > > You seem to have missed "hd1080p"? > > > > > > Nobody is using it. If we ever have a driver that uses it I think we can > > > add it. > > > > The PS3 supports 1080p over component > > https://manuals.playstation.net/document/en/ps3/current/settings/videooutput.html > > Yeah, and iirc the Xbox360 did too, but what I meant by nobody is using > it is that there's no driver using it currently. OK, it can be added later. > > > > In addition, "hd720p", "hd080i", and "hd1080p" are available in both 50 > > > > and 60 (actually 59.94) Hz, while "hd1080p" can also use 24 or 25 Hz. > > > > > > It looks like nouveau only exposes modes for 480p at 59.94Hz, 576p at > > > 50Hz, 720p at 60Hz, 1080i at 30Hz. > > > > PS3 supports both 50 and 60 Hz (same link above). > > I'm probably wary on this, but I'd rather stay at feature parity for > this series. There's already plenty of occasion to screw up something > that I'd rather not introduce new stuff I haven't been able to test :) > > Provided we can easily extend it to support these additional features of > course :) > > > > > Either you have to add them here (e.g. "hd720p50" and "hd720p60"), or > > > > handle them through "@<refresh>". The latter would impact "[PATCH v1 > > > > 09/35] drm/modes: Move named modes parsing to a separate function", as > > > > currently a named mode and a refresh rate can't be specified both. > > > > > > I think the former would make more sense. It simplifies a bit the > > > parser, and we're going to use a named mode anyway. > > > > > > > As "[PATCH v1 34/35] drm/modes: Introduce the tv_mode property as a > > > > command-line option" uses a separate "tv_mode" option, and not the main > > > > mode name, I think you want to add them here. > > > > > > It's a separate story I think, we could have a named mode hd720p50, > > > which would be equivalent to 1280x720,tv_mode=hd720p > > > > So where's the field rate in "1280x720,tv_mode=hd720p"? > > Yeah, sorry I meant 1280x720@50,tv_mode=hd720p Above you said "I think the former would make more sense", so that should be "1280x720,tv_mode=hd720p50"? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
On Tue, Aug 16, 2022 at 04:43:44PM +0200, Geert Uytterhoeven wrote: > > > > > Either you have to add them here (e.g. "hd720p50" and "hd720p60"), or > > > > > handle them through "@<refresh>". The latter would impact "[PATCH v1 > > > > > 09/35] drm/modes: Move named modes parsing to a separate function", as > > > > > currently a named mode and a refresh rate can't be specified both. > > > > > > > > I think the former would make more sense. It simplifies a bit the > > > > parser, and we're going to use a named mode anyway. > > > > > > > > > As "[PATCH v1 34/35] drm/modes: Introduce the tv_mode property as a > > > > > command-line option" uses a separate "tv_mode" option, and not the main > > > > > mode name, I think you want to add them here. > > > > > > > > It's a separate story I think, we could have a named mode hd720p50, > > > > which would be equivalent to 1280x720,tv_mode=hd720p > > > > > > So where's the field rate in "1280x720,tv_mode=hd720p"? > > > > Yeah, sorry I meant 1280x720@50,tv_mode=hd720p > > Above you said "I think the former would make more sense", so that > should be "1280x720,tv_mode=hd720p50"? No, 720p at 50Hz would be either hd720p50 or 1280x720@50,tv_mode=hd720p and 60Hz would be hd720p60 or 1280x720@60,tv_mode=hd720p Maxime
Den 16.08.2022 11.49, skrev Maxime Ripard: > On Tue, Aug 16, 2022 at 11:42:20AM +0200, Noralf Trønnes wrote: >> >> >> Den 16.08.2022 10.26, skrev Maxime Ripard: >>> Hi, >>> >>> On Mon, Aug 08, 2022 at 02:44:56PM +0200, Noralf Trønnes wrote: >>>> Den 29.07.2022 18.34, 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@cerno.tech> >>>>> >>>> >>>> Please also update Documentation/gpu/kms-properties.csv >>>> >>>> Requirements for adding a new property is found in >>>> Documentation/gpu/drm-kms.rst >>> >>> I knew this was going to be raised at some point, so I'm glad it's that >>> early :) >>> >>> I really don't know what to do there. If we stick by our usual rules, >>> then we can't have any of that work merged. >>> >>> However, I think the status quo is not really satisfactory either. >>> Indeed, we have a property, that doesn't follow those requirements >>> either, with a driver-specific content, and that stands in the way of >>> fixes and further improvements at both the core framework and driver >>> levels. >>> >>> So having that new property still seems like a net improvement at the >>> driver, framework and uAPI levels, even if it's not entirely following >>> the requirements we have in place. >>> >>> Even more so since, realistically, those kind of interfaces will never >>> get any new development on the user-space side of things, it's >>> considered by everyone as legacy. >>> >>> This also is something we need to support at some point if we want to >>> completely deprecate the fbdev drivers and provide decent alternatives >>> in KMS. >>> >>> So yeah, strictly speaking, we would not qualify for our requirements >>> there. I still think we have a strong case for an exception though. >> >> Which requirements would that be? The only one I can see is the >> documentation and maybe an IGT test. > > This is the one I had in mind > https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements > Oh right, I had forgotten about that one. One benefit of having a userspace implementation is that it increases the chance of widespread adoption having a working implementation to look at. I don't think the reason tv.mode is not used anywhere (that I know of) is because the driver picks the enum values resulting in no standard names. It's a niche thing and way down on the todo list. nouveau and ch7006 has a tv_norm module parameter which certainly doesn't help in moving people/projects over to the DRM property (downstream rpi also has it now). mpv[1] is a commandline media player that after a quick look might be a candidate for implementing the property without too much effort. How do you test the property? I've used modetest but I can only change to a tv.mode that matches the current display mode. I can't switch from ntsc to pal for instance. I have tried changing mode on rpi-5.15 (which I will switch to for the gud gadget), but I always end up with flip timeouts when changing the value: $ cat /proc/cpuinfo | grep Model Model : Raspberry Pi 4 Model B Rev 1.1 $ uname -a Linux pi4t 5.15.56-v7l+ #1575 SMP Fri Jul 22 20:29:46 BST 2022 armv7l GNU/Linux $ sudo dmesg -C $ modetest -M vc4 -s 45:720x480i -w 45:mode:1 setting mode 720x480i-29.97Hz on connectors 45, crtc 73 failed to set gamma: Function not implemented $ dmesg $ modetest -M vc4 -s 45:720x480i -w 45:mode:0 setting mode 720x480i-29.97Hz on connectors 45, crtc 73 failed to set gamma: Function not implemented $ dmesg [ 95.193059] [drm:drm_atomic_helper_wait_for_flip_done [drm_kms_helper]] *ERROR* [CRTC:73:crtc-1] flip_done timed out [ 105.433112] [drm:drm_crtc_commit_wait [drm]] *ERROR* flip_done timed out [ 105.433519] [drm:drm_atomic_helper_wait_for_dependencies [drm_kms_helper]] *ERROR* [CRTC:73:crtc-1] commit wait timed out [ 115.673095] [drm:drm_crtc_commit_wait [drm]] *ERROR* flip_done timed out [ 115.673498] [drm:drm_atomic_helper_wait_for_dependencies [drm_kms_helper]] *ERROR* [PLANE:63:plane-1] commit wait timed out [ 125.913106] [drm:drm_crtc_commit_wait [drm]] *ERROR* flip_done timed out [ 125.913510] vc4-drm gpu: [drm] *ERROR* Timed out waiting for commit [ 136.153411] [drm:drm_atomic_helper_wait_for_flip_done [drm_kms_helper]] *ERROR* [CRTC:73:crtc-1] flip_done timed out I doesn't help to reload vc4, I have to reboot to get it working again. I get this when reloading: [ 776.799784] vc4_vec fec13000.vec: Unbalanced pm_runtime_enable! I know this was working in rpi-5.10 on the Pi Zero (Pi4 tvout using vc4 did not work at all when I tried). Noralf. [1] https://mpv.io/ https://github.com/mpv-player/mpv/blob/master/video/out/drm_common.c https://github.com/mpv-player/mpv/blob/master/video/out/drm_atomic.c
Hi Maxime, On Tue, Aug 16, 2022 at 5:50 PM Maxime Ripard <maxime@cerno.tech> wrote: > On Tue, Aug 16, 2022 at 04:43:44PM +0200, Geert Uytterhoeven wrote: > > > > > > Either you have to add them here (e.g. "hd720p50" and "hd720p60"), or > > > > > > handle them through "@<refresh>". The latter would impact "[PATCH v1 > > > > > > 09/35] drm/modes: Move named modes parsing to a separate function", as > > > > > > currently a named mode and a refresh rate can't be specified both. > > > > > > > > > > I think the former would make more sense. It simplifies a bit the > > > > > parser, and we're going to use a named mode anyway. > > > > > > > > > > > As "[PATCH v1 34/35] drm/modes: Introduce the tv_mode property as a > > > > > > command-line option" uses a separate "tv_mode" option, and not the main > > > > > > mode name, I think you want to add them here. > > > > > > > > > > It's a separate story I think, we could have a named mode hd720p50, > > > > > which would be equivalent to 1280x720,tv_mode=hd720p > > > > > > > > So where's the field rate in "1280x720,tv_mode=hd720p"? > > > > > > Yeah, sorry I meant 1280x720@50,tv_mode=hd720p > > > > Above you said "I think the former would make more sense", so that > > should be "1280x720,tv_mode=hd720p50"? > > No, 720p at 50Hz would be either hd720p50 or 1280x720@50,tv_mode=hd720p > and 60Hz would be hd720p60 or 1280x720@60,tv_mode=hd720p I disagree: hd720p50 and hd720p60 are different TV modes. Treating them the same would be similar to treating unmodulated (e.g. component) PAL-N (25 frames/s) and PAL-M (30 frames/s) the same. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
On Wed, Aug 17, 2022 at 9:31 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Tue, Aug 16, 2022 at 5:50 PM Maxime Ripard <maxime@cerno.tech> wrote: > > On Tue, Aug 16, 2022 at 04:43:44PM +0200, Geert Uytterhoeven wrote: > > > > > > > Either you have to add them here (e.g. "hd720p50" and "hd720p60"), or > > > > > > > handle them through "@<refresh>". The latter would impact "[PATCH v1 > > > > > > > 09/35] drm/modes: Move named modes parsing to a separate function", as > > > > > > > currently a named mode and a refresh rate can't be specified both. > > > > > > > > > > > > I think the former would make more sense. It simplifies a bit the > > > > > > parser, and we're going to use a named mode anyway. > > > > > > > > > > > > > As "[PATCH v1 34/35] drm/modes: Introduce the tv_mode property as a > > > > > > > command-line option" uses a separate "tv_mode" option, and not the main > > > > > > > mode name, I think you want to add them here. > > > > > > > > > > > > It's a separate story I think, we could have a named mode hd720p50, > > > > > > which would be equivalent to 1280x720,tv_mode=hd720p > > > > > > > > > > So where's the field rate in "1280x720,tv_mode=hd720p"? > > > > > > > > Yeah, sorry I meant 1280x720@50,tv_mode=hd720p > > > > > > Above you said "I think the former would make more sense", so that > > > should be "1280x720,tv_mode=hd720p50"? > > > > No, 720p at 50Hz would be either hd720p50 or 1280x720@50,tv_mode=hd720p > > and 60Hz would be hd720p60 or 1280x720@60,tv_mode=hd720p > > I disagree: hd720p50 and hd720p60 are different TV modes. > Treating them the same would be similar to treating unmodulated (e.g. > component) PAL-N (25 frames/s) and PAL-M (30 frames/s) the same. IIRC from my PS3 -Linux days, not all HD(-Ready) TVs supported both hd720p50 and hd720p60. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
On Wed, Aug 17, 2022 at 09:31:18AM +0200, Geert Uytterhoeven wrote: > Hi Maxime, > > On Tue, Aug 16, 2022 at 5:50 PM Maxime Ripard <maxime@cerno.tech> wrote: > > On Tue, Aug 16, 2022 at 04:43:44PM +0200, Geert Uytterhoeven wrote: > > > > > > > Either you have to add them here (e.g. "hd720p50" and "hd720p60"), or > > > > > > > handle them through "@<refresh>". The latter would impact "[PATCH v1 > > > > > > > 09/35] drm/modes: Move named modes parsing to a separate function", as > > > > > > > currently a named mode and a refresh rate can't be specified both. > > > > > > > > > > > > I think the former would make more sense. It simplifies a bit the > > > > > > parser, and we're going to use a named mode anyway. > > > > > > > > > > > > > As "[PATCH v1 34/35] drm/modes: Introduce the tv_mode property as a > > > > > > > command-line option" uses a separate "tv_mode" option, and not the main > > > > > > > mode name, I think you want to add them here. > > > > > > > > > > > > It's a separate story I think, we could have a named mode hd720p50, > > > > > > which would be equivalent to 1280x720,tv_mode=hd720p > > > > > > > > > > So where's the field rate in "1280x720,tv_mode=hd720p"? > > > > > > > > Yeah, sorry I meant 1280x720@50,tv_mode=hd720p > > > > > > Above you said "I think the former would make more sense", so that > > > should be "1280x720,tv_mode=hd720p50"? > > > > No, 720p at 50Hz would be either hd720p50 or 1280x720@50,tv_mode=hd720p > > and 60Hz would be hd720p60 or 1280x720@60,tv_mode=hd720p > > I disagree: hd720p50 and hd720p60 are different TV modes. I agree, and I don't see how that command-line doesn't express that? Maxime
Hi Maxime, On Wed, Aug 17, 2022 at 9:47 AM Maxime Ripard <maxime@cerno.tech> wrote: > On Wed, Aug 17, 2022 at 09:31:18AM +0200, Geert Uytterhoeven wrote: > > On Tue, Aug 16, 2022 at 5:50 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > On Tue, Aug 16, 2022 at 04:43:44PM +0200, Geert Uytterhoeven wrote: > > > > > > > > Either you have to add them here (e.g. "hd720p50" and "hd720p60"), or > > > > > > > > handle them through "@<refresh>". The latter would impact "[PATCH v1 > > > > > > > > 09/35] drm/modes: Move named modes parsing to a separate function", as > > > > > > > > currently a named mode and a refresh rate can't be specified both. > > > > > > > > > > > > > > I think the former would make more sense. It simplifies a bit the > > > > > > > parser, and we're going to use a named mode anyway. > > > > > > > > > > > > > > > As "[PATCH v1 34/35] drm/modes: Introduce the tv_mode property as a > > > > > > > > command-line option" uses a separate "tv_mode" option, and not the main > > > > > > > > mode name, I think you want to add them here. > > > > > > > > > > > > > > It's a separate story I think, we could have a named mode hd720p50, > > > > > > > which would be equivalent to 1280x720,tv_mode=hd720p > > > > > > > > > > > > So where's the field rate in "1280x720,tv_mode=hd720p"? > > > > > > > > > > Yeah, sorry I meant 1280x720@50,tv_mode=hd720p > > > > > > > > Above you said "I think the former would make more sense", so that > > > > should be "1280x720,tv_mode=hd720p50"? > > > > > > No, 720p at 50Hz would be either hd720p50 or 1280x720@50,tv_mode=hd720p > > > and 60Hz would be hd720p60 or 1280x720@60,tv_mode=hd720p > > > > I disagree: hd720p50 and hd720p60 are different TV modes. > > I agree, and I don't see how that command-line doesn't express that? Oh, I see what you mean: yes, it expresses that. But it is inconsistent with the NTSC/PAL/SECAM/hd{480,576}[ip] modes, where the TV mode specifies both number of lines and frame rate. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
On Wed, Aug 17, 2022 at 10:35:07AM +0200, Geert Uytterhoeven wrote: > Hi Maxime, > > On Wed, Aug 17, 2022 at 9:47 AM Maxime Ripard <maxime@cerno.tech> wrote: > > On Wed, Aug 17, 2022 at 09:31:18AM +0200, Geert Uytterhoeven wrote: > > > On Tue, Aug 16, 2022 at 5:50 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > On Tue, Aug 16, 2022 at 04:43:44PM +0200, Geert Uytterhoeven wrote: > > > > > > > > > Either you have to add them here (e.g. "hd720p50" and "hd720p60"), or > > > > > > > > > handle them through "@<refresh>". The latter would impact "[PATCH v1 > > > > > > > > > 09/35] drm/modes: Move named modes parsing to a separate function", as > > > > > > > > > currently a named mode and a refresh rate can't be specified both. > > > > > > > > > > > > > > > > I think the former would make more sense. It simplifies a bit the > > > > > > > > parser, and we're going to use a named mode anyway. > > > > > > > > > > > > > > > > > As "[PATCH v1 34/35] drm/modes: Introduce the tv_mode property as a > > > > > > > > > command-line option" uses a separate "tv_mode" option, and not the main > > > > > > > > > mode name, I think you want to add them here. > > > > > > > > > > > > > > > > It's a separate story I think, we could have a named mode hd720p50, > > > > > > > > which would be equivalent to 1280x720,tv_mode=hd720p > > > > > > > > > > > > > > So where's the field rate in "1280x720,tv_mode=hd720p"? > > > > > > > > > > > > Yeah, sorry I meant 1280x720@50,tv_mode=hd720p > > > > > > > > > > Above you said "I think the former would make more sense", so that > > > > > should be "1280x720,tv_mode=hd720p50"? > > > > > > > > No, 720p at 50Hz would be either hd720p50 or 1280x720@50,tv_mode=hd720p > > > > and 60Hz would be hd720p60 or 1280x720@60,tv_mode=hd720p > > > > > > I disagree: hd720p50 and hd720p60 are different TV modes. > > > > I agree, and I don't see how that command-line doesn't express that? > > Oh, I see what you mean: yes, it expresses that. > But it is inconsistent with the NTSC/PAL/SECAM/hd{480,576}[ip] modes, > where the TV mode specifies both number of lines and frame rate. Only if we're using a named mode, and naming is hard :) Honestly, I'd be inclined to drop the hd* for now from this series. I don't have a hardware to test it with, for some we don't even have drivers that could implement these modes, we don't have a spec to work from, it looks like a recipe for failure :) Maxime
On Tue, Aug 16, 2022 at 09:35:24PM +0200, Noralf Trønnes wrote: > Den 16.08.2022 11.49, skrev Maxime Ripard: > > On Tue, Aug 16, 2022 at 11:42:20AM +0200, Noralf Trønnes wrote: > >> Den 16.08.2022 10.26, skrev Maxime Ripard: > >>> Hi, > >>> > >>> On Mon, Aug 08, 2022 at 02:44:56PM +0200, Noralf Trønnes wrote: > >>>> Den 29.07.2022 18.34, 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@cerno.tech> > >>>>> > >>>> > >>>> Please also update Documentation/gpu/kms-properties.csv > >>>> > >>>> Requirements for adding a new property is found in > >>>> Documentation/gpu/drm-kms.rst > >>> > >>> I knew this was going to be raised at some point, so I'm glad it's that > >>> early :) > >>> > >>> I really don't know what to do there. If we stick by our usual rules, > >>> then we can't have any of that work merged. > >>> > >>> However, I think the status quo is not really satisfactory either. > >>> Indeed, we have a property, that doesn't follow those requirements > >>> either, with a driver-specific content, and that stands in the way of > >>> fixes and further improvements at both the core framework and driver > >>> levels. > >>> > >>> So having that new property still seems like a net improvement at the > >>> driver, framework and uAPI levels, even if it's not entirely following > >>> the requirements we have in place. > >>> > >>> Even more so since, realistically, those kind of interfaces will never > >>> get any new development on the user-space side of things, it's > >>> considered by everyone as legacy. > >>> > >>> This also is something we need to support at some point if we want to > >>> completely deprecate the fbdev drivers and provide decent alternatives > >>> in KMS. > >>> > >>> So yeah, strictly speaking, we would not qualify for our requirements > >>> there. I still think we have a strong case for an exception though. > >> > >> Which requirements would that be? The only one I can see is the > >> documentation and maybe an IGT test. > > > > This is the one I had in mind > > https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements > > > > Oh right, I had forgotten about that one. > > One benefit of having a userspace implementation is that it increases > the chance of widespread adoption having a working implementation to > look at. I don't think the reason tv.mode is not used anywhere (that I > know of) is because the driver picks the enum values resulting in no > standard names. It probably doesn't help, but it's not what I was implying. > It's a niche thing and way down on the todo list. nouveau and ch7006 > has a tv_norm module parameter which certainly doesn't help in moving > people/projects over to the DRM property (downstream rpi also has it > now). Yeah, the RPi version is part of the reason I started working on this. We should also consider the named modes used by vc4 and sun4i. All these ad-hoc solutions are limited and (I think) come from the fact that we don't have a solution easy enough to use for drivers (and to discover). nouveau, ch7006, i915 and vc4 are using the tv.mode property for example, but sun4i and meson don't. sun4i relies on named modes to reimplement TV modes, but meson doesn't at all. And then nouveau has that extra command line parameter to set it up at boot time. It doesn't really make much sense to me, when all drivers have very similar needs, that none of them behave in the same way. And I think the non-standard property is partly to blame for this, since without some generic content we can't share code. This is what this series is about: every driver having similar capabilities and as trivially as possible. > mpv[1] is a commandline media player that after a quick look might be a > candidate for implementing the property without too much effort. Kodi might be another one. I can try to hack something around, but I'm really skeptical about whether a PR would be merged or not. > How do you test the property? I've used modetest but I can only change > to a tv.mode that matches the current display mode. I can't switch from > ntsc to pal for instance. Yep, if you want to change from PAL to NTSC, it will require a new mode. > I have tried changing mode on rpi-5.15 (which I will switch to for the > gud gadget), but I always end up with flip timeouts when changing the value: > > $ cat /proc/cpuinfo | grep Model > Model : Raspberry Pi 4 Model B Rev 1.1 > $ uname -a > Linux pi4t 5.15.56-v7l+ #1575 SMP Fri Jul 22 20:29:46 BST 2022 armv7l > GNU/Linux > $ sudo dmesg -C > $ modetest -M vc4 -s 45:720x480i -w 45:mode:1 > setting mode 720x480i-29.97Hz on connectors 45, crtc 73 > failed to set gamma: Function not implemented > > $ dmesg > $ modetest -M vc4 -s 45:720x480i -w 45:mode:0 > setting mode 720x480i-29.97Hz on connectors 45, crtc 73 > failed to set gamma: Function not implemented > > $ dmesg > [ 95.193059] [drm:drm_atomic_helper_wait_for_flip_done > [drm_kms_helper]] *ERROR* [CRTC:73:crtc-1] flip_done timed out > [ 105.433112] [drm:drm_crtc_commit_wait [drm]] *ERROR* flip_done timed out > [ 105.433519] [drm:drm_atomic_helper_wait_for_dependencies > [drm_kms_helper]] *ERROR* [CRTC:73:crtc-1] commit wait timed out > [ 115.673095] [drm:drm_crtc_commit_wait [drm]] *ERROR* flip_done timed out > [ 115.673498] [drm:drm_atomic_helper_wait_for_dependencies > [drm_kms_helper]] *ERROR* [PLANE:63:plane-1] commit wait timed out > [ 125.913106] [drm:drm_crtc_commit_wait [drm]] *ERROR* flip_done timed out > [ 125.913510] vc4-drm gpu: [drm] *ERROR* Timed out waiting for commit > [ 136.153411] [drm:drm_atomic_helper_wait_for_flip_done > [drm_kms_helper]] *ERROR* [CRTC:73:crtc-1] flip_done timed out So I haven't tested with 5.15, but I tested it with this series and it was working well, both with the compat layer and the new property. Maxime
Hi Maxime, On Wed, Aug 17, 2022 at 1:15 PM Maxime Ripard <maxime@cerno.tech> wrote: > On Wed, Aug 17, 2022 at 10:35:07AM +0200, Geert Uytterhoeven wrote: > > On Wed, Aug 17, 2022 at 9:47 AM Maxime Ripard <maxime@cerno.tech> wrote: > > > On Wed, Aug 17, 2022 at 09:31:18AM +0200, Geert Uytterhoeven wrote: > > > > On Tue, Aug 16, 2022 at 5:50 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > On Tue, Aug 16, 2022 at 04:43:44PM +0200, Geert Uytterhoeven wrote: > > > > > > > > > > Either you have to add them here (e.g. "hd720p50" and "hd720p60"), or > > > > > > > > > > handle them through "@<refresh>". The latter would impact "[PATCH v1 > > > > > > > > > > 09/35] drm/modes: Move named modes parsing to a separate function", as > > > > > > > > > > currently a named mode and a refresh rate can't be specified both. > > > > > > > > > > > > > > > > > > I think the former would make more sense. It simplifies a bit the > > > > > > > > > parser, and we're going to use a named mode anyway. > > > > > > > > > > > > > > > > > > > As "[PATCH v1 34/35] drm/modes: Introduce the tv_mode property as a > > > > > > > > > > command-line option" uses a separate "tv_mode" option, and not the main > > > > > > > > > > mode name, I think you want to add them here. > > > > > > > > > > > > > > > > > > It's a separate story I think, we could have a named mode hd720p50, > > > > > > > > > which would be equivalent to 1280x720,tv_mode=hd720p > > > > > > > > > > > > > > > > So where's the field rate in "1280x720,tv_mode=hd720p"? > > > > > > > > > > > > > > Yeah, sorry I meant 1280x720@50,tv_mode=hd720p > > > > > > > > > > > > Above you said "I think the former would make more sense", so that > > > > > > should be "1280x720,tv_mode=hd720p50"? > > > > > > > > > > No, 720p at 50Hz would be either hd720p50 or 1280x720@50,tv_mode=hd720p > > > > > and 60Hz would be hd720p60 or 1280x720@60,tv_mode=hd720p > > > > > > > > I disagree: hd720p50 and hd720p60 are different TV modes. > > > > > > I agree, and I don't see how that command-line doesn't express that? > > > > Oh, I see what you mean: yes, it expresses that. > > But it is inconsistent with the NTSC/PAL/SECAM/hd{480,576}[ip] modes, > > where the TV mode specifies both number of lines and frame rate. > > Only if we're using a named mode, and naming is hard :) That's not true: "640x480,tv_mode=PAL-N" would give me a mode with 625 lines and 25 frames/s, "640x480,tv_mode=PAL-M" would give me a mode with 525 lines and 30 frames/s. > Honestly, I'd be inclined to drop the hd* for now from this series. I > don't have a hardware to test it with, for some we don't even have > drivers that could implement these modes, we don't have a spec to work > from, it looks like a recipe for failure :) OK. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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 17.08.2022 13.46, skrev Maxime Ripard: > On Tue, Aug 16, 2022 at 09:35:24PM +0200, Noralf Trønnes wrote: >> Den 16.08.2022 11.49, skrev Maxime Ripard: >>> On Tue, Aug 16, 2022 at 11:42:20AM +0200, Noralf Trønnes wrote: >>>> Den 16.08.2022 10.26, skrev Maxime Ripard: >>>>> Hi, >>>>> >>>>> On Mon, Aug 08, 2022 at 02:44:56PM +0200, Noralf Trønnes wrote: >>>>>> Den 29.07.2022 18.34, 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@cerno.tech> >>>>>>> >>>>>> >>>>>> Please also update Documentation/gpu/kms-properties.csv >>>>>> >>>>>> Requirements for adding a new property is found in >>>>>> Documentation/gpu/drm-kms.rst >>>>> >>>>> I knew this was going to be raised at some point, so I'm glad it's that >>>>> early :) >>>>> >>>>> I really don't know what to do there. If we stick by our usual rules, >>>>> then we can't have any of that work merged. >>>>> >>>>> However, I think the status quo is not really satisfactory either. >>>>> Indeed, we have a property, that doesn't follow those requirements >>>>> either, with a driver-specific content, and that stands in the way of >>>>> fixes and further improvements at both the core framework and driver >>>>> levels. >>>>> >>>>> So having that new property still seems like a net improvement at the >>>>> driver, framework and uAPI levels, even if it's not entirely following >>>>> the requirements we have in place. >>>>> >>>>> Even more so since, realistically, those kind of interfaces will never >>>>> get any new development on the user-space side of things, it's >>>>> considered by everyone as legacy. >>>>> >>>>> This also is something we need to support at some point if we want to >>>>> completely deprecate the fbdev drivers and provide decent alternatives >>>>> in KMS. >>>>> >>>>> So yeah, strictly speaking, we would not qualify for our requirements >>>>> there. I still think we have a strong case for an exception though. >>>> >>>> Which requirements would that be? The only one I can see is the >>>> documentation and maybe an IGT test. >>> >>> This is the one I had in mind >>> https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements >>> >> >> Oh right, I had forgotten about that one. >> >> One benefit of having a userspace implementation is that it increases >> the chance of widespread adoption having a working implementation to >> look at. I don't think the reason tv.mode is not used anywhere (that I >> know of) is because the driver picks the enum values resulting in no >> standard names. > > It probably doesn't help, but it's not what I was implying. > >> It's a niche thing and way down on the todo list. nouveau and ch7006 >> has a tv_norm module parameter which certainly doesn't help in moving >> people/projects over to the DRM property (downstream rpi also has it >> now). > > Yeah, the RPi version is part of the reason I started working on this. > We should also consider the named modes used by vc4 and sun4i. All these > ad-hoc solutions are limited and (I think) come from the fact that we > don't have a solution easy enough to use for drivers (and to discover). > > nouveau, ch7006, i915 and vc4 are using the tv.mode property for > example, but sun4i and meson don't. > > sun4i relies on named modes to reimplement TV modes, but meson doesn't > at all. > > And then nouveau has that extra command line parameter to set it up at > boot time. > > It doesn't really make much sense to me, when all drivers have very > similar needs, that none of them behave in the same way. And I think the > non-standard property is partly to blame for this, since without some > generic content we can't share code. > > This is what this series is about: every driver having similar > capabilities and as trivially as possible. > >> mpv[1] is a commandline media player that after a quick look might be a >> candidate for implementing the property without too much effort. > > Kodi might be another one. I can try to hack something around, but I'm > really skeptical about whether a PR would be merged or not. > You can ask first before wasting time ofc. But this baffles me, if you don't think projects like Kodi which is TV centered want this, what kind of projects do you think want to use this property? >> How do you test the property? I've used modetest but I can only change >> to a tv.mode that matches the current display mode. I can't switch from >> ntsc to pal for instance. > > Yep, if you want to change from PAL to NTSC, it will require a new mode. > So userspace has to check tv.mode first and then create a display mode the driver will accept if switching to a different display mode is necessary? In other words, userspace can't discover from the kernel which display modes a certain tv.mode/norm provides before it is selected? If so, maybe libdrm should have some function(s) to deal with switching between modes that require a different display mode since knowledge about which display modes a tv.mode supports is needed before hand. Noralf.
On Wed, Aug 17, 2022 at 03:05:52PM +0200, Geert Uytterhoeven wrote: > Hi Maxime, > > On Wed, Aug 17, 2022 at 1:15 PM Maxime Ripard <maxime@cerno.tech> wrote: > > On Wed, Aug 17, 2022 at 10:35:07AM +0200, Geert Uytterhoeven wrote: > > > On Wed, Aug 17, 2022 at 9:47 AM Maxime Ripard <maxime@cerno.tech> wrote: > > > > On Wed, Aug 17, 2022 at 09:31:18AM +0200, Geert Uytterhoeven wrote: > > > > > On Tue, Aug 16, 2022 at 5:50 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > On Tue, Aug 16, 2022 at 04:43:44PM +0200, Geert Uytterhoeven wrote: > > > > > > > > > > > Either you have to add them here (e.g. "hd720p50" and "hd720p60"), or > > > > > > > > > > > handle them through "@<refresh>". The latter would impact "[PATCH v1 > > > > > > > > > > > 09/35] drm/modes: Move named modes parsing to a separate function", as > > > > > > > > > > > currently a named mode and a refresh rate can't be specified both. > > > > > > > > > > > > > > > > > > > > I think the former would make more sense. It simplifies a bit the > > > > > > > > > > parser, and we're going to use a named mode anyway. > > > > > > > > > > > > > > > > > > > > > As "[PATCH v1 34/35] drm/modes: Introduce the tv_mode property as a > > > > > > > > > > > command-line option" uses a separate "tv_mode" option, and not the main > > > > > > > > > > > mode name, I think you want to add them here. > > > > > > > > > > > > > > > > > > > > It's a separate story I think, we could have a named mode hd720p50, > > > > > > > > > > which would be equivalent to 1280x720,tv_mode=hd720p > > > > > > > > > > > > > > > > > > So where's the field rate in "1280x720,tv_mode=hd720p"? > > > > > > > > > > > > > > > > Yeah, sorry I meant 1280x720@50,tv_mode=hd720p > > > > > > > > > > > > > > Above you said "I think the former would make more sense", so that > > > > > > > should be "1280x720,tv_mode=hd720p50"? > > > > > > > > > > > > No, 720p at 50Hz would be either hd720p50 or 1280x720@50,tv_mode=hd720p > > > > > > and 60Hz would be hd720p60 or 1280x720@60,tv_mode=hd720p > > > > > > > > > > I disagree: hd720p50 and hd720p60 are different TV modes. > > > > > > > > I agree, and I don't see how that command-line doesn't express that? > > > > > > Oh, I see what you mean: yes, it expresses that. > > > But it is inconsistent with the NTSC/PAL/SECAM/hd{480,576}[ip] modes, > > > where the TV mode specifies both number of lines and frame rate. > > > > Only if we're using a named mode, and naming is hard :) > > That's not true: "640x480,tv_mode=PAL-N" would give me a mode with > 625 lines and 25 frames/s, "640x480,tv_mode=PAL-M" would give me a > mode with 525 lines and 30 frames/s. In that series, "640x480,tv_mode=PAL-N" would be rejected as invalid: https://lore.kernel.org/dri-devel/20220728-rpi-analog-tv-properties-v1-14-3d53ae722097@cerno.tech/ Maxime
Hi Maxime, On Wed, Aug 17, 2022 at 3:19 PM Maxime Ripard <maxime@cerno.tech> wrote: > On Wed, Aug 17, 2022 at 03:05:52PM +0200, Geert Uytterhoeven wrote: > > On Wed, Aug 17, 2022 at 1:15 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > On Wed, Aug 17, 2022 at 10:35:07AM +0200, Geert Uytterhoeven wrote: > > > > On Wed, Aug 17, 2022 at 9:47 AM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > On Wed, Aug 17, 2022 at 09:31:18AM +0200, Geert Uytterhoeven wrote: > > > > > > On Tue, Aug 16, 2022 at 5:50 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > On Tue, Aug 16, 2022 at 04:43:44PM +0200, Geert Uytterhoeven wrote: > > > > > > > > > > > > Either you have to add them here (e.g. "hd720p50" and "hd720p60"), or > > > > > > > > > > > > handle them through "@<refresh>". The latter would impact "[PATCH v1 > > > > > > > > > > > > 09/35] drm/modes: Move named modes parsing to a separate function", as > > > > > > > > > > > > currently a named mode and a refresh rate can't be specified both. > > > > > > > > > > > > > > > > > > > > > > I think the former would make more sense. It simplifies a bit the > > > > > > > > > > > parser, and we're going to use a named mode anyway. > > > > > > > > > > > > > > > > > > > > > > > As "[PATCH v1 34/35] drm/modes: Introduce the tv_mode property as a > > > > > > > > > > > > command-line option" uses a separate "tv_mode" option, and not the main > > > > > > > > > > > > mode name, I think you want to add them here. > > > > > > > > > > > > > > > > > > > > > > It's a separate story I think, we could have a named mode hd720p50, > > > > > > > > > > > which would be equivalent to 1280x720,tv_mode=hd720p > > > > > > > > > > > > > > > > > > > > So where's the field rate in "1280x720,tv_mode=hd720p"? > > > > > > > > > > > > > > > > > > Yeah, sorry I meant 1280x720@50,tv_mode=hd720p > > > > > > > > > > > > > > > > Above you said "I think the former would make more sense", so that > > > > > > > > should be "1280x720,tv_mode=hd720p50"? > > > > > > > > > > > > > > No, 720p at 50Hz would be either hd720p50 or 1280x720@50,tv_mode=hd720p > > > > > > > and 60Hz would be hd720p60 or 1280x720@60,tv_mode=hd720p > > > > > > > > > > > > I disagree: hd720p50 and hd720p60 are different TV modes. > > > > > > > > > > I agree, and I don't see how that command-line doesn't express that? > > > > > > > > Oh, I see what you mean: yes, it expresses that. > > > > But it is inconsistent with the NTSC/PAL/SECAM/hd{480,576}[ip] modes, > > > > where the TV mode specifies both number of lines and frame rate. > > > > > > Only if we're using a named mode, and naming is hard :) > > > > That's not true: "640x480,tv_mode=PAL-N" would give me a mode with > > 625 lines and 25 frames/s, "640x480,tv_mode=PAL-M" would give me a > > mode with 525 lines and 30 frames/s. > > In that series, "640x480,tv_mode=PAL-N" would be rejected as invalid: > > https://lore.kernel.org/dri-devel/20220728-rpi-analog-tv-properties-v1-14-3d53ae722097@cerno.tech/ It would become supported once the ideas from thread "[PATCH v1 04/35] drm/modes: Introduce 480i and 576i modes" are implemented... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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 17.08.2022 15.11, skrev Noralf Trønnes: > > > Den 17.08.2022 13.46, skrev Maxime Ripard: >> On Tue, Aug 16, 2022 at 09:35:24PM +0200, Noralf Trønnes wrote: >>> Den 16.08.2022 11.49, skrev Maxime Ripard: >>>> On Tue, Aug 16, 2022 at 11:42:20AM +0200, Noralf Trønnes wrote: >>>>> Den 16.08.2022 10.26, skrev Maxime Ripard: >>>>>> Hi, >>>>>> >>>>>> On Mon, Aug 08, 2022 at 02:44:56PM +0200, Noralf Trønnes wrote: >>>>>>> Den 29.07.2022 18.34, 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@cerno.tech> >>>>>>>> >>> How do you test the property? I've used modetest but I can only change >>> to a tv.mode that matches the current display mode. I can't switch from >>> ntsc to pal for instance. >> >> Yep, if you want to change from PAL to NTSC, it will require a new mode. >> > > So userspace has to check tv.mode first and then create a display mode > the driver will accept if switching to a different display mode is > necessary? In other words, userspace can't discover from the kernel > which display modes a certain tv.mode/norm provides before it is > selected? If so, maybe libdrm should have some function(s) to deal with > switching between modes that require a different display mode since > knowledge about which display modes a tv.mode supports is needed before > hand. > I haven't used vc4 on Pi4 in mainline before and have finally gotten it to work. I see that the connector reports 2 modes that together fit all tv.norms so userspace doesn't have to contruct a display mode, but it does need to know which display mode belongs to a certain tv.norm. When I try to use modetest I'm unable to set a mode: pi@pi4t:~ $ modetest -M vc4 -s 45:720x480i setting mode 720x480i-29.97Hz on connectors 45, crtc 68 failed to set mode: Function not implemented The errno is misleading, modetest does a drmModeDirtyFB before checking the error returned by drmModeSetCrtc. Setting the property succeeds, but the modeset still fails: pi@pi4t:~ $ modetest -M vc4 -s 45:720x480i -w 45:"tv norm":2 setting mode 720x480i-29.97Hz on connectors 45, crtc 68 failed to set mode: Function not implemented pi@pi4t:~ $ modetest -M vc4 -c 37 tv norm: flags: bitmask values: NTSC-443=0x1 NTSC-J=0x2 NTSC-M=0x4 PAL-B=0x10 PAL-M=0x200 PAL-N=0x400 SECAM-B=0x2000 value: 2 Here's the log, can you see if there's anything obvious in there: https://gist.github.com/notro/a079498bf6b64327105752b2bafa8858 Noralf.
On Wed, Aug 17, 2022 at 04:04:24PM +0200, Geert Uytterhoeven wrote: > Hi Maxime, > > On Wed, Aug 17, 2022 at 3:19 PM Maxime Ripard <maxime@cerno.tech> wrote: > > On Wed, Aug 17, 2022 at 03:05:52PM +0200, Geert Uytterhoeven wrote: > > > On Wed, Aug 17, 2022 at 1:15 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > On Wed, Aug 17, 2022 at 10:35:07AM +0200, Geert Uytterhoeven wrote: > > > > > On Wed, Aug 17, 2022 at 9:47 AM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > On Wed, Aug 17, 2022 at 09:31:18AM +0200, Geert Uytterhoeven wrote: > > > > > > > On Tue, Aug 16, 2022 at 5:50 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > On Tue, Aug 16, 2022 at 04:43:44PM +0200, Geert Uytterhoeven wrote: > > > > > > > > > > > > > Either you have to add them here (e.g. "hd720p50" and "hd720p60"), or > > > > > > > > > > > > > handle them through "@<refresh>". The latter would impact "[PATCH v1 > > > > > > > > > > > > > 09/35] drm/modes: Move named modes parsing to a separate function", as > > > > > > > > > > > > > currently a named mode and a refresh rate can't be specified both. > > > > > > > > > > > > > > > > > > > > > > > > I think the former would make more sense. It simplifies a bit the > > > > > > > > > > > > parser, and we're going to use a named mode anyway. > > > > > > > > > > > > > > > > > > > > > > > > > As "[PATCH v1 34/35] drm/modes: Introduce the tv_mode property as a > > > > > > > > > > > > > command-line option" uses a separate "tv_mode" option, and not the main > > > > > > > > > > > > > mode name, I think you want to add them here. > > > > > > > > > > > > > > > > > > > > > > > > It's a separate story I think, we could have a named mode hd720p50, > > > > > > > > > > > > which would be equivalent to 1280x720,tv_mode=hd720p > > > > > > > > > > > > > > > > > > > > > > So where's the field rate in "1280x720,tv_mode=hd720p"? > > > > > > > > > > > > > > > > > > > > Yeah, sorry I meant 1280x720@50,tv_mode=hd720p > > > > > > > > > > > > > > > > > > Above you said "I think the former would make more sense", so that > > > > > > > > > should be "1280x720,tv_mode=hd720p50"? > > > > > > > > > > > > > > > > No, 720p at 50Hz would be either hd720p50 or 1280x720@50,tv_mode=hd720p > > > > > > > > and 60Hz would be hd720p60 or 1280x720@60,tv_mode=hd720p > > > > > > > > > > > > > > I disagree: hd720p50 and hd720p60 are different TV modes. > > > > > > > > > > > > I agree, and I don't see how that command-line doesn't express that? > > > > > > > > > > Oh, I see what you mean: yes, it expresses that. > > > > > But it is inconsistent with the NTSC/PAL/SECAM/hd{480,576}[ip] modes, > > > > > where the TV mode specifies both number of lines and frame rate. > > > > > > > > Only if we're using a named mode, and naming is hard :) > > > > > > That's not true: "640x480,tv_mode=PAL-N" would give me a mode with > > > 625 lines and 25 frames/s, "640x480,tv_mode=PAL-M" would give me a > > > mode with 525 lines and 30 frames/s. > > > > In that series, "640x480,tv_mode=PAL-N" would be rejected as invalid: > > > > https://lore.kernel.org/dri-devel/20220728-rpi-analog-tv-properties-v1-14-3d53ae722097@cerno.tech/ > > It would become supported once the ideas from thread "[PATCH v1 04/35] > drm/modes: Introduce 480i and 576i modes" are implemented... Indeed, but I'm still not sure what your concern is here. "640x480,tv_mode=PAL-N" and "640x480,tv_mode=PAL-M" are both fairly obvious. You were initially saying that you had concern over the inconsistency of NTSC/PAL/SECAM where the TV mode would specify a number of lines and frame rate, but hd720p50 also specifies a number of line and frame rate? Maxime
Den 18.08.2022 01.23, skrev Noralf Trønnes: > > > Den 17.08.2022 15.11, skrev Noralf Trønnes: >> >> >> Den 17.08.2022 13.46, skrev Maxime Ripard: >>> On Tue, Aug 16, 2022 at 09:35:24PM +0200, Noralf Trønnes wrote: >>>> Den 16.08.2022 11.49, skrev Maxime Ripard: >>>>> On Tue, Aug 16, 2022 at 11:42:20AM +0200, Noralf Trønnes wrote: >>>>>> Den 16.08.2022 10.26, skrev Maxime Ripard: >>>>>>> Hi, >>>>>>> >>>>>>> On Mon, Aug 08, 2022 at 02:44:56PM +0200, Noralf Trønnes wrote: >>>>>>>> Den 29.07.2022 18.34, 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@cerno.tech> >>>>>>>>> > >>>> How do you test the property? I've used modetest but I can only change >>>> to a tv.mode that matches the current display mode. I can't switch from >>>> ntsc to pal for instance. >>> >>> Yep, if you want to change from PAL to NTSC, it will require a new mode. >>> >> >> So userspace has to check tv.mode first and then create a display mode >> the driver will accept if switching to a different display mode is >> necessary? In other words, userspace can't discover from the kernel >> which display modes a certain tv.mode/norm provides before it is >> selected? If so, maybe libdrm should have some function(s) to deal with >> switching between modes that require a different display mode since >> knowledge about which display modes a tv.mode supports is needed before >> hand. >> > > I haven't used vc4 on Pi4 in mainline before and have finally gotten it > to work. > > I see that the connector reports 2 modes that together fit all tv.norms > so userspace doesn't have to contruct a display mode, but it does need > to know which display mode belongs to a certain tv.norm. > > When I try to use modetest I'm unable to set a mode: > > pi@pi4t:~ $ modetest -M vc4 -s 45:720x480i > setting mode 720x480i-29.97Hz on connectors 45, crtc 68 > failed to set mode: Function not implemented > > The errno is misleading, modetest does a drmModeDirtyFB before checking > the error returned by drmModeSetCrtc. > > Setting the property succeeds, but the modeset still fails: > > pi@pi4t:~ $ modetest -M vc4 -s 45:720x480i -w 45:"tv norm":2 > setting mode 720x480i-29.97Hz on connectors 45, crtc 68 > failed to set mode: Function not implemented > > pi@pi4t:~ $ modetest -M vc4 -c > 37 tv norm: > flags: bitmask > values: NTSC-443=0x1 NTSC-J=0x2 NTSC-M=0x4 PAL-B=0x10 > PAL-M=0x200 PAL-N=0x400 SECAM-B=0x2000 > value: 2 > > Here's the log, can you see if there's anything obvious in there: > https://gist.github.com/notro/a079498bf6b64327105752b2bafa8858 > I'm one step closer as I now have fbcon working, I had forgotten to add enable_tvout=1 and I had disable_fw_kms_setup=1 which disables the video= mode on the kernel commandline. modetest still fails though, after alot of printk sprinkling, I've tracked it down to the drm_mode_equal test in drm_atomic_helper_connector_tv_check(). The aspect ratios differ: [ 61.336295] drm_atomic_helper_connector_tv_check: mode->picture_aspect_ratio=1 [ 61.336301] drm_atomic_helper_connector_tv_check: &crtc_state->mode->picture_aspect_ratio=0 Noralf.
Hi Maxime, On Thu, Aug 18, 2022 at 4:54 PM Maxime Ripard <maxime@cerno.tech> wrote: > On Wed, Aug 17, 2022 at 04:04:24PM +0200, Geert Uytterhoeven wrote: > > On Wed, Aug 17, 2022 at 3:19 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > On Wed, Aug 17, 2022 at 03:05:52PM +0200, Geert Uytterhoeven wrote: > > > > On Wed, Aug 17, 2022 at 1:15 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > On Wed, Aug 17, 2022 at 10:35:07AM +0200, Geert Uytterhoeven wrote: > > > > > > On Wed, Aug 17, 2022 at 9:47 AM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > On Wed, Aug 17, 2022 at 09:31:18AM +0200, Geert Uytterhoeven wrote: > > > > > > > > On Tue, Aug 16, 2022 at 5:50 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > On Tue, Aug 16, 2022 at 04:43:44PM +0200, Geert Uytterhoeven wrote: > > > > > > > > > > > > > > Either you have to add them here (e.g. "hd720p50" and "hd720p60"), or > > > > > > > > > > > > > > handle them through "@<refresh>". The latter would impact "[PATCH v1 > > > > > > > > > > > > > > 09/35] drm/modes: Move named modes parsing to a separate function", as > > > > > > > > > > > > > > currently a named mode and a refresh rate can't be specified both. > > > > > > > > > > > > > > > > > > > > > > > > > > I think the former would make more sense. It simplifies a bit the > > > > > > > > > > > > > parser, and we're going to use a named mode anyway. > > > > > > > > > > > > > > > > > > > > > > > > > > > As "[PATCH v1 34/35] drm/modes: Introduce the tv_mode property as a > > > > > > > > > > > > > > command-line option" uses a separate "tv_mode" option, and not the main > > > > > > > > > > > > > > mode name, I think you want to add them here. > > > > > > > > > > > > > > > > > > > > > > > > > > It's a separate story I think, we could have a named mode hd720p50, > > > > > > > > > > > > > which would be equivalent to 1280x720,tv_mode=hd720p > > > > > > > > > > > > > > > > > > > > > > > > So where's the field rate in "1280x720,tv_mode=hd720p"? > > > > > > > > > > > > > > > > > > > > > > Yeah, sorry I meant 1280x720@50,tv_mode=hd720p > > > > > > > > > > > > > > > > > > > > Above you said "I think the former would make more sense", so that > > > > > > > > > > should be "1280x720,tv_mode=hd720p50"? > > > > > > > > > > > > > > > > > > No, 720p at 50Hz would be either hd720p50 or 1280x720@50,tv_mode=hd720p > > > > > > > > > and 60Hz would be hd720p60 or 1280x720@60,tv_mode=hd720p > > > > > > > > > > > > > > > > I disagree: hd720p50 and hd720p60 are different TV modes. > > > > > > > > > > > > > > I agree, and I don't see how that command-line doesn't express that? > > > > > > > > > > > > Oh, I see what you mean: yes, it expresses that. > > > > > > But it is inconsistent with the NTSC/PAL/SECAM/hd{480,576}[ip] modes, > > > > > > where the TV mode specifies both number of lines and frame rate. > > > > > > > > > > Only if we're using a named mode, and naming is hard :) > > > > > > > > That's not true: "640x480,tv_mode=PAL-N" would give me a mode with > > > > 625 lines and 25 frames/s, "640x480,tv_mode=PAL-M" would give me a > > > > mode with 525 lines and 30 frames/s. > > > > > > In that series, "640x480,tv_mode=PAL-N" would be rejected as invalid: > > > > > > https://lore.kernel.org/dri-devel/20220728-rpi-analog-tv-properties-v1-14-3d53ae722097@cerno.tech/ > > > > It would become supported once the ideas from thread "[PATCH v1 04/35] > > drm/modes: Introduce 480i and 576i modes" are implemented... > > Indeed, but I'm still not sure what your concern is here. > "640x480,tv_mode=PAL-N" and "640x480,tv_mode=PAL-M" are both fairly > obvious. > > You were initially saying that you had concern over the inconsistency of > NTSC/PAL/SECAM where the TV mode would specify a number of lines and > frame rate, but hd720p50 also specifies a number of line and frame rate? My concern is that you want to call the TV mode "hd720p", which does not dictate the frame rate. I would like to have both "720p50" and "720p60", as they do dictate the frame rate, like all the non-hd modes. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
On Wed, Aug 17, 2022 at 03:11:56PM +0200, Noralf Trønnes wrote: > Den 17.08.2022 13.46, skrev Maxime Ripard: > > On Tue, Aug 16, 2022 at 09:35:24PM +0200, Noralf Trønnes wrote: > >> Den 16.08.2022 11.49, skrev Maxime Ripard: > >>> On Tue, Aug 16, 2022 at 11:42:20AM +0200, Noralf Trønnes wrote: > >>>> Den 16.08.2022 10.26, skrev Maxime Ripard: > >>>>> Hi, > >>>>> > >>>>> On Mon, Aug 08, 2022 at 02:44:56PM +0200, Noralf Trønnes wrote: > >>>>>> Den 29.07.2022 18.34, 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@cerno.tech> > >>>>>>> > >>>>>> > >>>>>> Please also update Documentation/gpu/kms-properties.csv > >>>>>> > >>>>>> Requirements for adding a new property is found in > >>>>>> Documentation/gpu/drm-kms.rst > >>>>> > >>>>> I knew this was going to be raised at some point, so I'm glad it's that > >>>>> early :) > >>>>> > >>>>> I really don't know what to do there. If we stick by our usual rules, > >>>>> then we can't have any of that work merged. > >>>>> > >>>>> However, I think the status quo is not really satisfactory either. > >>>>> Indeed, we have a property, that doesn't follow those requirements > >>>>> either, with a driver-specific content, and that stands in the way of > >>>>> fixes and further improvements at both the core framework and driver > >>>>> levels. > >>>>> > >>>>> So having that new property still seems like a net improvement at the > >>>>> driver, framework and uAPI levels, even if it's not entirely following > >>>>> the requirements we have in place. > >>>>> > >>>>> Even more so since, realistically, those kind of interfaces will never > >>>>> get any new development on the user-space side of things, it's > >>>>> considered by everyone as legacy. > >>>>> > >>>>> This also is something we need to support at some point if we want to > >>>>> completely deprecate the fbdev drivers and provide decent alternatives > >>>>> in KMS. > >>>>> > >>>>> So yeah, strictly speaking, we would not qualify for our requirements > >>>>> there. I still think we have a strong case for an exception though. > >>>> > >>>> Which requirements would that be? The only one I can see is the > >>>> documentation and maybe an IGT test. > >>> > >>> This is the one I had in mind > >>> https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements > >>> > >> > >> Oh right, I had forgotten about that one. > >> > >> One benefit of having a userspace implementation is that it increases > >> the chance of widespread adoption having a working implementation to > >> look at. I don't think the reason tv.mode is not used anywhere (that I > >> know of) is because the driver picks the enum values resulting in no > >> standard names. > > > > It probably doesn't help, but it's not what I was implying. > > > >> It's a niche thing and way down on the todo list. nouveau and ch7006 > >> has a tv_norm module parameter which certainly doesn't help in moving > >> people/projects over to the DRM property (downstream rpi also has it > >> now). > > > > Yeah, the RPi version is part of the reason I started working on this. > > We should also consider the named modes used by vc4 and sun4i. All these > > ad-hoc solutions are limited and (I think) come from the fact that we > > don't have a solution easy enough to use for drivers (and to discover). > > > > nouveau, ch7006, i915 and vc4 are using the tv.mode property for > > example, but sun4i and meson don't. > > > > sun4i relies on named modes to reimplement TV modes, but meson doesn't > > at all. > > > > And then nouveau has that extra command line parameter to set it up at > > boot time. > > > > It doesn't really make much sense to me, when all drivers have very > > similar needs, that none of them behave in the same way. And I think the > > non-standard property is partly to blame for this, since without some > > generic content we can't share code. > > > > This is what this series is about: every driver having similar > > capabilities and as trivially as possible. > > > >> mpv[1] is a commandline media player that after a quick look might be a > >> candidate for implementing the property without too much effort. > > > > Kodi might be another one. I can try to hack something around, but I'm > > really skeptical about whether a PR would be merged or not. > > > > You can ask first before wasting time ofc. Yep, will do. > But this baffles me, if you don't think projects like Kodi which is TV > centered want this, what kind of projects do you think want to use this > property? I mean, at this point it's really a consolidation of stuff we have scattered around the kernel tree, in order to clean up that mess, and not add more hacks. And it allows the current effort to move the remaining fbdev drivers into KMS. As far as userspace is concerned, I don't know who is still using it or cares today. I still believe that refactoring is beneficial though, if only to make one more nail in fbdev's coffin. > >> How do you test the property? I've used modetest but I can only change > >> to a tv.mode that matches the current display mode. I can't switch from > >> ntsc to pal for instance. > > > > Yep, if you want to change from PAL to NTSC, it will require a new mode. > > So userspace has to check tv.mode first and then create a display mode > the driver will accept if switching to a different display mode is > necessary? I'd expect drivers to expose both 576i and 480i (that's what vc4 and sun4i are doing), so the userspace can pick them up. > In other words, userspace can't discover from the kernel which display > modes a certain tv.mode/norm provides before it is selected? It's kind of the other way around in my mind, but the userspace would have to figure out what display mode can use what tv mode, yes. Even more so since Geert and I are discussing to allow continuous modes, so we would allow to have modes with the same (active) resolution but a different tv mode. > If so, maybe libdrm should have some function(s) to deal with > switching between modes that require a different display mode since > knowledge about which display modes a tv.mode supports is needed > before hand. I'm not sure what you mean here, sorry. It's fairly easy to update a property and the mode with atomic? Maxime
On Thu, Aug 18, 2022 at 05:01:38PM +0200, Noralf Trønnes wrote: > > > Den 18.08.2022 01.23, skrev Noralf Trønnes: > > > > > > Den 17.08.2022 15.11, skrev Noralf Trønnes: > >> > >> > >> Den 17.08.2022 13.46, skrev Maxime Ripard: > >>> On Tue, Aug 16, 2022 at 09:35:24PM +0200, Noralf Trønnes wrote: > >>>> Den 16.08.2022 11.49, skrev Maxime Ripard: > >>>>> On Tue, Aug 16, 2022 at 11:42:20AM +0200, Noralf Trønnes wrote: > >>>>>> Den 16.08.2022 10.26, skrev Maxime Ripard: > >>>>>>> Hi, > >>>>>>> > >>>>>>> On Mon, Aug 08, 2022 at 02:44:56PM +0200, Noralf Trønnes wrote: > >>>>>>>> Den 29.07.2022 18.34, 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@cerno.tech> > >>>>>>>>> > > > >>>> How do you test the property? I've used modetest but I can only change > >>>> to a tv.mode that matches the current display mode. I can't switch from > >>>> ntsc to pal for instance. > >>> > >>> Yep, if you want to change from PAL to NTSC, it will require a new mode. > >>> > >> > >> So userspace has to check tv.mode first and then create a display mode > >> the driver will accept if switching to a different display mode is > >> necessary? In other words, userspace can't discover from the kernel > >> which display modes a certain tv.mode/norm provides before it is > >> selected? If so, maybe libdrm should have some function(s) to deal with > >> switching between modes that require a different display mode since > >> knowledge about which display modes a tv.mode supports is needed before > >> hand. > >> > > > > I haven't used vc4 on Pi4 in mainline before and have finally gotten it > > to work. > > > > I see that the connector reports 2 modes that together fit all tv.norms > > so userspace doesn't have to contruct a display mode, but it does need > > to know which display mode belongs to a certain tv.norm. > > > > When I try to use modetest I'm unable to set a mode: > > > > pi@pi4t:~ $ modetest -M vc4 -s 45:720x480i > > setting mode 720x480i-29.97Hz on connectors 45, crtc 68 > > failed to set mode: Function not implemented > > > > The errno is misleading, modetest does a drmModeDirtyFB before checking > > the error returned by drmModeSetCrtc. > > > > Setting the property succeeds, but the modeset still fails: > > > > pi@pi4t:~ $ modetest -M vc4 -s 45:720x480i -w 45:"tv norm":2 > > setting mode 720x480i-29.97Hz on connectors 45, crtc 68 > > failed to set mode: Function not implemented > > > > pi@pi4t:~ $ modetest -M vc4 -c > > 37 tv norm: > > flags: bitmask > > values: NTSC-443=0x1 NTSC-J=0x2 NTSC-M=0x4 PAL-B=0x10 > > PAL-M=0x200 PAL-N=0x400 SECAM-B=0x2000 > > value: 2 > > > > Here's the log, can you see if there's anything obvious in there: > > https://gist.github.com/notro/a079498bf6b64327105752b2bafa8858 > > > > I'm one step closer as I now have fbcon working, I had forgotten to add > enable_tvout=1 and I had disable_fw_kms_setup=1 which disables the > video= mode on the kernel commandline. > > modetest still fails though, after alot of printk sprinkling, I've > tracked it down to the drm_mode_equal test in > drm_atomic_helper_connector_tv_check(). The aspect ratios differ: > > [ 61.336295] drm_atomic_helper_connector_tv_check: > mode->picture_aspect_ratio=1 > [ 61.336301] drm_atomic_helper_connector_tv_check: > &crtc_state->mode->picture_aspect_ratio=0 I haven't seen this when testing, but I'll have a look, thanks! Maxime
On Thu, Aug 18, 2022 at 05:20:42PM +0200, Geert Uytterhoeven wrote: > Hi Maxime, > > On Thu, Aug 18, 2022 at 4:54 PM Maxime Ripard <maxime@cerno.tech> wrote: > > On Wed, Aug 17, 2022 at 04:04:24PM +0200, Geert Uytterhoeven wrote: > > > On Wed, Aug 17, 2022 at 3:19 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > On Wed, Aug 17, 2022 at 03:05:52PM +0200, Geert Uytterhoeven wrote: > > > > > On Wed, Aug 17, 2022 at 1:15 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > On Wed, Aug 17, 2022 at 10:35:07AM +0200, Geert Uytterhoeven wrote: > > > > > > > On Wed, Aug 17, 2022 at 9:47 AM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > On Wed, Aug 17, 2022 at 09:31:18AM +0200, Geert Uytterhoeven wrote: > > > > > > > > > On Tue, Aug 16, 2022 at 5:50 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > On Tue, Aug 16, 2022 at 04:43:44PM +0200, Geert Uytterhoeven wrote: > > > > > > > > > > > > > > > Either you have to add them here (e.g. "hd720p50" and "hd720p60"), or > > > > > > > > > > > > > > > handle them through "@<refresh>". The latter would impact "[PATCH v1 > > > > > > > > > > > > > > > 09/35] drm/modes: Move named modes parsing to a separate function", as > > > > > > > > > > > > > > > currently a named mode and a refresh rate can't be specified both. > > > > > > > > > > > > > > > > > > > > > > > > > > > > I think the former would make more sense. It simplifies a bit the > > > > > > > > > > > > > > parser, and we're going to use a named mode anyway. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > As "[PATCH v1 34/35] drm/modes: Introduce the tv_mode property as a > > > > > > > > > > > > > > > command-line option" uses a separate "tv_mode" option, and not the main > > > > > > > > > > > > > > > mode name, I think you want to add them here. > > > > > > > > > > > > > > > > > > > > > > > > > > > > It's a separate story I think, we could have a named mode hd720p50, > > > > > > > > > > > > > > which would be equivalent to 1280x720,tv_mode=hd720p > > > > > > > > > > > > > > > > > > > > > > > > > > So where's the field rate in "1280x720,tv_mode=hd720p"? > > > > > > > > > > > > > > > > > > > > > > > > Yeah, sorry I meant 1280x720@50,tv_mode=hd720p > > > > > > > > > > > > > > > > > > > > > > Above you said "I think the former would make more sense", so that > > > > > > > > > > > should be "1280x720,tv_mode=hd720p50"? > > > > > > > > > > > > > > > > > > > > No, 720p at 50Hz would be either hd720p50 or 1280x720@50,tv_mode=hd720p > > > > > > > > > > and 60Hz would be hd720p60 or 1280x720@60,tv_mode=hd720p > > > > > > > > > > > > > > > > > > I disagree: hd720p50 and hd720p60 are different TV modes. > > > > > > > > > > > > > > > > I agree, and I don't see how that command-line doesn't express that? > > > > > > > > > > > > > > Oh, I see what you mean: yes, it expresses that. > > > > > > > But it is inconsistent with the NTSC/PAL/SECAM/hd{480,576}[ip] modes, > > > > > > > where the TV mode specifies both number of lines and frame rate. > > > > > > > > > > > > Only if we're using a named mode, and naming is hard :) > > > > > > > > > > That's not true: "640x480,tv_mode=PAL-N" would give me a mode with > > > > > 625 lines and 25 frames/s, "640x480,tv_mode=PAL-M" would give me a > > > > > mode with 525 lines and 30 frames/s. > > > > > > > > In that series, "640x480,tv_mode=PAL-N" would be rejected as invalid: > > > > > > > > https://lore.kernel.org/dri-devel/20220728-rpi-analog-tv-properties-v1-14-3d53ae722097@cerno.tech/ > > > > > > It would become supported once the ideas from thread "[PATCH v1 04/35] > > > drm/modes: Introduce 480i and 576i modes" are implemented... > > > > Indeed, but I'm still not sure what your concern is here. > > "640x480,tv_mode=PAL-N" and "640x480,tv_mode=PAL-M" are both fairly > > obvious. > > > > You were initially saying that you had concern over the inconsistency of > > NTSC/PAL/SECAM where the TV mode would specify a number of lines and > > frame rate, but hd720p50 also specifies a number of line and frame rate? > > My concern is that you want to call the TV mode "hd720p", which > does not dictate the frame rate. > I would like to have both "720p50" and "720p60", as they do dictate > the frame rate, like all the non-hd modes. But they don't? The refresh rate is part of the drm_display_mode, whereas that property is metadata and entirely separate from the display mode. You can even change it without changing the mode at all Maxime
Hi Maxime, On Thu, Aug 18, 2022 at 5:34 PM Maxime Ripard <maxime@cerno.tech> wrote: > On Thu, Aug 18, 2022 at 05:20:42PM +0200, Geert Uytterhoeven wrote: > > On Thu, Aug 18, 2022 at 4:54 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > On Wed, Aug 17, 2022 at 04:04:24PM +0200, Geert Uytterhoeven wrote: > > > > On Wed, Aug 17, 2022 at 3:19 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > On Wed, Aug 17, 2022 at 03:05:52PM +0200, Geert Uytterhoeven wrote: > > > > > > On Wed, Aug 17, 2022 at 1:15 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > On Wed, Aug 17, 2022 at 10:35:07AM +0200, Geert Uytterhoeven wrote: > > > > > > > > On Wed, Aug 17, 2022 at 9:47 AM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > On Wed, Aug 17, 2022 at 09:31:18AM +0200, Geert Uytterhoeven wrote: > > > > > > > > > > On Tue, Aug 16, 2022 at 5:50 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > > On Tue, Aug 16, 2022 at 04:43:44PM +0200, Geert Uytterhoeven wrote: > > > > > > > > > > > > > > > > Either you have to add them here (e.g. "hd720p50" and "hd720p60"), or > > > > > > > > > > > > > > > > handle them through "@<refresh>". The latter would impact "[PATCH v1 > > > > > > > > > > > > > > > > 09/35] drm/modes: Move named modes parsing to a separate function", as > > > > > > > > > > > > > > > > currently a named mode and a refresh rate can't be specified both. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I think the former would make more sense. It simplifies a bit the > > > > > > > > > > > > > > > parser, and we're going to use a named mode anyway. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > As "[PATCH v1 34/35] drm/modes: Introduce the tv_mode property as a > > > > > > > > > > > > > > > > command-line option" uses a separate "tv_mode" option, and not the main > > > > > > > > > > > > > > > > mode name, I think you want to add them here. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > It's a separate story I think, we could have a named mode hd720p50, > > > > > > > > > > > > > > > which would be equivalent to 1280x720,tv_mode=hd720p > > > > > > > > > > > > > > > > > > > > > > > > > > > > So where's the field rate in "1280x720,tv_mode=hd720p"? > > > > > > > > > > > > > > > > > > > > > > > > > > Yeah, sorry I meant 1280x720@50,tv_mode=hd720p > > > > > > > > > > > > > > > > > > > > > > > > Above you said "I think the former would make more sense", so that > > > > > > > > > > > > should be "1280x720,tv_mode=hd720p50"? > > > > > > > > > > > > > > > > > > > > > > No, 720p at 50Hz would be either hd720p50 or 1280x720@50,tv_mode=hd720p > > > > > > > > > > > and 60Hz would be hd720p60 or 1280x720@60,tv_mode=hd720p > > > > > > > > > > > > > > > > > > > > I disagree: hd720p50 and hd720p60 are different TV modes. > > > > > > > > > > > > > > > > > > I agree, and I don't see how that command-line doesn't express that? > > > > > > > > > > > > > > > > Oh, I see what you mean: yes, it expresses that. > > > > > > > > But it is inconsistent with the NTSC/PAL/SECAM/hd{480,576}[ip] modes, > > > > > > > > where the TV mode specifies both number of lines and frame rate. > > > > > > > > > > > > > > Only if we're using a named mode, and naming is hard :) > > > > > > > > > > > > That's not true: "640x480,tv_mode=PAL-N" would give me a mode with > > > > > > 625 lines and 25 frames/s, "640x480,tv_mode=PAL-M" would give me a > > > > > > mode with 525 lines and 30 frames/s. > > > > > > > > > > In that series, "640x480,tv_mode=PAL-N" would be rejected as invalid: > > > > > > > > > > https://lore.kernel.org/dri-devel/20220728-rpi-analog-tv-properties-v1-14-3d53ae722097@cerno.tech/ > > > > > > > > It would become supported once the ideas from thread "[PATCH v1 04/35] > > > > drm/modes: Introduce 480i and 576i modes" are implemented... > > > > > > Indeed, but I'm still not sure what your concern is here. > > > "640x480,tv_mode=PAL-N" and "640x480,tv_mode=PAL-M" are both fairly > > > obvious. > > > > > > You were initially saying that you had concern over the inconsistency of > > > NTSC/PAL/SECAM where the TV mode would specify a number of lines and > > > frame rate, but hd720p50 also specifies a number of line and frame rate? > > > > My concern is that you want to call the TV mode "hd720p", which > > does not dictate the frame rate. > > I would like to have both "720p50" and "720p60", as they do dictate > > the frame rate, like all the non-hd modes. > > But they don't? > > The refresh rate is part of the drm_display_mode, whereas that property > is metadata and entirely separate from the display mode. > > You can even change it without changing the mode at all Yes, the refresh rate is part of drm_display_mode. Vdisplay also is, but that doesn't mean you can set it to e.g. 700 when using "tv_mode=PAL-B". Some (combination of) parameters in drm_display_mode are dictated by the tv_mode. Perhaps the meaning of "tv_mode" should be clarified? What does it really mean, and what parameters does it (not) constrain? For e.g. "PAL-B", I know it's a mode with 625 lines and 30 frames/s (60 fields/s). For "hd720p" I know it is an analog mode with 750 lines, but it's still ambiguous, as I don't know if it is the variant with 60 or 50 frames/s. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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.07.2022 18.34, 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@cerno.tech> > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > index c06d0639d552..d7ff6c644c2f 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->tv_mode_property) { > state->tv.mode = val; > + } else if (property == config->tv_norm_property) { > + state->tv.norm = 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->tv_mode_property) { > *val = state->tv.mode; > + } else if (property == config->tv_norm_property) { > + *val = state->tv.norm; > } 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 e3142c8142b3..68a4e47f85a9 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -1637,6 +1637,7 @@ EXPORT_SYMBOL(drm_mode_create_tv_margin_properties); > /** > * drm_mode_create_tv_properties - create TV specific connector properties > * @dev: DRM device > + * @supported_tv_norms: Bitmask of TV norms supported (See DRM_MODE_TV_NORM_*) > * @num_modes: number of different TV formats (modes) supported > * @modes: array of pointers to strings containing name of each format > * > @@ -1649,11 +1650,40 @@ EXPORT_SYMBOL(drm_mode_create_tv_margin_properties); > * 0 on success or a negative error code on failure. > */ > int drm_mode_create_tv_properties(struct drm_device *dev, > + unsigned int supported_tv_norms, > unsigned int num_modes, > const char * const modes[]) > { > + static const struct drm_prop_enum_list tv_norm_values[] = { > + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_443) - 1, "NTSC-443" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_J) - 1, "NTSC-J" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_M) - 1, "NTSC-M" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_60) - 1, "PAL-60" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_B) - 1, "PAL-B" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_D) - 1, "PAL-D" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_G) - 1, "PAL-G" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_H) - 1, "PAL-H" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_I) - 1, "PAL-I" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_M) - 1, "PAL-M" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_N) - 1, "PAL-N" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_NC) - 1, "PAL-Nc" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_60) - 1, "SECAM-60" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_B) - 1, "SECAM-B" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_D) - 1, "SECAM-D" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_G) - 1, "SECAM-G" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_K) - 1, "SECAM-K" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_K1) - 1, "SECAM-K1" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_L) - 1, "SECAM-L" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_HD480I) - 1, "hd480i" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_HD480P) - 1, "hd480p" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_HD576I) - 1, "hd576i" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_HD576P) - 1, "hd576p" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_HD720P) - 1, "hd720p" }, > + { __builtin_ffs(DRM_MODE_TV_NORM_HD1080I) - 1, "hd1080i" }, > + }; > struct drm_property *tv_selector; > struct drm_property *tv_subconnector; > + struct drm_property *tv_norm; > unsigned int i; > > if (dev->mode_config.tv_select_subconnector_property) > @@ -1686,6 +1716,13 @@ int drm_mode_create_tv_properties(struct drm_device *dev, > if (drm_mode_create_tv_margin_properties(dev)) > goto nomem; > > + tv_norm = drm_property_create_bitmask(dev, 0, "tv norm", > + tv_norm_values, ARRAY_SIZE(tv_norm_values), > + supported_tv_norms); > + if (!tv_norm) > + goto nomem; > + dev->mode_config.tv_norm_property = tv_norm; > + > dev->mode_config.tv_mode_property = > drm_property_create(dev, DRM_MODE_PROP_ENUM, > "mode", num_modes); > diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c > index 4a788c1c9058..457529e5d857 100644 > --- a/drivers/gpu/drm/vc4/vc4_vec.c > +++ b/drivers/gpu/drm/vc4/vc4_vec.c > @@ -573,7 +573,9 @@ static int vc4_vec_bind(struct device *dev, struct device *master, void *data) > struct vc4_vec *vec; > int ret; > > - ret = drm_mode_create_tv_properties(drm, ARRAY_SIZE(tv_mode_names), > + ret = drm_mode_create_tv_properties(drm, > + 0, > + ARRAY_SIZE(tv_mode_names), > tv_mode_names); > if (ret) > return ret; > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index 1e9996b33cc8..78275e68ff66 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -143,6 +143,32 @@ enum subpixel_order { > > }; > > +#define DRM_MODE_TV_NORM_NTSC_443 (1 << 0) > +#define DRM_MODE_TV_NORM_NTSC_J (1 << 1) > +#define DRM_MODE_TV_NORM_NTSC_M (1 << 2) > +#define DRM_MODE_TV_NORM_PAL_60 (1 << 3) > +#define DRM_MODE_TV_NORM_PAL_B (1 << 4) > +#define DRM_MODE_TV_NORM_PAL_D (1 << 5) > +#define DRM_MODE_TV_NORM_PAL_G (1 << 6) > +#define DRM_MODE_TV_NORM_PAL_H (1 << 7) > +#define DRM_MODE_TV_NORM_PAL_I (1 << 8) > +#define DRM_MODE_TV_NORM_PAL_M (1 << 9) > +#define DRM_MODE_TV_NORM_PAL_N (1 << 10) > +#define DRM_MODE_TV_NORM_PAL_NC (1 << 11) > +#define DRM_MODE_TV_NORM_SECAM_60 (1 << 12) > +#define DRM_MODE_TV_NORM_SECAM_B (1 << 13) > +#define DRM_MODE_TV_NORM_SECAM_D (1 << 14) > +#define DRM_MODE_TV_NORM_SECAM_G (1 << 15) > +#define DRM_MODE_TV_NORM_SECAM_K (1 << 16) > +#define DRM_MODE_TV_NORM_SECAM_K1 (1 << 17) > +#define DRM_MODE_TV_NORM_SECAM_L (1 << 18) > +#define DRM_MODE_TV_NORM_HD480I (1 << 19) > +#define DRM_MODE_TV_NORM_HD480P (1 << 20) > +#define DRM_MODE_TV_NORM_HD576I (1 << 21) > +#define DRM_MODE_TV_NORM_HD576P (1 << 22) > +#define DRM_MODE_TV_NORM_HD720P (1 << 23) > +#define DRM_MODE_TV_NORM_HD1080I (1 << 24) > + This is an area where DRM overlaps with v4l2, see: - include/dt-bindings/display/sdtv-standards.h - v4l2_norm_to_name() Maybe we should follow suit, but if we do our own thing please mention why in the commit message. Noralf. > /** > * struct drm_scrambling: sink's scrambling support. > */ > @@ -687,6 +713,7 @@ struct drm_tv_connector_state { > enum drm_mode_subconnector subconnector; > struct drm_connector_tv_margins margins; > unsigned int mode; > + unsigned int norm; > unsigned int brightness; > unsigned int contrast; > unsigned int flicker_reduction; > @@ -1779,6 +1806,7 @@ void drm_connector_attach_dp_subconnector_property(struct drm_connector *connect > > int drm_mode_create_tv_margin_properties(struct drm_device *dev); > int drm_mode_create_tv_properties(struct drm_device *dev, > + unsigned int supported_tv_norms, > unsigned int num_modes, > const char * const modes[]); > void drm_connector_attach_tv_margin_properties(struct drm_connector *conn); > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > index 6b5e01295348..d9e79def8b92 100644 > --- a/include/drm/drm_mode_config.h > +++ b/include/drm/drm_mode_config.h > @@ -704,6 +704,12 @@ struct drm_mode_config { > */ > struct drm_property *dp_subconnector_property; > > + /** > + * @tv_norm_property: Optional TV property to select the TV > + * standard output on the connector. > + */ > + struct drm_property *tv_norm_property; > + > /** > * @tv_subconnector_property: Optional TV property to differentiate > * between different TV connector types. >
Den 18.08.2022 17.31, skrev Maxime Ripard: > On Thu, Aug 18, 2022 at 05:01:38PM +0200, Noralf Trønnes wrote: >> >> >> Den 18.08.2022 01.23, skrev Noralf Trønnes: >>> >>> >>> Den 17.08.2022 15.11, skrev Noralf Trønnes: >>>> >>>> >>>> Den 17.08.2022 13.46, skrev Maxime Ripard: >>>>> On Tue, Aug 16, 2022 at 09:35:24PM +0200, Noralf Trønnes wrote: >>>>>> Den 16.08.2022 11.49, skrev Maxime Ripard: >>>>>>> On Tue, Aug 16, 2022 at 11:42:20AM +0200, Noralf Trønnes wrote: >>>>>>>> Den 16.08.2022 10.26, skrev Maxime Ripard: >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> On Mon, Aug 08, 2022 at 02:44:56PM +0200, Noralf Trønnes wrote: >>>>>>>>>> Den 29.07.2022 18.34, 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@cerno.tech> >>>>>>>>>>> >>> >>>>>> How do you test the property? I've used modetest but I can only change >>>>>> to a tv.mode that matches the current display mode. I can't switch from >>>>>> ntsc to pal for instance. >>>>> >>>>> Yep, if you want to change from PAL to NTSC, it will require a new mode. >>>>> >>>> >>>> So userspace has to check tv.mode first and then create a display mode >>>> the driver will accept if switching to a different display mode is >>>> necessary? In other words, userspace can't discover from the kernel >>>> which display modes a certain tv.mode/norm provides before it is >>>> selected? If so, maybe libdrm should have some function(s) to deal with >>>> switching between modes that require a different display mode since >>>> knowledge about which display modes a tv.mode supports is needed before >>>> hand. >>>> >>> >>> I haven't used vc4 on Pi4 in mainline before and have finally gotten it >>> to work. >>> >>> I see that the connector reports 2 modes that together fit all tv.norms >>> so userspace doesn't have to contruct a display mode, but it does need >>> to know which display mode belongs to a certain tv.norm. >>> >>> When I try to use modetest I'm unable to set a mode: >>> >>> pi@pi4t:~ $ modetest -M vc4 -s 45:720x480i >>> setting mode 720x480i-29.97Hz on connectors 45, crtc 68 >>> failed to set mode: Function not implemented >>> >>> The errno is misleading, modetest does a drmModeDirtyFB before checking >>> the error returned by drmModeSetCrtc. >>> >>> Setting the property succeeds, but the modeset still fails: >>> >>> pi@pi4t:~ $ modetest -M vc4 -s 45:720x480i -w 45:"tv norm":2 >>> setting mode 720x480i-29.97Hz on connectors 45, crtc 68 >>> failed to set mode: Function not implemented >>> >>> pi@pi4t:~ $ modetest -M vc4 -c >>> 37 tv norm: >>> flags: bitmask >>> values: NTSC-443=0x1 NTSC-J=0x2 NTSC-M=0x4 PAL-B=0x10 >>> PAL-M=0x200 PAL-N=0x400 SECAM-B=0x2000 >>> value: 2 >>> >>> Here's the log, can you see if there's anything obvious in there: >>> https://gist.github.com/notro/a079498bf6b64327105752b2bafa8858 >>> >> >> I'm one step closer as I now have fbcon working, I had forgotten to add >> enable_tvout=1 and I had disable_fw_kms_setup=1 which disables the >> video= mode on the kernel commandline. >> >> modetest still fails though, after alot of printk sprinkling, I've >> tracked it down to the drm_mode_equal test in >> drm_atomic_helper_connector_tv_check(). The aspect ratios differ: >> >> [ 61.336295] drm_atomic_helper_connector_tv_check: >> mode->picture_aspect_ratio=1 >> [ 61.336301] drm_atomic_helper_connector_tv_check: >> &crtc_state->mode->picture_aspect_ratio=0 > > I haven't seen this when testing, but I'll have a look, thanks! I have found the cause, the kernel strips off the aspect ratio in drm_mode_getconnector() if drm_file->aspect_ratio_allowed is false. So I think the drm_mode_equal() test needs to be relaxed for legacy/non-atomic userspace to work. If I use modetest with atomic commit (-a) it works as is, having the drm_mode_equal() test: $ modetest -M vc4 -a -P 61@68:720x480 -s 45:720x480i I have a problem because the board hangs, either right away or after I press <enter> to quit modetest. I often get this, sometimes after 10s of seconds: [ 136.822963] Unhandled fault: asynchronous external abort (0x1211) at 0x00000000 ... [ 137.248496] bcm2711_get_temp [bcm2711_thermal] from thermal_zone_get_temp+0x54/0x74 Unloading bcm2711_thermal didn't help, in that case I got nothing, so the problem lies elsewhere. I have even tried with a fresh SD image and a fresh kernel, but it didn't help. I can switch from NTSC to PAL like this (but it still crashes): $ modetest -M vc4 -a -w 45:"tv norm":16 -P 61@68:720x576 -s 45:720x576i I had to patch modetest for that to work: diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c index 8ff6c80d..accd2166 100644 --- a/tests/modetest/modetest.c +++ b/tests/modetest/modetest.c @@ -2188,12 +2187,13 @@ int main(int argc, char **argv) dump_resource(&dev, planes); dump_resource(&dev, framebuffers); + if (dev.use_atomic) + dev.req = drmModeAtomicAlloc(); + for (i = 0; i < prop_count; ++i) set_property(&dev, &prop_args[i]); if (dev.use_atomic) { - dev.req = drmModeAtomicAlloc(); - if (set_preferred || (count && plane_count)) { uint64_t cap = 0; I use a composite to USB adapter to see the output: https://www.adafruit.com/product/4715 Noralf.
Hi, On Fri, Aug 19, 2022 at 11:35:42AM +0200, Geert Uytterhoeven wrote: > On Thu, Aug 18, 2022 at 5:34 PM Maxime Ripard <maxime@cerno.tech> wrote: > > On Thu, Aug 18, 2022 at 05:20:42PM +0200, Geert Uytterhoeven wrote: > > > On Thu, Aug 18, 2022 at 4:54 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > On Wed, Aug 17, 2022 at 04:04:24PM +0200, Geert Uytterhoeven wrote: > > > > > On Wed, Aug 17, 2022 at 3:19 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > On Wed, Aug 17, 2022 at 03:05:52PM +0200, Geert Uytterhoeven wrote: > > > > > > > On Wed, Aug 17, 2022 at 1:15 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > On Wed, Aug 17, 2022 at 10:35:07AM +0200, Geert Uytterhoeven wrote: > > > > > > > > > On Wed, Aug 17, 2022 at 9:47 AM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > On Wed, Aug 17, 2022 at 09:31:18AM +0200, Geert Uytterhoeven wrote: > > > > > > > > > > > On Tue, Aug 16, 2022 at 5:50 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > > > > > On Tue, Aug 16, 2022 at 04:43:44PM +0200, Geert Uytterhoeven wrote: > > > > > > > > > > > > > > > > > Either you have to add them here (e.g. "hd720p50" and "hd720p60"), or > > > > > > > > > > > > > > > > > handle them through "@<refresh>". The latter would impact "[PATCH v1 > > > > > > > > > > > > > > > > > 09/35] drm/modes: Move named modes parsing to a separate function", as > > > > > > > > > > > > > > > > > currently a named mode and a refresh rate can't be specified both. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I think the former would make more sense. It simplifies a bit the > > > > > > > > > > > > > > > > parser, and we're going to use a named mode anyway. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > As "[PATCH v1 34/35] drm/modes: Introduce the tv_mode property as a > > > > > > > > > > > > > > > > > command-line option" uses a separate "tv_mode" option, and not the main > > > > > > > > > > > > > > > > > mode name, I think you want to add them here. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > It's a separate story I think, we could have a named mode hd720p50, > > > > > > > > > > > > > > > > which would be equivalent to 1280x720,tv_mode=hd720p > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > So where's the field rate in "1280x720,tv_mode=hd720p"? > > > > > > > > > > > > > > > > > > > > > > > > > > > > Yeah, sorry I meant 1280x720@50,tv_mode=hd720p > > > > > > > > > > > > > > > > > > > > > > > > > > Above you said "I think the former would make more sense", so that > > > > > > > > > > > > > should be "1280x720,tv_mode=hd720p50"? > > > > > > > > > > > > > > > > > > > > > > > > No, 720p at 50Hz would be either hd720p50 or 1280x720@50,tv_mode=hd720p > > > > > > > > > > > > and 60Hz would be hd720p60 or 1280x720@60,tv_mode=hd720p > > > > > > > > > > > > > > > > > > > > > > I disagree: hd720p50 and hd720p60 are different TV modes. > > > > > > > > > > > > > > > > > > > > I agree, and I don't see how that command-line doesn't express that? > > > > > > > > > > > > > > > > > > Oh, I see what you mean: yes, it expresses that. > > > > > > > > > But it is inconsistent with the NTSC/PAL/SECAM/hd{480,576}[ip] modes, > > > > > > > > > where the TV mode specifies both number of lines and frame rate. > > > > > > > > > > > > > > > > Only if we're using a named mode, and naming is hard :) > > > > > > > > > > > > > > That's not true: "640x480,tv_mode=PAL-N" would give me a mode with > > > > > > > 625 lines and 25 frames/s, "640x480,tv_mode=PAL-M" would give me a > > > > > > > mode with 525 lines and 30 frames/s. > > > > > > > > > > > > In that series, "640x480,tv_mode=PAL-N" would be rejected as invalid: > > > > > > > > > > > > https://lore.kernel.org/dri-devel/20220728-rpi-analog-tv-properties-v1-14-3d53ae722097@cerno.tech/ > > > > > > > > > > It would become supported once the ideas from thread "[PATCH v1 04/35] > > > > > drm/modes: Introduce 480i and 576i modes" are implemented... > > > > > > > > Indeed, but I'm still not sure what your concern is here. > > > > "640x480,tv_mode=PAL-N" and "640x480,tv_mode=PAL-M" are both fairly > > > > obvious. > > > > > > > > You were initially saying that you had concern over the inconsistency of > > > > NTSC/PAL/SECAM where the TV mode would specify a number of lines and > > > > frame rate, but hd720p50 also specifies a number of line and frame rate? > > > > > > My concern is that you want to call the TV mode "hd720p", which > > > does not dictate the frame rate. > > > I would like to have both "720p50" and "720p60", as they do dictate > > > the frame rate, like all the non-hd modes. > > > > But they don't? > > > > The refresh rate is part of the drm_display_mode, whereas that property > > is metadata and entirely separate from the display mode. > > > > You can even change it without changing the mode at all > > Yes, the refresh rate is part of drm_display_mode. Vdisplay also > is, but that doesn't mean you can set it to e.g. 700 when using > "tv_mode=PAL-B". Some (combination of) parameters in drm_display_mode > are dictated by the tv_mode. But the opposite is also true: PAL-B and SECAM-B would be two different TV mode, but (could) have the same display mode. There's no equivalence or implication in that relationship, except for a smaller set of those parameters. But it's the entire display mode that we should compare. > Perhaps the meaning of "tv_mode" should be clarified? What does it > really mean, and what parameters does it (not) constrain? As far as I'm concerned, it's only about the encoding. We can check after the fact that, say, you don't try to use a mode line with than 525 lines and some NTSC variant, but the mode has precedence over the property. > For e.g. "PAL-B", I know it's a mode with 625 lines and 30 frames/s > (60 fields/s). > For "hd720p" I know it is an analog mode with 750 lines, but it's still > ambiguous, as I don't know if it is the variant with 60 or 50 frames/s. As far as the TV mode property is concerned, it doesn't encode neither whether it has 750 lines, nor the refresh rate. If you're talking about a named mode, then yeah, it's basically an alias for a mode + a property, so it does, but we can choose names that aren't ambiguous. Maxime
Hi, On Sat, Aug 20, 2022 at 10:12:46PM +0200, Noralf Trønnes wrote: > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > > index 1e9996b33cc8..78275e68ff66 100644 > > --- a/include/drm/drm_connector.h > > +++ b/include/drm/drm_connector.h > > @@ -143,6 +143,32 @@ enum subpixel_order { > > > > }; > > > > +#define DRM_MODE_TV_NORM_NTSC_443 (1 << 0) > > +#define DRM_MODE_TV_NORM_NTSC_J (1 << 1) > > +#define DRM_MODE_TV_NORM_NTSC_M (1 << 2) > > +#define DRM_MODE_TV_NORM_PAL_60 (1 << 3) > > +#define DRM_MODE_TV_NORM_PAL_B (1 << 4) > > +#define DRM_MODE_TV_NORM_PAL_D (1 << 5) > > +#define DRM_MODE_TV_NORM_PAL_G (1 << 6) > > +#define DRM_MODE_TV_NORM_PAL_H (1 << 7) > > +#define DRM_MODE_TV_NORM_PAL_I (1 << 8) > > +#define DRM_MODE_TV_NORM_PAL_M (1 << 9) > > +#define DRM_MODE_TV_NORM_PAL_N (1 << 10) > > +#define DRM_MODE_TV_NORM_PAL_NC (1 << 11) > > +#define DRM_MODE_TV_NORM_SECAM_60 (1 << 12) > > +#define DRM_MODE_TV_NORM_SECAM_B (1 << 13) > > +#define DRM_MODE_TV_NORM_SECAM_D (1 << 14) > > +#define DRM_MODE_TV_NORM_SECAM_G (1 << 15) > > +#define DRM_MODE_TV_NORM_SECAM_K (1 << 16) > > +#define DRM_MODE_TV_NORM_SECAM_K1 (1 << 17) > > +#define DRM_MODE_TV_NORM_SECAM_L (1 << 18) > > +#define DRM_MODE_TV_NORM_HD480I (1 << 19) > > +#define DRM_MODE_TV_NORM_HD480P (1 << 20) > > +#define DRM_MODE_TV_NORM_HD576I (1 << 21) > > +#define DRM_MODE_TV_NORM_HD576P (1 << 22) > > +#define DRM_MODE_TV_NORM_HD720P (1 << 23) > > +#define DRM_MODE_TV_NORM_HD1080I (1 << 24) > > + > > This is an area where DRM overlaps with v4l2, see: > - include/dt-bindings/display/sdtv-standards.h > - v4l2_norm_to_name() > > Maybe we should follow suit, but if we do our own thing please mention > why in the commit message. Are you suggesting that we'd share that definition with v4l2? I've tried to share some code in the past between v4l2 and DRM, and it got completely shut down so it's not something I'd like to try again, if possible. Maxime
Den 25.08.2022 15.44, skrev Maxime Ripard: > Hi, > > On Sat, Aug 20, 2022 at 10:12:46PM +0200, Noralf Trønnes wrote: >>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h >>> index 1e9996b33cc8..78275e68ff66 100644 >>> --- a/include/drm/drm_connector.h >>> +++ b/include/drm/drm_connector.h >>> @@ -143,6 +143,32 @@ enum subpixel_order { >>> >>> }; >>> >>> +#define DRM_MODE_TV_NORM_NTSC_443 (1 << 0) >>> +#define DRM_MODE_TV_NORM_NTSC_J (1 << 1) >>> +#define DRM_MODE_TV_NORM_NTSC_M (1 << 2) >>> +#define DRM_MODE_TV_NORM_PAL_60 (1 << 3) >>> +#define DRM_MODE_TV_NORM_PAL_B (1 << 4) >>> +#define DRM_MODE_TV_NORM_PAL_D (1 << 5) >>> +#define DRM_MODE_TV_NORM_PAL_G (1 << 6) >>> +#define DRM_MODE_TV_NORM_PAL_H (1 << 7) >>> +#define DRM_MODE_TV_NORM_PAL_I (1 << 8) >>> +#define DRM_MODE_TV_NORM_PAL_M (1 << 9) >>> +#define DRM_MODE_TV_NORM_PAL_N (1 << 10) >>> +#define DRM_MODE_TV_NORM_PAL_NC (1 << 11) >>> +#define DRM_MODE_TV_NORM_SECAM_60 (1 << 12) >>> +#define DRM_MODE_TV_NORM_SECAM_B (1 << 13) >>> +#define DRM_MODE_TV_NORM_SECAM_D (1 << 14) >>> +#define DRM_MODE_TV_NORM_SECAM_G (1 << 15) >>> +#define DRM_MODE_TV_NORM_SECAM_K (1 << 16) >>> +#define DRM_MODE_TV_NORM_SECAM_K1 (1 << 17) >>> +#define DRM_MODE_TV_NORM_SECAM_L (1 << 18) >>> +#define DRM_MODE_TV_NORM_HD480I (1 << 19) >>> +#define DRM_MODE_TV_NORM_HD480P (1 << 20) >>> +#define DRM_MODE_TV_NORM_HD576I (1 << 21) >>> +#define DRM_MODE_TV_NORM_HD576P (1 << 22) >>> +#define DRM_MODE_TV_NORM_HD720P (1 << 23) >>> +#define DRM_MODE_TV_NORM_HD1080I (1 << 24) >>> + >> >> This is an area where DRM overlaps with v4l2, see: >> - include/dt-bindings/display/sdtv-standards.h >> - v4l2_norm_to_name() >> >> Maybe we should follow suit, but if we do our own thing please mention >> why in the commit message. > > Are you suggesting that we'd share that definition with v4l2? > If possible, yes. > I've tried to share some code in the past between v4l2 and DRM, and it > got completely shut down so it's not something I'd like to try again, if > possible. > But that is a good enough reason not to do so. I just got the impression from some of Laurent's emails a while back that there was some cooperativ atmosphere, but I might be mistaken in my reading/understanding. It is ofc possible to just copy the values from sdtv-standards.h, but I see that hd* is missing from that list, so not sure if there's much point if it has to be extended without changing the source. We can at least use the names from v4l2_norm_to_name(), but from a quick look it seems you already do so. Noralf.
On Sun, Aug 21, 2022 at 01:43:40PM +0200, Noralf Trønnes wrote: > > > Den 18.08.2022 17.31, skrev Maxime Ripard: > > On Thu, Aug 18, 2022 at 05:01:38PM +0200, Noralf Trønnes wrote: > >> > >> > >> Den 18.08.2022 01.23, skrev Noralf Trønnes: > >>> > >>> > >>> Den 17.08.2022 15.11, skrev Noralf Trønnes: > >>>> > >>>> > >>>> Den 17.08.2022 13.46, skrev Maxime Ripard: > >>>>> On Tue, Aug 16, 2022 at 09:35:24PM +0200, Noralf Trønnes wrote: > >>>>>> Den 16.08.2022 11.49, skrev Maxime Ripard: > >>>>>>> On Tue, Aug 16, 2022 at 11:42:20AM +0200, Noralf Trønnes wrote: > >>>>>>>> Den 16.08.2022 10.26, skrev Maxime Ripard: > >>>>>>>>> Hi, > >>>>>>>>> > >>>>>>>>> On Mon, Aug 08, 2022 at 02:44:56PM +0200, Noralf Trønnes wrote: > >>>>>>>>>> Den 29.07.2022 18.34, 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@cerno.tech> > >>>>>>>>>>> > >>> > >>>>>> How do you test the property? I've used modetest but I can only change > >>>>>> to a tv.mode that matches the current display mode. I can't switch from > >>>>>> ntsc to pal for instance. > >>>>> > >>>>> Yep, if you want to change from PAL to NTSC, it will require a new mode. > >>>>> > >>>> > >>>> So userspace has to check tv.mode first and then create a display mode > >>>> the driver will accept if switching to a different display mode is > >>>> necessary? In other words, userspace can't discover from the kernel > >>>> which display modes a certain tv.mode/norm provides before it is > >>>> selected? If so, maybe libdrm should have some function(s) to deal with > >>>> switching between modes that require a different display mode since > >>>> knowledge about which display modes a tv.mode supports is needed before > >>>> hand. > >>>> > >>> > >>> I haven't used vc4 on Pi4 in mainline before and have finally gotten it > >>> to work. > >>> > >>> I see that the connector reports 2 modes that together fit all tv.norms > >>> so userspace doesn't have to contruct a display mode, but it does need > >>> to know which display mode belongs to a certain tv.norm. > >>> > >>> When I try to use modetest I'm unable to set a mode: > >>> > >>> pi@pi4t:~ $ modetest -M vc4 -s 45:720x480i > >>> setting mode 720x480i-29.97Hz on connectors 45, crtc 68 > >>> failed to set mode: Function not implemented > >>> > >>> The errno is misleading, modetest does a drmModeDirtyFB before checking > >>> the error returned by drmModeSetCrtc. > >>> > >>> Setting the property succeeds, but the modeset still fails: > >>> > >>> pi@pi4t:~ $ modetest -M vc4 -s 45:720x480i -w 45:"tv norm":2 > >>> setting mode 720x480i-29.97Hz on connectors 45, crtc 68 > >>> failed to set mode: Function not implemented > >>> > >>> pi@pi4t:~ $ modetest -M vc4 -c > >>> 37 tv norm: > >>> flags: bitmask > >>> values: NTSC-443=0x1 NTSC-J=0x2 NTSC-M=0x4 PAL-B=0x10 > >>> PAL-M=0x200 PAL-N=0x400 SECAM-B=0x2000 > >>> value: 2 > >>> > >>> Here's the log, can you see if there's anything obvious in there: > >>> https://gist.github.com/notro/a079498bf6b64327105752b2bafa8858 > >>> > >> > >> I'm one step closer as I now have fbcon working, I had forgotten to add > >> enable_tvout=1 and I had disable_fw_kms_setup=1 which disables the > >> video= mode on the kernel commandline. > >> > >> modetest still fails though, after alot of printk sprinkling, I've > >> tracked it down to the drm_mode_equal test in > >> drm_atomic_helper_connector_tv_check(). The aspect ratios differ: > >> > >> [ 61.336295] drm_atomic_helper_connector_tv_check: > >> mode->picture_aspect_ratio=1 > >> [ 61.336301] drm_atomic_helper_connector_tv_check: > >> &crtc_state->mode->picture_aspect_ratio=0 > > > > I haven't seen this when testing, but I'll have a look, thanks! > > I have found the cause, the kernel strips off the aspect ratio in > drm_mode_getconnector() if drm_file->aspect_ratio_allowed is false. So I > think the drm_mode_equal() test needs to be relaxed for > legacy/non-atomic userspace to work. Geert suggested I removed it too, so this check is gone now. > If I use modetest with atomic commit (-a) it works as is, having the > drm_mode_equal() test: > > $ modetest -M vc4 -a -P 61@68:720x480 -s 45:720x480i > > I have a problem because the board hangs, either right away or after I > press <enter> to quit modetest. > > I often get this, sometimes after 10s of seconds: > > [ 136.822963] Unhandled fault: asynchronous external abort (0x1211) at > 0x00000000 > ... > [ 137.248496] bcm2711_get_temp [bcm2711_thermal] from > thermal_zone_get_temp+0x54/0x74 > > Unloading bcm2711_thermal didn't help, in that case I got nothing, so > the problem lies elsewhere. > I have even tried with a fresh SD image and a fresh kernel, but it > didn't help. I got this too when working from current next (next-20220825) but it doesn't seem to happen on drm-misc-next-2022-08-20-1, so it's probably something unrelated? > I can switch from NTSC to PAL like this (but it still crashes): > > $ modetest -M vc4 -a -w 45:"tv norm":16 -P 61@68:720x576 -s 45:720x576i I've tested with my current branch, and it work fine with modetest, I'll put my commands in my next iteration so that you can compare? Maxime
On Thu, Aug 25, 2022 at 05:13:29PM +0200, Noralf Trønnes wrote: > > > Den 25.08.2022 15.44, skrev Maxime Ripard: > > Hi, > > > > On Sat, Aug 20, 2022 at 10:12:46PM +0200, Noralf Trønnes wrote: > >>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > >>> index 1e9996b33cc8..78275e68ff66 100644 > >>> --- a/include/drm/drm_connector.h > >>> +++ b/include/drm/drm_connector.h > >>> @@ -143,6 +143,32 @@ enum subpixel_order { > >>> > >>> }; > >>> > >>> +#define DRM_MODE_TV_NORM_NTSC_443 (1 << 0) > >>> +#define DRM_MODE_TV_NORM_NTSC_J (1 << 1) > >>> +#define DRM_MODE_TV_NORM_NTSC_M (1 << 2) > >>> +#define DRM_MODE_TV_NORM_PAL_60 (1 << 3) > >>> +#define DRM_MODE_TV_NORM_PAL_B (1 << 4) > >>> +#define DRM_MODE_TV_NORM_PAL_D (1 << 5) > >>> +#define DRM_MODE_TV_NORM_PAL_G (1 << 6) > >>> +#define DRM_MODE_TV_NORM_PAL_H (1 << 7) > >>> +#define DRM_MODE_TV_NORM_PAL_I (1 << 8) > >>> +#define DRM_MODE_TV_NORM_PAL_M (1 << 9) > >>> +#define DRM_MODE_TV_NORM_PAL_N (1 << 10) > >>> +#define DRM_MODE_TV_NORM_PAL_NC (1 << 11) > >>> +#define DRM_MODE_TV_NORM_SECAM_60 (1 << 12) > >>> +#define DRM_MODE_TV_NORM_SECAM_B (1 << 13) > >>> +#define DRM_MODE_TV_NORM_SECAM_D (1 << 14) > >>> +#define DRM_MODE_TV_NORM_SECAM_G (1 << 15) > >>> +#define DRM_MODE_TV_NORM_SECAM_K (1 << 16) > >>> +#define DRM_MODE_TV_NORM_SECAM_K1 (1 << 17) > >>> +#define DRM_MODE_TV_NORM_SECAM_L (1 << 18) > >>> +#define DRM_MODE_TV_NORM_HD480I (1 << 19) > >>> +#define DRM_MODE_TV_NORM_HD480P (1 << 20) > >>> +#define DRM_MODE_TV_NORM_HD576I (1 << 21) > >>> +#define DRM_MODE_TV_NORM_HD576P (1 << 22) > >>> +#define DRM_MODE_TV_NORM_HD720P (1 << 23) > >>> +#define DRM_MODE_TV_NORM_HD1080I (1 << 24) > >>> + > >> > >> This is an area where DRM overlaps with v4l2, see: > >> - include/dt-bindings/display/sdtv-standards.h > >> - v4l2_norm_to_name() > >> > >> Maybe we should follow suit, but if we do our own thing please mention > >> why in the commit message. > > > > Are you suggesting that we'd share that definition with v4l2? > > > > If possible, yes. > > > I've tried to share some code in the past between v4l2 and DRM, and it > > got completely shut down so it's not something I'd like to try again, if > > possible. > > > > But that is a good enough reason not to do so. I just got the impression > from some of Laurent's emails a while back that there was some > cooperativ atmosphere, but I might be mistaken in my reading/understanding. Here's the original thread: https://lore.kernel.org/lkml/cover.8ec406bf8f4f097e9dc909d5aac466556822f592.1555487650.git-series.maxime.ripard@bootlin.com/ It ended up stalling completely, without any will from either DRM or v4l2 to get this through. So I will not work on anything like that until both maintainership teams have expressed that it's something they actually want. > It is ofc possible to just copy the values from sdtv-standards.h, but I > see that hd* is missing from that list, so not sure if there's much > point if it has to be extended without changing the source. HD formats were dropped, so it's not a big deal. However, we are missing a few formats, but that were never used by either nouveau, i915 or any other driver. I'm not sure it's worth adding at that point, and we can always extend it later. Maxime
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index c06d0639d552..d7ff6c644c2f 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->tv_mode_property) { state->tv.mode = val; + } else if (property == config->tv_norm_property) { + state->tv.norm = 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->tv_mode_property) { *val = state->tv.mode; + } else if (property == config->tv_norm_property) { + *val = state->tv.norm; } 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 e3142c8142b3..68a4e47f85a9 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1637,6 +1637,7 @@ EXPORT_SYMBOL(drm_mode_create_tv_margin_properties); /** * drm_mode_create_tv_properties - create TV specific connector properties * @dev: DRM device + * @supported_tv_norms: Bitmask of TV norms supported (See DRM_MODE_TV_NORM_*) * @num_modes: number of different TV formats (modes) supported * @modes: array of pointers to strings containing name of each format * @@ -1649,11 +1650,40 @@ EXPORT_SYMBOL(drm_mode_create_tv_margin_properties); * 0 on success or a negative error code on failure. */ int drm_mode_create_tv_properties(struct drm_device *dev, + unsigned int supported_tv_norms, unsigned int num_modes, const char * const modes[]) { + static const struct drm_prop_enum_list tv_norm_values[] = { + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_443) - 1, "NTSC-443" }, + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_J) - 1, "NTSC-J" }, + { __builtin_ffs(DRM_MODE_TV_NORM_NTSC_M) - 1, "NTSC-M" }, + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_60) - 1, "PAL-60" }, + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_B) - 1, "PAL-B" }, + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_D) - 1, "PAL-D" }, + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_G) - 1, "PAL-G" }, + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_H) - 1, "PAL-H" }, + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_I) - 1, "PAL-I" }, + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_M) - 1, "PAL-M" }, + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_N) - 1, "PAL-N" }, + { __builtin_ffs(DRM_MODE_TV_NORM_PAL_NC) - 1, "PAL-Nc" }, + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_60) - 1, "SECAM-60" }, + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_B) - 1, "SECAM-B" }, + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_D) - 1, "SECAM-D" }, + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_G) - 1, "SECAM-G" }, + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_K) - 1, "SECAM-K" }, + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_K1) - 1, "SECAM-K1" }, + { __builtin_ffs(DRM_MODE_TV_NORM_SECAM_L) - 1, "SECAM-L" }, + { __builtin_ffs(DRM_MODE_TV_NORM_HD480I) - 1, "hd480i" }, + { __builtin_ffs(DRM_MODE_TV_NORM_HD480P) - 1, "hd480p" }, + { __builtin_ffs(DRM_MODE_TV_NORM_HD576I) - 1, "hd576i" }, + { __builtin_ffs(DRM_MODE_TV_NORM_HD576P) - 1, "hd576p" }, + { __builtin_ffs(DRM_MODE_TV_NORM_HD720P) - 1, "hd720p" }, + { __builtin_ffs(DRM_MODE_TV_NORM_HD1080I) - 1, "hd1080i" }, + }; struct drm_property *tv_selector; struct drm_property *tv_subconnector; + struct drm_property *tv_norm; unsigned int i; if (dev->mode_config.tv_select_subconnector_property) @@ -1686,6 +1716,13 @@ int drm_mode_create_tv_properties(struct drm_device *dev, if (drm_mode_create_tv_margin_properties(dev)) goto nomem; + tv_norm = drm_property_create_bitmask(dev, 0, "tv norm", + tv_norm_values, ARRAY_SIZE(tv_norm_values), + supported_tv_norms); + if (!tv_norm) + goto nomem; + dev->mode_config.tv_norm_property = tv_norm; + dev->mode_config.tv_mode_property = drm_property_create(dev, DRM_MODE_PROP_ENUM, "mode", num_modes); diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c index 4a788c1c9058..457529e5d857 100644 --- a/drivers/gpu/drm/vc4/vc4_vec.c +++ b/drivers/gpu/drm/vc4/vc4_vec.c @@ -573,7 +573,9 @@ static int vc4_vec_bind(struct device *dev, struct device *master, void *data) struct vc4_vec *vec; int ret; - ret = drm_mode_create_tv_properties(drm, ARRAY_SIZE(tv_mode_names), + ret = drm_mode_create_tv_properties(drm, + 0, + ARRAY_SIZE(tv_mode_names), tv_mode_names); if (ret) return ret; diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 1e9996b33cc8..78275e68ff66 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -143,6 +143,32 @@ enum subpixel_order { }; +#define DRM_MODE_TV_NORM_NTSC_443 (1 << 0) +#define DRM_MODE_TV_NORM_NTSC_J (1 << 1) +#define DRM_MODE_TV_NORM_NTSC_M (1 << 2) +#define DRM_MODE_TV_NORM_PAL_60 (1 << 3) +#define DRM_MODE_TV_NORM_PAL_B (1 << 4) +#define DRM_MODE_TV_NORM_PAL_D (1 << 5) +#define DRM_MODE_TV_NORM_PAL_G (1 << 6) +#define DRM_MODE_TV_NORM_PAL_H (1 << 7) +#define DRM_MODE_TV_NORM_PAL_I (1 << 8) +#define DRM_MODE_TV_NORM_PAL_M (1 << 9) +#define DRM_MODE_TV_NORM_PAL_N (1 << 10) +#define DRM_MODE_TV_NORM_PAL_NC (1 << 11) +#define DRM_MODE_TV_NORM_SECAM_60 (1 << 12) +#define DRM_MODE_TV_NORM_SECAM_B (1 << 13) +#define DRM_MODE_TV_NORM_SECAM_D (1 << 14) +#define DRM_MODE_TV_NORM_SECAM_G (1 << 15) +#define DRM_MODE_TV_NORM_SECAM_K (1 << 16) +#define DRM_MODE_TV_NORM_SECAM_K1 (1 << 17) +#define DRM_MODE_TV_NORM_SECAM_L (1 << 18) +#define DRM_MODE_TV_NORM_HD480I (1 << 19) +#define DRM_MODE_TV_NORM_HD480P (1 << 20) +#define DRM_MODE_TV_NORM_HD576I (1 << 21) +#define DRM_MODE_TV_NORM_HD576P (1 << 22) +#define DRM_MODE_TV_NORM_HD720P (1 << 23) +#define DRM_MODE_TV_NORM_HD1080I (1 << 24) + /** * struct drm_scrambling: sink's scrambling support. */ @@ -687,6 +713,7 @@ struct drm_tv_connector_state { enum drm_mode_subconnector subconnector; struct drm_connector_tv_margins margins; unsigned int mode; + unsigned int norm; unsigned int brightness; unsigned int contrast; unsigned int flicker_reduction; @@ -1779,6 +1806,7 @@ void drm_connector_attach_dp_subconnector_property(struct drm_connector *connect int drm_mode_create_tv_margin_properties(struct drm_device *dev); int drm_mode_create_tv_properties(struct drm_device *dev, + unsigned int supported_tv_norms, unsigned int num_modes, const char * const modes[]); void drm_connector_attach_tv_margin_properties(struct drm_connector *conn); diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index 6b5e01295348..d9e79def8b92 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -704,6 +704,12 @@ struct drm_mode_config { */ struct drm_property *dp_subconnector_property; + /** + * @tv_norm_property: Optional TV property to select the TV + * standard output on the connector. + */ + struct drm_property *tv_norm_property; + /** * @tv_subconnector_property: Optional TV property to differentiate * between different TV connector types.
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@cerno.tech>