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@xxxxxxxxxxx>
> ---
> 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
>
Attachment:
signature.asc
Description: PGP signature
