diff mbox series

[v7,16/23] drm/probe-helper: Provide a TV get_modes helper

Message ID 20220728-rpi-analog-tv-properties-v7-16-7072a478c6b3@cerno.tech (mailing list archive)
State New, archived
Headers show
Series drm: Analog TV Improvements | expand

Commit Message

Maxime Ripard Nov. 7, 2022, 2:16 p.m. UTC 
From: Noralf Trønnes <noralf@tronnes.org>

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: Noralf Trønnes <noralf@tronnes.org>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>

---
Changes in v7:
- Used Noralf's implementation

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(+)

Comments

Noralf Trønnes Nov. 7, 2022, 6:11 p.m. UTC | #1 
Den 07.11.2022 15.16, skrev Maxime Ripard:
> From: Noralf Trønnes <noralf@tronnes.org>
> 
> 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: Noralf Trønnes <noralf@tronnes.org>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> 
> ---
> Changes in v7:
> - Used Noralf's implementation
> 
> 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 2fc21df709bc..edb2e4c4530a 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;
> +}

This function is not used in the new implementation.

I hope you have tested this patch since I didn't even compile test my
implementation (probably should have said so...)

Noralf.

> +
> +/**
> + * 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_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;
> +}
> +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
>
Maxime Ripard Nov. 9, 2022, 3:41 p.m. UTC | #2 
Hi Noralf,

On Mon, Nov 07, 2022 at 07:11:27PM +0100, Noralf Trønnes wrote:
> 
> 
> Den 07.11.2022 15.16, skrev Maxime Ripard:
> > From: Noralf Trønnes <noralf@tronnes.org>
> > 
> > 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: Noralf Trønnes <noralf@tronnes.org>
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > 
> > ---
> > Changes in v7:
> > - Used Noralf's implementation
> > 
> > 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 2fc21df709bc..edb2e4c4530a 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;
> > +}
> 
> This function is not used in the new implementation.
>
> I hope you have tested this patch since I didn't even compile test my
> implementation (probably should have said so...)

You nailed it ;)

I had tested it (but missed the warning), and added unit tests to make
sure it was behaving properly, and it did. I'll send the unit tests in
my next version.

Thanks
Maxime
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 2fc21df709bc..edb2e4c4530a 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_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;
+}
+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