From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com ([192.55.52.88]:61962 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752290AbbE1KsW (ORCPT ); Thu, 28 May 2015 06:48:22 -0400 Date: Thu, 28 May 2015 13:48:08 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Jani Nikula Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: Fix DDC probe for passive adapters Message-ID: <20150528104808.GM18908@intel.com> References: <1432803106-4105-1-git-send-email-jani.nikula@intel.com> <1432807539-11944-1-git-send-email-jani.nikula@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1432807539-11944-1-git-send-email-jani.nikula@intel.com> Sender: stable-owner@vger.kernel.org List-ID: 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 > 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 > 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 > . > > [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 > 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(-) > > 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. 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. > + > + return ret; > +} > + > static u32 gmbus_func(struct i2c_adapter *adapter) > { > return i2c_bit_algo.functionality(adapter) & > -- > 2.1.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrj�l� Intel OTC