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 <[email protected]>
>
> 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 <[email protected]>
> + */
> +
> +#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 <[email protected]>");
> +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
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 <[email protected]>
>>
>> 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
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 <[email protected]>
>>>
>>> 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
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 <[email protected]>
> > > >
> > > > 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