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. I'll create helpers in vkms_config_test.c replacing the list_* APIs with iterators and send v4. 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 >
