From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: Fix DDC probe for passive adapters Date: Thu, 28 May 2015 14:28:34 +0200 Message-ID: <20150528122834.GF8341@phenom.ffwll.local> References: <1432803106-4105-1-git-send-email-jani.nikula@intel.com> <1432807539-11944-1-git-send-email-jani.nikula@intel.com> <20150528104808.GM18908@intel.com> <874mmxgcim.fsf@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Content-Disposition: inline In-Reply-To: <874mmxgcim.fsf@intel.com> Sender: stable-owner@vger.kernel.org To: Jani Nikula Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= , intel-gfx@lists.freedesktop.org, stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org On Thu, May 28, 2015 at 02:36:01PM +0300, Jani Nikula wrote: > On Thu, 28 May 2015, Ville Syrj=E4l=E4 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 a= s HDMI > >> devices, as they do not have a sink device in them to respond to a= ny AUX > >> traffic. When probing these dongles over the DDC, sometimes they w= ill > >> NAK the first attempt even though the transaction is valid and the= y > >> 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. > >>=20 > >> That, however, was thwarted by the fix for [1]: > >>=20 > >> commit 9292f37e1f5c79400254dca46f83313488093825 > >> Author: Eugeni Dodonov > >> Date: Thu Jan 5 09:34:28 2012 -0200 > >>=20 > >> drm: give up on edid retries when i2c bus is not responding > >>=20 > >> 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 wa= s > >> possible because the underlying i2c bit banging algorithm had retr= ies of > >> its own (which, of course, were part of the reason for the bug the > >> commit fixes). > >>=20 > >> Since its introduction in > >>=20 > >> commit f899fc64cda8569d0529452aafc0da31c042df2e > >> Author: Chris Wilson > >> Date: Tue Jul 20 15:44:45 2010 -0700 > >>=20 > >> drm/i915: use GMBUS to manage i2c links > >>=20 > >> we've been flipping back and forth enabling the GMBUS transfers, b= ut > >> 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. > >>=20 > >> Retry GMBUS once on -ENXIO to mitigate the issues with passive ada= pters. > >>=20 > >> This patch is based on the work, and commit message, by Todd Previ= te > >> . > >>=20 > >> [1] https://bugs.freedesktop.org/show_bug.cgi?id=3D41059 > >>=20 > >> v2: Don't retry if using bit banging. > >>=20 > >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=3D85924 > >> Cc: Todd Previte > >> Cc: stable@vger.kernel.org > >> Signed-off-by: Jani Nikula > >> --- > >> drivers/gpu/drm/i915/intel_i2c.c | 25 ++++++++++++++++++++++--- > >> 1 file changed, 22 insertions(+), 3 deletions(-) > >>=20 > >> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i9= 15/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) > >> } > >> =20 > >> 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 =3D container_of(adapter, > >> struct intel_gmbus, > >> @@ -593,6 +591,27 @@ out: > >> return ret; > >> } > >> =20 > >> +static int > >> +gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int= num) > >> +{ > >> + struct intel_gmbus *bus =3D container_of(adapter, struct intel_g= mbus, > >> + adapter); > >> + int ret; > >> + > >> + ret =3D 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 =3D=3D -ENXIO && !bus->force_bit) > >> + ret =3D 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. >=20 > Which is also the case for the retry loop in drm_do_probe_ddc_edid fo= r > errors other than -ENXIO. >=20 > How likely do you think it is to *not* get -ENXIO at first, but get i= t > in a later message? >=20 > > 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. >=20 > 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 frui= t. >=20 > Do you think this makes the driver worse? >=20 > 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/in= tel_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 =3D bus->dev_priv; - int i, reg_offset; + int i =3D 0, reg_offset; int ret =3D 0; =20 intel_aux_display_runtime_get(dev_priv); @@ -499,9 +499,10 @@ gmbus_xfer(struct i2c_adapter *adapter, =20 reg_offset =3D dev_priv->gpio_mmio_base; =20 +retry: I915_WRITE(GMBUS0 + reg_offset, bus->reg0); =20 - for (i =3D 0; i < num; i++) { + for (i; i < num; i++) { if (gmbus_is_index_read(msgs, i, num)) { ret =3D gmbus_xfer_index_read(dev_priv, &msgs[i]); i +=3D 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); =20 + if (bla) + goto retry; + goto out; =20 timeout: --- Totally untested ofc ;-) Cheers, Daniel --=20 Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch