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

["José Expósito" <jose.exposito89@gmail.com>]

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.

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@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
>