Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i8771445c:Fastmail
Date:   Tue, 16 Aug 2022 10:26:12 +0200
From:   Maxime Ripard <maxime@cerno.tech>
To:     Noralf =?utf-8?Q?Tr=C3=B8nnes?= <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
Message-ID: <20220816082612.grebxql5ynnfnvfd@houat>
References: <20220728-rpi-analog-tv-properties-v1-0-3d53ae722097@cerno.tech>
 <20220728-rpi-analog-tv-properties-v1-5-3d53ae722097@cerno.tech>
 <9fdecae2-80ad-6212-0522-7dccf9fb57be@tronnes.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="3hhom4iyskwekp7q"
Content-Disposition: inline
In-Reply-To: <9fdecae2-80ad-6212-0522-7dccf9fb57be@tronnes.org>
Precedence: bulk


--3hhom4iyskwekp7q
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hi,

On Mon, Aug 08, 2022 at 02:44:56PM +0200, Noralf Tr=F8nnes 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.
> >=20
> > 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 t=
op
> > of it, both in kernel and user-space.
> >=20
> > 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.
> >=20
> > We'll then be able to phase out the older tv mode property.
> >=20
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> >=20
>=20
> Please also update Documentation/gpu/kms-properties.csv
>=20
> 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_at=
omic_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 =3D val;
> >  	} else if (property =3D=3D config->tv_mode_property) {
> >  		state->tv.mode =3D val;
> > +	} else if (property =3D=3D config->tv_norm_property) {
> > +		state->tv.norm =3D val;
> >  	} else if (property =3D=3D config->tv_brightness_property) {
> >  		state->tv.brightness =3D val;
> >  	} else if (property =3D=3D config->tv_contrast_property) {
> > @@ -810,6 +812,8 @@ drm_atomic_connector_get_property(struct drm_connec=
tor *connector,
> >  		*val =3D state->tv.margins.bottom;
> >  	} else if (property =3D=3D config->tv_mode_property) {
> >  		*val =3D state->tv.mode;
> > +	} else if (property =3D=3D config->tv_norm_property) {
> > +		*val =3D state->tv.norm;
> >  	} else if (property =3D=3D config->tv_brightness_property) {
> >  		*val =3D state->tv.brightness;
> >  	} else if (property =3D=3D config->tv_contrast_property) {
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_conn=
ector.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_propertie=
s);
> >  /**
> >   * drm_mode_create_tv_properties - create TV specific connector proper=
ties
> >   * @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_propert=
ies);
> >   * 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[] =3D {
> > +		{ __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;
> > =20
> >  	if (dev->mode_config.tv_select_subconnector_property)
> > @@ -1686,6 +1716,13 @@ int drm_mode_create_tv_properties(struct drm_dev=
ice *dev,
> >  	if (drm_mode_create_tv_margin_properties(dev))
> >  		goto nomem;
> > =20
> > +	tv_norm =3D drm_property_create_bitmask(dev, 0, "tv norm",
> > +					   tv_norm_values, ARRAY_SIZE(tv_norm_values),
> > +					   supported_tv_norms);
>=20
> 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

--3hhom4iyskwekp7q
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCYvtUpAAKCRDj7w1vZxhR
xZfoAQC88sDanQdYkKbOW7CFB7CVfnGcKpWIJXqulm0ThhwhLAD+Ow27epX78Lu3
zhK/iVL50Tvq00zxu4GIvhL7L791JQA=
=cQh2
-----END PGP SIGNATURE-----

--3hhom4iyskwekp7q--