Re: [PATCH i-g-t v2 2/5] lib/igt_kms: Add helper to wait for a specific status on a connector

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

On 06/11/24 - 15:17, Kamil Konieczny wrote:
> Hi Louis,
> On 2024-10-22 at 12:28:36 +0200, Louis Chauvet wrote:
> > During testing with chamelium, it is frequent to wait for a specific
> > connector status. This new helper is polling the DRM API to wait for this
> > status. This allows detecting new status without notifier systems (which
> > can fail if hot plug detection is not working properly on the device under
> > test.
> > 
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > ---
> >  lib/igt_kms.c | 38 ++++++++++++++++++++++++++++++++++++++
> >  lib/igt_kms.h |  3 +++
> >  2 files changed, 41 insertions(+)
> > 
> > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > index 195868646a14..40c0a207cd17 100644
> > --- a/lib/igt_kms.c
> > +++ b/lib/igt_kms.c
> > @@ -7143,3 +7143,41 @@ float igt_default_detect_timeout(void)
> >  
> >  	return timeout;
> >  }
> > +
> > +/**
> > + * igt_wait_for_connector_status - Wait for at most @timeout that the connector @connector_id
> > + * status become @drm_mode
> > + *
> > + * @drm_fd: drm file descriptor
> > + * @connector_id: connector to monitor
> > + * @timeout: maximum duration to wait, in second

 * @timeout: maximum duration to wait, in second. If 0.0, it will use the 
value of igt_default_detect_timeout().

> > + * @drm_mode: mode to wait for, see enum drmModeConnection
> > + *
> > + * Returns: true when the status is reached, false if there is a timeout
> > + */
> > +bool igt_wait_for_connector_status(int drm_fd, unsigned int connector_id, double timeout,
> > +				   int drm_mode)
> > +{
> > +	drmModeConnector *connector;
> > +	struct timespec start, end;
> > +
> > +	if (timeout == 0.0)
> 
> Floating comparision is tricky, I would suggest something like
> 	if (abs(timeout) < epsilon)

I know that float comparison are tricky, but the goal is to allow the 
caller to ask for the default timeout. I agree it is missing some 
documentation. 

Maybe I can do timeout == -1.0 (nobody will ask for a negative timeout 
except on purpose), is it better?
 
> > +		timeout = igt_default_detect_timeout();
> 
> What if it is still zero here?

It means that the user configured the timeout to 0.0, so this will 
probably probably fail, but on purpose.

I agree that this may not work as expected, see my change bellow:

> Regards,
> Kamil
> 
> > +
> > +	clock_gettime(CLOCK_MONOTONIC, &start);
> > +	clock_gettime(CLOCK_MONOTONIC, &end);

Change the previous line to:

	end = start;

> > +
> > +	while (igt_time_elapsed(&start, &end) <= timeout) {

So here the igt_time_elapsed returns 0.0, so there will be one attempt to 
detect the connector.

> > +		connector = drmModeGetConnector(drm_fd, connector_id);
> > +		if (connector && connector->connection == drm_mode) {
> > +			free(connector);
> > +			return true;
> > +		}
> > +		free(connector);
> > +		clock_gettime(CLOCK_MONOTONIC, &end);
> > +	}
> > +
> > +	igt_debug("Timeout waiting for connection status %d on connector %d\n", drm_mode,
> > +		  connector_id);
> > +	return false;
> > +}
> > diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> > index 4f0030264d9f..4cd4a4f65460 100644
> > --- a/lib/igt_kms.h
> > +++ b/lib/igt_kms.h
> > @@ -1263,4 +1263,7 @@ void igt_reset_link_params(int drm_fd, igt_output_t *output);
> >  
> >  float igt_default_detect_timeout(void);
> >  
> > +bool igt_wait_for_connector_status(int drm_fd, unsigned int connector_id, double timeout,
> > +				   int drm_mode);
> > +
> >  #endif /* __IGT_KMS_H__ */
> > 
> > -- 
> > 2.46.2
> >