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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@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@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@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@gmail.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Jani Nikula <jani.nikula@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.

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