Re: [PATCH v8 06/24] drm/modes: Add a function to generate analog display modes

["Maíra Canal" <mairacanal@riseup.net>]

Hi Maxime,

On 11/10/22 08:07, Maxime Ripard wrote:
> Multiple drivers (meson, vc4, sun4i) define analog TV 525-lines and
> 625-lines modes in their drivers.
> 
> Since those modes are fairly standard, and that we'll need to use them
> in more places in the future, it makes sense to move their definition
> into the core framework.
> 
> However, analog display usually have fairly loose timings requirements,
> the only discrete parameters being the total number of lines and pixel
> clock frequency. Thus, we created a function that will create a display
> mode from the standard, the pixel frequency and the active area.
> 
> Tested-by: Mateusz Kwiatkowski <kfyatek+publicgit@gmail.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> 
> ---
> Changes in v6:
> - Fix typo
> 
> Changes in v4:
> - Reworded the line length check comment
> - Switch to HZ_PER_KHZ in tests
> - Use previous timing to fill our mode
> - Move the number of lines check earlier
> ---
>  drivers/gpu/drm/drm_modes.c            | 474 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/tests/Makefile         |   1 +
>  drivers/gpu/drm/tests/drm_modes_test.c | 145 ++++++++++
>  include/drm/drm_modes.h                |  17 ++
>  4 files changed, 637 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tests/drm_modes_test.c b/drivers/gpu/drm/tests/drm_modes_test.c
> new file mode 100644
> index 000000000000..afeda9f07859
> --- /dev/null
> +++ b/drivers/gpu/drm/tests/drm_modes_test.c
> @@ -0,0 +1,145 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Kunit test for drm_modes functions
> + */
> +
> +#include <drm/drm_drv.h>
> +#include <drm/drm_modes.h>
> +
> +#include <kunit/test.h>
> +
> +#include <linux/units.h>
> +
> +#include "drm_kunit_helpers.h"
> +
> +struct drm_modes_test_priv {
> +	struct drm_device *drm;
> +};
> +
> +static int drm_modes_test_init(struct kunit *test)
> +{
> +	struct drm_modes_test_priv *priv;
> +
> +	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_NULL(test, priv);
> +
> +	priv->drm = drm_kunit_device_init(test, DRIVER_MODESET, "drm-modes-test");
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->drm);
> +
> +	test->priv = priv;
> +
> +	return 0;
> +}
> +

As you did on the other tests, it would be nice to use the same naming
convention as the other DRM tests. So, maybe change the "drm_modes"
prefix to "drm_test_modes".

> +static void drm_modes_analog_tv_ntsc_480i(struct kunit *test)
> +{
> +	struct drm_modes_test_priv *priv = test->priv;
> +	struct drm_display_mode *mode;
> +
> +	mode = drm_analog_tv_mode(priv->drm,
> +				  DRM_MODE_TV_MODE_NTSC,
> +				  13500 * HZ_PER_KHZ, 720, 480,
> +				  true);
> +	KUNIT_ASSERT_NOT_NULL(test, mode);
> +
> +	KUNIT_EXPECT_EQ(test, drm_mode_vrefresh(mode), 60);
> +	KUNIT_EXPECT_EQ(test, mode->hdisplay, 720);
> +
> +	/* BT.601 defines hsync_start at 736 for 480i */
> +	KUNIT_EXPECT_EQ(test, mode->hsync_start, 736);
> +
> +	/*
> +	 * The NTSC standard expects a line to take 63.556us. With a
> +	 * pixel clock of 13.5 MHz, a pixel takes around 74ns, so we
> +	 * need to have 63556ns / 74ns = 858.
> +	 *
> +	 * This is also mandated by BT.601.
> +	 */
> +	KUNIT_EXPECT_EQ(test, mode->htotal, 858);
> +
> +	KUNIT_EXPECT_EQ(test, mode->vdisplay, 480);
> +	KUNIT_EXPECT_EQ(test, mode->vtotal, 525);
> +}
> +
> +static void drm_modes_analog_tv_ntsc_480i_inlined(struct kunit *test)
> +{
> +	struct drm_modes_test_priv *priv = test->priv;
> +	struct drm_display_mode *expected, *mode;
> +
> +	expected = drm_analog_tv_mode(priv->drm,
> +				      DRM_MODE_TV_MODE_NTSC,
> +				      13500 * HZ_PER_KHZ, 720, 480,
> +				      true);
> +	KUNIT_ASSERT_NOT_NULL(test, expected);
> +
> +	mode = drm_mode_analog_ntsc_480i(priv->drm);
> +	KUNIT_ASSERT_NOT_NULL(test, mode);
> +
> +	KUNIT_EXPECT_TRUE(test, drm_mode_equal(expected, mode));
> +}
> +
> +static void drm_modes_analog_tv_pal_576i(struct kunit *test)
> +{
> +	struct drm_modes_test_priv *priv = test->priv;
> +	struct drm_display_mode *mode;
> +
> +	mode = drm_analog_tv_mode(priv->drm,
> +				  DRM_MODE_TV_MODE_PAL,
> +				  13500 * HZ_PER_KHZ, 720, 576,
> +				  true);
> +	KUNIT_ASSERT_NOT_NULL(test, mode);
> +
> +	KUNIT_EXPECT_EQ(test, drm_mode_vrefresh(mode), 50);
> +	KUNIT_EXPECT_EQ(test, mode->hdisplay, 720);
> +
> +	/* BT.601 defines hsync_start at 732 for 576i */
> +	KUNIT_EXPECT_EQ(test, mode->hsync_start, 732);
> +
> +	/*
> +	 * The PAL standard expects a line to take 64us. With a pixel
> +	 * clock of 13.5 MHz, a pixel takes around 74ns, so we need to
> +	 * have 64000ns / 74ns = 864.
> +	 *
> +	 * This is also mandated by BT.601.
> +	 */
> +	KUNIT_EXPECT_EQ(test, mode->htotal, 864);
> +
> +	KUNIT_EXPECT_EQ(test, mode->vdisplay, 576);
> +	KUNIT_EXPECT_EQ(test, mode->vtotal, 625);
> +}
> +
> +static void drm_modes_analog_tv_pal_576i_inlined(struct kunit *test)
> +{
> +	struct drm_modes_test_priv *priv = test->priv;
> +	struct drm_display_mode *expected, *mode;
> +
> +	expected = drm_analog_tv_mode(priv->drm,
> +				      DRM_MODE_TV_MODE_PAL,
> +				      13500 * HZ_PER_KHZ, 720, 576,
> +				      true);
> +	KUNIT_ASSERT_NOT_NULL(test, expected);
> +
> +	mode = drm_mode_analog_pal_576i(priv->drm);
> +	KUNIT_ASSERT_NOT_NULL(test, mode);
> +
> +	KUNIT_EXPECT_TRUE(test, drm_mode_equal(expected, mode));
> +}
> +
> +static struct kunit_case drm_modes_analog_tv_tests[] = {
> +	KUNIT_CASE(drm_modes_analog_tv_ntsc_480i),
> +	KUNIT_CASE(drm_modes_analog_tv_ntsc_480i_inlined),
> +	KUNIT_CASE(drm_modes_analog_tv_pal_576i),
> +	KUNIT_CASE(drm_modes_analog_tv_pal_576i_inlined),
> +	{ }
> +};
> +
> +static struct kunit_suite drm_modes_analog_tv_test_suite = {
> +	.name = "drm_modes_analog_tv",
> +	.init = drm_modes_test_init,
> +	.test_cases = drm_modes_analog_tv_tests,
> +};
> +
> +kunit_test_suites(
> +	&drm_modes_analog_tv_test_suite
> +);

Considering that there is only one suite, you could use the
kunit_test_suite macro instead.

Best Regards,
- Maíra Canal

> +MODULE_LICENSE("GPL v2");