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@xxxxxxxxx>
> Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
>
> ---
> 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");
