Re: [PATCH i-g-t 17/39] lib/vkms: Test attaching planes to CRTCs

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

On Thu, Feb 27, 2025 at 02:15:01PM +0100, Louis Chauvet wrote:
> 
> 
> Le 18/02/2025 à 17:49, José Expósito a écrit :
> > Add helpers to attach and detach planes and CRTCs and a test checking
> > the different valid and invalid cases.
> > 
> > Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> > ---
> >   lib/igt_vkms.c             | 91 ++++++++++++++++++++++++++++++++++++++
> >   lib/igt_vkms.h             |  4 ++
> >   tests/vkms/vkms_configfs.c | 81 +++++++++++++++++++++++++++++++++
> >   3 files changed, 176 insertions(+)
> > 
> > diff --git a/lib/igt_vkms.c b/lib/igt_vkms.c
> > index f5e193a1c..147234e5b 100644
> > --- a/lib/igt_vkms.c
> > +++ b/lib/igt_vkms.c
> > @@ -119,6 +119,22 @@ static const char *get_pipeline_item_dir_name(enum vkms_pipeline_item item)
> >   	igt_assert(!"Cannot be reached: Unknown VKMS pipeline item type");
> >   }
> > +static const char *get_attach_dir_name(enum vkms_pipeline_item item)
> > +{
> > +	switch (item) {
> > +	case VKMS_PIPELINE_ITEM_PLANE:
> > +		return "possible_crtcs";
> > +	case VKMS_PIPELINE_ITEM_ENCODER:
> > +		return "possible_crtcs";
> > +	case VKMS_PIPELINE_ITEM_CONNECTOR:
> > +		return "possible_encoders";
> > +	default:
> > +		break;
> > +	}
> > +
> > +	igt_assert(!"Cannot be reached: Unknown VKMS attach directory name");
> > +}
> > +
> >   static void add_pipeline_item(igt_vkms_t *dev, enum vkms_pipeline_item item,
> >   			      const char *name)
> >   {
> > @@ -136,6 +152,51 @@ static void add_pipeline_item(igt_vkms_t *dev, enum vkms_pipeline_item item,
> >   		     path, errno, strerror(errno));
> >   }
> > +static bool attach_pipeline_item(igt_vkms_t *dev,
> > +				 enum vkms_pipeline_item src_item,
> > +				 const char *src_item_name,
> > +				 enum vkms_pipeline_item dst_item,
> > +				 const char *dst_item_name)
> > +{
> > +	char src_path[PATH_MAX];
> > +	char dst_path[PATH_MAX];
> > +	const char *src_dir_name;
> > +	const char *attach_dir_name;
> > +	const char *dst_dir_name;
> > +	int ret;
> > +
> > +	src_dir_name = get_pipeline_item_dir_name(src_item);
> > +	attach_dir_name = get_attach_dir_name(src_item);
> > +	snprintf(src_path, sizeof(src_path), "%s/%s/%s/%s/%s", dev->path,
> > +		 src_dir_name, src_item_name, attach_dir_name, dst_item_name);
> 
> The more I read those functions, the more I think my proposition of having
> full path for pipeline item is better than having only its name.
> 
> Or maybe, to avoid the duplication a macro could help:
> 
> 	#define ITEM_FMT "%s/%s/%s"
> 	#define ITEM_ARGS(dev, src_item, src_item_name) (dev),
> get_pipeline_item_dir_name((src_item)), (src_item_name)
> 
> I don't see major drawbacks for both of my proposition, so take what you
> think is better.
> 
> > +
> > +	dst_dir_name = get_pipeline_item_dir_name(dst_item);
> > +	snprintf(dst_path, sizeof(dst_path), "%s/%s/%s", dev->path,
> > +		 dst_dir_name, dst_item_name);
> > +
> > +	ret = symlink(dst_path, src_path);
> > +	return ret == 0;
> 
> Why this function has a return value? For all the other igt_vkms_device_*
> functions, they return void, and do assertions to stop the test if it fails.

Attaching or detaching pipeline items can fail and I want to be able to test
both the success and the error cases from the tests.

While, it is true that this is not consistent with "igt_vkms_device_add_*()"
functions, adding new pipeline items is not expected to fail and returning a
boolean would add a bunch of additional assertions in the tests for no real
gain.

Jose

> > +}
> > +
> > +static bool detach_pipeline_item(igt_vkms_t *dev,
> > +				 enum vkms_pipeline_item src_item,
> > +				 const char *src_item_name,
> > +				 const char *dst_item_name)
> > +{
> > +	char link_path[PATH_MAX];
> > +	const char *src_dir_name;
> > +	const char *attach_dir_name;
> > +	int ret;
> > +
> > +	src_dir_name = get_pipeline_item_dir_name(src_item);
> > +	attach_dir_name = get_attach_dir_name(src_item);
> > +	snprintf(link_path, sizeof(link_path), "%s/%s/%s/%s/%s", dev->path,
> > +		 src_dir_name, src_item_name, attach_dir_name, dst_item_name);
> > +
> > +	ret = unlink(link_path);
> > +	return ret == 0;
> > +}
> > +
> >   /**
> >    * igt_require_vkms_configfs:
> >    *
> > @@ -368,6 +429,36 @@ void igt_vkms_plane_set_type(igt_vkms_t *dev, const char *name, int type)
> >   	write_int(path, type);
> >   }
> > +/**
> > + * igt_vkms_plane_attach_crtc:
> > + * @dev: Target device
> > + * @plane_name: Target plane name
> > + * @crtc_name: Destination CRTC name
> > + *
> > + * Attach a plane to a CRTC. Return true on success and false on error.
> > + */
> > +bool igt_vkms_plane_attach_crtc(igt_vkms_t *dev, const char *plane_name,
> > +				const char *crtc_name)
> > +{
> > +	return attach_pipeline_item(dev, VKMS_PIPELINE_ITEM_PLANE, plane_name,
> > +				    VKMS_PIPELINE_ITEM_CRTC, crtc_name);
> > +}
> > +
> > +/**
> > + * igt_vkms_plane_detach_crtc:
> > + * @dev: Target device
> > + * @plane_name: Target plane name
> > + * @crtc_name: Destination CRTC name
> > + *
> > + * Detach a plane from a CRTC. Return true on success and false on error.
> > + */
> > +bool igt_vkms_plane_detach_crtc(igt_vkms_t *dev, const char *plane_name,
> > +				const char *crtc_name)
> > +{
> > +	return detach_pipeline_item(dev, VKMS_PIPELINE_ITEM_PLANE, plane_name,
> > +				    crtc_name);
> > +}
> > +
> >   /**
> >    * igt_vkms_device_add_crtc:
> >    * @dev: Device to add the CRTC to
> > diff --git a/lib/igt_vkms.h b/lib/igt_vkms.h
> > index 50f42aa4b..fc8db268b 100644
> > --- a/lib/igt_vkms.h
> > +++ b/lib/igt_vkms.h
> > @@ -32,6 +32,10 @@ void igt_vkms_device_set_enabled(igt_vkms_t *dev, bool enabled);
> >   void igt_vkms_device_add_plane(igt_vkms_t *dev, const char *name);
> >   int igt_vkms_plane_get_type(igt_vkms_t *dev, const char *name);
> >   void igt_vkms_plane_set_type(igt_vkms_t *dev, const char *name, int type);
> > +bool igt_vkms_plane_attach_crtc(igt_vkms_t *dev, const char *plane_name,
> > +				const char *crtc_name);
> > +bool igt_vkms_plane_detach_crtc(igt_vkms_t *dev, const char *plane_name,
> > +				const char *crtc_name);
> >   void igt_vkms_device_add_crtc(igt_vkms_t *dev, const char *name);
> >   bool igt_vkms_crtc_is_writeback_enabled(igt_vkms_t *dev, const char *name);
> > diff --git a/tests/vkms/vkms_configfs.c b/tests/vkms/vkms_configfs.c
> > index ea84d9f82..e1572d65f 100644
> > --- a/tests/vkms/vkms_configfs.c
> > +++ b/tests/vkms/vkms_configfs.c
> > @@ -94,6 +94,20 @@ static void assert_wrong_bool_values(const char *path)
> >   	}
> >   }
> > +static bool attach(const char *src_path, const char *dst_path,
> > +		   const char *link_name)
> > +{
> > +	char link_path[PATH_MAX];
> > +	int ret;
> > +
> > +	ret = snprintf(link_path, sizeof(link_path), "%s/%s", src_path, link_name);
> > +	igt_assert(ret >= 0 && ret < sizeof(link_path));
> > +
> > +	ret = symlink(dst_path, link_path);
> > +
> > +	return ret == 0;
> > +}
> > +
> >   /**
> >    * SUBTEST: device-default-files
> >    * Description: Test that creating a VKMS device creates the default files and
> > @@ -476,6 +490,72 @@ static void test_connector_wrong_values(void)
> >   	igt_vkms_device_destroy(dev);
> >   }
> > +/**
> > + * SUBTEST: attach-plane-to-crtc
> > + * Description: Check that errors are handled while attaching planes to CRTCs.
> > + */
> > +
> > +static void test_attach_plane_to_crtc(void)
> > +{
> > +	igt_vkms_t *dev1;
> > +	igt_vkms_t *dev2;
> > +	char plane1[PATH_MAX];
> > +	char crtc1[PATH_MAX];
> > +	char connector1[PATH_MAX];
> > +	char crtc2[PATH_MAX];
> > +	char dev2_enabled_path[PATH_MAX];
> > +	bool ok;
> > +
> > +	dev1 = igt_vkms_device_create("test_attach_plane_to_crtc_1");
> > +	igt_assert(dev1);
> > +
> > +	dev2 = igt_vkms_device_create("test_attach_plane_to_crtc_2");
> > +	igt_assert(dev2);
> > +
> > +	igt_vkms_device_add_plane(dev1, "plane1");
> > +	igt_vkms_device_add_crtc(dev1, "crtc1");
> > +	igt_vkms_device_add_connector(dev1, "connector1");
> > +	igt_vkms_device_add_crtc(dev2, "crtc2");
> > +
> > +	snprintf(plane1, sizeof(plane1), "%s/planes/plane1/possible_crtcs",
> > +		 dev1->path);
> > +	snprintf(crtc1, sizeof(crtc1), "%s/crtcs/crtc1", dev1->path);
> > +	snprintf(connector1, sizeof(connector1), "%s/connectors/connector1",
> > +		 dev1->path);
> > +	snprintf(crtc2, sizeof(crtc2), "%s/crtcs/crtc2", dev2->path);
> > +	snprintf(dev2_enabled_path, sizeof(dev2_enabled_path), "%s/enabled",
> > +		 dev2->path);
> > +
> > +	/* Error: Attach a plane to a connector */
> > +	ok = attach(plane1, connector1, "connector");
> > +	igt_assert_f(!ok, "Attaching plane1 to connector1 should fail\n");
> > +
> > +	/* Error: Attach a plane to a random file */
> > +	ok = attach(plane1, dev2_enabled_path, "file");
> > +	igt_assert_f(!ok, "Attaching plane1 to a random file should fail\n");
> > +
> > +	/* Error: Attach a plane to a CRTC from other device */
> > +	ok = attach(plane1, crtc2, "crtc2");
> > +	igt_assert_f(!ok, "Attaching plane1 to crtc2 should fail\n");
> > +
> > +	/* OK: Attaching plane1 to crtc1 */
> > +	ok = igt_vkms_plane_attach_crtc(dev1, "plane1", "crtc1");
> > +	igt_assert_f(ok, "Error attaching plane1 to crtc1\n");
> > +
> > +	/* Error: Attaching plane1 to crtc1 twice */
> > +	ok = attach(plane1, crtc1, "crtc1_duplicated");
> > +	igt_assert_f(!ok, "Error attaching plane1 to crtc1 twice should fail");
> > +
> > +	/* OK: Detaching and attaching again */
> > +	ok = igt_vkms_plane_detach_crtc(dev1, "plane1", "crtc1");
> > +	igt_assert_f(ok, "Error detaching plane1 from crtc1\n");
> > +	ok = igt_vkms_plane_attach_crtc(dev1, "plane1", "crtc1");
> > +	igt_assert_f(ok, "Error attaching plane1 to crtc1\n");
> > +
> > +	igt_vkms_device_destroy(dev1);
> > +	igt_vkms_device_destroy(dev2);
> > +}
> > +
> >   igt_main
> >   {
> >   	struct {
> > @@ -495,6 +575,7 @@ igt_main
> >   		{ "connector-default-files", test_connector_default_files },
> >   		{ "connector-default-values", test_connector_default_values },
> >   		{ "connector-wrong-values", test_connector_wrong_values },
> > +		{ "attach-plane-to-crtc", test_attach_plane_to_crtc },
> >   	};
> >   	igt_fixture {
> 
> -- 
> Louis Chauvet, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 
>