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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 28 May 2015, Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Thu, May 28, 2015 at 02:36:01PM +0300, Jani Nikula wrote:
>> On Thu, 28 May 2015, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
>> >> 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@xxxxxxxxxxxxxxxxxx>
>> >> 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@xxxxxxxxx>.
>> >> 
>> >> [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@xxxxxxxxx>
>> >> Cc: stable@xxxxxxxxxxxxxxx
>> >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>> >> ---
>> >>  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. ;)
>
> Doesn't look too complicated tdrt here:
>
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index 92072f56e418..ae9f4be1b644 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 = bus->dev_priv;
> -	int i, reg_offset;
> +	int i = 0, reg_offset;
>  	int ret = 0;
>  
>  	intel_aux_display_runtime_get(dev_priv);
> @@ -499,9 +499,10 @@ gmbus_xfer(struct i2c_adapter *adapter,
>  
>  	reg_offset = dev_priv->gpio_mmio_base;
>  
> +retry:
>  	I915_WRITE(GMBUS0 + reg_offset, bus->reg0);
>  
> -	for (i = 0; i < num; i++) {
> +	for (i; i < num; i++) {
>  		if (gmbus_is_index_read(msgs, i, num)) {
>  			ret = gmbus_xfer_index_read(dev_priv, &msgs[i]);
>  			i += 1;  /* set i to the index of the read xfer */
> @@ -576,6 +577,9 @@ clear_err:
>  			 adapter->name, msgs[i].addr,
>  			 (msgs[i].flags & I2C_M_RD) ? 'r' : 'w', msgs[i].len);
>  
> +	if (bla)
> +		goto retry;

Bla indeed. Already too many gotos in this piece of code to my taste...

> +
>  	goto out;
>  
>  timeout:
>
> ---
> Totally untested ofc ;-)

Hey don't worry, so was mine. But at least mine compiles. ;)

BR,
Jani.



>
> Cheers, Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux