On 9/29/22 13:31, Maxime Ripard 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@xxxxxxxxxx> > > --- > Changes in v4: > - Removed MODULE macros > --- > drivers/gpu/drm/drm_client_modeset.c | 4 + > drivers/gpu/drm/tests/drm_client_modeset_test.c | 105 ++++++++++++++++++++++++ > 2 files changed, 109 insertions(+) > > 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..09dc5acbbc42 > --- /dev/null > +++ b/drivers/gpu/drm/tests/drm_client_modeset_test.c > @@ -0,0 +1,105 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2022 Maxime Ripard <mripard@xxxxxxxxxx> > + */ > + > +#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; Small nit: I guess this should be added on patch 21/30, as it is not being currently used. > + 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; I believe it would be nicer to use KUNIT_ASSERT_NOT_NULL here, instead of returning a error. > + test->priv = priv; > + > + priv->drm = drm_kunit_device_init(test, "drm-client-modeset-test"); > + if (IS_ERR(priv->drm)) > + return PTR_ERR(priv->drm); Here you could use KUNIT_ASSERT_NOT_ERR_OR_NULL. > + > + ret = drmm_connector_init(priv->drm, &priv->connector, > + &drm_client_modeset_connector_funcs, > + DRM_MODE_CONNECTOR_Unknown, > + NULL); > + if (ret) > + return ret; Same idea here. This would make it more compliant to the KUnit API. > + drm_connector_helper_add(&priv->connector, &drm_client_modeset_connector_helper_funcs); > + > + return 0; > + > +} > + > +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); You could use KUNIT_ASSERT_NOT_NULL here. > + > + 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); You could also use KUNIT_ASSERT_NOT_NULL here. This idea could also apply to the patches 21/30 and 22/30, which have a similar structure and are also using KUNIT_ASSERT_PTR_NE. Best Regards, - Maíra Canal > + > + 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, > + .test_cases = drm_pick_cmdline_tests > +}; > + > +kunit_test_suite(drm_pick_cmdline_test_suite); >