Re: [PATCH i-g-t v2 3/5] lib/igt_kms: Add function to list connected connectors

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

On 06/11/24 - 15:20, Kamil Konieczny wrote:
> Hi Louis,
> On 2024-10-22 at 12:28:37 +0200, 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().
> > 
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > ---
> >  lib/igt_kms.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  lib/igt_kms.h |  1 +
> >  2 files changed, 55 insertions(+)
> > 
> > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > index 40c0a207cd17..44b546ae52dc 100644
> > --- a/lib/igt_kms.c
> > +++ b/lib/igt_kms.c
> > @@ -7181,3 +7181,57 @@ bool igt_wait_for_connector_status(int drm_fd, unsigned int connector_id, double
> >  		  connector_id);
> >  	return false;
> >  }
> > +
> > +/**
> > + * igt_get_connected_connectors - List all connectors reported as CONNECTED by DRM.
> > + *
> > + * @drm_fd: DRM file description
> > + * @connector_ids: out pointer for the list of connector ids connected. It must be freed by the
> > + * caller.
> > + *
> > + * The returns value is the number of connectors
> > + */
> > +int igt_get_connected_connectors(int drm_fd, uint32_t **connector_ids)
> > +{
> > +	int connected_count = 0;
> > +	drmModeResPtr resources;
> > +	struct timespec start, end;
> > +
> > +	*connector_ids = NULL;
> > +
> > +	resources = drmModeGetResources(drm_fd);
> > +	for (int j = 0; j < resources->count_connectors; j++) {
> > +		drmModeConnectorPtr connector = NULL;
> > +
> > +		connector = drmModeGetConnector(drm_fd, resources->connectors[j]);
> > +		/*
> > +		 * This time is required as sometimes some id in the connector list are not totally
> > +		 * ready or can disappear
> > +		 */
> > +		clock_gettime(CLOCK_MONOTONIC, &start);
> > +		clock_gettime(CLOCK_MONOTONIC, &end);

(same change as previous patch, end = start)

> > +		while (!connector &&
> > +		       igt_time_elapsed(&start, &end) < igt_default_detect_timeout()) {
> > +			connector = drmModeGetConnector(drm_fd, resources->connectors[j]);
> > +			clock_gettime(CLOCK_MONOTONIC, &end);
> > +		}

After this loop, we have either:
- connector != NULL and elapsed < timeout
- elapsed > timeout

> > +
> > +		if (igt_time_elapsed(&start, &end) < igt_default_detect_timeout()) {

If this condition is true, it means that connector must be set

> > +			igt_assert(connector);
> 
> Please try to not assert in lib functions, unless abslutly nessesery.
> Why not just return zero in case no connected connectors are detected.

So an assert here is only a hint for the developper, this should never 
fail. And if it fails, it is a bug, the previous loop must ensure that 
connector is set.
 
> Regards,
> Kamil
> 
> > +
> > +			if (connector->connection == DRM_MODE_CONNECTED) {
> > +				*connector_ids = reallocarray(*connector_ids,
> > +							      connected_count
> > +							      + 1, sizeof(**connector_ids));
> > +				igt_assert(*connector_ids);
> > +				(*connector_ids)[connected_count] = resources->connectors[j];
> > +				connected_count++;
> > +			}
> > +			drmModeFreeConnector(connector);
> > +		}
> > +	}
> > +	drmModeFreeResources(resources);
> > +
> > +	return
> > +	    connected_count;
> > +}
> > diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> > index 4cd4a4f65460..4674b271638a 100644
> > --- a/lib/igt_kms.h
> > +++ b/lib/igt_kms.h
> > @@ -1265,5 +1265,6 @@ float igt_default_detect_timeout(void);
> >  
> >  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.46.2
> >