[PATCH v2 00/11] Increase coverage on drm_framebuffer.c

[PATCH v2 00/11] Increase coverage on drm_framebuffer.c

[Carlos Eduardo Gallo Filho]

This patchset includes new KUnit tests for 7 untested functions from
drm_framebuffer.c and improvements to the existent one.

The first patch replace the use of dev_private member from drm_device
mock on the existent test by embedding it into an outer struct containing
a generic pointer.

The patches 2 and 4 extends the test of drm_internal_framebuffer_create()
by creating a new test case and adding new parameters to the existent case.

The patch 3 just replace a strcpy() call to strscpy().

Finally, the remainder of this set contains 7 new test cases, one for each
of the follow functions:

- drm_framebuffer_check_src_coords()
- drm_framebuffer_cleanup()
- drm_framebuffer_lookup()
- drm_framebuffer_init()
- drm_framebuffer_free()
- drm_mode_addfb2()
- drm_fb_release()

Signed-off-by: Carlos Eduardo Gallo Filho <gcarlos@disroot.org>
---
v2:
  - Split strcpy to strscpy change on drm_test_framebuffer_create test
  into a separate patch [PATCH 03/11].
  - Extensively use drm_kunit_helper_alloc_drm_device() for drm_device mock.
  - Reorder kunit cases alphabetically.
  - Rename check_src_coords_case.
  - Remove unnecessary comments.
  - Add framebuffer size as a parameter on drm_framebuffer_check_src_coords
  case.
  - Replace drm_mode_object_add() call to drm_framebuffer_init() on
  drm_framebuffer_lookup() test.
  - Let fb1.dev unset instead of set it to wrong_drm to test mismatched
  drm_device passed as drm_framebuffer_init() argument on
  drm_framebuffer_init() test.
  - Clean the framebuffer object on drm_framebuffer_init_test.
---
Carlos Eduardo Gallo Filho (11):
  drm/tests: Stop using deprecated dev_private member on drm_framebuffer
    tests
  drm/tests: Add parameters to the drm_test_framebuffer_create test
  drm/tests: Replace strcpy to strscpy on drm_test_framebuffer_create
    test
  drm/tests: Add test case for drm_internal_framebuffer_create()
  drm/tests: Add test for drm_framebuffer_check_src_coords()
  drm/tests: Add test for drm_framebuffer_cleanup()
  drm/tests: Add test for drm_framebuffer_lookup()
  drm/tests: Add test for drm_framebuffer_init()
  drm/tests: Add test for drm_framebuffer_free()
  drm/tests: Add test for drm_mode_addfb2()
  drm/tests: Add test for drm_fb_release()

 drivers/gpu/drm/drm_framebuffer.c            |   1 +
 drivers/gpu/drm/tests/drm_framebuffer_test.c | 562 ++++++++++++++++++-
 2 files changed, 549 insertions(+), 14 deletions(-)

-- 
2.41.0

[PATCH v2 01/11] drm/tests: Stop using deprecated dev_private member on drm_framebuffer tests

[Carlos Eduardo Gallo Filho]

The dev_private member of drm_device is deprecated and its use should
be avoided. Stop using it by embedding the drm_device onto a mock struct
with a void pointer like dev_private, using it instead.

Also start using drm_kunit_helper_alloc_drm_device() for allocating
the drm_device mock.

Signed-off-by: Carlos Eduardo Gallo Filho <gcarlos@disroot.org>
---
v2:
  - Start using drm_kunit_helper_alloc_drm_device() for drm_device mock.
  - Rename struct drm_mock to drm_framebuffer_test_priv
---
 drivers/gpu/drm/tests/drm_framebuffer_test.c | 42 ++++++++++++++------
 1 file changed, 29 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c b/drivers/gpu/drm/tests/drm_framebuffer_test.c
index f759d9f3b76e..9c6170edd5f7 100644
--- a/drivers/gpu/drm/tests/drm_framebuffer_test.c
+++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c
@@ -8,8 +8,10 @@
 #include <kunit/test.h>
 
 #include <drm/drm_device.h>
+#include <drm/drm_drv.h>
 #include <drm/drm_mode.h>
 #include <drm/drm_fourcc.h>
+#include <drm/drm_kunit_helpers.h>
 #include <drm/drm_print.h>
 
 #include "../drm_crtc_internal.h"
@@ -317,11 +319,17 @@ static const struct drm_framebuffer_test drm_framebuffer_create_cases[] = {
 },
 };
 
+struct drm_framebuffer_test_priv {
+	struct drm_device dev;
+	void *private;
+};
+
 static struct drm_framebuffer *fb_create_mock(struct drm_device *dev,
 					      struct drm_file *file_priv,
 					      const struct drm_mode_fb_cmd2 *mode_cmd)
 {
-	int *buffer_created = dev->dev_private;
+	struct drm_framebuffer_test_priv *priv = container_of(dev, typeof(*priv), dev);
+	int *buffer_created = priv->private;
 	*buffer_created = 1;
 	return ERR_PTR(-EINVAL);
 }
@@ -332,29 +340,37 @@ static struct drm_mode_config_funcs mock_config_funcs = {
 
 static int drm_framebuffer_test_init(struct kunit *test)
 {
-	struct drm_device *mock;
+	struct device *parent;
+	struct drm_framebuffer_test_priv *priv;
+	struct drm_device *dev;
+
+	parent = drm_kunit_helper_alloc_device(test);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
 
-	mock = kunit_kzalloc(test, sizeof(*mock), GFP_KERNEL);
-	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mock);
+	priv = drm_kunit_helper_alloc_drm_device(test, parent, typeof(*priv),
+						 dev, 0);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+	dev = &priv->dev;
 
-	mock->mode_config.min_width = MIN_WIDTH;
-	mock->mode_config.max_width = MAX_WIDTH;
-	mock->mode_config.min_height = MIN_HEIGHT;
-	mock->mode_config.max_height = MAX_HEIGHT;
-	mock->mode_config.funcs = &mock_config_funcs;
+	dev->mode_config.min_width = MIN_WIDTH;
+	dev->mode_config.max_width = MAX_WIDTH;
+	dev->mode_config.min_height = MIN_HEIGHT;
+	dev->mode_config.max_height = MAX_HEIGHT;
+	dev->mode_config.funcs = &mock_config_funcs;
 
-	test->priv = mock;
+	test->priv = priv;
 	return 0;
 }
 
 static void drm_test_framebuffer_create(struct kunit *test)
 {
 	const struct drm_framebuffer_test *params = test->param_value;
-	struct drm_device *mock = test->priv;
+	struct drm_framebuffer_test_priv *priv = test->priv;
+	struct drm_device *dev = &priv->dev;
 	int buffer_created = 0;
 
-	mock->dev_private = &buffer_created;
-	drm_internal_framebuffer_create(mock, &params->cmd, NULL);
+	priv->private = &buffer_created;
+	drm_internal_framebuffer_create(dev, &params->cmd, NULL);
 	KUNIT_EXPECT_EQ(test, params->buffer_created, buffer_created);
 }
 
-- 
2.41.0

[PATCH v2 02/11] drm/tests: Add parameters to the drm_test_framebuffer_create test

[Carlos Eduardo Gallo Filho]

Extend the existing test case to cover:
1. Invalid flag atribute in the struct drm_mode_fb_cmd2.
2. Pixel format which requires non-linear modifier with
DRM_FORMAT_MOD_LINEAR set.
3. Non-zero buffer offset for an unused plane

Also replace strcpy to strscpy on test_to_desc for improved security
and reliability.

Signed-off-by: Carlos Eduardo Gallo Filho <gcarlos@disroot.org>
---
v2:
  - Remove strcpy to strscpy change.
---
 drivers/gpu/drm/tests/drm_framebuffer_test.c | 21 ++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c b/drivers/gpu/drm/tests/drm_framebuffer_test.c
index 9c6170edd5f7..659cbd5a3be3 100644
--- a/drivers/gpu/drm/tests/drm_framebuffer_test.c
+++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c
@@ -21,6 +21,8 @@
 #define MIN_HEIGHT 4
 #define MAX_HEIGHT 4096
 
+#define DRM_MODE_FB_INVALID BIT(2)
+
 struct drm_framebuffer_test {
 	int buffer_created;
 	struct drm_mode_fb_cmd2 cmd;
@@ -85,6 +87,18 @@ static const struct drm_framebuffer_test drm_framebuffer_create_cases[] = {
 		 .pitches = { 4 * MAX_WIDTH, 0, 0 },
 	}
 },
+{ .buffer_created = 0, .name = "ABGR8888 Non-zero buffer offset for unused plane",
+	.cmd = { .width = MAX_WIDTH, .height = MAX_HEIGHT, .pixel_format = DRM_FORMAT_ABGR8888,
+		 .handles = { 1, 0, 0 }, .offsets = { UINT_MAX / 2, UINT_MAX / 2, 0 },
+		 .pitches = { 4 * MAX_WIDTH, 0, 0 }, .flags = DRM_MODE_FB_MODIFIERS,
+	}
+},
+{ .buffer_created = 0, .name = "ABGR8888 Invalid flag",
+	.cmd = { .width = MAX_WIDTH, .height = MAX_HEIGHT, .pixel_format = DRM_FORMAT_ABGR8888,
+		 .handles = { 1, 0, 0 }, .offsets = { UINT_MAX / 2, 0, 0 },
+		 .pitches = { 4 * MAX_WIDTH, 0, 0 }, .flags = DRM_MODE_FB_INVALID,
+	}
+},
 { .buffer_created = 1, .name = "ABGR8888 Set DRM_MODE_FB_MODIFIERS without modifiers",
 	.cmd = { .width = MAX_WIDTH, .height = MAX_HEIGHT, .pixel_format = DRM_FORMAT_ABGR8888,
 		 .handles = { 1, 0, 0 }, .offsets = { UINT_MAX / 2, 0, 0 },
@@ -264,6 +278,13 @@ static const struct drm_framebuffer_test drm_framebuffer_create_cases[] = {
 		 .pitches = { MAX_WIDTH, DIV_ROUND_UP(MAX_WIDTH, 2), DIV_ROUND_UP(MAX_WIDTH, 2) },
 	}
 },
+{ .buffer_created = 0, .name = "YUV420_10BIT Invalid modifier(DRM_FORMAT_MOD_LINEAR)",
+	.cmd = { .width = MAX_WIDTH, .height = MAX_HEIGHT, .pixel_format = DRM_FORMAT_YUV420_10BIT,
+		 .handles = { 1, 0, 0 }, .flags = DRM_MODE_FB_MODIFIERS,
+		 .modifier = { DRM_FORMAT_MOD_LINEAR, 0, 0 },
+		 .pitches = { MAX_WIDTH, 0, 0 },
+	}
+},
 { .buffer_created = 1, .name = "X0L2 Normal sizes",
 	.cmd = { .width = 600, .height = 600, .pixel_format = DRM_FORMAT_X0L2,
 		 .handles = { 1, 0, 0 }, .pitches = { 1200, 0, 0 }
-- 
2.41.0

[PATCH v2 03/11] drm/tests: Replace strcpy to strscpy on drm_test_framebuffer_create test

[Carlos Eduardo Gallo Filho]

Replace the use of strcpy to strscpy on the test_to_desc of the
drm_test_framebuffer_create test for better security and reliability.

Signed-off-by: Carlos Eduardo Gallo Filho <gcarlos@disroot.org>
---
 drivers/gpu/drm/tests/drm_framebuffer_test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c b/drivers/gpu/drm/tests/drm_framebuffer_test.c
index 659cbd5a3be3..eb76a71644e9 100644
--- a/drivers/gpu/drm/tests/drm_framebuffer_test.c
+++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c
@@ -397,7 +397,7 @@ static void drm_test_framebuffer_create(struct kunit *test)
 
 static void drm_framebuffer_test_to_desc(const struct drm_framebuffer_test *t, char *desc)
 {
-	strcpy(desc, t->name);
+	strscpy(desc, t->name, KUNIT_PARAM_DESC_SIZE);
 }
 
 KUNIT_ARRAY_PARAM(drm_framebuffer_create, drm_framebuffer_create_cases,
-- 
2.41.0

[PATCH v2 04/11] drm/tests: Add test case for drm_internal_framebuffer_create()

[Carlos Eduardo Gallo Filho]

Introduce a test to cover the creation of framebuffer with
modifier on a device that doesn't support it.

Signed-off-by: Carlos Eduardo Gallo Filho <gcarlos@disroot.org>
---
v2:
  - Reorder kunit cases alphabetically.
---
 drivers/gpu/drm/tests/drm_framebuffer_test.c | 28 ++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c b/drivers/gpu/drm/tests/drm_framebuffer_test.c
index eb76a71644e9..8a9b6d08d639 100644
--- a/drivers/gpu/drm/tests/drm_framebuffer_test.c
+++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c
@@ -403,8 +403,36 @@ static void drm_framebuffer_test_to_desc(const struct drm_framebuffer_test *t, c
 KUNIT_ARRAY_PARAM(drm_framebuffer_create, drm_framebuffer_create_cases,
 		  drm_framebuffer_test_to_desc);
 
+/*
+ * This test is very similar to drm_test_framebuffer_create, except that it
+ * set dev->mode_config.fb_modifiers_not_supported member to 1, covering
+ * the case of trying to create a framebuffer with modifiers without the
+ * device really supporting it.
+ */
+static void drm_test_framebuffer_modifiers_not_supported(struct kunit *test)
+{
+	struct drm_framebuffer_test_priv *priv = test->priv;
+	struct drm_device *dev = &priv->dev;
+	int buffer_created = 0;
+
+	/* A valid cmd with modifier */
+	struct drm_mode_fb_cmd2 cmd = {
+		.width = MAX_WIDTH, .height = MAX_HEIGHT,
+		.pixel_format = DRM_FORMAT_ABGR8888, .handles = { 1, 0, 0 },
+		.offsets = { UINT_MAX / 2, 0, 0 }, .pitches = { 4 * MAX_WIDTH, 0, 0 },
+		.flags = DRM_MODE_FB_MODIFIERS,
+	};
+
+	priv->private = &buffer_created;
+	dev->mode_config.fb_modifiers_not_supported = 1;
+
+	drm_internal_framebuffer_create(dev, &cmd, NULL);
+	KUNIT_EXPECT_EQ(test, 0, buffer_created);
+}
+
 static struct kunit_case drm_framebuffer_tests[] = {
 	KUNIT_CASE_PARAM(drm_test_framebuffer_create, drm_framebuffer_create_gen_params),
+	KUNIT_CASE(drm_test_framebuffer_modifiers_not_supported),
 	{ }
 };
 
-- 
2.41.0

[PATCH v2 05/11] drm/tests: Add test for drm_framebuffer_check_src_coords()

[Carlos Eduardo Gallo Filho]

Add a parametrized test for the drm_framebuffer_check_src_coords function.

Signed-off-by: Carlos Eduardo Gallo Filho <gcarlos@disroot.org>
---
v2:
  - Order kunit cases alphabetically.
  - Rename check_src_coords_case to drm_framebuffer_check_src_coords_case.
  - Remove unnecessary comments.
  - Add framebuffer size as a parameter and use edge values.
---
 drivers/gpu/drm/drm_framebuffer.c            |  1 +
 drivers/gpu/drm/tests/drm_framebuffer_test.c | 61 ++++++++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index d3ba0698b84b..a4b264e4ac97 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -99,6 +99,7 @@ int drm_framebuffer_check_src_coords(uint32_t src_x, uint32_t src_y,
 
 	return 0;
 }
+EXPORT_SYMBOL_FOR_TESTS_ONLY(drm_framebuffer_check_src_coords);
 
 /**
  * drm_mode_addfb - add an FB to the graphics configuration
diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c b/drivers/gpu/drm/tests/drm_framebuffer_test.c
index 8a9b6d08d639..7b51862fb0f2 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_framebuffer.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_kunit_helpers.h>
 #include <drm/drm_print.h>
@@ -430,7 +431,67 @@ static void drm_test_framebuffer_modifiers_not_supported(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, 0, buffer_created);
 }
 
+/* Parameters for testing drm_framebuffer_check_src_coords function */
+struct drm_framebuffer_check_src_coords_case {
+	const char *name;
+	const int expect;
+	const unsigned int fb_size;
+	const uint32_t src_x;
+	const uint32_t src_y;
+
+	/* Deltas to be applied on source */
+	const uint32_t dsrc_w;
+	const uint32_t dsrc_h;
+};
+
+static const struct drm_framebuffer_check_src_coords_case
+drm_framebuffer_check_src_coords_cases[] = {
+	{ .name = "Success: source fits into fb",
+	  .expect = 0,
+	},
+	{ .name = "Fail: overflowing fb with x-axis coordinate",
+	  .expect = -ENOSPC, .src_x = 1, .fb_size = UINT_MAX,
+	},
+	{ .name = "Fail: overflowing fb with y-axis coordinate",
+	  .expect = -ENOSPC, .src_y = 1, .fb_size = UINT_MAX,
+	},
+	{ .name = "Fail: overflowing fb with source width",
+	  .expect = -ENOSPC, .dsrc_w = 1, .fb_size = UINT_MAX - 1,
+	},
+	{ .name = "Fail: overflowing fb with source height",
+	  .expect = -ENOSPC, .dsrc_h = 1, .fb_size = UINT_MAX - 1,
+	},
+};
+
+static void drm_test_framebuffer_check_src_coords(struct kunit *test)
+{
+	const struct drm_framebuffer_check_src_coords_case *params = test->param_value;
+	const uint32_t src_x = params->src_x;
+	const uint32_t src_y = params->src_y;
+	const uint32_t src_w = (params->fb_size << 16) + params->dsrc_w;
+	const uint32_t src_h = (params->fb_size << 16) + params->dsrc_h;
+	const struct drm_framebuffer fb = {
+		.width  = params->fb_size,
+		.height = params->fb_size
+	};
+	int ret;
+
+	ret = drm_framebuffer_check_src_coords(src_x, src_y, src_w, src_h, &fb);
+	KUNIT_EXPECT_EQ(test, ret, params->expect);
+}
+
+static void
+check_src_coords_test_to_desc(const struct drm_framebuffer_check_src_coords_case *t,
+			      char *desc)
+{
+	strscpy(desc, t->name, KUNIT_PARAM_DESC_SIZE);
+}
+
+KUNIT_ARRAY_PARAM(check_src_coords, drm_framebuffer_check_src_coords_cases,
+		  check_src_coords_test_to_desc);
+
 static struct kunit_case drm_framebuffer_tests[] = {
+	KUNIT_CASE_PARAM(drm_test_framebuffer_check_src_coords, check_src_coords_gen_params),
 	KUNIT_CASE_PARAM(drm_test_framebuffer_create, drm_framebuffer_create_gen_params),
 	KUNIT_CASE(drm_test_framebuffer_modifiers_not_supported),
 	{ }
-- 
2.41.0

[PATCH v2 06/11] drm/tests: Add test for drm_framebuffer_cleanup()

[Carlos Eduardo Gallo Filho]

Add a single KUnit test case for the drm_framebuffer_cleanup 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 | 37 ++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c b/drivers/gpu/drm/tests/drm_framebuffer_test.c
index 7b51862fb0f2..a63f30985f75 100644
--- a/drivers/gpu/drm/tests/drm_framebuffer_test.c
+++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c
@@ -490,8 +490,45 @@ check_src_coords_test_to_desc(const struct drm_framebuffer_check_src_coords_case
 KUNIT_ARRAY_PARAM(check_src_coords, drm_framebuffer_check_src_coords_cases,
 		  check_src_coords_test_to_desc);
 
+static void drm_test_framebuffer_cleanup(struct kunit *test)
+{
+	struct drm_framebuffer_test_priv *priv = test->priv;
+	struct drm_device *dev = &priv->dev;
+	struct list_head *fb_list = &dev->mode_config.fb_list;
+	struct drm_framebuffer fb1 = { .dev = dev };
+	struct drm_framebuffer fb2 = { .dev = dev };
+
+	/* This must result on [fb_list] -> fb1 -> fb2 */
+	list_add_tail(&fb1.head, fb_list);
+	list_add_tail(&fb2.head, fb_list);
+	dev->mode_config.num_fb = 2;
+
+	KUNIT_ASSERT_PTR_EQ(test, fb_list->prev, &fb2.head);
+	KUNIT_ASSERT_PTR_EQ(test, fb_list->next, &fb1.head);
+	KUNIT_ASSERT_PTR_EQ(test, fb1.head.prev, fb_list);
+	KUNIT_ASSERT_PTR_EQ(test, fb1.head.next, &fb2.head);
+	KUNIT_ASSERT_PTR_EQ(test, fb2.head.prev, &fb1.head);
+	KUNIT_ASSERT_PTR_EQ(test, fb2.head.next, fb_list);
+
+	drm_framebuffer_cleanup(&fb1);
+
+	/* Now [fb_list] -> fb2 */
+	KUNIT_ASSERT_PTR_EQ(test, fb_list->prev, &fb2.head);
+	KUNIT_ASSERT_PTR_EQ(test, fb_list->next, &fb2.head);
+	KUNIT_ASSERT_PTR_EQ(test, fb2.head.prev, fb_list);
+	KUNIT_ASSERT_PTR_EQ(test, fb2.head.next, fb_list);
+	KUNIT_ASSERT_EQ(test, dev->mode_config.num_fb, 1);
+
+	drm_framebuffer_cleanup(&fb2);
+
+	/* Now fb_list is empty */
+	KUNIT_ASSERT_TRUE(test, list_empty(fb_list));
+	KUNIT_ASSERT_EQ(test, dev->mode_config.num_fb, 0);
+}
+
 static struct kunit_case drm_framebuffer_tests[] = {
 	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),
 	KUNIT_CASE(drm_test_framebuffer_modifiers_not_supported),
 	{ }
-- 
2.41.0

[PATCH v2 07/11] drm/tests: Add test for drm_framebuffer_lookup()

[Carlos Eduardo Gallo Filho]

Add a single KUnit test case for the drm_framebuffer_lookup function.

Signed-off-by: Carlos Eduardo Gallo Filho <gcarlos@disroot.org>
---
v2:
  - Reorder kunit cases alphabetically.
  - Replace drm_mode_object_add() call to drm_framebuffer_init().
  - Rely on drm_kunit_helper_alloc_device() for mock initialization.
---
 drivers/gpu/drm/tests/drm_framebuffer_test.c | 26 ++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c b/drivers/gpu/drm/tests/drm_framebuffer_test.c
index a63f30985f75..fb9589dd8aed 100644
--- a/drivers/gpu/drm/tests/drm_framebuffer_test.c
+++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c
@@ -526,10 +526,36 @@ static void drm_test_framebuffer_cleanup(struct kunit *test)
 	KUNIT_ASSERT_EQ(test, dev->mode_config.num_fb, 0);
 }
 
+static void drm_test_framebuffer_lookup(struct kunit *test)
+{
+	struct drm_framebuffer_test_priv *priv = test->priv;
+	struct drm_device *dev = &priv->dev;
+	struct drm_format_info format = { };
+	struct drm_framebuffer fb1 = { .dev = dev, .format = &format };
+	struct drm_framebuffer *fb2;
+	uint32_t id = 0;
+	int ret;
+
+	ret = drm_framebuffer_init(dev, &fb1, NULL);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+	id = fb1.base.id;
+
+	/* Looking for fb1 */
+	fb2 = drm_framebuffer_lookup(dev, NULL, id);
+	KUNIT_EXPECT_PTR_EQ(test, fb2, &fb1);
+
+	/* Looking for an inexistent framebuffer */
+	fb2 = drm_framebuffer_lookup(dev, NULL, id + 1);
+	KUNIT_EXPECT_NULL(test, fb2);
+
+	drm_framebuffer_cleanup(&fb1);
+}
+
 static struct kunit_case drm_framebuffer_tests[] = {
 	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),
+	KUNIT_CASE(drm_test_framebuffer_lookup),
 	KUNIT_CASE(drm_test_framebuffer_modifiers_not_supported),
 	{ }
 };
-- 
2.41.0

[PATCH v2 08/11] drm/tests: Add test for drm_framebuffer_init()

[Carlos Eduardo Gallo Filho]

Add a single KUnit test case for the drm_framebuffer_init function.

Signed-off-by: Carlos Eduardo Gallo Filho <gcarlos@disroot.org>
---
v2:
  - Reorder kunit cases alphabetically.
  - Let fb1.dev unset instead of set it to wrong_drm to test mismatched
    drm_device passed as drm_framebuffer_init() argument.
  - Clean the framebuffer object.
---
 drivers/gpu/drm/tests/drm_framebuffer_test.c | 52 ++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c b/drivers/gpu/drm/tests/drm_framebuffer_test.c
index fb9589dd8aed..eedd5e920279 100644
--- a/drivers/gpu/drm/tests/drm_framebuffer_test.c
+++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c
@@ -551,10 +551,62 @@ static void drm_test_framebuffer_lookup(struct kunit *test)
 	drm_framebuffer_cleanup(&fb1);
 }
 
+static void drm_test_framebuffer_init(struct kunit *test)
+{
+	struct drm_framebuffer_test_priv *priv = test->priv;
+	struct drm_device *dev = &priv->dev;
+	struct drm_format_info format = { };
+	struct drm_framebuffer fb1 = { .format = &format };
+	struct drm_framebuffer *fb2;
+	struct drm_framebuffer_funcs funcs = { };
+	int ret;
+
+	/* Fails if fb->dev doesn't point to the drm_device passed on first arg */
+	ret = drm_framebuffer_init(dev, &fb1, &funcs);
+	KUNIT_ASSERT_EQ(test, ret, -EINVAL);
+	fb1.dev = dev;
+
+	/* Fails if fb.format isn't set */
+	fb1.format = NULL;
+	ret = drm_framebuffer_init(dev, &fb1, &funcs);
+	KUNIT_ASSERT_EQ(test, ret, -EINVAL);
+	fb1.format = &format;
+
+	ret = drm_framebuffer_init(dev, &fb1, &funcs);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	/*
+	 * Check if fb->funcs is actually set to the drm_framebuffer_funcs
+	 * passed to it
+	 */
+	KUNIT_EXPECT_PTR_EQ(test, fb1.funcs, &funcs);
+
+	/* The fb->comm must be set to the current running process */
+	KUNIT_EXPECT_STREQ(test, fb1.comm, current->comm);
+
+	/* The fb->base must be successfully initialized */
+	KUNIT_EXPECT_NE(test, fb1.base.id, 0);
+	KUNIT_EXPECT_EQ(test, fb1.base.type, DRM_MODE_OBJECT_FB);
+	KUNIT_EXPECT_EQ(test, kref_read(&fb1.base.refcount), 1);
+	KUNIT_EXPECT_PTR_EQ(test, fb1.base.free_cb, &drm_framebuffer_free);
+
+	/* Checks if the fb is really published and findable */
+	fb2 = drm_framebuffer_lookup(dev, NULL, fb1.base.id);
+	KUNIT_EXPECT_PTR_EQ(test, fb2, &fb1);
+
+	/* There must be just that one fb initialized */
+	KUNIT_EXPECT_EQ(test, dev->mode_config.num_fb, 1);
+	KUNIT_EXPECT_PTR_EQ(test, dev->mode_config.fb_list.prev, &fb1.head);
+	KUNIT_EXPECT_PTR_EQ(test, dev->mode_config.fb_list.next, &fb1.head);
+
+	drm_framebuffer_cleanup(&fb1);
+}
+
 static struct kunit_case drm_framebuffer_tests[] = {
 	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),
+	KUNIT_CASE(drm_test_framebuffer_init),
 	KUNIT_CASE(drm_test_framebuffer_lookup),
 	KUNIT_CASE(drm_test_framebuffer_modifiers_not_supported),
 	{ }
-- 
2.41.0

[PATCH v2 09/11] drm/tests: Add test for drm_framebuffer_free()

[Carlos Eduardo Gallo Filho]

Add a single KUnit test case for the drm_framebuffer_free function.

Signed-off-by: Carlos Eduardo Gallo Filho <gcarlos@disroot.org>
---
v2:
  - Reorder kunit cases alphabetically.
---
 drivers/gpu/drm/tests/drm_framebuffer_test.c | 49 ++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c b/drivers/gpu/drm/tests/drm_framebuffer_test.c
index eedd5e920279..dbe412d0dca4 100644
--- a/drivers/gpu/drm/tests/drm_framebuffer_test.c
+++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c
@@ -602,10 +602,59 @@ static void drm_test_framebuffer_init(struct kunit *test)
 	drm_framebuffer_cleanup(&fb1);
 }
 
+static void destroy_free_mock(struct drm_framebuffer *fb)
+{
+	struct drm_framebuffer_test_priv *priv = container_of(fb->dev, typeof(*priv), dev);
+	int *buffer_freed = priv->private;
+	*buffer_freed = 1;
+}
+
+static struct drm_framebuffer_funcs framebuffer_funcs_free_mock = {
+	.destroy = destroy_free_mock,
+};
+
+static void drm_test_framebuffer_free(struct kunit *test)
+{
+	struct drm_framebuffer_test_priv *priv = test->priv;
+	struct drm_device *dev = &priv->dev;
+	struct drm_mode_object *obj;
+	struct drm_framebuffer fb = {
+		.dev = dev,
+		.funcs = &framebuffer_funcs_free_mock,
+	};
+	int buffer_freed = 0;
+	int id, ret;
+
+	priv->private = &buffer_freed;
+
+	/*
+	 * Case where the fb isn't registered. Just test if
+	 * drm_framebuffer_free calls fb->funcs->destroy
+	 */
+	drm_framebuffer_free(&fb.base.refcount);
+	KUNIT_EXPECT_EQ(test, buffer_freed, 1);
+
+	buffer_freed = 0;
+
+	ret = drm_mode_object_add(dev, &fb.base, DRM_MODE_OBJECT_FB);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+	id = fb.base.id;
+
+	/* Now, test with the fb registered, that must end unregistered */
+	drm_framebuffer_free(&fb.base.refcount);
+	KUNIT_EXPECT_EQ(test, fb.base.id, 0);
+	KUNIT_EXPECT_EQ(test, buffer_freed, 1);
+
+	/* Test if the old id of the fb was really removed from the idr pool */
+	obj = drm_mode_object_find(dev, NULL, id, DRM_MODE_OBJECT_FB);
+	KUNIT_EXPECT_PTR_EQ(test, obj, NULL);
+}
+
 static struct kunit_case drm_framebuffer_tests[] = {
 	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),
+	KUNIT_CASE(drm_test_framebuffer_free),
 	KUNIT_CASE(drm_test_framebuffer_init),
 	KUNIT_CASE(drm_test_framebuffer_lookup),
 	KUNIT_CASE(drm_test_framebuffer_modifiers_not_supported),
-- 
2.41.0

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

[Carlos Eduardo Gallo Filho]

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);
+
 	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,
 };
 
-- 
2.41.0

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

[Carlos Eduardo Gallo Filho]

Add a single KUnit test case for the drm_fb_release function, which also
implicitly test the static legacy_remove_fb function.

Signed-off-by: Carlos Eduardo Gallo Filho <gcarlos@disroot.org>
---
v2:
  - Rely on drm_kunit_helper_alloc_device() for mock initialization.
---
 drivers/gpu/drm/tests/drm_framebuffer_test.c | 142 +++++++++++++++++++
 1 file changed, 142 insertions(+)

diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c b/drivers/gpu/drm/tests/drm_framebuffer_test.c
index 149e1985e53f..70b14e05dc83 100644
--- a/drivers/gpu/drm/tests/drm_framebuffer_test.c
+++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c
@@ -7,6 +7,7 @@
 
 #include <kunit/test.h>
 
+#include <drm/drm_crtc.h>
 #include <drm/drm_device.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_mode.h>
@@ -750,7 +751,148 @@ static void drm_test_framebuffer_addfb2(struct kunit *test)
 	KUNIT_EXPECT_PTR_EQ(test, fb->filp_head.next, &file_priv->fbs);
 }
 
+static void drm_framebuffer_fb_release_remove_mock(struct kref *kref)
+{
+	struct drm_framebuffer *fb = container_of(kref, typeof(*fb), base.refcount);
+	struct drm_framebuffer_test_priv *priv = container_of(fb->dev, typeof(*priv), dev);
+	bool *obj_released = priv->private;
+
+	obj_released[fb->base.id - 1] = true;
+}
+
+static int crtc_set_config_fb_release_mock(struct drm_mode_set *set,
+					   struct drm_modeset_acquire_ctx *ctx)
+{
+	struct drm_crtc *crtc = set->crtc;
+	struct drm_framebuffer_test_priv *priv = container_of(crtc->dev, typeof(*priv), dev);
+	bool *obj_released = priv->private;
+
+	obj_released[crtc->base.id - 1] = true;
+	obj_released[crtc->primary->base.id - 1] = true;
+	return 0;
+}
+
+static int disable_plane_fb_release_mock(struct drm_plane *plane,
+					 struct drm_modeset_acquire_ctx *ctx)
+{
+	struct drm_framebuffer_test_priv *priv = container_of(plane->dev, typeof(*priv), dev);
+	bool *obj_released = priv->private;
+
+	obj_released[plane->base.id - 1] = true;
+	return 0;
+}
+
+#define NUM_OBJS 5
+
+/*
+ * The drm_fb_release function implicitly calls at some point the
+ * drm_framebuffer_remove, which actually removes framebuffers
+ * based on the driver supporting or not the atomic API. To simplify
+ * this test, let it rely on legacy removing and leave the atomic remove
+ * to be tested in another test case. By doing that, we can also test
+ * the legacy_remove_fb function entirely.
+ */
+static void drm_test_fb_release(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_plane_funcs plane_funcs = {
+		.disable_plane = disable_plane_fb_release_mock
+	};
+	struct drm_crtc_funcs crtc_funcs = {
+		.set_config = crtc_set_config_fb_release_mock
+	};
+	struct drm_framebuffer *fb1, *fb2;
+	struct drm_plane *plane1, *plane2;
+	struct drm_crtc *crtc;
+	bool *obj_released;
+
+	/*
+	 * obj_released[i] where "i" is the obj.base.id - 1. Note that the
+	 * "released" word means different things for each kind of obj, which
+	 * in case of a framebuffer, means that it was freed, while for the
+	 * crtc and plane, means that it was deactivated.
+	 */
+	obj_released = kunit_kzalloc(test, NUM_OBJS * sizeof(bool), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, obj_released);
+	priv->private = obj_released;
+
+	fb1 = kunit_kzalloc(test, sizeof(*fb1), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fb1);
+	list_add(&fb1->filp_head, &file_priv->fbs);
+	kref_init(&fb1->base.refcount);
+	fb1->dev = dev;
+	fb1->base.free_cb = drm_framebuffer_fb_release_remove_mock;
+	fb1->base.id = 1;
+
+	fb2 = kunit_kzalloc(test, sizeof(*fb2), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fb2);
+	list_add(&fb2->filp_head, &file_priv->fbs);
+	kref_init(&fb2->base.refcount);
+	fb2->dev = dev;
+	fb2->base.free_cb = drm_framebuffer_fb_release_remove_mock;
+	fb2->base.id = 2;
+
+	plane1 = kunit_kzalloc(test, sizeof(*plane1), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, plane1);
+	list_add(&plane1->head, &dev->mode_config.plane_list);
+	drm_modeset_lock_init(&plane1->mutex);
+	plane1->dev = dev;
+	plane1->funcs = &plane_funcs;
+	plane1->base.id = 3;
+
+	plane2 = kunit_kzalloc(test, sizeof(*plane2), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, plane2);
+	list_add(&plane2->head, &dev->mode_config.plane_list);
+	drm_modeset_lock_init(&plane2->mutex);
+	plane2->dev = dev;
+	plane2->funcs = &plane_funcs;
+	plane2->base.id = 4;
+
+	crtc = kunit_kzalloc(test, sizeof(*crtc), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc);
+	list_add(&crtc->head, &dev->mode_config.crtc_list);
+	drm_modeset_lock_init(&crtc->mutex);
+	crtc->dev = dev;
+	crtc->funcs = &crtc_funcs;
+	crtc->base.id = 5;
+
+	/*
+	 * Attach fb2 to some planes to stress the case where we have more than
+	 * one reference to the fb. plane1 is attached to crtc as primary plane
+	 * and plane2 will represent any non-primary plane, allowing to cover
+	 * all codepaths on legacy_remove_fb
+	 */
+	crtc->primary = plane1;
+	plane1->crtc = crtc;
+	plane1->fb = fb2;
+	plane2->fb = fb2;
+	/* Each plane holds one reference to fb */
+	drm_framebuffer_get(fb2);
+	drm_framebuffer_get(fb2);
+
+	drm_fb_release(file_priv);
+
+	KUNIT_EXPECT_TRUE(test, list_empty(&file_priv->fbs));
+
+	/* Every object from this test should be released */
+	for (int i = 0; i < 5; i++)
+		KUNIT_EXPECT_EQ(test, obj_released[i], 1);
+
+	KUNIT_EXPECT_FALSE(test, kref_read(&fb1->base.refcount));
+	KUNIT_EXPECT_FALSE(test, kref_read(&fb2->base.refcount));
+
+	KUNIT_EXPECT_PTR_EQ(test, plane1->crtc, NULL);
+	KUNIT_EXPECT_PTR_EQ(test, plane1->fb, NULL);
+	KUNIT_EXPECT_PTR_EQ(test, plane1->old_fb, NULL);
+	KUNIT_EXPECT_PTR_EQ(test, plane2->crtc, NULL);
+	KUNIT_EXPECT_PTR_EQ(test, plane2->fb, NULL);
+	KUNIT_EXPECT_PTR_EQ(test, plane2->old_fb, NULL);
+}
+
 static struct kunit_case drm_framebuffer_tests[] = {
+	KUNIT_CASE(drm_test_fb_release),
 	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),
-- 
2.41.0

Re: [PATCH v2 01/11] drm/tests: Stop using deprecated dev_private member on drm_framebuffer tests

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2225 bytes --]

Hi,

On Tue, Oct 24, 2023 at 04:09:52PM -0300, Carlos Eduardo Gallo Filho wrote:
> The dev_private member of drm_device is deprecated and its use should
> be avoided. Stop using it by embedding the drm_device onto a mock struct
> with a void pointer like dev_private, using it instead.
> 
> Also start using drm_kunit_helper_alloc_drm_device() for allocating
> the drm_device mock.
> 
> Signed-off-by: Carlos Eduardo Gallo Filho <gcarlos@disroot.org>
> ---
> v2:
>   - Start using drm_kunit_helper_alloc_drm_device() for drm_device mock.
>   - Rename struct drm_mock to drm_framebuffer_test_priv
> ---
>  drivers/gpu/drm/tests/drm_framebuffer_test.c | 42 ++++++++++++++------
>  1 file changed, 29 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c b/drivers/gpu/drm/tests/drm_framebuffer_test.c
> index f759d9f3b76e..9c6170edd5f7 100644
> --- a/drivers/gpu/drm/tests/drm_framebuffer_test.c
> +++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c
> @@ -8,8 +8,10 @@
>  #include <kunit/test.h>
>  
>  #include <drm/drm_device.h>
> +#include <drm/drm_drv.h>
>  #include <drm/drm_mode.h>
>  #include <drm/drm_fourcc.h>
> +#include <drm/drm_kunit_helpers.h>
>  #include <drm/drm_print.h>
>  
>  #include "../drm_crtc_internal.h"
> @@ -317,11 +319,17 @@ static const struct drm_framebuffer_test drm_framebuffer_create_cases[] = {
>  },
>  };
>  
> +struct drm_framebuffer_test_priv {
> +	struct drm_device dev;
> +	void *private;
> +};
> +

I'm not super confident with using a void pointer to whatever the test
wants it to be. Especially since it seems like you only use it to store
whether the buffer has been created, so I guess this could just be
converted to a boolean?

>  static struct drm_framebuffer *fb_create_mock(struct drm_device *dev,
>  					      struct drm_file *file_priv,
>  					      const struct drm_mode_fb_cmd2 *mode_cmd)
>  {
> -	int *buffer_created = dev->dev_private;
> +	struct drm_framebuffer_test_priv *priv = container_of(dev, typeof(*priv), dev);
> +	int *buffer_created = priv->private;
>  	*buffer_created = 1;

And then you just need to change that line to priv->buffer_created = true;

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 02/11] drm/tests: Add parameters to the drm_test_framebuffer_create test

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1798 bytes --]

Hi,

On Tue, Oct 24, 2023 at 04:09:53PM -0300, Carlos Eduardo Gallo Filho wrote:
> Extend the existing test case to cover:
> 1. Invalid flag atribute in the struct drm_mode_fb_cmd2.
> 2. Pixel format which requires non-linear modifier with
> DRM_FORMAT_MOD_LINEAR set.
> 3. Non-zero buffer offset for an unused plane
> 
> Also replace strcpy to strscpy on test_to_desc for improved security
> and reliability.
> 
> Signed-off-by: Carlos Eduardo Gallo Filho <gcarlos@disroot.org>
> ---
> v2:
>   - Remove strcpy to strscpy change.
> ---
>  drivers/gpu/drm/tests/drm_framebuffer_test.c | 21 ++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c b/drivers/gpu/drm/tests/drm_framebuffer_test.c
> index 9c6170edd5f7..659cbd5a3be3 100644
> --- a/drivers/gpu/drm/tests/drm_framebuffer_test.c
> +++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c
> @@ -21,6 +21,8 @@
>  #define MIN_HEIGHT 4
>  #define MAX_HEIGHT 4096
>  
> +#define DRM_MODE_FB_INVALID BIT(2)
> +
>  struct drm_framebuffer_test {
>  	int buffer_created;
>  	struct drm_mode_fb_cmd2 cmd;
> @@ -85,6 +87,18 @@ static const struct drm_framebuffer_test drm_framebuffer_create_cases[] = {
>  		 .pitches = { 4 * MAX_WIDTH, 0, 0 },
>  	}
>  },
> +{ .buffer_created = 0, .name = "ABGR8888 Non-zero buffer offset for unused plane",
> +	.cmd = { .width = MAX_WIDTH, .height = MAX_HEIGHT, .pixel_format = DRM_FORMAT_ABGR8888,
> +		 .handles = { 1, 0, 0 }, .offsets = { UINT_MAX / 2, UINT_MAX / 2, 0 },
> +		 .pitches = { 4 * MAX_WIDTH, 0, 0 }, .flags = DRM_MODE_FB_MODIFIERS,
> +	}
> +},

I know that the other tests are like that too, but I'd really like a
comment that explains what corner case this test is supposed to test.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 03/11] drm/tests: Replace strcpy to strscpy on drm_test_framebuffer_create test

[Maxime Ripard]

On Tue, 24 Oct 2023 16:09:54 -0300, Carlos Eduardo Gallo Filho wrote:
> Replace the use of strcpy to strscpy on the test_to_desc of the
> drm_test_framebuffer_create test for better security and reliability.
> 
> Signed-off-by: Carlos Eduardo Gallo Filho <gcarlos@disroot.org>

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

Re: [PATCH v2 04/11] drm/tests: Add test case for drm_internal_framebuffer_create()

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2449 bytes --]

Hi,

On Tue, Oct 24, 2023 at 04:09:55PM -0300, Carlos Eduardo Gallo Filho wrote:
> Introduce a test to cover the creation of framebuffer with
> modifier on a device that doesn't support it.
> 
> Signed-off-by: Carlos Eduardo Gallo Filho <gcarlos@disroot.org>
> ---
> v2:
>   - Reorder kunit cases alphabetically.
> ---
>  drivers/gpu/drm/tests/drm_framebuffer_test.c | 28 ++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c b/drivers/gpu/drm/tests/drm_framebuffer_test.c
> index eb76a71644e9..8a9b6d08d639 100644
> --- a/drivers/gpu/drm/tests/drm_framebuffer_test.c
> +++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c
> @@ -403,8 +403,36 @@ static void drm_framebuffer_test_to_desc(const struct drm_framebuffer_test *t, c
>  KUNIT_ARRAY_PARAM(drm_framebuffer_create, drm_framebuffer_create_cases,
>  		  drm_framebuffer_test_to_desc);
>  
> +/*
> + * This test is very similar to drm_test_framebuffer_create, except that it

We shouldn't rely on the another test for the documentation. If
drm_test_framebuffer_create is ever removed or renamed, then the reader
won't have any clue what it was supposed to test.

We should provide a documentation that doesn't require any additional
context.

> + * set dev->mode_config.fb_modifiers_not_supported member to 1, covering
> + * the case of trying to create a framebuffer with modifiers without the
> + * device really supporting it.
> + */

So I guess something like:

Test that, if a device doesn't support modifiers, a framebuffer
allocation using them will fail.

> +static void drm_test_framebuffer_modifiers_not_supported(struct kunit *test)
> +{
> +	struct drm_framebuffer_test_priv *priv = test->priv;
> +	struct drm_device *dev = &priv->dev;
> +	int buffer_created = 0;
> +
> +	/* A valid cmd with modifier */
> +	struct drm_mode_fb_cmd2 cmd = {
> +		.width = MAX_WIDTH, .height = MAX_HEIGHT,
> +		.pixel_format = DRM_FORMAT_ABGR8888, .handles = { 1, 0, 0 },
> +		.offsets = { UINT_MAX / 2, 0, 0 }, .pitches = { 4 * MAX_WIDTH, 0, 0 },
> +		.flags = DRM_MODE_FB_MODIFIERS,
> +	};
> +
> +	priv->private = &buffer_created;
> +	dev->mode_config.fb_modifiers_not_supported = 1;
> +
> +	drm_internal_framebuffer_create(dev, &cmd, NULL);
> +	KUNIT_EXPECT_EQ(test, 0, buffer_created);

We should test the returned value of drm_internal_framebuffer_create here.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 06/11] drm/tests: Add test for drm_framebuffer_cleanup()

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2380 bytes --]

Hi,

On Tue, Oct 24, 2023 at 04:09:57PM -0300, Carlos Eduardo Gallo Filho wrote:
> Add a single KUnit test case for the drm_framebuffer_cleanup 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 | 37 ++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c b/drivers/gpu/drm/tests/drm_framebuffer_test.c
> index 7b51862fb0f2..a63f30985f75 100644
> --- a/drivers/gpu/drm/tests/drm_framebuffer_test.c
> +++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c
> @@ -490,8 +490,45 @@ check_src_coords_test_to_desc(const struct drm_framebuffer_check_src_coords_case
>  KUNIT_ARRAY_PARAM(check_src_coords, drm_framebuffer_check_src_coords_cases,
>  		  check_src_coords_test_to_desc);
>  
> +static void drm_test_framebuffer_cleanup(struct kunit *test)

Again, we should document *what* we are testing here. Your commit says
that you're testing drm_framebuffer_cleanup(), but even after reading
that test I have no idea which tests have been covered or not.

> +{
> +	struct drm_framebuffer_test_priv *priv = test->priv;
> +	struct drm_device *dev = &priv->dev;
> +	struct list_head *fb_list = &dev->mode_config.fb_list;
> +	struct drm_framebuffer fb1 = { .dev = dev };
> +	struct drm_framebuffer fb2 = { .dev = dev };
> +
> +	/* This must result on [fb_list] -> fb1 -> fb2 */
> +	list_add_tail(&fb1.head, fb_list);
> +	list_add_tail(&fb2.head, fb_list);
> +	dev->mode_config.num_fb = 2;
> +
> +	KUNIT_ASSERT_PTR_EQ(test, fb_list->prev, &fb2.head);
> +	KUNIT_ASSERT_PTR_EQ(test, fb_list->next, &fb1.head);
> +	KUNIT_ASSERT_PTR_EQ(test, fb1.head.prev, fb_list);
> +	KUNIT_ASSERT_PTR_EQ(test, fb1.head.next, &fb2.head);
> +	KUNIT_ASSERT_PTR_EQ(test, fb2.head.prev, &fb1.head);
> +	KUNIT_ASSERT_PTR_EQ(test, fb2.head.next, fb_list);
> +
> +	drm_framebuffer_cleanup(&fb1);

I think that for that test to be meaningful, we should first have init
the framebuffer using drm_framebuffer_init. Otherwise we will just
chasing our tail trying to mock drm_framebuffer_init. Plus, we don't
want to test if drm_framebuffer_cleanup works if we hand-crafted a
framebuffer.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 07/11] drm/tests: Add test for drm_framebuffer_lookup()

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1887 bytes --]

Hi,

On Tue, Oct 24, 2023 at 04:09:58PM -0300, Carlos Eduardo Gallo Filho wrote:
> Add a single KUnit test case for the drm_framebuffer_lookup function.
> 
> Signed-off-by: Carlos Eduardo Gallo Filho <gcarlos@disroot.org>
> ---
> v2:
>   - Reorder kunit cases alphabetically.
>   - Replace drm_mode_object_add() call to drm_framebuffer_init().
>   - Rely on drm_kunit_helper_alloc_device() for mock initialization.
> ---
>  drivers/gpu/drm/tests/drm_framebuffer_test.c | 26 ++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c b/drivers/gpu/drm/tests/drm_framebuffer_test.c
> index a63f30985f75..fb9589dd8aed 100644
> --- a/drivers/gpu/drm/tests/drm_framebuffer_test.c
> +++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c
> @@ -526,10 +526,36 @@ static void drm_test_framebuffer_cleanup(struct kunit *test)
>  	KUNIT_ASSERT_EQ(test, dev->mode_config.num_fb, 0);
>  }
>  
> +static void drm_test_framebuffer_lookup(struct kunit *test)

Again, documentation.

> +{
> +	struct drm_framebuffer_test_priv *priv = test->priv;
> +	struct drm_device *dev = &priv->dev;
> +	struct drm_format_info format = { };
> +	struct drm_framebuffer fb1 = { .dev = dev, .format = &format };
> +	struct drm_framebuffer *fb2;
> +	uint32_t id = 0;
> +	int ret;
> +
> +	ret = drm_framebuffer_init(dev, &fb1, NULL);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +	id = fb1.base.id;
> +
> +	/* Looking for fb1 */
> +	fb2 = drm_framebuffer_lookup(dev, NULL, id);
> +	KUNIT_EXPECT_PTR_EQ(test, fb2, &fb1);

I would rename the variables to expected_fb and fb (or returned_fb);

You also need to call drm_framebuffer_put on fb2.

> +	/* Looking for an inexistent framebuffer */
> +	fb2 = drm_framebuffer_lookup(dev, NULL, id + 1);
> +	KUNIT_EXPECT_NULL(test, fb2);

This should be a separate test

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 08/11] drm/tests: Add test for drm_framebuffer_init()

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3107 bytes --]

On Tue, Oct 24, 2023 at 04:09:59PM -0300, Carlos Eduardo Gallo Filho wrote:
> Add a single KUnit test case for the drm_framebuffer_init function.
> 
> Signed-off-by: Carlos Eduardo Gallo Filho <gcarlos@disroot.org>
> ---
> v2:
>   - Reorder kunit cases alphabetically.
>   - Let fb1.dev unset instead of set it to wrong_drm to test mismatched
>     drm_device passed as drm_framebuffer_init() argument.
>   - Clean the framebuffer object.
> ---
>  drivers/gpu/drm/tests/drm_framebuffer_test.c | 52 ++++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c b/drivers/gpu/drm/tests/drm_framebuffer_test.c
> index fb9589dd8aed..eedd5e920279 100644
> --- a/drivers/gpu/drm/tests/drm_framebuffer_test.c
> +++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c
> @@ -551,10 +551,62 @@ static void drm_test_framebuffer_lookup(struct kunit *test)
>  	drm_framebuffer_cleanup(&fb1);
>  }
>  
> +static void drm_test_framebuffer_init(struct kunit *test)

Documentation

> +{
> +	struct drm_framebuffer_test_priv *priv = test->priv;
> +	struct drm_device *dev = &priv->dev;
> +	struct drm_format_info format = { };
> +	struct drm_framebuffer fb1 = { .format = &format };
> +	struct drm_framebuffer *fb2;
> +	struct drm_framebuffer_funcs funcs = { };
> +	int ret;
> +
> +	/* Fails if fb->dev doesn't point to the drm_device passed on first arg */
> +	ret = drm_framebuffer_init(dev, &fb1, &funcs);
> +	KUNIT_ASSERT_EQ(test, ret, -EINVAL);
> +	fb1.dev = dev;
> +
> +	/* Fails if fb.format isn't set */
> +	fb1.format = NULL;
> +	ret = drm_framebuffer_init(dev, &fb1, &funcs);
> +	KUNIT_ASSERT_EQ(test, ret, -EINVAL);
> +	fb1.format = &format;
> +
> +	ret = drm_framebuffer_init(dev, &fb1, &funcs);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	/*
> +	 * Check if fb->funcs is actually set to the drm_framebuffer_funcs
> +	 * passed to it
> +	 */
> +	KUNIT_EXPECT_PTR_EQ(test, fb1.funcs, &funcs);
> +
> +	/* The fb->comm must be set to the current running process */
> +	KUNIT_EXPECT_STREQ(test, fb1.comm, current->comm);
> +
> +	/* The fb->base must be successfully initialized */
> +	KUNIT_EXPECT_NE(test, fb1.base.id, 0);
> +	KUNIT_EXPECT_EQ(test, fb1.base.type, DRM_MODE_OBJECT_FB);
> +	KUNIT_EXPECT_EQ(test, kref_read(&fb1.base.refcount), 1);
> +	KUNIT_EXPECT_PTR_EQ(test, fb1.base.free_cb, &drm_framebuffer_free);
> +
> +	/* Checks if the fb is really published and findable */
> +	fb2 = drm_framebuffer_lookup(dev, NULL, fb1.base.id);
> +	KUNIT_EXPECT_PTR_EQ(test, fb2, &fb1);
> +
> +	/* There must be just that one fb initialized */
> +	KUNIT_EXPECT_EQ(test, dev->mode_config.num_fb, 1);
> +	KUNIT_EXPECT_PTR_EQ(test, dev->mode_config.fb_list.prev, &fb1.head);
> +	KUNIT_EXPECT_PTR_EQ(test, dev->mode_config.fb_list.next, &fb1.head);
> +
> +	drm_framebuffer_cleanup(&fb1);
> +}

You're testing different failure cases, so these should all be their own
tests. Otherwise, you'll just get a single test failure that doesn't
really provide any feedback on what went wrong.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 09/11] drm/tests: Add test for drm_framebuffer_free()

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2695 bytes --]

On Tue, Oct 24, 2023 at 04:10:00PM -0300, Carlos Eduardo Gallo Filho wrote:
> Add a single KUnit test case for the drm_framebuffer_free function.
> 
> Signed-off-by: Carlos Eduardo Gallo Filho <gcarlos@disroot.org>
> ---
> v2:
>   - Reorder kunit cases alphabetically.
> ---
>  drivers/gpu/drm/tests/drm_framebuffer_test.c | 49 ++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c b/drivers/gpu/drm/tests/drm_framebuffer_test.c
> index eedd5e920279..dbe412d0dca4 100644
> --- a/drivers/gpu/drm/tests/drm_framebuffer_test.c
> +++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c
> @@ -602,10 +602,59 @@ static void drm_test_framebuffer_init(struct kunit *test)
>  	drm_framebuffer_cleanup(&fb1);
>  }
>  
> +static void destroy_free_mock(struct drm_framebuffer *fb)
> +{
> +	struct drm_framebuffer_test_priv *priv = container_of(fb->dev, typeof(*priv), dev);
> +	int *buffer_freed = priv->private;
> +	*buffer_freed = 1;
> +}

Yeah, definitely not a fan of a pointer being used for multiple things
depending on the caller.

Just add another boolean to drm_framebuffer_test_priv, that way it will
be obvious to anyone, and it will be simpler if we ever rework that part
of the tests.

> +static struct drm_framebuffer_funcs framebuffer_funcs_free_mock = {
> +	.destroy = destroy_free_mock,
> +};
> +
> +static void drm_test_framebuffer_free(struct kunit *test)

I'm sure you get the idea now :)

> +{
> +	struct drm_framebuffer_test_priv *priv = test->priv;
> +	struct drm_device *dev = &priv->dev;
> +	struct drm_mode_object *obj;
> +	struct drm_framebuffer fb = {
> +		.dev = dev,
> +		.funcs = &framebuffer_funcs_free_mock,
> +	};
> +	int buffer_freed = 0;
> +	int id, ret;
> +
> +	priv->private = &buffer_freed;
> +
> +	/*
> +	 * Case where the fb isn't registered. Just test if
> +	 * drm_framebuffer_free calls fb->funcs->destroy
> +	 */
> +	drm_framebuffer_free(&fb.base.refcount);
> +	KUNIT_EXPECT_EQ(test, buffer_freed, 1);
> +
> +	buffer_freed = 0;
> +
> +	ret = drm_mode_object_add(dev, &fb.base, DRM_MODE_OBJECT_FB);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +	id = fb.base.id;
> +
> +	/* Now, test with the fb registered, that must end unregistered */
> +	drm_framebuffer_free(&fb.base.refcount);
> +	KUNIT_EXPECT_EQ(test, fb.base.id, 0);
> +	KUNIT_EXPECT_EQ(test, buffer_freed, 1);
> +
> +	/* Test if the old id of the fb was really removed from the idr pool */
> +	obj = drm_mode_object_find(dev, NULL, id, DRM_MODE_OBJECT_FB);
> +	KUNIT_EXPECT_PTR_EQ(test, obj, NULL);
> +}

And for that as well, these should all be separate tests.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6386 bytes --]

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
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]