| Message ID | 20230825160725.12861-6-gcarlos@disroot.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Increase coverage on drm_framebuffer.c | expand |
Hi Carlos, On 8/25/23 13:07, 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> > --- > 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 0e0e8216bbbc..16d9cf4bed88 100644 > --- a/drivers/gpu/drm/tests/drm_framebuffer_test.c > +++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c > @@ -370,6 +370,9 @@ static int drm_framebuffer_test_init(struct kunit *test) > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mock); > dev = &mock->dev; > > + mutex_init(&dev->mode_config.fb_lock); What about drmm_mutex_init()? Best Regards, - Maíra > + INIT_LIST_HEAD(&dev->mode_config.fb_list); > + dev->mode_config.num_fb = 0; > dev->mode_config.min_width = MIN_WIDTH; > dev->mode_config.max_width = MAX_WIDTH; > dev->mode_config.min_height = MIN_HEIGHT; > @@ -380,6 +383,14 @@ static int drm_framebuffer_test_init(struct kunit *test) > return 0; > } > > +static void drm_framebuffer_test_exit(struct kunit *test) > +{ > + struct drm_mock *mock = test->priv; > + struct drm_device *dev = &mock->dev; > + > + mutex_destroy(&dev->mode_config.fb_lock); > +} > + > static void drm_test_framebuffer_create(struct kunit *test) > { > const struct drm_framebuffer_test *params = test->param_value; > @@ -483,7 +494,44 @@ static void check_src_coords_test_to_desc(const struct check_src_coords_case *t, > KUNIT_ARRAY_PARAM(check_src_coords, check_src_coords_cases, > check_src_coords_test_to_desc); > > +static void drm_test_framebuffer_cleanup(struct kunit *test) > +{ > + struct drm_mock *mock = test->priv; > + struct drm_device *dev = &mock->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(drm_test_framebuffer_cleanup), > KUNIT_CASE(drm_test_framebuffer_modifiers_not_supported), > 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), > @@ -493,6 +541,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, > }; >
Hi Maíra, On 8/26/23 11:06, Maíra Canal wrote: > Hi Carlos, > > On 8/25/23 13:07, 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> >> --- >> 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 0e0e8216bbbc..16d9cf4bed88 100644 >> --- a/drivers/gpu/drm/tests/drm_framebuffer_test.c >> +++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c >> @@ -370,6 +370,9 @@ static int drm_framebuffer_test_init(struct kunit >> *test) >> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mock); >> dev = &mock->dev; >> + mutex_init(&dev->mode_config.fb_lock); > > What about drmm_mutex_init()? I took a look into it and as far I understand it would be useful if the drm_device was allocated with drmm_kalloc(), sure? As far we are using kunit_kalloc here and the drm_device is automatically deallocated when the test finishes, what would be the better by using drmm_mutex_init? It isn't that I don't wanna use it, I just didn't understand how exactly it works and how could I use it in that code. Should I replace the drm_device allocation to use drmm? Thanks, Carlos > Best Regards, > - Maíra > >> + INIT_LIST_HEAD(&dev->mode_config.fb_list); >> + dev->mode_config.num_fb = 0; >> dev->mode_config.min_width = MIN_WIDTH; >> dev->mode_config.max_width = MAX_WIDTH; >> dev->mode_config.min_height = MIN_HEIGHT; >> @@ -380,6 +383,14 @@ static int drm_framebuffer_test_init(struct >> kunit *test) >> return 0; >> } >> +static void drm_framebuffer_test_exit(struct kunit *test) >> +{ >> + struct drm_mock *mock = test->priv; >> + struct drm_device *dev = &mock->dev; >> + >> + mutex_destroy(&dev->mode_config.fb_lock); >> +} >> + >> static void drm_test_framebuffer_create(struct kunit *test) >> { >> const struct drm_framebuffer_test *params = test->param_value; >> @@ -483,7 +494,44 @@ static void check_src_coords_test_to_desc(const >> struct check_src_coords_case *t, >> KUNIT_ARRAY_PARAM(check_src_coords, check_src_coords_cases, >> check_src_coords_test_to_desc); >> +static void drm_test_framebuffer_cleanup(struct kunit *test) >> +{ >> + struct drm_mock *mock = test->priv; >> + struct drm_device *dev = &mock->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(drm_test_framebuffer_cleanup), >> KUNIT_CASE(drm_test_framebuffer_modifiers_not_supported), >> 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), >> @@ -493,6 +541,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, >> };
Hi Carlos, On 9/4/23 14:22, Carlos wrote: > Hi Maíra, > > On 8/26/23 11:06, Maíra Canal wrote: >> Hi Carlos, >> >> On 8/25/23 13:07, 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> >>> --- >>> 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 0e0e8216bbbc..16d9cf4bed88 100644 >>> --- a/drivers/gpu/drm/tests/drm_framebuffer_test.c >>> +++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c >>> @@ -370,6 +370,9 @@ static int drm_framebuffer_test_init(struct kunit >>> *test) >>> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mock); >>> dev = &mock->dev; >>> + mutex_init(&dev->mode_config.fb_lock); >> >> What about drmm_mutex_init()? > I took a look into it and as far I understand it would be useful if > the drm_device was allocated with drmm_kalloc(), sure? As far we I'm not sure if we can allocate drm_device with drmm_kzalloc(), as we need a DRM device for drmm_kzalloc(). drmm_kzalloc() is just a &drm_device managed version of kzalloc(). > are using kunit_kalloc here and the drm_device is automatically > deallocated when the test finishes, what would be the better by > using drmm_mutex_init? > Actually, thinking it better now, I think we cannot use drmm_mutex_init() here, as we are not calling drm_dev_put(). Best Regards, - Maíra > It isn't that I don't wanna use it, I just didn't understand how > exactly it works and how could I use it in that code. Should I > replace the drm_device allocation to use drmm? > > Thanks, > Carlos > >> Best Regards, >> - Maíra >> >>> + INIT_LIST_HEAD(&dev->mode_config.fb_list); >>> + dev->mode_config.num_fb = 0; >>> dev->mode_config.min_width = MIN_WIDTH; >>> dev->mode_config.max_width = MAX_WIDTH; >>> dev->mode_config.min_height = MIN_HEIGHT; >>> @@ -380,6 +383,14 @@ static int drm_framebuffer_test_init(struct >>> kunit *test) >>> return 0; >>> } >>> +static void drm_framebuffer_test_exit(struct kunit *test) >>> +{ >>> + struct drm_mock *mock = test->priv; >>> + struct drm_device *dev = &mock->dev; >>> + >>> + mutex_destroy(&dev->mode_config.fb_lock); >>> +} >>> + >>> static void drm_test_framebuffer_create(struct kunit *test) >>> { >>> const struct drm_framebuffer_test *params = test->param_value; >>> @@ -483,7 +494,44 @@ static void check_src_coords_test_to_desc(const >>> struct check_src_coords_case *t, >>> KUNIT_ARRAY_PARAM(check_src_coords, check_src_coords_cases, >>> check_src_coords_test_to_desc); >>> +static void drm_test_framebuffer_cleanup(struct kunit *test) >>> +{ >>> + struct drm_mock *mock = test->priv; >>> + struct drm_device *dev = &mock->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(drm_test_framebuffer_cleanup), >>> KUNIT_CASE(drm_test_framebuffer_modifiers_not_supported), >>> 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), >>> @@ -493,6 +541,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, >>> };
On Fri, Sep 08, 2023 at 05:42:07PM -0300, Maira Canal wrote: > Hi Carlos, > > On 9/4/23 14:22, Carlos wrote: > > Hi Maíra, > > > > On 8/26/23 11:06, Maíra Canal wrote: > > > Hi Carlos, > > > > > > On 8/25/23 13:07, 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> > > > > --- > > > > 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 0e0e8216bbbc..16d9cf4bed88 100644 > > > > --- a/drivers/gpu/drm/tests/drm_framebuffer_test.c > > > > +++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c > > > > @@ -370,6 +370,9 @@ static int drm_framebuffer_test_init(struct > > > > kunit *test) > > > > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mock); > > > > dev = &mock->dev; > > > > + mutex_init(&dev->mode_config.fb_lock); > > > > > > What about drmm_mutex_init()? > > I took a look into it and as far I understand it would be useful if > > the drm_device was allocated with drmm_kalloc(), sure? As far we > > I'm not sure if we can allocate drm_device with drmm_kzalloc(), as we > need a DRM device for drmm_kzalloc(). drmm_kzalloc() is just a > &drm_device managed version of kzalloc(). We should use drm_kunit_helper_alloc_drm_device_with_driver Maxime
diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c b/drivers/gpu/drm/tests/drm_framebuffer_test.c index 0e0e8216bbbc..16d9cf4bed88 100644 --- a/drivers/gpu/drm/tests/drm_framebuffer_test.c +++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c @@ -370,6 +370,9 @@ static int drm_framebuffer_test_init(struct kunit *test) KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mock); dev = &mock->dev; + mutex_init(&dev->mode_config.fb_lock); + INIT_LIST_HEAD(&dev->mode_config.fb_list); + dev->mode_config.num_fb = 0; dev->mode_config.min_width = MIN_WIDTH; dev->mode_config.max_width = MAX_WIDTH; dev->mode_config.min_height = MIN_HEIGHT; @@ -380,6 +383,14 @@ static int drm_framebuffer_test_init(struct kunit *test) return 0; } +static void drm_framebuffer_test_exit(struct kunit *test) +{ + struct drm_mock *mock = test->priv; + struct drm_device *dev = &mock->dev; + + mutex_destroy(&dev->mode_config.fb_lock); +} + static void drm_test_framebuffer_create(struct kunit *test) { const struct drm_framebuffer_test *params = test->param_value; @@ -483,7 +494,44 @@ static void check_src_coords_test_to_desc(const struct check_src_coords_case *t, KUNIT_ARRAY_PARAM(check_src_coords, check_src_coords_cases, check_src_coords_test_to_desc); +static void drm_test_framebuffer_cleanup(struct kunit *test) +{ + struct drm_mock *mock = test->priv; + struct drm_device *dev = &mock->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(drm_test_framebuffer_cleanup), KUNIT_CASE(drm_test_framebuffer_modifiers_not_supported), 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), @@ -493,6 +541,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_framebuffer_cleanup function. Signed-off-by: Carlos Eduardo Gallo Filho <gcarlos@disroot.org> --- drivers/gpu/drm/tests/drm_framebuffer_test.c | 49 ++++++++++++++++++++ 1 file changed, 49 insertions(+)