lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <842076aa-8d7c-96d6-ba46-d0e66dacd2df@tronnes.org>
Date:   Sun, 6 Nov 2022 17:33:48 +0100
From:   Noralf Trønnes <noralf@...nnes.org>
To:     maxime@...no.tech, Karol Herbst <kherbst@...hat.com>,
        Emma Anholt <emma@...olt.net>, Ben Skeggs <bskeggs@...hat.com>,
        Chen-Yu Tsai <wens@...e.org>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>,
        Samuel Holland <samuel@...lland.org>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        David Airlie <airlied@...ux.ie>,
        Maxime Ripard <mripard@...nel.org>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Lyude Paul <lyude@...hat.com>
Cc:     linux-sunxi@...ts.linux.dev, intel-gfx@...ts.freedesktop.org,
        Phil Elwell <phil@...pberrypi.com>,
        linux-arm-kernel@...ts.infradead.org,
        nouveau@...ts.freedesktop.org, Hans de Goede <hdegoede@...hat.com>,
        Dom Cobley <dom@...pberrypi.com>,
        Mateusz Kwiatkowski <kfyatek+publicgit@...il.com>,
        dri-devel@...ts.freedesktop.org,
        Dave Stevenson <dave.stevenson@...pberrypi.com>,
        linux-kernel@...r.kernel.org,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Noralf Trønnes <noralf@...nnes.org>
Subject: Re: [PATCH v6 16/23] drm/probe-helper: Provide a TV get_modes helper



Den 26.10.2022 17.33, skrev maxime@...no.tech:
> 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@...no.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;
> +

Superfluous linebreak

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

availables -> available

> + * @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);

Nit: You can use the dev variable here and below.

> +		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;

You leak the ntsc mode when returning (possibly).

> +
> +		tv_modes[count++] = mode;
> +	}
> +

Maybe check for count being zero here?

> +	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;

You leak both modes when returning here. Maybe move this up before
allocation to simplify error handling.

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

I realised that we don't verify tv_mode coming from the command line,
not here and not in the reset helper. Should we do that? A driver should
be programmed defensively to handle an illegal/unsupported value, but it
doesn't feel right to allow an illegal enum value coming through the
core/helpers.

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

I know this is not expensive, but you're looping over the property
values 7 times. An alternative solution is to rebuild the supported bitmask:

int drm_connector_helper_tv_get_modes(struct drm_connector *connector)
{
...
	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 supported_tv_modes = 0;

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

	if (supported_tv_modes & ntsc_modes)
...
	if (supported_tv_modes & pal_modes)
...

	if (BIT(default_mode) & ntsc_modes)
		first_mode_idx = 0;
	else
		first_mode_idx = 1;


Up to you if you want to do this.

Noralf.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ