[Intel-gfx] [PATCH v2] drm/i915: Fix DDC probe for passive adapters

[Ville Syrjälä]

On Thu, May 28, 2015 at 02:36:01PM +0300, Jani Nikula wrote:
> On Thu, 28 May 2015, Ville Syrjälä <ville.syrjala at linux.intel.com> 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 at intel.com>
> >> 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 at chris-wilson.co.uk>
> >> 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 at gmail.com>.
> >> 
> >> [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 at gmail.com>
> >> Cc: stable at vger.kernel.org
> >> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> >> ---
> >>  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?

I suppose for our normal use cases it should be unlikely.

> 
> > 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. ;)

I suppose the simple thing is good enough at least for now.

In fact, after another look I'm also not sure i2c-algo-bit is
entirely sane as it'll do a stop+start when retrying the address.
If it was originally meant to be a repeated start things might go
badly. Eg. for EDID read if it would for some reason ACK the
segment address, but then temporarily NAK one of the later
addresses, the segment address would get reset to 0 when the
STOP is issued. So we'd end up accessing the wrong segment
in the end. So retrying the full thing may be actually more
correct. I suppose if someone is going to do further work on
this i2c-algo-bit might need to be changed to reissue repeated
starts when it already issued one instead of doing the
stop+start unconditionally.

I also noticed that bus->force_bit locking is a bit lax in places
(in pre-existing code already). Something to look at later perhaps.

Anyway the patch seems sane enough so
Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>

-- 
Ville Syrjälä
Intel OTC