| Message ID | 20231024191002.1620-11-gcarlos@disroot.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Increase coverage on drm_framebuffer.c | expand |
On Tue, Oct 24, 2023 at 04:10:01PM -0300, Carlos Eduardo Gallo Filho wrote: > Add a single KUnit test case for the drm_mode_addfb2 function. > > Signed-off-by: Carlos Eduardo Gallo Filho <gcarlos@disroot.org> > --- > v2: > - Reorder kunit cases alphabetically. > - Rely on drm_kunit_helper_alloc_device() for mock initialization. > --- > drivers/gpu/drm/tests/drm_framebuffer_test.c | 104 ++++++++++++++++++- > 1 file changed, 103 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c b/drivers/gpu/drm/tests/drm_framebuffer_test.c > index dbe412d0dca4..149e1985e53f 100644 > --- a/drivers/gpu/drm/tests/drm_framebuffer_test.c > +++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c > @@ -10,6 +10,7 @@ > #include <drm/drm_device.h> > #include <drm/drm_drv.h> > #include <drm/drm_mode.h> > +#include <drm/drm_file.h> > #include <drm/drm_framebuffer.h> > #include <drm/drm_fourcc.h> > #include <drm/drm_kunit_helpers.h> > @@ -341,8 +342,18 @@ static const struct drm_framebuffer_test drm_framebuffer_create_cases[] = { > }, > }; > > +/* > + * This struct is intended to provide a way to mocked functions communicate > + * with the outer test when it can't be achieved by using its return value. In > + * this way, the functions that receive the mocked drm_device, for example, can > + * grab a reference to @private and actually return something to be used on some > + * expectation. Also having the @test member allows doing expectations from > + * inside mocked functions. > + */ > struct drm_framebuffer_test_priv { > struct drm_device dev; > + struct drm_file file_priv; > + struct kunit *test; > void *private; > }; > > @@ -365,14 +376,16 @@ static int drm_framebuffer_test_init(struct kunit *test) > struct device *parent; > struct drm_framebuffer_test_priv *priv; > struct drm_device *dev; > + struct drm_file *file_priv; > > parent = drm_kunit_helper_alloc_device(test); > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent); > > priv = drm_kunit_helper_alloc_drm_device(test, parent, typeof(*priv), > - dev, 0); > + dev, DRIVER_MODESET); > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv); > dev = &priv->dev; > + file_priv = &priv->file_priv; > > dev->mode_config.min_width = MIN_WIDTH; > dev->mode_config.max_width = MAX_WIDTH; > @@ -380,10 +393,22 @@ static int drm_framebuffer_test_init(struct kunit *test) > dev->mode_config.max_height = MAX_HEIGHT; > dev->mode_config.funcs = &mock_config_funcs; > > + mutex_init(&file_priv->fbs_lock); > + INIT_LIST_HEAD(&file_priv->fbs); > + mock_drm_getfile() is what you should use there > test->priv = priv; > + > return 0; > } > > +static void drm_framebuffer_test_exit(struct kunit *test) > +{ > + struct drm_framebuffer_test_priv *priv = test->priv; > + struct drm_file *file_priv = &priv->file_priv; > + > + mutex_destroy(&file_priv->fbs_lock); > +} and fput(file) here, which should probably be a kunit action. > + > static void drm_test_framebuffer_create(struct kunit *test) > { > const struct drm_framebuffer_test *params = test->param_value; > @@ -650,7 +675,83 @@ static void drm_test_framebuffer_free(struct kunit *test) > KUNIT_EXPECT_PTR_EQ(test, obj, NULL); > } > > +static struct drm_framebuffer * > +fb_create_addfb2_mock(struct drm_device *dev, struct drm_file *file_priv, > + const struct drm_mode_fb_cmd2 *cmd) > +{ > + struct drm_framebuffer_test_priv *priv = container_of(dev, typeof(*priv), dev); > + struct drm_framebuffer *fb; > + struct kunit *test = priv->test; kunit_get_current_test(); > + > + fb = kunit_kzalloc(test, sizeof(*fb), GFP_KERNEL); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fb); That's probably a bad idea to allocate the framebuffer unit kunit_kzalloc there. It will get freed at the end of the test but the DRM device is still around then so it will probably result in a use-after-free. > + > + fb->base.id = 1; > + > + priv->private = fb; > + return fb; Again, I don't think we should fake a buffer here, we should allocate a real one. We want to test the behaviour of add_fb2, not whether our mock of the framebuffer creation is good enough. > +} > + > +static struct drm_mode_config_funcs config_funcs_addfb2_mock = { > + .fb_create = fb_create_addfb2_mock, > +}; > + > +static void drm_test_framebuffer_addfb2(struct kunit *test) > +{ > + struct drm_framebuffer_test_priv *priv = test->priv; > + struct drm_device *dev = &priv->dev; > + struct drm_file *file_priv = &priv->file_priv; > + struct drm_framebuffer *fb = NULL; > + u32 driver_features; > + int ret; > + > + /* A valid cmd */ > + struct drm_mode_fb_cmd2 cmd = { > + .width = 600, .height = 600, > + .pixel_format = DRM_FORMAT_ABGR8888, > + .handles = { 1, 0, 0 }, .pitches = { 4 * 600, 0, 0 }, > + }; > + > + priv->test = test; > + dev->mode_config.funcs = &config_funcs_addfb2_mock; > + > + /* Must fail due to missing DRIVER_MODESET support */ > + driver_features = dev->driver_features; > + dev->driver_features = 0u; > + ret = drm_mode_addfb2(dev, &cmd, file_priv); > + KUNIT_EXPECT_EQ(test, ret, -EOPNOTSUPP); > + KUNIT_ASSERT_PTR_EQ(test, priv->private, NULL); > + dev->driver_features = driver_features; > + > + /* > + * Set an invalid cmd to trigger a fail on the > + * drm_internal_framebuffer_create function > + */ > + cmd.width = 0; > + ret = drm_mode_addfb2(dev, &cmd, file_priv); > + KUNIT_EXPECT_EQ(test, ret, -EINVAL); > + KUNIT_ASSERT_PTR_EQ(test, priv->private, NULL); > + cmd.width = 600; /* restore to a valid value */ > + > + ret = drm_mode_addfb2(dev, &cmd, file_priv); > + KUNIT_EXPECT_EQ(test, ret, 0); > + > + /* The fb_create_addfb2_mock set fb id to 1 */ > + KUNIT_EXPECT_EQ(test, cmd.fb_id, 1); > + > + fb = priv->private; > + > + /* The fb must be properly added to the file_priv->fbs list */ > + KUNIT_EXPECT_PTR_EQ(test, file_priv->fbs.prev, &fb->filp_head); > + KUNIT_EXPECT_PTR_EQ(test, file_priv->fbs.next, &fb->filp_head); > + > + /* There must be just one fb on the list */ > + KUNIT_EXPECT_PTR_EQ(test, fb->filp_head.prev, &file_priv->fbs); > + KUNIT_EXPECT_PTR_EQ(test, fb->filp_head.next, &file_priv->fbs); > +} > + All these should be separate, documented, tests. Maxime >
diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c b/drivers/gpu/drm/tests/drm_framebuffer_test.c index dbe412d0dca4..149e1985e53f 100644 --- a/drivers/gpu/drm/tests/drm_framebuffer_test.c +++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c @@ -10,6 +10,7 @@ #include <drm/drm_device.h> #include <drm/drm_drv.h> #include <drm/drm_mode.h> +#include <drm/drm_file.h> #include <drm/drm_framebuffer.h> #include <drm/drm_fourcc.h> #include <drm/drm_kunit_helpers.h> @@ -341,8 +342,18 @@ static const struct drm_framebuffer_test drm_framebuffer_create_cases[] = { }, }; +/* + * This struct is intended to provide a way to mocked functions communicate + * with the outer test when it can't be achieved by using its return value. In + * this way, the functions that receive the mocked drm_device, for example, can + * grab a reference to @private and actually return something to be used on some + * expectation. Also having the @test member allows doing expectations from + * inside mocked functions. + */ struct drm_framebuffer_test_priv { struct drm_device dev; + struct drm_file file_priv; + struct kunit *test; void *private; }; @@ -365,14 +376,16 @@ static int drm_framebuffer_test_init(struct kunit *test) struct device *parent; struct drm_framebuffer_test_priv *priv; struct drm_device *dev; + struct drm_file *file_priv; parent = drm_kunit_helper_alloc_device(test); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent); priv = drm_kunit_helper_alloc_drm_device(test, parent, typeof(*priv), - dev, 0); + dev, DRIVER_MODESET); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv); dev = &priv->dev; + file_priv = &priv->file_priv; dev->mode_config.min_width = MIN_WIDTH; dev->mode_config.max_width = MAX_WIDTH; @@ -380,10 +393,22 @@ static int drm_framebuffer_test_init(struct kunit *test) dev->mode_config.max_height = MAX_HEIGHT; dev->mode_config.funcs = &mock_config_funcs; + mutex_init(&file_priv->fbs_lock); + INIT_LIST_HEAD(&file_priv->fbs); + test->priv = priv; + return 0; } +static void drm_framebuffer_test_exit(struct kunit *test) +{ + struct drm_framebuffer_test_priv *priv = test->priv; + struct drm_file *file_priv = &priv->file_priv; + + mutex_destroy(&file_priv->fbs_lock); +} + static void drm_test_framebuffer_create(struct kunit *test) { const struct drm_framebuffer_test *params = test->param_value; @@ -650,7 +675,83 @@ static void drm_test_framebuffer_free(struct kunit *test) KUNIT_EXPECT_PTR_EQ(test, obj, NULL); } +static struct drm_framebuffer * +fb_create_addfb2_mock(struct drm_device *dev, struct drm_file *file_priv, + const struct drm_mode_fb_cmd2 *cmd) +{ + struct drm_framebuffer_test_priv *priv = container_of(dev, typeof(*priv), dev); + struct drm_framebuffer *fb; + struct kunit *test = priv->test; + + fb = kunit_kzalloc(test, sizeof(*fb), GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fb); + + fb->base.id = 1; + + priv->private = fb; + return fb; +} + +static struct drm_mode_config_funcs config_funcs_addfb2_mock = { + .fb_create = fb_create_addfb2_mock, +}; + +static void drm_test_framebuffer_addfb2(struct kunit *test) +{ + struct drm_framebuffer_test_priv *priv = test->priv; + struct drm_device *dev = &priv->dev; + struct drm_file *file_priv = &priv->file_priv; + struct drm_framebuffer *fb = NULL; + u32 driver_features; + int ret; + + /* A valid cmd */ + struct drm_mode_fb_cmd2 cmd = { + .width = 600, .height = 600, + .pixel_format = DRM_FORMAT_ABGR8888, + .handles = { 1, 0, 0 }, .pitches = { 4 * 600, 0, 0 }, + }; + + priv->test = test; + dev->mode_config.funcs = &config_funcs_addfb2_mock; + + /* Must fail due to missing DRIVER_MODESET support */ + driver_features = dev->driver_features; + dev->driver_features = 0u; + ret = drm_mode_addfb2(dev, &cmd, file_priv); + KUNIT_EXPECT_EQ(test, ret, -EOPNOTSUPP); + KUNIT_ASSERT_PTR_EQ(test, priv->private, NULL); + dev->driver_features = driver_features; + + /* + * Set an invalid cmd to trigger a fail on the + * drm_internal_framebuffer_create function + */ + cmd.width = 0; + ret = drm_mode_addfb2(dev, &cmd, file_priv); + KUNIT_EXPECT_EQ(test, ret, -EINVAL); + KUNIT_ASSERT_PTR_EQ(test, priv->private, NULL); + cmd.width = 600; /* restore to a valid value */ + + ret = drm_mode_addfb2(dev, &cmd, file_priv); + KUNIT_EXPECT_EQ(test, ret, 0); + + /* The fb_create_addfb2_mock set fb id to 1 */ + KUNIT_EXPECT_EQ(test, cmd.fb_id, 1); + + fb = priv->private; + + /* The fb must be properly added to the file_priv->fbs list */ + KUNIT_EXPECT_PTR_EQ(test, file_priv->fbs.prev, &fb->filp_head); + KUNIT_EXPECT_PTR_EQ(test, file_priv->fbs.next, &fb->filp_head); + + /* There must be just one fb on the list */ + KUNIT_EXPECT_PTR_EQ(test, fb->filp_head.prev, &file_priv->fbs); + KUNIT_EXPECT_PTR_EQ(test, fb->filp_head.next, &file_priv->fbs); +} + static struct kunit_case drm_framebuffer_tests[] = { + KUNIT_CASE(drm_test_framebuffer_addfb2), KUNIT_CASE_PARAM(drm_test_framebuffer_check_src_coords, check_src_coords_gen_params), KUNIT_CASE(drm_test_framebuffer_cleanup), KUNIT_CASE_PARAM(drm_test_framebuffer_create, drm_framebuffer_create_gen_params), @@ -664,6 +765,7 @@ static struct kunit_case drm_framebuffer_tests[] = { static struct kunit_suite drm_framebuffer_test_suite = { .name = "drm_framebuffer", .init = drm_framebuffer_test_init, + .exit = drm_framebuffer_test_exit, .test_cases = drm_framebuffer_tests, };
Add a single KUnit test case for the drm_mode_addfb2 function. Signed-off-by: Carlos Eduardo Gallo Filho <gcarlos@disroot.org> --- v2: - Reorder kunit cases alphabetically. - Rely on drm_kunit_helper_alloc_device() for mock initialization. --- drivers/gpu/drm/tests/drm_framebuffer_test.c | 104 ++++++++++++++++++- 1 file changed, 103 insertions(+), 1 deletion(-)