| Message ID | 20220728-rpi-analog-tv-properties-v2-13-f733a0ed9f90@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Analog TV Improvements | expand |
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@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? 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@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@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@kernel.org>"); > +MODULE_LICENSE("GPL"); >
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@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...
Hi Am 23.09.22 um 11:26 schrieb Javier Martinez Canillas: > 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@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 That document says "...one option is to conditionally #include the test file...". This doesn't sound like a strong requirement. > > 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... > There's the problem of interference between the source files when building the code. It's also not the same source code after including the test file. At a minimum, including the tests' source file further includes more files. <kunit/tests.h> also includes quite a few of Linux header files. IMHO the current convention (if any) is far from optimal and we should consider breaking it. Best regards Thomas
On 9/23/22 12:30, Thomas Zimmermann wrote: [...] >>>> + >>>> +#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 > > That document says "...one option is to conditionally #include the test > file...". This doesn't sound like a strong requirement. > That's true. >> >> 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... >> > > There's the problem of interference between the source files when > building the code. It's also not the same source code after including > the test file. At a minimum, including the tests' source file further > includes more files. <kunit/tests.h> also includes quite a few of Linux > header files. > > IMHO the current convention (if any) is far from optimal and we should > consider breaking it. > Yes, I agree with that. But probably we should be explicit about the wrapper export symbols to access static functions pattern in the KUnit docs so other subsystems could do the same and it becomes a convention ? > Best regards > Thomas >
On Fri, Sep 23, 2022 at 12:30:09PM +0200, Thomas Zimmermann wrote: > Am 23.09.22 um 11:26 schrieb Javier Martinez Canillas: > > 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@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 > > That document says "...one option is to conditionally #include the test > file...". This doesn't sound like a strong requirement. No, but this is the only option documented, which still indicates a very strong preference. > > 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. And I'm afraid this just doesn't scale. If we start testing more and more static functions, do we really want to have that wrapper for each of them? > > 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... > > > > There's the problem of interference between the source files when building > the code. It's also not the same source code after including the test file. > At a minimum, including the tests' source file further includes more files. > <kunit/tests.h> also includes quite a few of Linux header files. > > IMHO the current convention (if any) is far from optimal and we should > consider breaking it. I mean... this is a discussion about theoretical issues. If there is indeed some regular build errors on this, then sure, we can change it. I'm confident that will affect pretty much every one using kunit equally though, so I'm fairly sure the documentation itself will have changed. But right now we're discussing an alternative because of a problem we never experienced. I don't see the point of breaking the consistency provided by the documentation for something not backed by any actual problem. Maxime
On Thu, 22 Sep 2022, Maxime Ripard <maxime@cerno.tech> wrote: > 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..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 > 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 [snip] > +MODULE_AUTHOR("Maxime Ripard <mripard@kernel.org>"); > +MODULE_LICENSE("GPL"); I think these annotations are incompatible with including the unit test, and, in this case, affect drm.ko. And we'll have two kinds of tests, those that get built via tests/Makefile, and those that get included, like this one, which should not be mentioned in tests/Makefile. BR, Jani.
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 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@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@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@kernel.org>"); +MODULE_LICENSE("GPL");
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>