Re: [PATCH v3 08/14] drm/vkms: Allow to configure multiple planes

[Louis Chauvet <louis.chauvet@bootlin.com>]

Le 17/02/2025 à 17:34, José Expósito a écrit :
> Hi Louis,
> 
> Thanks for the quick review.
> 
> On Mon, Feb 17, 2025 at 04:45:37PM +0100, Louis Chauvet wrote:
>> Hi José,
>>
>> Thanks for this new iteration!
>>
>> Le 17/02/2025 à 11:01, José Expósito a écrit :
>>> Add a list of planes to vkms_config and create as many planes as
>>> configured during output initialization.
>>>
>>> For backwards compatibility, add one primary plane and, if configured,
>>> one cursor plane and NUM_OVERLAY_PLANES planes to the default
>>> configuration.
>>>
>>> Co-developed-by: Louis Chauvet <louis.chauvet@bootlin.com>
>>> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
>>> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
>>> ---
>>>    .clang-format                                 |   1 +
>>>    drivers/gpu/drm/vkms/tests/vkms_config_test.c | 140 +++++++++++++++++-
>>>    drivers/gpu/drm/vkms/vkms_config.c            | 127 +++++++++++++++-
>>>    drivers/gpu/drm/vkms/vkms_config.h            |  75 +++++++++-
>>>    drivers/gpu/drm/vkms/vkms_output.c            |  42 ++----
>>>    5 files changed, 349 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/.clang-format b/.clang-format
>>> index fe1aa1a30d40..c585d2a5b395 100644
>>> --- a/.clang-format
>>> +++ b/.clang-format
>>> @@ -690,6 +690,7 @@ ForEachMacros:
>>>      - 'v4l2_m2m_for_each_src_buf'
>>>      - 'v4l2_m2m_for_each_src_buf_safe'
>>>      - 'virtio_device_for_each_vq'
>>> +  - 'vkms_config_for_each_plane'
>>>      - 'while_for_each_ftrace_op'
>>>      - 'xa_for_each'
>>>      - 'xa_for_each_marked'
>>> diff --git a/drivers/gpu/drm/vkms/tests/vkms_config_test.c b/drivers/gpu/drm/vkms/tests/vkms_config_test.c
>>> index 6e07139d261c..fe6f079902fd 100644
>>> --- a/drivers/gpu/drm/vkms/tests/vkms_config_test.c
>>> +++ b/drivers/gpu/drm/vkms/tests/vkms_config_test.c
>>> @@ -24,6 +24,10 @@ static void vkms_config_test_empty_config(struct kunit *test)
>>>    	dev_name = NULL;
>>>    	KUNIT_EXPECT_STREQ(test, vkms_config_get_device_name(config), "test");
>>> +	KUNIT_EXPECT_TRUE(test, list_empty(&config->planes));
>>
>> Instead of testing directly a "private" field (planes), can we use something
>> like:
>>
>> int count;
>> vkms_config_for_each_plane(config, plane_cfg)
>> 	count++;
>> ASSERT_EQ(count, 0);
>>
>> So we don't make config->plane "public".
>>
>> Same comment for connectors, crtc and encoders.
> 
> On other calls to list_empty() and also list_count_nodes() and
> list_first_entry() we are also accessing "private" fields.

True, I forgot those, thanks for noticing

> I'll create helpers in vkms_config_test.c replacing the list_* APIs with
> iterators and send v4.

I think we should not create helpers we don't use in the vkms driver 
itself, so we don't increase the surface API in vkms_config.c

You can simply create helpers in vkms_config_test.c that use the 
existing vkms_config API:

int vkms_config_*_count(config) {
	int count = 0;
	vkms_config_for_each_*(config, _) {
		count++;
	}
	return count;
}

struct vkms_config_* vkms_config_*_first(config) {
	vkms_config_for_each_*(config, item)
		return item;
	return NULL;
}

If we ever need to use those helpers in the vkms driver, we can easily 
move them at that time. Until then, the only "public" API we advertise 
for vkms_config is vkms_config_for_each_* (so if we need to refactor, it 
will be easier).

Thanks,
Louis Chauvet

> Thanks!
> Jose
>   
>> With this:
>> Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>
>> (sorry, I did not notice this on your v2)
>>
>> Thanks,
>> Louis Chauvet
>>
>> -- 
>> Louis Chauvet, Bootlin
>> Embedded Linux and Kernel engineering
>> https://bootlin.com
>>

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com