From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH v5] drm/i915: Fix DDC probe for passive adapters Date: Tue, 2 Jun 2015 19:40:35 +0300 Message-ID: <20150602164035.GC5176@intel.com> References: <20150602130909.GZ5176@intel.com> <1433262075-23775-1-git-send-email-jani.nikula@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: <1433262075-23775-1-git-send-email-jani.nikula@intel.com> Sender: stable-owner@vger.kernel.org To: Jani Nikula Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org On Tue, Jun 02, 2015 at 07:21:15PM +0300, Jani Nikula wrote: > Passive DP->DVI/HDMI dongles on DP++ ports show up to the system as H= DMI > 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. >=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 tim= e > 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). >=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, 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 lo= op > on first encounter of -ENXIO. This, combined with Eugeni's commit, br= oke > the retry on -ENXIO. >=20 > Retry GMBUS once on -ENXIO on first message to mitigate the issues wi= th > passive adapters. >=20 > This patch is based on the work, and commit message, by Todd Previte > . >=20 > [1] https://bugs.freedesktop.org/show_bug.cgi?id=3D41059 >=20 > v2: Don't retry if using bit banging. >=20 > v3: Move retry within gmbux_xfer, retry only on first message. >=20 > v4: Initialize GMBUS0 on retry (Ville). >=20 > v5: Take index reads into account (Ville). >=20 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=3D85924 > Cc: Todd Previte > Cc: stable@vger.kernel.org > Signed-off-by: Jani Nikula OK, I think I'm done shooting any more holes into this one: Reviewed-by: Ville Syrj=E4l=E4 Hopefully it works too. I don't have very good intuition into the gmbus hardware. I think one day I need to set up a bit-banged i2c slave and play around with it just for fun. > --- > drivers/gpu/drm/i915/intel_i2c.c | 20 +++++++++++++++++--- > 1 file changed, 17 insertions(+), 3 deletions(-) >=20 > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/= intel_i2c.c > index 92072f56e418..a64f26c670af 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, inc, try =3D 0, reg_offset; > int ret =3D 0; > =20 > intel_aux_display_runtime_get(dev_priv); > @@ -499,12 +499,14 @@ 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 < num; i +=3D inc) { > + inc =3D 1; > 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 */ > + inc =3D 2; /* an index read is two msgs */ > } else if (msgs[i].flags & I2C_M_RD) { > ret =3D gmbus_xfer_read(dev_priv, &msgs[i], 0); > } else { > @@ -576,6 +578,18 @@ clear_err: > adapter->name, msgs[i].addr, > (msgs[i].flags & I2C_M_RD) ? 'r' : 'w', msgs[i].len); > =20 > + /* > + * Passive adapters sometimes NAK the first probe. Retry the first > + * message once on -ENXIO for GMBUS transfers; the bit banging algo= rithm > + * 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 && i =3D=3D 0 && try++ =3D=3D 0) { > + DRM_DEBUG_KMS("GMBUS [%s] NAK on first message, retry\n", > + adapter->name); > + goto retry; > + } > + > goto out; > =20 > timeout: > --=20 > 2.1.4 --=20 Ville Syrj=E4l=E4 Intel OTC