Re: [PATCH i-g-t 13/39] lib/vkms: Test encoder default files

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

Hi Louis,

On Thu, Feb 27, 2025 at 02:14:52PM +0100, Louis Chauvet wrote:
> 
> 
> Le 18/02/2025 à 17:49, José Expósito a écrit :
> > Add a helper to create a encoder and a test checking that the default
> > files and directories are created.
> > 
> > Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> > ---
> >   lib/igt_vkms.c             | 12 ++++++++++++
> >   lib/igt_vkms.h             |  2 ++
> >   tests/vkms/vkms_configfs.c | 29 +++++++++++++++++++++++++++++
> >   3 files changed, 43 insertions(+)
> > 
> > diff --git a/lib/igt_vkms.c b/lib/igt_vkms.c
> > index d881a2417..299891377 100644
> > --- a/lib/igt_vkms.c
> > +++ b/lib/igt_vkms.c
> > @@ -416,3 +416,15 @@ void igt_vkms_crtc_set_writeback_enabled(igt_vkms_t *dev, const char *name,
> >   	write_bool(path, writeback);
> >   }
> > +
> > +/**
> > + * igt_vkms_device_add_encoder:
> > + * @dev: Device to add the encoder to
> > + * @name: Encoder name
> > + *
> > + * Add a new encoder to the VKMS device.
> > + */
> > +void igt_vkms_device_add_encoder(igt_vkms_t *dev, const char *name)
> > +{
> > +	add_pipeline_item(dev, VKMS_PIPELINE_ITEM_ENCODER, name);
> > +}
> > diff --git a/lib/igt_vkms.h b/lib/igt_vkms.h
> > index cb0278544..b4b048c6d 100644
> > --- a/lib/igt_vkms.h
> > +++ b/lib/igt_vkms.h
> > @@ -38,4 +38,6 @@ bool igt_vkms_crtc_is_writeback_enabled(igt_vkms_t *dev, const char *name);
> >   void igt_vkms_crtc_set_writeback_enabled(igt_vkms_t *dev, const char *name,
> >   					 bool writeback);
> > +void igt_vkms_device_add_encoder(igt_vkms_t *dev, const char *name);
> > +
> >   #endif /* __IGT_VKMS_H__ */
> > diff --git a/tests/vkms/vkms_configfs.c b/tests/vkms/vkms_configfs.c
> > index af42e364d..753ea2d85 100644
> > --- a/tests/vkms/vkms_configfs.c
> > +++ b/tests/vkms/vkms_configfs.c
> > @@ -345,6 +345,34 @@ static void test_crtc_wrong_values(void)
> >   	igt_vkms_device_destroy(dev);
> >   }
> > +/**
> > + * SUBTEST: encoder-default-files
> > + * Description: Test that creating a encoder creates the default files and
> > + *              directories.
> > + */
> > +
> > +static void test_encoder_default_files(void)
> > +{
> > +	igt_vkms_t *dev;
> > +	char path[PATH_MAX];
> > +
> > +	const char *dirs[] = {
> > +		"possible_crtcs",
> > +	};
> > +
> > +	dev = igt_vkms_device_create(__func__);
> > +	igt_assert(dev);
> > +
> > +	igt_vkms_device_add_encoder(dev, "encoder0");
> 
> I maybe found a solution for the duplication of path generation, can the
> igt_vkms_device_add_* return the path of the object?
> 
> To avoid malloc/free, you can simply add an optional pointer "path":

I decided to add something similar to this, but instead of making it part
of the igt_vkms_device_add_*() functions, I created new helpers that are
used both in the tests and in the lib.

I'll send v2 in a bit with them.

Thanks a lot for the huge effort reviewing all of this.

Jose

> void igt_vkms_device_add_encoder(dev, name, path, max_len) {
> 	if (path)
> 		igt_assert_le(snprintf(path, max_len, "....", ...),
> 				max_len);
> 	[...]
> }
> 
> > +	snprintf(path, sizeof(path), "%s/encoders/encoder0", dev->path);
> > +	assert_default_files(path,
> > +			     NULL, 0,
> > +			     dirs, ARRAY_SIZE(dirs));
> > +
> > +	igt_vkms_device_destroy(dev);
> > +}
> > +
> >   igt_main
> >   {
> >   	struct {
> > @@ -360,6 +388,7 @@ igt_main
> >   		{ "crtc-default-files", test_crtc_default_files },
> >   		{ "crtc-default-values", test_crtc_default_values },
> >   		{ "crtc-wrong-values", test_crtc_wrong_values },
> > +		{ "encoder-default-files", test_encoder_default_files },
> >   	};
> >   	igt_fixture {
> 
> -- 
> Louis Chauvet, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 
>