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.
Signed-off-by: Brad Campbell brad@fnarfbargle.com
diff --git a/drivers/gpu/drm/radeon/atombios_dp.c b/drivers/gpu/drm/radeon/atombios_dp.c index 7ad43c6..b8450f4 100644 --- a/drivers/gpu/drm/radeon/atombios_dp.c +++ b/drivers/gpu/drm/radeon/atombios_dp.c @@ -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");
On Thu, Sep 29, 2011 at 10:21 AM, Brad Campbell brad@fnarfbargle.com wrote:
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
Signed-off-by: Brad Campbell brad@fnarfbargle.com
diff --git a/drivers/gpu/drm/radeon/atombios_dp.c b/drivers/gpu/drm/radeon/atombios_dp.c index 7ad43c6..b8450f4 100644 --- a/drivers/gpu/drm/radeon/atombios_dp.c +++ b/drivers/gpu/drm/radeon/atombios_dp.c @@ -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");
On 29/09/11 22:36, Alex Deucher wrote:
On Thu, Sep 29, 2011 at 10:21 AM, Brad Campbellbrad@fnarfbargle.com wrote:
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);
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.
On 29/09/11 22:36, Alex Deucher wrote:
On Thu, Sep 29, 2011 at 10:21 AM, Brad Campbellbrad@fnarfbargle.com wrote:
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?
How's this look ?
diff --git a/drivers/gpu/drm/radeon/atombios_dp.c b/drivers/gpu/drm/radeon/atombios_dp.c index 7ad43c6..aa5624a 100644 --- a/drivers/gpu/drm/radeon/atombios_dp.c +++ b/drivers/gpu/drm/radeon/atombios_dp.c @@ -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; }
On 29/09/11 23:21, Brad Campbell wrote:
On 29/09/11 22:36, Alex Deucher wrote:
On Thu, Sep 29, 2011 at 10:21 AM, Brad Campbellbrad@fnarfbargle.com wrote:
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?
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?
diff --git a/drivers/gpu/drm/radeon/atombios_dp.c b/drivers/gpu/drm/radeon/atombios_dp.c index 7ad43c6..aacc97d 100644 --- a/drivers/gpu/drm/radeon/atombios_dp.c +++ b/drivers/gpu/drm/radeon/atombios_dp.c @@ -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; }
On Thu, Sep 29, 2011 at 8:23 PM, Brad Campbell lists2009@fnarfbargle.com wrote:
On 29/09/11 23:21, Brad Campbell wrote:
On 29/09/11 22:36, Alex Deucher wrote:
On Thu, Sep 29, 2011 at 10:21 AM, Brad Campbellbrad@fnarfbargle.com wrote:
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?
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?
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
diff --git a/drivers/gpu/drm/radeon/atombios_dp.c b/drivers/gpu/drm/radeon/atombios_dp.c index 7ad43c6..aacc97d 100644 --- a/drivers/gpu/drm/radeon/atombios_dp.c +++ b/drivers/gpu/drm/radeon/atombios_dp.c @@ -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; }
On 30/09/11 12:59, Alex Deucher wrote:
On Thu, Sep 29, 2011 at 8:23 PM, Brad Campbell
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?
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;}
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 ?
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.
On Fri, Sep 30, 2011 at 1:14 AM, Brad Campbell lists2009@fnarfbargle.com wrote:
On 30/09/11 12:59, Alex Deucher wrote:
On Thu, Sep 29, 2011 at 8:23 PM, Brad Campbell
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?
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; }
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
From: Alex Deucher alexander.deucher@amd.com
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 brad@fnarfbargle.com
Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=41121
Signed-off-by: Alex Deucher alexander.deucher@amd.com Cc: Brad Campbell brad@fnarfbargle.com Cc: stable@kernel.org --- drivers/gpu/drm/radeon/atombios_dp.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/radeon/atombios_dp.c b/drivers/gpu/drm/radeon/atombios_dp.c index 7ad43c6..f526fa7 100644 --- a/drivers/gpu/drm/radeon/atombios_dp.c +++ b/drivers/gpu/drm/radeon/atombios_dp.c @@ -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; }
From: Alex Deucher alexander.deucher@amd.com
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 lists2009@fnarfbargle.com
Signed-off-by: Alex Deucher alexander.deucher@amd.com Cc: Brad Campbell lists2009@fnarfbargle.com Cc: stable@kernel.org --- drivers/gpu/drm/radeon/atombios_dp.c | 12 ++++++++---- 1 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/radeon/atombios_dp.c b/drivers/gpu/drm/radeon/atombios_dp.c index f526fa7..4da2388 100644 --- a/drivers/gpu/drm/radeon/atombios_dp.c +++ b/drivers/gpu/drm/radeon/atombios_dp.c @@ -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,
dri-devel@lists.freedesktop.org
-
Alex Deucher -
alexdeucher＠gmail.com
-
Brad Campbell -
Brad Campbell