[PATCH 0/2] drm/i915/dp: native aux defer retry timeout & retry limit

[PATCH 0/2] drm/i915/dp: native aux defer retry timeout & retry limit

[Jani Nikula]

These should fix the hang in the bug referenced in patch 2/2. Based on
some links the actual error might be a firmware bug in the docking
station, but we shouldn't trust on DP sinks to not keep replying aux
defer indefinitely.

Even with the docking station's firmware fixed, this may not be enough
to actually get the output working. They are branch devices, possibly
MST devices, which we still suck at.

But hey, at least we won't freeze the system.

The retry timeout and limit are in line with Thierry's dp aux series
[1], so this should in fact ease the transition by catching any
regressions caused by this change up front.

BR,
Jani.

[1] http://mid.gmane.org/1390332263-11974-1-git-send-email-treding@nvidia.com


Jani Nikula (2):
  drm/i915/dp: increase native aux defer retry timeout
  drm/i915/dp: add native aux defer retry limit

 drivers/gpu/drm/i915/intel_dp.c |   19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

-- 
1.7.9.5

[PATCH 1/2] drm/i915/dp: increase native aux defer retry timeout

[Jani Nikula]

Give more slack to sink devices before retrying on native aux
defer. AFAICT the 100 us timeout was not based on the DP spec.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 50381f765504..abbc80243234 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -596,7 +596,7 @@ intel_dp_aux_native_write(struct intel_dp *intel_dp,
 		if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_ACK)
 			break;
 		else if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_DEFER)
-			udelay(100);
+			usleep_range(400, 500);
 		else
 			return -EIO;
 	}
@@ -648,7 +648,7 @@ intel_dp_aux_native_read(struct intel_dp *intel_dp,
 			return ret - 1;
 		}
 		else if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_DEFER)
-			udelay(100);
+			usleep_range(400, 500);
 		else
 			return -EIO;
 	}
-- 
1.7.9.5

[PATCH 2/2] drm/i915/dp: add native aux defer retry limit

[Jani Nikula]

Retrying indefinitely places too much trust on the aux implementation of
the sink devices.

Reported-by: Daniel Martin <consume.noise@gmail.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=71267
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index abbc80243234..bd1df502bc34 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -577,6 +577,7 @@ intel_dp_aux_native_write(struct intel_dp *intel_dp,
 	uint8_t	msg[20];
 	int msg_bytes;
 	uint8_t	ack;
+	int retry;
 
 	if (WARN_ON(send_bytes > 16))
 		return -E2BIG;
@@ -588,19 +589,21 @@ intel_dp_aux_native_write(struct intel_dp *intel_dp,
 	msg[3] = send_bytes - 1;
 	memcpy(&msg[4], send, send_bytes);
 	msg_bytes = send_bytes + 4;
-	for (;;) {
+	for (retry = 0; retry < 7; retry++) {
 		ret = intel_dp_aux_ch(intel_dp, msg, msg_bytes, &ack, 1);
 		if (ret < 0)
 			return ret;
 		ack >>= 4;
 		if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_ACK)
-			break;
+			return send_bytes;
 		else if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_DEFER)
 			usleep_range(400, 500);
 		else
 			return -EIO;
 	}
-	return send_bytes;
+
+	DRM_ERROR("too many retries, giving up\n");
+	return -EIO;
 }
 
 /* Write a single byte to the aux channel in native mode */
@@ -622,6 +625,7 @@ intel_dp_aux_native_read(struct intel_dp *intel_dp,
 	int reply_bytes;
 	uint8_t ack;
 	int ret;
+	int retry;
 
 	if (WARN_ON(recv_bytes > 19))
 		return -E2BIG;
@@ -635,7 +639,7 @@ intel_dp_aux_native_read(struct intel_dp *intel_dp,
 	msg_bytes = 4;
 	reply_bytes = recv_bytes + 1;
 
-	for (;;) {
+	for (retry = 0; retry < 7; retry++) {
 		ret = intel_dp_aux_ch(intel_dp, msg, msg_bytes,
 				      reply, reply_bytes);
 		if (ret == 0)
@@ -652,6 +656,9 @@ intel_dp_aux_native_read(struct intel_dp *intel_dp,
 		else
 			return -EIO;
 	}
+
+	DRM_ERROR("too many retries, giving up\n");
+	return -EIO;
 }
 
 static int
-- 
1.7.9.5

Re: [PATCH 1/2] drm/i915/dp: increase native aux defer retry timeout

[Chris Wilson]

On Tue, Feb 11, 2014 at 11:52:04AM +0200, Jani Nikula wrote:
> Give more slack to sink devices before retrying on native aux
> defer. AFAICT the 100 us timeout was not based on the DP spec.

If the issue is that there is an unknown amount of time that must be
waited before the device is ready, should we not try increasing the
sleep value on each iteration?

If you were to do that, I would put patch 2 (which lgtm) first.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915/dp: increase native aux defer retry timeout

[Jani Nikula]

On Tue, 11 Feb 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Tue, Feb 11, 2014 at 11:52:04AM +0200, Jani Nikula wrote:
>> Give more slack to sink devices before retrying on native aux
>> defer. AFAICT the 100 us timeout was not based on the DP spec.
>
> If the issue is that there is an unknown amount of time that must be
> waited before the device is ready, should we not try increasing the
> sleep value on each iteration?
>
> If you were to do that, I would put patch 2 (which lgtm) first.

All of this code will change when we eventually move to the common DP
aux helpers. I'd like to get such fixes in the common code after it's
been merged. Diverging now makes the conversion harder.

I also thought this would be the better order bisect-wise. 2/2 first
might be unnecessarily strict, possibly creating a false culprit in
bisect for any other possible DP issues. Maybe that is a silly thing to
worry about.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 2/2] drm/i915/dp: add native aux defer retry limit

[Daniel Vetter]

On Tue, Feb 11, 2014 at 10:52 AM, Jani Nikula <jani.nikula@intel.com> wrote:
> Retrying indefinitely places too much trust on the aux implementation of
> the sink devices.
>
> Reported-by: Daniel Martin <consume.noise@gmail.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=71267
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Brings us closer to the new shared dp aux helpers and generally makes
sense so

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

on both. I also think we need a cc: stable on the 2nd on here.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/2] drm/i915/dp: add native aux defer retry limit

[Daniel Vetter]

On Tue, Feb 11, 2014 at 03:17:58PM +0100, Daniel Vetter wrote:
> On Tue, Feb 11, 2014 at 10:52 AM, Jani Nikula <jani.nikula@intel.com> wrote:
> > Retrying indefinitely places too much trust on the aux implementation of
> > the sink devices.
> >
> > Reported-by: Daniel Martin <consume.noise@gmail.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=71267
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> Brings us closer to the new shared dp aux helpers and generally makes
> sense so
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> on both. I also think we need a cc: stable on the 2nd on here.

I've pulled them into -fixes, thanks for the patches.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/2] drm/i915/dp: add native aux defer retry limit

[Jani Nikula]

On Thu, 13 Feb 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Feb 11, 2014 at 03:17:58PM +0100, Daniel Vetter wrote:
>> On Tue, Feb 11, 2014 at 10:52 AM, Jani Nikula <jani.nikula@intel.com> wrote:
>> > Retrying indefinitely places too much trust on the aux implementation of
>> > the sink devices.
>> >
>> > Reported-by: Daniel Martin <consume.noise@gmail.com>
>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=71267
>> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> 
>> Brings us closer to the new shared dp aux helpers and generally makes
>> sense so
>> 
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> 
>> on both. I also think we need a cc: stable on the 2nd on here.

This comes a bit late I know, but I think it would be the safest to cc:
stable both of them.

Jani.

>
> I've pulled them into -fixes, thanks for the patches.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center