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

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

[Jani Nikula]

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

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 | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 92072f56e418..e58f39e167fb 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,24 @@ out:
 	return ret;
 }
 
+static int
+gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
+{
+	int ret;
+
+	ret = do_gmbus_xfer(adapter, msgs, num);
+
+	/*
+	 * Passive adapters sometimes NAK the first probe. Retry once on
+	 * -ENXIO. See also the retry loop in drm_do_probe_ddc_edid, which bails
+	 * out on the first -ENXIO.
+	 */
+	if (ret == -ENXIO)
+		ret = do_gmbus_xfer(adapter, msgs, num);
+
+	return ret;
+}
+
 static u32 gmbus_func(struct i2c_adapter *adapter)
 {
 	return i2c_bit_algo.functionality(adapter) &
-- 
2.1.4

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

[Jani Nikula]

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);
+
+	return ret;
+}
+
 static u32 gmbus_func(struct i2c_adapter *adapter)
 {
 	return i2c_bit_algo.functionality(adapter) &
-- 
2.1.4

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

[Ville Syrjälä]

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.

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.

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

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

[Jani Nikula]

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

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

[Ville Syrjälä]

On Thu, May 28, 2015 at 02:36:01PM +0300, Jani Nikula wrote:
> 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?

I suppose for our normal use cases it should be unlikely.

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

I suppose the simple thing is good enough at least for now.

In fact, after another look I'm also not sure i2c-algo-bit is
entirely sane as it'll do a stop+start when retrying the address.
If it was originally meant to be a repeated start things might go
badly. Eg. for EDID read if it would for some reason ACK the
segment address, but then temporarily NAK one of the later
addresses, the segment address would get reset to 0 when the
STOP is issued. So we'd end up accessing the wrong segment
in the end. So retrying the full thing may be actually more
correct. I suppose if someone is going to do further work on
this i2c-algo-bit might need to be changed to reissue repeated
starts when it already issued one instead of doing the
stop+start unconditionally.

I also noticed that bus->force_bit locking is a bit lax in places
(in pre-existing code already). Something to look at later perhaps.

Anyway the patch seems sane enough so
Reviewed-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>

-- 
Ville Syrj�l�
Intel OTC

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

[Daniel Vetter]

On Thu, May 28, 2015 at 02:36:01PM +0300, Jani Nikula wrote:
> 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. ;)

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

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

[Jani Nikula]

On Thu, 28 May 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, May 28, 2015 at 02:36:01PM +0300, Jani Nikula wrote:
>> 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. ;)
>
> 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

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

[Jani Nikula]

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 on first message 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.

v3: Move retry within gmbux_xfer, retry only on first message.

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 | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 92072f56e418..5519ad3a9574 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, try = 0, reg_offset;
 	int ret = 0;
 
 	intel_aux_display_runtime_get(dev_priv);
@@ -502,6 +502,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
 	I915_WRITE(GMBUS0 + reg_offset, bus->reg0);
 
 	for (i = 0; i < num; i++) {
+retry:
 		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,18 @@ clear_err:
 			 adapter->name, msgs[i].addr,
 			 (msgs[i].flags & I2C_M_RD) ? 'r' : 'w', msgs[i].len);
 
+	/*
+	 * 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 && i == 0 && try++ == 0) {
+		DRM_DEBUG_KMS("GMBUS [%s] NAK on first message, retry\n",
+			      adapter->name);
+		goto retry;
+	}
+
 	goto out;
 
 timeout:
-- 
2.1.4

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

[Ville Syrjälä]

On Tue, Jun 02, 2015 at 11:29:23AM +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 on first message 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.
> 
> v3: Move retry within gmbux_xfer, retry only on first message.
> 
> 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 | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index 92072f56e418..5519ad3a9574 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, try = 0, reg_offset;
>  	int ret = 0;
>  
>  	intel_aux_display_runtime_get(dev_priv);
> @@ -502,6 +502,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
>  	I915_WRITE(GMBUS0 + reg_offset, bus->reg0);
>  
>  	for (i = 0; i < num; i++) {
> +retry:
>  		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,18 @@ clear_err:
>  			 adapter->name, msgs[i].addr,
>  			 (msgs[i].flags & I2C_M_RD) ? 'r' : 'w', msgs[i].len);
>  
> +	/*
> +	 * 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 && i == 0 && try++ == 0) {
> +		DRM_DEBUG_KMS("GMBUS [%s] NAK on first message, retry\n",
> +			      adapter->name);
> +		goto retry;
> +	}
> +

This would leave GMBUS0 set to 0 for the retry.

>  	goto out;
>  
>  timeout:
> -- 
> 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

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

[Jani Nikula]

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 on first message 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.

v3: Move retry within gmbux_xfer, retry only on first message.

v4: Initialize GMBUS0 on retry (Ville).

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 | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 92072f56e418..5f2e88d39baf 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, try = 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 < 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,18 @@ clear_err:
 			 adapter->name, msgs[i].addr,
 			 (msgs[i].flags & I2C_M_RD) ? 'r' : 'w', msgs[i].len);
 
+	/*
+	 * 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 && i == 0 && try++ == 0) {
+		DRM_DEBUG_KMS("GMBUS [%s] NAK on first message, retry\n",
+			      adapter->name);
+		goto retry;
+	}
+
 	goto out;
 
 timeout:
-- 
2.1.4

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

[Ville Syrjälä]

On Tue, Jun 02, 2015 at 01:42:52PM +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 on first message 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.
> 
> v3: Move retry within gmbux_xfer, retry only on first message.
> 
> v4: Initialize GMBUS0 on retry (Ville).
> 
> 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 | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index 92072f56e418..5f2e88d39baf 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, try = 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 < 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 */

This increment means we'll never retry if we decice to use
gmbus_xfer_index_read() for the first two messages. AFAICS that
should always be the case for EDID reads where we don't send the
segment address.

> @@ -576,6 +577,18 @@ clear_err:
>  			 adapter->name, msgs[i].addr,
>  			 (msgs[i].flags & I2C_M_RD) ? 'r' : 'w', msgs[i].len);
>  
> +	/*
> +	 * 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 && i == 0 && try++ == 0) {
> +		DRM_DEBUG_KMS("GMBUS [%s] NAK on first message, retry\n",
> +			      adapter->name);
> +		goto retry;
> +	}
> +
>  	goto out;
>  
>  timeout:
> -- 
> 2.1.4

-- 
Ville Syrj�l�
Intel OTC

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

[Jani Nikula]

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 on first message 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.

v3: Move retry within gmbux_xfer, retry only on first message.

v4: Initialize GMBUS0 on retry (Ville).

v5: Take index reads into account (Ville).

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 | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 92072f56e418..a64f26c670af 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, inc, try = 0, reg_offset;
 	int ret = 0;
 
 	intel_aux_display_runtime_get(dev_priv);
@@ -499,12 +499,14 @@ 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 < num; i += inc) {
+		inc = 1;
 		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 */
+			inc = 2; /* an index read is two msgs */
 		} else if (msgs[i].flags & I2C_M_RD) {
 			ret = gmbus_xfer_read(dev_priv, &msgs[i], 0);
 		} else {
@@ -576,6 +578,18 @@ clear_err:
 			 adapter->name, msgs[i].addr,
 			 (msgs[i].flags & I2C_M_RD) ? 'r' : 'w', msgs[i].len);
 
+	/*
+	 * Passive adapters sometimes NAK the first probe. Retry the first
+	 * message 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 && i == 0 && try++ == 0) {
+		DRM_DEBUG_KMS("GMBUS [%s] NAK on first message, retry\n",
+			      adapter->name);
+		goto retry;
+	}
+
 	goto out;
 
 timeout:
-- 
2.1.4

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

[Ville Syrjälä]

On Tue, Jun 02, 2015 at 07:21:15PM +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 on first message 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.
> 
> v3: Move retry within gmbux_xfer, retry only on first message.
> 
> v4: Initialize GMBUS0 on retry (Ville).
> 
> v5: Take index reads into account (Ville).
> 
> 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>

OK, I think I'm done shooting any more holes into this one:
Reviewed-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>

Hopefully it works too. I don't have very good intuition into the gmbus
hardware. I think one day I need to set up a bit-banged i2c slave and
play around with it just for fun.

> ---
>  drivers/gpu/drm/i915/intel_i2c.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index 92072f56e418..a64f26c670af 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, inc, try = 0, reg_offset;
>  	int ret = 0;
>  
>  	intel_aux_display_runtime_get(dev_priv);
> @@ -499,12 +499,14 @@ 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 < num; i += inc) {
> +		inc = 1;
>  		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 */
> +			inc = 2; /* an index read is two msgs */
>  		} else if (msgs[i].flags & I2C_M_RD) {
>  			ret = gmbus_xfer_read(dev_priv, &msgs[i], 0);
>  		} else {
> @@ -576,6 +578,18 @@ clear_err:
>  			 adapter->name, msgs[i].addr,
>  			 (msgs[i].flags & I2C_M_RD) ? 'r' : 'w', msgs[i].len);
>  
> +	/*
> +	 * Passive adapters sometimes NAK the first probe. Retry the first
> +	 * message 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 && i == 0 && try++ == 0) {
> +		DRM_DEBUG_KMS("GMBUS [%s] NAK on first message, retry\n",
> +			      adapter->name);
> +		goto retry;
> +	}
> +
>  	goto out;
>  
>  timeout:
> -- 
> 2.1.4

-- 
Ville Syrj�l�
Intel OTC

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

[Jani Nikula]

On Tue, 02 Jun 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, Jun 02, 2015 at 07:21:15PM +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 on first message 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.
>> 
>> v3: Move retry within gmbux_xfer, retry only on first message.
>> 
>> v4: Initialize GMBUS0 on retry (Ville).
>> 
>> v5: Take index reads into account (Ville).
>> 
>> 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>
>
> OK, I think I'm done shooting any more holes into this one:
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

\o/

Thanks for all the review. I suck.

> Hopefully it works too. I don't have very good intuition into the gmbus
> hardware. I think one day I need to set up a bit-banged i2c slave and
> play around with it just for fun.

AFAICT it should be functionally equivalent to the v1 patch that got
tested in the referenced bug. I'm hoping to get a test result for this
one too.


>
>> ---
>>  drivers/gpu/drm/i915/intel_i2c.c | 20 +++++++++++++++++---
>>  1 file changed, 17 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
>> index 92072f56e418..a64f26c670af 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, inc, try = 0, reg_offset;
>>  	int ret = 0;
>>  
>>  	intel_aux_display_runtime_get(dev_priv);
>> @@ -499,12 +499,14 @@ 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 < num; i += inc) {
>> +		inc = 1;
>>  		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 */
>> +			inc = 2; /* an index read is two msgs */
>>  		} else if (msgs[i].flags & I2C_M_RD) {
>>  			ret = gmbus_xfer_read(dev_priv, &msgs[i], 0);
>>  		} else {
>> @@ -576,6 +578,18 @@ clear_err:
>>  			 adapter->name, msgs[i].addr,
>>  			 (msgs[i].flags & I2C_M_RD) ? 'r' : 'w', msgs[i].len);
>>  
>> +	/*
>> +	 * Passive adapters sometimes NAK the first probe. Retry the first
>> +	 * message 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 && i == 0 && try++ == 0) {
>> +		DRM_DEBUG_KMS("GMBUS [%s] NAK on first message, retry\n",
>> +			      adapter->name);
>> +		goto retry;
>> +	}
>> +
>>  	goto out;
>>  
>>  timeout:
>> -- 
>> 2.1.4
>
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Jani Nikula, Intel Open Source Technology Center

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

[Jani Nikula]

On Tue, 02 Jun 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, Jun 02, 2015 at 07:21:15PM +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 on first message 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.
>> 
>> v3: Move retry within gmbux_xfer, retry only on first message.
>> 
>> v4: Initialize GMBUS0 on retry (Ville).
>> 
>> v5: Take index reads into account (Ville).
>> 
>> 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>
>
> OK, I think I'm done shooting any more holes into this one:
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Hopefully it works too. I don't have very good intuition into the gmbus
> hardware. I think one day I need to set up a bit-banged i2c slave and
> play around with it just for fun.

Pushed to drm-intel-fixes, with Tested-by from Jim Bride. Thanks for the
review.

BR,
Jani.



>
>> ---
>>  drivers/gpu/drm/i915/intel_i2c.c | 20 +++++++++++++++++---
>>  1 file changed, 17 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
>> index 92072f56e418..a64f26c670af 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, inc, try = 0, reg_offset;
>>  	int ret = 0;
>>  
>>  	intel_aux_display_runtime_get(dev_priv);
>> @@ -499,12 +499,14 @@ 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 < num; i += inc) {
>> +		inc = 1;
>>  		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 */
>> +			inc = 2; /* an index read is two msgs */
>>  		} else if (msgs[i].flags & I2C_M_RD) {
>>  			ret = gmbus_xfer_read(dev_priv, &msgs[i], 0);
>>  		} else {
>> @@ -576,6 +578,18 @@ clear_err:
>>  			 adapter->name, msgs[i].addr,
>>  			 (msgs[i].flags & I2C_M_RD) ? 'r' : 'w', msgs[i].len);
>>  
>> +	/*
>> +	 * Passive adapters sometimes NAK the first probe. Retry the first
>> +	 * message 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 && i == 0 && try++ == 0) {
>> +		DRM_DEBUG_KMS("GMBUS [%s] NAK on first message, retry\n",
>> +			      adapter->name);
>> +		goto retry;
>> +	}
>> +
>>  	goto out;
>>  
>>  timeout:
>> -- 
>> 2.1.4
>
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Jani Nikula, Intel Open Source Technology Center