Nouveau - Oct 2022 - [PATCH v6 16/23] drm/probe-helper: Provide a TV get_modes helper

Most of the TV connectors will need a similar get_modes implementation
that will, depending on the drivers' capabilities, register the 480i and
576i modes.

That implementation will also need to set the preferred flag and order
the modes based on the driver and users preferrence.

This is especially important to guarantee that a userspace stack such as
Xorg can start and pick up the preferred mode while maintaining a
working output.

Signed-off-by: Maxime Ripard <maxime at cerno.tech>

---
Changes in v6:
- New patch
---
 drivers/gpu/drm/drm_probe_helper.c | 97 ++++++++++++++++++++++++++++++++++++++
 include/drm/drm_probe_helper.h     |  1 +
 2 files changed, 98 insertions(+)

diff --git a/drivers/gpu/drm/drm_probe_helper.c
b/drivers/gpu/drm/drm_probe_helper.c
index 69b0b2b9cc1c..4a60575f5c66 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -1147,3 +1147,100 @@ int drm_connector_helper_get_modes(struct drm_connector
*connector)
 	return count;
 }
 EXPORT_SYMBOL(drm_connector_helper_get_modes);
+
+static bool tv_mode_supported(struct drm_connector *connector,
+			      enum drm_connector_tv_mode mode)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_property *property = dev->mode_config.tv_mode_property;
+
+	unsigned int i;
+
+	for (i = 0; i < property->num_values; i++)
+		if (property->values[i] == mode)
+			return true;
+
+	return false;
+}
+
+/**
+ * drm_connector_helper_tv_get_modes - Fills the modes availables to a TV
connector
+ * @connector: The connector
+ *
+ * Fills the available modes for a TV connector based on the supported
+ * TV modes, and the default mode expressed by the kernel command line.
+ *
+ * This can be used as the default TV connector helper .get_modes() hook
+ * if the driver does not need any special processing.
+ *
+ * Returns:
+ * The number of modes added to the connector.
+ */
+int drm_connector_helper_tv_get_modes(struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_cmdline_mode *cmdline = &connector->cmdline_mode;
+	struct drm_display_mode *tv_modes[2] = {};
+	struct drm_display_mode *mode;
+	unsigned int first_mode_idx;
+	unsigned int count = 0;
+	uint64_t default_mode;
+	int ret;
+
+	if (!dev->mode_config.tv_mode_property)
+		return 0;
+
+	if (tv_mode_supported(connector, DRM_MODE_TV_MODE_NTSC) ||
+	    tv_mode_supported(connector, DRM_MODE_TV_MODE_NTSC_443) ||
+	    tv_mode_supported(connector, DRM_MODE_TV_MODE_NTSC_J) ||
+	    tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL_M)) {
+		mode = drm_mode_analog_ntsc_480i(connector->dev);
+		if (!mode)
+			return 0;
+
+		tv_modes[count++] = mode;
+	}
+
+	if (tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL) ||
+	    tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL_N) ||
+	    tv_mode_supported(connector, DRM_MODE_TV_MODE_SECAM)) {
+		mode = drm_mode_analog_pal_576i(connector->dev);
+		if (!mode)
+			return 0;
+
+		tv_modes[count++] = mode;
+	}
+
+	if (count == 1) {
+		mode->type |= DRM_MODE_TYPE_PREFERRED;
+		drm_mode_probed_add(connector, mode);
+		return count;
+	}
+
+	ret = drm_object_property_get_default_value(&connector->base,
+						    dev->mode_config.tv_mode_property,
+						    &default_mode);
+	if (ret)
+		return 0;
+
+	if (cmdline->tv_mode_specified)
+		default_mode = cmdline->tv_mode;
+
+	if ((default_mode == DRM_MODE_TV_MODE_NTSC) ||
+	    (default_mode == DRM_MODE_TV_MODE_NTSC_443) ||
+	    (default_mode == DRM_MODE_TV_MODE_NTSC_J) ||
+	    (default_mode == DRM_MODE_TV_MODE_PAL_M))
+		first_mode_idx = 0;
+	else
+		first_mode_idx = 1;
+
+	mode = tv_modes[first_mode_idx];
+	mode->type |= DRM_MODE_TYPE_PREFERRED;
+	drm_mode_probed_add(connector, mode);
+
+	mode = first_mode_idx ? tv_modes[0] : tv_modes[1];
+	drm_mode_probed_add(connector, mode);
+
+	return count;
+}
+EXPORT_SYMBOL(drm_connector_helper_tv_get_modes);
diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h
index 5880daa14624..4977e0ab72db 100644
--- a/include/drm/drm_probe_helper.h
+++ b/include/drm/drm_probe_helper.h
@@ -35,5 +35,6 @@ int drm_connector_helper_get_modes_from_ddc(struct
drm_connector *connector);
 int drm_connector_helper_get_modes_fixed(struct drm_connector *connector,
 					 const struct drm_display_mode *fixed_mode);
 int drm_connector_helper_get_modes(struct drm_connector *connector);
+int drm_connector_helper_tv_get_modes(struct drm_connector *connector);
 
 #endif

-- 
b4 0.11.0-dev-99e3a

Hi Maxime,

First of all, nice idea with the helper function that can be reused by different
drivers. This is neat!

But looking at this function, it feels a bit overcomplicated. You're
creating
the two modes, then checking which one is the default, then set the preferred
one and possibly reorder them. Maybe it can be simplified somehow?

Although when I tried to refactor it myself, I ended up with something
that's
not better at all. Maybe it needs to be complicated, after all :(

Anyway, the current version seems to have a couple of bugs:
> +	if (tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL) ||
> +	    tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL_N) ||
> +	    tv_mode_supported(connector, DRM_MODE_TV_MODE_SECAM)) {
> +		mode = drm_mode_analog_pal_576i(connector->dev);
> +		if (!mode)
> +			return 0;
> +
> +		tv_modes[count++] = mode;
> +	}
If the 480i mode has been created properly, but there's an error creating
the
576i one (we enter the if (!mode) clause), the 480i mode will leak.
> +	if (count == 1) {
You're handling the count == 1 case specially, but if count == 0, the rest
of
the code will assume that two modes exist and probably segfault in the process.
> +	ret = drm_object_property_get_default_value(&connector->base,
> +						    dev->mode_config.tv_mode_property,
> +						    &default_mode);
> +	if (ret)
> +		return 0;
> +
> +	if (cmdline->tv_mode_specified)
> +		default_mode = cmdline->tv_mode;
In case of an error (ret != 0), the modes created so far in the tv_modes array
will leak.

Also, I wonder if maybe the if (cmdline->tv_mode_specified) clause should go
first? If we're going to use the default from cmdline, there's no point
in even
querying the property default value.

Best regards,
Mateusz Kwiatkowski

Den 27.10.2022 00.02, skrev Mateusz Kwiatkowski:
> Hi Maxime,
> 
> First of all, nice idea with the helper function that can be reused by
different
> drivers. This is neat!
> 
> But looking at this function, it feels a bit overcomplicated. You're
creating
> the two modes, then checking which one is the default, then set the
preferred
> one and possibly reorder them. Maybe it can be simplified somehow?
> 
> Although when I tried to refactor it myself, I ended up with something
that's
> not better at all. Maybe it needs to be complicated, after all :(
> 
I also thought that the function was complicated/difficult to read, in
particular the index stuff at the end, but I also failed in finding a
"better" solution, just a different one ;)

Noralf.

My version:

int drm_connector_helper_tv_get_modes(struct drm_connector *connector)
{
	struct drm_device *dev = connector->dev;
	struct drm_property *tv_mode_property = dev->mode_config.tv_mode_property;
	struct drm_cmdline_mode *cmdline = &connector->cmdline_mode;
	unsigned int ntsc_modes = BIT(DRM_MODE_TV_MODE_NTSC) |
				  BIT(DRM_MODE_TV_MODE_NTSC_443) |
				  BIT(DRM_MODE_TV_MODE_NTSC_J) |
				  BIT(DRM_MODE_TV_MODE_PAL_M);
	unsigned int pal_modes = BIT(DRM_MODE_TV_MODE_PAL) |
				 BIT(DRM_MODE_TV_MODE_PAL_N) |
				 BIT(DRM_MODE_TV_MODE_SECAM);
	unsigned int tv_modes[2] = { UINT_MAX, UINT_MAX };
	unsigned int i, supported_tv_modes = 0;

	if (!tv_mode_property)
		return 0;

	for (i = 0; i < tv_mode_property->num_values; i++)
		supported_tv_modes |= BIT(tv_mode_property->values[i]);

	if ((supported_tv_modes & ntsc_modes) && (supported_tv_modes &
pal_modes)) {
		uint64_t default_mode;

		if (drm_object_property_get_default_value(&connector->base,
							  tv_mode_property,
							  &default_mode))
			return 0;

		if (cmdline->tv_mode_specified)
			default_mode = cmdline->tv_mode;

		if (BIT(default_mode) & ntsc_modes) {
			tv_modes[0] = DRM_MODE_TV_MODE_NTSC;
			tv_modes[1] = DRM_MODE_TV_MODE_PAL;
		} else {
			tv_modes[0] = DRM_MODE_TV_MODE_PAL;
			tv_modes[1] = DRM_MODE_TV_MODE_NTSC;
		}
	} else if (supported_tv_modes & ntsc_modes) {
		tv_modes[0] = DRM_MODE_TV_MODE_NTSC;
	} else if (supported_tv_modes & pal_modes) {
		tv_modes[0] = DRM_MODE_TV_MODE_PAL;
	} else {
		return 0;
	}
	
	for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
		struct drm_display_mode *mode;

		if (tv_modes[i] == DRM_MODE_TV_MODE_NTSC)
			mode = drm_mode_analog_ntsc_480i(dev);
		else if (tv_modes[i] == DRM_MODE_TV_MODE_PAL)
			mode = drm_mode_analog_pal_576i(dev);
		else
			break;
		if (!mode)
			return i;
		if (!i)
			mode->type |= DRM_MODE_TYPE_PREFERRED;
		drm_mode_probed_add(connector, mode);
	}

	return i;
}