[Intel-gfx] [PATCH v2] drm/i915: Fix DDC probe for passive adapters
Daniel Vetter
daniel at ffwll.ch
Thu May 28 05:28:34 PDT 2015
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?
>
> > 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;
+
goto out;
timeout:
---
Totally untested ofc ;-)
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list