* Radeon regression fix
@ 2011-09-29 14:21 Brad Campbell
2011-09-29 14:36 ` Alex Deucher
0 siblings, 1 reply; 8+ messages in thread
From: Brad Campbell @ 2011-09-29 14:21 UTC (permalink / raw)
To: alexdeucher; +Cc: airlied, dri-devel, linux-kernel
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");
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: Radeon regression fix
2011-09-29 14:21 Radeon regression fix Brad Campbell
@ 2011-09-29 14:36 ` Alex Deucher
2011-09-29 15:10 ` Brad Campbell
2011-09-29 15:21 ` Brad Campbell
0 siblings, 2 replies; 8+ messages in thread
From: Alex Deucher @ 2011-09-29 14:36 UTC (permalink / raw)
To: Brad Campbell; +Cc: airlied, dri-devel, linux-kernel
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");
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Radeon regression fix
2011-09-29 14:36 ` Alex Deucher
@ 2011-09-29 15:10 ` Brad Campbell
2011-09-29 15:21 ` Brad Campbell
1 sibling, 0 replies; 8+ messages in thread
From: Brad Campbell @ 2011-09-29 15:10 UTC (permalink / raw)
To: Alex Deucher; +Cc: airlied, dri-devel, linux-kernel
On 29/09/11 22:36, Alex Deucher wrote:
> 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);
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Radeon regression fix
2011-09-29 14:36 ` Alex Deucher
2011-09-29 15:10 ` Brad Campbell
@ 2011-09-29 15:21 ` Brad Campbell
2011-09-30 0:23 ` Brad Campbell
1 sibling, 1 reply; 8+ messages in thread
From: Brad Campbell @ 2011-09-29 15:21 UTC (permalink / raw)
To: Alex Deucher; +Cc: airlied, dri-devel, linux-kernel
On 29/09/11 22:36, Alex Deucher wrote:
> 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?
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;
}
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: Radeon regression fix
2011-09-29 15:21 ` Brad Campbell
@ 2011-09-30 0:23 ` Brad Campbell
2011-09-30 4:59 ` Alex Deucher
0 siblings, 1 reply; 8+ messages in thread
From: Brad Campbell @ 2011-09-30 0:23 UTC (permalink / raw)
To: Alex Deucher; +Cc: airlied, dri-devel, linux-kernel
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 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?
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;
}
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: Radeon regression fix
2011-09-30 0:23 ` Brad Campbell
@ 2011-09-30 4:59 ` Alex Deucher
2011-09-30 5:14 ` Brad Campbell
0 siblings, 1 reply; 8+ messages in thread
From: Alex Deucher @ 2011-09-30 4:59 UTC (permalink / raw)
To: Brad Campbell; +Cc: airlied, dri-devel, linux-kernel
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 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?
>
> 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;
> }
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Radeon regression fix
2011-09-30 4:59 ` Alex Deucher
@ 2011-09-30 5:14 ` Brad Campbell
2011-09-30 5:39 ` Alex Deucher
0 siblings, 1 reply; 8+ messages in thread
From: Brad Campbell @ 2011-09-30 5:14 UTC (permalink / raw)
To: Alex Deucher; +Cc: airlied, dri-devel, linux-kernel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Radeon regression fix
2011-09-30 5:14 ` Brad Campbell
@ 2011-09-30 5:39 ` Alex Deucher
0 siblings, 0 replies; 8+ messages in thread
From: Alex Deucher @ 2011-09-30 5:39 UTC (permalink / raw)
To: Brad Campbell; +Cc: airlied, dri-devel, linux-kernel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-09-30 5:39 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-29 14:21 Radeon regression fix Brad Campbell
2011-09-29 14:36 ` Alex Deucher
2011-09-29 15:10 ` Brad Campbell
2011-09-29 15:21 ` Brad Campbell
2011-09-30 0:23 ` Brad Campbell
2011-09-30 4:59 ` Alex Deucher
2011-09-30 5:14 ` Brad Campbell
2011-09-30 5:39 ` Alex Deucher
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox