Thomas Zimmermann
2022-Sep-23 09:15 UTC
[Nouveau] [PATCH v2 13/33] drm/client: Add some tests for drm_connector_pick_cmdline_mode()
Hi Am 22.09.22 um 16:25 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 at cerno.tech> > > diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c > index bbc535cc50dd..d553e793e673 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_client_modeset_test.c" > +#endifI strongly dislike this style of including source files in each other. It's a recipe for all kind of build errors. Can you do something else? As the tested function is an internal interface, maybe export a wrapper if tests are enabled, like this: #ifdef CONFIG_DRM_KUNIT_TEST struct drm_display_mode * drm_connector_pick_cmdline_mode_kunit(drm_conenctor) { return drm_connector_pick_cmdline_mode(connector) } EXPORT_SYMBOL(drm_connector_pick_cmdline_mode_kunit) #endif The wrapper's declaration can be located in the kunit test file. Best regards Thomas> diff --git a/drivers/gpu/drm/tests/drm_client_modeset_test.c b/drivers/gpu/drm/tests/drm_client_modeset_test.c > new file mode 100644 > index 000000000000..46335de7bc6b > --- /dev/null > +++ b/drivers/gpu/drm/tests/drm_client_modeset_test.c > @@ -0,0 +1,114 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2022 Maxime Ripard <mripard at kernel.org> > + */ > + > +#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> > + > +#include "drm_kunit_helpers.h" > + > +struct drm_client_modeset_test_priv { > + struct drm_device *drm; > + struct drm_connector connector; > +}; > + > +static int drm_client_modeset_connector_get_modes(struct drm_connector *connector) > +{ > + struct drm_display_mode *mode; > + int count; > + > + count = drm_add_modes_noedid(connector, 1920, 1200); > + > + return count; > +} > + > +static const struct drm_connector_helper_funcs drm_client_modeset_connector_helper_funcs = { > + .get_modes = drm_client_modeset_connector_get_modes, > +}; > + > +static const struct drm_connector_funcs drm_client_modeset_connector_funcs = { > +}; > + > +static int drm_client_modeset_test_init(struct kunit *test) > +{ > + struct drm_client_modeset_test_priv *priv; > + int ret; > + > + priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + test->priv = priv; > + > + priv->drm = drm_kunit_device_init("drm-client-modeset-test"); > + if (IS_ERR(priv->drm)) > + return PTR_ERR(priv->drm); > + > + ret = drmm_connector_init(priv->drm, &priv->connector, > + &drm_client_modeset_connector_funcs, > + DRM_MODE_CONNECTOR_Unknown, > + NULL); > + if (ret) > + return ret; > + drm_connector_helper_add(&priv->connector, &drm_client_modeset_connector_helper_funcs); > + > + return 0; > +} > + > +static void drm_client_modeset_test_exit(struct kunit *test) > +{ > + struct drm_client_modeset_test_priv *priv = test->priv; > + > + drm_kunit_device_exit(priv->drm); > +} > + > +static void drm_pick_cmdline_res_1920_1080_60(struct kunit *test) > +{ > + struct drm_client_modeset_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 at 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_pick_cmdline_tests[] = { > + KUNIT_CASE(drm_pick_cmdline_res_1920_1080_60), > + {} > +}; > + > +static struct kunit_suite drm_pick_cmdline_test_suite = { > + .name = "drm_pick_cmdline", > + .init = drm_client_modeset_test_init, > + .exit = drm_client_modeset_test_exit, > + .test_cases = drm_pick_cmdline_tests > +}; > + > +kunit_test_suite(drm_pick_cmdline_test_suite); > +MODULE_AUTHOR("Maxime Ripard <mripard at kernel.org>"); > +MODULE_LICENSE("GPL"); >-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 N?rnberg, Germany (HRB 36809, AG N?rnberg) Gesch?ftsf?hrer: Ivo Totev -------------- next part -------------- A non-text attachment was scrubbed... Name: OpenPGP_signature Type: application/pgp-signature Size: 840 bytes Desc: OpenPGP digital signature URL: <https://lists.freedesktop.org/archives/nouveau/attachments/20220923/26057694/attachment.sig>
Javier Martinez Canillas
2022-Sep-23 09:26 UTC
[Nouveau] [PATCH v2 13/33] drm/client: Add some tests for drm_connector_pick_cmdline_mode()
On 9/23/22 11:15, Thomas Zimmermann wrote:> Hi > > Am 22.09.22 um 16:25 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 at cerno.tech> >> >> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c >> index bbc535cc50dd..d553e793e673 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_client_modeset_test.c" >> +#endif > > I strongly dislike this style of including source files in each other. > It's a recipe for all kind of build errors. Can you do something else? >This seems to be the convention used to test static functions and what's documented in the Kunit docs: https://kunit.dev/third_party/kernel/docs/tips.html#testing-static-functions I agree with you that's not ideal but I think that consistency with how it is done by other subsystems is also important.> As the tested function is an internal interface, maybe export a wrapper > if tests are enabled, like this: > > #ifdef CONFIG_DRM_KUNIT_TEST > struct drm_display_mode * > drm_connector_pick_cmdline_mode_kunit(drm_conenctor) > { > return drm_connector_pick_cmdline_mode(connector) > } > EXPORT_SYMBOL(drm_connector_pick_cmdline_mode_kunit) > #endif > > The wrapper's declaration can be located in the kunit test file. >But that's also not nice since we are artificially exposing these only to allow the static functions to be called from unit tests. And would be a different approach than the one used by all other subsystems... -- Best regards, Javier Martinez Canillas Core Platforms Red Hat