| Message ID | 20230825161108.13701-1-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:11, 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> > --- > 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 3d14d35b4c4d..50d88bf3fa65 100644 > --- a/drivers/gpu/drm/tests/drm_framebuffer_test.c > +++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c > @@ -557,8 +557,60 @@ static void drm_test_framebuffer_lookup(struct kunit *test) > KUNIT_EXPECT_NULL(test, fb2); > } > > +static void drm_test_framebuffer_init(struct kunit *test) > +{ > + struct drm_mock *mock = test->priv; > + struct drm_device *dev = &mock->dev; > + struct drm_device wrong_drm = { }; > + struct drm_format_info format = { }; > + struct drm_framebuffer fb1 = { .dev = dev, .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 */ > + fb1.dev = &wrong_drm; > + ret = drm_framebuffer_init(dev, &fb1, &funcs); > + KUNIT_EXPECT_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_EXPECT_EQ(test, ret, -EINVAL); > + fb1.format = &format; > + > + ret = drm_framebuffer_init(dev, &fb1, &funcs); > + KUNIT_EXPECT_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_EQ(test, fb1.base.id, 1); > + 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); Shouldn't we clean the framebuffer object? Best Regards, - Maíra > +} > + > static struct kunit_case drm_framebuffer_tests[] = { > KUNIT_CASE(drm_test_framebuffer_cleanup), > + KUNIT_CASE(drm_test_framebuffer_init), > KUNIT_CASE(drm_test_framebuffer_lookup), > KUNIT_CASE(drm_test_framebuffer_modifiers_not_supported), > KUNIT_CASE_PARAM(drm_test_framebuffer_check_src_coords, check_src_coords_gen_params),
Hi Maíra, On 8/26/23 11:16, Maíra Canal wrote: > Hi Carlos, > > On 8/25/23 13:11, 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> >> --- >> 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 3d14d35b4c4d..50d88bf3fa65 100644 >> --- a/drivers/gpu/drm/tests/drm_framebuffer_test.c >> +++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c >> @@ -557,8 +557,60 @@ static void drm_test_framebuffer_lookup(struct >> kunit *test) >> KUNIT_EXPECT_NULL(test, fb2); >> } >> +static void drm_test_framebuffer_init(struct kunit *test) >> +{ >> + struct drm_mock *mock = test->priv; >> + struct drm_device *dev = &mock->dev; >> + struct drm_device wrong_drm = { }; >> + struct drm_format_info format = { }; >> + struct drm_framebuffer fb1 = { .dev = dev, .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 */ >> + fb1.dev = &wrong_drm; >> + ret = drm_framebuffer_init(dev, &fb1, &funcs); >> + KUNIT_EXPECT_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_EXPECT_EQ(test, ret, -EINVAL); >> + fb1.format = &format; >> + >> + ret = drm_framebuffer_init(dev, &fb1, &funcs); >> + KUNIT_EXPECT_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_EQ(test, fb1.base.id, 1); >> + 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); > > Shouldn't we clean the framebuffer object? What did you mean by "clean"? Firstly I supposed that it would be about freeing some dynamically allocated frambuffer, but it's statically allocated, so I believe it isn't what you are meaning. Is there some collateral effect I'm not taking into account? Thanks, Carlos > Best Regards, > - Maíra > >> +} >> + >> static struct kunit_case drm_framebuffer_tests[] = { >> KUNIT_CASE(drm_test_framebuffer_cleanup), >> + KUNIT_CASE(drm_test_framebuffer_init), >> KUNIT_CASE(drm_test_framebuffer_lookup), >> KUNIT_CASE(drm_test_framebuffer_modifiers_not_supported), >> KUNIT_CASE_PARAM(drm_test_framebuffer_check_src_coords, >> check_src_coords_gen_params),
Hi Carlos, On 9/4/23 14:41, Carlos wrote: > Hi Maíra, > > On 8/26/23 11:16, Maíra Canal wrote: >> Hi Carlos, >> >> On 8/25/23 13:11, 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> >>> --- >>> 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 3d14d35b4c4d..50d88bf3fa65 100644 >>> --- a/drivers/gpu/drm/tests/drm_framebuffer_test.c >>> +++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c >>> @@ -557,8 +557,60 @@ static void drm_test_framebuffer_lookup(struct >>> kunit *test) >>> KUNIT_EXPECT_NULL(test, fb2); >>> } >>> +static void drm_test_framebuffer_init(struct kunit *test) >>> +{ >>> + struct drm_mock *mock = test->priv; >>> + struct drm_device *dev = &mock->dev; >>> + struct drm_device wrong_drm = { }; >>> + struct drm_format_info format = { }; >>> + struct drm_framebuffer fb1 = { .dev = dev, .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 */ >>> + fb1.dev = &wrong_drm; >>> + ret = drm_framebuffer_init(dev, &fb1, &funcs); >>> + KUNIT_EXPECT_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_EXPECT_EQ(test, ret, -EINVAL); >>> + fb1.format = &format; >>> + >>> + ret = drm_framebuffer_init(dev, &fb1, &funcs); >>> + KUNIT_EXPECT_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_EQ(test, fb1.base.id, 1); >>> + 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); BTW I believe we should also make sure that dev->mode_config.num_fb was incremented by 1. >>> + >>> + /* 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); >> >> Shouldn't we clean the framebuffer object? > What did you mean by "clean"? Firstly I supposed that it would be about > freeing some dynamically allocated frambuffer, but it's statically > allocated, so I believe it isn't what you are meaning. Is there some > collateral effect I'm not taking into account? I was talking about calling the function `drm_framebuffer_cleanup()`. Best Regards, - Maíra > > Thanks, > Carlos > >> Best Regards, >> - Maíra >> >>> +} >>> + >>> static struct kunit_case drm_framebuffer_tests[] = { >>> KUNIT_CASE(drm_test_framebuffer_cleanup), >>> + KUNIT_CASE(drm_test_framebuffer_init), >>> KUNIT_CASE(drm_test_framebuffer_lookup), >>> KUNIT_CASE(drm_test_framebuffer_modifiers_not_supported), >>> KUNIT_CASE_PARAM(drm_test_framebuffer_check_src_coords, >>> check_src_coords_gen_params),
Hi Maíra, On 9/8/23 5:22 PM, Maira Canal wrote: > Hi Carlos, > > On 9/4/23 14:41, Carlos wrote: >> Hi Maíra, >> >> On 8/26/23 11:16, Maíra Canal wrote: >>> Hi Carlos, >>> >>> On 8/25/23 13:11, 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> >>>> --- >>>> 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 3d14d35b4c4d..50d88bf3fa65 100644 >>>> --- a/drivers/gpu/drm/tests/drm_framebuffer_test.c >>>> +++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c >>>> @@ -557,8 +557,60 @@ static void drm_test_framebuffer_lookup(struct >>>> kunit *test) >>>> KUNIT_EXPECT_NULL(test, fb2); >>>> } >>>> +static void drm_test_framebuffer_init(struct kunit *test) >>>> +{ >>>> + struct drm_mock *mock = test->priv; >>>> + struct drm_device *dev = &mock->dev; >>>> + struct drm_device wrong_drm = { }; >>>> + struct drm_format_info format = { }; >>>> + struct drm_framebuffer fb1 = { .dev = dev, .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 */ >>>> + fb1.dev = &wrong_drm; >>>> + ret = drm_framebuffer_init(dev, &fb1, &funcs); >>>> + KUNIT_EXPECT_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_EXPECT_EQ(test, ret, -EINVAL); >>>> + fb1.format = &format; >>>> + >>>> + ret = drm_framebuffer_init(dev, &fb1, &funcs); >>>> + KUNIT_EXPECT_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_EQ(test, fb1.base.id, 1); >>>> + 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); > > BTW I believe we should also make sure that dev->mode_config.num_fb was > incremented by 1. Isn't that already tested below? Since the start value for dev->mode_config.num_fb is 0, by expecting it to be 1 seems to test that it's being incremented by 1. Or what are you suggesting it to let it more explicit? > >>>> + >>>> + /* 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); >>> >>> Shouldn't we clean the framebuffer object? >> What did you mean by "clean"? Firstly I supposed that it would be about >> freeing some dynamically allocated frambuffer, but it's statically >> allocated, so I believe it isn't what you are meaning. Is there some >> collateral effect I'm not taking into account? > > I was talking about calling the function `drm_framebuffer_cleanup()`. Would you explain why we should need that here? Since the drm_device (and that fb, of course) is destroyed after the test, do we need to worry about this? Thanks, Carlos > > Best Regards, > - Maíra > >> >> Thanks, >> Carlos >> >>> Best Regards, >>> - Maíra >>> >>>> +} >>>> + >>>> static struct kunit_case drm_framebuffer_tests[] = { >>>> KUNIT_CASE(drm_test_framebuffer_cleanup), >>>> + KUNIT_CASE(drm_test_framebuffer_init), >>>> KUNIT_CASE(drm_test_framebuffer_lookup), >>>> KUNIT_CASE(drm_test_framebuffer_modifiers_not_supported), >>>> KUNIT_CASE_PARAM(drm_test_framebuffer_check_src_coords, >>>> check_src_coords_gen_params),
diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c b/drivers/gpu/drm/tests/drm_framebuffer_test.c index 3d14d35b4c4d..50d88bf3fa65 100644 --- a/drivers/gpu/drm/tests/drm_framebuffer_test.c +++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c @@ -557,8 +557,60 @@ static void drm_test_framebuffer_lookup(struct kunit *test) KUNIT_EXPECT_NULL(test, fb2); } +static void drm_test_framebuffer_init(struct kunit *test) +{ + struct drm_mock *mock = test->priv; + struct drm_device *dev = &mock->dev; + struct drm_device wrong_drm = { }; + struct drm_format_info format = { }; + struct drm_framebuffer fb1 = { .dev = dev, .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 */ + fb1.dev = &wrong_drm; + ret = drm_framebuffer_init(dev, &fb1, &funcs); + KUNIT_EXPECT_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_EXPECT_EQ(test, ret, -EINVAL); + fb1.format = &format; + + ret = drm_framebuffer_init(dev, &fb1, &funcs); + KUNIT_EXPECT_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_EQ(test, fb1.base.id, 1); + 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); +} + static struct kunit_case drm_framebuffer_tests[] = { KUNIT_CASE(drm_test_framebuffer_cleanup), + KUNIT_CASE(drm_test_framebuffer_init), KUNIT_CASE(drm_test_framebuffer_lookup), KUNIT_CASE(drm_test_framebuffer_modifiers_not_supported), KUNIT_CASE_PARAM(drm_test_framebuffer_check_src_coords, check_src_coords_gen_params),
Add a single KUnit test case for the drm_framebuffer_init function. Signed-off-by: Carlos Eduardo Gallo Filho <gcarlos@disroot.org> --- drivers/gpu/drm/tests/drm_framebuffer_test.c | 52 ++++++++++++++++++++ 1 file changed, 52 insertions(+)