Noralf Trønnes
2022-Sep-24 15:52 UTC
[Nouveau] [PATCH v2 09/33] drm/connector: Add TV standard property
Den 22.09.2022 16.25, skrev Maxime Ripard:> The TV mode property has been around for a while now to select and get the > current TV mode output on an analog TV connector. > > Despite that property name being generic, its content isn't and has been > driver-specific which makes it hard to build any generic behaviour on top > of it, both in kernel and user-space. > > Let's create a new enum tv norm property, that can contain any of the > analog TV standards currently supported by kernel drivers. Each driver can > then pass in a bitmask of the modes it supports, and the property > creation function will filter out the modes not supported. > > We'll then be able to phase out the older tv mode property. > > Signed-off-by: Maxime Ripard <maxime at cerno.tech> >Please can you add per patch changelogs, it's hard to review when I have to recall what might have happened with each patch. If you do it drm style and put in the commit message it should be easy enough to do.> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index 4e4fbc9e0049..e7aa8de08f5b 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -980,6 +980,18 @@ static const struct drm_prop_enum_list drm_dvi_i_subconnector_enum_list[] = { > DRM_ENUM_NAME_FN(drm_get_dvi_i_subconnector_name, > drm_dvi_i_subconnector_enum_list) > > +static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = { > + { DRM_MODE_TV_MODE_NONE, "None" }, > + { DRM_MODE_TV_MODE_NTSC, "NTSC" },I think going back to plain NTSC and PAL is a good choice for the common variants.> + { DRM_MODE_TV_MODE_NTSC_443, "NTSC-443" }, > + { DRM_MODE_TV_MODE_NTSC_J, "NTSC-J" }, > + { DRM_MODE_TV_MODE_PAL, "PAL" }, > + { DRM_MODE_TV_MODE_PAL_M, "PAL-M" }, > + { DRM_MODE_TV_MODE_PAL_N, "PAL-N" }, > + { DRM_MODE_TV_MODE_SECAM, "SECAM" }, > +}; > +DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list) > + > static const struct drm_prop_enum_list drm_tv_select_enum_list[] = { > { DRM_MODE_SUBCONNECTOR_Automatic, "Automatic" }, /* DVI-I and TV-out */ > { DRM_MODE_SUBCONNECTOR_Composite, "Composite" }, /* TV-out */ > @@ -1645,6 +1657,10 @@ EXPORT_SYMBOL(drm_mode_create_tv_margin_properties); > * responsible for allocating a list of format names and passing them to > * this routine. > * > + * NOTE: This functions registers the deprecated "mode" connector > + * property to select the analog TV mode (ie, NTSC, PAL, etc.). New > + * drivers must use drm_mode_create_tv_properties() instead. > + * > * Returns: > * 0 on success or a negative error code on failure. > */ > @@ -1686,7 +1702,6 @@ int drm_mode_create_tv_properties_legacy(struct drm_device *dev, > if (drm_mode_create_tv_margin_properties(dev)) > goto nomem; > > - > if (num_modes) { > dev->mode_config.legacy_tv_mode_property > drm_property_create(dev, DRM_MODE_PROP_ENUM, > @@ -1735,6 +1750,49 @@ int drm_mode_create_tv_properties_legacy(struct drm_device *dev, > } > EXPORT_SYMBOL(drm_mode_create_tv_properties_legacy); > > +/** > + * drm_mode_create_tv_properties - create TV specific connector properties > + * @dev: DRM device > + * @supported_tv_modes: Bitmask of TV modes supported (See DRM_MODE_TV_MODE_*) > + > + * Called by a driver's TV initialization routine, this function creates > + * the TV specific connector properties for a given device. Caller is > + * responsible for allocating a list of format names and passing them to > + * this routine.Copy-paste error, there are no format names in this version.> + * > + * Returns: > + * 0 on success or a negative error code on failure. > + */ > +int drm_mode_create_tv_properties(struct drm_device *dev, > + unsigned int supported_tv_modes) > +{ > + struct drm_prop_enum_list tv_mode_list[DRM_MODE_TV_MODE_MAX]; > + struct drm_property *tv_mode; > + unsigned int i, len = 0; > + > + if (dev->mode_config.tv_mode_property) > + return 0; > + > + for (i = 0; i < DRM_MODE_TV_MODE_MAX; i++) { > + if (!(supported_tv_modes & BIT(i))) > + continue; > + > + tv_mode_list[len].type = i; > + tv_mode_list[len].name = drm_get_tv_mode_name(i); > + len++; > + } > + > + tv_mode = drm_property_create_enum(dev, 0, "TV mode", > + tv_mode_list, len); > + if (!tv_mode) > + return -ENOMEM; > + > + dev->mode_config.tv_mode_property = tv_mode; > + > + return drm_mode_create_tv_properties_legacy(dev, 0, NULL); > +} > +EXPORT_SYMBOL(drm_mode_create_tv_properties); > + > /** > * drm_mode_create_scaling_mode_property - create scaling mode property > * @dev: DRM device > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index d566b4a4709c..fffacbfd0a45 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -143,6 +143,71 @@ enum subpixel_order { > > }; > > +/** > + * enum drm_connector_tv_mode - Analog TV output mode > + * > + * This enum is used to indicate the TV output mode used on an analog TV > + * connector. > + * > + * WARNING: The values of this enum is uABI since they're exposed in the > + * "TV mode" connector property. > + */ > +enum drm_connector_tv_mode { > + /** > + * @DRM_MODE_TV_MODE_NONE: Placeholder to not default on one > + * variant or the other when nothing is set. > + */ > + DRM_MODE_TV_MODE_NONE = 0,How is this supposed to be used? Noralf.> + > + /** > + * @DRM_MODE_TV_MODE_NTSC: CCIR System M (aka 525-lines) > + * together with the NTSC Color Encoding. > + */ > + DRM_MODE_TV_MODE_NTSC, > + > + /** > + * @DRM_MODE_TV_MODE_NTSC_443: Variant of > + * @DRM_MODE_TV_MODE_NTSC. Uses a color subcarrier frequency > + * of 4.43 MHz. > + */ > + DRM_MODE_TV_MODE_NTSC_443, > + > + /** > + * @DRM_MODE_TV_MODE_NTSC_J: Variant of @DRM_MODE_TV_MODE_NTSC > + * used in Japan. Uses a black level equals to the blanking > + * level. > + */ > + DRM_MODE_TV_MODE_NTSC_J, > + > + /** > + * @DRM_MODE_TV_MODE_PAL: CCIR System B together with the PAL > + * color system. > + */ > + DRM_MODE_TV_MODE_PAL, > + > + /** > + * @DRM_MODE_TV_MODE_PAL_M: CCIR System M (aka 525-lines) > + * together with the PAL color encoding > + */ > + DRM_MODE_TV_MODE_PAL_M, > + > + /** > + * @DRM_MODE_TV_MODE_PAL_N: CCIR System N together with the PAL > + * color encoding. It uses 625 lines, but has a color subcarrier > + * frequency of 3.58MHz, the SECAM color space, and narrower > + * channels compared to most of the other PAL variants. > + */ > + DRM_MODE_TV_MODE_PAL_N, > + > + /** > + * @DRM_MODE_TV_MODE_SECAM: CCIR System B together with the > + * SECAM color system. > + */ > + DRM_MODE_TV_MODE_SECAM, > + > + DRM_MODE_TV_MODE_MAX, > +}; > + > /** > * struct drm_scrambling: sink's scrambling support. > */ > @@ -696,6 +761,7 @@ struct drm_connector_tv_margins { > * @subconnector: detected subconnector > * @margins: TV margins > * @legacy_mode: Legacy TV mode, driver specific value > + * @mode: TV mode > * @brightness: brightness in percent > * @contrast: contrast in percent > * @flicker_reduction: flicker reduction in percent > @@ -708,6 +774,7 @@ struct drm_tv_connector_state { > enum drm_mode_subconnector subconnector; > struct drm_connector_tv_margins margins; > unsigned int legacy_mode; > + unsigned int mode; > unsigned int brightness; > unsigned int contrast; > unsigned int flicker_reduction; > @@ -1789,6 +1856,7 @@ const char *drm_get_subpixel_order_name(enum subpixel_order order); > const char *drm_get_dpms_name(int val); > const char *drm_get_dvi_i_subconnector_name(int val); > const char *drm_get_dvi_i_select_name(int val); > +const char *drm_get_tv_mode_name(int val); > const char *drm_get_tv_subconnector_name(int val); > const char *drm_get_tv_select_name(int val); > const char *drm_get_dp_subconnector_name(int val); > @@ -1802,6 +1870,8 @@ int drm_mode_create_tv_margin_properties(struct drm_device *dev); > int drm_mode_create_tv_properties_legacy(struct drm_device *dev, > unsigned int num_modes, > const char * const modes[]); > +int drm_mode_create_tv_properties(struct drm_device *dev, > + unsigned int supported_tv_modes); > void drm_connector_attach_tv_margin_properties(struct drm_connector *conn); > int drm_mode_create_scaling_mode_property(struct drm_device *dev); > int drm_connector_attach_content_type_property(struct drm_connector *dev); > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > index 35a827175c24..10a6f7d1df0d 100644 > --- a/include/drm/drm_mode_config.h > +++ b/include/drm/drm_mode_config.h > @@ -718,9 +718,17 @@ struct drm_mode_config { > /** > * @legacy_tv_mode_property: Optional TV property to select > * the output TV mode. > + * > + * Superseeded by @tv_mode_property > */ > struct drm_property *legacy_tv_mode_property; > > + /** > + * @tv_mode_property: Optional TV property to select the TV > + * standard output on the connector. > + */ > + struct drm_property *tv_mode_property; > + > /** > * @tv_left_margin_property: Optional TV property to set the left > * margin (expressed in pixels). >
Maxime Ripard
2022-Sep-26 10:01 UTC
[Nouveau] [PATCH v2 09/33] drm/connector: Add TV standard property
On Sat, Sep 24, 2022 at 05:52:29PM +0200, Noralf Tr?nnes wrote:> Den 22.09.2022 16.25, skrev Maxime Ripard: > > The TV mode property has been around for a while now to select and get the > > current TV mode output on an analog TV connector. > > > > Despite that property name being generic, its content isn't and has been > > driver-specific which makes it hard to build any generic behaviour on top > > of it, both in kernel and user-space. > > > > Let's create a new enum tv norm property, that can contain any of the > > analog TV standards currently supported by kernel drivers. Each driver can > > then pass in a bitmask of the modes it supports, and the property > > creation function will filter out the modes not supported. > > > > We'll then be able to phase out the older tv mode property. > > > > Signed-off-by: Maxime Ripard <maxime at cerno.tech> > > Please can you add per patch changelogs, it's hard to review when I have > to recall what might have happened with each patch. If you do it drm > style and put in the commit message it should be easy enough to do.I certainly don't want to start that discussion, but I'm really not a fan of that format either. I'll do it for that series if you prefer.> > +/** > > + * enum drm_connector_tv_mode - Analog TV output mode > > + * > > + * This enum is used to indicate the TV output mode used on an analog TV > > + * connector. > > + * > > + * WARNING: The values of this enum is uABI since they're exposed in the > > + * "TV mode" connector property. > > + */ > > +enum drm_connector_tv_mode { > > + /** > > + * @DRM_MODE_TV_MODE_NONE: Placeholder to not default on one > > + * variant or the other when nothing is set. > > + */ > > + DRM_MODE_TV_MODE_NONE = 0, > > How is this supposed to be used?It's not supposed to be used. It was a suggestion from Mateusz to avoid to default to any standard when we don't initialize something. I don't have any strong feeling about it, so I can drop it if you prefer. Maxime -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 228 bytes Desc: not available URL: <https://lists.freedesktop.org/archives/nouveau/attachments/20220926/3e3ea74a/attachment.sig>