Re: [PATCH i-g-t 20/39] tests/vkms_configfs: Test enablement without pipeline items

[Louis Chauvet <louis.chauvet@bootlin.com>]

Le 18/02/2025 à 17:49, José Expósito a écrit :
> It shouldn't be possible to enable a device without creating and
> attaching its pipeline items.
> 
> Test that the device is not enabled and also that no actual device was
> created using libdrm.
> 
> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> ---
>   tests/vkms/vkms_configfs.c | 60 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 60 insertions(+)
> 
> diff --git a/tests/vkms/vkms_configfs.c b/tests/vkms/vkms_configfs.c
> index dc3dbe80d..87d660fff 100644
> --- a/tests/vkms/vkms_configfs.c
> +++ b/tests/vkms/vkms_configfs.c
> @@ -108,6 +108,42 @@ static bool attach(const char *src_path, const char *dst_path,
>   	return ret == 0;
>   }
>   
> +static drmDevicePtr find_device(const char *name, drmDevicePtr *devices,
> +				int n_devices)
> +{
> +	drmDevicePtr dev;
> +	const char *dev_name;
> +
> +	for (int i = 0; i < n_devices; i++) {
> +		dev = devices[i];
> +
> +		if (!(dev->available_nodes & BIT(DRM_NODE_PRIMARY)))
> +			continue;
> +
> +		if (dev->bustype != DRM_BUS_PLATFORM)
> +			continue;

Hummm, I think this may have issues with the change proposed by Greg in 
[1], I don't know how the drmDevicePtr should look like with the new 
faux_device struct.

The declaration in xf86drm.h is:

typedef struct _drmDevice {
     char **nodes; /* DRM_NODE_MAX sized array */
     int available_nodes; /* DRM_NODE_* bitmask */
     int bustype;
     union {
         drmPciBusInfoPtr pci;
         drmUsbBusInfoPtr usb;
         drmPlatformBusInfoPtr platform;
         drmHost1xBusInfoPtr host1x;
     } businfo;
     union {
         drmPciDeviceInfoPtr pci;
         drmUsbDeviceInfoPtr usb;
         drmPlatformDeviceInfoPtr platform;
         drmHost1xDeviceInfoPtr host1x;
     } deviceinfo;
} drmDevice, *drmDevicePtr;

But if vkms/vgem switch to faux_device, all users will break,

But with faux_device integration, everything using it with VKMS will break.

Isn't this a breakage of the UAPI? The vkms devices are currently in 
"platform_bus", but if we change it to faux_device, it will be on a 
"faux_bus".

[1]: https://lore.kernel.org/all/2025021029-snout-swivel-9a45@gregkh/

(not for the patch itself, but the comment just above, that may apply 
for vgem_drv too, I will reply to [1] to have the comment at the right 
place)
+CC: Thomas Zimmerman
+CC: Greg Kroah-Hartman

> +
> +		dev_name = dev->businfo.platform->fullname;
> +		if (strncmp(name, dev_name, strlen(name)) == 0)
> +			return dev;
> +	}
> +
> +	return NULL;
> +}
> +
> +static bool device_exists(const char *name)
> +{
> +	drmDevicePtr devices[64];
> +	int n_devices;
> +	bool exists;
> +
> +	n_devices = drmGetDevices(devices, ARRAY_SIZE(devices));
> +	exists = !!find_device(name, devices, n_devices);
> +	drmFreeDevices(devices, n_devices);
> +
> +	return exists;
> +}
> +
>   /**
>    * SUBTEST: device-default-files
>    * Description: Test that creating a VKMS device creates the default files and
> @@ -689,6 +725,29 @@ static void test_attach_connector_to_encoder(void)
>   	igt_vkms_device_destroy(dev2);
>   }
>   
> +/**
> + * SUBTEST: enable-no-pipeline-items
> + * Description: Try to enable a VKMS device without adding any pipeline items
> + *              and test that it fails.
> + */
> +
> +static void test_enable_no_pipeline_items(void)
> +{
> +	igt_vkms_t *dev;
> +
> +	dev = igt_vkms_device_create(__func__);
> +	igt_assert(dev);
> +
> +	/* Try to enable it and check that the device is not set as enabled */
> +	igt_vkms_device_set_enabled(dev, true);
> +	igt_assert(!igt_vkms_device_is_enabled(dev));
> +
> +	/* Check that no actual device was created*/
> +	igt_assert(!device_exists(__func__));

I think to properly validate that no new device were created, we need to 
have another test "working configuration" where we can confirm that the 
configfs name is the same as the device name.

> +	igt_vkms_device_destroy(dev);
> +}
> +
>   igt_main
>   {
>   	struct {
> @@ -711,6 +770,7 @@ igt_main
>   		{ "attach-plane-to-crtc", test_attach_plane_to_crtc },
>   		{ "attach-encoder-to-crtc", test_attach_encoder_to_crtc },
>   		{ "attach-connector-to-encoder", test_attach_connector_to_encoder },
> +		{ "enable-no-pipeline-items", test_enable_no_pipeline_items },
>   	};
>   
>   	igt_fixture {

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com