[Intel-gfx] [PATCH 1/2] Give up on edid retries when i2c tells us that bus is not there

[Jean Delvare]

Hi Dave, Eugeni, Alex,

On Tue, 18 Oct 2011 11:02:00 +0100, Dave Airlie wrote:
> > This allows to avoid talking to a non-existent bus repeatedly until we
> > finally timeout. The non-existent bus is signalled by -ENXIO error,
> > provided by i2c_algo_bit:bit_doAddress call.
> >
> > As the advantage of such change, all the other routines which use
> > drm_get_edid would benefit for this timeout.
> >
> > This change should fix
> > https://bugs.freedesktop.org/show_bug.cgi?id=41059 and improve overall
> > edid detection timing by 10-30% in most cases, and by a much larger margin
> > in case of phantom outputs.
> 
> Jean, Alex,
> 
> I'm thinking of thowing this into -next, any objections?

This seems to be the wrong place for the test. The code below is really
testing for non-responsive (possibly not present) EDID, NOT
"non-existent adapter". Non-existent adapter should be checked in the
firmware if possible, or failing that, by testing the bus lines at bus
creation time, as i2c_algo_bit.bit_test=1 is doing. While this setting
has been known to cause trouble recently (not per se but because it was
triggering a bug somewhere else in the radeon driver), it might still
have value, and could be changed to a per-adapter setting by exporting
the test function (I have a patch ready doing exactly this) and letting
video drivers test their I2C buses and discard the non-working ones if
they want.

I am worried that the patch below will cause regressions on other
machines. There's a comment right before the loop which reads:

	/* The core i2c driver will automatically retry the transfer if the
	 * adapter reports EAGAIN. However, we find that bit-banging transfers
	 * are susceptible to errors under a heavily loaded machine and
	 * generate spurious NAKs and timeouts. Retrying the transfer
	 * of the individual block a few times seems to overcome this.
	 */

So the retries are there for a reason, and -ENXIO is exactly what you
get on spurious NAKs.

One thing which may make sense would be to make the retry count a
module parameter, instead of a hard-coded value, so that users can
lower the value if they care more about boot time than reliability. But
again, ideally problematic buses would not have been created in the
first place.

> >
> > Signed-off-by: Eugeni Dodonov <eugeni.dodonov at intel.com>
> > ---
> >  drivers/gpu/drm/drm_edid.c |    5 +++++
> >  1 files changed, 5 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 7425e5c..1bca6d7 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -265,6 +265,11 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
> >                        }
> >                };
> >                ret = i2c_transfer(adapter, msgs, 2);
> > +               if (ret == -ENXIO) {
> > +                       DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n",
> > +                                       adapter->name);
> > +                       break;
> > +               }
> >        } while (ret != 2 && --retries);
> >
> >        return ret == 2 ? 0 : -1;

-- 
Jean Delvare