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@xxxxxxxxxxx>
Signed-off-by: Louis Chauvet <louis.chauvet@xxxxxxxxxxx>
Signed-off-by: José Expósito <jose.exposito89@xxxxxxxxx>
---
.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@xxxxxxxxxxx>
(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
