Re: [PATCH i-g-t 35/39] lib/vkms: Test changing enabled device planes

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

Le 28/02/2025 à 12:52, José Expósito a écrit :
> Hi Louis,
> 
> Thanks a lot for reviewing this series, there were a ton of
> patches. I hope they were easy enough to understand :)

Apart from the meson part, the rest is straightforward to understand, 
your tests are very clear! It was just long to review because there are 
39 patches, not because they were complex.

> I won't have time to look into all of your reviews this week,
> but I'll try to at least anwser to this one as I see you are
> finding some test failures.

Don't panic, almost all of my reviews are R-bys!

> On Fri, Feb 28, 2025 at 09:51:47AM +0100, Louis Chauvet wrote:
>>
>>
>> Le 18/02/2025 à 17:50, José Expósito a écrit :
>>> Test that, once a VKMS device is enabled, the plane values can't change
>>> and that deleting it or the attached CRTCs doesn't change the VKMS
>>> device.
>>>
>>> Add a function that performs a basic validation checking that the
>>> device created matches the expected one.
>>>
>>> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
>>
>> The next tests don't pass on my VM, can you share details on your setup?
>> (kernel branch, architecture)
> 
> I'm testing on a QEMU VM (x86_64), using this kernel branch [1], which
> is basically [2] + [3].

Perfect, I work on almost the same branch, but I will test with your

> This is the script I'm using to run the IGT tests:
> 
>    $ meson setup build ; ninja -C build && \
>       ssh -p 2222 jose@127.0.0.1 -t \
>              'cd shared/igt-gpu-tools && \
>              sudo modprobe vkms && \
>              sudo systemctl isolate multi-user.target && \
>              sudo IGT_DEVICE="sys:/sys/devices/platform/vkms" \
>                  ./build/tests/vkms/vkms_configfs ; \
>              sudo systemctl isolate graphical.target'
> 
> Basically, I build in the host and run on the VM by sharing the code
> and builds in "shared/igt-gpu-tools" inside the VM.

I have a very similar setup (except I use virtme-ng to configure the 
VM), so I don't think this is an issue. Thanks for sharing those details!

> [1] https://github.com/JoseExposito/linux/tree/patch-vkms-configfs
> [2] https://lore.kernel.org/all/20250218101214.5790-1-jose.exposito89@gmail.com/
> [3] https://lore.kernel.org/dri-devel/20250225175936.7223-1-jose.exposito89@gmail.com/T/
> 
>>> ---
>>>    lib/igt_vkms.c             |  27 +++++++
>>>    lib/igt_vkms.h             |   1 +
>>>    tests/vkms/vkms_configfs.c | 147 +++++++++++++++++++++++++++++++++++++
>>>    3 files changed, 175 insertions(+)
>>>
>>> diff --git a/lib/igt_vkms.c b/lib/igt_vkms.c
>>> index 4c44efec9..3dab7a823 100644
>>> --- a/lib/igt_vkms.c
>>> +++ b/lib/igt_vkms.c
>>> [...]
>>> +static void assert_device_config(igt_vkms_config_t *cfg)
>>> +{
>>> +	drmDevicePtr devices[64];
>>> +	drmDevicePtr dev;
>>> +	drmModeResPtr res;
>>> +	drmModePlaneResPtr plane_res;
>>> +	drmModeConnectorPtr connector_res;
>>> +	igt_vkms_crtc_config_t *crtc;
>>> +	igt_vkms_connector_config_t *connector;
>>> +	int n_devices;
>>> +	int n_planes = 0;
>>> +	int n_crtcs = 0;
>>> +	int n_encoders = 0;
>>> +	int n_connectors = 0;
>>> +	int n_connector_status_cfg[4] = {0};
>>> +	int n_connector_status_drm[4] = {0};
>>> +	int fd;
>>> +
>>> +	n_devices = drmGetDevices(devices, ARRAY_SIZE(devices));
>>> +
>>> +	dev = find_device(cfg->device_name, devices, n_devices);
>>> +	igt_assert_f(dev, "Device '%s' not found\n", cfg->device_name);
>>> +
>>> +	fd = open(dev->nodes[DRM_NODE_PRIMARY], O_RDONLY);
>>> +	igt_assert_f(fd >= 0, "Error opening device '%s' at path '%s'\n",
>>> +		     cfg->device_name, dev->nodes[DRM_NODE_PRIMARY]);
>>> +	igt_assert_f(!drmSetClientCap(fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1),
>>> +		     "Error setting DRM_CLIENT_CAP_UNIVERSAL_PLANES\n");
>>> +	igt_assert_f(!drmSetClientCap(fd, DRM_CLIENT_CAP_ATOMIC, 1),
>>> +		     "Error setting DRM_CLIENT_CAP_ATOMIC\n");
>>> +	igt_assert_f(!drmSetClientCap(fd, DRM_CLIENT_CAP_WRITEBACK_CONNECTORS, 1),
>>> +		     "Error setting DRM_CLIENT_CAP_WRITEBACK_CONNECTORS\n");
>>> +
>>> +	res = drmModeGetResources(fd);
>>> +	igt_assert_f(res, "Error getting resources\n");
>>> +	plane_res = drmModeGetPlaneResources(fd);
>>> +	igt_assert_f(plane_res, "Error getting plane resources\n");
>>> +
>>> +	for (int n = 0; (&cfg->planes[n])->name; n++)
>>> +		n_planes++;
>>> +
>>> +	for (int n = 0; (crtc = &cfg->crtcs[n])->name; n++) {
>>> +		n_crtcs++;
>>> +
>>> +		if (crtc->writeback) {
>>> +			n_encoders++;
>>> +			n_connectors++;
>>> +			n_connector_status_cfg[DRM_MODE_UNKNOWNCONNECTION]++;
> 
> I wonder why this is not working for you.
> 
> I see in your comment in 39/39 that you are not getting the right numner
> of connectors when "crtc->writeback == true", but I'm adding them here.

True! I missed this part (I read too fast, sorry). So this is probably 
not the problem.

> Could add a log and check if this is actually the problem, please?

I will re-run the tests on Monday to print exactly what are the 
values/expected values, and search why they fails.

Thanks!
Louis Chauvet

> Best wishes,
> Jose
> 
>>> +		}
>>> +	}
>>> +
>>> +	for (int n = 0; (&cfg->encoders[n])->name; n++)
>>> +		n_encoders++;
>>> +
>>> +	for (int n = 0; (connector = &cfg->connectors[n])->name; n++) {
>>> +		n_connectors++;
>>> +		n_connector_status_cfg[connector->status]++;
>>> +	}
>>> +
>>> +	for (int n = 0; n < res->count_connectors; n++) {
>>> +		connector_res = drmModeGetConnectorCurrent(fd,
>>> +							   res->connectors[n]);
>>> +		n_connector_status_drm[connector_res->connection]++;
>>> +		drmModeFreeConnector(connector_res);
>>> +	}
>>
>> I think the main issue here is something I already observed: you need to
>> force probe the connector status (and in this test you really want to do
>> it), so you should use `drmModeGetConnector`.
>>
>> I just tested, it seems to work, except for the last test about connector
>> hotplug (see my review).
>>
>>> +
>>> +	igt_assert_eq(n_planes, plane_res->count_planes);
>>> +	igt_assert_eq(n_crtcs, res->count_crtcs);
>>> +	igt_assert_eq(n_encoders, res->count_encoders);
>>> +	igt_assert_eq(n_connectors, res->count_connectors);
>>> +	igt_assert_eq(n_connector_status_cfg[DRM_MODE_CONNECTED],
>>> +		      n_connector_status_drm[DRM_MODE_CONNECTED]);
>>> +	igt_assert_eq(n_connector_status_cfg[DRM_MODE_DISCONNECTED],
>>> +		      n_connector_status_drm[DRM_MODE_DISCONNECTED]);
>>> +	igt_assert_eq(n_connector_status_cfg[DRM_MODE_UNKNOWNCONNECTION],
>>> +		      n_connector_status_drm[DRM_MODE_UNKNOWNCONNECTION]);
>>
>> ^ Fails on those asserts
>>
>>> +
>>> +	drmModeFreePlaneResources(plane_res);
>>> +	drmModeFreeResources(res);
>>> +	close(fd);
>>> +	drmFreeDevices(devices, n_devices);
>>> +}
>>> +
>>>    /**
>>>     * SUBTEST: device-default-files
>>>     * Description: Test that creating a VKMS device creates the default files and
>>> @@ -1414,6 +1497,69 @@ static void test_enable_too_many_connectors(void)
>>>    	igt_vkms_device_destroy(dev);
>>>    }
>>> +/**
>>> + * SUBTEST: enabled-plane-cannot-change
>>> + * Description: Test that, once a VKMS device is enabled, the plane values can't
>>> + *              change and that deleting it or the attached CRTCs doesn't change
>>> + *              the VKMS device.
>>> + */
>>> +
>>> +static void test_enabled_plane_cannot_change(void)
>>> +{
>>> +	igt_vkms_t *dev;
>>> +
>>> +	igt_vkms_config_t cfg = {
>>> +		.device_name = __func__,
>>> +		.planes = {
>>> +			{
>>> +				.name = "plane0",
>>> +				.type = DRM_PLANE_TYPE_PRIMARY,
>>> +				.possible_crtcs = { "crtc0"},
>>> +			},
>>> +			{
>>> +				.name = "plane1",
>>> +				.type = DRM_PLANE_TYPE_PRIMARY,
>>> +				.possible_crtcs = { "crtc1"},
>>> +			},
>>> +		},
>>> +		.crtcs = {
>>> +			{ .name = "crtc0" },
>>> +			{ .name = "crtc1" },
>>> +		},
>>> +		.encoders = {
>>> +			{ .name = "encoder0", .possible_crtcs = { "crtc0" } },
>>> +			{ .name = "encoder1", .possible_crtcs = { "crtc1" } },
>>> +		},
>>> +		.connectors = {
>>> +			{
>>> +				.name = "connector0",
>>> +				.possible_encoders = { "encoder0", "encoder1" },
>>> +			},
>>> +		},
>>> +	};
>>> +
>>> +	dev = igt_vkms_device_create_from_config(&cfg);
>>> +	igt_assert(dev);
>>> +
>>> +	igt_vkms_device_set_enabled(dev, true);
>>> +	igt_assert(igt_vkms_device_is_enabled(dev));
>>> +	assert_device_config(&cfg);
>>> +
>>> +	/* Try to change values */
>>> +	igt_vkms_plane_set_type(dev, "plane0", DRM_PLANE_TYPE_OVERLAY);
>>> +	igt_assert_eq(igt_vkms_plane_get_type(dev, "plane0"),
>>> +		      DRM_PLANE_TYPE_PRIMARY);
>>> +
>>> +	igt_assert(!igt_vkms_plane_attach_crtc(dev, "plane0", "crtc1"));
>>> +
>>> +	/* Deleting pipeline items doesn't affect the device */
>>> +	igt_assert(igt_vkms_plane_detach_crtc(dev, "plane0", "crtc0"));
>>> +	igt_assert(igt_vkms_device_remove_plane(dev, "plane0"));
>>> +	assert_device_config(&cfg);
>>> +
>>> +	igt_vkms_device_destroy(dev);
>>> +}
>>> +
>>>    igt_main
>>>    {
>>>    	struct {
>>> @@ -1451,6 +1597,7 @@ igt_main
>>>    		{ "enable-crtc-no-encoder", test_enable_crtc_no_encoder },
>>>    		{ "enable-no-connectors", test_enable_no_connectors },
>>>    		{ "enable-too-many-connectors", test_enable_too_many_connectors },
>>> +		{ "enabled-plane-cannot-change", test_enabled_plane_cannot_change },
>>>    	};
>>>    	igt_fixture {
>>
>> -- 
>> Louis Chauvet, Bootlin
>> Embedded Linux and Kernel engineering
>> https://bootlin.com
>>
>>

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