Re: [PATCH v1 05/35] drm/connector: Add TV standard property

From: Maxime Ripard <maxime@cerno.tech>
To: "Noralf Trønnes" <noralf@tronnes.org>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>,
	Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
	Chen-Yu Tsai <wens@csie.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Jerome Brunet <jbrunet@baylibre.com>,
	Samuel Holland <samuel@sholland.org>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Daniel Vetter <daniel@ffwll.ch>, Emma Anholt <emma@anholt.net>,
	David Airlie <airlied@linux.ie>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Kevin Hilman <khilman@baylibre.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org,
	Phil Elwell <phil@raspberrypi.com>,
	Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>,
	linux-arm-kernel@lists.infradead.org,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	Dave Stevenson <dave.stevenson@raspberrypi.com>,
	linux-amlogic@lists.infradead.org,
	dri-devel@lists.freedesktop.org, Dom Cobley <dom@raspberrypi.com>
Subject: Re: [PATCH v1 05/35] drm/connector: Add TV standard property
Date: Tue, 16 Aug 2022 10:26:12 +0200	[thread overview]
Message-ID: <20220816082612.grebxql5ynnfnvfd@houat> (raw)
In-Reply-To: <9fdecae2-80ad-6212-0522-7dccf9fb57be@tronnes.org>

[-- Attachment #1: Type: text/plain, Size: 6881 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]