Re: [PATCH v2 13/33] drm/client: Add some tests for drm_connector_pick_cmdline_mode()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>

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

--
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

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux