On Thu, 28 May 2015, Daniel Vetter <daniel@xxxxxxxx> wrote: > On Thu, May 28, 2015 at 02:36:01PM +0300, Jani Nikula wrote: >> On Thu, 28 May 2015, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote: >> > On Thu, May 28, 2015 at 01:05:39PM +0300, Jani Nikula wrote: >> >> Passive DP->DVI/HDMI dongles on DP++ ports show up to the system as HDMI >> >> devices, as they do not have a sink device in them to respond to any AUX >> >> traffic. When probing these dongles over the DDC, sometimes they will >> >> NAK the first attempt even though the transaction is valid and they >> >> support the DDC protocol. The retry loop inside of >> >> drm_do_probe_ddc_edid() would normally catch this case and try the >> >> transaction again, resulting in success. >> >> >> >> That, however, was thwarted by the fix for [1]: >> >> >> >> commit 9292f37e1f5c79400254dca46f83313488093825 >> >> Author: Eugeni Dodonov <eugeni.dodonov@xxxxxxxxx> >> >> Date: Thu Jan 5 09:34:28 2012 -0200 >> >> >> >> drm: give up on edid retries when i2c bus is not responding >> >> >> >> This added code to exit immediately if the return code from the >> >> i2c_transfer function was -ENXIO in order to reduce the amount of time >> >> spent in waiting for unresponsive or disconnected devices. That was >> >> possible because the underlying i2c bit banging algorithm had retries of >> >> its own (which, of course, were part of the reason for the bug the >> >> commit fixes). >> >> >> >> Since its introduction in >> >> >> >> commit f899fc64cda8569d0529452aafc0da31c042df2e >> >> Author: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> >> >> Date: Tue Jul 20 15:44:45 2010 -0700 >> >> >> >> drm/i915: use GMBUS to manage i2c links >> >> >> >> we've been flipping back and forth enabling the GMBUS transfers, but >> >> we've settled since then. The GMBUS implementation does not do any >> >> retries, however, bailing out of the drm_do_probe_ddc_edid() retry loop >> >> on first encounter of -ENXIO. This, combined with Eugeni's commit, broke >> >> the retry on -ENXIO. >> >> >> >> Retry GMBUS once on -ENXIO to mitigate the issues with passive adapters. >> >> >> >> This patch is based on the work, and commit message, by Todd Previte >> >> <tprevite@xxxxxxxxx>. >> >> >> >> [1] https://bugs.freedesktop.org/show_bug.cgi?id=41059 >> >> >> >> v2: Don't retry if using bit banging. >> >> >> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85924 >> >> Cc: Todd Previte <tprevite@xxxxxxxxx> >> >> Cc: stable@xxxxxxxxxxxxxxx >> >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx> >> >> --- >> >> drivers/gpu/drm/i915/intel_i2c.c | 25 ++++++++++++++++++++++--- >> >> 1 file changed, 22 insertions(+), 3 deletions(-) >> >> >> >> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c >> >> index 92072f56e418..c3f72b509d1f 100644 >> >> --- a/drivers/gpu/drm/i915/intel_i2c.c >> >> +++ b/drivers/gpu/drm/i915/intel_i2c.c >> >> @@ -478,9 +478,7 @@ gmbus_xfer_index_read(struct drm_i915_private *dev_priv, struct i2c_msg *msgs) >> >> } >> >> >> >> static int >> >> -gmbus_xfer(struct i2c_adapter *adapter, >> >> - struct i2c_msg *msgs, >> >> - int num) >> >> +do_gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num) >> >> { >> >> struct intel_gmbus *bus = container_of(adapter, >> >> struct intel_gmbus, >> >> @@ -593,6 +591,27 @@ out: >> >> return ret; >> >> } >> >> >> >> +static int >> >> +gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num) >> >> +{ >> >> + struct intel_gmbus *bus = container_of(adapter, struct intel_gmbus, >> >> + adapter); >> >> + int ret; >> >> + >> >> + ret = do_gmbus_xfer(adapter, msgs, num); >> >> + >> >> + /* >> >> + * Passive adapters sometimes NAK the first probe. Retry once on -ENXIO >> >> + * for GMBUS transfers; the bit banging algorithm has retries >> >> + * internally. See also the retry loop in drm_do_probe_ddc_edid, which >> >> + * bails out on the first -ENXIO. >> >> + */ >> >> + if (ret == -ENXIO && !bus->force_bit) >> >> + ret = do_gmbus_xfer(adapter, msgs, num); >> > >> > i2c-algo-bit does the retry for each msg when sending the address. This >> > on the other hand will redo the entire transfer. So if we get a nak but >> > not on the first message we end up repeating the succesful part of the >> > transfer twice. >> >> Which is also the case for the retry loop in drm_do_probe_ddc_edid for >> errors other than -ENXIO. >> >> How likely do you think it is to *not* get -ENXIO at first, but get it >> in a later message? >> >> > To match i2c-algo-bit we'd need to do the retry for each individual >> > message. I suppose that would make the error handling more >> > complicated as we'd supposedly still need to clear the error, but >> > then repeat the same msg without generating a STOP in between. >> >> Looking at the code, and i2c-algo-bit.c, I'm not sure if I'd be >> comfortable backporting something like that to stable. It does get >> complicated. So sure, this is an attempt to pick the low hanging fruit. >> >> Do you think this makes the driver worse? >> >> I plead item (c) of the Reviewer's statement of oversight. ;) > > Doesn't look too complicated tdrt here: > > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c > index 92072f56e418..ae9f4be1b644 100644 > --- a/drivers/gpu/drm/i915/intel_i2c.c > +++ b/drivers/gpu/drm/i915/intel_i2c.c > @@ -486,7 +486,7 @@ gmbus_xfer(struct i2c_adapter *adapter, > struct intel_gmbus, > adapter); > struct drm_i915_private *dev_priv = bus->dev_priv; > - int i, reg_offset; > + int i = 0, reg_offset; > int ret = 0; > > intel_aux_display_runtime_get(dev_priv); > @@ -499,9 +499,10 @@ gmbus_xfer(struct i2c_adapter *adapter, > > reg_offset = dev_priv->gpio_mmio_base; > > +retry: > I915_WRITE(GMBUS0 + reg_offset, bus->reg0); > > - for (i = 0; i < num; i++) { > + for (i; i < num; i++) { > if (gmbus_is_index_read(msgs, i, num)) { > ret = gmbus_xfer_index_read(dev_priv, &msgs[i]); > i += 1; /* set i to the index of the read xfer */ > @@ -576,6 +577,9 @@ clear_err: > adapter->name, msgs[i].addr, > (msgs[i].flags & I2C_M_RD) ? 'r' : 'w', msgs[i].len); > > + if (bla) > + goto retry; Bla indeed. Already too many gotos in this piece of code to my taste... > + > goto out; > > timeout: > > --- > Totally untested ofc ;-) Hey don't worry, so was mine. But at least mine compiles. ;) BR, Jani. > > Cheers, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- Jani Nikula, Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html