| Message ID | a39f080b-81a5-4c93-b3f7-7cb0a58daca3@rwthex-w2-a.rwth-ad.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Dec 31, 2017 at 10:34:54PM +0000, Stefan Brüns wrote: > The ACK/NACK implementation as found in e.g. the G965 has the falling > clock edge and the release of the data line after the ACK for the received > byte happen at the same time. > > This is conformant with the I2C specification, which allows a zero hold > time, see footnote [3]: "A device must internally provide a hold time of > at least 300 ns for the SDA signal (with respect to the V IH(min) of the > SCL signal) to bridge the undefined region of the falling edge of SCL." > > Some HDMI-to-VGA converters apparently fail to adhere to this requirement > and latch SDA at the falling clock edge, so instead of an ACK > sometimes a NACK is read and the slave (i.e. the EDID ROM) ends the > transfer. > > The bitbanging releases the data line for the ACK only 1/4 bit time after > the falling clock edge, so a slave will see the correct value no matter > if it samples at the rising or the falling clock edge or in the center. > > Fallback to bitbanging is already done for the CRT connector. > > Bug: https://bugs.freedesktop.org/show_bug.cgi?id=92685 s/Bug:/Bugzilla: Did we get the confirmation that this also fix the Skylake issue initially reported? > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > > --- > > Changes in v2: > - Fix/enhance commit message, no code changes > > drivers/gpu/drm/i915/intel_hdmi.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index 4dea833f9d1b..847cda4c017c 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -1573,12 +1573,20 @@ intel_hdmi_set_edid(struct drm_connector *connector) > struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); > struct edid *edid; > bool connected = false; > + struct i2c_adapter *i2c; > > intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); > > - edid = drm_get_edid(connector, > - intel_gmbus_get_adapter(dev_priv, > - intel_hdmi->ddc_bus)); > + i2c = intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus); > + > + edid = drm_get_edid(connector, i2c); > + > + if (!edid && !intel_gmbus_is_forced_bit(i2c)) { > + DRM_DEBUG_KMS("HDMI GMBUS EDID read failed, retry using GPIO bit-banging\n"); > + intel_gmbus_force_bit(i2c, true); > + edid = drm_get_edid(connector, i2c); > + intel_gmbus_force_bit(i2c, false); > + } Approach seems fine for this case. I just wonder what would be the risks of forcing this bit and edid read when nothing is present on the other end? > > intel_hdmi_dp_dual_mode_detect(connector, edid != NULL); > > -- > 2.15.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoting Rodrigo Vivi (2018-01-02 19:12:18) > On Sun, Dec 31, 2017 at 10:34:54PM +0000, Stefan Brüns wrote: > > + edid = drm_get_edid(connector, i2c); > > + > > + if (!edid && !intel_gmbus_is_forced_bit(i2c)) { > > + DRM_DEBUG_KMS("HDMI GMBUS EDID read failed, retry using GPIO bit-banging\n"); > > + intel_gmbus_force_bit(i2c, true); > > + edid = drm_get_edid(connector, i2c); > > + intel_gmbus_force_bit(i2c, false); > > + } > > Approach seems fine for this case. > I just wonder what would be the risks of forcing this bit and edid read when nothing is present on the other end? Should be no more risky than using GMBUS as the bit-banging is the underlying HW protocol; it should just be adding an extra delay to the disconnected probe. Offset against the chance that it fixes detection of borderline devices. I would say that given the explanation above, the question is why not apply it universally? (Bonus points for including the explanation as comments.) -Chris
On Tue, 02 Jan 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Rodrigo Vivi (2018-01-02 19:12:18) >> On Sun, Dec 31, 2017 at 10:34:54PM +0000, Stefan Brüns wrote: >> > + edid = drm_get_edid(connector, i2c); >> > + >> > + if (!edid && !intel_gmbus_is_forced_bit(i2c)) { >> > + DRM_DEBUG_KMS("HDMI GMBUS EDID read failed, retry using GPIO bit-banging\n"); >> > + intel_gmbus_force_bit(i2c, true); >> > + edid = drm_get_edid(connector, i2c); >> > + intel_gmbus_force_bit(i2c, false); >> > + } >> >> Approach seems fine for this case. >> I just wonder what would be the risks of forcing this bit and edid read when nothing is present on the other end? > > Should be no more risky than using GMBUS as the bit-banging is the > underlying HW protocol; it should just be adding an extra delay to > the disconnected probe. Offset against the chance that it fixes > detection of borderline devices. > > I would say that given the explanation above, the question is why not > apply it universally? (Bonus points for including the explanation as > comments.) I'm wondering, is gmbus too fast for the adapters, does gmbus generally have different timing for the ack/nak as described in the commit message than bit banging, or are the adapters just plain buggy? Do we have any control over gmbus timings (don't have the time to peruse the bpsec just now)? BR, Jani. > -Chris > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wednesday, January 3, 2018 8:14:47 AM CET Jani Nikula wrote: > On Tue, 02 Jan 2018, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Quoting Rodrigo Vivi (2018-01-02 19:12:18) > > > >> On Sun, Dec 31, 2017 at 10:34:54PM +0000, Stefan Brüns wrote: > >> > + edid = drm_get_edid(connector, i2c); > >> > + > >> > + if (!edid && !intel_gmbus_is_forced_bit(i2c)) { > >> > + DRM_DEBUG_KMS("HDMI GMBUS EDID read failed, retry using > >> > GPIO bit-banging\n"); + intel_gmbus_force_bit(i2c, true); > >> > + edid = drm_get_edid(connector, i2c); > >> > + intel_gmbus_force_bit(i2c, false); > >> > + } > >> > >> Approach seems fine for this case. > >> I just wonder what would be the risks of forcing this bit and edid read > >> when nothing is present on the other end?> > > Should be no more risky than using GMBUS as the bit-banging is the > > underlying HW protocol; it should just be adding an extra delay to > > the disconnected probe. Offset against the chance that it fixes > > detection of borderline devices. > > > > I would say that given the explanation above, the question is why not > > apply it universally? (Bonus points for including the explanation as > > comments.) > > I'm wondering, is gmbus too fast for the adapters, does gmbus generally > have different timing for the ack/nak as described in the commit message > than bit banging, or are the adapters just plain buggy? Do we have any > control over gmbus timings (don't have the time to peruse the bpsec just > now)? I have seen two different behaviours, one on the ~2009 GM965, the other on the ~2013 Haswell. The Haswell provides a 250..500ns hold time, the other does not. There is a flag in the GMBUS0 register, GMBUS_HOLD_EXT, "300ns hold time, rsvd on Pineview". The driver does not set this flag. Possibly it is always set/ implied on the Haswell (which is post-Pineview), and should be set for anything older than Pineview. There is another odd fact with the GM965, according to the register setting it should run at 100 kBit/s, but it only runs at 30 kBit/s. The Haswell runs at 100 kBit/s, as specified. As there are also idle periods ever 8 bytes, the EDID read takes 270ms before it fails. The bitbanging code, running at 45 kBit/s (2 * 20us per clock cycle plus overhead) on the other hand just needs 58 ms, but keeps one core busy (udelay). Unfortunately I currently have no older system than the Haswell available, so I can not check if the GMBUS_HOLD_EXT flag has any effect. Kind regards, Stefan
On Sun, Dec 31, 2017 at 11:34:54PM +0100, Stefan Brüns wrote: > The ACK/NACK implementation as found in e.g. the G965 has the falling > clock edge and the release of the data line after the ACK for the received > byte happen at the same time. > > This is conformant with the I2C specification, which allows a zero hold > time, see footnote [3]: "A device must internally provide a hold time of > at least 300 ns for the SDA signal (with respect to the V IH(min) of the > SCL signal) to bridge the undefined region of the falling edge of SCL." > > Some HDMI-to-VGA converters apparently fail to adhere to this requirement > and latch SDA at the falling clock edge, so instead of an ACK > sometimes a NACK is read and the slave (i.e. the EDID ROM) ends the > transfer. > > The bitbanging releases the data line for the ACK only 1/4 bit time after > the falling clock edge, so a slave will see the correct value no matter > if it samples at the rising or the falling clock edge or in the center. > > Fallback to bitbanging is already done for the CRT connector. > > Bug: https://bugs.freedesktop.org/show_bug.cgi?id=92685 > > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> > > --- > > Changes in v2: > - Fix/enhance commit message, no code changes Ok, found v2, merged this one instead. -Daniel > > drivers/gpu/drm/i915/intel_hdmi.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index 4dea833f9d1b..847cda4c017c 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -1573,12 +1573,20 @@ intel_hdmi_set_edid(struct drm_connector *connector) > struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); > struct edid *edid; > bool connected = false; > + struct i2c_adapter *i2c; > > intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); > > - edid = drm_get_edid(connector, > - intel_gmbus_get_adapter(dev_priv, > - intel_hdmi->ddc_bus)); > + i2c = intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus); > + > + edid = drm_get_edid(connector, i2c); > + > + if (!edid && !intel_gmbus_is_forced_bit(i2c)) { > + DRM_DEBUG_KMS("HDMI GMBUS EDID read failed, retry using GPIO bit-banging\n"); > + intel_gmbus_force_bit(i2c, true); > + edid = drm_get_edid(connector, i2c); > + intel_gmbus_force_bit(i2c, false); > + } > > intel_hdmi_dp_dual_mode_detect(connector, edid != NULL); > > -- > 2.15.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 4dea833f9d1b..847cda4c017c 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1573,12 +1573,20 @@ intel_hdmi_set_edid(struct drm_connector *connector) struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); struct edid *edid; bool connected = false; + struct i2c_adapter *i2c; intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); - edid = drm_get_edid(connector, - intel_gmbus_get_adapter(dev_priv, - intel_hdmi->ddc_bus)); + i2c = intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus); + + edid = drm_get_edid(connector, i2c); + + if (!edid && !intel_gmbus_is_forced_bit(i2c)) { + DRM_DEBUG_KMS("HDMI GMBUS EDID read failed, retry using GPIO bit-banging\n"); + intel_gmbus_force_bit(i2c, true); + edid = drm_get_edid(connector, i2c); + intel_gmbus_force_bit(i2c, false); + } intel_hdmi_dp_dual_mode_detect(connector, edid != NULL);
The ACK/NACK implementation as found in e.g. the G965 has the falling clock edge and the release of the data line after the ACK for the received byte happen at the same time. This is conformant with the I2C specification, which allows a zero hold time, see footnote [3]: "A device must internally provide a hold time of at least 300 ns for the SDA signal (with respect to the V IH(min) of the SCL signal) to bridge the undefined region of the falling edge of SCL." Some HDMI-to-VGA converters apparently fail to adhere to this requirement and latch SDA at the falling clock edge, so instead of an ACK sometimes a NACK is read and the slave (i.e. the EDID ROM) ends the transfer. The bitbanging releases the data line for the ACK only 1/4 bit time after the falling clock edge, so a slave will see the correct value no matter if it samples at the rising or the falling clock edge or in the center. Fallback to bitbanging is already done for the CRT connector. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=92685 Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de> --- Changes in v2: - Fix/enhance commit message, no code changes drivers/gpu/drm/i915/intel_hdmi.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)