diff mbox series

[v1,08/35] drm/client: Add some tests for drm_connector_pick_cmdline_mode()

Message ID 20220728-rpi-analog-tv-properties-v1-8-3d53ae722097@cerno.tech (mailing list archive)
State New, archived
Delegated to: Neil Armstrong
Headers show
Series drm: Analog TV Improvements | expand

Commit Message

Maxime Ripard July 29, 2022, 4:34 p.m. UTC 
drm_connector_pick_cmdline_mode() is in charge of finding a proper
drm_display_mode from the definition we got in the video= command line
argument.

Let's add some unit tests to make sure we're not getting any regressions
there.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Comments

Thomas Zimmermann Aug. 2, 2022, 10:14 a.m. UTC | #1 
Hi Maxime

Am 29.07.22 um 18:34 schrieb Maxime Ripard:
> drm_connector_pick_cmdline_mode() is in charge of finding a proper
> drm_display_mode from the definition we got in the video= command line
> argument.
> 
> Let's add some unit tests to make sure we're not getting any regressions
> there.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> 
> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> index bbc535cc50dd..ee6b8f193c24 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -1237,3 +1237,7 @@ int drm_client_modeset_dpms(struct drm_client_dev *client, int mode)
>   	return ret;
>   }
>   EXPORT_SYMBOL(drm_client_modeset_dpms);
> +
> +#ifdef CONFIG_DRM_KUNIT_TEST
> +#include "tests/drm_mode_test.c"
> +#endif

Including source files is somewhat ugly, prolongs compile times and 
could even interfere with the actual source code. Can we do this in some 
other way?

I suggest to add the tests here and export them for use in the test 
case. Something like

#ifdef CONFIG_DRM_KUNIT_TEST
static drm_mode_res_1920_1080_60()
{
   ...
}

struct kunit_case drm_mode_tests[] = {
   drm_mode_res_1920_1080_60
};
EXPORT_SYMBOL(drm_mode_tests);
#endif

This would add the tests next to the tested code, but leave the test 
driver in drm_mode_test.c.

Best regards
Thomas

> diff --git a/drivers/gpu/drm/tests/drm_mode_test.c b/drivers/gpu/drm/tests/drm_mode_test.c
> new file mode 100644
> index 000000000000..0f71519788a7
> --- /dev/null
> +++ b/drivers/gpu/drm/tests/drm_mode_test.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022 Maxime Ripard <mripard@kernel.org>
> + */
> +
> +#include <drm/drm_mode.h>
> +#include <kunit/test.h>
> +
> +#include <drm/drm_connector.h>
> +#include <drm/drm_edid.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_modeset_helper_vtables.h>
> +#include <drm/drm_probe_helper.h>
> +
> +struct drm_mode_test_priv {
> +	struct device *dev;
> +	struct drm_device *drm;
> +	struct drm_connector connector;
> +};
> +
> +static const struct drm_mode_config_funcs drm_mode_config_funcs = {
> +};
> +
> +static const struct drm_driver drm_mode_driver = {
> +};
> +
> +static int drm_mode_connector_get_modes(struct drm_connector *connector)
> +{
> +	struct drm_display_mode *mode;
> +	int ret;
> +
> +	ret = drm_add_modes_noedid(connector, 1920, 1200);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static const struct drm_connector_helper_funcs drm_mode_connector_helper_funcs = {
> +	.get_modes = drm_mode_connector_get_modes,
> +};
> +
> +static const struct drm_connector_funcs drm_mode_connector_funcs = {
> +};
> +
> +static int drm_mode_test_init(struct kunit *test)
> +{
> +	struct drm_mode_test_priv *priv;
> +	int ret;
> +
> +	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +	test->priv = priv;
> +
> +	priv->dev = root_device_register("drm-mode-test");
> +	if (IS_ERR(priv->dev))
> +		return PTR_ERR(priv->dev);
> +
> +	priv->drm = drm_dev_alloc(&drm_mode_driver, priv->dev);
> +	if (IS_ERR(priv->drm))
> +		return PTR_ERR(priv->drm);
> +	priv->drm->mode_config.funcs = &drm_mode_config_funcs;
> +
> +	ret = drmm_mode_config_init(priv->drm);
> +	if (ret)
> +		return ret;
> +
> +	ret = drmm_connector_init(priv->drm, &priv->connector,
> +				  &drm_mode_connector_funcs,
> +				  DRM_MODE_CONNECTOR_Unknown,
> +				  NULL);
> +	if (ret)
> +		return ret;
> +	drm_connector_helper_add(&priv->connector, &drm_mode_connector_helper_funcs);
> +
> +	return 0;
> +}
> +
> +static void drm_mode_test_exit(struct kunit *test)
> +{
> +	struct drm_mode_test_priv *priv = test->priv;
> +
> +	drm_dev_put(priv->drm);
> +	root_device_unregister(priv->dev);
> +}
> +
> +static void drm_mode_res_1920_1080_60(struct kunit *test)
> +{
> +	struct drm_mode_test_priv *priv = test->priv;
> +	struct drm_device *drm = priv->drm;
> +	struct drm_connector *connector = &priv->connector;
> +	struct drm_cmdline_mode *cmdline_mode = &connector->cmdline_mode;
> +	struct drm_display_mode *expected_mode, *mode;
> +	const char *cmdline = "1920x1080@60";
> +	int ret;
> +
> +	expected_mode = drm_mode_find_dmt(priv->drm, 1920, 1080, 60, false);
> +	KUNIT_ASSERT_PTR_NE(test, expected_mode, NULL);
> +
> +	KUNIT_ASSERT_TRUE(test,
> +			  drm_mode_parse_command_line_for_connector(cmdline,
> +								    connector,
> +								    cmdline_mode));
> +
> +	mutex_lock(&drm->mode_config.mutex);
> +	ret = drm_helper_probe_single_connector_modes(connector, 1920, 1080);
> +	mutex_unlock(&drm->mode_config.mutex);
> +	KUNIT_ASSERT_GT(test, ret, 0);
> +
> +	mode = drm_connector_pick_cmdline_mode(connector);
> +	KUNIT_ASSERT_PTR_NE(test, mode, NULL);
> +
> +	KUNIT_EXPECT_TRUE(test, drm_mode_equal(expected_mode, mode));
> +}
> +
> +static struct kunit_case drm_mode_tests[] = {
> +	KUNIT_CASE(drm_mode_res_1920_1080_60),
> +	{}
> +};
> +
> +static struct kunit_suite drm_mode_test_suite = {
> +	.name = "drm_mode",
> +	.init = drm_mode_test_init,
> +	.exit = drm_mode_test_exit,
> +	.test_cases = drm_mode_tests
> +};
> +
> +kunit_test_suite(drm_mode_test_suite);
> +MODULE_AUTHOR("Maxime Ripard <mripard@kernel.org>");
> +MODULE_LICENSE("GPL");
>
Maxime Ripard Aug. 15, 2022, 8:42 a.m. UTC | #2 
Hi Thomas,

On Tue, Aug 02, 2022 at 12:14:29PM +0200, Thomas Zimmermann wrote:
> Am 29.07.22 um 18:34 schrieb Maxime Ripard:
> > drm_connector_pick_cmdline_mode() is in charge of finding a proper
> > drm_display_mode from the definition we got in the video= command line
> > argument.
> > 
> > Let's add some unit tests to make sure we're not getting any regressions
> > there.
> > 
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > 
> > diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> > index bbc535cc50dd..ee6b8f193c24 100644
> > --- a/drivers/gpu/drm/drm_client_modeset.c
> > +++ b/drivers/gpu/drm/drm_client_modeset.c
> > @@ -1237,3 +1237,7 @@ int drm_client_modeset_dpms(struct drm_client_dev *client, int mode)
> >   	return ret;
> >   }
> >   EXPORT_SYMBOL(drm_client_modeset_dpms);
> > +
> > +#ifdef CONFIG_DRM_KUNIT_TEST
> > +#include "tests/drm_mode_test.c"
> > +#endif
> 
> Including source files is somewhat ugly, prolongs compile times and could
> even interfere with the actual source code. Can we do this in some other
> way?

Yeah, this irks me a bit as well, but it's the preferred way of doing it
according to the kunit doc:
https://www.kernel.org/doc/html/latest/dev-tools/kunit/usage.html#testing-static-functions

> I suggest to add the tests here and export them for use in the test case.
> Something like
> 
> #ifdef CONFIG_DRM_KUNIT_TEST
> static drm_mode_res_1920_1080_60()
> {
>   ...
> }
> 
> struct kunit_case drm_mode_tests[] = {
>   drm_mode_res_1920_1080_60
> };
> EXPORT_SYMBOL(drm_mode_tests);
> #endif
> 
> This would add the tests next to the tested code, but leave the test driver
> in drm_mode_test.c.

The test suite is fairly small for now, but if we end up with dozens of
tests like what is there for the command line parser (which could happen
for that kind of functions), I'm very afraid that the original source
file will become unreadable, while this has the advantage to keep the
original file readability.

Maxime
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
index bbc535cc50dd..ee6b8f193c24 100644
--- a/drivers/gpu/drm/drm_client_modeset.c
+++ b/drivers/gpu/drm/drm_client_modeset.c
@@ -1237,3 +1237,7 @@  int drm_client_modeset_dpms(struct drm_client_dev *client, int mode)
 	return ret;
 }
 EXPORT_SYMBOL(drm_client_modeset_dpms);
+
+#ifdef CONFIG_DRM_KUNIT_TEST
+#include "tests/drm_mode_test.c"
+#endif
diff --git a/drivers/gpu/drm/tests/drm_mode_test.c b/drivers/gpu/drm/tests/drm_mode_test.c
new file mode 100644
index 000000000000..0f71519788a7
--- /dev/null
+++ b/drivers/gpu/drm/tests/drm_mode_test.c
@@ -0,0 +1,132 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2022 Maxime Ripard <mripard@kernel.org>
+ */
+
+#include <drm/drm_mode.h>
+#include <kunit/test.h>
+
+#include <drm/drm_connector.h>
+#include <drm/drm_edid.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_probe_helper.h>
+
+struct drm_mode_test_priv {
+	struct device *dev;
+	struct drm_device *drm;
+	struct drm_connector connector;
+};
+
+static const struct drm_mode_config_funcs drm_mode_config_funcs = {
+};
+
+static const struct drm_driver drm_mode_driver = {
+};
+
+static int drm_mode_connector_get_modes(struct drm_connector *connector)
+{
+	struct drm_display_mode *mode;
+	int ret;
+
+	ret = drm_add_modes_noedid(connector, 1920, 1200);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static const struct drm_connector_helper_funcs drm_mode_connector_helper_funcs = {
+	.get_modes = drm_mode_connector_get_modes,
+};
+
+static const struct drm_connector_funcs drm_mode_connector_funcs = {
+};
+
+static int drm_mode_test_init(struct kunit *test)
+{
+	struct drm_mode_test_priv *priv;
+	int ret;
+
+	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	test->priv = priv;
+
+	priv->dev = root_device_register("drm-mode-test");
+	if (IS_ERR(priv->dev))
+		return PTR_ERR(priv->dev);
+
+	priv->drm = drm_dev_alloc(&drm_mode_driver, priv->dev);
+	if (IS_ERR(priv->drm))
+		return PTR_ERR(priv->drm);
+	priv->drm->mode_config.funcs = &drm_mode_config_funcs;
+
+	ret = drmm_mode_config_init(priv->drm);
+	if (ret)
+		return ret;
+
+	ret = drmm_connector_init(priv->drm, &priv->connector,
+				  &drm_mode_connector_funcs,
+				  DRM_MODE_CONNECTOR_Unknown,
+				  NULL);
+	if (ret)
+		return ret;
+	drm_connector_helper_add(&priv->connector, &drm_mode_connector_helper_funcs);
+
+	return 0;
+}
+
+static void drm_mode_test_exit(struct kunit *test)
+{
+	struct drm_mode_test_priv *priv = test->priv;
+
+	drm_dev_put(priv->drm);
+	root_device_unregister(priv->dev);
+}
+
+static void drm_mode_res_1920_1080_60(struct kunit *test)
+{
+	struct drm_mode_test_priv *priv = test->priv;
+	struct drm_device *drm = priv->drm;
+	struct drm_connector *connector = &priv->connector;
+	struct drm_cmdline_mode *cmdline_mode = &connector->cmdline_mode;
+	struct drm_display_mode *expected_mode, *mode;
+	const char *cmdline = "1920x1080@60";
+	int ret;
+
+	expected_mode = drm_mode_find_dmt(priv->drm, 1920, 1080, 60, false);
+	KUNIT_ASSERT_PTR_NE(test, expected_mode, NULL);
+
+	KUNIT_ASSERT_TRUE(test,
+			  drm_mode_parse_command_line_for_connector(cmdline,
+								    connector,
+								    cmdline_mode));
+
+	mutex_lock(&drm->mode_config.mutex);
+	ret = drm_helper_probe_single_connector_modes(connector, 1920, 1080);
+	mutex_unlock(&drm->mode_config.mutex);
+	KUNIT_ASSERT_GT(test, ret, 0);
+
+	mode = drm_connector_pick_cmdline_mode(connector);
+	KUNIT_ASSERT_PTR_NE(test, mode, NULL);
+
+	KUNIT_EXPECT_TRUE(test, drm_mode_equal(expected_mode, mode));
+}
+
+static struct kunit_case drm_mode_tests[] = {
+	KUNIT_CASE(drm_mode_res_1920_1080_60),
+	{}
+};
+
+static struct kunit_suite drm_mode_test_suite = {
+	.name = "drm_mode",
+	.init = drm_mode_test_init,
+	.exit = drm_mode_test_exit,
+	.test_cases = drm_mode_tests
+};
+
+kunit_test_suite(drm_mode_test_suite);
+MODULE_AUTHOR("Maxime Ripard <mripard@kernel.org>");
+MODULE_LICENSE("GPL");