Skip to contentSkip to conversation

drm/i915/hsw: Fix workaround for server AUX channel clock divisor

PATCH Superseded [email protected] 4 participants 6 messages May 19, 2015 – May 22, 2015

Subsystems (2)
DRM drivers Intel i915

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 drm/i915/hsw: Fix workaround for server AUX channel clock divisor

From: Jim Bride <[email protected]>

According to the HSW b-spec we need to try clock divisors of 63 and 72, each 3 or more times, when attempting DP AUX channel communication on a server chipset. This actually wasn't happening due to a short-circuit that only checked the DP_AUX_CH_CTL_DONE bit in status rather than checking that the operation was done and that DP_AUX_CH_CTL_TIME_OUT_ERROR was not set.

Signed-off-by Jim Bride <[email protected]>
---
 drivers/gpu/drm/i915/intel_dp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_dp.c +2 -1
index 0edc305..c01a3f9 100644
@@ -895,7 +895,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 			if (status & DP_AUX_CH_CTL_DONE)
 				break;
 		}
-		if (status & DP_AUX_CH_CTL_DONE)
+		if ((status & DP_AUX_CH_CTL_DONE) &&
+		    !(status & DP_AUX_CH_CTL_TIME_OUT_ERROR))
 			break;
 	}
Signature · 6 lines
1.9.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Reviewed-by on v1 1/1
#1 lore raw

Hi Jim,

I checked the BSpec as well and there's nothing indicating that these two bits are mutually exclusive. They are both sticky though, and if the loop times out 5 times, both _DONE and _TIMEOUT may very well be set. In that case the current code would just exit and never bother to change clock dividers. So I think your code here is valid.

The only thing you may want to do is capture some of the IRC discussion though. In particular, whether or not this is really HSW-specific, how this affects other platforms and and the additional delays incurred by running through the loop again after changing AUX channel clock dividers would be good information to put in there. That's probably more of a bikeshed though.

Reviewed-by Todd Previte <[email protected]>

-T

Quoted 19 lines · On 5/19/2015 9:13 AM, [email protected] wrote:

From: Jim Bride <[email protected]>

According to the HSW b-spec we need to try clock divisors of 63 and 72, each 3 or more times, when attempting DP AUX channel communication on a server chipset. This actually wasn't happening due to a short-circuit that only checked the DP_AUX_CH_CTL_DONE bit in status rather than checking that the operation was done and that DP_AUX_CH_CTL_TIME_OUT_ERROR was not set.

Signed-off-by Jim Bride <[email protected]>
---
  drivers/gpu/drm/i915/intel_dp.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_dp.c +0 -0
index 0edc305..c01a3f9 100644

drivers/gpu/drm/i915/intel_dp.c intel_dp_aux_ch(struct intel_dp *intel_dp, · line 902

Quoted 9 lines
drivers/gpu/drm/i915/intel_dp.c +2 -1
@@ -895,7 +895,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
  			if (status & DP_AUX_CH_CTL_DONE)
  				break;
  		}
-		if (status & DP_AUX_CH_CTL_DONE)
+		if ((status & DP_AUX_CH_CTL_DONE) &&
+		    !(status & DP_AUX_CH_CTL_TIME_OUT_ERROR))
  			break;
  	}

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

#2 lore raw
Quoted 7 lines · Todd Previte

Hi Jim,

I checked the BSpec as well and there's nothing indicating that these two bits are mutually exclusive. They are both sticky though, and if the loop times out 5 times, both _DONE and _TIMEOUT may very well be set. In that case the current code would just exit and never bother to change clock dividers. So I think your code here is valid.

Shouldn't we we checking receive error as well then?

In fact looking at our code after the loop, it's expecting DONE to be set before it'll even report receive/timeout errors to the user as EIO/ETIMEDOUT, otherwise it'll just return EBUSY.

Quoted 46 lines

The only thing you may want to do is capture some of the IRC discussion though. In particular, whether or not this is really HSW-specific, how this affects other platforms and and the additional delays incurred by running through the loop again after changing AUX channel clock dividers would be good information to put in there. That's probably more of a bikeshed though.

Reviewed-by Todd Previte <[email protected]>

-T

Quoted 19 lines · On 5/19/2015 9:13 AM, [email protected] wrote:

From: Jim Bride <[email protected]>

According to the HSW b-spec we need to try clock divisors of 63 and 72, each 3 or more times, when attempting DP AUX channel communication on a server chipset. This actually wasn't happening due to a short-circuit that only checked the DP_AUX_CH_CTL_DONE bit in status rather than checking that the operation was done and that DP_AUX_CH_CTL_TIME_OUT_ERROR was not set.

Signed-off-by Jim Bride <[email protected]>
---
  drivers/gpu/drm/i915/intel_dp.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_dp.c +0 -0
index 0edc305..c01a3f9 100644

drivers/gpu/drm/i915/intel_dp.c intel_dp_aux_ch(struct intel_dp *intel_dp, · line 902

Quoted 9 lines
drivers/gpu/drm/i915/intel_dp.c +2 -1
@@ -895,7 +895,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
  			if (status & DP_AUX_CH_CTL_DONE)
  				break;
  		}
-		if (status & DP_AUX_CH_CTL_DONE)
+		if ((status & DP_AUX_CH_CTL_DONE) &&
+		    !(status & DP_AUX_CH_CTL_TIME_OUT_ERROR))
  			break;
  	}

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 6 lines
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
maintainer
#3 lore raw
Quoted 10 lines · Ville Syrjälä
Quoted 7 lines · Todd Previte

Hi Jim,

I checked the BSpec as well and there's nothing indicating that these two bits are mutually exclusive. They are both sticky though, and if the loop times out 5 times, both _DONE and _TIMEOUT may very well be set. In that case the current code would just exit and never bother to change clock dividers. So I think your code here is valid.

Shouldn't we we checking receive error as well then?

In other words,

drivers/gpu/drm/i915/intel_dp.c +2 -4
index 7cf3fd43071a..179af62d803d 100644
@@ -893,10 +893,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 				continue;
 			}
 			if (status & DP_AUX_CH_CTL_DONE)
-				break;
+				goto done;
 		}
-		if (status & DP_AUX_CH_CTL_DONE)
-			break;
 	}
 
 	if ((status & DP_AUX_CH_CTL_DONE) == 0) {

@@ -921,7 +919,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 		ret = -ETIMEDOUT;
 		goto out;
 	}
-
+done:
 	/* Unload any bytes sent back from the other side */
 	recv_bytes = ((status & DP_AUX_CH_CTL_MESSAGE_SIZE_MASK) >>
 		      DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT);
Quoted 59 lines

In fact looking at our code after the loop, it's expecting DONE to be set before it'll even report receive/timeout errors to the user as EIO/ETIMEDOUT, otherwise it'll just return EBUSY.

Quoted 46 lines

The only thing you may want to do is capture some of the IRC discussion though. In particular, whether or not this is really HSW-specific, how this affects other platforms and and the additional delays incurred by running through the loop again after changing AUX channel clock dividers would be good information to put in there. That's probably more of a bikeshed though.

Reviewed-by Todd Previte <[email protected]>

-T

Quoted 19 lines · On 5/19/2015 9:13 AM, [email protected] wrote:

From: Jim Bride <[email protected]>

According to the HSW b-spec we need to try clock divisors of 63 and 72, each 3 or more times, when attempting DP AUX channel communication on a server chipset. This actually wasn't happening due to a short-circuit that only checked the DP_AUX_CH_CTL_DONE bit in status rather than checking that the operation was done and that DP_AUX_CH_CTL_TIME_OUT_ERROR was not set.

Signed-off-by Jim Bride <[email protected]>
---
  drivers/gpu/drm/i915/intel_dp.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_dp.c +0 -0
index 0edc305..c01a3f9 100644

drivers/gpu/drm/i915/intel_dp.c intel_dp_aux_ch(struct intel_dp *intel_dp, · line 902

Quoted 9 lines
drivers/gpu/drm/i915/intel_dp.c +2 -1
@@ -895,7 +895,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
  			if (status & DP_AUX_CH_CTL_DONE)
  				break;
  		}
-		if (status & DP_AUX_CH_CTL_DONE)
+		if ((status & DP_AUX_CH_CTL_DONE) &&
+		    !(status & DP_AUX_CH_CTL_TIME_OUT_ERROR))
  			break;
  	}

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 6 lines
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
author
#4 lore raw
Quoted 32 lines · Jani Nikula
Quoted 10 lines · Ville Syrjälä
Quoted 7 lines · Todd Previte

Hi Jim,

I checked the BSpec as well and there's nothing indicating that these two bits are mutually exclusive. They are both sticky though, and if the loop times out 5 times, both _DONE and _TIMEOUT may very well be set. In that case the current code would just exit and never bother to change clock dividers. So I think your code here is valid.

Shouldn't we we checking receive error as well then?

In other words,

drivers/gpu/drm/i915/intel_dp.c +1 -3
index 7cf3fd43071a..179af62d803d 100644
@@ -893,10 +893,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 				continue;
 			}
 			if (status & DP_AUX_CH_CTL_DONE)
-				break;
+				goto done;
 		}
-		if (status & DP_AUX_CH_CTL_DONE)
-			break;
 	}
 
 	if ((status & DP_AUX_CH_CTL_DONE) == 0) {

drivers/gpu/drm/i915/intel_dp.c intel_dp_aux_ch(struct intel_dp *intel_dp, · line 925

Quoted 8 lines
drivers/gpu/drm/i915/intel_dp.c +1 -1
@@ -921,7 +919,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 		ret = -ETIMEDOUT;
 		goto out;
 	}
-
+done:
 	/* Unload any bytes sent back from the other side */
 	recv_bytes = ((status & DP_AUX_CH_CTL_MESSAGE_SIZE_MASK) >>
 		      DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT);

This looks good, except I think we might want something more like:

drivers/gpu/drm/i915/intel_dp.c +2 -3
index 0edc305..89ab714 100644
@@ -893,10 +893,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
                                continue;
                        }
                        if (status & DP_AUX_CH_CTL_DONE)
-                               break;
+                               goto done;;
                }
-               if (status & DP_AUX_CH_CTL_DONE)
-                       break;
        }
 
        if ((status & DP_AUX_CH_CTL_DONE) == 0) {

@@ -905,6 +903,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
                goto out;
        }
 
+done:
        /* Check for timeout or receive error.
         * Timeouts occur when the sink is not connected
         */

This way we actually set ret appropriately in the event of a receive error or a timeout.

Thoughts?

Jim

Quoted 55 lines
Quoted 55 lines

In fact looking at our code after the loop, it's expecting DONE to be set before it'll even report receive/timeout errors to the user as EIO/ETIMEDOUT, otherwise it'll just return EBUSY.

Quoted 46 lines

The only thing you may want to do is capture some of the IRC discussion though. In particular, whether or not this is really HSW-specific, how this affects other platforms and and the additional delays incurred by running through the loop again after changing AUX channel clock dividers would be good information to put in there. That's probably more of a bikeshed though.

Reviewed-by Todd Previte <[email protected]>

-T

Quoted 19 lines · On 5/19/2015 9:13 AM, [email protected] wrote:

From: Jim Bride <[email protected]>

According to the HSW b-spec we need to try clock divisors of 63 and 72, each 3 or more times, when attempting DP AUX channel communication on a server chipset. This actually wasn't happening due to a short-circuit that only checked the DP_AUX_CH_CTL_DONE bit in status rather than checking that the operation was done and that DP_AUX_CH_CTL_TIME_OUT_ERROR was not set.

Signed-off-by Jim Bride <[email protected]>
---
  drivers/gpu/drm/i915/intel_dp.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_dp.c +0 -0
index 0edc305..c01a3f9 100644

drivers/gpu/drm/i915/intel_dp.c intel_dp_aux_ch(struct intel_dp *intel_dp, · line 902

Quoted 9 lines
drivers/gpu/drm/i915/intel_dp.c +2 -1
@@ -895,7 +895,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
  			if (status & DP_AUX_CH_CTL_DONE)
  				break;
  		}
-		if (status & DP_AUX_CH_CTL_DONE)
+		if ((status & DP_AUX_CH_CTL_DONE) &&
+		    !(status & DP_AUX_CH_CTL_TIME_OUT_ERROR))
  			break;
  	}

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 14 lines
Quoted 7 lines
Signature · 6 lines
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

maintainer
#5 lore raw
Quoted 69 lines · Jim Bride
Quoted 32 lines · Jani Nikula
Quoted 10 lines · Ville Syrjälä
Quoted 7 lines · Todd Previte

Hi Jim,

I checked the BSpec as well and there's nothing indicating that these two bits are mutually exclusive. They are both sticky though, and if the loop times out 5 times, both _DONE and _TIMEOUT may very well be set. In that case the current code would just exit and never bother to change clock dividers. So I think your code here is valid.

Shouldn't we we checking receive error as well then?

In other words,

drivers/gpu/drm/i915/intel_dp.c +1 -3
index 7cf3fd43071a..179af62d803d 100644
@@ -893,10 +893,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 				continue;
 			}
 			if (status & DP_AUX_CH_CTL_DONE)
-				break;
+				goto done;
 		}
-		if (status & DP_AUX_CH_CTL_DONE)
-			break;
 	}
 
 	if ((status & DP_AUX_CH_CTL_DONE) == 0) {

drivers/gpu/drm/i915/intel_dp.c intel_dp_aux_ch(struct intel_dp *intel_dp, · line 925

Quoted 8 lines
drivers/gpu/drm/i915/intel_dp.c +1 -1
@@ -921,7 +919,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 		ret = -ETIMEDOUT;
 		goto out;
 	}
-
+done:
 	/* Unload any bytes sent back from the other side */
 	recv_bytes = ((status & DP_AUX_CH_CTL_MESSAGE_SIZE_MASK) >>
 		      DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT);

This looks good, except I think we might want something more like:

drivers/gpu/drm/i915/intel_dp.c +2 -3
index 0edc305..89ab714 100644
@@ -893,10 +893,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
                                continue;
                        }
                        if (status & DP_AUX_CH_CTL_DONE)
-                               break;
+                               goto done;;
                }
-               if (status & DP_AUX_CH_CTL_DONE)
-                       break;
        }
 
        if ((status & DP_AUX_CH_CTL_DONE) == 0) {

@@ -905,6 +903,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
                goto out;
        }
 
+done:
        /* Check for timeout or receive error.
         * Timeouts occur when the sink is not connected
         */
Quoted 4 lines

This way we actually set ret appropriately in the event of a receive error or a timeout.

Thoughts?

When we hit the "goto done" part, I think we have all the error conditions already covered within the innermost loop, and everything is okay. We have to drop out of the loops to hit the errors.

But I'm fine either way.

BR,
Jani

Quoted 73 lines

Jim

Quoted 69 lines
Quoted 62 lines

In fact looking at our code after the loop, it's expecting DONE to be set before it'll even report receive/timeout errors to the user as EIO/ETIMEDOUT, otherwise it'll just return EBUSY.

Quoted 46 lines

The only thing you may want to do is capture some of the IRC discussion though. In particular, whether or not this is really HSW-specific, how this affects other platforms and and the additional delays incurred by running through the loop again after changing AUX channel clock dividers would be good information to put in there. That's probably more of a bikeshed though.

Reviewed-by Todd Previte <[email protected]>

-T

Quoted 19 lines · On 5/19/2015 9:13 AM, [email protected] wrote:

From: Jim Bride <[email protected]>

According to the HSW b-spec we need to try clock divisors of 63 and 72, each 3 or more times, when attempting DP AUX channel communication on a server chipset. This actually wasn't happening due to a short-circuit that only checked the DP_AUX_CH_CTL_DONE bit in status rather than checking that the operation was done and that DP_AUX_CH_CTL_TIME_OUT_ERROR was not set.

Signed-off-by Jim Bride <[email protected]>
---
  drivers/gpu/drm/i915/intel_dp.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_dp.c +0 -0
index 0edc305..c01a3f9 100644

drivers/gpu/drm/i915/intel_dp.c intel_dp_aux_ch(struct intel_dp *intel_dp, · line 902

Quoted 9 lines
drivers/gpu/drm/i915/intel_dp.c +2 -1
@@ -895,7 +895,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
  			if (status & DP_AUX_CH_CTL_DONE)
  				break;
  		}
-		if (status & DP_AUX_CH_CTL_DONE)
+		if ((status & DP_AUX_CH_CTL_DONE) &&
+		    !(status & DP_AUX_CH_CTL_TIME_OUT_ERROR))
  			break;
  	}

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 6 lines
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx