Re: [PATCH v6 14/23] drm/modes: Properly generate a drm_display_mode from a named mode

From: Mateusz Kwiatkowski <kfyatek@gmail.com>
To: maxime@cerno.tech, Karol Herbst <kherbst@redhat.com>,
	Emma Anholt <emma@anholt.net>, Ben Skeggs <bskeggs@redhat.com>,
	Chen-Yu Tsai <wens@csie.org>,
	Rodrigo Vivi <rodrigo.vivi@intel.com>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Jani Nikula <jani.nikula@linux.intel.com>,
	Daniel Vetter <daniel@ffwll.ch>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
	Samuel Holland <samuel@sholland.org>,
	Jernej Skrabec <jernej.skrabec@gmail.com>,
	David Airlie <airlied@linux.ie>,
	Maxime Ripard <mripard@kernel.org>,
	Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
	Lyude Paul <lyude@redhat.com>
Cc: "Dom Cobley" <dom@raspberrypi.com>,
	"Dave Stevenson" <dave.stevenson@raspberrypi.com>,
	nouveau@lists.freedesktop.org, intel-gfx@lists.freedesktop.org,
	linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
	"Phil Elwell" <phil@raspberrypi.com>,
	"Hans de Goede" <hdegoede@redhat.com>,
	"Noralf Trønnes" <noralf@tronnes.org>,
	"Geert Uytterhoeven" <geert@linux-m68k.org>,
	linux-sunxi@lists.linux.dev,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v6 14/23] drm/modes: Properly generate a drm_display_mode from a named mode
Date: Wed, 26 Oct 2022 23:25:20 +0200	[thread overview]
Message-ID: <1adf0b0b-3743-4b5a-9416-823b8bf903fa@gmail.com> (raw)
In-Reply-To: <20220728-rpi-analog-tv-properties-v6-14-e7792734108f@cerno.tech>

Hi Maxime,

> +static struct drm_display_mode *drm_named_mode(struct drm_device *dev,
> +					       struct drm_cmdline_mode *cmd)
> +{
> +	struct drm_display_mode *mode;
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(drm_named_modes); i++) {
> +		const struct drm_named_mode *named_mode = &drm_named_modes[i];
> +
> +		if (strcmp(cmd->name, named_mode->name))
> +			continue;
> +
> +		if (!named_mode->tv_mode)
> +			continue;
> +
> +		mode = drm_analog_tv_mode(dev,
> +					  named_mode->tv_mode,
> +					  named_mode->pixel_clock_khz * 1000,
> +					  named_mode->xres,
> +					  named_mode->yres,
> +					  named_mode->flags & DRM_MODE_FLAG_INTERLACE);
> +		if (!mode)
> +			return NULL;
> +
> +		return mode;
> +	}
> +
> +	return NULL;
> +}
> +

You didn't add tv_mode_specified to struct drm_named_mode, and left the
if (!named_mode->tv_mode) condition here. This will break on NTSC.

Best regards,
Mateusz Kwiatkowski