[PATCH v2 10/11] drm/tests: Add test for drm_mode_addfb2()

[Maxime Ripard]

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 at 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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20231025/e5d33630/attachment.sig>