| Message ID | 20230825160725.12861-2-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: > 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. > > Signed-off-by: Carlos Eduardo Gallo Filho <gcarlos@disroot.org> > --- > drivers/gpu/drm/tests/drm_framebuffer_test.c | 29 +++++++++++++------- > 1 file changed, 19 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c b/drivers/gpu/drm/tests/drm_framebuffer_test.c > index f759d9f3b76e..173d42b145ed 100644 > --- a/drivers/gpu/drm/tests/drm_framebuffer_test.c > +++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c > @@ -317,11 +317,17 @@ static const struct drm_framebuffer_test drm_framebuffer_create_cases[] = { > }, > }; > > +struct drm_mock { > + struct drm_device dev; > + void *private; > +}; Could we call it drm_device_mock or maybe drm_framebuffer_mock? I believe that drm_mock its a bit generic. Also, wouldn't be better to create a `int buffer_created` variable instead of creating a `void *private`? Best Regards, - Maíra > + > 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_mock *mock = container_of(dev, typeof(*mock), dev); > + int *buffer_created = mock->private; > *buffer_created = 1; > return ERR_PTR(-EINVAL); > } > @@ -332,16 +338,18 @@ static struct drm_mode_config_funcs mock_config_funcs = { > > static int drm_framebuffer_test_init(struct kunit *test) > { > - struct drm_device *mock; > + struct drm_mock *mock; > + struct drm_device *dev; > > mock = kunit_kzalloc(test, sizeof(*mock), GFP_KERNEL); > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mock); > + dev = &mock->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; > return 0; > @@ -350,11 +358,12 @@ static int drm_framebuffer_test_init(struct kunit *test) > 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_mock *mock = test->priv; > + struct drm_device *dev = &mock->dev; > int buffer_created = 0; > > - mock->dev_private = &buffer_created; > - drm_internal_framebuffer_create(mock, ¶ms->cmd, NULL); > + mock->private = &buffer_created; > + drm_internal_framebuffer_create(dev, ¶ms->cmd, NULL); > KUNIT_EXPECT_EQ(test, params->buffer_created, buffer_created); > } >
Hi Maíra, thanks for reviewing! On 8/26/23 10:53 AM, Maíra Canal wrote: > Hi Carlos, > > On 8/25/23 13:07, 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. >> >> Signed-off-by: Carlos Eduardo Gallo Filho <gcarlos@disroot.org> >> --- >> drivers/gpu/drm/tests/drm_framebuffer_test.c | 29 +++++++++++++------- >> 1 file changed, 19 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c >> b/drivers/gpu/drm/tests/drm_framebuffer_test.c >> index f759d9f3b76e..173d42b145ed 100644 >> --- a/drivers/gpu/drm/tests/drm_framebuffer_test.c >> +++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c >> @@ -317,11 +317,17 @@ static const struct drm_framebuffer_test >> drm_framebuffer_create_cases[] = { >> }, >> }; >> +struct drm_mock { >> + struct drm_device dev; >> + void *private; >> +}; > > Could we call it drm_device_mock or maybe drm_framebuffer_mock? I > believe that drm_mock its a bit generic. I could agree that it's a bit generic. Exclusively for this patch, drm_device_mock could be a good candidate, but later in this patchset I use that same struct to allocate a drm_file mock too, so I think that the name must at least fit well to it too. In that case I would prefer naming it drm_framebuffer_mock, but doesn't it looks like a name for a drm_framebuffer mock, which isn't the case? I'm trying to figure out another name to it but I'm not able to do that. > Also, wouldn't be better to create a `int buffer_created` variable > instead of creating a `void *private`? Again, I could agree with that for this patch only, but the `void *private` is used in that way in some other tests from this series too. Anyway, I noticed that all but one test is using it to "return" integer (boolean, to be honest) values from mocked functions, except by the fb_create_addfb2_mock function on patch 9, that use it to return a reference to a drm_framebuffer. So, in that case,do you think it would be better to have explicitly some boolean on the struct instead of this void pointer? If so, should I keep that void pointer for fb_create_addfb2_mock use or should I replace it to a drm_framebuffer pointer? By the way, I guess that having a void pointer on a general purpose struct like that would let further tests to adapt it to its own use, but I don't really know if it worth the effort. Thanks, Carlos > Best Regards, > - Maíra > >> + >> 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_mock *mock = container_of(dev, typeof(*mock), dev); >> + int *buffer_created = mock->private; >> *buffer_created = 1; >> return ERR_PTR(-EINVAL); >> } >> @@ -332,16 +338,18 @@ static struct drm_mode_config_funcs >> mock_config_funcs = { >> static int drm_framebuffer_test_init(struct kunit *test) >> { >> - struct drm_device *mock; >> + struct drm_mock *mock; >> + struct drm_device *dev; >> mock = kunit_kzalloc(test, sizeof(*mock), GFP_KERNEL); >> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mock); >> + dev = &mock->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; >> return 0; >> @@ -350,11 +358,12 @@ static int drm_framebuffer_test_init(struct >> kunit *test) >> 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_mock *mock = test->priv; >> + struct drm_device *dev = &mock->dev; >> int buffer_created = 0; >> - mock->dev_private = &buffer_created; >> - drm_internal_framebuffer_create(mock, ¶ms->cmd, NULL); >> + mock->private = &buffer_created; >> + drm_internal_framebuffer_create(dev, ¶ms->cmd, NULL); >> KUNIT_EXPECT_EQ(test, params->buffer_created, buffer_created); >> }
diff --git a/drivers/gpu/drm/tests/drm_framebuffer_test.c b/drivers/gpu/drm/tests/drm_framebuffer_test.c index f759d9f3b76e..173d42b145ed 100644 --- a/drivers/gpu/drm/tests/drm_framebuffer_test.c +++ b/drivers/gpu/drm/tests/drm_framebuffer_test.c @@ -317,11 +317,17 @@ static const struct drm_framebuffer_test drm_framebuffer_create_cases[] = { }, }; +struct drm_mock { + 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_mock *mock = container_of(dev, typeof(*mock), dev); + int *buffer_created = mock->private; *buffer_created = 1; return ERR_PTR(-EINVAL); } @@ -332,16 +338,18 @@ static struct drm_mode_config_funcs mock_config_funcs = { static int drm_framebuffer_test_init(struct kunit *test) { - struct drm_device *mock; + struct drm_mock *mock; + struct drm_device *dev; mock = kunit_kzalloc(test, sizeof(*mock), GFP_KERNEL); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mock); + dev = &mock->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; return 0; @@ -350,11 +358,12 @@ static int drm_framebuffer_test_init(struct kunit *test) 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_mock *mock = test->priv; + struct drm_device *dev = &mock->dev; int buffer_created = 0; - mock->dev_private = &buffer_created; - drm_internal_framebuffer_create(mock, ¶ms->cmd, NULL); + mock->private = &buffer_created; + drm_internal_framebuffer_create(dev, ¶ms->cmd, NULL); KUNIT_EXPECT_EQ(test, params->buffer_created, buffer_created); }
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. Signed-off-by: Carlos Eduardo Gallo Filho <gcarlos@disroot.org> --- drivers/gpu/drm/tests/drm_framebuffer_test.c | 29 +++++++++++++------- 1 file changed, 19 insertions(+), 10 deletions(-)