Den 07.11.2022 15.16, skrev Maxime Ripard:
> From: Noralf Trønnes <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Maxime Ripard <[email protected]>
>
> ---
> 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
>
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 <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Maxime Ripard <[email protected]>
> >
> > ---
> > 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