Re: [PATCH i-g-t v4 30/46] tests/unigraf: Add basic unigraf tests

["Luca Ceresoli" <luca.ceresoli@bootlin.com>]

On Mon Nov 10, 2025 at 2:39 PM CET, Louis Chauvet wrote:
> Adds two tests to validate unigraf communication.
>
> unigraf-connected checks if the device is connected and if the EDID is
> properly applied.
>
> ungiraf-disconnect checks if the device can be connected/reconnected using
> SST and MST configurations.

Nit: why not using the same verb tense for the tewo tests? Either both
imperative ("connect" and "disconnect") or both past participle
("connected" and "disconnected"). Perhaps the imperative is better unless
there are different conventions in IGT.

> +++ b/tests/unigraf/unigraf_connectivity.c
> @@ -0,0 +1,123 @@
> +// SPDX-License-Identifier: MIT
> +
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <xf86drmMode.h>
> +
> +#include "drmtest.h"
> +#include "igt_aux.h"
> +#include "igt_core.h"
> +#include "igt_kms.h"
> +#include "unigraf/unigraf.h"
> +
> +/**
> + * TEST: unigraf connectivity
> + * Category: Core
> + * Description: Testing connectivity with a unigraf device
> + *
> + * SUBTEST: unigraf-open
> + * Description: Make sure that a unigraf device can be accessed
> + *
> + * SUBTEST: unigraf-connected
> + * Description: Make sure that the unigraf device is connected to the DUT

The "unigraf-open" test does not exist, and you missed the
"unigraf-disconnect[ed]" test.

> +IGT_TEST_DESCRIPTION("Test basic unigraf connectivity");
> +igt_main
> +{
> +	int drm_fd;
> +
> +	igt_fixture {
> +		drm_fd = drm_open_driver_master(DRIVER_ANY);
> +	}
> +
> +	igt_describe("Make sure that the unigraf device is connected to the DUT");
> +	igt_subtest("unigraf-connected") {
> +		drmModePropertyBlobPtr edid_blob = NULL;
> +		struct igt_display display;
> +		uint64_t edid_blob_id;
> +		igt_output_t *output;
> +		uint32_t unigraf_edid_len;
> +		void *unigraf_edid;
> +		bool found = false;
> +
> +		unigraf_require_device(drm_fd);
> +		unigraf_hpd_deassert();
> +		sleep(1);
> +		unigraf_set_sst();
> +		unigraf_hpd_assert();
> +
> +		sleep(5);
> +
> +		igt_display_require(&display, drm_fd);
> +		sleep(5);
> +
> +		unigraf_edid = unigraf_read_edid(0, &unigraf_edid_len);
> +
> +		for_each_connected_output(&display, output) {
> +			if (output->config.connector->connector_type ==
> +			    DRM_MODE_CONNECTOR_DisplayPort) {
> +				igt_assert(kmstest_get_property(drm_fd,
> +								output->config.connector->connector_id,
> +								DRM_MODE_OBJECT_CONNECTOR, "EDID",
> +								NULL, &edid_blob_id, NULL));
> +				edid_blob = drmModeGetPropertyBlob(drm_fd, edid_blob_id);
> +				if (!edid_blob)
> +					continue;
> +
> +				found |= memcmp(unigraf_edid, edid_blob->data,
> +						min(edid_blob->length, unigraf_edid_len)) == 0;

Even though it would work, using a bitwise OR is strange here. @found is
not really a bitmask, and the == operator does not return one. I'd use a
more intuitive form:

				if (memcmp(...) == 0)
					  found = true;

> +
> +				drmModeFreePropertyBlob(edid_blob);

Why not breaking the loop when found?

	if (found)
		break;

> +			}
> +		}
> +		igt_assert_f(found, "No output with the correct EDID was found\n");
> +
> +		free(unigraf_edid);
> +	}
> +
> +	igt_describe("Make sure that the unigraf device can be deconnected/reconnected");
                                                               disconnected

> +	igt_subtest("ungiraf-disconnect") {
> +		int newly_connected_count, already_connected_count, diff_len;
> +		uint32_t *newly_connected = NULL, *already_connected = NULL, *diff = NULL;
> +		int max_count;
> +
> +		unigraf_require_device(drm_fd);
> +		max_count = unigraf_get_mst_stream_max_count();
> +
> +		unigraf_hpd_deassert();
> +
> +		already_connected_count = igt_get_connected_connectors(drm_fd, &already_connected);
> +
> +		igt_debug("Already connected count: %d\n", already_connected_count);
> +
> +		// i = 0 is SST

If I got what you mean with this comment, I'd clarify it as:

		// i = 0 is SST so we need to process max_count + 1 streams

Otherwise looks good.

Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com