[PATCH i-g-t] lib/igt_kms: add retry loop in igt_enable_connectors()

[Luca Coelho]

On Wed, 2024-05-15 at 19:22 +0200, Kamil Konieczny wrote:
> Hi Luca,
> On 2024-05-14 at 12:49:57 +0300, Luca Coelho wrote:
> > Sometimes the connector is not fully initialized yet when we try to
> > read it in igt_enable_connectors() and this causes a warning to be
> > issued and the test to fail.  This can happen in normal situations,
> > because there's no synchronization between the driver and IGT.  In
> > some tests we load the module and immediately start reading the
> > information, but the driver may not be fully initialized at that
> > point, causing the problem.
> > 
> > To prevent this, add a retry loop in igt_enable_connectors() so we
> > retry a few times, after a small delay, to wait for the initialization
> > to complete.
> > 
> > The number of retries and the delay between retries were chose after
> > running lots of tests, but these values may have to be adjusted after
> > running more tests on different machines.
> > 
> > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10436
> > Signed-off-by: Luca Coelho <luciano.coelho at intel.com>
> > ---
> >  lib/igt_kms.c | 29 +++++++++++++++++++++++++----
> >  1 file changed, 25 insertions(+), 4 deletions(-)
> > 
> > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > index f82102144ea1..3832bf54a2fc 100644
> > --- a/lib/igt_kms.c
> > +++ b/lib/igt_kms.c
> > @@ -5386,7 +5386,10 @@ void igt_wait_for_vblank(int drm_fd, int crtc_offset)
> >   */
> >  void igt_enable_connectors(int drm_fd)
> >  {
> > +#define MAX_TRIES	10
> > +#define SLEEP_DURATION	50000
> >  	drmModeRes *res;
> > +	int tries;
> >  
> >  	res = drmModeGetResources(drm_fd);
> >  	if (!res)
> > @@ -5395,10 +5398,28 @@ void igt_enable_connectors(int drm_fd)
> >  	for (int i = 0; i < res->count_connectors; i++) {
> >  		drmModeConnector *c;
> >  
> > -		/* Do a probe. This may be the first action after booting */
> > -		c = drmModeGetConnector(drm_fd, res->connectors[i]);
> > -		if (!c) {
> > -			igt_warn("Could not read connector %u: %m\n", res->connectors[i]);
> > +		/*
> > +		 * The kernel returns the count of connectors before
> > +		 * they're all fully set up, so we can have a race
> > +		 * condition where we try to get the connector when
> > +		 * it's not fully set up yet.  To avoid failing here
> > +		 * in these cases, retry a few times.
> > +		 */
> > +		for (tries = 0; tries < MAX_TRIES; tries++) {
> > +			/* Do a probe. This may be the first action after booting */
> > +			c = drmModeGetConnector(drm_fd, res->connectors[i]);
> > +			if (c)
> > +				break;
> > +
> > +			igt_info("Could not read connector %u: %m (try %d of %d)\n",
> ----------- ^
> Use igt_debug or print summary after for(tries < MAX_TRIES) loop.

Right, this is better as igt_debug.  I'll change it.


> 
> > +				  res->connectors[i], tries, MAX_TRIES);
> > +
> > +			usleep(SLEEP_DURATION);
> > +		}
> > +
> > +		if (tries == MAX_TRIES) {
> > +			igt_warn("Could not read connector %u after %d tries, skipping\n",
> > +				 res->connectors[i], MAX_TRIES);
> >  			continue;
> 
> How long could it take in worst case?

The values I used were based on a lot of trials on a local machine
where I could reproduce the issue.  We usually pass after the first
retry, which is after a 50 msec delay.  The maximum number of retries I
have needed was 4, which is a 200 msec delay.  This is the worst case I
have seen.  But to be sure, I chose to do 10 retries (500 msecs).

IOW, if everything goes well (i.e. no retry is needed), there's no
additional delay compared to before my patch.  If a retry is needed,
the added delay is going to be between 50 and 200 msecs.

I'll send v2 in a bit.

--
Cheers,
Luca.