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

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 28 May 2015, Ville Syrjälä <ville.syrjala@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@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.

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?

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


BR,
Jani.



>
>> +
>> +	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

-- 
Jani Nikula, Intel Open Source Technology Center