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

[Carlos]

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 at 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, &params->cmd, NULL);
>> +    mock->private = &buffer_created;
>> +    drm_internal_framebuffer_create(dev, &params->cmd, NULL);
>>       KUNIT_EXPECT_EQ(test, params->buffer_created, buffer_created);
>>   }