Radeon regression fix
Subsystems (2)
This patch fixes a regression introduced between 2.6.39 & 3.1-rc1 whereby the displayport AUX channel stopped re-trying commands that elicited a DEFER response.
drivers/gpu/drm/radeon/atombios_dp.c +0 -0
@@ -60,11 +60,13 @@ static int radeon_process_aux_ch(struct radeon_i2c_chan *chan, int index = GetIndexIntoMasterTable(COMMAND,
ProcessAuxChannelTransaction);
unsigned char *base;
int recv_bytes;
+ int retry_count = 0;
memset(&args, 0, sizeof(args));
base = (unsigned char *)rdev->mode_info.atom_context->scratch;
+retry:
memcpy(base, send, send_bytes);
args.v1.lpAuxRequest = 0;
@@ -79,6 +81,16 @@ static int radeon_process_aux_ch(struct radeon_i2c_chan *chan,
*ack = args.v1.ucReplyStatus;
+ /* defer */
+ if (args.v1.ucReplyStatus == 0x20){
+ DRM_DEBUG_KMS("dp_aux_ch defer\n");
+ /* 10 is an arbitrary value from the pre-regression patch
+ in practice I've never seen more than one */
+ if (retry_count++ < 10)
+ goto retry;
+ return -EBUSY;
+ }
+
/* timeout */
if (args.v1.ucReplyStatus == 1) {
DRM_DEBUG_KMS("dp_aux_ch timeout\n");
Quoted 4 lines · Brad Campbell
This patch fixes a regression introduced between 2.6.39 & 3.1-rc1 whereby the displayport AUX channel stopped re-trying commands that elicited a DEFER response.
It should still be retrying, just restructured slightly. The retry logic just moved into radeon_dp_i2c_aux_ch(), radeon_dp_aux_native_write(), and radeon_dp_aux_native_read(), e.g.,
else if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER) udelay(400);
Perhaps the delay is causing a problem. Does removing the udelay(400); help?
Alex
Quoted 43 lines
Signed-off-by Brad Campbell <[email protected]>drivers/gpu/drm/radeon/atombios_dp.c +2
-0@@ -60,11 +60,13 @@ static int radeon_process_aux_ch(struct radeon_i2c_chan *chan, int index = GetIndexIntoMasterTable(COMMAND, ProcessAuxChannelTransaction); unsigned char *base; int recv_bytes; + int retry_count = 0; memset(&args, 0, sizeof(args)); base = (unsigned char *)rdev->mode_info.atom_context->scratch; +retry: memcpy(base, send, send_bytes);args.v1.lpAuxRequest = 0;
drivers/gpu/drm/radeon/atombios_dp.c +4
-0@@ -79,6 +81,16 @@ static int radeon_process_aux_ch(struct radeon_i2c_chan *chan, *ack = args.v1.ucReplyStatus; + /* defer */ + if (args.v1.ucReplyStatus == 0x20){ + DRM_DEBUG_KMS("dp_aux_ch defer\n"); + /* 10 is an arbitrary value from the pre-regressionpatch
+ in practice I've never seen more than one */
+ if (retry_count++ < 10)
+ goto retry;
+ return-EBUSY;
+ }
+
/* timeout */
if (args.v1.ucReplyStatus == 1) {DRM_DEBUG_KMS("dp_aux_ch timeout\n");
Quoted 11 lines · Alex Deucher
Quoted 4 lines · Brad Campbell
This patch fixes a regression introduced between 2.6.39& 3.1-rc1 whereby the displayport AUX channel stopped re-trying commands that elicited a DEFER response.
It should still be retrying, just restructured slightly. The retry logic just moved into
radeon_dp_i2c_aux_ch(),radeon_dp_aux_native_write(), andradeon_dp_aux_native_read(), e.g.,else if ((ack& AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER) udelay(400);
One problem with that logic I'm afraid.
if (ret == 0)
return -EPROTO;
if (ret < 0)
return ret;
if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_ACK)
return ret;
else if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER)
udelay(400);
else
return -EIO;
}ret == 0 with a defer as there is no data in the packet. It never even gets past the first hurdle.
Quoted 6 lines · Alex Deucher
Quoted 4 lines · Brad Campbell
This patch fixes a regression introduced between 2.6.39& 3.1-rc1 whereby the displayport AUX channel stopped re-trying commands that elicited a DEFER response.
Quoted 8 lines
It should still be retrying, just restructured slightly. The retry logic just moved into
radeon_dp_i2c_aux_ch(),radeon_dp_aux_native_write(), andradeon_dp_aux_native_read(), e.g.,else if ((ack& AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER) udelay(400);Perhaps the delay is causing a problem. Does removing the udelay(400); help?
How's this look ?
drivers/gpu/drm/radeon/atombios_dp.c +0 -0
@@ -128,12 +128,14 @@ static int radeon_dp_aux_native_write(struct radeon_connector *radeon_connector,while (1 ) { ret = radeon_process_aux_ch(dig_connector->dp_i2c_bus, msg, msg_bytes, NULL, 0,
delay, &ack);
+ if (ret == 0 && ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER)){
+ udelay(400);
+ continue;
+ }
if (ret < 0)
return ret;
if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_ACK)
break;
- else if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER)
- udelay(400);
else
return -EIO;
}
@@ -158,14 +160,16 @@ static int radeon_dp_aux_native_read(struct radeon_connector *radeon_connector,
while (1) {
ret = radeon_process_aux_ch(dig_connector->dp_i2c_bus,
msg, msg_bytes, recv, recv_bytes, delay, &ack);
+ if (ret == 0 && ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER)){
+ udelay(400);
+ continue;
+ }
if (ret == 0)
return -EPROTO;
if (ret < 0)
return ret;
if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_ACK)
return ret;
- else if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER)
- udelay(400);
else
return -EIO;
}Quoted 13 lines · Brad Campbell
Quoted 12 lines · Alex Deucher
Quoted 5 lines · Brad Campbell
This patch fixes a regression introduced between 2.6.39& 3.1-rc1 whereby the displayport AUX channel stopped re-trying commands that elicited a DEFER
response.It should still be retrying, just restructured slightly. The retry logic just moved into
radeon_dp_i2c_aux_ch(),radeon_dp_aux_native_write(), andradeon_dp_aux_native_read(), e.g.,
Quoted 5 lines
Quoted 5 lines
else if ((ack&
AUX_NATIVE_REPLY_MASK) ==AUX_NATIVE_REPLY_DEFER) udelay(400);Perhaps the delay is causing a problem. Does removing the udelay(400); help?
Looking at it with a nights sleep, it's obvious the code path in aux_native_write is ok. Is this a bit cleaner than the last patch?
drivers/gpu/drm/radeon/atombios_dp.c +0 -0
@@ -158,14 +158,17 @@ static int radeon_dp_aux_native_read(struct radeon_connector *radeon_connector,while (1 ) { ret = radeon_process_aux_ch(dig_connector->dp_i2c_bus, msg, msg_bytes, recv,
recv_bytes, delay, &ack);
- if (ret == 0)
+ if (ret == 0){
+ if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER){
+ udelay(400);
+ continue;
+ }
return -EPROTO;
+ }
if (ret < 0)
return ret;
if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_ACK)
return ret;
- else if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER)
- udelay(400);
else
return -EIO;
}Quoted 17 lines · Brad Campbell
Quoted 16 lines · Brad Campbell
Quoted 14 lines · Alex Deucher
Quoted 6 lines · Brad Campbell
This patch fixes a regression introduced between 2.6.39& 3.1-rc1 whereby the displayport AUX channel stopped re-trying commands that elicited a DEFER
response.It should still be retrying, just restructured slightly. The retry logic just moved into
radeon_dp_i2c_aux_ch(),radeon_dp_aux_native_write(), andradeon_dp_aux_native_read(), e.g.,
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
else if ((ack&
AUX_NATIVE_REPLY_MASK) ==AUX_NATIVE_REPLY_DEFER) udelay(400);Perhaps the delay is causing a problem. Does removing the udelay(400); help?
Looking at it with a nights sleep, it's obvious the code path in
aux_native_writeis ok. Is this a bit cleaner than the last patch?
Looks pretty good. I was thinking of something more like this (sorry for the lack of a patch, I'm away from my source trees at the moment):
while (1) {
ret = radeon_process_aux_ch(dig_connector->dp_i2c_bus,
msg, msg_bytes, recv, recv_bytes, delay, &ack);
if (ret < 0)
return ret;
if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_ACK)
return ret;
else if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER)
udelay(400);
else if (ret == 0)
return -EPROTO;
else
return -EIO;
}Thanks for tracking this down.
Alex
Quoted 32 lines
drivers/gpu/drm/radeon/atombios_dp.c +6
-2@@ -158,14 +158,17 @@ static int radeon_dp_aux_native_read(struct radeon_connector *radeon_connector, while (1) { ret = radeon_process_aux_ch(dig_connector->dp_i2c_bus, msg, msg_bytes, recv, recv_bytes, delay, &ack); - if (ret == 0) + if (ret == 0){ + if ((ack & AUX_NATIVE_REPLY_MASK) ==AUX_NATIVE_REPLY_DEFER ){ + udelay(400); + continue; + } return -EPROTO; + } if (ret < 0) return ret; if ((ack & AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_ACK) return ret; - else if ((ack & AUX_NATIVE_REPLY_MASK) ==
AUX_NATIVE_REPLY_DEFER)
- udelay(400);
else
return-EIO;
}
Quoted 8 lines · Alex Deucher
On Thu, Sep 29, 2011 at 8:23 PM, Brad Campbell
Quoted 2 lines
Looking at it with a nights sleep, it's obvious the code path in
aux_native_writeis ok. Is this a bit cleaner than the last patch?Looks pretty good. I was thinking of something more like this (sorry for the lack of a patch, I'm away from my source trees at the moment):
Quoted 15 lines
while (1) {
ret = radeon_process_aux_ch(dig_connector->dp_i2c_bus,
msg, msg_bytes, recv, recv_bytes, delay,&ack);
if (ret< 0)
return ret;
if ((ack& AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_ACK)
return ret;
else if ((ack& AUX_NATIVE_REPLY_MASK) == AUX_NATIVE_REPLY_DEFER)
udelay(400);
else if (ret == 0)
return -EPROTO;
else
return -EIO;
}Yep, that looks cleaner.
My only thought was the pre-3.0 code had a limit to the number of retries. Was that for a specific reason or is it ok to attempt to retry indefinitely if we receive a DEFER ?
Quoted 2 lines
Thanks for tracking this down.
No worries. I learned quite a bit about some kernel internals and more than a few quirks about apple hardware while muddling around.
Quoted 28 lines · Brad Campbell
Quoted 14 lines · Alex Deucher
On Thu, Sep 29, 2011 at 8:23 PM, Brad Campbell
Quoted 2 lines
Looking at it with a nights sleep, it's obvious the code path in
aux_native_writeis ok. Is this a bit cleaner than the last patch?Looks pretty good. I was thinking of something more like this (sorry for the lack of a patch, I'm away from my source trees at the moment):
while (1) {
ret =radeon_process_aux_ch(dig_connector->dp_i2c_bus, msg, msg_bytes, recv, recv_bytes, delay,&ack);Quoted 12 lines
if (ret< 0)
return ret;
if ((ack& AUX_NATIVE_REPLY_MASK) ==AUX_NATIVE_REPLY_ACK) return ret;
else if ((ack& AUX_NATIVE_REPLY_MASK) ==AUX_NATIVE_REPLY_DEFER)
udelay(400);
else if (ret == 0)
return-EPROTO;
else
return-EIO;
}
Quoted 5 lines
Yep, that looks cleaner.
My only thought was the pre-3.0 code had a limit to the number of retries. Was that for a specific reason or is it ok to attempt to retry indefinitely if we receive a DEFER ?
I need to double check the DP spec, but I think it's 4. The while (1) loops in radeon_dp_aux_native_write() and radeon_dp_aux_native_read() should probably be changed to:
for (retry = 0; retry < 4; retry++)
to match what we do in radeon_dp_i2c_aux_ch().
Alex
PATCH 1/2 drm/radeon/kms: fix regression in DP aux defer handling
From: Alex Deucher <[email protected]>
An incorrect ordering in the error checking code lead to DP aux defer being skipped in the aux native write path. Move the bytes transferred check (ret == 0) below the defer check.
Tracked down by: Brad Campbell <[email protected]>
Fixes:
https://bugs.freedesktop.org/show_bug.cgi?id=41121
--- drivers/gpu/drm/radeon/atombios_dp.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/gpu/drm/radeon/atombios_dp.c +2 -2
@@ -158,14 +158,14 @@ static int radeon_dp_aux_native_read(struct radeon_connector *radeon_connector,while (1 ) { ret =radeon_process_aux_ch (dig_connector->dp_i2c_bus, msg, msg_bytes, recv, recv_bytes, delay, &ack); -if (ret ==0 ) -return -EPROTO ;if (ret <0 )return ret;if ((ack &AUX_NATIVE_REPLY_MASK ) ==AUX_NATIVE_REPLY_ACK )return ret;else if ((ack &AUX_NATIVE_REPLY_MASK ) ==AUX_NATIVE_REPLY_DEFER )udelay (400 ); +else if (ret ==0 ) +return -EPROTO ;else return -EIO ; }
Signature · 1 line
1.7.1.1
PATCH 2/2 drm/radeon/kms: add retry limits for native DP aux defer
In reply to Alex DeucherFrom: Alex Deucher <[email protected]>
The previous code could potentially loop forever. Limit the number of DP aux defer retries to 4 for native aux transactions, same as i2c over aux transactions.
Noticed by: Brad Campbell <[email protected]>
--- drivers/gpu/drm/radeon/atombios_dp.c | 12 ++++++++---- 1 files changed, 8 insertions(+), 4 deletions(-)
drivers/gpu/drm/radeon/atombios_dp.c +8 -4
@@ -115,6 +115,7 @@ static int radeon_dp_aux_native_write(struct radeon_connector *radeon_connector,u8 msg[20 ];int msg_bytes = send_bytes +4 ;u8 ack; +unsigned retry;if (send_bytes >16 )return -1 ;
@@ -125,20 +126,20 @@ static int radeon_dp_aux_native_write(struct radeon_connector *radeon_connector, msg[3 ] = (msg_bytes <<4 ) | (send_bytes -1 );memcpy (&msg[4 ], send, send_bytes); -while (1 ) { +for (retry =0 ; retry <4 ; retry++) { ret =radeon_process_aux_ch (dig_connector->dp_i2c_bus, msg, msg_bytes,NULL ,0 , delay, &ack);if (ret <0 )return ret;if ((ack &AUX_NATIVE_REPLY_MASK ) ==AUX_NATIVE_REPLY_ACK ) -break ; +return send_bytes;else if ((ack &AUX_NATIVE_REPLY_MASK ) ==AUX_NATIVE_REPLY_DEFER )udelay (400 );else return -EIO ; } -return send_bytes; +return -EIO ; } static int radeon_dp_aux_native_read(struct radeon_connector *radeon_connector,
@@ -149,13 +150,14 @@ static int radeon_dp_aux_native_read(struct radeon_connector *radeon_connector,int msg_bytes =4 ;u8 ack;int ret; +unsigned retry; msg[0 ] = address; msg[1 ] = address >>8 ; msg[2 ] =AUX_NATIVE_READ <<4 ; msg[3 ] = (msg_bytes <<4 ) | (recv_bytes -1 ); -while (1 ) { +for (retry =0 ; retry <4 ; retry++) { ret =radeon_process_aux_ch (dig_connector->dp_i2c_bus, msg, msg_bytes, recv, recv_bytes, delay, &ack);if (ret <0 )
@@ -169,6 +171,8 @@ static int radeon_dp_aux_native_read(struct radeon_connector *radeon_connector,else return -EIO ; } + +return -EIO ; } static void radeon_write_dpcd_reg(struct radeon_connector *radeon_connector,
Signature · 1 line
1.7.1.1