Re: [PATCH i-g-t v9 03/49] lib/igt_kms: Add function to list connected connectors

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

On 3/24/26 11:34, Kamil Konieczny wrote:
> Hi Louis,
> On 2026-03-16 at 17:17:24 +0100, Louis Chauvet wrote:
>> Introduce the igt_get_connected_connectors() function, which returns a
>> list of connector IDs that are currently connected according to DRM.
>>
>> This function includes a timeout mechanism because connectors can be
>> unplugged at any time. Also, especially with MST, the connector can be
>> listed by drmModeGetResources() but not yet accessible with
>> drmModeGetConnector().
>>
>> Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>
>> Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
>> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
>> ---
>>   lib/igt_kms.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   lib/igt_kms.h |  1 +
>>   2 files changed, 81 insertions(+)
>>
>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> index 4a496ab256f0..72a38334f921 100644
>> --- a/lib/igt_kms.c
>> +++ b/lib/igt_kms.c
>> @@ -8094,6 +8094,31 @@ igt_crtc_t *igt_random_crtc(igt_display_t *display)
>>   	return crtcs[rand() % n];
>>   }
>>   
>> +static drmModeConnectorPtr igt_wait_for_connector(int drm_fd, unsigned int connector_id,
>> +						  double timeout)
>> +{
>> +	drmModeConnectorPtr connector = NULL;
>> +	struct timespec start, end;
>> +
>> +	connector = drmModeGetConnector(drm_fd, connector_id);
>> +	/*
>> +	 * This time is required as sometimes some id in the connector list are not totally
>> +	 * ready or can disappear
>> +	 */
>> +	igt_assert_eq(igt_gettime(&start), 0);
>> +	end = start;
>> +
>> +	while (!connector &&
>> +	       igt_time_elapsed(&start, &end) <= timeout) {
>> +		drmModeFreeConnector(connector);
>> +		connector = drmModeGetConnector(drm_fd, connector_id);
>> +		usleep(10000);
> 
> Same here, use defined constant.
> 
>> +		igt_assert_eq(igt_gettime(&end), 0);
>> +	}
>> +
>> +	return connector;
>> +}
>> +
>>   /**
>>    * igt_wait_for_connector_status:
>>    * @drm_fd: drm file descriptor
>> @@ -8134,3 +8159,58 @@ bool igt_wait_for_connector_status(int drm_fd, unsigned int connector_id, double
>>   
>>   	return false;
>>   }
>> +
>> +/**
>> + * igt_get_connected_connectors:
>> + *
>> + * @drm_fd: DRM file descriptor
>> + * @connector_ids: Out pointer for the array of connector ids connected. The function
>> + * will allocate and store the pointer in this out pointer. The caller is
>> + * responsible to free the inner pointer using free(*connector_ids). caller.
> 
> s/ caller.//
> 
>> + *
>> + * This will probe all the connectors and return the list of all
>> + * connected connectors.
> 
> Document that this could assert. Also add newline here.
> 
>> + * Returns: The number of connectors in @connector_ids
>> + */
>> +int igt_get_connected_connectors(int drm_fd, uint32_t **connector_ids)
>> +{
>> +	uint32_t *tmp_alloc;
>> +	int connected_count = 0;
>> +	double timeout = igt_default_display_detect_timeout();
>> +	drmModeResPtr resources;
>> +
>> +	igt_assert(drm_fd);
> 
> -1 is invalid but will not assert.
> 
>> +	*connector_ids = NULL;
>> +
>> +	resources = drmModeGetResources(drm_fd);
>> +	igt_assert(resources);
> 
> Cannot you just return 0 instead of asserting?
> Or at least use igt_assert_f() with explanation message.

If the drmModeGetResources fails, it means that there is a big kernel 
issue or that the drm_fd is not a DRM file descriptor. In either case I 
think it is useless to continue or try to handle the error in the test case.

>> +	for (int j = 0; j < resources->count_connectors; j++) {
>> +		int status = DRM_MODE_UNKNOWNCONNECTION;
>> +
>> +		drmModeConnectorPtr connector = igt_wait_for_connector(drm_fd,
>> +								       resources->connectors[j],
>> +								       timeout);
>> +
>> +		if (connector)
>> +			status = connector->connection;
>> +		drmModeFreeConnector(connector);
>> +
>> +		if (status == DRM_MODE_CONNECTED) {
>> +			tmp_alloc = reallocarray(*connector_ids,
>> +						 connected_count
>> +						 + 1, sizeof(**connector_ids));
> 
> Why this line breaks? imho better:
> 			tmp_alloc = reallocarray(*connector_ids,
> 						 connected_count + 1,
> 						 sizeof(**connector_ids));

I agree, it was probably the autowrap of my editor.

>> +			if (tmp_alloc) {
>> +				*connector_ids = tmp_alloc;
>> +			} else {
>> +				free(*connector_ids);
>> +				drmModeFreeResources(resources);
>> +				igt_assert(false);
> 
> Use igt_assert_f() with explanation message.
> 
>> +			}
> 
> Btw better flow is with error check first:
> 			if (!tmp_alloc) {
> 				...handle error here with assert
> 			}
> 
> 			*connector_ids = tmp_alloc;

Right, done for the next iteration.

> Regards,
> Kamil
> 
>> +			(*connector_ids)[connected_count] = resources->connectors[j];
>> +			connected_count++;
>> +		}
>> +	}
>> +	drmModeFreeResources(resources);
>> +
>> +	return connected_count;
>> +}
>> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
>> index 934f24d5915b..4efd40a3ad08 100644
>> --- a/lib/igt_kms.h
>> +++ b/lib/igt_kms.h
>> @@ -1303,5 +1303,6 @@ igt_colorop_t *igt_find_colorop(igt_display_t *display, uint32_t id);
>>   
>>   bool igt_wait_for_connector_status(int drm_fd, unsigned int connector_id, double timeout,
>>   				   int drm_mode);
>> +int igt_get_connected_connectors(int drm_fd, uint32_t **connector_ids);
>>   
>>   #endif /* __IGT_KMS_H__ */
>>
>> -- 
>> 2.52.0
>>