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
Thomas Zimmermann
2022-Sep-23 10:30 UTC
[Nouveau] [PATCH v2 13/33] drm/client: Add some tests for drm_connector_pick_cmdline_mode()
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 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-functionsThat 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 -- 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/c645f35f/attachment.sig>