Date: Tue, 2 Jan 2018 11:12:18 -0800
From: Rodrigo Vivi <rodrigo.vivi@intel.com>
To: Stefan =?iso-8859-1?Q?Br=FCns?= <stefan.bruens@rwth-aachen.de>
Cc: dri-devel@lists.freedesktop.org, David Airlie <airlied@linux.ie>,
        intel-gfx@lists.freedesktop.org,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] drm/i915: Try EDID bitbanging on HDMI after failed
 read
Message-ID: <20180102191218.53beopvnxsg2etmg@intel.com>
References: <a39f080b-81a5-4c93-b3f7-7cb0a58daca3@rwthex-w2-a.rwth-ad.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <a39f080b-81a5-4c93-b3f7-7cb0a58daca3@rwthex-w2-a.rwth-ad.de>
User-Agent: NeoMutt/20170912 (1.9.0)
Sender: linux-kernel-owner@vger.kernel.org

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